Hi hackers, 6198420 ensured that has_privs_of_role() is used for predefined roles, which means that the role inheritance hierarchy is checked instead of mere role membership. However, inheritance is still not respected for pg_hba.conf. Specifically, "samerole", "samegroup", and "+" still use is_member_of_role_nosuper().
The attached patch introduces has_privs_of_role_nosuper() and uses it for the aforementioned pg_hba.conf functionality. I think this is desirable for consistency. If a role_a has membership in role_b but none of its privileges (i.e., NOINHERIT), does it make sense that role_a should match +role_b in pg_hba.conf? It is true that role_a could always "SET ROLE role_b", and with this change, the user won't even have the ability to log in to run SET ROLE. But I'm not sure if that's a strong enough argument for deviating from the standard role privilege checks. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 5c1a5aaaff949fc25afaa6856ca2a85a54c8efdc Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Fri, 1 Apr 2022 11:40:51 -0700 Subject: [PATCH v1 1/1] Use has_privs_of_role for samerole, samegroup, and + in pg_hba.conf. 6198420 ensured that has_privs_of_role() is used for predefined roles, which means that the role privilege inheritance hierarchy is checked instead of mere role membership. However, inheritance is still not respected for pg_hba.conf. This change alters the authentication logic to consider role privileges instead of just role membership. Do not back-patch. Author: Nathan Bossart --- doc/src/sgml/client-auth.sgml | 18 +++++++++--------- src/backend/libpq/hba.c | 19 ++++++++++--------- src/backend/utils/adt/acl.c | 23 +++++++++++++++++++++++ src/include/utils/acl.h | 1 + 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 142b0affcb..5c63842e52 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -221,12 +221,12 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl The value <literal>sameuser</literal> specifies that the record matches if the requested database has the same name as the requested user. The value <literal>samerole</literal> specifies that - the requested user must be a member of the role with the same + the requested user must have privileges of the role with the same name as the requested database. (<literal>samegroup</literal> is an obsolete but still accepted spelling of <literal>samerole</literal>.) - Superusers are not considered to be members of a role for the - purposes of <literal>samerole</literal> unless they are explicitly - members of the role, directly or indirectly, and not just by + Superusers are not considered to have privileges of a role for the + purposes of <literal>samerole</literal> unless they explicitly have + privileges of the role, directly or indirectly, and not just by virtue of being a superuser. The value <literal>replication</literal> specifies that the record matches if a physical replication connection is requested, however, it @@ -252,10 +252,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl database user, or a group name preceded by <literal>+</literal>. (Recall that there is no real distinction between users and groups in <productname>PostgreSQL</productname>; a <literal>+</literal> mark really means - <quote>match any of the roles that are directly or indirectly members + <quote>match any of the roles that directly or indirectly have privileges of this role</quote>, while a name without a <literal>+</literal> mark matches only that specific role.) For this purpose, a superuser is only - considered to be a member of a role if they are explicitly a member + considered to have privileges of a role if they explicitly have privileges of the role, directly or indirectly, and not just by virtue of being a superuser. Multiple user names can be supplied by separating them with commas. @@ -788,9 +788,9 @@ host all all 192.168.0.0/16 ident map=omicro # If these are the only three lines for local connections, they will # allow local users to connect only to their own databases (databases # with the same name as their database user name) except for administrators -# and members of role "support", who can connect to all databases. The file -# $PGDATA/admins contains a list of names of administrators. Passwords -# are required in all cases. +# and roles with privileges of role "support", who can connect to all +# databases. The file $PGDATA/admins contains a list of names of +# administrators. Passwords are required in all cases. # # TYPE DATABASE USER ADDRESS METHOD local sameuser all md5 diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f8393ca8ed..247118fa30 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -546,13 +546,13 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines, /* - * Does user belong to role? + * Does user have privileges of role? * * userid is the OID of the role given as the attempted login identifier. - * We check to see if it is a member of the specified role name. + * We check to see if it has privileges of the specified role name. */ static bool -is_member(Oid userid, const char *role) +has_privs(Oid userid, const char *role) { Oid roleid; @@ -565,11 +565,12 @@ is_member(Oid userid, const char *role) return false; /* if target role not exist, say "no" */ /* - * See if user is directly or indirectly a member of role. For this - * purpose, a superuser is not considered to be automatically a member of - * the role, so group auth only applies to explicit membership. + * See if user directly or indirectly has privileges of role. For this + * purpose, a superuser is not considered to automatically have + * privileges of the role, so group auth only applies to explicit + * privileges. */ - return is_member_of_role_nosuper(userid, roleid); + return has_privs_of_role_nosuper(userid, roleid); } /* @@ -586,7 +587,7 @@ check_role(const char *role, Oid roleid, List *tokens) tok = lfirst(cell); if (!tok->quoted && tok->string[0] == '+') { - if (is_member(roleid, tok->string + 1)) + if (has_privs(roleid, tok->string + 1)) return true; } else if (token_matches(tok, role) || @@ -627,7 +628,7 @@ check_db(const char *dbname, const char *role, Oid roleid, List *tokens) else if (token_is_keyword(tok, "samegroup") || token_is_keyword(tok, "samerole")) { - if (is_member(roleid, dbname)) + if (has_privs(roleid, dbname)) return true; } else if (token_is_keyword(tok, "replication")) diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 83cf7ac9ff..061fa171fc 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -4855,6 +4855,29 @@ has_privs_of_role(Oid member, Oid role) } +/* + * Does member have the privileges of role, not considering superuserness? + * + * This is identical to has_privs_of_role except we ignore superuser + * status. + */ +bool +has_privs_of_role_nosuper(Oid member, Oid role) +{ + /* Fast path for simple case */ + if (member == role) + return true; + + /* + * Find all the roles that member has the privileges of, including + * multi-level recursion, then see if target role is any one of them. + */ + return list_member_oid(roles_is_member_of(member, ROLERECURSE_PRIVS, + InvalidOid, NULL), + role); +} + + /* * Is member a member of role (directly or indirectly)? * diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h index 91ce3d8e9c..bf9f22124e 100644 --- a/src/include/utils/acl.h +++ b/src/include/utils/acl.h @@ -208,6 +208,7 @@ extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId, extern int aclmembers(const Acl *acl, Oid **roleids); extern bool has_privs_of_role(Oid member, Oid role); +extern bool has_privs_of_role_nosuper(Oid member, Oid role); extern bool is_member_of_role(Oid member, Oid role); extern bool is_member_of_role_nosuper(Oid member, Oid role); extern bool is_admin_of_role(Oid member, Oid role); -- 2.25.1