On Thu, Oct 06, 2022 at 07:33:46AM -0400, Joe Conway wrote:
> On 10/6/22 04:09, Michael Paquier wrote:
>> This patch looks simple, but it is a very sensitive area so I think
>> that we should be really careful. pg_hba.conf does not have a lot of
>> test coverage, so I'd really prefer if we add something to see the
>> difference of behavior and check the behavior that we are switching
>> here.
>
> Agreed
Here is a new version of the patch with a test.
>> Joe, you are registered as a reviewer and committer of this patch, by
>> the way. Are you planning to look at it?
>
> I am meaning to get to it, but as you say wanted to spend some time to
> understand the nuances and life keeps getting in the way. I will try to
> prioritize it over the next week.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 8ab3b80b61790045277f5c9fbc6214cad5c14b58 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandboss...@gmail.com>
Date: Fri, 1 Apr 2022 11:40:51 -0700
Subject: [PATCH v2 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/meson.build | 1 +
src/test/authentication/t/004_privs.pl | 101 +++++++++++++++++++++++++
6 files changed, 145 insertions(+), 18 deletions(-)
create mode 100644 src/test/authentication/t/004_privs.pl
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/meson.build b/src/test/authentication/meson.build
index c2b48c43c9..a98b19158c 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -7,6 +7,7 @@ tests += {
't/001_password.pl',
't/002_saslprep.pl',
't/003_peer.pl',
+ 't/004_privs.pl',
],
},
}
diff --git a/src/test/authentication/t/004_privs.pl b/src/test/authentication/t/004_privs.pl
new file mode 100644
index 0000000000..40b7e9f6e5
--- /dev/null
+++ b/src/test/authentication/t/004_privs.pl
@@ -0,0 +1,101 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+# Tests for role inheritance with pg_hba.conf.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+ my $node = shift;
+ my $database = shift;
+ my $role = shift;
+ my $hba_method = shift;
+
+ unlink($node->data_dir . '/pg_hba.conf');
+ $node->append_conf('pg_hba.conf', "local $database $role $hba_method");
+ $node->reload;
+ return;
+}
+
+# Test access for a connection string, useful to wrap all tests into one.
+# Extra named parameters are passed to connect_ok/fails as-is.
+sub test_conn
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my ($node, $connstr, $method, $expected_res, %params) = @_;
+ my $status_string = 'failed';
+ $status_string = 'success' if ($expected_res eq 0);
+
+ my $testname =
+ "authentication $status_string for method $method, connstr $connstr";
+
+ if ($expected_res eq 0)
+ {
+ $node->connect_ok($connstr, $testname, %params);
+ }
+ else
+ {
+ # No checks of the error message, only the status code.
+ $node->connect_fails($connstr, $testname, %params);
+ }
+}
+
+# Initialize primary node
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf('postgresql.conf', "log_connections = on\n");
+$node->start;
+
+# Create database and roles for tests
+$node->safe_psql('postgres', "CREATE DATABASE role1;");
+$node->safe_psql('postgres', "CREATE ROLE role1 LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role2 LOGIN SUPERUSER INHERIT IN ROLE role1 PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role3 LOGIN SUPERUSER NOINHERIT IN ROLE role1 PASSWORD 'pass';");
+
+# Test role inheritance is respected for +
+$ENV{"PGPASSWORD"} = 'pass';
+reset_pg_hba($node, 'all', '+role1', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samerole
+$ENV{"PGDATABASE"} = 'role1';
+reset_pg_hba($node, 'samerole', 'all', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samegroup
+reset_pg_hba($node, 'samegroup', 'all', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+done_testing();
--
2.25.1
