Обсуждение: Recognizing superuser in pg_hba.conf

It can sometimes be useful to match against a superuser in pg_hba.conf.
For example, one could imagine wanting to reject nonsuperuser from a
particular database.


This used to be possible by creating an empty role and matching against
that, but that functionality was removed (a long time ago) by commit
94cd0f1ad8a.


Adding another keyword can break backwards compatibility, of course.  So
that is an issue that needs to be discussed, but I don't imagine too
many people are using role names "superuser" and "nonsuperuser". Those
who are will have to quote them.

-- 

Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> It can sometimes be useful to match against a superuser in pg_hba.conf.

Seems like a reasonable desire.

> Adding another keyword can break backwards compatibility, of course.  So
> that is an issue that needs to be discussed, but I don't imagine too
> many people are using role names "superuser" and "nonsuperuser". Those
> who are will have to quote them.

I'm not very happy about the continuing creep of pseudo-reserved database
and user names in pg_hba.conf.  I wish we'd adjust the notation so that
these keywords are syntactically distinct from ordinary names.  Given
the precedent that "+" and "@" prefixes change what an identifier means,
maybe we could use "*" or some other punctuation character as a keyword
prefix?  We'd have to give grandfather exceptions to the existing
keywords, at least for a while, but we could say that new ones won't be
recognized without the prefix.

            regards, tom lane

On 28/12/2019 19:07, Tom Lane wrote:
> Vik Fearing <vik.fearing@2ndquadrant.com> writes:
>> It can sometimes be useful to match against a superuser in pg_hba.conf.
> Seems like a reasonable desire.
>
>> Adding another keyword can break backwards compatibility, of course.  So
>> that is an issue that needs to be discussed, but I don't imagine too
>> many people are using role names "superuser" and "nonsuperuser". Those
>> who are will have to quote them.
> I'm not very happy about the continuing creep of pseudo-reserved database
> and user names in pg_hba.conf.  I wish we'd adjust the notation so that
> these keywords are syntactically distinct from ordinary names.  Given
> the precedent that "+" and "@" prefixes change what an identifier means,
> maybe we could use "*" or some other punctuation character as a keyword
> prefix?  We'd have to give grandfather exceptions to the existing
> keywords, at least for a while, but we could say that new ones won't be
> recognized without the prefix.


I'm all for this (and even suggested it during the IRC conversation that
prompted this patch). It's rife with bikeshedding, though.  My original
proposal was to use '&' and Andrew Gierth would have used ':'.


I will submit two patches, one that recognizes the sigil for all the
other keywords, and then an update of this patch.

-- 

Vik

On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> > these keywords are syntactically distinct from ordinary names.  Given
> > the precedent that "+" and "@" prefixes change what an identifier means,
> > maybe we could use "*" or some other punctuation character as a keyword
> > prefix?  We'd have to give grandfather exceptions to the existing
> > keywords, at least for a while, but we could say that new ones won't be
> > recognized without the prefix.
>
> I'm all for this (and even suggested it during the IRC conversation that
> prompted this patch). It's rife with bikeshedding, though.  My original
> proposal was to use '&' and Andrew Gierth would have used ':'.

I think this is a good proposal regardless of which character we
decide to use. My order of preference from highest-to-lowest would
probably be :*&, but maybe that's just because I'm reading this on
Sunday rather than on Tuesday.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>> I'm all for this (and even suggested it during the IRC conversation that
>> prompted this patch). It's rife with bikeshedding, though.  My original
>> proposal was to use '&' and Andrew Gierth would have used ':'.

> I think this is a good proposal regardless of which character we
> decide to use. My order of preference from highest-to-lowest would
> probably be :*&, but maybe that's just because I'm reading this on
> Sunday rather than on Tuesday.

I don't have any particular objection to '&' if people prefer that.
But ':' seems like it would introduce confusion with the
variable-substitution notation used in psql and some other places.

It's not that hard to imagine that somebody might want a
variable-substitution notation in pg_hba.conf someday, so we should
leave syntax room for one, and ':' seems like a likely choice
for it (although I suppose a case could be made for '$' too).

            regards, tom lane

On Sun, Dec 29, 2019 at 11:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't have any particular objection to '&' if people prefer that.
> But ':' seems like it would introduce confusion with the
> variable-substitution notation used in psql and some other places.
>
> It's not that hard to imagine that somebody might want a
> variable-substitution notation in pg_hba.conf someday, so we should
> leave syntax room for one, and ':' seems like a likely choice
> for it (although I suppose a case could be made for '$' too).

Well, as I say, I don't care very much... I hope we can agree on
something and move forward.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 29/12/2019 17:31, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>>> I'm all for this (and even suggested it during the IRC conversation that
>>> prompted this patch). It's rife with bikeshedding, though.  My original
>>> proposal was to use '&' and Andrew Gierth would have used ':'.
>> I think this is a good proposal regardless of which character we
>> decide to use. My order of preference from highest-to-lowest would
>> probably be :*&, but maybe that's just because I'm reading this on
>> Sunday rather than on Tuesday.
> I don't have any particular objection to '&' if people prefer that.


I wrote the patch so I got to decide. :-)  I will also volunteer to do
the grunt work of changing the symbol if consensus wants that, though.


It turns out that my original patch didn't really change, all the meat
is in the keywords patch.  The superuser patch is to be applied on top
of the keywords patch.

-- 

Vik Fearing

On 29/12/2019 23:10, Vik Fearing wrote:
> On 29/12/2019 17:31, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>>>> I'm all for this (and even suggested it during the IRC conversation that
>>>> prompted this patch). It's rife with bikeshedding, though.  My original
>>>> proposal was to use '&' and Andrew Gierth would have used ':'.
>>> I think this is a good proposal regardless of which character we
>>> decide to use. My order of preference from highest-to-lowest would
>>> probably be :*&, but maybe that's just because I'm reading this on
>>> Sunday rather than on Tuesday.
>> I don't have any particular objection to '&' if people prefer that.
>
> I wrote the patch so I got to decide. :-)  I will also volunteer to do
> the grunt work of changing the symbol if consensus wants that, though.
>
>
> It turns out that my original patch didn't really change, all the meat
> is in the keywords patch.  The superuser patch is to be applied on top
> of the keywords patch.
>

I missed a few places in the tap tests.  New keywords patch attached,
superuser patch unchanged.

-- 

Vik Fearing

On Mon, Dec 30, 2019 at 11:56:17AM +0100, Vik Fearing wrote:
> On 29/12/2019 23:10, Vik Fearing wrote:
> > On 29/12/2019 17:31, Tom Lane wrote:
> >> Robert Haas <robertmhaas@gmail.com> writes:
> >>> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> >>>> I'm all for this (and even suggested it during the IRC conversation that
> >>>> prompted this patch). It's rife with bikeshedding, though.  My original
> >>>> proposal was to use '&' and Andrew Gierth would have used ':'.
> >>> I think this is a good proposal regardless of which character we
> >>> decide to use. My order of preference from highest-to-lowest would
> >>> probably be :*&, but maybe that's just because I'm reading this on
> >>> Sunday rather than on Tuesday.
> >> I don't have any particular objection to '&' if people prefer that.
> >
> > I wrote the patch so I got to decide. :-)  I will also volunteer to do
> > the grunt work of changing the symbol if consensus wants that, though.
> >
> >
> > It turns out that my original patch didn't really change, all the meat
> > is in the keywords patch.  The superuser patch is to be applied on top
> > of the keywords patch.
> >
> 
> I missed a few places in the tap tests.  New keywords patch attached,
> superuser patch unchanged.
> 
> -- 
> 
> Vik Fearing
> 


Patches apply cleanly to 0ce38730ac72029f3f2c95ae80b44f5b9060cbcc, and
include documentation. They could use an example of the new
capability, possibly included in the sample pg_hba.conf, e.g. 

    host    &all            &superuser      0.0.0.0/0               reject

or similar.

The feature works as described, and is useful. I have thus far been
unable to make it crash.

I haven't used intentionally hostile strings to test it, as I didn't
see those as an important attack surface. This is because by the time
someone hostile can write to pg_hba.conf, they've got all the control
they need to manipulate the entire node, including root exploits.

I've marked this as Ready for Committer.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Greetings,

* Vik Fearing (vik.fearing@2ndquadrant.com) wrote:
> On 29/12/2019 23:10, Vik Fearing wrote:
> > On 29/12/2019 17:31, Tom Lane wrote:
> >> Robert Haas <robertmhaas@gmail.com> writes:
> >>> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> >>>> I'm all for this (and even suggested it during the IRC conversation that
> >>>> prompted this patch). It's rife with bikeshedding, though.  My original
> >>>> proposal was to use '&' and Andrew Gierth would have used ':'.
> >>> I think this is a good proposal regardless of which character we
> >>> decide to use. My order of preference from highest-to-lowest would
> >>> probably be :*&, but maybe that's just because I'm reading this on
> >>> Sunday rather than on Tuesday.
> >> I don't have any particular objection to '&' if people prefer that.
> >
> > I wrote the patch so I got to decide. :-)  I will also volunteer to do
> > the grunt work of changing the symbol if consensus wants that, though.
> >
> > It turns out that my original patch didn't really change, all the meat
> > is in the keywords patch.  The superuser patch is to be applied on top
> > of the keywords patch.
>
> I missed a few places in the tap tests.  New keywords patch attached,
> superuser patch unchanged.

We already have a reserved namespace when it comes to roles,
specifically "pg_"..  why invent something new like this '&' prefix when
we could just declare that 'pg_superusers' is a role to which all
superusers are members?  Or something along those lines?

Thanks,

Stephen

On 02/01/2020 20:52, Stephen Frost wrote:
> Greetings,
>
> * Vik Fearing (vik.fearing@2ndquadrant.com) wrote:
>> On 29/12/2019 23:10, Vik Fearing wrote:
>>> On 29/12/2019 17:31, Tom Lane wrote:
>>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>>> On Sat, Dec 28, 2019 at 2:02 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>>>>>> I'm all for this (and even suggested it during the IRC conversation that
>>>>>> prompted this patch). It's rife with bikeshedding, though.  My original
>>>>>> proposal was to use '&' and Andrew Gierth would have used ':'.
>>>>> I think this is a good proposal regardless of which character we
>>>>> decide to use. My order of preference from highest-to-lowest would
>>>>> probably be :*&, but maybe that's just because I'm reading this on
>>>>> Sunday rather than on Tuesday.
>>>> I don't have any particular objection to '&' if people prefer that.
>>> I wrote the patch so I got to decide. :-)  I will also volunteer to do
>>> the grunt work of changing the symbol if consensus wants that, though.
>>>
>>> It turns out that my original patch didn't really change, all the meat
>>> is in the keywords patch.  The superuser patch is to be applied on top
>>> of the keywords patch.
>> I missed a few places in the tap tests.  New keywords patch attached,
>> superuser patch unchanged.
> We already have a reserved namespace when it comes to roles,
> specifically "pg_"..  why invent something new like this '&' prefix when
> we could just declare that 'pg_superusers' is a role to which all
> superusers are members?  Or something along those lines?


This is an argument against the superusers patch, but surely you are not
suggesting we add a pg_all role that contains all users?  And what about
the keywords that aren't for users?

-- 

Vik Fearing

Stephen Frost <sfrost@snowman.net> writes:
> We already have a reserved namespace when it comes to roles,
> specifically "pg_"..  why invent something new like this '&' prefix when
> we could just declare that 'pg_superusers' is a role to which all
> superusers are members?  Or something along those lines?

Meh.  If the things aren't actually roles, I think this'd just
add confusion.  Or were you proposing to implement them as roles?
I'm not sure if that would be practical in every case.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 > Stephen Frost <sfrost@snowman.net> writes:
 >> We already have a reserved namespace when it comes to roles,
 >> specifically "pg_".. why invent something new like this '&' prefix
 >> when we could just declare that 'pg_superusers' is a role to which
 >> all superusers are members? Or something along those lines?

 Tom> Meh. If the things aren't actually roles, I think this'd just add
 Tom> confusion. Or were you proposing to implement them as roles? I'm
 Tom> not sure if that would be practical in every case.

In fact my original suggestion when this idea was discussed on IRC was
to remove the current superuser flag and turn it into a role; but the
issue then is that role membership is inherited and superuserness
currently isn't, so that's a more intrusive change.

-- 
Andrew (irc:RhodiumToad)

## Stephen Frost (sfrost@snowman.net):

> We already have a reserved namespace when it comes to roles,
> specifically "pg_"..  why invent something new like this '&' prefix when
> we could just declare that 'pg_superusers' is a role to which all
> superusers are members?  Or something along those lines?

Taking this idea one step further (back?): with any non-trivial
number of (user-)roles in the database, DBAs would be well advised
to use group(-role)s for privilege management anyways. It's not
to unreasonable to grant SUPERUSER through a group, too. Although
I'm not sure we'd need a new pg_superuser role here, we're not
inventing a new set of object privileges as in e.g. pg_monitor;
the DBA can just create their own superuser group.
Is there really a need to add more features, or would it be sufficient
to make the applications of group roles more prominent in the docs?
(I've seen way too many cases in which people where granting privileges
to individual users when they should have used groups, so I might
be biased).

Regards,
Christoph

-- 
Spare Space

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> Meh. If the things aren't actually roles, I think this'd just add
>  Tom> confusion. Or were you proposing to implement them as roles? I'm
>  Tom> not sure if that would be practical in every case.

> In fact my original suggestion when this idea was discussed on IRC was
> to remove the current superuser flag and turn it into a role; but the
> issue then is that role membership is inherited and superuserness
> currently isn't, so that's a more intrusive change.

To cover the proposed functionality, you'd still need some way to
select not-superuser.  So I don't think this fully answers the need
even if we wanted to do it.

It's possible that role-ifying everything and then allowing "!role"
in the pg_hba.conf syntax would be enough.  Not sure though.

More generally, allowing inheritance of superuser scares me a bit
from a security standpoint.  I wouldn't mind turning all the other
legacy role properties into grantable roles, but I *like* the fact
that that one is special.

            regards, tom lane

Stephen Frost <sfrost@snowman.net> writes:
> On Thu, Jan 2, 2020 at 15:50 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> To cover the proposed functionality, you'd still need some way to
>> select not-superuser.  So I don't think this fully answers the need
>> even if we wanted to do it.

> Sorry- why do we need that..?  The first match for a pg_hba line wins, so
> you can define all the access methods that superuser accounts are allowed
> to use first, then a “reject” line for superuser accounts, and then
> whatever else you want after that.

Seems kind of awkward.  Or more to the point: you can already do whatever
you want in pg_hba.conf, as long as you're willing to be verbose enough
(and, perhaps, willing to maintain group memberships to fit your needs).
The discussion here, IMO, is about offering useful shorthands.
So a facility like "!role" seems potentially useful.  Maybe it's not
really, but I don't think we should reject it just because there's
a verbose and non-obvious way to get the same result.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > On Thu, Jan 2, 2020 at 15:50 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> To cover the proposed functionality, you'd still need some way to
> >> select not-superuser.  So I don't think this fully answers the need
> >> even if we wanted to do it.
>
> > Sorry- why do we need that..?  The first match for a pg_hba line wins, so
> > you can define all the access methods that superuser accounts are allowed
> > to use first, then a “reject” line for superuser accounts, and then
> > whatever else you want after that.
>
> Seems kind of awkward.  Or more to the point: you can already do whatever
> you want in pg_hba.conf, as long as you're willing to be verbose enough
> (and, perhaps, willing to maintain group memberships to fit your needs).

Sure it's awkward, but it's how people actually deal with these things
today.  I'm not against improving on that situation but I also don't
hear tons of complaints about it either, so I do think we should be
thoughtful when it comes to making changes here.

> The discussion here, IMO, is about offering useful shorthands.

In general, I'm alright with that idea, but I do want to make sure we're
really being thoughtful when it comes to inventing new syntax that will
only work in this one place and will have to be handled specially by any
tools or anything that wants to generate or look at this.

What are we going to have be displayed through pg_hba_file_rules() for
this '!role' or whatever else, in the 'user_name' column?  (Also, ugh,
I find calling that column 'user_name' mildly offensive considering that
function was added well after roles, and it's not like it really meant
'user name' even before then..).

Yes, I'm sure we could just have it be the text '!role' and make
everyone who cares have to parse out that field, in SQL, to figure out
who it really applies to and basically just make everyone deal with it
but I remain skeptical about if it's really a particularly good
approach.

> So a facility like "!role" seems potentially useful.  Maybe it's not
> really, but I don't think we should reject it just because there's
> a verbose and non-obvious way to get the same result.

I don't agree that it's "non-obvious" that if you have a config file
where "first match wins" that things which don't match the first line
are, by definition, NOT whatever that first line was and then fall
through to the next, where you could use 'reject' if you want.  In fact,
I've always kinda figured that's what 'reject' was for, though I'll
admit that it's been around for far longer than I've been involved in
the project (sadly, I hadn't discovered PG yet back in 1998).

Thanks,

Stephen

So it's not clear to me whether we have any meeting of the minds
on wanting this patch.  In the meantime, though, the cfbot
reports that the patch breaks the ssl tests.  Why is that?

            regards, tom lane

On 06/01/2020 17:03, Tom Lane wrote:
> So it's not clear to me whether we have any meeting of the minds
> on wanting this patch.  In the meantime, though, the cfbot
> reports that the patch breaks the ssl tests.  Why is that?


I have no idea.  I cannot reproduce the failure locally.

-- 

Vik Fearing

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> On 06/01/2020 17:03, Tom Lane wrote:
>> So it's not clear to me whether we have any meeting of the minds
>> on wanting this patch.  In the meantime, though, the cfbot
>> reports that the patch breaks the ssl tests.  Why is that?

> I have no idea.  I cannot reproduce the failure locally.

Hm, it blows up pretty thoroughly for me too, on a RHEL6 box.
Are you sure you're running that test -- check-world doesn't do it?

At least in the 001_ssltests test, the failures seem to all look
like this in the TAP test's log file:

psql: error: could not connect to server: could not initiate GSSAPI security context: Unspecified GSS failure.  Minor
codemay provide more information 
could not initiate GSSAPI security context: Credentials cache file '/tmp/krb5cc_502' not found

There are no matching entries in the postmaster log file, so this
seems to be strictly a client-side failure.

(Are we *really* putting security credentials in /tmp ???)

            regards, tom lane

On 2020-01-06 17:03, Tom Lane wrote:
> So it's not clear to me whether we have any meeting of the minds
> on wanting this patch.

This fairly far-ranging syntax reorganization of pg_hba.conf doesn't 
appeal to me.  pg_hba.conf is complicated enough conceptually for users, 
but AFAICT nobody ever complained about the syntax or the lexical 
structure specifically.  Assigning meaning to randomly chosen special 
characters, moreover in a security-relevant file, seems like the wrong 
way to go.

Moreover, this thread has morphed from what it says in the subject line 
to changing the syntax of pg_hba.conf in a somewhat fundamental way.  So 
at the very least someone should post a comprehensive summary of what is 
being proposed, instead of just attaching patches that implement 
whatever was discussed across the thread.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 08/01/2020 23:13, Peter Eisentraut wrote:
> On 2020-01-06 17:03, Tom Lane wrote:
>> So it's not clear to me whether we have any meeting of the minds
>> on wanting this patch.
>
> This fairly far-ranging syntax reorganization of pg_hba.conf doesn't
> appeal to me.  pg_hba.conf is complicated enough conceptually for
> users, but AFAICT nobody ever complained about the syntax or the
> lexical structure specifically.  Assigning meaning to randomly chosen
> special characters, moreover in a security-relevant file, seems like
> the wrong way to go.
>
> Moreover, this thread has morphed from what it says in the subject
> line to changing the syntax of pg_hba.conf in a somewhat fundamental
> way.  So at the very least someone should post a comprehensive summary
> of what is being proposed, instead of just attaching patches that
> implement whatever was discussed across the thread.
>

What is being proposed is what is in the Subject and the original
patch.  The other patch is because Tom didn't like "the continuing creep
of pseudo-reserved database and user names" so I wrote a patch to mark
such reserved names and rebased my original patch on top of it.  Only
the docs changed in the rebase.  The original patch (or its rebase) is
what I am interested in.

-- 

Vik Fearing

Simon Riggs <simon@2ndquadrant.com> writes:
> On Wed, 8 Jan 2020 at 23:55, Vik Fearing <vik.fearing@2ndquadrant.com>
> wrote:
>> What is being proposed is what is in the Subject and the original
>> patch.  The other patch is because Tom didn't like "the continuing creep
>> of pseudo-reserved database and user names" so I wrote a patch to mark
>> such reserved names and rebased my original patch on top of it.  Only
>> the docs changed in the rebase.  The original patch (or its rebase) is
>> what I am interested in.

> Hopefully there will be no danger of me gaining access if I have a crafted
> rolename?
> postgres=# create role "&backdoor";
> CREATE ROLE

Well, the existence of such a role name wouldn't by itself cause any
change in the way that pg_hba.conf is parsed.  If you could then
persuade a superuser to insert a pg_hba.conf line that is trying
to match your username, the line might do something else than what the
superuser expected, which is bad.  But the *exact* same hazard applies
to proposals based on inventing pseudo-reserved keywords (by which
I mean things that look like names, but aren't reserved words, so that
somebody could create a role name matching them).  Either way, an
uninformed superuser could be tricked.

What I'm basically objecting to is the pseudo-reservedness.  If we
don't want to dodge that with special syntax, we should dodge it
by making sure the keywords are actually reserved names.  In other
words, add a "pg_" prefix, as somebody else suggested upthread.
I don't personally find that prettier than a punctuation prefix,
but I can live with it if other people do.

BTW, although that solution works for the immediate need of
keywords that have to be distinguished from role names, it doesn't
currently scale to keywords for the database column, because we
don't treat "pg_" as a reserved prefix for database names:

regression=# create role pg_zit;
ERROR:  role name "pg_zit" is reserved
DETAIL:  Role names starting with "pg_" are reserved.
regression=# create database pg_zit;
CREATE DATABASE

Should we do so, or wait till there's an immediate need to?

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> What I'm basically objecting to is the pseudo-reservedness.  If we
> don't want to dodge that with special syntax, we should dodge it
> by making sure the keywords are actually reserved names.  In other
> words, add a "pg_" prefix, as somebody else suggested upthread.

Yes, that was my suggestion, and it was also my change a few major
versions ago that actually reserved the "pg_" prefix for roles.

> BTW, although that solution works for the immediate need of
> keywords that have to be distinguished from role names, it doesn't
> currently scale to keywords for the database column, because we
> don't treat "pg_" as a reserved prefix for database names:
>
> regression=# create role pg_zit;
> ERROR:  role name "pg_zit" is reserved
> DETAIL:  Role names starting with "pg_" are reserved.
> regression=# create database pg_zit;
> CREATE DATABASE
>
> Should we do so, or wait till there's an immediate need to?

I seem to recall (but it was years ago, so I might be wrong) advocating
that we should reserve the 'pg_' prefix for *all* object types.  All I
can recall is that there wasn't much backing for the idea (though I also
don't recall any specific objection, and it's also quite possible that
there was simply no response to the idea).

For my 2c, I'd *much* rather we reserve it across the board, and sooner
than later, since that would hopefully reduce the impact on people.  The
only justification for *not* reserving it is if we KNOW that we'll never
need a special one of those, but, well, we're well past that for
database names already- look at the fact that we've got a "replication"
one, for example.  Maybe we can't ever un-reserve that, but I like the
idea of reserving "pg_" for database names and then having
"pg_replication" be allowed to mean replication connections and then
encouraging users to use that instead.

Thanks,

Stephen

On Thu, Jan 9, 2020 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What I'm basically objecting to is the pseudo-reservedness.  If we
> don't want to dodge that with special syntax, we should dodge it
> by making sure the keywords are actually reserved names.

You know, as I was reading this email, I got to thinking: aren't we
engineering a solution to a problem for which we already have a
solution?

The documentation says:

"Quoting one of the keywords in a database, user, or address field
(e.g., all or replication) makes the word lose its special character,
and just match a database, user, or host with that name."

So if you've writing a pg_hba.conf file that contains a specific role
name, and you want to make sure it doesn't get confused with a current
or future keyword, just quote it. If you don't quote it, make sure to
RTFM at the time and when upgrading.

If you want to argue that this isn't the cleanest possible solution to
the problem, I think I would agree. If we were doing this over again,
we could probably design a better syntax for pg_hba.conf, perhaps one
where all specific role names have to be quoted and anything that's
not quoted is expected to be a keyword. But, as it is, nothing's
really broken here, and practical confusion is unlikely. If someone
has a role named "superuser", then it's probably a superuser account,
and the worst that will happen is that we'll match all superuser
accounts rather than only that one. If someone has a non-superuser
account called "superuser", or if they have an account named
"nonsuperuser," then, uh, that's lame, and if this patch causes them
to improve their choice of role names, that's good. If it causes them
to use quotes, that's also good.

But I think I'm coming around to the view that we're making what ought
to be a simple change complicated, and we should just not do that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jan 9, 2020 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm basically objecting to is the pseudo-reservedness.  If we
>> don't want to dodge that with special syntax, we should dodge it
>> by making sure the keywords are actually reserved names.

> ...
> But I think I'm coming around to the view that we're making what ought
> to be a simple change complicated, and we should just not do that.

The problem is that we keep deciding that okay, it probably won't hurt
anybody if this particular thing-that-ought-to-be-a-reserved-word isn't
really reserved.  Your exercise in justifying that for "superuser" is
not unlike every other previous argument about this.  Sooner or later
that's going to fail, and somebody's going to have a security problem
because they didn't know that a particular name has magic properties
in a particular context.  (Which, indeed, maybe it didn't have when
they chose it.)  Claiming they should have known better isn't where
I want to be when that happens.

I don't want to keep going down that path.  These things are effectively
reserved words, and they need to act like reserved words, so that you
get an error if you misuse them.  Silently doing something else than
what (one reasonable reading of) a pg_hba.conf entry seems to imply
is *bad*.

            regards, tom lane

On Thu, Jan 9, 2020 at 11:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The problem is that we keep deciding that okay, it probably won't hurt
> anybody if this particular thing-that-ought-to-be-a-reserved-word isn't
> really reserved.  Your exercise in justifying that for "superuser" is
> not unlike every other previous argument about this.  Sooner or later
> that's going to fail, and somebody's going to have a security problem
> because they didn't know that a particular name has magic properties
> in a particular context.  (Which, indeed, maybe it didn't have when
> they chose it.)  Claiming they should have known better isn't where
> I want to be when that happens.

But, again, we already *have* a way of solving this problem: use
quotes. As Simon pointed out, your proposed solution isn't really a
solution at all, because & can appear in role names. It probably
won't, but there probably also won't be a role name that matches
either of these keywords, so it's just six of one, half a dozen of the
other. The thing that really solves it is quoting.

Now I admit that if we decide pg_hba.conf keywords have to start with
"pg_" and prevent names beginning with "pg_" from being used as object
names, then we'd have TWO ways of distinguishing between a keyword and
an object name. But I don't think TMTOWTDI is the right design
principle here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jan 9, 2020 at 11:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The problem is that we keep deciding that okay, it probably won't hurt
>> anybody if this particular thing-that-ought-to-be-a-reserved-word isn't
>> really reserved.

> But, again, we already *have* a way of solving this problem: use
> quotes. As Simon pointed out, your proposed solution isn't really a
> solution at all, because & can appear in role names.

I'm not sure that the pg_hba.conf parser allows that without quotes,
but in any case it's irrelevant to the proposal to use a pg_ prefix.
We don't allow non-built-in role names to be spelled that way,
quoted or not.

> Now I admit that if we decide pg_hba.conf keywords have to start with
> "pg_" and prevent names beginning with "pg_" from being used as object
> names, then we'd have TWO ways of distinguishing between a keyword and
> an object name. But I don't think TMTOWTDI is the right design
> principle here.

The principle I'm concerned about is not letting a configuration file
that was perfectly fine in version N silently become a security hazard
in version N+1.  The only way I will accept your proposal is if we
change the pg_hba.conf parser to *require* quotes around every
role and DB name that's not meant to be a keyword, so that people get
used to that requirement.  But I doubt that idea will fly.

            regards, tom lane

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> But, again, we already *have* a way of solving this problem: use
> quotes. As Simon pointed out, your proposed solution isn't really a
> solution at all, because & can appear in role names. It probably
> won't, but there probably also won't be a role name that matches
> either of these keywords, so it's just six of one, half a dozen of the
> other. The thing that really solves it is quoting.

I really just can't agree with the idea that:

"&superuser"

and

&superuser

in pg_hba.conf should mean materially different things and have far
reaching security differences.  Depending on quoting in pg_hba.conf for
this distinction is an altogether bad idea.

> Now I admit that if we decide pg_hba.conf keywords have to start with
> "pg_" and prevent names beginning with "pg_" from being used as object
> names, then we'd have TWO ways of distinguishing between a keyword and
> an object name. But I don't think TMTOWTDI is the right design
> principle here.

There is a *really* big difference here though which makes this not "two
ways to do the same thing"- you *can't* create a user starting with
"pg_".  You *can* create a user with an '&' in it.  If we prevented you
from being able to create users with '&' in it then I'd be more open to
the idea of using '&' to mean something special in pg_hba, and then it
really would be two different ways to do the same thing, but that's not
actually what's being proposed here.

Thanks,

Stephen

Hi,

I see this patch is marked as RFC since 12/30, but there seems to be
quite a lot of discussion about the syntax, keywords and how exactly to
identify the superuser. So I'll switch it back to needs review, which I
think is a better representation of the current state.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I see this patch is marked as RFC since 12/30, but there seems to be
> quite a lot of discussion about the syntax, keywords and how exactly to
> identify the superuser. So I'll switch it back to needs review, which I
> think is a better representation of the current state.

Somebody switched it to RFC again, despite the facts that

(a) there is absolutely no consensus about what syntax to use
(and some of the proposals imply very different patches),

(b) there's been no discussion at all since the last CF, and

(c) the patch is still failing in the cfbot (src/test/ssl fails).

While resolving (c) would seem to be the author's problem, I don't
think it's worth putting effort into that detail until we have
some meeting of the minds about (a).  So I'll put this back to
"needs review".

            regards, tom lane

> On 30 Mar 2020, at 20:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> I see this patch is marked as RFC since 12/30, but there seems to be
>> quite a lot of discussion about the syntax, keywords and how exactly to
>> identify the superuser. So I'll switch it back to needs review, which I
>> think is a better representation of the current state.
>
> Somebody switched it to RFC again, despite the facts that
>
> (a) there is absolutely no consensus about what syntax to use
> (and some of the proposals imply very different patches),
>
> (b) there's been no discussion at all since the last CF, and
>
> (c) the patch is still failing in the cfbot (src/test/ssl fails).
>
> While resolving (c) would seem to be the author's problem, I don't
> think it's worth putting effort into that detail until we have
> some meeting of the minds about (a).  So I'll put this back to
> "needs review".

Since there hasn't been any more progress on this since the last CF, and the
fact that the outcome may result in a completely different patch, I'm inclined
to mark this returned with feedback rather than have it linger.  The discussion
can continue and the entry be re-opened.

Thoughts?

cheers ./daniel

On 7/2/20 3:14 PM, Daniel Gustafsson wrote:
>> On 30 Mar 2020, at 20:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>>> I see this patch is marked as RFC since 12/30, but there seems to be
>>> quite a lot of discussion about the syntax, keywords and how exactly to
>>> identify the superuser. So I'll switch it back to needs review, which I
>>> think is a better representation of the current state.
>>
>> Somebody switched it to RFC again, despite the facts that
>>
>> (a) there is absolutely no consensus about what syntax to use
>> (and some of the proposals imply very different patches),
>>
>> (b) there's been no discussion at all since the last CF, and
>>
>> (c) the patch is still failing in the cfbot (src/test/ssl fails).
>>
>> While resolving (c) would seem to be the author's problem, I don't
>> think it's worth putting effort into that detail until we have
>> some meeting of the minds about (a).  So I'll put this back to
>> "needs review".
> 
> Since there hasn't been any more progress on this since the last CF, and the
> fact that the outcome may result in a completely different patch, I'm inclined
> to mark this returned with feedback rather than have it linger.  The discussion
> can continue and the entry be re-opened.
> 
> Thoughts?


No objection.
-- 
Vik Fearing