Thread: use has_privs_of_role() for pg_hba.conf
Hi hackers, 6198420 ensured that has_privs_of_role() is used for predefined roles, which means that the role inheritance hierarchy is checked instead of mere role membership. However, inheritance is still not respected for pg_hba.conf. Specifically, "samerole", "samegroup", and "+" still use is_member_of_role_nosuper(). The attached patch introduces has_privs_of_role_nosuper() and uses it for the aforementioned pg_hba.conf functionality. I think this is desirable for consistency. If a role_a has membership in role_b but none of its privileges (i.e., NOINHERIT), does it make sense that role_a should match +role_b in pg_hba.conf? It is true that role_a could always "SET ROLE role_b", and with this change, the user won't even have the ability to log in to run SET ROLE. But I'm not sure if that's a strong enough argument for deviating from the standard role privilege checks. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Apr 1, 2022 at 6:06 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > > Hi hackers, > > 6198420 ensured that has_privs_of_role() is used for predefined roles, > which means that the role inheritance hierarchy is checked instead of mere > role membership. However, inheritance is still not respected for > pg_hba.conf. Specifically, "samerole", "samegroup", and "+" still use > is_member_of_role_nosuper(). > > The attached patch introduces has_privs_of_role_nosuper() and uses it for > the aforementioned pg_hba.conf functionality. I think this is desirable > for consistency. If a role_a has membership in role_b but none of its > privileges (i.e., NOINHERIT), does it make sense that role_a should match > +role_b in pg_hba.conf? It is true that role_a could always "SET ROLE > role_b", and with this change, the user won't even have the ability to log > in to run SET ROLE. But I'm not sure if that's a strong enough argument > for deviating from the standard role privilege checks. > > Thoughts? > Good catch, I think this is a logical followup to the previous has_privs_of_role patch. Reviewed and +1
On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote:
> Good catch, I think this is a logical followup to the previous
> has_privs_of_role patch.
>
> Reviewed and +1
Thanks! I created a commitfest entry for this:
https://commitfest.postgresql.org/38/3609/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Apr 04, 2022 at 07:25:51AM -0700, Nathan Bossart wrote: > On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote: >> Good catch, I think this is a logical followup to the previous >> has_privs_of_role patch. >> >> Reviewed and +1 > > Thanks! I created a commitfest entry for this: This patch looks simple, but it is a very sensitive area so I think that we should be really careful. pg_hba.conf does not have a lot of test coverage, so I'd really prefer if we add something to see the difference of behavior and check the behavior that we are switching here. What I have just committed in 051b096 would help a bit here, actually, and changing pg_hba.conf rules with rule reload is cheap. Joe, you are registered as a reviewer and committer of this patch, by the way. Are you planning to look at it? -- Michael
Attachment
On 10/6/22 04:09, Michael Paquier wrote: > On Mon, Apr 04, 2022 at 07:25:51AM -0700, Nathan Bossart wrote: >> On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote: >>> Good catch, I think this is a logical followup to the previous >>> has_privs_of_role patch. >>> >>> Reviewed and +1 >> >> Thanks! I created a commitfest entry for this: > > This patch looks simple, but it is a very sensitive area so I think > that we should be really careful. pg_hba.conf does not have a lot of > test coverage, so I'd really prefer if we add something to see the > difference of behavior and check the behavior that we are switching > here. Agreed > Joe, you are registered as a reviewer and committer of this patch, by > the way. Are you planning to look at it? I am meaning to get to it, but as you say wanted to spend some time to understand the nuances and life keeps getting in the way. I will try to prioritize it over the next week. Joe -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Oct 06, 2022 at 07:33:46AM -0400, Joe Conway wrote: > On 10/6/22 04:09, Michael Paquier wrote: >> This patch looks simple, but it is a very sensitive area so I think >> that we should be really careful. pg_hba.conf does not have a lot of >> test coverage, so I'd really prefer if we add something to see the >> difference of behavior and check the behavior that we are switching >> here. > > Agreed Here is a new version of the patch with a test. >> Joe, you are registered as a reviewer and committer of this patch, by >> the way. Are you planning to look at it? > > I am meaning to get to it, but as you say wanted to spend some time to > understand the nuances and life keeps getting in the way. I will try to > prioritize it over the next week. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Oct 06, 2022 at 10:43:43AM -0700, Nathan Bossart wrote:
> Here is a new version of the patch with a test.
Thanks, that helps a lot. Now I grab the difference even if your
previous patch was already switching the documentation to tell exactly
that. On the ground of 6198420, it looks indeed strange to not do the
same for pg_hba.conf. That makes the whole story more consistent, for
one.
+$node->safe_psql('postgres', "CREATE DATABASE role1;");
+$node->safe_psql('postgres', "CREATE ROLE role1 LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role2 LOGIN SUPERUSER INHERIT IN ROLE role1 PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role3 LOGIN SUPERUSER NOINHERIT IN ROLE role1 PASSWORD 'pass';");
So this comes down to role3, where HEAD allows a connection as long as
it is a member of role1 for +role1, samegroup and samerole, but the
patch would prevent the connection when role3 does not inherit the
permissions of role1, even if it is a superuser.
samegroup is a synonym of samerole, but fine by me to keep the full
coverage and all three sets.
Rather than putting that in a separate script, which means
initializing a new node, etc. could it be better to put that in
001_password.pl instead? It would be cheaper.
--
Michael
Attachment
On Fri, Oct 07, 2022 at 11:06:47AM +0900, Michael Paquier wrote: > Rather than putting that in a separate script, which means > initializing a new node, etc. could it be better to put that in > 001_password.pl instead? It would be cheaper. Works for me. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Oct 06, 2022 at 08:27:11PM -0700, Nathan Bossart wrote:
> On Fri, Oct 07, 2022 at 11:06:47AM +0900, Michael Paquier wrote:
>> Rather than putting that in a separate script, which means
>> initializing a new node, etc. could it be better to put that in
>> 001_password.pl instead? It would be cheaper.
>
> Works for me.
Thanks. I would perhaps use names less generic than role{1,2,3} for
the roles or "role1" for the database name, but the logic looks
sound.
--
Michael
Attachment
On Fri, Apr 1, 2022 at 6:07 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > > 6198420 ensured that has_privs_of_role() is used for predefined roles, > which means that the role inheritance hierarchy is checked instead of mere > role membership. However, inheritance is still not respected for > pg_hba.conf. Specifically, "samerole", "samegroup", and "+" still use > is_member_of_role_nosuper(). > > The attached patch introduces has_privs_of_role_nosuper() and uses it for > the aforementioned pg_hba.conf functionality. I think this is desirable > for consistency. If a role_a has membership in role_b but none of its > privileges (i.e., NOINHERIT), does it make sense that role_a should match > +role_b in pg_hba.conf? It is true that role_a could always "SET ROLE > role_b", and with this change, the user won't even have the ability to log > in to run SET ROLE. But I'm not sure if that's a strong enough argument > for deviating from the standard role privilege checks. I hadn't noticed this thread before. I'm not sure whether this is properly considered a privilege check. It could even be an anti-privilege, if the pg_hba.conf line in question is maked "reject". I'm not taking the position that what this patch does is wrong, but I *am* taking the position that it's a judgement call what the correct behavior is here. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, Oct 07, 2022 at 03:34:51PM +0900, Michael Paquier wrote:
> Thanks. I would perhaps use names less generic than role{1,2,3} for
> the roles or "role1" for the database name, but the logic looks
> sound.
Here is a new version with more descriptive role names.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachment
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Oct 07, 2022 at 03:34:51PM +0900, Michael Paquier wrote:
>> Thanks. I would perhaps use names less generic than role{1,2,3} for
>> the roles or "role1" for the database name, but the logic looks
>> sound.
> Here is a new version with more descriptive role names.
There's another problem there, which is that buildfarm animals
using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain
about role names that don't start with "regress_".
regards, tom lane
On Fri, Oct 07, 2022 at 04:18:59PM -0400, Tom Lane wrote: > There's another problem there, which is that buildfarm animals > using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain > about role names that don't start with "regress_". Huh, I hadn't noticed that one before. It looks like roles must start with "regress_" and database names must include "regression", so I ended up using "regress_regression_group" for the samegroup/samerole tests. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Oct 07, 2022 at 07:59:08AM -0400, Robert Haas wrote: > I hadn't noticed this thread before. > > I'm not sure whether this is properly considered a privilege check. It > could even be an anti-privilege, if the pg_hba.conf line in question > is maked "reject". > > I'm not taking the position that what this patch does is wrong, but I > *am* taking the position that it's a judgement call what the correct > behavior is here. The interpretation can go both ways I guess. Now I find the argument to treat a HBA entry based on privileges and not membership quite appealing in terms of consistency wiht SET ROLE, particularly considering the recent thread with predefined roles. Also, it seems to me here that it would become easier to reason around role hierarchies, one case being HBA entries that include predefined roles for the role(s) to match. -- Michael
Attachment
On 10/7/22 17:58, Nathan Bossart wrote: > On Fri, Oct 07, 2022 at 04:18:59PM -0400, Tom Lane wrote: >> There's another problem there, which is that buildfarm animals >> using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain >> about role names that don't start with "regress_". > > Huh, I hadn't noticed that one before. It looks like roles must start with > "regress_" and database names must include "regression", so I ended up > using "regress_regression_group" for the samegroup/samerole tests. Thanks -- looks good to me. If there are no other comments or concerns, I will commit/push by the end of the weekend. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes:
> Thanks -- looks good to me. If there are no other comments or concerns,
> I will commit/push by the end of the weekend.
Robert seems to think that this patch might be completely misguided,
so I'm not sure we have real consensus. I think he may have a point.
An angle that he didn't bring up is that we've had proposals, and
even I think a patch, for inventing database-local privileges.
If that were to become a thing, it would interact very badly with
this idea, because it would often not be clear which set of privileges
to consider. As long as HBA checks consider membership, and we don't
invent database-local role membership, there's no problem.
regards, tom lane
On Sat, Oct 8, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Joe Conway <mail@joeconway.com> writes: > > Thanks -- looks good to me. If there are no other comments or concerns, > > I will commit/push by the end of the weekend. > > Robert seems to think that this patch might be completely misguided, > so I'm not sure we have real consensus. I think he may have a point. > > An angle that he didn't bring up is that we've had proposals, and > even I think a patch, for inventing database-local privileges. > If that were to become a thing, it would interact very badly with > this idea, because it would often not be clear which set of privileges > to consider. As long as HBA checks consider membership, and we don't > invent database-local role membership, there's no problem. This argument feels a little bit thin to me, because (1) one could equally well postulate that we'd want to invent database-local role membership and (2) presumably the relevant set of privileges would be those for the database to which the user wishes to authenticate. I think what is bothering me is a feeling that a privilege is something that you get because you've authenticated. If you haven't authenticated yet, you have no privileges. So why should it matter whether the role to which you could hypothetically authenticate would inherit the privileges of some other role or not? Or to put it another way, I don't have any intuition for why someone would want the system to behave in this way rather than in the way that it does now. In general, what role inheritance does is actually pretty easy to understand: either you just have the ability to access the privileges of some other role at need, or you have those privileges all the time even without activating them explicitly. I think in most cases people will expect membership in a predefined role or a role used as a group to behave in the second way, and membership in a login role to be used in the first way, but I think there will likely be some exceptions in both directions, which is fine, because we can support that. But the usage where you mention a group in pg_hba.conf feels orthogonal to all of that to me. In that case, it's not really about privileges at all, or at least I don't think so. It's about letting one group of people log into the system from, say, a certain IP address, and others not (or maybe the reverse). It seems reasonably likely that you wouldn't want the role you used for grouping purposes in a case like this to hold any privileges at all, or that if it did have any privileges you wouldn't want them accessible in any way to the group members, because if you create a group called people_who_can_log_in_from_the_modem_pool, you do not therefore want to end up with tables owned by people_who_can_log_in_from_the_modem_pool. Under that theory, this patch is going in the wrong direction. Now there may be some other scenario in which the patch is going in exactly the right direction, and if I knew what it was, maybe I'd agree that the patch was a great idea. But I haven't seen anything like that on the thread. Basically, the argument is just that the change would make things more consistent. However, it might be an abuse of the term. If you go out and buy blue curtains because you have a blue couch, that's consistent interior decor. If you go out and buy a blue car because you have a blue couch, that's not really consistent anything, it's just two fairly-unrelated things that are both blue. -- Robert Haas EDB: http://www.enterprisedb.com
On Sat, Oct 8, 2022 at 8:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Oct 8, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Joe Conway <mail@joeconway.com> writes:
> > Thanks -- looks good to me. If there are no other comments or concerns,
> > I will commit/push by the end of the weekend.
>
> Robert seems to think that this patch might be completely misguided,
> so I'm not sure we have real consensus. I think he may have a point.
I think what is bothering me is a feeling that a privilege is
something that you get because you've authenticated. If you haven't
authenticated yet, you have no privileges. So why should it matter
whether the role to which you could hypothetically authenticate would
inherit the privileges of some other role or not?
Or to put it another way, I don't have any intuition for why someone
would want the system to behave in this way rather than in the way
that it does now.
I'm also in the "inheritance isn't relevant here" camp. One doesn't inherit an ability to LOGIN from a group that has a LOGIN attribute. The [NO]INHERIT attribute doesn't even apply. This feature is so closely related to LOGIN that [NO]INHERIT should likewise not apply here as well.
We've decided to conjoin two arguably orthogonal concerns here and need to keep in mind that any given aspect of the overall capability might very well only apply to a subset of the system. In this case inheritance only applies to object permissions, not attributes, and not authentication (which doesn't have any kind of explicit permission bit in the system to inherit, making it just like LOGIN).
I would tend to agree that even membership probably shouldn't be involved here, and that this entire feature would be implemented in an orthogonal manner. I don't see any specific need to try and move to a more isolated implementation, but trying to involve inheritance just seems wrong. The status quo seems like a good place to stay.
David J.
On Sat, Oct 08, 2022 at 11:46:50AM -0400, Robert Haas wrote: > Now there may be some other scenario in which the patch is going in > exactly the right direction, and if I knew what it was, maybe I'd > agree that the patch was a great idea. But I haven't seen anything > like that on the thread. Basically, the argument is just that the > change would make things more consistent. However, it might be an > abuse of the term. If you go out and buy blue curtains because you > have a blue couch, that's consistent interior decor. If you go out and > buy a blue car because you have a blue couch, that's not really > consistent anything, it's just two fairly-unrelated things that are > both blue. I believe I started this thread after reviewing the remaining uses of is_member_of_role() after 6198420 was committed and wondering whether this case was an oversight. If upon closer inspection we think that mere membership is appropriate for pg_hba.conf, I'm fully prepared to go and mark this commitfest entry as Rejected. It obviously does not seem as clear-cut as 6198420. And I'll admit I don't have a concrete use-case in hand to justify the behavior change. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Sat, Oct 08, 2022 at 09:57:02AM -0700, David G. Johnston wrote: > I would tend to agree that even membership probably shouldn't be involved > here, and that this entire feature would be implemented in an orthogonal > manner. I don't see any specific need to try and move to a more isolated > implementation, but trying to involve inheritance just seems wrong. The > status quo seems like a good place to stay. Okay, I think there are sufficient votes against this change to simply mark it Rejected. Thanks for the discussion! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Sat, Oct 08, 2022 at 10:12:22AM -0700, Nathan Bossart wrote: > Okay, I think there are sufficient votes against this change to simply mark > it Rejected. Thanks for the discussion! Even if the patch is at the end rejected, I think that the test is still useful once you switch its logic to use membership and not inherited privileges for the roles created, and there is zero coverage for "samplegroup" and its kind currently. -- Michael
Attachment
On Sun, Oct 09, 2022 at 10:19:51AM +0900, Michael Paquier wrote: > Even if the patch is at the end rejected, I think that the test is > still useful once you switch its logic to use membership and not > inherited privileges for the roles created, and there is zero coverage > for "samplegroup" and its kind currently. Here you go. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Sun, Oct 09, 2022 at 02:13:48PM -0700, Nathan Bossart wrote: > Here you go. Thanks, applied. It took me a few minutes to note that regress_regression_* is required in the object names because we need to use the same name for the parent role and the database, with "regress_" being required for the role and "regression" being required for the database. I have added an extra section where pg_hba.conf is set to match only the parent role, while on it. perltidy has reshaped things in an interesting way, because the generated log_[un]like is long, it seems. -- Michael
Attachment
On Tue, Oct 11, 2022 at 02:01:07PM +0900, Michael Paquier wrote: > Thanks, applied. It took me a few minutes to note that > regress_regression_* is required in the object names because we need > to use the same name for the parent role and the database, with > "regress_" being required for the role and "regression" being required > for the database. I have added an extra section where pg_hba.conf is > set to match only the parent role, while on it. perltidy has reshaped > things in an interesting way, because the generated log_[un]like is > long, it seems. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Greetings, * Nathan Bossart (nathandbossart@gmail.com) wrote: > On Sat, Oct 08, 2022 at 11:46:50AM -0400, Robert Haas wrote: > > Now there may be some other scenario in which the patch is going in > > exactly the right direction, and if I knew what it was, maybe I'd > > agree that the patch was a great idea. But I haven't seen anything > > like that on the thread. Basically, the argument is just that the > > change would make things more consistent. However, it might be an > > abuse of the term. If you go out and buy blue curtains because you > > have a blue couch, that's consistent interior decor. If you go out and > > buy a blue car because you have a blue couch, that's not really > > consistent anything, it's just two fairly-unrelated things that are > > both blue. > > I believe I started this thread after reviewing the remaining uses of > is_member_of_role() after 6198420 was committed and wondering whether this > case was an oversight. If upon closer inspection we think that mere > membership is appropriate for pg_hba.conf, I'm fully prepared to go and > mark this commitfest entry as Rejected. It obviously does not seem as > clear-cut as 6198420. And I'll admit I don't have a concrete use-case in > hand to justify the behavior change. Looks like we've already ended up there, but my recollection of this is that it was very much intentional to use is_member_of_role() here. Perhaps it should have been better commented (as all uses of is_member_of_role() instead of has_privs_of_role() really should have lots of comments as to exactly why it makes sense in those cases). Thanks, Stephen