Proposal: allow database-specific role memberships

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

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

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:

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:

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:

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:

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:

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 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: