Re: Proposal: allow database-specific role memberships
On Wed, Sep 07, 2022 at 12:50:32PM +0500, Ibrar Ahmed wrote: > The patch requires a rebase, please do that. > > Hunk #5 succeeded at 454 (offset 28 lines). 1 out of 5 hunks FAILED > -- saving rejects to file doc/src/sgml/ref/grant.sgml.rej There has been no updates on this thread for one month, so this has been switched as RwF. -- Michael signature.asc Description: PGP signature
Re: Proposal: allow database-specific role memberships
On Mon, Jul 25, 2022 at 4:03 AM Kenaniah Cerny wrote: > Hi Antonin, > > Thank you again for the detailed review and questions. It was encouraging > to see the increasing level of nuance in this latest round. > > It's not clear from the explanation of the GRANT ... IN DATABASE ... / >> GRANT >> ... IN CURRENT DATABASE ... that, even if "membership in ... will be >> effective only when the recipient is connected to the database ...", the >> ADMIN option might not be "fully effective". > > > While I'm not entirely sure what you mean by fully effective, it sounds > like you may have expected a database-specific WITH ADMIN OPTION grant to > be able to take effect when connected to a different database (such as > being able to use db_4's database-specific grants when connected to db_3). > The documentation updated in this patch specifies that membership (for > database-specific grants) would be effective only when the grantee is > connected to the same database that the grant was issued for. > > In the case of attempting to make a role grant to db_4 from within db_3, > the user would need to have a cluster-wide admin option for the role being > granted, as the test case you referenced in your example aims to verify. > > I have added a couple of lines to the documentation included with this > patch in order to clarify. > > >> Specifically on the regression tests: >> >> * The function check_memberships() has no parameters - is there a >> reason not to use a view? >> > > I believe a view would work just as well -- this was an implementation > detail that was fashioned to match the pre-existing rolenames.sql file's > test format. > > >> * I'm not sure if the pg_auth_members catalog can contain InvalidOid >> in >> other columns than dbid. Thus I think that the query in >> check_memberships() only needs an outer JOIN for the pg_database >> table, >> while the other joins can be inner. >> > > This is probably true. The tests run just as well using inner joins for > pg_roles, as this latest version of the patch reflects. > > >> * In this part >> >> SET SESSION AUTHORIZATION role_read_12_noinherit; >> SELECT * FROM data; -- error >> SET ROLE role_read_12; -- error >> SELECT * FROM data; -- error >> >> I think you don't need to query the table again if the SET ROLE >> statement >> failed and the same query had been executed before the SET ROLE. > > > I left that last query in place as a sanity check to ensure that > role_read_12's privileges were indeed not in effect after the call to SET > ROLE. > > As we appear to now be working through the minutiae, it is my hope that > this will soon be ready for merge. > > - Kenaniah > The patch requires a rebase, please do that. Hunk #5 succeeded at 454 (offset 28 lines). 1 out of 5 hunks FAILED -- saving rejects to file doc/src/sgml/ref/grant.sgml.rej ... ... -- Ibrar Ahmed
Re: Proposal: allow database-specific role memberships
Hi Antonin, Thank you again for the detailed review and questions. It was encouraging to see the increasing level of nuance in this latest round. It's not clear from the explanation of the GRANT ... IN DATABASE ... / GRANT > ... IN CURRENT DATABASE ... that, even if "membership in ... will be > effective only when the recipient is connected to the database ...", the > ADMIN option might not be "fully effective". While I'm not entirely sure what you mean by fully effective, it sounds like you may have expected a database-specific WITH ADMIN OPTION grant to be able to take effect when connected to a different database (such as being able to use db_4's database-specific grants when connected to db_3). The documentation updated in this patch specifies that membership (for database-specific grants) would be effective only when the grantee is connected to the same database that the grant was issued for. In the case of attempting to make a role grant to db_4 from within db_3, the user would need to have a cluster-wide admin option for the role being granted, as the test case you referenced in your example aims to verify. I have added a couple of lines to the documentation included with this patch in order to clarify. > Specifically on the regression tests: > > * The function check_memberships() has no parameters - is there a > reason not to use a view? > I believe a view would work just as well -- this was an implementation detail that was fashioned to match the pre-existing rolenames.sql file's test format. > * I'm not sure if the pg_auth_members catalog can contain InvalidOid in > other columns than dbid. Thus I think that the query in > check_memberships() only needs an outer JOIN for the pg_database > table, > while the other joins can be inner. > This is probably true. The tests run just as well using inner joins for pg_roles, as this latest version of the patch reflects. > * In this part > > SET SESSION AUTHORIZATION role_read_12_noinherit; > SELECT * FROM data; -- error > SET ROLE role_read_12; -- error > SELECT * FROM data; -- error > > I think you don't need to query the table again if the SET ROLE > statement > failed and the same query had been executed before the SET ROLE. I left that last query in place as a sanity check to ensure that role_read_12's privileges were indeed not in effect after the call to SET ROLE. As we appear to now be working through the minutiae, it is my hope that this will soon be ready for merge. - Kenaniah database-role-memberships-v11.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Kenaniah Cerny wrote: > Rebased yet again... > > On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny wrote: > The "unsafe_tests" directory is where the pre-existing role tests were > located. According to the readme of the "unsafe_tests" directory, the tests > contained within are not run during "make installcheck" because they could > have side-effects that seem undesirable for a production installation. This > seemed like a reasonable location as the new tests that this patch > introduces also modifies the "state" of the database cluster by adding, > modifying, and removing roles & databases (including template1). ok, I missed the purpose of "unsafe_tests" so far, thanks. > Regarding roles_is_member_of(), the nuance is that role "A" in your example > would only be considered a member of role "B" (and by extension role "C") > when connected to the database in which "A" was granted database-specific > membership to "B". > Conversely, when connected to any other database, "A" would not be > considered to be a member of "B". > > This patch is designed to solve the scenarios in which one may want to > grant constrained access to a broader set of privileges. For example, > membership in "pg_read_all_data" effectively grants SELECT and USAGE rights > on everything (implicitly cluster-wide in today's implementation). By > granting a role membership to "pg_read_all_data" within the context of a > specific database, the grantee's read-everything privilege is effectively > constrained to just that specific database (as membership within > "pg_read_all_data" would not otherwise be held). ok, I tried to view the problem rather from general perspective. However, the permissions like "pg_read_all_data" are unusual in that they are rather strong and at the same time they are usually located at the top of the groups hierarchy. I've got no better idea how to solve the problem. A few more comments on the patch: * It's not clear from the explanation of the GRANT ... IN DATABASE ... / GRANT ... IN CURRENT DATABASE ... that, even if "membership in ... will be effective only when the recipient is connected to the database ...", the ADMIN option might not be "fully effective". I refer to the part of the regression tests starting with -- Ensure database-specific admin option can only grant within that database For example, "role_read_34" does have the ADMIN option for the "pg_read_all_data" role and for the "db_4" database: GRANT pg_read_all_data TO role_read_34 IN DATABASE db_4 WITH ADMIN OPTION; (in other words, "role_read_34" does have the database-specific membership in "pg_read_all_data"), but it cannot use the option (in other words, cannot use some ability resulting from that membership) unless the session to that database is active: \connect db_3 SET SESSION AUTHORIZATION role_read_34; ... GRANT pg_read_all_data TO role_granted IN CURRENT DATABASE; -- success GRANT pg_read_all_data TO role_granted IN DATABASE db_3; -- notice NOTICE: role "role_granted" is already a member of role "pg_read_all_data" in database "db_3" GRANT pg_read_all_data TO role_granted IN DATABASE db_4; -- error ERROR: must have admin option on role "pg_read_all_data" Specifically on the regression tests: * The function check_memberships() has no parameters - is there a reason not to use a view? * I'm not sure if the pg_auth_members catalog can contain InvalidOid in other columns than dbid. Thus I think that the query in check_memberships() only needs an outer JOIN for the pg_database table, while the other joins can be inner. * In this part SET SESSION AUTHORIZATION role_read_12_noinherit; SELECT * FROM data; -- error SET ROLE role_read_12; -- error SELECT * FROM data; -- error I think you don't need to query the table again if the SET ROLE statement failed and the same query had been executed before the SET ROLE. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: Proposal: allow database-specific role memberships
Rebased yet again... On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny wrote: > Hi Antonin, > > First of all, thank you so much for taking the time to review my patch. > I'll answer your questions in reverse order: > > The "unsafe_tests" directory is where the pre-existing role tests were > located. According to the readme of the "unsafe_tests" directory, the tests > contained within are not run during "make installcheck" because they could > have side-effects that seem undesirable for a production installation. This > seemed like a reasonable location as the new tests that this patch > introduces also modifies the "state" of the database cluster by adding, > modifying, and removing roles & databases (including template1). > > Regarding roles_is_member_of(), the nuance is that role "A" in your > example would only be considered a member of role "B" (and by extension > role "C") when connected to the database in which "A" was granted > database-specific membership to "B". Conversely, when connected to any > other database, "A" would not be considered to be a member of "B". > > This patch is designed to solve the scenarios in which one may want to > grant constrained access to a broader set of privileges. For example, > membership in "pg_read_all_data" effectively grants SELECT and USAGE rights > on everything (implicitly cluster-wide in today's implementation). By > granting a role membership to "pg_read_all_data" within the context of a > specific database, the grantee's read-everything privilege is effectively > constrained to just that specific database (as membership within > "pg_read_all_data" would not otherwise be held). > > A rebased version is attached. > > Thanks again! > > - Kenaniah > > On Wed, Jun 29, 2022 at 6:45 AM Antonin Houska wrote: > >> Kenaniah Cerny wrote: >> >> > Attached is a newly-rebased patch -- would love to get a review from >> someone whenever possible. >> >> I've picked this patch for a review. The patch currently does not apply >> to the >> master branch, so I could only read the diff. Following are my comments: >> >> * I think that roles_is_member_of() deserves a comment explaining why the >> code >> that you moved into append_role_memberships() needs to be called twice, >> i.e. once for global memberships and once for the database-specific >> ones. >> >> I think the reason is that if, for example, role "A" is a >> database-specific >> member of role "B" and "B" is a "global" member of role "C", then "A" >> should >> not be considered a member of "C", unless "A" is granted "C" >> explicitly. Is >> this behavior intended? >> >> Note that in this example, the "C" members are a superset of "B" >> members, >> and thus "C" should have weaker permissions on database objects than >> "B". What's then the reason to not consider "A" a member of "C"? If "C" >> gives its members some permissions of "B" (e.g. "pg_write_all_data"), >> then I >> think the roles hierarchy is poorly designed. >> >> A counter-example might help me to understand. >> >> * Why do you think that "unsafe_tests" is the appropriate name for the >> directory that contains regression tests? >> >> I can spend more time on the review if the patch gets rebased. >> >> -- >> Antonin Houska >> Web: https://www.cybertec-postgresql.com >> > database-role-memberships-v10.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Hi Antonin, First of all, thank you so much for taking the time to review my patch. I'll answer your questions in reverse order: The "unsafe_tests" directory is where the pre-existing role tests were located. According to the readme of the "unsafe_tests" directory, the tests contained within are not run during "make installcheck" because they could have side-effects that seem undesirable for a production installation. This seemed like a reasonable location as the new tests that this patch introduces also modifies the "state" of the database cluster by adding, modifying, and removing roles & databases (including template1). Regarding roles_is_member_of(), the nuance is that role "A" in your example would only be considered a member of role "B" (and by extension role "C") when connected to the database in which "A" was granted database-specific membership to "B". Conversely, when connected to any other database, "A" would not be considered to be a member of "B". This patch is designed to solve the scenarios in which one may want to grant constrained access to a broader set of privileges. For example, membership in "pg_read_all_data" effectively grants SELECT and USAGE rights on everything (implicitly cluster-wide in today's implementation). By granting a role membership to "pg_read_all_data" within the context of a specific database, the grantee's read-everything privilege is effectively constrained to just that specific database (as membership within "pg_read_all_data" would not otherwise be held). A rebased version is attached. Thanks again! - Kenaniah On Wed, Jun 29, 2022 at 6:45 AM Antonin Houska wrote: > Kenaniah Cerny wrote: > > > Attached is a newly-rebased patch -- would love to get a review from > someone whenever possible. > > I've picked this patch for a review. The patch currently does not apply to > the > master branch, so I could only read the diff. Following are my comments: > > * I think that roles_is_member_of() deserves a comment explaining why the > code > that you moved into append_role_memberships() needs to be called twice, > i.e. once for global memberships and once for the database-specific ones. > > I think the reason is that if, for example, role "A" is a > database-specific > member of role "B" and "B" is a "global" member of role "C", then "A" > should > not be considered a member of "C", unless "A" is granted "C" explicitly. > Is > this behavior intended? > > Note that in this example, the "C" members are a superset of "B" members, > and thus "C" should have weaker permissions on database objects than > "B". What's then the reason to not consider "A" a member of "C"? If "C" > gives its members some permissions of "B" (e.g. "pg_write_all_data"), > then I > think the roles hierarchy is poorly designed. > > A counter-example might help me to understand. > > * Why do you think that "unsafe_tests" is the appropriate name for the > directory that contains regression tests? > > I can spend more time on the review if the patch gets rebased. > > -- > Antonin Houska > Web: https://www.cybertec-postgresql.com > database-role-memberships-v9.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Kenaniah Cerny wrote: > Attached is a newly-rebased patch -- would love to get a review from someone > whenever possible. I've picked this patch for a review. The patch currently does not apply to the master branch, so I could only read the diff. Following are my comments: * I think that roles_is_member_of() deserves a comment explaining why the code that you moved into append_role_memberships() needs to be called twice, i.e. once for global memberships and once for the database-specific ones. I think the reason is that if, for example, role "A" is a database-specific member of role "B" and "B" is a "global" member of role "C", then "A" should not be considered a member of "C", unless "A" is granted "C" explicitly. Is this behavior intended? Note that in this example, the "C" members are a superset of "B" members, and thus "C" should have weaker permissions on database objects than "B". What's then the reason to not consider "A" a member of "C"? If "C" gives its members some permissions of "B" (e.g. "pg_write_all_data"), then I think the roles hierarchy is poorly designed. A counter-example might help me to understand. * Why do you think that "unsafe_tests" is the appropriate name for the directory that contains regression tests? I can spend more time on the review if the patch gets rebased. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: Proposal: allow database-specific role memberships
Patch doesn't apply again... [image: 1jfj7m.jpg]
Re: Proposal: allow database-specific role memberships
Hi all, cfbot is once again green as of the v7 patch: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3374 - Kenaniah
Re: Proposal: allow database-specific role memberships
Thanks Andres, An updated patch is attached. - Kenaniah On Mon, Mar 21, 2022 at 5:40 PM Andres Freund wrote: > Hi, > > On 2022-01-22 22:56:44 +0800, Julien Rouhaud wrote: > > On Sat, Jan 22, 2022 at 05:28:05AM -0800, Kenaniah Cerny wrote: > > > Thank you so much for the backtrace! > > > > > > This latest patch should address by moving the dumpRoleMembership call > to > > > before the pointer is closed. > > > > Thanks! The cfbot turned green since: > > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3374 > > red again: > https://cirrus-ci.com/task/5516269981007872?logs=test_world#L1480 > > Marked as waiting-on-author. > > - Andres > database-role-memberships-v7.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Hi, On 2022-01-22 22:56:44 +0800, Julien Rouhaud wrote: > On Sat, Jan 22, 2022 at 05:28:05AM -0800, Kenaniah Cerny wrote: > > Thank you so much for the backtrace! > > > > This latest patch should address by moving the dumpRoleMembership call to > > before the pointer is closed. > > Thanks! The cfbot turned green since: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3374 red again: https://cirrus-ci.com/task/5516269981007872?logs=test_world#L1480 Marked as waiting-on-author. - Andres
Re: Proposal: allow database-specific role memberships
Hi, On Sat, Jan 22, 2022 at 05:28:05AM -0800, Kenaniah Cerny wrote: > Thank you so much for the backtrace! > > This latest patch should address by moving the dumpRoleMembership call to > before the pointer is closed. Thanks! The cfbot turned green since: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3374
Re: Proposal: allow database-specific role memberships
Thank you so much for the backtrace!
This latest patch should address by moving the dumpRoleMembership call to
before the pointer is closed.
On Sat, Jan 22, 2022 at 1:11 AM Julien Rouhaud wrote:
> Hi,
>
> On Fri, Jan 21, 2022 at 07:01:21PM -0800, Kenaniah Cerny wrote:
> > Thanks for the feedback.
> >
> > I have attached an alternate version of the v5 patch that incorporates
> the
> > suggested changes to the documentation and DRYs up some of the acl.c code
> > for comparison. As for the databaseOid / InvalidOid parameter, I'm open
> to
> > any suggestions for how to make that even cleaner, but am currently at a
> > loss as to how that would look.
> >
> > CI is showing a failure to run pg_dump on just the Linux - Debian
> Bullseye
> > job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any
> > ideas as to where I should look in order to debug that?
>
> Did you try to reproduce it on some GNU/Linux system? FTR I had and I get
> a
> segfault in pg_dumpall:
>
> (gdb) bt
> #0 __pthread_kill_implementation (threadid=,
> signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
> #1 0x7f329e7e40cf in __pthread_kill_internal (signo=6,
> threadid=) at pthread_kill.c:78
> #2 0x7f329e7987a2 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #3 0x7f329e783449 in __GI_abort () at abort.c:79
> #4 0x7f329e7d85d8 in __libc_message (action=action@entry=do_abort,
> fmt=fmt@entry=0x7f329e90b6aa "%s\n") at ../sysdeps/posix/libc_fatal.c:155
> #5 0x7f329e7edcfa in malloc_printerr (str=str@entry=0x7f329e9092c3
> "free(): invalid pointer") at malloc.c:5536
> #6 0x7f329e7ef504 in _int_free (av=, p= out>, have_lock=0) at malloc.c:4327
> #7 0x7f329e7f1f81 in __GI___libc_free (mem=) at
> malloc.c:3279
> #8 0x7f329e7dbec5 in __GI__IO_free_backup_area
> (fp=fp@entry=0x561775f126c0)
> at genops.c:190
> #9 0x7f329e7db6af in _IO_new_file_overflow (f=0x561775f126c0, ch=-1)
> at fileops.c:758
> #10 0x7f329e7da7be in _IO_new_file_xsputn (n=2, data=,
> f=) at
> /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947
> #11 _IO_new_file_xsputn (f=0x561775f126c0, data=, n=2) at
> fileops.c:1197
> #12 0x7f329e7cfd32 in __GI__IO_fwrite (buf=0x7ffc90bb0ac0, size=1,
> count=2, fp=0x561775f126c0) at
> /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947
> #13 0x56177483c758 in flushbuffer (target=0x7ffc90bb0a90) at
> snprintf.c:310
> #14 0x56177483c4e8 in pg_vfprintf (stream=0x561775f126c0,
> fmt=0x561774840dec "\n\n", args=0x7ffc90bb0f00) at snprintf.c:259
> #15 0x56177483c5ce in pg_fprintf (stream=0x561775f126c0,
> fmt=0x561774840dec "\n\n") at snprintf.c:270
> #16 0x561774831893 in dumpRoleMembership (conn=0x561775f09600,
> databaseId=0x561775f152d2 "1") at pg_dumpall.c:991
> #17 0x561774832426 in dumpDatabases (conn=0x561775f09600) at
> pg_dumpall.c:1332
> #18 0x56177483049e in main (argc=3, argv=0x7ffc90bb1658) at
> pg_dumpall.c:596
>
> I didn't look in detail, but:
>
> @@ -1323,6 +1327,10 @@ dumpDatabases(PGconn *conn)
> exit_nicely(1);
> }
>
> + /* Dump database-specific roles if server is running 15.0 or later
> */
> + if (server_version >= 15)
> + dumpRoleMembership(conn, dbid);
> +
>
> Isn't that trying print to OPF after the possible fclose(OPF) a bit before
> and
> before it's reopened?
>
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 1e65c426b28e..a0beec6136e6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1642,11 +1642,10 @@ SCRAM-SHA-256$iteration count:
- Because user identities are cluster-wide,
- pg_auth_members
- is shared across all databases of a cluster: there is only one
- copy of pg_auth_members per cluster, not
- one per database.
+ User identities are cluster-wide, but role memberships can be either
+ cluster-wide or database-specific (as specified by the value of the
+ dbid column). The pg_auth_members
+ catalog is shared across all databases of a cluster.
@@ -1703,6 +1702,17 @@ SCRAM-SHA-256$iteration count:
roleid to others
+
+
+
+ dbid oid
+ (references pg_database.oid)
+
+
+ ID of the database that this membership is constrained to; zero if membership is cluster-wide
+
+
+
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index a897712de2e5..98bcfed5f507 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -93,6 +93,7 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
[ GRANTED BY role_specification ]
GRANT role_name [, ...] TO role_specification [, ...]
+[ IN DATABASE database_name | IN CURRENT DATABASE ]
[ WITH ADMIN OPTION ]
[ GRANTED BY role_specification ]
@@ -243,7 +244,23 @@ GRANT role_name [, ...] TO GRANT command grants membership
in a role to one or more other roles.
Re: Proposal: allow database-specific role memberships
Hi, On Fri, Jan 21, 2022 at 07:01:21PM -0800, Kenaniah Cerny wrote: > Thanks for the feedback. > > I have attached an alternate version of the v5 patch that incorporates the > suggested changes to the documentation and DRYs up some of the acl.c code > for comparison. As for the databaseOid / InvalidOid parameter, I'm open to > any suggestions for how to make that even cleaner, but am currently at a > loss as to how that would look. > > CI is showing a failure to run pg_dump on just the Linux - Debian Bullseye > job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any > ideas as to where I should look in order to debug that? Did you try to reproduce it on some GNU/Linux system? FTR I had and I get a segfault in pg_dumpall: (gdb) bt #0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 #1 0x7f329e7e40cf in __pthread_kill_internal (signo=6, threadid=) at pthread_kill.c:78 #2 0x7f329e7987a2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x7f329e783449 in __GI_abort () at abort.c:79 #4 0x7f329e7d85d8 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7f329e90b6aa "%s\n") at ../sysdeps/posix/libc_fatal.c:155 #5 0x7f329e7edcfa in malloc_printerr (str=str@entry=0x7f329e9092c3 "free(): invalid pointer") at malloc.c:5536 #6 0x7f329e7ef504 in _int_free (av=, p=, have_lock=0) at malloc.c:4327 #7 0x7f329e7f1f81 in __GI___libc_free (mem=) at malloc.c:3279 #8 0x7f329e7dbec5 in __GI__IO_free_backup_area (fp=fp@entry=0x561775f126c0) at genops.c:190 #9 0x7f329e7db6af in _IO_new_file_overflow (f=0x561775f126c0, ch=-1) at fileops.c:758 #10 0x7f329e7da7be in _IO_new_file_xsputn (n=2, data=, f=) at /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947 #11 _IO_new_file_xsputn (f=0x561775f126c0, data=, n=2) at fileops.c:1197 #12 0x7f329e7cfd32 in __GI__IO_fwrite (buf=0x7ffc90bb0ac0, size=1, count=2, fp=0x561775f126c0) at /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947 #13 0x56177483c758 in flushbuffer (target=0x7ffc90bb0a90) at snprintf.c:310 #14 0x56177483c4e8 in pg_vfprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n", args=0x7ffc90bb0f00) at snprintf.c:259 #15 0x56177483c5ce in pg_fprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n") at snprintf.c:270 #16 0x561774831893 in dumpRoleMembership (conn=0x561775f09600, databaseId=0x561775f152d2 "1") at pg_dumpall.c:991 #17 0x561774832426 in dumpDatabases (conn=0x561775f09600) at pg_dumpall.c:1332 #18 0x56177483049e in main (argc=3, argv=0x7ffc90bb1658) at pg_dumpall.c:596 I didn't look in detail, but: @@ -1323,6 +1327,10 @@ dumpDatabases(PGconn *conn) exit_nicely(1); } + /* Dump database-specific roles if server is running 15.0 or later */ + if (server_version >= 15) + dumpRoleMembership(conn, dbid); + Isn't that trying print to OPF after the possible fclose(OPF) a bit before and before it's reopened?
Re: Proposal: allow database-specific role memberships
Thanks for the feedback. I have attached an alternate version of the v5 patch that incorporates the suggested changes to the documentation and DRYs up some of the acl.c code for comparison. As for the databaseOid / InvalidOid parameter, I'm open to any suggestions for how to make that even cleaner, but am currently at a loss as to how that would look. CI is showing a failure to run pg_dump on just the Linux - Debian Bullseye job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any ideas as to where I should look in order to debug that? Kenaniah On Fri, Jan 21, 2022 at 3:04 PM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny wrote: > >> The latest rebased version of the patch is attached. >> > > As I was just reminded, we tend to avoid specifying specific PostgreSQL > versions in our documentation. We just say what the current version does. > Here, the note sentences at lines 62 and 63 don't follow documentation > norms on that score and should just be removed. The last two sentences > belong in the main description body, not a note. Thus the whole note goes > away. > > I don't think I really appreciated the value this feature would have when > combined with the predefined roles like pg_read_all_data and > pg_write_all_data. > > I suppose I don't really appreciate the warning about SUPERUSER, etc...or > at least why this warning is somehow specific to the per-database version > of role membership. If this warning is desirable it should be worded to > apply to role membership in general - and possibly proposed as a separate > patch for consideration. > > I didn't dive deeply but I think we now have at three places in the acl.c > code where after setting memlist from the system cache we perform nearly > identical for loops to generate the final roles_list. Possibly this needs > a refactor first so that you can introduce the per-database stuff more > succinctly. Basically, the vast majority of this commit is just adding > InvalidOid and databaseOid all other the place - with a few minor code > changes to accommodate the new arguments. The acl.c code should try and be > made done the same after post-refactor. > > David J. > > database-role-memberships-v5-alternate.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny wrote: > The latest rebased version of the patch is attached. > As I was just reminded, we tend to avoid specifying specific PostgreSQL versions in our documentation. We just say what the current version does. Here, the note sentences at lines 62 and 63 don't follow documentation norms on that score and should just be removed. The last two sentences belong in the main description body, not a note. Thus the whole note goes away. I don't think I really appreciated the value this feature would have when combined with the predefined roles like pg_read_all_data and pg_write_all_data. I suppose I don't really appreciate the warning about SUPERUSER, etc...or at least why this warning is somehow specific to the per-database version of role membership. If this warning is desirable it should be worded to apply to role membership in general - and possibly proposed as a separate patch for consideration. I didn't dive deeply but I think we now have at three places in the acl.c code where after setting memlist from the system cache we perform nearly identical for loops to generate the final roles_list. Possibly this needs a refactor first so that you can introduce the per-database stuff more succinctly. Basically, the vast majority of this commit is just adding InvalidOid and databaseOid all other the place - with a few minor code changes to accommodate the new arguments. The acl.c code should try and be made done the same after post-refactor. David J.
Re: Proposal: allow database-specific role memberships
The latest rebased version of the patch is attached. On Tue, Jan 11, 2022 at 11:01 PM Julien Rouhaud wrote: > Hi, > > On Thu, Dec 2, 2021 at 2:26 AM Kenaniah Cerny wrote: > > > > Attached is a rebased version of the patch that omits catversion.h in > order to avoid conflicts. > > Unfortunately even without that the patch doesn't apply anymore > according to the cfbot: http://cfbot.cputube.org/patch_36_3374.log > > 1 out of 3 hunks FAILED -- saving rejects to file > src/backend/parser/gram.y.rej > [...] > 2 out of 8 hunks FAILED -- saving rejects to file > src/bin/pg_dump/pg_dumpall.c.rej > > Could you send a rebased version? > > In the meantime I'm switching the patch to Waiting on Author. > database-role-memberships-v5.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Hi, On Thu, Dec 2, 2021 at 2:26 AM Kenaniah Cerny wrote: > > Attached is a rebased version of the patch that omits catversion.h in order > to avoid conflicts. Unfortunately even without that the patch doesn't apply anymore according to the cfbot: http://cfbot.cputube.org/patch_36_3374.log 1 out of 3 hunks FAILED -- saving rejects to file src/backend/parser/gram.y.rej [...] 2 out of 8 hunks FAILED -- saving rejects to file src/bin/pg_dump/pg_dumpall.c.rej Could you send a rebased version? In the meantime I'm switching the patch to Waiting on Author.
Re: Proposal: allow database-specific role memberships
Thank you for the advice! Attached is a rebased version of the patch that omits catversion.h in order to avoid conflicts. On Wed, Nov 17, 2021 at 6:17 AM Daniel Gustafsson wrote: > > On 28 Oct 2021, at 21:39, Kenaniah Cerny wrote: > > > Thank you Asif. A rebased patch is attached. > > This patch fails to apply yet again, this time due to a collusion in > catversion.h. I think it's fine to omit the change in catversion.h as it's > likely to repeatedly cause conflicts, and instead just mention it on the > thread. Any committer picking it up will know to perform the change > anyways, > so leaving it out can keep the patch from conflicting. > > -- > Daniel Gustafsson https://vmware.com/ > > database-role-memberships-v4.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
> On 28 Oct 2021, at 21:39, Kenaniah Cerny wrote: > Thank you Asif. A rebased patch is attached. This patch fails to apply yet again, this time due to a collusion in catversion.h. I think it's fine to omit the change in catversion.h as it's likely to repeatedly cause conflicts, and instead just mention it on the thread. Any committer picking it up will know to perform the change anyways, so leaving it out can keep the patch from conflicting. -- Daniel Gustafsson https://vmware.com/
Re: Proposal: allow database-specific role memberships
Thank you Asif. A rebased patch is attached. On Thu, Oct 28, 2021 at 11:04 AM Asif Rehman wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: not tested > Spec compliant: not tested > Documentation:not tested > > The patch does not apply on HEAD anymore. Looks like it needs to be > rebased. > > The new status of this patch is: Waiting on Author > database-role-memberships-v3.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested The patch does not apply on HEAD anymore. Looks like it needs to be rebased. The new status of this patch is: Waiting on Author
Re: Proposal: allow database-specific role memberships
Hi all, Thank you for the feedback so far! Attached is a completed implementation (including tests and documentation). Based on the feedback I have received so far, I will be submitting this implementation to the commitfest. Thanks again, Kenaniah On Mon, Oct 11, 2021 at 9:05 AM Stephen Frost wrote: > Greetings, > > * David G. Johnston (david.g.johns...@gmail.com) wrote: > > On Monday, October 11, 2021, Stephen Frost wrote: > > > I don't think "just don't grant access to those other databases" > > > is actually a proper answer- there is certainly a use-case for "I want > > > user X to have read access to all tables in *this* database, and also > > > allow them to connect to some other database but not have that same > > > level of access there." > > > > Sure, that has a benefit. But creating a second user for the other > > database and putting the onus on the user to use the correct credentials > > when logging into a particular database is a valid option - it is in > fact > > the status quo. Due to the complexity of adding a whole new grant > > dimension to the system the status quo is an appealing option. Annoyance > > factor aside it technically solves the per-database permissions problem > put > > forth. > > I disagree entirely that forcing users to have multiple accounts and to > deal with "using the correct one" is at all reasonable. That's an utter > hack that results in a given user having multiple different accounts- > something that gets really ugly to deal with in enterprise deployments > which use any kind of centralized authentication system. > > No, that's not a solution. Perhaps there's another way to implement > this capability that is simpler than what's proposed here, but saying > "just give each user two accounts" isn't a solution. Sure, it'll work > for existing released versions of PG, just like there's a lot of things > that people can do to hack around our deficiencies, but that doesn't > change that these are areas which we are lacking and where we should be > trying to provide a proper solution. > > Thanks, > > Stephen > database-role-memberships-v2.patch Description: Binary data
Re: Proposal: allow database-specific role memberships
Greetings, * David G. Johnston (david.g.johns...@gmail.com) wrote: > On Monday, October 11, 2021, Stephen Frost wrote: > > I don't think "just don't grant access to those other databases" > > is actually a proper answer- there is certainly a use-case for "I want > > user X to have read access to all tables in *this* database, and also > > allow them to connect to some other database but not have that same > > level of access there." > > Sure, that has a benefit. But creating a second user for the other > database and putting the onus on the user to use the correct credentials > when logging into a particular database is a valid option - it is in fact > the status quo. Due to the complexity of adding a whole new grant > dimension to the system the status quo is an appealing option. Annoyance > factor aside it technically solves the per-database permissions problem put > forth. I disagree entirely that forcing users to have multiple accounts and to deal with "using the correct one" is at all reasonable. That's an utter hack that results in a given user having multiple different accounts- something that gets really ugly to deal with in enterprise deployments which use any kind of centralized authentication system. No, that's not a solution. Perhaps there's another way to implement this capability that is simpler than what's proposed here, but saying "just give each user two accounts" isn't a solution. Sure, it'll work for existing released versions of PG, just like there's a lot of things that people can do to hack around our deficiencies, but that doesn't change that these are areas which we are lacking and where we should be trying to provide a proper solution. Thanks, Stephen signature.asc Description: PGP signature
Re: Proposal: allow database-specific role memberships
On Monday, October 11, 2021, Stephen Frost wrote: > > I don't think "just don't grant access to those other databases" > is actually a proper answer- there is certainly a use-case for "I want > user X to have read access to all tables in *this* database, and also > allow them to connect to some other database but not have that same > level of access there." > Sure, that has a benefit. But creating a second user for the other database and putting the onus on the user to use the correct credentials when logging into a particular database is a valid option - it is in fact the status quo. Due to the complexity of adding a whole new grant dimension to the system the status quo is an appealing option. Annoyance factor aside it technically solves the per-database permissions problem put forth. David J.
Re: Proposal: allow database-specific role memberships
On Mon, 11 Oct 2021 at 11:01, Stephen Frost wrote: > Having an ability to GRANT predefined roles within a particular database > is certainly something that I'd considered when adding the pg_read/write > data roles. I'm not super thrilled with the idea of adding a column to > pg_auth_members just for predefined roles though and I'm not sure that > such role membership makes sense for non-predefined roles. Would > welcome input from others as to if that's something that would make > sense or if folks have asked about that before. We'd need to carefully > think through what this means in terms of making sure we don't end up > with any loops too. > I think the ability to grant a role within a particular database would be useful. For example, imagine I have a dev/testing instance with multiple databases, each a copy of production modified in some way for different testing purposes. For example, one might be scrambled data (to make the testing data non- or less- confidential); another might be limited to data from the last year (to reduce the size of everything); another might be limited to 1% of all the customers (to reduce the size in a different way); and of course these could be combined. It’s easy to imagine that I might want to grant a user the ability to connect to all of these databases, but to have different privileges. For example, maybe they have read_confidential_data in the scrambled database but not in the reduced-but-not-scrambled databases. But maybe they have a lesser level of access to these databases, so just using the connect privilege won't do the job. I’ve already found it a bit weird that I can set per-role, per-database settings (e.g search_path), and of course privileges on individual objects, but not which roles the role is a member of. I haven’t thought about implementation at all however. The thought occurs to me that the union of all the role memberships in all the database should form a directed acyclic graph. In other words, you could not have X a member of Y (possibly indirectly) in one database while Y is a member of X in another database; the role memberships in each database would then be a subset of the complete graph of memberships.
Re: Proposal: allow database-specific role memberships
Greetings, * David G. Johnston (david.g.johns...@gmail.com) wrote: > On Sun, Oct 10, 2021 at 2:29 PM Kenaniah Cerny wrote: > > > In building off of prior art regarding the 'pg_read_all_data' and > > 'pg_write_all_data' roles, I would like to propose an extension to roles > > that would allow for database-specific role memberships (for the purpose of > > granting database-specific privileges) as an additional layer of > > abstraction. > > > > = Problem = > > > > There is currently no mechanism to grant the privileges afforded by the > > default roles on a per-database basis. This makes it difficult to cleanly > > accomplish permissions such as 'db_datareader' and 'db_datawriter' (which > > are database-level roles in SQL Server that respectively grant read and > > write access within a specific database). > > > > The recently-added 'pg_read_all_data' and 'pg_write_all_data' work > > similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide. > > My first impression is that this is more complex than just restricting > which databases users are allowed to connect to. The added flexibility > this would provide has some benefit but doesn't seem worth the added > complexity. Having an ability to GRANT predefined roles within a particular database is certainly something that I'd considered when adding the pg_read/write data roles. I'm not super thrilled with the idea of adding a column to pg_auth_members just for predefined roles though and I'm not sure that such role membership makes sense for non-predefined roles. Would welcome input from others as to if that's something that would make sense or if folks have asked about that before. We'd need to carefully think through what this means in terms of making sure we don't end up with any loops too. Does seem like we'd probably need to change more than just what's suggested here so that you could, for example, ask "is role X a member of role Y in database Z" without actually being connected to database Z. That's just a matter of adding some functions though- the existing functions would work with just the assumption that you're asking about within the current database. I don't think "just don't grant access to those other databases" is actually a proper answer- there is certainly a use-case for "I want user X to have read access to all tables in *this* database, and also allow them to connect to some other database but not have that same level of access there." Thanks, Stephen signature.asc Description: PGP signature
Re: Proposal: allow database-specific role memberships
On Sun, Oct 10, 2021 at 2:29 PM Kenaniah Cerny wrote: > In building off of prior art regarding the 'pg_read_all_data' and > 'pg_write_all_data' roles, I would like to propose an extension to roles > that would allow for database-specific role memberships (for the purpose of > granting database-specific privileges) as an additional layer of > abstraction. > > = Problem = > > There is currently no mechanism to grant the privileges afforded by the > default roles on a per-database basis. This makes it difficult to cleanly > accomplish permissions such as 'db_datareader' and 'db_datawriter' (which > are database-level roles in SQL Server that respectively grant read and > write access within a specific database). > > The recently-added 'pg_read_all_data' and 'pg_write_all_data' work > similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide. > My first impression is that this is more complex than just restricting which databases users are allowed to connect to. The added flexibility this would provide has some benefit but doesn't seem worth the added complexity. David J.
Proposal: allow database-specific role memberships
Hi all, In building off of prior art regarding the 'pg_read_all_data' and 'pg_write_all_data' roles, I would like to propose an extension to roles that would allow for database-specific role memberships (for the purpose of granting database-specific privileges) as an additional layer of abstraction. = Problem = There is currently no mechanism to grant the privileges afforded by the default roles on a per-database basis. This makes it difficult to cleanly accomplish permissions such as 'db_datareader' and 'db_datawriter' (which are database-level roles in SQL Server that respectively grant read and write access within a specific database). The recently-added 'pg_read_all_data' and 'pg_write_all_data' work similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide. = Proposal = I propose an extension to the GRANT / REVOKE syntax as well as an additional column within pg_auth_members in order to track role memberships that are only effective within the specified database. Role membership (and subsequent privileges) would be calculated using the following algorithm: - Check for regular (cluster-wide) role membership (the way it works today) - Check for database-specific role membership based on the currently-connected database Attached is a proof of concept patch that implements this. = Implementation Notes = - A new column (pg_auth_members.dbid) in the system catalog that is set to InvalidOid for regular role memberships, or the oid of the given database for database-specific role memberships. - GRANT / REVOKE syntax has been extended to include the ability to specify a database-specific role membership: - "IN DATABASE database_name" would cause the GRANT to be applicable only within the specified database. - "IN CURRENT DATABASE" would cause the GRANT to be applicable only within the currently-connected database. - Omission of the clause would create a regular (cluster-wide) role membership (the way it works today). The proposed syntax (applies to REVOKE as well): GRANT role_name [, ...] TO role_specification [, ...] [ IN DATABASE database_name | IN CURRENT DATABASE ] [ WITH ADMIN OPTION ] [ GRANTED BY role_specification ] - DROP DATABASE has been updated to clean up any database-specific role memberships that are associated with the database being dropped. - pg_dump_all will dump database-specific role memberships using the "IN CURRENT DATABASE" syntax. (pg_dump has not been modified) - is_admin_of_role()'s signature has been updated to include the oid of the database being checked as a third argument. This now returns true if the member has WITH ADMIN OPTION either globally or for the database given. - roles_is_member_of() will additionally include any database-specific role memberships for the database being checked in its result set. = Example = CREATE DATABASE accounting; CREATE DATABASE sales; CREATE ROLE alice; CREATE ROLE bob; -- Alice is granted read-all privileges cluster-wide (nothing new here) GRANT pg_read_all_data TO alice; -- Bob is granted read-all privileges to just the accounting database GRANT pg_read_all_data TO bob IN DATABASE accounting; = Final Thoughts = This is my first attempt at contributing code to the project, and I would not self-identify as a C programmer. I wanted to get a sense for how receptive the contributors and community would be to this proposal and whether there were any concerns or preferred alternatives before I further embark on a fool's errand. Thoughts? Thanks, -- Kenaniah poc-database-role-membership-v1.patch Description: Binary data
