pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCREATEROLE users vs. role properties
10+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-01-24 f1358ca52d Adjust interaction of CREATEROLE with role properties.
* CREATEROLE users vs. role properties
@ 2023-01-18 17:15 Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2023-01-18 17:15 UTC (permalink / raw)
To: pgsql-hackers
On Mon, Jan 16, 2023 at 2:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 1. It's still possible for a CREATEROLE user to hand out role
> attributes that they don't possess. The new prohibitions in
> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
> from handing out membership in a role on which they lack sufficient
> permissions, but they don't prevent a CREATEROLE user who lacks
> CREATEDB from creating a new user who does have CREATEDB. I think we
> should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
> the same rule that we now use for role memberships: you've got to have
> the property in order to give it to someone else. In the case of
> CREATEDB, this would tighten the current rules, which allow you to
> give out CREATEDB without having it. In the case of REPLICATION and
> BYPASSRLS, this would liberalize the current rules: right now, a
> CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
> even if they possess those attributes.
>
> This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
> properties. It seems possible to me that someone might want to set up
> a CREATEROLE user who can't make more such users, and this proposal
> doesn't manufacture any way of doing that. It also doesn't let you
> constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
> for some other user. I think that's OK. It might be nice to have ways
> of imposing such restrictions at some point in the future, but it is
> not very obvious what to do about such cases and, importantly, I don't
> think there's any security impact from failing to address those cases.
> If a CREATEROLE user without CREATEDB can create a new role that does
> have CREATEDB, that's a privilege escalation. If they can hand out
> CREATEROLE, that isn't: they already have it.
Here is a patch implementing the above proposal. Since this is fairly
closely related to already-committed work, I would like to get this
into v16. That way, all the changes to how CREATEROLE works will go
into a single release, which seems less confusing for users. It is
also fairly clear to me that this is an improvement over the status
quo. Sometimes things that seem clear to me turn out to be false, so
if this change seems like a problem to you, please let me know.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Adjust-interaction-of-CREATEROLE-with-role-proper.patch (application/octet-stream, 19.1K) download
From e2e8b89d1b3ceda882c919b3cd68a81ccb5580f4 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 18 Jan 2023 12:04:51 -0500
Subject: [PATCH v1] Adjust interaction of CREATEROLE with role properties.
Previously, a CREATEROLE user without SUPERUSER could not alter
REPLICATION users in any way, and could not set the BYPASSRLS
attribute. However, they could manipulate the CREATEDB property
even if they themselves did not possess it.
With this change, a CREATEROLE user without SUPERUSER can set or
clear the REPLICATION, BYPASSRLS, or CREATEDB property on a new
role or a role that they have rights to manage if and only if
that property is set for their own role.
This implements the standard idea that you can't give permissions
you don't have (but you can give the ones you do have). We might
in the future want to provide more powerful ways to constrain
what a CREATEROLE user can do - for example, to limit whether
CONNECTION LIMIT can be set or the values to which it can be set -
but that is left as future work.
---
doc/src/sgml/ref/alter_role.sgml | 13 ++--
doc/src/sgml/ref/create_role.sgml | 23 ++-----
src/backend/commands/dbcommands.c | 3 +-
src/backend/commands/user.c | 82 +++++++++++------------
src/include/commands/dbcommands.h | 1 +
src/test/regress/expected/create_role.out | 53 ++++++++++++---
src/test/regress/sql/create_role.sql | 45 +++++++++++--
7 files changed, 137 insertions(+), 83 deletions(-)
diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index fbb4612e70..ff2b88e9b6 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -70,11 +70,14 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
<link linkend="sql-revoke"><command>REVOKE</command></link> for that.)
Attributes not mentioned in the command retain their previous settings.
Database superusers can change any of these settings for any role.
- Roles having <literal>CREATEROLE</literal> privilege can change any of these
- settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
- and <literal>BYPASSRLS</literal>; but only for non-superuser and
- non-replication roles for which they have been
- granted <literal>ADMIN OPTION</literal>.
+ Non-superuser roles having <literal>CREATEROLE</literal> privilege can
+ change most of these properties, but only for non-superuser and
+ non-replication roles for which they have been granted
+ <literal>ADMIN OPTION</literal>. Non-superusers cannot change the
+ <literal>SUPERUSER</literal> property and can change the
+ <literal>CREATEDB</literal>, <literal>REPLICATION</literal>, and
+ <literal>BYPASSRLS</literal> properties only if they possess the
+ corresponding property themselves.
Ordinary roles can only change their own password.
</para>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 7ce4e38b45..c101df6e2f 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -109,6 +109,8 @@ in sync when changing the above synopsis!
<literal>NOCREATEDB</literal> will deny a role the ability to
create databases. If not specified,
<literal>NOCREATEDB</literal> is the default.
+ Only superuser roles or roles with <literal>CREATEDB</literal>
+ can specify <literal>CREATEDB</literal>.
</para>
</listitem>
</varlistentry>
@@ -188,8 +190,8 @@ in sync when changing the above synopsis!
highly privileged role, and should only be used on roles actually
used for replication. If not specified,
<literal>NOREPLICATION</literal> is the default.
- You must be a superuser to create a new role having the
- <literal>REPLICATION</literal> attribute.
+ Only superuser roles or roles with <literal>REPLICATION</literal>
+ can specify <literal>REPLICATION</literal>.
</para>
</listitem>
</varlistentry>
@@ -201,8 +203,8 @@ in sync when changing the above synopsis!
<para>
These clauses determine whether a role bypasses every row-level
security (RLS) policy. <literal>NOBYPASSRLS</literal> is the default.
- You must be a superuser to create a new role having
- the <literal>BYPASSRLS</literal> attribute.
+ Only superuser roles or roles with <literal>BYPASSRLS</literal>
+ can specify <literal>BYPASSRLS</literal>.
</para>
<para>
@@ -390,19 +392,6 @@ in sync when changing the above synopsis!
specified in the SQL standard.
</para>
- <para>
- Be careful with the <literal>CREATEROLE</literal> privilege. There is no concept of
- inheritance for the privileges of a <literal>CREATEROLE</literal>-role. That
- means that even if a role does not have a certain privilege but is allowed
- to create other roles, it can easily create another role with different
- privileges than its own (except for creating roles with superuser
- privileges). For example, if the role <quote>user</quote> has the
- <literal>CREATEROLE</literal> privilege but not the <literal>CREATEDB</literal> privilege,
- nonetheless it can create a new role with the <literal>CREATEDB</literal>
- privilege. Therefore, regard roles that have the <literal>CREATEROLE</literal>
- privilege as almost-superuser-roles.
- </para>
-
<para>
<productname>PostgreSQL</productname> includes a program <xref
linkend="app-createuser"/> that has
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 518ffca09a..1f4ce2fb9c 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -121,7 +121,6 @@ static bool get_db_info(const char *name, LOCKMODE lockmode,
Oid *dbTablespace, char **dbCollate, char **dbCtype, char **dbIculocale,
char *dbLocProvider,
char **dbCollversion);
-static bool have_createdb_privilege(void);
static void remove_dbtablespaces(Oid db_id);
static bool check_db_file_conflict(Oid db_id);
static int errdetail_busy_db(int notherbackends, int npreparedxacts);
@@ -2742,7 +2741,7 @@ get_db_info(const char *name, LOCKMODE lockmode,
}
/* Check if current user has createdb privileges */
-static bool
+bool
have_createdb_privilege(void)
{
bool result = false;
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 4d193a6f9a..3a92e930c0 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -311,33 +311,28 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
bypassrls = boolVal(dbypassRLS->arg);
/* Check some permissions first */
- if (issuper)
+ if (!superuser_arg(currentUserId))
{
- if (!superuser())
+ if (!has_createrole_privilege(currentUserId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to create role")));
+ if (issuper)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create superusers")));
- }
- else if (isreplication)
- {
- if (!superuser())
+ if (createdb && !have_createdb_privilege())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create replication users")));
- }
- else if (bypassrls)
- {
- if (!superuser())
+ errmsg("must have createdb permission to create createdb users")));
+ if (isreplication && !has_rolreplication(currentUserId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create bypassrls users")));
- }
- else
- {
- if (!have_createrole_privilege())
+ errmsg("must have replication permission to create replication users")));
+ if (bypassrls && !has_bypassrls_privilege(currentUserId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create role")));
+ errmsg("must have bypassrls to create bypassrls users")));
}
/*
@@ -748,32 +743,11 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
rolename = pstrdup(NameStr(authform->rolname));
roleid = authform->oid;
- /*
- * To mess with a superuser or replication role in any way you gotta be
- * superuser. We also insist on superuser to change the BYPASSRLS
- * property.
- */
- if (authform->rolsuper || dissuper)
- {
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to alter superuser roles or change superuser attribute")));
- }
- else if (authform->rolreplication || disreplication)
- {
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to alter replication roles or change replication attribute")));
- }
- else if (dbypassRLS)
- {
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to change bypassrls attribute")));
- }
+ /* To mess with a superuser in any way you gotta be superuser. */
+ if (!superuser() && (authform->rolsuper || dissuper))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to alter superuser roles or change superuser attribute")));
/*
* Most changes to a role require that you both have CREATEROLE privileges
@@ -784,7 +758,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
{
/* things an unprivileged user certainly can't do */
if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
- dvalidUntil)
+ dvalidUntil || disreplication || dbypassRLS)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied")));
@@ -795,6 +769,26 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must have CREATEROLE privilege to change another user's password")));
}
+ else if (!superuser())
+ {
+ /*
+ * Even if you have both CREATEROLE and ADMIN OPTION on a role, you
+ * can only change the CREATEDB, REPLICATION, or BYPASSRLS attributes
+ * if they are set for your own role (or you are the superuser).
+ */
+ if (dcreatedb && !have_createdb_privilege())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must have createdb privilege to change createdb attribute")));
+ if (disreplication && !has_rolreplication(currentUserId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must have replication privilege to change replication attribute")));
+ if (dbypassRLS && !has_bypassrls_privilege(currentUserId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must have bypassrls privilege to change bypassrls attribute")));
+ }
/* To add members to a role, you need ADMIN OPTION. */
if (drolemembers && !is_admin_of_role(currentUserId, roleid))
diff --git a/src/include/commands/dbcommands.h b/src/include/commands/dbcommands.h
index 9b8818315a..5fbc3ca752 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -30,6 +30,7 @@ extern ObjectAddress AlterDatabaseOwner(const char *dbname, Oid newOwnerId);
extern Oid get_database_oid(const char *dbname, bool missing_ok);
extern char *get_database_name(Oid dbid);
+extern bool have_createdb_privilege(void);
extern void check_encoding_locale_matches(int encoding, const char *collate, const char *ctype);
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index cd49feabb3..c8beb36bab 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -2,19 +2,51 @@
CREATE ROLE regress_role_super SUPERUSER;
CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS;
GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION;
+CREATE ROLE regress_role_limited_admin CREATEROLE;
CREATE ROLE regress_role_normal;
--- fail, only superusers can create users with these privileges
-SET SESSION AUTHORIZATION regress_role_admin;
+-- fail, CREATEROLE user can't give away role attributes without having them
+SET SESSION AUTHORIZATION regress_role_limited_admin;
CREATE ROLE regress_nosuch_superuser SUPERUSER;
ERROR: must be superuser to create superusers
CREATE ROLE regress_nosuch_replication_bypassrls REPLICATION BYPASSRLS;
-ERROR: must be superuser to create replication users
+ERROR: must have replication permission to create replication users
CREATE ROLE regress_nosuch_replication REPLICATION;
-ERROR: must be superuser to create replication users
+ERROR: must have replication permission to create replication users
CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
-ERROR: must be superuser to create bypassrls users
--- ok, having CREATEROLE is enough to create users with these privileges
+ERROR: must have bypassrls to create bypassrls users
+CREATE ROLE regress_nosuch_createdb CREATEDB;
+ERROR: must have createdb permission to create createdb users
+-- ok, can create a role without any special attributes
+CREATE ROLE regress_role_limited;
+-- fail, can't give it in any of the restricted attributes
+ALTER ROLE regress_role_limited SUPERUSER;
+ERROR: must be superuser to alter superuser roles or change superuser attribute
+ALTER ROLE regress_role_limited REPLICATION;
+ERROR: must have replication privilege to change replication attribute
+ALTER ROLE regress_role_limited CREATEDB;
+ERROR: must have createdb privilege to change createdb attribute
+ALTER ROLE regress_role_limited BYPASSRLS;
+ERROR: must have bypassrls privilege to change bypassrls attribute
+DROP ROLE regress_role_limited;
+-- ok, can give away these role attributes if you have them
+SET SESSION AUTHORIZATION regress_role_admin;
+CREATE ROLE regress_replication_bypassrls REPLICATION BYPASSRLS;
+CREATE ROLE regress_replication REPLICATION;
+CREATE ROLE regress_bypassrls BYPASSRLS;
CREATE ROLE regress_createdb CREATEDB;
+-- ok, can toggle these role attributes off and on if you have them
+ALTER ROLE regress_replication NOREPLICATION;
+ALTER ROLE regress_replication REPLICATION;
+ALTER ROLE regress_bypassrls NOBYPASSRLS;
+ALTER ROLE regress_bypassrls BYPASSRLS;
+ALTER ROLE regress_createdb NOCREATEDB;
+ALTER ROLE regress_createdb CREATEDB;
+-- fail, can't toggle SUPERUSER
+ALTER ROLE regress_createdb SUPERUSER;
+ERROR: must be superuser to alter superuser roles or change superuser attribute
+ALTER ROLE regress_createdb NOSUPERUSER;
+ERROR: must be superuser to alter superuser roles or change superuser attribute
+-- ok, having CREATEROLE is enough to create users with these privileges
CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION;
CREATE ROLE regress_login LOGIN;
@@ -53,9 +85,9 @@ ERROR: permission denied to create database
CREATE ROLE regress_plainrole;
-- ok, roles with CREATEROLE can create new roles with it
CREATE ROLE regress_rolecreator CREATEROLE;
--- ok, roles with CREATEROLE can create new roles with privilege they lack
-CREATE ROLE regress_hasprivs CREATEDB CREATEROLE LOGIN INHERIT
- CONNECTION LIMIT 5;
+-- ok, roles with CREATEROLE can create new roles with different role
+-- attributes, including CREATEROLE
+CREATE ROLE regress_hasprivs CREATEROLE LOGIN INHERIT CONNECTION LIMIT 5;
-- ok, we should be able to modify a role we created
COMMENT ON ROLE regress_hasprivs IS 'some comment';
ALTER ROLE regress_hasprivs RENAME TO regress_tenant;
@@ -164,6 +196,9 @@ DROP ROLE regress_plainrole;
-- must revoke privileges before dropping role
REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
-- ok, should be able to drop non-superuser roles we created
+DROP ROLE regress_replication_bypassrls;
+DROP ROLE regress_replication;
+DROP ROLE regress_bypassrls;
DROP ROLE regress_createdb;
DROP ROLE regress_createrole;
DROP ROLE regress_login;
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 6b90336da2..19031b4612 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -2,17 +2,47 @@
CREATE ROLE regress_role_super SUPERUSER;
CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS;
GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION;
+CREATE ROLE regress_role_limited_admin CREATEROLE;
CREATE ROLE regress_role_normal;
--- fail, only superusers can create users with these privileges
-SET SESSION AUTHORIZATION regress_role_admin;
+-- fail, CREATEROLE user can't give away role attributes without having them
+SET SESSION AUTHORIZATION regress_role_limited_admin;
CREATE ROLE regress_nosuch_superuser SUPERUSER;
CREATE ROLE regress_nosuch_replication_bypassrls REPLICATION BYPASSRLS;
CREATE ROLE regress_nosuch_replication REPLICATION;
CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
+CREATE ROLE regress_nosuch_createdb CREATEDB;
--- ok, having CREATEROLE is enough to create users with these privileges
+-- ok, can create a role without any special attributes
+CREATE ROLE regress_role_limited;
+
+-- fail, can't give it in any of the restricted attributes
+ALTER ROLE regress_role_limited SUPERUSER;
+ALTER ROLE regress_role_limited REPLICATION;
+ALTER ROLE regress_role_limited CREATEDB;
+ALTER ROLE regress_role_limited BYPASSRLS;
+DROP ROLE regress_role_limited;
+
+-- ok, can give away these role attributes if you have them
+SET SESSION AUTHORIZATION regress_role_admin;
+CREATE ROLE regress_replication_bypassrls REPLICATION BYPASSRLS;
+CREATE ROLE regress_replication REPLICATION;
+CREATE ROLE regress_bypassrls BYPASSRLS;
CREATE ROLE regress_createdb CREATEDB;
+
+-- ok, can toggle these role attributes off and on if you have them
+ALTER ROLE regress_replication NOREPLICATION;
+ALTER ROLE regress_replication REPLICATION;
+ALTER ROLE regress_bypassrls NOBYPASSRLS;
+ALTER ROLE regress_bypassrls BYPASSRLS;
+ALTER ROLE regress_createdb NOCREATEDB;
+ALTER ROLE regress_createdb CREATEDB;
+
+-- fail, can't toggle SUPERUSER
+ALTER ROLE regress_createdb SUPERUSER;
+ALTER ROLE regress_createdb NOSUPERUSER;
+
+-- ok, having CREATEROLE is enough to create users with these privileges
CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION;
CREATE ROLE regress_login LOGIN;
@@ -56,9 +86,9 @@ CREATE ROLE regress_plainrole;
-- ok, roles with CREATEROLE can create new roles with it
CREATE ROLE regress_rolecreator CREATEROLE;
--- ok, roles with CREATEROLE can create new roles with privilege they lack
-CREATE ROLE regress_hasprivs CREATEDB CREATEROLE LOGIN INHERIT
- CONNECTION LIMIT 5;
+-- ok, roles with CREATEROLE can create new roles with different role
+-- attributes, including CREATEROLE
+CREATE ROLE regress_hasprivs CREATEROLE LOGIN INHERIT CONNECTION LIMIT 5;
-- ok, we should be able to modify a role we created
COMMENT ON ROLE regress_hasprivs IS 'some comment';
@@ -150,6 +180,9 @@ DROP ROLE regress_plainrole;
REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
-- ok, should be able to drop non-superuser roles we created
+DROP ROLE regress_replication_bypassrls;
+DROP ROLE regress_replication;
+DROP ROLE regress_bypassrls;
DROP ROLE regress_createdb;
DROP ROLE regress_createrole;
DROP ROLE regress_login;
--
2.37.1 (Apple Git-137.1)
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
@ 2023-01-18 23:17 ` Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:20 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 10+ messages in thread
From: Nathan Bossart @ 2023-01-18 23:17 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On Wed, Jan 18, 2023 at 12:15:33PM -0500, Robert Haas wrote:
> On Mon, Jan 16, 2023 at 2:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> 1. It's still possible for a CREATEROLE user to hand out role
>> attributes that they don't possess. The new prohibitions in
>> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
>> from handing out membership in a role on which they lack sufficient
>> permissions, but they don't prevent a CREATEROLE user who lacks
>> CREATEDB from creating a new user who does have CREATEDB. I think we
>> should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
>> the same rule that we now use for role memberships: you've got to have
>> the property in order to give it to someone else. In the case of
>> CREATEDB, this would tighten the current rules, which allow you to
>> give out CREATEDB without having it. In the case of REPLICATION and
>> BYPASSRLS, this would liberalize the current rules: right now, a
>> CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
>> even if they possess those attributes.
>>
>> This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
>> properties. It seems possible to me that someone might want to set up
>> a CREATEROLE user who can't make more such users, and this proposal
>> doesn't manufacture any way of doing that. It also doesn't let you
>> constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
>> for some other user. I think that's OK. It might be nice to have ways
>> of imposing such restrictions at some point in the future, but it is
>> not very obvious what to do about such cases and, importantly, I don't
>> think there's any security impact from failing to address those cases.
>> If a CREATEROLE user without CREATEDB can create a new role that does
>> have CREATEDB, that's a privilege escalation. If they can hand out
>> CREATEROLE, that isn't: they already have it.
>
> Here is a patch implementing the above proposal. Since this is fairly
> closely related to already-committed work, I would like to get this
> into v16. That way, all the changes to how CREATEROLE works will go
> into a single release, which seems less confusing for users. It is
> also fairly clear to me that this is an improvement over the status
> quo. Sometimes things that seem clear to me turn out to be false, so
> if this change seems like a problem to you, please let me know.
This seems like a clear improvement to me. However, as the attribute
system becomes more sophisticated, I think we ought to improve the error
messages in user.c. IMHO messages like "permission denied" could be
greatly improved with some added context.
For example, if I want to change a role's password, I need both CREATEROLE
and ADMIN OPTION on the role, but the error message only mentions
CREATEROLE.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole password 'test';
ERROR: must have CREATEROLE privilege to change another user's password
Similarly, if I want to allow a role to grant REPLICATION to another role,
I have to give it CREATEROLE, REPLICATION, and membership with ADMIN
OPTION. If the role is missing CREATEROLE or membership with ADMIN OPTION,
it'll only ever see a "permission denied" error.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# alter role createrole with replication;
ALTER ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# grant otherrole to createrole;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# grant otherrole to createrole with admin option;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ALTER ROLE
If it has both CREATEROLE and membership with ADMIN OPTION (but not
REPLICATION), it'll see a more helpful message.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# grant otherrole to createrole with admin option;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: must have replication privilege to change replication attribute
This probably shouldn't block your patch, but I think it's worth doing in
v16 since there are other changes in this area. I'm happy to help.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
@ 2023-01-19 09:35 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
1 sibling, 1 reply; 10+ messages in thread
From: tushar @ 2023-01-19 09:35 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On 1/19/23 4:47 AM, Nathan Bossart wrote:
> This seems like a clear improvement to me. However, as the attribute
> system becomes more sophisticated, I think we ought to improve the error
> messages in user.c. IMHO messages like "permission denied" could be
> greatly improved with some added context.
I observed this behavior where the role is having creatrole but still
it's unable to pass it to another user.
postgres=# create role abc1 login createrole;
CREATE ROLE
postgres=# create user test1;
CREATE ROLE
postgres=# \c - abc1
You are now connected to database "postgres" as user "abc1".
postgres=> alter role test1 with createrole ;
ERROR: permission denied
postgres=>
which was working previously without patch.
Is this an expected behavior?
--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
@ 2023-01-19 11:15 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: tushar @ 2023-01-19 11:15 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On 1/19/23 3:05 PM, tushar wrote:
> which was working previously without patch.
My bad, I was testing against PG v15 but this issue is not
reproducible on master (without patch).
As you mentioned- "This implements the standard idea that you can't give
permissions
you don't have (but you can give the ones you do have)" but here the
role is having
createrole privilege that he cannot pass on to another user? Is this
expected?
postgres=# create role fff with createrole;
CREATE ROLE
postgres=# create role xxx;
CREATE ROLE
postgres=# set role fff;
SET
postgres=> alter role xxx with createrole;
ERROR: permission denied
postgres=>
--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
@ 2023-01-19 15:04 ` Robert Haas <robertmhaas@gmail.com>
2023-01-23 15:25 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2023-01-19 15:04 UTC (permalink / raw)
To: tushar <tushar.ahuja@enterprisedb.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Thu, Jan 19, 2023 at 6:15 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> postgres=# create role fff with createrole;
> CREATE ROLE
> postgres=# create role xxx;
> CREATE ROLE
> postgres=# set role fff;
> SET
> postgres=> alter role xxx with createrole;
> ERROR: permission denied
> postgres=>
Here fff would need ADMIN OPTION on xxx to be able to make modifications to it.
See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=cf5eb37c5ee0cc54c80d95c1695d7fca1f7c6...
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
@ 2023-01-23 15:25 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-23 16:57 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: tushar @ 2023-01-23 15:25 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Thu, Jan 19, 2023 at 8:34 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jan 19, 2023 at 6:15 AM tushar <tushar.ahuja@enterprisedb.com>
> wrote:
> > postgres=# create role fff with createrole;
> > CREATE ROLE
> > postgres=# create role xxx;
> > CREATE ROLE
> > postgres=# set role fff;
> > SET
> > postgres=> alter role xxx with createrole;
> > ERROR: permission denied
> > postgres=>
>
> Here fff would need ADMIN OPTION on xxx to be able to make modifications
> to it.
>
> See
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=cf5eb37c5ee0cc54c80d95c1695d7fca1f7c6...
Thanks, Robert, that was helpful.
Please refer to this scenario where I am able to give createrole privileges
but not replication privilege to role
postgres=# create role t1 createrole;
CREATE ROLE
postgres=# create role t2 replication;
CREATE ROLE
postgres=# create role t3;
CREATE ROLE
postgres=# grant t3 to t1,t2 with admin option;
GRANT ROLE
postgres=# set session authorization t1;
SET
*postgres=> alter role t3 createrole ;ALTER ROLE*
postgres=> set session authorization t2;
SET
postgres=> alter role t3 replication;
ERROR: permission denied
This same behavior was observed in v14 as well but why i am able to give
createrole grant but not replication?
regards,
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-23 15:25 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
@ 2023-01-23 16:57 ` Robert Haas <robertmhaas@gmail.com>
2023-01-24 14:07 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2023-01-23 16:57 UTC (permalink / raw)
To: tushar <tushar.ahuja@enterprisedb.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Mon, Jan 23, 2023 at 10:25 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> Please refer to this scenario where I am able to give createrole privileges but not replication privilege to role
>
> postgres=# create role t1 createrole;
> CREATE ROLE
> postgres=# create role t2 replication;
> CREATE ROLE
> postgres=# create role t3;
> CREATE ROLE
> postgres=# grant t3 to t1,t2 with admin option;
> GRANT ROLE
> postgres=# set session authorization t1;
> SET
> postgres=> alter role t3 createrole ;
> ALTER ROLE
> postgres=> set session authorization t2;
> SET
> postgres=> alter role t3 replication;
> ERROR: permission denied
>
> This same behavior was observed in v14 as well but why i am able to give createrole grant but not replication?
In previous releases, you needed to have CREATEROLE in order to be
able to perform user management functions. In master, you still need
CREATEROLE, and you also need ADMIN OPTION on the role. In this
scenario, only t1 meets those requirements with respect to t3, so only
t1 can manage t3. t2 can SET ROLE to t3 and grant membership in t3,
but it can't set role properties on t3 or change t3's password or
things like that, because the ability to make user management changes
is controlled by CREATEROLE.
The patch is only intended to change behavior in the case where you
possess both CREATEROLE and also ADMIN OPTION on the target role (but
not SUPERUSER). In that scenario, it intends to change whether you can
give or remove the CREATEDB, REPLICATION, and BYPASSRLS properties
from a user.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-23 15:25 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-23 16:57 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
@ 2023-01-24 14:07 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-24 16:00 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: tushar @ 2023-01-24 14:07 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Mon, Jan 23, 2023 at 10:28 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> In previous releases, you needed to have CREATEROLE in order to be
> able to perform user management functions. In master, you still need
> CREATEROLE, and you also need ADMIN OPTION on the role. In this
> scenario, only t1 meets those requirements with respect to t3, so only
> t1 can manage t3. t2 can SET ROLE to t3 and grant membership in t3,
> but it can't set role properties on t3 or change t3's password or
> things like that, because the ability to make user management changes
> is controlled by CREATEROLE.
>
ok.
>
> The patch is only intended to change behavior in the case where you
> possess both CREATEROLE and also ADMIN OPTION on the target role (but
> not SUPERUSER). In that scenario, it intends to change whether you can
> give or remove the CREATEDB, REPLICATION, and BYPASSRLS properties
> from a user.
>
right, Neha/I have tested with different scenarios using
createdb/replication/bypassrls and other
privileges properties on the role. also checked
pg_dumpall/pg_basebackup and everything looks fine.
regards,
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-23 15:25 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
2023-01-23 16:57 ` Re: CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-24 14:07 ` Re: CREATEROLE users vs. role properties tushar <tushar.ahuja@enterprisedb.com>
@ 2023-01-24 16:00 ` Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Robert Haas @ 2023-01-24 16:00 UTC (permalink / raw)
To: tushar <tushar.ahuja@enterprisedb.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Tue, Jan 24, 2023 at 9:07 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> right, Neha/I have tested with different scenarios using createdb/replication/bypassrls and other
> privileges properties on the role. also checked pg_dumpall/pg_basebackup and everything looks fine.
Thanks. I have committed the patch.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: CREATEROLE users vs. role properties
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Re: CREATEROLE users vs. role properties Nathan Bossart <nathandbossart@gmail.com>
@ 2023-01-19 15:20 ` Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 10+ messages in thread
From: Robert Haas @ 2023-01-19 15:20 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Wed, Jan 18, 2023 at 6:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > Here is a patch implementing the above proposal. Since this is fairly
> > closely related to already-committed work, I would like to get this
> > into v16. That way, all the changes to how CREATEROLE works will go
> > into a single release, which seems less confusing for users. It is
> > also fairly clear to me that this is an improvement over the status
> > quo. Sometimes things that seem clear to me turn out to be false, so
> > if this change seems like a problem to you, please let me know.
>
> This seems like a clear improvement to me.
Cool.
> However, as the attribute
> system becomes more sophisticated, I think we ought to improve the error
> messages in user.c. IMHO messages like "permission denied" could be
> greatly improved with some added context.
>
> This probably shouldn't block your patch, but I think it's worth doing in
> v16 since there are other changes in this area. I'm happy to help.
That would be great. I agree that it's good to try to improve the
error messages. It hasn't been entirely clear to me how to do that.
For instance, I don't think we want to say something like:
ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
role, or in lieu of both of those to be superuser, to set the
CONNECTION LIMIT for another role
ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
role, plus also CREATEDB, or in lieu of all that to be superuser, to
remove the CREATEDB property from another role
Such messages are long and we'd end up with a lot of variants. It's
possible that the messages could be multi-tier. For instance, if we
determine that you're trying to manage users and you don't have
permission to manage ANY user, we could say:
ERROR: permission to manage roles denied
DETAIL: You must have the CREATEROLE privilege or be a superuser to
manage roles.
If you could potentially manage some user, but not the one you're
trying to manage, we could say:
ERROR: permission to manage role "%s" denied
DETAIL: You need ADMIN OPTION on the target role to manage it.
If you have permission to manage the target role but not in the
requested manner, we could then say something like:
ERROR: permission to manage CREATEDB for role "%s" denied
DETAIL: You need CREATEDB to manage it.
This is just one idea, and maybe not the best one. I'm just trying to
say that I think this is basically an organizational problem. We need
a plan for how we're going to report errors that is not too
complicated to implement with reasonable effort, and that will produce
messages that users will understand. I'd be delighted if you wanted to
provide either ideas or patches...
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-01-24 16:00 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 17:15 CREATEROLE users vs. role properties Robert Haas <robertmhaas@gmail.com>
2023-01-18 23:17 ` Nathan Bossart <nathandbossart@gmail.com>
2023-01-19 09:35 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 11:15 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-19 15:04 ` Robert Haas <robertmhaas@gmail.com>
2023-01-23 15:25 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-23 16:57 ` Robert Haas <robertmhaas@gmail.com>
2023-01-24 14:07 ` tushar <tushar.ahuja@enterprisedb.com>
2023-01-24 16:00 ` Robert Haas <robertmhaas@gmail.com>
2023-01-19 15:20 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox