Re: Proposal: allow database-specific role memberships - Mailing list pgsql-hackers

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