Обсуждение: allow specifying direct role membership in pg_hba.conf

Hi hackers,

I've attached a small patch that allows specifying only direct members
of a group in pg_hba.conf.  The "+" prefix offered today matches both
direct and indirect role members, which may complicate some role
setups.  For example, if you have one set of roles that are members of
the "pam" role and another set that are members of the "scram-sha-256"
role, granting membership in a PAM role to a SCRAM role might
inadvertently modify the desired authentication method for the
grantee.  If only direct membership is considered, no such inadvertent
authentication method change would occur.

I chose "&" as a new group name prefix for this purpose.  This choice
seemed as good as any, but I'm open to changing it if anyone has
suggestions.  For determining direct role membership, I added a new
function in acl.c that matches other related functions.  I added a new
role cache type since it seemed to fit in reasonably well, but it seems
unlikely that there is any real performance benefit versus simply
open-coding the syscache lookup.

I didn't see any existing authentication tests for groups at first
glance.  If folks are interested in this functionality, I can work on
adding some tests for this stuff.

Nathan

On 5/13/21 7:38 PM, Bossart, Nathan wrote:
> Hi hackers,
>
> I've attached a small patch that allows specifying only direct members
> of a group in pg_hba.conf.  The "+" prefix offered today matches both
> direct and indirect role members, which may complicate some role
> setups.  For example, if you have one set of roles that are members of
> the "pam" role and another set that are members of the "scram-sha-256"
> role, granting membership in a PAM role to a SCRAM role might
> inadvertently modify the desired authentication method for the
> grantee.  If only direct membership is considered, no such inadvertent
> authentication method change would occur.
>
> I chose "&" as a new group name prefix for this purpose.  This choice
> seemed as good as any, but I'm open to changing it if anyone has
> suggestions.  For determining direct role membership, I added a new
> function in acl.c that matches other related functions.  I added a new
> role cache type since it seemed to fit in reasonably well, but it seems
> unlikely that there is any real performance benefit versus simply
> open-coding the syscache lookup.
>
> I didn't see any existing authentication tests for groups at first
> glance.  If folks are interested in this functionality, I can work on
> adding some tests for this stuff.
>

Do we really want to be creating two classes of role membership?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 5/13/21 7:38 PM, Bossart, Nathan wrote:
>> I've attached a small patch that allows specifying only direct members
>> of a group in pg_hba.conf.

> Do we really want to be creating two classes of role membership?

Yeah, this seems to be going against the clear meaning of the
SQL spec.  I realize you can argue that pg_hba.conf doesn't have
to follow the spec, but it doesn't seem like a terribly good idea
to interpret role membership differently in different places.

            regards, tom lane

On 05/13/21 19:38, Bossart, Nathan wrote:
> I chose "&" as a new group name prefix for this purpose.  This choice

If pg_hba syntax changes are being entertained, I would love to be able
to set ssl_min_protocol_version locally in a hostssl rule.

Some clients at $work are stuck with ancient SSL libraries, but I would
much rather be able to weaken ssl_min_protocol_version just for them
than do it globally.

Regards,
-Chap

Greetings,

* Chapman Flack (chap@anastigmatix.net) wrote:
> If pg_hba syntax changes are being entertained, I would love to be able
> to set ssl_min_protocol_version locally in a hostssl rule.
>
> Some clients at $work are stuck with ancient SSL libraries, but I would
> much rather be able to weaken ssl_min_protocol_version just for them
> than do it globally.

This (unlike what was actually proposed) does seem like it'd be a useful
improvement.  Not sure exaclty how it would work but I'm generally on
board with the idea.

Thanks,

Stephen

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 5/13/21 7:38 PM, Bossart, Nathan wrote:
> >> I've attached a small patch that allows specifying only direct members
> >> of a group in pg_hba.conf.
>
> > Do we really want to be creating two classes of role membership?
>
> Yeah, this seems to be going against the clear meaning of the
> SQL spec.  I realize you can argue that pg_hba.conf doesn't have
> to follow the spec, but it doesn't seem like a terribly good idea
> to interpret role membership differently in different places.

Agreed.

The lack of any particular justifcation for wanting this isn't a useful
way to propose a patch either.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Chapman Flack (chap@anastigmatix.net) wrote:
>> If pg_hba syntax changes are being entertained, I would love to be able
>> to set ssl_min_protocol_version locally in a hostssl rule.
>> Some clients at $work are stuck with ancient SSL libraries, but I would
>> much rather be able to weaken ssl_min_protocol_version just for them
>> than do it globally.

> This (unlike what was actually proposed) does seem like it'd be a useful
> improvement.  Not sure exaclty how it would work but I'm generally on
> board with the idea.

Seems like putting GUCs directly into pg_hba would be a mess.  Would
it be enough to tell people to use ALTER ROLE/DATABASE SET for this,
and then fix things so that we recheck the protocol version (and
possibly bail out) after absorbing those settings?

I can think of objections to this:

* If you actually want to tie the restriction to source IP addresses,
rather than users or databases, this doesn't get the job done.

* The authentication cycle would be completed (or at least mostly
so) before we bail out; so if the concern is about packet-sniffing
or MITM attacks, maybe this would expose too much.

But it does have the advantage of being something it seems like
we could get done easily.

            regards, tom lane

On Fri, May 14, 2021 at 8:58 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Chapman Flack (chap@anastigmatix.net) wrote:
> > If pg_hba syntax changes are being entertained, I would love to be able
> > to set ssl_min_protocol_version locally in a hostssl rule.
> >
> > Some clients at $work are stuck with ancient SSL libraries, but I would
> > much rather be able to weaken ssl_min_protocol_version just for them
> > than do it globally.
>
> This (unlike what was actually proposed) does seem like it'd be a useful
> improvement.  Not sure exaclty how it would work but I'm generally on
> board with the idea.

I agree, but I have no idea how you could do that within the current
pg_hba.conf.

The row is selected by the combination of username/database/ipaddress.
But you have to pick the minimum TLS version before the client has
sent that... Basically we have to make the choice long before we've
even started looking at pg_hba.

It would be good to have a way to do it, but I'm not sure pg_hba.conf
is the place for it.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 05/17/21 16:15, Magnus Hagander wrote:
> The row is selected by the combination of username/database/ipaddress.
> But you have to pick the minimum TLS version before the client has
> sent that... Basically we have to make the choice long before we've
> even started looking at pg_hba.

Use the peer IP address to pre-filter the available pg_hba entries to
those pertaining to that address ... choose a min protocol version that's
the min specified among those ... then get the username and database name
(by which point a protocol has been negotiated), then further filter the
list down to those pertaining to that user and database and allowing that
protocol version?

Yes, clunky, but avoids a more ambitious redesign of pg_hba.

I'm not sure a more ambitious redesign would be a bad thing in principle;
the pg_hba.conf syntax seems rather clunky and limiting to begin with,
and I keep wondering why it isn't in shared tables or something. But
I suppose a lot of external admin tools have some knowledge of it?

Regards,
-Chap

On Mon, May 17, 2021 at 10:31 PM Chapman Flack <chap@anastigmatix.net> wrote:
>
> On 05/17/21 16:15, Magnus Hagander wrote:
> > The row is selected by the combination of username/database/ipaddress.
> > But you have to pick the minimum TLS version before the client has
> > sent that... Basically we have to make the choice long before we've
> > even started looking at pg_hba.
>
> Use the peer IP address to pre-filter the available pg_hba entries to
> those pertaining to that address ... choose a min protocol version that's
> the min specified among those ... then get the username and database name
> (by which point a protocol has been negotiated), then further filter the
> list down to those pertaining to that user and database and allowing that
> protocol version?
>
> Yes, clunky, but avoids a more ambitious redesign of pg_hba.

So you're saying that some entries int he parameter section would
depend on the db/user/ip combo and some would depend just on the ip?

That seems like an absolutely terrible idea to me, especially since
this is about security configuration. Way too easy to get wrong by
people who don't know how the internals work. People will *definitely*
set those parameter thinking that they can do it based on the db and
user as well.


> I'm not sure a more ambitious redesign would be a bad thing in principle;
> the pg_hba.conf syntax seems rather clunky and limiting to begin with,
> and I keep wondering why it isn't in shared tables or something. But
> I suppose a lot of external admin tools have some knowledge of it?

I think we'd either need a redesign of that, or a completely different
way of configuring pre-authentication settings.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 05/17/21 16:35, Magnus Hagander wrote:
> So you're saying that some entries int he parameter section would
> depend on the db/user/ip combo and some would depend just on the ip?

I don't *think* that's what I was saying. What I was thinking was this:

The pg_hba.conf file is an ordered list of entries. Each entry can specify
a (broad or narrow) set of IPs it applies to, a (broad or narrow) set of
databases it applies to, and a (broad or narrow) set of users it applies to.

Also, in this hypothetical, it can specify a min protocol version.

Right now, we're doing something like this:

1. accept an incoming connection, learning the client IP
2. SSLRequest message leads to negotiating TLS
3. StartupMessage supplies the desired database and user name
4. pg_hba entries are consulted once and filtered down to the first one
applicable to the client IP, database, and username (and SSLness)
5. that entry is used for authentication


I suggested only:

Insert step 1½, filter the pg_hba entries down to only those that could
possibly accept a connection from this IP address. This is an improper
subset of the whole list, and an improper superset of the singleton to be
generated later in step 4.

Step 2 still negotiates TLS, but can fail early if the protocol would
be older than the oldest allowed in the pre-filtered list.

Step 4 takes that pre-filtered list and completes the restriction down to
first entry matching the IP, database, and username. This should be the
same singleton it would generate now. But it can fail-fast if that entry
would require a higher protocol version than what's been negotiated,
before sending the corresponding authentication request message, so no
authentication data will be exchanged over a less-secure channel than
intended. However, the user, database name, and options in the Startup
message might have been exposed over a lower TLS version than intended.
Maybe that's not the end of the world?

Regards,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> On 05/17/21 16:35, Magnus Hagander wrote:
>> So you're saying that some entries int he parameter section would
>> depend on the db/user/ip combo and some would depend just on the ip?

> I don't *think* that's what I was saying. What I was thinking was this:
> ...

This seems pretty horrid to me, not only from a complexity standpoint,
but because it would break the principle that pg_hba.conf entries are
applied in order.

On the whole, I'm afraid that this idea is going to create a lot
more problems than it solves.

            regards, tom lane

On 05/17/21 17:55, Tom Lane wrote:
> This seems pretty horrid to me, not only from a complexity standpoint,
> but because it would break the principle that pg_hba.conf entries are
> applied in order.

This makes twice in a row that I've failed to see how.

If you go through the entries, in order, and simply prune from the list
the ones you can already prove would never apply to this connection, how
does that break the ordering principle?

Regards,
-Chap

On 05/17/21 21:19, Chapman Flack wrote:
> This makes twice in a row that I've failed to see how.
> 
> If you go through the entries, in order, and simply prune from the list
> the ones you can already prove would never apply to this connection, how
> does that break the ordering principle?


Ok, I see how what I proposed looks out-of-order just in that it lets the
initial TLS negotiation be influenced by the minimum version over all
potentially-applicable entries.

But that's just an optimization anyway. The same ultimate effect would be
achieved by unconditionally allowing anything back to TLSv1 to be negotiated
at SSLRequest time, and then (processing the entries in order as always)
rejecting the connection if the first one that could apply expects a higher
protocol version.

The pre-scan and use of the minimum version encountered has only the effect
of fast-failing a TLS negotiation for a version that won't possibly succeed.

Regards,
-Chap

On Mon, May 17, 2021 at 11:18 PM Chapman Flack <chap@anastigmatix.net> wrote:
>
> On 05/17/21 16:35, Magnus Hagander wrote:
> > So you're saying that some entries int he parameter section would
> > depend on the db/user/ip combo and some would depend just on the ip?
>
> I don't *think* that's what I was saying. What I was thinking was this:
>
> The pg_hba.conf file is an ordered list of entries. Each entry can specify
> a (broad or narrow) set of IPs it applies to, a (broad or narrow) set of
> databases it applies to, and a (broad or narrow) set of users it applies to.
>
> Also, in this hypothetical, it can specify a min protocol version.
>
> Right now, we're doing something like this:
>
> 1. accept an incoming connection, learning the client IP
> 2. SSLRequest message leads to negotiating TLS
> 3. StartupMessage supplies the desired database and user name
> 4. pg_hba entries are consulted once and filtered down to the first one
> applicable to the client IP, database, and username (and SSLness)
> 5. that entry is used for authentication
>
>
> I suggested only:
>
> Insert step 1½, filter the pg_hba entries down to only those that could
> possibly accept a connection from this IP address. This is an improper
> subset of the whole list, and an improper superset of the singleton to be
> generated later in step 4.
>
> Step 2 still negotiates TLS, but can fail early if the protocol would
> be older than the oldest allowed in the pre-filtered list.

Nop, this is *exactly* what I'm referring to as being a bad idea.

Step 1 1/2 in this *ignores* the fact that you may have specified a
restriction on username and database name in pg_hba.conf, because it
hasn't seen them yet. Thus,  a parameter such as min_tls_version would
not respect the username/databasename field, whereas other parameters
would. That is a massive risk of misconfiguration.

I mean, if you have
hostssl somedatabase someuser 10.0.0.0/24 gss
hostssl somedatabase supseruser 10.0.0.0/24 gss tls_min_version=1.3

One would reasonably expect that "someuser" can connect with whatever
the default version i for tls_min_versino, whereas "superuser" would
require a minimum of 1.3. But that's *not* what would happen --
superuser would also be allowed to connect with a lower version if
that's allowed in the global set.


> Step 4 takes that pre-filtered list and completes the restriction down to
> first entry matching the IP, database, and username. This should be the
> same singleton it would generate now. But it can fail-fast if that entry
> would require a higher protocol version than what's been negotiated,
> before sending the corresponding authentication request message, so no
> authentication data will be exchanged over a less-secure channel than
> intended. However, the user, database name, and options in the Startup
> message might have been exposed over a lower TLS version than intended.
> Maybe that's not the end of the world?

That is exactly the problem. And while that may hold true of current
auth methods, it may not hold true of all. And it could still trigger
things like an ident callback if that is allowed etc.

So I stand by thinking this is the wrong place to solve the problem. I
agree it would be good to be able to do it, but I don't agree on
overloading it on pg_hba.conf, which is complicated enough already.
And for security config, simplicity is pretty much always better.

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 05/18/21 04:54, Magnus Hagander wrote:

> I mean, if you have
> hostssl somedatabase someuser 10.0.0.0/24 gss
> hostssl somedatabase supseruser 10.0.0.0/24 gss tls_min_version=1.3
> 
> One would reasonably expect that "someuser" can connect with whatever
> the default version i for tls_min_versino, whereas "superuser" would
> require a minimum of 1.3. But that's *not* what would happen --
> superuser would also be allowed to connect with a lower version if
> that's allowed in the global set.

Negatory. "superuser" would be allowed to send a StartupMessage
containing the strings "somedatabase" and "superuser" (and possibly
some settings of options) over a lower version if that's allowed
in the global set ... and would then have the connection rejected
because the negotiated protocol was lower than 1.3, without seeing
any authentication message or having a chance to send any sensitive
authentication credentials.

So the risk of any information exposure over a too-low TLS version
is limited to the name of a database, the name of a user, and possibly
the settings of some options, and no sensitive authentication data.

Regards,
-Chap

On 5/18/21 8:05 AM, Chapman Flack wrote:
> On 05/18/21 04:54, Magnus Hagander wrote:
>
>> I mean, if you have
>> hostssl somedatabase someuser 10.0.0.0/24 gss
>> hostssl somedatabase supseruser 10.0.0.0/24 gss tls_min_version=1.3
>>
>> One would reasonably expect that "someuser" can connect with whatever
>> the default version i for tls_min_versino, whereas "superuser" would
>> require a minimum of 1.3. But that's *not* what would happen --
>> superuser would also be allowed to connect with a lower version if
>> that's allowed in the global set.
> Negatory. "superuser" would be allowed to send a StartupMessage
> containing the strings "somedatabase" and "superuser" (and possibly
> some settings of options) over a lower version if that's allowed
> in the global set ... and would then have the connection rejected
> because the negotiated protocol was lower than 1.3, without seeing
> any authentication message or having a chance to send any sensitive
> authentication credentials.
>
> So the risk of any information exposure over a too-low TLS version
> is limited to the name of a database, the name of a user, and possibly
> the settings of some options, and no sensitive authentication data.
>


We are way off $subject. If we want to continue this discussion please
use an appropriate subject.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com