Date: Fri, 7 Oct 2022 12:44:30 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Joe Conway <mail@joeconway.com>,
	Joshua Brindle <joshua.brindle@crunchydata.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: use has_privs_of_role() for pg_hba.conf
Message-ID: <20221007194430.GA550749@nathanxps13>
References: <20220401220648.GA615994@nathanxps13>
 <CAGB+Vh5epaDEu7aRokcuc_a7yG0B49u9ebpdViJHpyFN-exJtQ@mail.gmail.com>
 <20220404142551.GB663209@nathanxps13>
 <Yz6NRrfd7n2AXoHh@paquier.xyz>
 <491dba16-55df-d5fd-1ae7-01b5140987f0@joeconway.com>
 <20221006174343.GA444656@nathanxps13>
 <Yz+Jt9537vjfO733@paquier.xyz>
 <20221007032711.GA499771@nathanxps13>
 <Yz/Ii6dr1CSu4Bnv@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="TB36FDmn/VVEgNH/"
Content-Disposition: inline
In-Reply-To: <Yz/Ii6dr1CSu4Bnv@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20221007194430.GA550749%40nathanxps13>
Precedence: bulk


--TB36FDmn/VVEgNH/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Oct 07, 2022 at 03:34:51PM +0900, Michael Paquier wrote:
> Thanks.  I would perhaps use names less generic than role{1,2,3} for
> the roles or "role1" for the database name, but the logic looks
> sound.

Here is a new version with more descriptive role names.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--TB36FDmn/VVEgNH/
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patch"

From 908944cead44868edbc06118fed0232087b73583 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 1 Apr 2022 11:40:51 -0700
Subject: [PATCH v4 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 +
 src/test/authentication/t/001_password.pl | 48 +++++++++++++++++++++++
 5 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c6f1b70fd3..b06b57f169 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 4637426d62..faa675f4af 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -547,13 +547,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;
 
@@ -566,11 +566,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);
 }
 
 /*
@@ -587,7 +588,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) ||
@@ -628,7 +629,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 4fac402e5b..cb5587d926 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -4928,6 +4928,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 9a4df3a5da..eded5e0f96 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,6 +209,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);
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 93df77aa4e..08c2fd8b01 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -200,4 +200,52 @@ append_to_file(
 
 test_conn($node, 'user=md5_role', 'password from pgpass', 0);
 
+unlink($pgpassfile);
+delete $ENV{"PGPASSFILE"};
+
+# Create database and roles for inheritance tests
+reset_pg_hba($node, 'all', 'all', 'trust');
+$node->safe_psql('postgres', "CREATE DATABASE grouprole;");
+$node->safe_psql('postgres', "CREATE ROLE grouprole LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE memberrole_inherit LOGIN SUPERUSER INHERIT IN ROLE grouprole PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE memberrole_noinherit LOGIN SUPERUSER NOINHERIT IN ROLE grouprole PASSWORD 'pass';");
+
+# Test role inheritance is respected for +
+$ENV{"PGPASSWORD"} = 'pass';
+reset_pg_hba($node, 'all', '+grouprole', 'scram-sha-256');
+test_conn($node, 'user=grouprole', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="grouprole" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_inherit', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="memberrole_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_noinherit', 'scram-sha-256', 2,
+	log_unlike =>
+	  [qr/connection authenticated: identity="memberrole_noinherit" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samerole
+$ENV{"PGDATABASE"} = 'grouprole';
+reset_pg_hba($node, 'samerole', 'all', 'scram-sha-256');
+test_conn($node, 'user=grouprole', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="grouprole" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_inherit', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="memberrole_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_noinherit', 'scram-sha-256', 2,
+	log_unlike =>
+	  [qr/connection authenticated: identity="memberrole_noinherit" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samegroup
+reset_pg_hba($node, 'samegroup', 'all', 'scram-sha-256');
+test_conn($node, 'user=grouprole', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="grouprole" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_inherit', 'scram-sha-256', 0,
+	log_like =>
+	  [qr/connection authenticated: identity="memberrole_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=memberrole_noinherit', 'scram-sha-256', 2,
+	log_unlike =>
+	  [qr/connection authenticated: identity="memberrole_noinherit" method=scram-sha-256/]);
+
 done_testing();
-- 
2.25.1


--TB36FDmn/VVEgNH/--