use has_privs_of_role() for pg_hba.conf
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
Attachments:
v1-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patchtext/x-diff; charset=us-asciiDownload
From 5c1a5aaaff949fc25afaa6856ca2a85a54c8efdc Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@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
On Fri, Apr 1, 2022 at 6:06 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
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?
Good catch, I think this is a logical followup to the previous
has_privs_of_role patch.
Reviewed and +1
On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote:
Good catch, I think this is a logical followup to the previous
has_privs_of_role patch.Reviewed and +1
Thanks! I created a commitfest entry for this:
https://commitfest.postgresql.org/38/3609/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Apr 04, 2022 at 07:25:51AM -0700, Nathan Bossart wrote:
On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote:
Good catch, I think this is a logical followup to the previous
has_privs_of_role patch.Reviewed and +1
Thanks! I created a commitfest entry for this:
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. What I have just committed in 051b096 would help a bit here,
actually, and changing pg_hba.conf rules with rule reload is cheap.
Joe, you are registered as a reviewer and committer of this patch, by
the way. Are you planning to look at it?
--
Michael
On 10/6/22 04:09, Michael Paquier wrote:
On Mon, Apr 04, 2022 at 07:25:51AM -0700, Nathan Bossart wrote:
On Mon, Apr 04, 2022 at 09:36:13AM -0400, Joshua Brindle wrote:
Good catch, I think this is a logical followup to the previous
has_privs_of_role patch.Reviewed and +1
Thanks! I created a commitfest entry for this:
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
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.
Joe
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
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
Attachments:
v2-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patchtext/x-diff; charset=us-asciiDownload
From 8ab3b80b61790045277f5c9fbc6214cad5c14b58 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@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
On Thu, Oct 06, 2022 at 10:43:43AM -0700, Nathan Bossart wrote:
Here is a new version of the patch with a test.
Thanks, that helps a lot. Now I grab the difference even if your
previous patch was already switching the documentation to tell exactly
that. On the ground of 6198420, it looks indeed strange to not do the
same for pg_hba.conf. That makes the whole story more consistent, for
one.
+$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';");
So this comes down to role3, where HEAD allows a connection as long as
it is a member of role1 for +role1, samegroup and samerole, but the
patch would prevent the connection when role3 does not inherit the
permissions of role1, even if it is a superuser.samegroup is a synonym of samerole, but fine by me to keep the full
coverage and all three sets.
Rather than putting that in a separate script, which means
initializing a new node, etc. could it be better to put that in
001_password.pl instead? It would be cheaper.
--
Michael
On Fri, Oct 07, 2022 at 11:06:47AM +0900, Michael Paquier wrote:
Rather than putting that in a separate script, which means
initializing a new node, etc. could it be better to put that in
001_password.pl instead? It would be cheaper.
Works for me.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patchtext/x-diff; charset=us-asciiDownload
From 3a274e6b4f7fc8bf7eda3579d8bb2378c73f2098 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 1 Apr 2022 11:40:51 -0700
Subject: [PATCH v3 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..dea51c6bad 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 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
On Thu, Oct 06, 2022 at 08:27:11PM -0700, Nathan Bossart wrote:
On Fri, Oct 07, 2022 at 11:06:47AM +0900, Michael Paquier wrote:
Rather than putting that in a separate script, which means
initializing a new node, etc. could it be better to put that in
001_password.pl instead? It would be cheaper.Works for me.
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.
--
Michael
On Fri, Apr 1, 2022 at 6:07 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
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.
I hadn't noticed this thread before.
I'm not sure whether this is properly considered a privilege check. It
could even be an anti-privilege, if the pg_hba.conf line in question
is maked "reject".
I'm not taking the position that what this patch does is wrong, but I
*am* taking the position that it's a judgement call what the correct
behavior is here.
--
Robert Haas
EDB: http://www.enterprisedb.com
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
Attachments:
v4-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patchtext/x-diff; charset=us-asciiDownload
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
Nathan Bossart <nathandbossart@gmail.com> writes:
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.
There's another problem there, which is that buildfarm animals
using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain
about role names that don't start with "regress_".
regards, tom lane
On Fri, Oct 07, 2022 at 04:18:59PM -0400, Tom Lane wrote:
There's another problem there, which is that buildfarm animals
using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain
about role names that don't start with "regress_".
Huh, I hadn't noticed that one before. It looks like roles must start with
"regress_" and database names must include "regression", so I ended up
using "regress_regression_group" for the samegroup/samerole tests.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v5-0001-Use-has_privs_of_role-for-samerole-samegroup-and-.patchtext/x-diff; charset=us-asciiDownload
From 6fe6da4ec1fe6792aa991721b77d6c2adb27697b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 1 Apr 2022 11:40:51 -0700
Subject: [PATCH v5 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..abbf310d3e 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 regress_regression_group;");
+$node->safe_psql('postgres', "CREATE ROLE regress_regression_group LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE regress_member_inherit LOGIN SUPERUSER INHERIT IN ROLE regress_regression_group PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE regress_member_noinherit LOGIN SUPERUSER NOINHERIT IN ROLE regress_regression_group PASSWORD 'pass';");
+
+# Test role inheritance is respected for +
+$ENV{"PGPASSWORD"} = 'pass';
+reset_pg_hba($node, 'all', '+regress_regression_group', 'scram-sha-256');
+test_conn($node, 'user=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_inherit', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_noinherit', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_member_noinherit" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samerole
+$ENV{"PGDATABASE"} = 'regress_regression_group';
+reset_pg_hba($node, 'samerole', 'all', 'scram-sha-256');
+test_conn($node, 'user=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_inherit', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_noinherit', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_member_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=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_inherit', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member_inherit" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member_noinherit', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_member_noinherit" method=scram-sha-256/]);
+
done_testing();
--
2.25.1
On Fri, Oct 07, 2022 at 07:59:08AM -0400, Robert Haas wrote:
I hadn't noticed this thread before.
I'm not sure whether this is properly considered a privilege check. It
could even be an anti-privilege, if the pg_hba.conf line in question
is maked "reject".I'm not taking the position that what this patch does is wrong, but I
*am* taking the position that it's a judgement call what the correct
behavior is here.
The interpretation can go both ways I guess. Now I find the argument
to treat a HBA entry based on privileges and not membership quite
appealing in terms of consistency wiht SET ROLE, particularly
considering the recent thread with predefined roles. Also, it seems
to me here that it would become easier to reason around role
hierarchies, one case being HBA entries that include predefined
roles for the role(s) to match.
--
Michael
On 10/7/22 17:58, Nathan Bossart wrote:
On Fri, Oct 07, 2022 at 04:18:59PM -0400, Tom Lane wrote:
There's another problem there, which is that buildfarm animals
using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain
about role names that don't start with "regress_".Huh, I hadn't noticed that one before. It looks like roles must start with
"regress_" and database names must include "regression", so I ended up
using "regress_regression_group" for the samegroup/samerole tests.
Thanks -- looks good to me. If there are no other comments or concerns,
I will commit/push by the end of the weekend.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes:
Thanks -- looks good to me. If there are no other comments or concerns,
I will commit/push by the end of the weekend.
Robert seems to think that this patch might be completely misguided,
so I'm not sure we have real consensus. I think he may have a point.
An angle that he didn't bring up is that we've had proposals, and
even I think a patch, for inventing database-local privileges.
If that were to become a thing, it would interact very badly with
this idea, because it would often not be clear which set of privileges
to consider. As long as HBA checks consider membership, and we don't
invent database-local role membership, there's no problem.
regards, tom lane
On Sat, Oct 8, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
Thanks -- looks good to me. If there are no other comments or concerns,
I will commit/push by the end of the weekend.Robert seems to think that this patch might be completely misguided,
so I'm not sure we have real consensus. I think he may have a point.An angle that he didn't bring up is that we've had proposals, and
even I think a patch, for inventing database-local privileges.
If that were to become a thing, it would interact very badly with
this idea, because it would often not be clear which set of privileges
to consider. As long as HBA checks consider membership, and we don't
invent database-local role membership, there's no problem.
This argument feels a little bit thin to me, because (1) one could
equally well postulate that we'd want to invent database-local role
membership and (2) presumably the relevant set of privileges would be
those for the database to which the user wishes to authenticate.
I think what is bothering me is a feeling that a privilege is
something that you get because you've authenticated. If you haven't
authenticated yet, you have no privileges. So why should it matter
whether the role to which you could hypothetically authenticate would
inherit the privileges of some other role or not?
Or to put it another way, I don't have any intuition for why someone
would want the system to behave in this way rather than in the way
that it does now. In general, what role inheritance does is actually
pretty easy to understand: either you just have the ability to access
the privileges of some other role at need, or you have those
privileges all the time even without activating them explicitly. I
think in most cases people will expect membership in a predefined role
or a role used as a group to behave in the second way, and membership
in a login role to be used in the first way, but I think there will
likely be some exceptions in both directions, which is fine, because
we can support that.
But the usage where you mention a group in pg_hba.conf feels
orthogonal to all of that to me. In that case, it's not really about
privileges at all, or at least I don't think so. It's about letting
one group of people log into the system from, say, a certain IP
address, and others not (or maybe the reverse). It seems reasonably
likely that you wouldn't want the role you used for grouping purposes
in a case like this to hold any privileges at all, or that if it did
have any privileges you wouldn't want them accessible in any way to
the group members, because if you create a group called
people_who_can_log_in_from_the_modem_pool, you do not therefore want
to end up with tables owned by
people_who_can_log_in_from_the_modem_pool. Under that theory, this
patch is going in the wrong direction.
Now there may be some other scenario in which the patch is going in
exactly the right direction, and if I knew what it was, maybe I'd
agree that the patch was a great idea. But I haven't seen anything
like that on the thread. Basically, the argument is just that the
change would make things more consistent. However, it might be an
abuse of the term. If you go out and buy blue curtains because you
have a blue couch, that's consistent interior decor. If you go out and
buy a blue car because you have a blue couch, that's not really
consistent anything, it's just two fairly-unrelated things that are
both blue.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Sat, Oct 8, 2022 at 8:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Oct 8, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
Thanks -- looks good to me. If there are no other comments or concerns,
I will commit/push by the end of the weekend.Robert seems to think that this patch might be completely misguided,
so I'm not sure we have real consensus. I think he may have a point.I think what is bothering me is a feeling that a privilege is
something that you get because you've authenticated. If you haven't
authenticated yet, you have no privileges. So why should it matter
whether the role to which you could hypothetically authenticate would
inherit the privileges of some other role or not?Or to put it another way, I don't have any intuition for why someone
would want the system to behave in this way rather than in the way
that it does now.
I'm also in the "inheritance isn't relevant here" camp. One doesn't
inherit an ability to LOGIN from a group that has a LOGIN attribute. The
[NO]INHERIT attribute doesn't even apply. This feature is so closely
related to LOGIN that [NO]INHERIT should likewise not apply here as well.
We've decided to conjoin two arguably orthogonal concerns here and need to
keep in mind that any given aspect of the overall capability might very
well only apply to a subset of the system. In this case inheritance only
applies to object permissions, not attributes, and not authentication
(which doesn't have any kind of explicit permission bit in the system to
inherit, making it just like LOGIN).
I would tend to agree that even membership probably shouldn't be involved
here, and that this entire feature would be implemented in an orthogonal
manner. I don't see any specific need to try and move to a more isolated
implementation, but trying to involve inheritance just seems wrong. The
status quo seems like a good place to stay.
David J.
On Sat, Oct 08, 2022 at 11:46:50AM -0400, Robert Haas wrote:
Now there may be some other scenario in which the patch is going in
exactly the right direction, and if I knew what it was, maybe I'd
agree that the patch was a great idea. But I haven't seen anything
like that on the thread. Basically, the argument is just that the
change would make things more consistent. However, it might be an
abuse of the term. If you go out and buy blue curtains because you
have a blue couch, that's consistent interior decor. If you go out and
buy a blue car because you have a blue couch, that's not really
consistent anything, it's just two fairly-unrelated things that are
both blue.
I believe I started this thread after reviewing the remaining uses of
is_member_of_role() after 6198420 was committed and wondering whether this
case was an oversight. If upon closer inspection we think that mere
membership is appropriate for pg_hba.conf, I'm fully prepared to go and
mark this commitfest entry as Rejected. It obviously does not seem as
clear-cut as 6198420. And I'll admit I don't have a concrete use-case in
hand to justify the behavior change.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sat, Oct 08, 2022 at 09:57:02AM -0700, David G. Johnston wrote:
I would tend to agree that even membership probably shouldn't be involved
here, and that this entire feature would be implemented in an orthogonal
manner. I don't see any specific need to try and move to a more isolated
implementation, but trying to involve inheritance just seems wrong. The
status quo seems like a good place to stay.
Okay, I think there are sufficient votes against this change to simply mark
it Rejected. Thanks for the discussion!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sat, Oct 08, 2022 at 10:12:22AM -0700, Nathan Bossart wrote:
Okay, I think there are sufficient votes against this change to simply mark
it Rejected. Thanks for the discussion!
Even if the patch is at the end rejected, I think that the test is
still useful once you switch its logic to use membership and not
inherited privileges for the roles created, and there is zero coverage
for "samplegroup" and its kind currently.
--
Michael
On Sun, Oct 09, 2022 at 10:19:51AM +0900, Michael Paquier wrote:
Even if the patch is at the end rejected, I think that the test is
still useful once you switch its logic to use membership and not
inherited privileges for the roles created, and there is zero coverage
for "samplegroup" and its kind currently.
Here you go.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
hba_test.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 93df77aa4e..5d4b30e5da 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 regress_regression_group;");
+$node->safe_psql('postgres', "CREATE ROLE regress_regression_group LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE regress_member LOGIN SUPERUSER IN ROLE regress_regression_group PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE regress_not_member LOGIN SUPERUSER PASSWORD 'pass';");
+
+# Test role membership is respected for +
+$ENV{"PGPASSWORD"} = 'pass';
+reset_pg_hba($node, 'all', '+regress_regression_group', 'scram-sha-256');
+test_conn($node, 'user=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member" method=scram-sha-256/]);
+test_conn($node, 'user=regress_not_member', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_not_member" method=scram-sha-256/]);
+
+# Test role membership is respected for samerole
+$ENV{"PGDATABASE"} = 'regress_regression_group';
+reset_pg_hba($node, 'samerole', 'all', 'scram-sha-256');
+test_conn($node, 'user=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member" method=scram-sha-256/]);
+test_conn($node, 'user=regress_not_member', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_not_member" method=scram-sha-256/]);
+
+# Test role membership is respected for samegroup
+reset_pg_hba($node, 'samegroup', 'all', 'scram-sha-256');
+test_conn($node, 'user=regress_regression_group', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_regression_group" method=scram-sha-256/]);
+test_conn($node, 'user=regress_member', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="regress_member" method=scram-sha-256/]);
+test_conn($node, 'user=regress_not_member', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="regress_not_member" method=scram-sha-256/]);
+
done_testing();
On Sun, Oct 09, 2022 at 02:13:48PM -0700, Nathan Bossart wrote:
Here you go.
Thanks, applied. It took me a few minutes to note that
regress_regression_* is required in the object names because we need
to use the same name for the parent role and the database, with
"regress_" being required for the role and "regression" being required
for the database. I have added an extra section where pg_hba.conf is
set to match only the parent role, while on it. perltidy has reshaped
things in an interesting way, because the generated log_[un]like is
long, it seems.
--
Michael
On Tue, Oct 11, 2022 at 02:01:07PM +0900, Michael Paquier wrote:
Thanks, applied. It took me a few minutes to note that
regress_regression_* is required in the object names because we need
to use the same name for the parent role and the database, with
"regress_" being required for the role and "regression" being required
for the database. I have added an extra section where pg_hba.conf is
set to match only the parent role, while on it. perltidy has reshaped
things in an interesting way, because the generated log_[un]like is
long, it seems.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Greetings,
* Nathan Bossart (nathandbossart@gmail.com) wrote:
On Sat, Oct 08, 2022 at 11:46:50AM -0400, Robert Haas wrote:
Now there may be some other scenario in which the patch is going in
exactly the right direction, and if I knew what it was, maybe I'd
agree that the patch was a great idea. But I haven't seen anything
like that on the thread. Basically, the argument is just that the
change would make things more consistent. However, it might be an
abuse of the term. If you go out and buy blue curtains because you
have a blue couch, that's consistent interior decor. If you go out and
buy a blue car because you have a blue couch, that's not really
consistent anything, it's just two fairly-unrelated things that are
both blue.I believe I started this thread after reviewing the remaining uses of
is_member_of_role() after 6198420 was committed and wondering whether this
case was an oversight. If upon closer inspection we think that mere
membership is appropriate for pg_hba.conf, I'm fully prepared to go and
mark this commitfest entry as Rejected. It obviously does not seem as
clear-cut as 6198420. And I'll admit I don't have a concrete use-case in
hand to justify the behavior change.
Looks like we've already ended up there, but my recollection of this is
that it was very much intentional to use is_member_of_role() here.
Perhaps it should have been better commented (as all uses of
is_member_of_role() instead of has_privs_of_role() really should have
lots of comments as to exactly why it makes sense in those cases).
Thanks,
Stephen