Thread: Default Roles
All, Attached is a stripped-down version of the default roles patch which includes only the 'pg_signal_backend' default role. This provides the framework and structure for other default roles to be added and formally reserves the 'pg_' role namespace. This is split into two patches, the first to formally reserve 'pg_', the second to add the new default role. Will add to the March commitfest for review. Thanks! Stephen
Attachment
On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Attached is a stripped-down version of the default roles patch which
> includes only the 'pg_signal_backend' default role. This provides the
> framework and structure for other default roles to be added and formally
> reserves the 'pg_' role namespace. This is split into two patches, the
> first to formally reserve 'pg_', the second to add the new default role.
>
> Will add to the March commitfest for review.
Here is a review of the first patch:
+ if (!IsA(node, RoleSpec))
+ elog(ERROR, "invalid node type %d", node->type);
That looks strange. Why not just Assert(IsA(node, RoleSpec))?
+
+ return;
Useless return.
@@ -673,6 +673,7 @@ dumpRoles(PGconn *conn) "pg_catalog.shobj_description(oid,
'pg_authid') as rolcomment, " "rolname =
current_user AS is_current_user " "FROM pg_authid "
+ "WHERE rolname !~ '^pg_' "
"ORDER BY 2"); else if (server_version >= 90100) printfPQExpBuffer(buf,
@@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn) "LEFT JOIN pg_authid ur
on
ur.oid = a.roleid " "LEFT JOIN pg_authid um on
um.oid = a.member " "LEFT JOIN pg_authid ug on
ug.oid = a.grantor "
+ "WHERE NOT (ur.rolname ~
'^pg_' AND um.rolname ~ '^pg_')" "ORDER BY 1,2,3");
If I'm reading this correctly, when dumping a 9.5 server, we'll
silently drop any roles whose names start with pg_ from the dump, and
hope that doesn't break anything. When dumping a 9.4 or older server,
we'll include those roles and hope that they miraculously restore on
the new server. I'm thinking neither of those approaches is going to
work out, and the difference between them seems totally unprincipled.
@@ -631,7 +637,8 @@ check_is_install_user(ClusterInfo *cluster) res = executeQueryOrDie(conn,
"SELECT rolsuper, oid " "FROM
pg_catalog.pg_roles "
- "WHERE rolname
= current_user");
+ "WHERE rolname
= current_user "
+ "AND rolname
!~ '^pg_'");
/* * We only allow the install user in the new cluster (see comment below)
@@ -647,7 +654,8 @@ check_is_install_user(ClusterInfo *cluster)
res = executeQueryOrDie(conn, "SELECT COUNT(*) "
- "FROM
pg_catalog.pg_roles ");
+ "FROM
pg_catalog.pg_roles "
+ "WHERE rolname
!~ '^pg_'");
if (PQntuples(res) != 1) pg_fatal("could not determine the number of users\n");
What bad thing would happen without these changes that would be
avoided with these changes? I can't think of anything.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Attached is a stripped-down version of the default roles patch which
> > includes only the 'pg_signal_backend' default role. This provides the
> > framework and structure for other default roles to be added and formally
> > reserves the 'pg_' role namespace. This is split into two patches, the
> > first to formally reserve 'pg_', the second to add the new default role.
> >
> > Will add to the March commitfest for review.
>
> Here is a review of the first patch:
>
> + if (!IsA(node, RoleSpec))
> + elog(ERROR, "invalid node type %d", node->type);
>
> That looks strange. Why not just Assert(IsA(node, RoleSpec))?
Sure, that works, done.
> +
> + return;
>
> Useless return.
Removed.
> @@ -673,6 +673,7 @@ dumpRoles(PGconn *conn)
> "pg_catalog.shobj_description(oid,
> 'pg_authid') as rolcomment, "
> "rolname =
> current_user AS is_current_user "
> "FROM pg_authid "
> + "WHERE rolname !~ '^pg_' "
> "ORDER BY 2");
> else if (server_version >= 90100)
> printfPQExpBuffer(buf,
> @@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn)
> "LEFT JOIN pg_authid ur on
> ur.oid = a.roleid "
> "LEFT JOIN pg_authid um on
> um.oid = a.member "
> "LEFT JOIN pg_authid ug on
> ug.oid = a.grantor "
> + "WHERE NOT (ur.rolname ~
> '^pg_' AND um.rolname ~ '^pg_')"
> "ORDER BY 1,2,3");
>
> If I'm reading this correctly, when dumping a 9.5 server, we'll
> silently drop any roles whose names start with pg_ from the dump, and
> hope that doesn't break anything. When dumping a 9.4 or older server,
> we'll include those roles and hope that they miraculously restore on
> the new server. I'm thinking neither of those approaches is going to
> work out, and the difference between them seems totally unprincipled.
That needed to be updated to be 9.6 and 9.5, of course.
Further, you make a good point that 9.6's pg_dumpall should really
always exclude any roles which start with 'pg_', throwing a warning if
it finds them.
Note that pg_upgrade won't proceed with an upgrade of a system that has
any 'pg_' roles.
> @@ -631,7 +637,8 @@ check_is_install_user(ClusterInfo *cluster)
> res = executeQueryOrDie(conn,
> "SELECT rolsuper, oid "
> "FROM
> pg_catalog.pg_roles "
> - "WHERE rolname
> = current_user");
> + "WHERE rolname
> = current_user "
> + "AND rolname
> !~ '^pg_'");
>
> /*
> * We only allow the install user in the new cluster (see comment below)
> @@ -647,7 +654,8 @@ check_is_install_user(ClusterInfo *cluster)
>
> res = executeQueryOrDie(conn,
> "SELECT COUNT(*) "
> - "FROM
> pg_catalog.pg_roles ");
> + "FROM
> pg_catalog.pg_roles "
> + "WHERE rolname
> !~ '^pg_'");
>
> if (PQntuples(res) != 1)
> pg_fatal("could not determine the number of users\n");
>
> What bad thing would happen without these changes that would be
> avoided with these changes? I can't think of anything.
This function ("check_is_install_user") is simply checking that the user
we are logged in as is the 'install' user and that there aren't any
other users in the destination cluster. The initial check is perhaps a
bit paranoid- it shouldn't be possible for a 'pg_' role to be the one
which pg_upgrade has logged into the cluster with as none of the 'pg_'
roles have the 'login' privilege.
For the second check, pg_upgrade expects a pristine cluster to perform
the binary upgrade into, which includes checking that there aren't any
roles besides the 'install' role in the destination cluster. Since
default roles are created at initdb time, the destination cluster *will*
have other roles in it besides the install role, but only roles whose
names start with 'pg_', and those are ok because they're from initdb.
Updated (and rebased) patch attached.
Thanks for the review!
Stephen
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed * Applies cleanly to current master (3063e7a84026ced2aadd2262f75eebbe6240f85b) * ./configure && make -j4 ok DOCUMENTATION * Documentation ok, both code (code comments) and docs. * Documentation covers signalling backends/backup/monitor as well as the obvious modification to the role sections CODE * Checks on roles are fairly comprehensive: restrict reserved rolenames (creation/modification), prohibit granting to reservedroles * The patch is surprisingly non-intrusive/self-contained considering the functionality. TOOLS * Covers pg_upgrade -- "/* 9.5 and below should not have roles starting with pg_ */" * Covers pg_dumpall (do not export creation of system-reserved roles) * Includes support in psql (\dgS) + accompanying documentation REGRESSION TESTS * Includes regression tests; Seem quite complete (including GRANT/REVOKE on reserved roles) Suggestion for committer: addregression tests for each reserved role? (just for completeness' sake) * make installcheck-world passes; build on amd64 / gcc4.9.2 (Debian 4.9.2-10)- btree_gin tests fail / no contrib installed;Assumed ok * Nitpick: tests mention the still nonexistant pg_monitor / pg_backup reserved roles ; Might as well use some obviously reserved-but-absurdrolename instead Comment for future enhancement: might make sense to split role checking/access control functionality into a separate module,as opposed to having to include pg_authid.h everywhere I'm Thinking about Michael and Heikki's upcoming authentication revamp re. SCRAM/multiple authenticators: authentication!= authorization (apropos "has_privs_of_role()" ) Testing: - pg_signal_backend Ok Looking forward to seeing the other proposed default roles in! The new status of this patch is: Ready for Committer
If this gets into 9.6, we give users another full release cycle to ensure there are no reserved rolenames in use. Then, I reckon that the additional roles/system-role-based fine-grained authorization could go in for 9.7 without much trouble -- this is badly needed, IMHO Thank you, Stephen and all others who provided feedback. On 03/30/2016 01:14 PM, Jose Luis Tallon wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > * Applies cleanly to current master (3063e7a84026ced2aadd2262f75eebbe6240f85b) > * ./configure && make -j4 ok > [snip] > > Looking forward to seeing the other proposed default roles in! > > > The new status of this patch is: Ready for Committer >
Robert, José, I've rebased this on top of master and added a few additional checks and regression tests. I'm planning to continue going over the patch tomorrow morning with plans to push this before the feature freeze deadline. Thanks! Stephen
Attachment
On 04/07/2016 09:50 PM, Stephen Frost wrote:
> Robert, José,
>
> I've rebased this on top of master and added a few additional checks and
> regression tests.
Applies and compiles cleanly, of course. Passes all 164 tests, too.
- make installcheck-world ok
- interdiff checked, nothing very surprising
*Tests: using "pg_abcdef" (very unlikely to ever exist) is indeed better than
using "pg_backup" to test system 'reservedness'
*Documentation: changes seem to make it less repetitive regarding
"pg_signal_backend". Should reduce diff size when future system roles
get added ;)
*Code:
Spotted the added if (strncmp(*newval, "pg_", 3) == 0) at src/backend/commands/variable.c (plus
pre-existing)src/bin/pg_dump/pg_dumpall.c
I hadn't realized it could be needed there... I'm not familiar enough
with the code just yet.
I reckon there's no need to add a separate helper to check this at the
moment; might be needed later, when the superuser review patches get
merged :)
> I'm planning to continue going over the patch tomorrow morning with
> plans to push this before the feature freeze deadline.
Good. Thank you for the effort.
/ J.L.
On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote: > I'm planning to continue going over the patch tomorrow morning with > plans to push this before the feature freeze deadline. > --- a/src/test/regress/expected/rolenames.out > +++ b/src/test/regress/expected/rolenames.out > +GRANT testrol0 TO pg_abc; -- error > +ERROR: role "pg_abc" is reserved > +DETAIL: Cannot GRANT roles to a reserved role. The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It should block this ALTER ROLE if it blocks the corresponding GRANT.
On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote: > On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote: > > I'm planning to continue going over the patch tomorrow morning with > > plans to push this before the feature freeze deadline. > > > --- a/src/test/regress/expected/rolenames.out > > +++ b/src/test/regress/expected/rolenames.out > > > +GRANT testrol0 TO pg_abc; -- error > > +ERROR: role "pg_abc" is reserved > > +DETAIL: Cannot GRANT roles to a reserved role. > > The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It > should block this ALTER ROLE if it blocks the corresponding GRANT. One more thing: > --- a/src/bin/pg_dump/pg_dumpall.c > +++ b/src/bin/pg_dump/pg_dumpall.c > @@ -665,7 +665,7 @@ dumpRoles(PGconn *conn) > int i; > > /* note: rolconfig is dumped later */ > - if (server_version >= 90500) > + if (server_version >= 90600) This need distinct branches for 9.5 and for 9.6+. Today's code would treat a 9.5 cluster like a 9.1 cluster and fail to dump rolbypassrls attributes. > printfPQExpBuffer(buf, > "SELECT oid, rolname, rolsuper, rolinherit, " > "rolcreaterole, rolcreatedb, " > @@ -674,6 +674,7 @@ dumpRoles(PGconn *conn) > "pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment, " > "rolname = current_user AS is_current_user " > "FROM pg_authid " > + "WHERE rolname !~ '^pg_' " > "ORDER BY 2"); > else if (server_version >= 90100) > printfPQExpBuffer(buf,
On Mon, Apr 18, 2016 at 12:05 PM, Noah Misch <noah@leadboat.com> wrote:
> On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote:
>> On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote:
>> > I'm planning to continue going over the patch tomorrow morning with
>> > plans to push this before the feature freeze deadline.
>>
>> > --- a/src/test/regress/expected/rolenames.out
>> > +++ b/src/test/regress/expected/rolenames.out
>>
>> > +GRANT testrol0 TO pg_abc; -- error
>> > +ERROR: role "pg_abc" is reserved
>> > +DETAIL: Cannot GRANT roles to a reserved role.
>>
>> The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It
>> should block this ALTER ROLE if it blocks the corresponding GRANT.
Following this logic, shouldn't CREATE ROLE USER be forbidden as well?
=# create role toto1 user pg_signal_backend;
CREATE ROLE
=# create role toto2;
CREATE ROLE
=# alter role toto2 user pg_signal_backend;
ALTER ROLE
=# \dgS+ pg_signal_backend
List of roles
Role name | Attributes | Member of | Description
-------------------+--------------+---------------+-------------
pg_signal_backend | Cannot login | {toto1,toto2} |
In short a reserved role should never be member of another role/group,
as per the attached.
--
Michael
Attachment
* Noah Misch (noah@leadboat.com) wrote: > On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote: > > I'm planning to continue going over the patch tomorrow morning with > > plans to push this before the feature freeze deadline. > > > --- a/src/test/regress/expected/rolenames.out > > +++ b/src/test/regress/expected/rolenames.out > > > +GRANT testrol0 TO pg_abc; -- error > > +ERROR: role "pg_abc" is reserved > > +DETAIL: Cannot GRANT roles to a reserved role. > > The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It > should block this ALTER ROLE if it blocks the corresponding GRANT. Agreed, I specifically recall looking at that bit of code, but I think I got myself convinced that it was the other way around (that the ALTER would end up granting pg_signal_backend to testrol0, which would be fine), but you're right, this needs to be prevented also. Will fix. Thanks! Stephen
On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch <noah@leadboat.com> wrote: > On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote: >> On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote: >> > I'm planning to continue going over the patch tomorrow morning with >> > plans to push this before the feature freeze deadline. >> >> > --- a/src/test/regress/expected/rolenames.out >> > +++ b/src/test/regress/expected/rolenames.out >> >> > +GRANT testrol0 TO pg_abc; -- error >> > +ERROR: role "pg_abc" is reserved >> > +DETAIL: Cannot GRANT roles to a reserved role. >> >> The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It >> should block this ALTER ROLE if it blocks the corresponding GRANT. > > One more thing: > >> --- a/src/bin/pg_dump/pg_dumpall.c >> +++ b/src/bin/pg_dump/pg_dumpall.c >> @@ -665,7 +665,7 @@ dumpRoles(PGconn *conn) >> int i; >> >> /* note: rolconfig is dumped later */ >> - if (server_version >= 90500) >> + if (server_version >= 90600) > > This need distinct branches for 9.5 and for 9.6+. Today's code would treat a > 9.5 cluster like a 9.1 cluster and fail to dump rolbypassrls attributes. Stephen, did something in today's pile o' commits fix this? If so, which one? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote: > On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch <noah@leadboat.com> wrote: > > On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote: > >> On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote: > >> > I'm planning to continue going over the patch tomorrow morning with > >> > plans to push this before the feature freeze deadline. > >> > >> > --- a/src/test/regress/expected/rolenames.out > >> > +++ b/src/test/regress/expected/rolenames.out > >> > >> > +GRANT testrol0 TO pg_abc; -- error > >> > +ERROR: role "pg_abc" is reserved > >> > +DETAIL: Cannot GRANT roles to a reserved role. > >> > >> The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend". It > >> should block this ALTER ROLE if it blocks the corresponding GRANT. > > > > One more thing: > > > >> --- a/src/bin/pg_dump/pg_dumpall.c > >> +++ b/src/bin/pg_dump/pg_dumpall.c > >> @@ -665,7 +665,7 @@ dumpRoles(PGconn *conn) > >> int i; > >> > >> /* note: rolconfig is dumped later */ > >> - if (server_version >= 90500) > >> + if (server_version >= 90600) > > > > This need distinct branches for 9.5 and for 9.6+. Today's code would treat a > > 9.5 cluster like a 9.1 cluster and fail to dump rolbypassrls attributes. > > Stephen, did something in today's pile o' commits fix this? If so, which one? No. I had it in my list but missed it while being anxious about getting the other patches pushed. I'll push the fix shortly. Thanks! Stephen