Re: [PATCH v2] use has_privs_for_role for predefined roles
On Sat, Apr 2, 2022 at 1:32 PM Joe Conway wrote: > I saw that Robert added documentation and it already reads correctly I > believe, except possibly an unrelated issue: > > 8<-- > >A role which replication whose privileges users are required to > possess >in order to make use of the shell backup target. >If this is not set, any replication user may make use of the >shell backup target. > > 8<-- > > Robert, should that actually be: > s/role which replication/role with replication/ Oh, good catch. Actually shouldn't "with replication" just be deleted? Or maybe it needs to be reworded more, for clarity: "Replication users must possess the privileges of this role in order to ..." -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/28/22 15:56, Robert Haas wrote: On Mon, Mar 21, 2022 at 4:15 PM Joe Conway wrote: Robert -- any opinion on this? If I am not mistaken it is code that you are actively working on. Woops, I only just saw this. I don't mind if you want to change the calls to is_member_of_role() in basebackup_server.c and basebackup_to_shell.c to has_privs_of_role(). Done as originally posted. On that last note, I did not find basebackup_to_shell.required_role documented at all, and did not attempt to fix that. I saw that Robert added documentation and it already reads correctly I believe, except possibly an unrelated issue: 8<-- A role which replication whose privileges users are required to possess in order to make use of the shell backup target. If this is not set, any replication user may make use of the shell backup target. 8<-- Robert, should that actually be: s/role which replication/role with replication/ ? Joe
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/28/22 15:56, Robert Haas wrote: On Mon, Mar 21, 2022 at 4:15 PM Joe Conway wrote: Robert -- any opinion on this? If I am not mistaken it is code that you are actively working on. Woops, I only just saw this. I don't mind if you want to change the calls to is_member_of_role() in basebackup_server.c and basebackup_to_shell.c to has_privs_of_role(). No worries -- I will take care of that shortly. However, it's not clear to me why it's different than the calls we have in other places, like calculate_database_size() and the relatively widely-used check_is_member_of_role(). I will have to go refresh my memory, but when I looked at those sites closely it all made sense to me. I think most if not all of them were checking for the ability to switch to the other role, not actually checking for privileges by virtue of belonging to that role. As long as we have a bunch of different practices in different parts of the code base I can't see people getting this right consistently ... leaving aside any possible disagreement about which way is "right". When I take the next pass I can consider whether additional comments will help and report back. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Mon, Mar 21, 2022 at 4:15 PM Joe Conway wrote: > Robert -- any opinion on this? If I am not mistaken it is code that you > are actively working on. Woops, I only just saw this. I don't mind if you want to change the calls to is_member_of_role() in basebackup_server.c and basebackup_to_shell.c to has_privs_of_role(). However, it's not clear to me why it's different than the calls we have in other places, like calculate_database_size() and the relatively widely-used check_is_member_of_role(). As long as we have a bunch of different practices in different parts of the code base I can't see people getting this right consistently ... leaving aside any possible disagreement about which way is "right". -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/21/22 16:15, Joe Conway wrote: On 3/20/22 12:38, Stephen Frost wrote: Greetings, On Sun, Mar 20, 2022 at 18:31 Joshua Brindle mailto:joshua.brin...@crunchydata.com>> wrote: On Sun, Mar 20, 2022 at 12:27 PM Joe Conway mailto:m...@joeconway.com>> wrote: > > On 3/3/22 11:26, Joshua Brindle wrote: > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway mailto:m...@joeconway.com>> wrote: > >> > >> On 2/10/22 14:28, Nathan Bossart wrote: > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > >> >> On 2/9/22 13:13, Nathan Bossart wrote: > >> >>> I do wonder if users find the differences between predefined roles and role > >> >>> attributes confusing. INHERIT doesn't govern role attributes, but it will > >> >>> govern predefined roles when this patch is applied. Maybe the role > >> >>> attribute system should eventually be deprecated in favor of using > >> >>> predefined roles for everything. Or perhaps the predefined roles should be > >> >>> converted to role attributes. > >> >> > >> >> Yep, I was suggesting that the latter would have been preferable to me while > >> >> Robert seemed to prefer the former. Honestly I could be happy with either of > >> >> those solutions, but as I alluded to that is probably a discussion for the > >> >> next development cycle since I don't see us doing that big a change in this > >> >> one. > >> > > >> > I agree. I still think Joshua's proposed patch is a worthwhile improvement > >> > for v15. > >> > >> +1 > >> > >> I am planning to get into it in detail this weekend. So far I have > >> really just ensured it merges cleanly and passes make world. > > > > Rebased patch to apply to master attached. > > Well longer than I planned, but finally took a closer look. > > I made one minor editorial fix to Joshua's patch, rebased to current > master, and added two missing call sites that presumably are related to > recent commits for pg_basebackup. FWIW I pinged Stephen when I saw the basebackup changes included is_member_of and he didn't think they necessarily needed to be changed since they aren't accessible to human and you can't SET ROLE on a replication connection in order to access the role where inheritance was blocked. Yeah, though that should really be very clearly explained in comments around that code as anything that uses is_member should really be viewed as an exception. I also wouldn’t be against using has_privs there anyway and saying that you shouldn’t be trying to connect as one role on a replication connection and using the privs of another when you don’t automatically inherit those rights, but on the assumption that the committer intended is_member there because SET ROLE isn’t something one does on replication connections, I’m alright with it being as is. Robert -- any opinion on this? If I am not mistaken it is code that you are actively working on. Lacking any feedback from Robert, I removed my changes related to basebackup and pushed Joshua's patch with one minor editorial fix. What to do with the basebackup changes can go on the open items list for pg15 I guess. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/20/22 12:38, Stephen Frost wrote: Greetings, On Sun, Mar 20, 2022 at 18:31 Joshua Brindle mailto:joshua.brin...@crunchydata.com>> wrote: On Sun, Mar 20, 2022 at 12:27 PM Joe Conway mailto:m...@joeconway.com>> wrote: > > On 3/3/22 11:26, Joshua Brindle wrote: > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway mailto:m...@joeconway.com>> wrote: > >> > >> On 2/10/22 14:28, Nathan Bossart wrote: > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > >> >> On 2/9/22 13:13, Nathan Bossart wrote: > >> >>> I do wonder if users find the differences between predefined roles and role > >> >>> attributes confusing. INHERIT doesn't govern role attributes, but it will > >> >>> govern predefined roles when this patch is applied. Maybe the role > >> >>> attribute system should eventually be deprecated in favor of using > >> >>> predefined roles for everything. Or perhaps the predefined roles should be > >> >>> converted to role attributes. > >> >> > >> >> Yep, I was suggesting that the latter would have been preferable to me while > >> >> Robert seemed to prefer the former. Honestly I could be happy with either of > >> >> those solutions, but as I alluded to that is probably a discussion for the > >> >> next development cycle since I don't see us doing that big a change in this > >> >> one. > >> > > >> > I agree. I still think Joshua's proposed patch is a worthwhile improvement > >> > for v15. > >> > >> +1 > >> > >> I am planning to get into it in detail this weekend. So far I have > >> really just ensured it merges cleanly and passes make world. > > > > Rebased patch to apply to master attached. > > Well longer than I planned, but finally took a closer look. > > I made one minor editorial fix to Joshua's patch, rebased to current > master, and added two missing call sites that presumably are related to > recent commits for pg_basebackup. FWIW I pinged Stephen when I saw the basebackup changes included is_member_of and he didn't think they necessarily needed to be changed since they aren't accessible to human and you can't SET ROLE on a replication connection in order to access the role where inheritance was blocked. Yeah, though that should really be very clearly explained in comments around that code as anything that uses is_member should really be viewed as an exception. I also wouldn’t be against using has_privs there anyway and saying that you shouldn’t be trying to connect as one role on a replication connection and using the privs of another when you don’t automatically inherit those rights, but on the assumption that the committer intended is_member there because SET ROLE isn’t something one does on replication connections, I’m alright with it being as is. Robert -- any opinion on this? If I am not mistaken it is code that you are actively working on. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
Greetings, On Sun, Mar 20, 2022 at 18:31 Joshua Brindle wrote: > On Sun, Mar 20, 2022 at 12:27 PM Joe Conway wrote: > > > > On 3/3/22 11:26, Joshua Brindle wrote: > > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote: > > >> > > >> On 2/10/22 14:28, Nathan Bossart wrote: > > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > > >> >> On 2/9/22 13:13, Nathan Bossart wrote: > > >> >>> I do wonder if users find the differences between predefined > roles and role > > >> >>> attributes confusing. INHERIT doesn't govern role attributes, > but it will > > >> >>> govern predefined roles when this patch is applied. Maybe the > role > > >> >>> attribute system should eventually be deprecated in favor of using > > >> >>> predefined roles for everything. Or perhaps the predefined roles > should be > > >> >>> converted to role attributes. > > >> >> > > >> >> Yep, I was suggesting that the latter would have been preferable > to me while > > >> >> Robert seemed to prefer the former. Honestly I could be happy with > either of > > >> >> those solutions, but as I alluded to that is probably a discussion > for the > > >> >> next development cycle since I don't see us doing that big a > change in this > > >> >> one. > > >> > > > >> > I agree. I still think Joshua's proposed patch is a worthwhile > improvement > > >> > for v15. > > >> > > >> +1 > > >> > > >> I am planning to get into it in detail this weekend. So far I have > > >> really just ensured it merges cleanly and passes make world. > > > > > > Rebased patch to apply to master attached. > > > > Well longer than I planned, but finally took a closer look. > > > > I made one minor editorial fix to Joshua's patch, rebased to current > > master, and added two missing call sites that presumably are related to > > recent commits for pg_basebackup. > > FWIW I pinged Stephen when I saw the basebackup changes included > is_member_of and he didn't think they necessarily needed to be changed > since they aren't accessible to human and you can't SET ROLE on a > replication connection in order to access the role where inheritance > was blocked. Yeah, though that should really be very clearly explained in comments around that code as anything that uses is_member should really be viewed as an exception. I also wouldn’t be against using has_privs there anyway and saying that you shouldn’t be trying to connect as one role on a replication connection and using the privs of another when you don’t automatically inherit those rights, but on the assumption that the committer intended is_member there because SET ROLE isn’t something one does on replication connections, I’m alright with it being as is. Kind Regards, Stephen P Frost >
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Sun, Mar 20, 2022 at 12:34 PM Joe Conway wrote: > > On 3/20/22 12:31, Joshua Brindle wrote: > > On Sun, Mar 20, 2022 at 12:27 PM Joe Conway wrote: > >> > >> On 3/3/22 11:26, Joshua Brindle wrote: > >> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote: > >> >> > >> >> On 2/10/22 14:28, Nathan Bossart wrote: > >> >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > >> >> >> On 2/9/22 13:13, Nathan Bossart wrote: > >> >> >>> I do wonder if users find the differences between predefined roles > >> >> >>> and role > >> >> >>> attributes confusing. INHERIT doesn't govern role attributes, but > >> >> >>> it will > >> >> >>> govern predefined roles when this patch is applied. Maybe the role > >> >> >>> attribute system should eventually be deprecated in favor of using > >> >> >>> predefined roles for everything. Or perhaps the predefined roles > >> >> >>> should be > >> >> >>> converted to role attributes. > >> >> >> > >> >> >> Yep, I was suggesting that the latter would have been preferable to > >> >> >> me while > >> >> >> Robert seemed to prefer the former. Honestly I could be happy with > >> >> >> either of > >> >> >> those solutions, but as I alluded to that is probably a discussion > >> >> >> for the > >> >> >> next development cycle since I don't see us doing that big a change > >> >> >> in this > >> >> >> one. > >> >> > > >> >> > I agree. I still think Joshua's proposed patch is a worthwhile > >> >> > improvement > >> >> > for v15. > >> >> > >> >> +1 > >> >> > >> >> I am planning to get into it in detail this weekend. So far I have > >> >> really just ensured it merges cleanly and passes make world. > >> > > >> > Rebased patch to apply to master attached. > >> > >> Well longer than I planned, but finally took a closer look. > >> > >> I made one minor editorial fix to Joshua's patch, rebased to current > >> master, and added two missing call sites that presumably are related to > >> recent commits for pg_basebackup. > > > > FWIW I pinged Stephen when I saw the basebackup changes included > > is_member_of and he didn't think they necessarily needed to be changed > > since they aren't accessible to human and you can't SET ROLE on a > > replication connection in order to access the role where inheritance > > was blocked. > > Maybe worth a discussion, but it seems strange to me to treat them > differently when the whole purpose of this patch is to make things > consistent ¯\_(ツ)_/¯ > I don't have a strong opinion either way, just noting that it's a possible exception area due to how it is used. Thanks for committing this.
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/20/22 12:31, Joshua Brindle wrote: On Sun, Mar 20, 2022 at 12:27 PM Joe Conway wrote: On 3/3/22 11:26, Joshua Brindle wrote: > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote: >> >> On 2/10/22 14:28, Nathan Bossart wrote: >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: >> >> On 2/9/22 13:13, Nathan Bossart wrote: >> >>> I do wonder if users find the differences between predefined roles and role >> >>> attributes confusing. INHERIT doesn't govern role attributes, but it will >> >>> govern predefined roles when this patch is applied. Maybe the role >> >>> attribute system should eventually be deprecated in favor of using >> >>> predefined roles for everything. Or perhaps the predefined roles should be >> >>> converted to role attributes. >> >> >> >> Yep, I was suggesting that the latter would have been preferable to me while >> >> Robert seemed to prefer the former. Honestly I could be happy with either of >> >> those solutions, but as I alluded to that is probably a discussion for the >> >> next development cycle since I don't see us doing that big a change in this >> >> one. >> > >> > I agree. I still think Joshua's proposed patch is a worthwhile improvement >> > for v15. >> >> +1 >> >> I am planning to get into it in detail this weekend. So far I have >> really just ensured it merges cleanly and passes make world. > > Rebased patch to apply to master attached. Well longer than I planned, but finally took a closer look. I made one minor editorial fix to Joshua's patch, rebased to current master, and added two missing call sites that presumably are related to recent commits for pg_basebackup. FWIW I pinged Stephen when I saw the basebackup changes included is_member_of and he didn't think they necessarily needed to be changed since they aren't accessible to human and you can't SET ROLE on a replication connection in order to access the role where inheritance was blocked. Maybe worth a discussion, but it seems strange to me to treat them differently when the whole purpose of this patch is to make things consistent ¯\_(ツ)_/¯ Joe
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Sun, Mar 20, 2022 at 12:27 PM Joe Conway wrote: > > On 3/3/22 11:26, Joshua Brindle wrote: > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote: > >> > >> On 2/10/22 14:28, Nathan Bossart wrote: > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > >> >> On 2/9/22 13:13, Nathan Bossart wrote: > >> >>> I do wonder if users find the differences between predefined roles and > >> >>> role > >> >>> attributes confusing. INHERIT doesn't govern role attributes, but it > >> >>> will > >> >>> govern predefined roles when this patch is applied. Maybe the role > >> >>> attribute system should eventually be deprecated in favor of using > >> >>> predefined roles for everything. Or perhaps the predefined roles > >> >>> should be > >> >>> converted to role attributes. > >> >> > >> >> Yep, I was suggesting that the latter would have been preferable to me > >> >> while > >> >> Robert seemed to prefer the former. Honestly I could be happy with > >> >> either of > >> >> those solutions, but as I alluded to that is probably a discussion for > >> >> the > >> >> next development cycle since I don't see us doing that big a change in > >> >> this > >> >> one. > >> > > >> > I agree. I still think Joshua's proposed patch is a worthwhile > >> > improvement > >> > for v15. > >> > >> +1 > >> > >> I am planning to get into it in detail this weekend. So far I have > >> really just ensured it merges cleanly and passes make world. > > > > Rebased patch to apply to master attached. > > Well longer than I planned, but finally took a closer look. > > I made one minor editorial fix to Joshua's patch, rebased to current > master, and added two missing call sites that presumably are related to > recent commits for pg_basebackup. FWIW I pinged Stephen when I saw the basebackup changes included is_member_of and he didn't think they necessarily needed to be changed since they aren't accessible to human and you can't SET ROLE on a replication connection in order to access the role where inheritance was blocked. > On that last note, I did not find basebackup_to_shell.required_role > documented at all, and did not attempt to fix that. > > When this thread petered out, it seemed that Robert was at least neutral > on the patch, and everyone else was +1 on applying it to master for pg15. > > As such, if there are any other issues, complaints, etc., please speak > real soon now... > > Thanks, > > Joe
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 3/3/22 11:26, Joshua Brindle wrote:
On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote:
On 2/10/22 14:28, Nathan Bossart wrote:
> On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> On 2/9/22 13:13, Nathan Bossart wrote:
>>> I do wonder if users find the differences between predefined roles and role
>>> attributes confusing. INHERIT doesn't govern role attributes, but it will
>>> govern predefined roles when this patch is applied. Maybe the role
>>> attribute system should eventually be deprecated in favor of using
>>> predefined roles for everything. Or perhaps the predefined roles should be
>>> converted to role attributes.
>>
>> Yep, I was suggesting that the latter would have been preferable to me while
>> Robert seemed to prefer the former. Honestly I could be happy with either of
>> those solutions, but as I alluded to that is probably a discussion for the
>> next development cycle since I don't see us doing that big a change in this
>> one.
>
> I agree. I still think Joshua's proposed patch is a worthwhile improvement
> for v15.
+1
I am planning to get into it in detail this weekend. So far I have
really just ensured it merges cleanly and passes make world.
Rebased patch to apply to master attached.
Well longer than I planned, but finally took a closer look.
I made one minor editorial fix to Joshua's patch, rebased to current
master, and added two missing call sites that presumably are related to
recent commits for pg_basebackup.
On that last note, I did not find basebackup_to_shell.required_role
documented at all, and did not attempt to fix that.
When this thread petered out, it seemed that Robert was at least neutral
on the patch, and everyone else was +1 on applying it to master for pg15.
As such, if there are any other issues, complaints, etc., please speak
real soon now...
Thanks,
JoeUse has_privs_for_roles for predefined role checks
Generally if a role is granted membership to another role with NOINHERIT they must use SET ROLE to access the privileges of that role, however with predefined roles the membership and privilege is conflated. Fix that by replacing is_member_of_role with has_privs_for_role for predefined roles. Patch does not remove is_member_of_role from acl.h, but it does add a warning not to use that function for privilege checking. Not backpatched based on hackers list discussion.
Author: Joshua Brindle
Reviewed-by: Stephen Frost, Nathan Bossart, Joe Conway
Discussion: https://postgr.es/m/flat/cagb+vh4zv_tvkt2tv3qns6tum_f_9icmuj0zjywwcgvi4pa...@mail.gmail.com
diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index d7d84d0..03addf1 100644
*** a/contrib/adminpack/adminpack.c
--- b/contrib/adminpack/adminpack.c
*** convert_and_check_filename(text *arg)
*** 79,85
* files on the server as the PG user, so no need to do any further checks
* here.
*/
! if (is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
return filename;
/*
--- 79,85
* files on the server as the PG user, so no need to do any further checks
* here.
*/
! if (has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
return filename;
/*
diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index d82cb6d..f0ddef1 100644
*** a/contrib/basebackup_to_shell/basebackup_to_shell.c
--- b/contrib/basebackup_to_shell/basebackup_to_shell.c
*** _PG_init(void)
*** 90,96
}
/*
! * We choose to defer sanity sanity checking until shell_get_sink(), and so
* just pass the target detail through without doing anything. However, we do
* permissions checks here, before any real work has been done.
*/
--- 90,96
}
/*
! * We choose to defer sanity checking until shell_get_sink(), and so
* just pass the target detail through without doing anything. However, we do
* permissions checks here, before any real work has been done.
*/
*** shell_check_detail(char *target, char *t
*** 103,109
StartTransactionCommand();
roleid = get_role_oid(shell_required_role, true);
! if (!is_member_of_role(GetUserId(), roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to use basebackup_to_shell")));
--- 103,109
StartTransactionCommand();
roleid = get_role_oid(shell_required_role, true);
! if (!has_privs_of_role(GetUserId(), roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to use basebackup_to_shell")));
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 0ac6e4e..14acdb2 100644
*** a/contrib/file_fdw/expected/file_fdw.out
--- b/contrib/file_fdw/expected/file_fdw.out
*** ALTER FOREIGN TABLE agg_text OWNER TO re
*** 459,465
ALTER FOREIGN TABLE agg_text OPTIONS (SET format
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Thu, Feb 10, 2022 at 2:37 PM Joe Conway wrote: > > On 2/10/22 14:28, Nathan Bossart wrote: > > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > >> On 2/9/22 13:13, Nathan Bossart wrote: > >>> I do wonder if users find the differences between predefined roles and > >>> role > >>> attributes confusing. INHERIT doesn't govern role attributes, but it will > >>> govern predefined roles when this patch is applied. Maybe the role > >>> attribute system should eventually be deprecated in favor of using > >>> predefined roles for everything. Or perhaps the predefined roles should > >>> be > >>> converted to role attributes. > >> > >> Yep, I was suggesting that the latter would have been preferable to me > >> while > >> Robert seemed to prefer the former. Honestly I could be happy with either > >> of > >> those solutions, but as I alluded to that is probably a discussion for the > >> next development cycle since I don't see us doing that big a change in this > >> one. > > > > I agree. I still think Joshua's proposed patch is a worthwhile improvement > > for v15. > > +1 > > I am planning to get into it in detail this weekend. So far I have > really just ensured it merges cleanly and passes make world. > Rebased patch to apply to master attached. v5-0001-use-has_privs_for_roles-for-predefined-role-checks.patch Description: Binary data
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 2/10/22 14:28, Nathan Bossart wrote: On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: On 2/9/22 13:13, Nathan Bossart wrote: I do wonder if users find the differences between predefined roles and role attributes confusing. INHERIT doesn't govern role attributes, but it will govern predefined roles when this patch is applied. Maybe the role attribute system should eventually be deprecated in favor of using predefined roles for everything. Or perhaps the predefined roles should be converted to role attributes. Yep, I was suggesting that the latter would have been preferable to me while Robert seemed to prefer the former. Honestly I could be happy with either of those solutions, but as I alluded to that is probably a discussion for the next development cycle since I don't see us doing that big a change in this one. I agree. I still think Joshua's proposed patch is a worthwhile improvement for v15. +1 I am planning to get into it in detail this weekend. So far I have really just ensured it merges cleanly and passes make world. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote: > On 2/9/22 13:13, Nathan Bossart wrote: >> I do wonder if users find the differences between predefined roles and role >> attributes confusing. INHERIT doesn't govern role attributes, but it will >> govern predefined roles when this patch is applied. Maybe the role >> attribute system should eventually be deprecated in favor of using >> predefined roles for everything. Or perhaps the predefined roles should be >> converted to role attributes. > > Yep, I was suggesting that the latter would have been preferable to me while > Robert seemed to prefer the former. Honestly I could be happy with either of > those solutions, but as I alluded to that is probably a discussion for the > next development cycle since I don't see us doing that big a change in this > one. I agree. I still think Joshua's proposed patch is a worthwhile improvement for v15. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Feb 9, 2022 at 3:58 PM Robert Haas wrote: > > On Wed, Feb 9, 2022 at 1:13 PM Nathan Bossart > wrote: > > I do wonder if users find the differences between predefined roles and role > > attributes confusing. INHERIT doesn't govern role attributes, but it will > > govern predefined roles when this patch is applied. Maybe the role > > attribute system should eventually be deprecated in favor of using > > predefined roles for everything. Or perhaps the predefined roles should be > > converted to role attributes. > > I couldn't agree more. Apparently it's even confusing to developers, > because otherwise (1) we wouldn't have the problem the patch proposes > to fix in the first place and (2) I would have immediately been > convinced of the value of the patch once it showed up. Since those > things didn't happen, this is apparently confusing to (1) whoever > wrote the code that this patch fixes and (2) me. > My original patch removed is_member_of to address #1 above, but that was rejected[1]. There is now a warning in the header beside it to hopefully dissuade improper usage going forward. 1. https://www.postgresql.org/message-id/254275.1635357633%40sss.pgh.pa.us
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 2/9/22 13:13, Nathan Bossart wrote: On Tue, Feb 08, 2022 at 10:54:50PM -0500, Robert Haas wrote: On Tue, Feb 8, 2022 at 7:38 PM Joe Conway wrote: If we were to start all over again with this feature my vote would be to do things differently than we have done. I would not have called them predefined roles, and I would have used attributes of roles (e.g. make rolsuper into a bitmap rather than a boolean) rather than role membership to implement them. But I didn't find time to participate in the original discussion or review/write the code, so I have little room to complain. Yep, fair. I kind of like the predefined role concept myself. I find it sort of elegant, mostly because I think it scales better than a bitmask, which can run out of bits surprisingly rapidly. But opinions can vary, of course. I do wonder if users find the differences between predefined roles and role attributes confusing. INHERIT doesn't govern role attributes, but it will govern predefined roles when this patch is applied. Maybe the role attribute system should eventually be deprecated in favor of using predefined roles for everything. Or perhaps the predefined roles should be converted to role attributes. Yep, I was suggesting that the latter would have been preferable to me while Robert seemed to prefer the former. Honestly I could be happy with either of those solutions, but as I alluded to that is probably a discussion for the next development cycle since I don't see us doing that big a change in this one. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Feb 9, 2022 at 1:13 PM Nathan Bossart wrote: > I do wonder if users find the differences between predefined roles and role > attributes confusing. INHERIT doesn't govern role attributes, but it will > govern predefined roles when this patch is applied. Maybe the role > attribute system should eventually be deprecated in favor of using > predefined roles for everything. Or perhaps the predefined roles should be > converted to role attributes. I couldn't agree more. Apparently it's even confusing to developers, because otherwise (1) we wouldn't have the problem the patch proposes to fix in the first place and (2) I would have immediately been convinced of the value of the patch once it showed up. Since those things didn't happen, this is apparently confusing to (1) whoever wrote the code that this patch fixes and (2) me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Feb 08, 2022 at 10:54:50PM -0500, Robert Haas wrote: > On Tue, Feb 8, 2022 at 7:38 PM Joe Conway wrote: >> If we were to start all over again with this feature my vote would be to >> do things differently than we have done. I would not have called them >> predefined roles, and I would have used attributes of roles (e.g. make >> rolsuper into a bitmap rather than a boolean) rather than role >> membership to implement them. But I didn't find time to participate in >> the original discussion or review/write the code, so I have little room >> to complain. > > Yep, fair. I kind of like the predefined role concept myself. I find > it sort of elegant, mostly because I think it scales better than a > bitmask, which can run out of bits surprisingly rapidly. But opinions > can vary, of course. I do wonder if users find the differences between predefined roles and role attributes confusing. INHERIT doesn't govern role attributes, but it will govern predefined roles when this patch is applied. Maybe the role attribute system should eventually be deprecated in favor of using predefined roles for everything. Or perhaps the predefined roles should be converted to role attributes. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Feb 8, 2022 at 7:38 PM Joe Conway wrote: > If we were to start all over again with this feature my vote would be to > do things differently than we have done. I would not have called them > predefined roles, and I would have used attributes of roles (e.g. make > rolsuper into a bitmap rather than a boolean) rather than role > membership to implement them. But I didn't find time to participate in > the original discussion or review/write the code, so I have little room > to complain. Yep, fair. I kind of like the predefined role concept myself. I find it sort of elegant, mostly because I think it scales better than a bitmask, which can run out of bits surprisingly rapidly. But opinions can vary, of course. > However since we did call these things predefined roles, and used role > membership to implement them, I think they ought to behave both > self-consistently as a group, and like other real roles. > > That is what this patch does unless I am missing something. Yes, I see that. > I guess an alternative is to discuss a "proper fix", but it seems to me > that would be a version 16 thing given how late we are in this > development cycle and how invasive it is likely to be. And doing nothing > for pg15 is not a very satisfying proposition :-/ True. The fact that we don't have consistency among existing predefined roles is IMHO the best argument for this patch. That surely can't be right. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 2/8/22 10:07, Robert Haas wrote: On Tue, Feb 8, 2022 at 10:00 AM Joshua Brindle wrote: 4 predefined roles currently use has_privs_of_role in master. Further, pg_monitor, as an SQL-only predefined role, also behaves consistently with the INHERIT rules that other roles do. In order for SQL-only predefined roles to ignore INHERIT we would need to hardcode bypasses for them, which IMO seems like the worst possible solution to the current inconsistency. I agree we need to make the situation consistent. But if you think there's exactly one problem here and this patch fixes it, I emphatically disagree. If we were to start all over again with this feature my vote would be to do things differently than we have done. I would not have called them predefined roles, and I would have used attributes of roles (e.g. make rolsuper into a bitmap rather than a boolean) rather than role membership to implement them. But I didn't find time to participate in the original discussion or review/write the code, so I have little room to complain. However since we did call these things predefined roles, and used role membership to implement them, I think they ought to behave both self-consistently as a group, and like other real roles. That is what this patch does unless I am missing something. I guess an alternative is to discuss a "proper fix", but it seems to me that would be a version 16 thing given how late we are in this development cycle and how invasive it is likely to be. And doing nothing for pg15 is not a very satisfying proposition :-/ Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Feb 8, 2022 at 10:00 AM Joshua Brindle wrote: > 4 predefined roles currently use has_privs_of_role in master. > > Further, pg_monitor, as an SQL-only predefined role, also behaves > consistently with the INHERIT rules that other roles do. > > In order for SQL-only predefined roles to ignore INHERIT we would need > to hardcode bypasses for them, which IMO seems like the worst possible > solution to the current inconsistency. I agree we need to make the situation consistent. But if you think there's exactly one problem here and this patch fixes it, I emphatically disagree. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Feb 8, 2022 at 8:46 AM Robert Haas wrote: > > On Tue, Feb 8, 2022 at 6:59 AM Joe Conway wrote: > > This is similar to bob's access to the default superuser privilege to > > read data in someone else's table (must SET ROLE to access that capability). > > > > But it is different from bob's access to inherited privileges which are > > GRANTed: > > Yeah. I think right here you've put your finger on what's been bugging > me about this: it's similar to one thing, and it's different from > another. To you and Joshua and Stephen, it seems 100% obvious that > these roles should work like grants of other roles. But I think of > them as capabilities derived from the superuser account, and so I'm > sort of tempted to think that they should work the way the superuser > bit does. And that's why I don't think the fact that they work the > other way is "just a bug" -- it's one of two possible ways that > someone could think that it ought to work based on how other things in > the system actually do work. > > I'm not hard stuck on the idea that the current behavior is right, but > I don't think that we can really say that we've made things fully > consistent unless we make things like SUPERUSER and BYPASSRLS work the > same way that you want to make predefined roles work. And probably do > something about the INHERIT flag too because the current situation > seems like a hot mess. I think hot mess is an apt description of the current situation, for example consider that: src/backend/catalog/aclchk.c 3931: has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA)) 3943: has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA)) 4279: (has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA) || 4280: has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA))) src/backend/storage/ipc/signalfuncs.c 82: if (!has_privs_of_role(GetUserId(), proc->roleId) && 83: !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) src/backend/storage/ipc/procarray.c 3843: if (!has_privs_of_role(GetUserId(), proc->roleId) && 3844: !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) src/backend/tcop/utility.c 943: if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINTER)) 4 predefined roles currently use has_privs_of_role in master. Further, pg_monitor, as an SQL-only predefined role, also behaves consistently with the INHERIT rules that other roles do. In order for SQL-only predefined roles to ignore INHERIT we would need to hardcode bypasses for them, which IMO seems like the worst possible solution to the current inconsistency.
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Feb 8, 2022 at 6:59 AM Joe Conway wrote: > This is similar to bob's access to the default superuser privilege to > read data in someone else's table (must SET ROLE to access that capability). > > But it is different from bob's access to inherited privileges which are > GRANTed: Yeah. I think right here you've put your finger on what's been bugging me about this: it's similar to one thing, and it's different from another. To you and Joshua and Stephen, it seems 100% obvious that these roles should work like grants of other roles. But I think of them as capabilities derived from the superuser account, and so I'm sort of tempted to think that they should work the way the superuser bit does. And that's why I don't think the fact that they work the other way is "just a bug" -- it's one of two possible ways that someone could think that it ought to work based on how other things in the system actually do work. I'm not hard stuck on the idea that the current behavior is right, but I don't think that we can really say that we've made things fully consistent unless we make things like SUPERUSER and BYPASSRLS work the same way that you want to make predefined roles work. And probably do something about the INHERIT flag too because the current situation seems like a hot mess. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 2/7/22 12:09, Robert Haas wrote: On Mon, Feb 7, 2022 at 11:13 AM Joe Conway wrote: It is confusing and IMHO dangerous that the predefined roles currently work differently than regular roles eith respect to privilege inheritance. I feel like that's kind of a conclusory statement, as opposed to making an argument. I mean that this tells me something about how you feel, but it doesn't really help me understand why you feel that way. The argument is that we call these things "predefined roles", but they do not behave the same way normal "roles" behave. Someone not intimately familiar with that fact could easily make bad assumptions, and therefore wind up with misconfigured security settings. In other words, it violates the principle of least astonishment (POLA). As Joshua said nearby, it simply jumps out at me as a bug. --- After more thought, perhaps the real problem is that these things should not have been called "predefined roles" at all. I know, the horse has already left the barn on that, but in any case... They are (to me at least) similar in concept to Linux capabilities in that they allow roles other than superusers to do a certain subset of the things historically reserved for superusers through a special mechanism (hardcoded) rather than through the normal privilege system (GRANTS/ACLs). As an example, the predefined role pg_read_all_settings allows a non-superuser to read GUC normally reserved for superuser access only. If I create a new user "bob" with the default INHERIT attribute, and I grant postgres to bob, bob must SET ROLE to postgres in order to access the capability to read superuser settings. This is similar to bob's access to the default superuser privilege to read data in someone else's table (must SET ROLE to access that capability). But it is different from bob's access to inherited privileges which are GRANTed: 8<-- psql nmx psql (15devel) Type "help" for help. nmx=# create user bob; CREATE ROLE nmx=# grant postgres to bob; GRANT ROLE nmx=# \q 8<-- -and- 8<-- psql -U bob nmx psql (15devel) Type "help" for help. nmx=> select current_user; current_user -- bob (1 row) nmx=> show stats_temp_directory; ERROR: must be superuser or have privileges of pg_read_all_settings to examine "stats_temp_directory" nmx=> set role postgres; SET nmx=# show stats_temp_directory; stats_temp_directory -- pg_stat_tmp (1 row) nmx=# select current_user; current_user -- postgres (1 row) nmx=# select * from foo; id 42 (1 row) nmx=# reset role; RESET nmx=> select current_user; current_user -- bob (1 row) nmx=> select * from foo; ERROR: permission denied for table foo nmx=> set role postgres; SET nmx=# grant select on table foo to postgres; GRANT nmx=# reset role; RESET nmx=> select current_user; current_user -- bob (1 row) nmx=> select * from foo; id 42 (1 row) 8<-- Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Mon, Feb 7, 2022 at 12:09 PM Robert Haas wrote: > > On Mon, Feb 7, 2022 at 11:13 AM Joe Conway wrote: > > Easily worked around with one additional level of role: > > Interesting. > > > > But in the absence of that, it seems clearly better for predefined > > > roles to disregard INHERIT and just always grant the rights they are > > > intended to give. Because if we don't do that, then we end up with > > > people having to SET ROLE to the predefined role and perform actions > > > directly as that role, which seems like it can't be what we want. I > > > almost feel like we ought to be looking for ways of preventing people > > > from doing SET ROLE to a predefined role altogether, not encouraging > > > them to do it. > > I disagree with this though. > > > > It is confusing and IMHO dangerous that the predefined roles currently > > work differently than regular roles eith respect to privilege inheritance. > > I feel like that's kind of a conclusory statement, as opposed to > making an argument. I mean that this tells me something about how you > feel, but it doesn't really help me understand why you feel that way. > > I suppose one argument in favor of your position is that if it > happened to be sri who was granted a predefined role, sunita would > inherit the rest of sr's privileges only with SET ROLE, but the > predefined role either way (IIUC, which I might not). If that's so, > then I guess I see the point, but I'm still sort of inclined to think > we're just trading one set of problems in for a different set. I just > have such a hard time imaging anyone using NOINHERIT in anger and > being happy with the result > IMO this is inarguably a plain bug. The inheritance system works one way for pre-defined roles and another way for other roles - and the difference isn't even documented. The question is whether there is a security issue warranting back patching, which is a bit of a tossup I think. According to git history it's always worked this way, and the possible breakage of pre-existing clusters seems maybe not worth it.
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Mon, Feb 7, 2022 at 11:13 AM Joe Conway wrote: > Easily worked around with one additional level of role: Interesting. > > But in the absence of that, it seems clearly better for predefined > > roles to disregard INHERIT and just always grant the rights they are > > intended to give. Because if we don't do that, then we end up with > > people having to SET ROLE to the predefined role and perform actions > > directly as that role, which seems like it can't be what we want. I > > almost feel like we ought to be looking for ways of preventing people > > from doing SET ROLE to a predefined role altogether, not encouraging > > them to do it. > I disagree with this though. > > It is confusing and IMHO dangerous that the predefined roles currently > work differently than regular roles eith respect to privilege inheritance. I feel like that's kind of a conclusory statement, as opposed to making an argument. I mean that this tells me something about how you feel, but it doesn't really help me understand why you feel that way. I suppose one argument in favor of your position is that if it happened to be sri who was granted a predefined role, sunita would inherit the rest of sr's privileges only with SET ROLE, but the predefined role either way (IIUC, which I might not). If that's so, then I guess I see the point, but I'm still sort of inclined to think we're just trading one set of problems in for a different set. I just have such a hard time imaging anyone using NOINHERIT in anger and being happy with the result -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 2/7/22 10:35, Robert Haas wrote: On Sun, Feb 6, 2022 at 12:24 PM Tom Lane wrote: Joe Conway writes: > I'd like to pick this patch up and see it through to commit/push. > Presumably that will include back-patching to all supported pg versions. > Before I go through the effort to back-patch, does anyone want to argue > that this should *not* be back-patched? Hm, I'm -0.5 or so. I think changing security-related behaviors in a stable branch is a hard sell unless you are closing a security hole. This is a fine improvement for HEAD but I'm inclined to leave the back branches alone. I think the threshold to back-patch a clear behavior change is pretty high, so I do not think it should be back-patched. ok I am also not convinced that a sufficient argument has been made for changing this in master. This isn't the only thread where NOINHERIT has come up lately, and I have to admit that I'm not a fan. Let's suppose that I have two users, let's say sunita and sri. In the real world, Sunita is Sri's manager and needs to be able to perform actions as Sri when Sri is out of the office, but it isn't desirable for Sunita to have Sri's privileges in all situations all the time. So we mark role sunita as NOINHERIT and grant sri to sunita. Then it turns out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her pg_execute_server_program. Now, if she can't exercise this privilege without setting role to the prefined role, that's bad, isn't it? I mean, we want her to be able to copy between *her* tables and various shell commands, not the tables owned by pg_execute_server_program, of which there are presumably none. Easily worked around with one additional level of role: 8<--- nmx=# create user sunita; CREATE ROLE nmx=# create user sri superuser; CREATE ROLE nmx=# create user sri_alt noinherit; CREATE ROLE nmx=# grant sri to sri_alt; GRANT ROLE nmx=# grant sri_alt to sunita; GRANT ROLE nmx=# grant pg_execute_server_program to sunita; GRANT ROLE nmx=# set session authorization sri; SET nmx=# create table foo(id int); CREATE TABLE nmx=# insert into foo values(42); INSERT 0 1 nmx=# set session authorization sunita; SET nmx=> select * from foo; ERROR: permission denied for table foo nmx=> set role sri; SET nmx=# select * from foo; id 42 (1 row) nmx=# reset role; RESET nmx=> select current_user; current_user -- sunita (1 row) nmx=> create temp table sfoo(f1 text); CREATE TABLE nmx=> copy sfoo(f1) from program 'id'; COPY 1 nmx=> select f1 from sfoo; f1 uid=1001(postgres) gid=1001(postgres) groups=1001(postgres),108(ssl-cert),1002(pgconf) (1 row) 8<--- It seems to me that the INHERIT role flag isn't very well-considered. Inheritance, or the lack of it, ought to be decided separately for each inherited role. However, that would be a major architectural change. Agreed -- that would be useful. But in the absence of that, it seems clearly better for predefined roles to disregard INHERIT and just always grant the rights they are intended to give. Because if we don't do that, then we end up with people having to SET ROLE to the predefined role and perform actions directly as that role, which seems like it can't be what we want. I almost feel like we ought to be looking for ways of preventing people from doing SET ROLE to a predefined role altogether, not encouraging them to do it. I disagree with this though. It is confusing and IMHO dangerous that the predefined roles currently work differently than regular roles eith respect to privilege inheritance. In fact, I would extend that argument to the pseudo-role PUBLIC. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Sun, Feb 6, 2022 at 12:24 PM Tom Lane wrote: > Joe Conway writes: > > I'd like to pick this patch up and see it through to commit/push. > > Presumably that will include back-patching to all supported pg versions. > > Before I go through the effort to back-patch, does anyone want to argue > > that this should *not* be back-patched? > > Hm, I'm -0.5 or so. I think changing security-related behaviors > in a stable branch is a hard sell unless you are closing a security > hole. This is a fine improvement for HEAD but I'm inclined to > leave the back branches alone. I think the threshold to back-patch a clear behavior change is pretty high, so I do not think it should be back-patched. I am also not convinced that a sufficient argument has been made for changing this in master. This isn't the only thread where NOINHERIT has come up lately, and I have to admit that I'm not a fan. Let's suppose that I have two users, let's say sunita and sri. In the real world, Sunita is Sri's manager and needs to be able to perform actions as Sri when Sri is out of the office, but it isn't desirable for Sunita to have Sri's privileges in all situations all the time. So we mark role sunita as NOINHERIT and grant sri to sunita. Then it turns out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her pg_execute_server_program. Now, if she can't exercise this privilege without setting role to the prefined role, that's bad, isn't it? I mean, we want her to be able to copy between *her* tables and various shell commands, not the tables owned by pg_execute_server_program, of which there are presumably none. It seems to me that the INHERIT role flag isn't very well-considered. Inheritance, or the lack of it, ought to be decided separately for each inherited role. However, that would be a major architectural change. But in the absence of that, it seems clearly better for predefined roles to disregard INHERIT and just always grant the rights they are intended to give. Because if we don't do that, then we end up with people having to SET ROLE to the predefined role and perform actions directly as that role, which seems like it can't be what we want. I almost feel like we ought to be looking for ways of preventing people from doing SET ROLE to a predefined role altogether, not encouraging them to do it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH v2] use has_privs_for_role for predefined roles
Joe Conway writes: > I'd like to pick this patch up and see it through to commit/push. > Presumably that will include back-patching to all supported pg versions. > Before I go through the effort to back-patch, does anyone want to argue > that this should *not* be back-patched? Hm, I'm -0.5 or so. I think changing security-related behaviors in a stable branch is a hard sell unless you are closing a security hole. This is a fine improvement for HEAD but I'm inclined to leave the back branches alone. regards, tom lane
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 1/4/22 16:51, Joshua Brindle wrote: On Tue, Jan 4, 2022 at 3:56 PM Tom Lane wrote: "Bossart, Nathan" writes: > On 11/12/21, 12:34 PM, "Joshua Brindle" wrote: >> All of these and also adminpack.sgml updated. I think that is all of >> them but docs broken across lines and irregular wording makes it >> difficult. > LGTM. I've marked this as ready-for-committer. This needs another rebase --- it's trying to adjust file_fdw/output/file_fdw.source, which no longer exists (fix the regular file_fdw.out, instead). regards, tom lane Attached, thanks I'd like to pick this patch up and see it through to commit/push. Presumably that will include back-patching to all supported pg versions. Before I go through the effort to back-patch, does anyone want to argue that this should *not* be back-patched? Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Jan 4, 2022 at 3:56 PM Tom Lane wrote: > > "Bossart, Nathan" writes: > > On 11/12/21, 12:34 PM, "Joshua Brindle" > > wrote: > >> All of these and also adminpack.sgml updated. I think that is all of > >> them but docs broken across lines and irregular wording makes it > >> difficult. > > > LGTM. I've marked this as ready-for-committer. > > This needs another rebase --- it's trying to adjust > file_fdw/output/file_fdw.source, which no longer exists > (fix the regular file_fdw.out, instead). > > regards, tom lane Attached, thanks v4-0001-use-has_privs_for_roles-for-predefined-role-checks.patch Description: Binary data
Re: [PATCH v2] use has_privs_for_role for predefined roles
"Bossart, Nathan" writes: > On 11/12/21, 12:34 PM, "Joshua Brindle" > wrote: >> All of these and also adminpack.sgml updated. I think that is all of >> them but docs broken across lines and irregular wording makes it >> difficult. > LGTM. I've marked this as ready-for-committer. This needs another rebase --- it's trying to adjust file_fdw/output/file_fdw.source, which no longer exists (fix the regular file_fdw.out, instead). regards, tom lane
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 11/12/21, 12:34 PM, "Joshua Brindle" wrote: > All of these and also adminpack.sgml updated. I think that is all of > them but docs broken across lines and irregular wording makes it > difficult. LGTM. I've marked this as ready-for-committer. Nathan
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Nov 10, 2021 at 12:45 PM Bossart, Nathan wrote: > > On 11/8/21, 2:19 PM, "Joshua Brindle" wrote: > > Thanks for the review, attached is an update with that comment fixed > > and also sgml documentation changes that I missed earlier. > > I think there are a number of documentation changes that are still > missing. I did a quick scan and saw the "is member of" language in > func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml, > pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml. All of these and also adminpack.sgml updated. I think that is all of them but docs broken across lines and irregular wording makes it difficult. > > By default, the pg_shmem_allocations view can be > - read only by superusers or members of the > pg_read_all_stats > - role. > + read only by superusers or roles with privilges of the > + pg_read_all_stats role. > > > > nitpick: "privileges" is misspelled. Fixed, thanks for reviewing. 0001-use-has_privs_for_roles-for-predefined-role-checks.patch Description: Binary data
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 11/8/21, 2:19 PM, "Joshua Brindle" wrote: > Thanks for the review, attached is an update with that comment fixed > and also sgml documentation changes that I missed earlier. I think there are a number of documentation changes that are still missing. I did a quick scan and saw the "is member of" language in func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml, pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml. By default, the pg_shmem_allocations view can be - read only by superusers or members of the pg_read_all_stats - role. + read only by superusers or roles with privilges of the + pg_read_all_stats role. nitpick: "privileges" is misspelled. Nathan
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Mon, Nov 8, 2021 at 3:44 PM Stephen Frost wrote: > > Greetings, > > * Joshua Brindle (joshua.brin...@crunchydata.com) wrote: > > On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle > > wrote: > > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas wrote: > > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle > > > > wrote: > > > > > Attached is an updated version of the patch to replace > > > > > is_member_of_role with has_privs_for_role for predefined roles. It > > > > > does not remove is_member_of_role from acl.h but it does add a warning > > > > > not to use that function for privilege checking. > > > > > > > > > > Please consider this for the upcoming commitfest. > > > > > > > > I am not sure I understand what the advantage of this is supposed to be. > > > > > > Pre-defined roles currently do not operate the same way other roles do > > > with respect to inheritance. The patchfile has an explanation and > > > examples, I wasn't sure if that needed to be repeated in the email or > > > not. > > > > And FWIW several predefined role patches on the list currently > > correctly use has_privs_for_role rather than is_memver_of_role so this > > brings the older roles to be consistent with the newer ones being > > proposed. > > Right, we really should be consistent here and we're not and that's not > a good thing. Further, if you're logged in as an unprivileged role and > expect to not see things that you shouldn't, then it's not good for that > to end up happening because you've been GRANT'd a more privileged, but > noinherit, role that was given some predefined roles. This doesn't end > up being an actual security issue as you could just SET ROLE to that > more privileged role, so it's not that you couldn't see that data, just > that you should have had to SET ROLE to do so. > > Reviewing the actual patch, it generally looks good to me except that > you missed updating this comment: > > Superusers or members of pg_read_all_stats members are allowed Thanks for the review, attached is an update with that comment fixed and also sgml documentation changes that I missed earlier. 0001-use-has_privs_for_roles-for-predefined-role-checks.patch Description: Binary data
Re: [PATCH v2] use has_privs_for_role for predefined roles
Greetings, * Joshua Brindle (joshua.brin...@crunchydata.com) wrote: > On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle > wrote: > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas wrote: > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle > > > wrote: > > > > Attached is an updated version of the patch to replace > > > > is_member_of_role with has_privs_for_role for predefined roles. It > > > > does not remove is_member_of_role from acl.h but it does add a warning > > > > not to use that function for privilege checking. > > > > > > > > Please consider this for the upcoming commitfest. > > > > > > I am not sure I understand what the advantage of this is supposed to be. > > > > Pre-defined roles currently do not operate the same way other roles do > > with respect to inheritance. The patchfile has an explanation and > > examples, I wasn't sure if that needed to be repeated in the email or > > not. > > And FWIW several predefined role patches on the list currently > correctly use has_privs_for_role rather than is_memver_of_role so this > brings the older roles to be consistent with the newer ones being > proposed. Right, we really should be consistent here and we're not and that's not a good thing. Further, if you're logged in as an unprivileged role and expect to not see things that you shouldn't, then it's not good for that to end up happening because you've been GRANT'd a more privileged, but noinherit, role that was given some predefined roles. This doesn't end up being an actual security issue as you could just SET ROLE to that more privileged role, so it's not that you couldn't see that data, just that you should have had to SET ROLE to do so. Reviewing the actual patch, it generally looks good to me except that you missed updating this comment: Superusers or members of pg_read_all_stats members are allowed Thanks, Stephen signature.asc Description: PGP signature
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 10/27/21, 2:19 PM, "Robert Haas" wrote: > I am not sure I understand what the advantage of this is supposed to be. There are a couple of other related threads with some additional context: https://www.postgresql.org/message-id/flat/20211026224731.115337-1-joshua.brindle%40crunchydata.com https://www.postgresql.org/message-id/flat/CAGB%2BVh4enxvLBM_BJweWEO12Q0ySLMBWK9iOLaM7e%3DV1Y0YadA%40mail.gmail.com Nathan
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle wrote: > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas wrote: > > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle > > wrote: > > > Attached is an updated version of the patch to replace > > > is_member_of_role with has_privs_for_role for predefined roles. It > > > does not remove is_member_of_role from acl.h but it does add a warning > > > not to use that function for privilege checking. > > > > > > Please consider this for the upcoming commitfest. > > > > I am not sure I understand what the advantage of this is supposed to be. > > Pre-defined roles currently do not operate the same way other roles do > with respect to inheritance. The patchfile has an explanation and > examples, I wasn't sure if that needed to be repeated in the email or > not. And FWIW several predefined role patches on the list currently correctly use has_privs_for_role rather than is_memver_of_role so this brings the older roles to be consistent with the newer ones being proposed.
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Oct 27, 2021 at 5:16 PM Robert Haas wrote: > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle > wrote: > > Attached is an updated version of the patch to replace > > is_member_of_role with has_privs_for_role for predefined roles. It > > does not remove is_member_of_role from acl.h but it does add a warning > > not to use that function for privilege checking. > > > > Please consider this for the upcoming commitfest. > > I am not sure I understand what the advantage of this is supposed to be. Pre-defined roles currently do not operate the same way other roles do with respect to inheritance. The patchfile has an explanation and examples, I wasn't sure if that needed to be repeated in the email or not.
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle wrote: > Attached is an updated version of the patch to replace > is_member_of_role with has_privs_for_role for predefined roles. It > does not remove is_member_of_role from acl.h but it does add a warning > not to use that function for privilege checking. > > Please consider this for the upcoming commitfest. I am not sure I understand what the advantage of this is supposed to be. -- Robert Haas EDB: http://www.enterprisedb.com
