Thread: 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
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
Attachment
On Sun, Oct 10, 2021 at 2:29 PM Kenaniah Cerny <kenaniah@gmail.com> 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.
Greetings, * David G. Johnston (david.g.johnston@gmail.com) wrote: > On Sun, Oct 10, 2021 at 2:29 PM Kenaniah Cerny <kenaniah@gmail.com> 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
Attachment
On Mon, 11 Oct 2021 at 11:01, Stephen Frost <sfrost@snowman.net> 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.
On Monday, October 11, 2021, Stephen Frost <sfrost@snowman.net> 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.
Greetings, * David G. Johnston (david.g.johnston@gmail.com) wrote: > On Monday, October 11, 2021, Stephen Frost <sfrost@snowman.net> 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
Attachment
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 <sfrost@snowman.net> wrote:
Greetings,
* David G. Johnston (david.g.johnston@gmail.com) wrote:
> On Monday, October 11, 2021, Stephen Frost <sfrost@snowman.net> 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
Attachment
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
Thank you Asif. A rebased patch is attached.
On Thu, Oct 28, 2021 at 11:04 AM Asif Rehman <asifr.rehman@gmail.com> 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
Attachment
> On 28 Oct 2021, at 21:39, Kenaniah Cerny <kenaniah@gmail.com> 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/
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 <daniel@yesql.se> wrote:
> On 28 Oct 2021, at 21:39, Kenaniah Cerny <kenaniah@gmail.com> 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/
Attachment
Hi, On Thu, Dec 2, 2021 at 2:26 AM Kenaniah Cerny <kenaniah@gmail.com> 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.
The latest rebased version of the patch is attached.
On Tue, Jan 11, 2022 at 11:01 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
On Thu, Dec 2, 2021 at 2:26 AM Kenaniah Cerny <kenaniah@gmail.com> 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.
Attachment
On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny <kenaniah@gmail.com> 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.
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.johnston@gmail.com> wrote:
On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny <kenaniah@gmail.com> 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.
Attachment
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=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 #1 0x00007f329e7e40cf in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78 #2 0x00007f329e7987a2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x00007f329e783449 in __GI_abort () at abort.c:79 #4 0x00007f329e7d85d8 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7f329e90b6aa "%s\n") at ../sysdeps/posix/libc_fatal.c:155 #5 0x00007f329e7edcfa in malloc_printerr (str=str@entry=0x7f329e9092c3 "free(): invalid pointer") at malloc.c:5536 #6 0x00007f329e7ef504 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:4327 #7 0x00007f329e7f1f81 in __GI___libc_free (mem=<optimized out>) at malloc.c:3279 #8 0x00007f329e7dbec5 in __GI__IO_free_backup_area (fp=fp@entry=0x561775f126c0) at genops.c:190 #9 0x00007f329e7db6af in _IO_new_file_overflow (f=0x561775f126c0, ch=-1) at fileops.c:758 #10 0x00007f329e7da7be in _IO_new_file_xsputn (n=2, data=<optimized out>, f=<optimized out>) 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=<optimized out>, n=2) at fileops.c:1197 #12 0x00007f329e7cfd32 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 0x000056177483c758 in flushbuffer (target=0x7ffc90bb0a90) at snprintf.c:310 #14 0x000056177483c4e8 in pg_vfprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n", args=0x7ffc90bb0f00) at snprintf.c:259 #15 0x000056177483c5ce in pg_fprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n") at snprintf.c:270 #16 0x0000561774831893 in dumpRoleMembership (conn=0x561775f09600, databaseId=0x561775f152d2 "1") at pg_dumpall.c:991 #17 0x0000561774832426 in dumpDatabases (conn=0x561775f09600) at pg_dumpall.c:1332 #18 0x000056177483049e 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 >= 150000) + dumpRoleMembership(conn, dbid); + Isn't that trying print to OPF after the possible fclose(OPF) a bit before and before it's reopened?
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 <rjuju123@gmail.com> 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=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1 0x00007f329e7e40cf in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
#2 0x00007f329e7987a2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3 0x00007f329e783449 in __GI_abort () at abort.c:79
#4 0x00007f329e7d85d8 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7f329e90b6aa "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#5 0x00007f329e7edcfa in malloc_printerr (str=str@entry=0x7f329e9092c3 "free(): invalid pointer") at malloc.c:5536
#6 0x00007f329e7ef504 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:4327
#7 0x00007f329e7f1f81 in __GI___libc_free (mem=<optimized out>) at malloc.c:3279
#8 0x00007f329e7dbec5 in __GI__IO_free_backup_area (fp=fp@entry=0x561775f126c0) at genops.c:190
#9 0x00007f329e7db6af in _IO_new_file_overflow (f=0x561775f126c0, ch=-1) at fileops.c:758
#10 0x00007f329e7da7be in _IO_new_file_xsputn (n=2, data=<optimized out>, f=<optimized out>) 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=<optimized out>, n=2) at fileops.c:1197
#12 0x00007f329e7cfd32 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 0x000056177483c758 in flushbuffer (target=0x7ffc90bb0a90) at snprintf.c:310
#14 0x000056177483c4e8 in pg_vfprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n", args=0x7ffc90bb0f00) at snprintf.c:259
#15 0x000056177483c5ce in pg_fprintf (stream=0x561775f126c0, fmt=0x561774840dec "\n\n") at snprintf.c:270
#16 0x0000561774831893 in dumpRoleMembership (conn=0x561775f09600, databaseId=0x561775f152d2 "1") at pg_dumpall.c:991
#17 0x0000561774832426 in dumpDatabases (conn=0x561775f09600) at pg_dumpall.c:1332
#18 0x000056177483049e 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 >= 150000)
+ dumpRoleMembership(conn, dbid);
+
Isn't that trying print to OPF after the possible fclose(OPF) a bit before and
before it's reopened?
Attachment
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
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
Thanks Andres,
An updated patch is attached.
- Kenaniah
On Mon, Mar 21, 2022 at 5:40 PM Andres Freund <andres@anarazel.de> 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
Attachment
Hi all,
- Kenaniah
Patch doesn't apply again...
Attachment
I love that jpg! I'm saving it.
Attached is a newly-rebased patch -- would love to get a review from someone whenever possible.
Thanks,
- Kenaniah
On Fri, Apr 1, 2022 at 7:32 AM Greg Stark <stark@mit.edu> wrote:
Patch doesn't apply again...
Attachment
Kenaniah Cerny <kenaniah@gmail.com> 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
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 <ah@cybertec.at> wrote:
Kenaniah Cerny <kenaniah@gmail.com> 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
Attachment
Rebased yet again...
On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny <kenaniah@gmail.com> 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!- KenaniahOn Wed, Jun 29, 2022 at 6:45 AM Antonin Houska <ah@cybertec.at> wrote:Kenaniah Cerny <kenaniah@gmail.com> 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
Attachment
Kenaniah Cerny <kenaniah@gmail.com> wrote:
> Rebased yet again...
>
> On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny <kenaniah@gmail.com> 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
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
Attachment
On Mon, Jul 25, 2022 at 4:03 AM Kenaniah Cerny <kenaniah@gmail.com> 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
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