Re: [HACKERS] Default Roles
* Robert Haas (robertmh...@gmail.com) wrote: > On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch 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 signature.asc Description: Digital signature
Re: [HACKERS] Default Roles
On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
* Noah Misch (n...@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 signature.asc Description: Digital signature
Re: [HACKERS] Default Roles
On Mon, Apr 18, 2016 at 12:05 PM, Noah Misch 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
catalog-acl-group.patch
Description: application/download
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
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, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
Noah, Fujii, all,
* Noah Misch (n...@leadboat.com) wrote:
> At the C level, have a pgstattuple function and a pgstattuple_v1_4 function.
> Let them differ only in that the former has a superuser check. Binary
> upgrades will use the former, and fresh CREATE EXTENSION shall use the latter.
Attached is a patch which implements this for the pgstattuple
extensions. The changes are pretty straight-forward, but I'm not going
to commit this under the gun of the feature freeze without at least
another committer reviewing it or getting an extension for a couple days
to play with it further and convince myself it's safe.
Ultimately, I'd like for this to be included in 9.6 as it'd be an
example use-case for others to follow when updating their extensions to
make use of the new pg_dump features, but I certainly don't see it as
being critical to the release.
Fujii, my apologies for not getting this done earlier, I know this is a
capability you are looking forward to having.
Thanks!
Stephen
From 0db7ebf549aeee7f04b8383ac391f349f810ef4b Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Fri, 8 Apr 2016 17:18:27 -0400
Subject: [PATCH] Remove superuser checks in pgstattuple 1.4
Now that we track initial privileges on extension objects and changes to
those permissions, we can drop the superuser() checks from the various
functions which are part of the pgstattuple extension.
Since a pg_upgrade will preserve the version of the extension which
existed prior to the upgrade, we can't simply modify the existing
functions but instead need to create new functions which remove the
checks and update the SQL-level functions to use the new functions
(and to REVOKE EXECUTE rights on those functions from PUBLIC).
Approach suggested by Noah.
---
contrib/pgstattuple/Makefile | 2 +-
contrib/pgstattuple/pgstatapprox.c| 35 ++--
contrib/pgstattuple/pgstatindex.c | 108 +++--
contrib/pgstattuple/pgstattuple--1.3--1.4.sql | 111 ++
contrib/pgstattuple/pgstattuple--1.3.sql | 95 --
contrib/pgstattuple/pgstattuple--1.4.sql | 111 ++
contrib/pgstattuple/pgstattuple.c | 36 +
contrib/pgstattuple/pgstattuple.control | 2 +-
8 files changed, 392 insertions(+), 108 deletions(-)
create mode 100644 contrib/pgstattuple/pgstattuple--1.3--1.4.sql
delete mode 100644 contrib/pgstattuple/pgstattuple--1.3.sql
create mode 100644 contrib/pgstattuple/pgstattuple--1.4.sql
diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index 6083dab..01f1feb 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,7 +4,7 @@ MODULE_big = pgstattuple
OBJS = pgstattuple.o pgstatindex.o pgstatapprox.o $(WIN32RES)
EXTENSION = pgstattuple
-DATA = pgstattuple--1.3.sql pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.4.sql pgstattuple--1.3--1.4.sql pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
PGFILEDESC = "pgstattuple - tuple-level statistics"
REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index b7734fa..5671791 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -29,6 +29,9 @@
#include "commands/vacuum.h"
PG_FUNCTION_INFO_V1(pgstattuple_approx);
+PG_FUNCTION_INFO_V1(pgstattuple_approx_v1_4);
+
+Datum pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo);
typedef struct output_type
{
@@ -209,6 +212,33 @@ Datum
pgstattuple_approx(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
+
+ if (!superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions";
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+/*
+ * As of pgstattuple version 1.4, we no longer need to check if the user
+ * is a superuser because we REVOKE EXECUTE on the function from PUBLIC.
+ * Users can then grant access to it based on their policies.
+ *
+ * Otherwise identical to pgstattuple_approx (above).
+ */
+Datum
+pgstattuple_approx_v1_4(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+Datum
+pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo)
+{
Relation rel;
output_type stat = {0};
TupleDesc tupdesc;
@@ -217,11 +247,6 @@ pgstattuple_approx(PG_FUNCTION_ARGS)
HeapTuple ret;
int i = 0;
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to use pgstattuple functions";
-
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
diff --git a/contrib/pgstattuple/pgstatinde
Re: [HACKERS] Default Roles
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 addedif (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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
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
From a0724d91ffd1a93034d5b5d5df2b4ff54339d763 Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Mon, 29 Feb 2016 21:27:46 -0500
Subject: [PATCH 1/2] Reserve the "pg_" namespace for roles
This will prevent users from creating roles which begin with "pg_" and
will check for those roles before allowing an upgrade using pg_upgrade.
This will allow for default roles to be provided at initdb time.
---
doc/src/sgml/ref/psql-ref.sgml | 8 +--
src/backend/catalog/catalog.c | 5 +++--
src/backend/commands/foreigncmds.c | 4
src/backend/commands/policy.c | 5 +
src/backend/commands/schemacmds.c | 4
src/backend/commands/user.c | 40 +
src/backend/commands/variable.c | 3 +++
src/backend/utils/adt/acl.c | 39
src/bin/pg_dump/pg_dumpall.c| 11 -
src/bin/pg_upgrade/check.c | 40 +++--
src/bin/psql/command.c | 4 ++--
src/bin/psql/describe.c | 5 -
src/bin/psql/describe.h | 2 +-
src/bin/psql/help.c | 4 ++--
src/include/utils/acl.h | 1 +
src/test/regress/expected/rolenames.out | 20 +
src/test/regress/sql/rolenames.sql | 10 +
17 files changed, 192 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d8b9a03..60f8822 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1365,13 +1365,15 @@ testdb=>
-\dg[+] [ pattern ]
+\dg[S+] [ pattern ]
Lists database roles.
(Since the concepts of users and groups have been
unified into roles, this command is now equivalent to
\du.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
If pattern is specified,
only those roles whose names match the pattern are listed.
If the form \dg+ is used, additional information
@@ -1525,13 +1527,15 @@ testdb=>
-\du[+] [ pattern ]
+\du[S+] [ pattern ]
Lists database roles.
(Since the concepts of users and groups have been
unified into roles, this command is now equivalent to
\dg.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
If pattern is specified,
only those roles whose names match the pattern are listed.
If the form \du+ is used, additional information
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index bead2c1..d1cf45b 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -184,8 +184,9 @@ IsToastNamespace(Oid namespaceId)
* True iff name starts with the pg_ prefix.
*
* For some classes of objects, the prefix pg_ is reserved for
- * system objects only. As of 8.0, this is only true for
- * schema and tablespace names.
+ * system objects only. As of 8.0, this was only true for
+ * schema and tablespace names. With 9.6, this is also true
+ * for roles.
*/
bool
IsReservedName(const char *name)
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 804bab2..c402e64 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1148,6 +1148,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
else
useId = get_rolespec_oid(stmt->user, false);
+ /* Additional check to protect reserved role names */
+ check_rolespec_name(stmt->user,
+ "Cannot specify reserved role as mapping user.");
+
/* Check that the server exists. */
srv = GetForeignServerByName(stmt->servername, false);
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 93d15e4..146b36c 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -176,8 +176,13 @@ policy_role_list_to_array(List *roles, int *num_roles)
return role_oids;
}
else
+ {
+ /* Additional check to protect reserved role names */
+ check_rolespec_name((Node *) spec,
+ "Cannot specify reserved role as policy target");
role_oids[i++] =
ObjectIdGetDatum(get_rolespec_oid((Node *) spec, false));
+ }
}
return role_oids;
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index a60ceb8..6ecfb70 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -78,6 +78,10 @@ CreateSchemaCommand(CreateSchemaStm
Re: [HACKERS] Default Roles (was: Additional role attributes)
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Sun, Apr 03, 2016 at 10:27:02PM -0400, Stephen Frost wrote: > > * Fujii Masao (masao.fu...@gmail.com) wrote: > > > Currently only superusers can call pgstattuple(). > > > > I started looking into this. > > > > If we were starting from a green field, the pg_dump dump catalog ACLs > > patch would work just fine for this case. Simply remove the superuser > > checks and REVOKE EXECUTE from public in the script and we're done. > > > > Unfortunately, we aren't, and that's where things get complicated. The > > usual pg_upgrade case will, quite correctly, dump out the objects > > exactly as they exist from the 9.5-or-earlier system and restore them > > into the 9.6 system, however, the new .so will be installed and that .so > > won't have the superuser checks in it. > > > > The only approach to addressing this which I can think of offhand would > > be to have the new .so library check the version of the extension and, > > for the 1.3 (pre-9.6) and previous versions, keep the superuser check, > > but skip it for 1.4 (9.6) and later versions. > > At the C level, have a pgstattuple function and a pgstattuple_v1_4 function. > Let them differ only in that the former has a superuser check. Binary > upgrades will use the former, and fresh CREATE EXTENSION shall use the latter. Excellent suggestion and many thanks for that. I'll draft up a patch for that. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Sun, Apr 03, 2016 at 10:27:02PM -0400, Stephen Frost wrote: > * Fujii Masao (masao.fu...@gmail.com) wrote: > > Currently only superusers can call pgstattuple(). > > I started looking into this. > > If we were starting from a green field, the pg_dump dump catalog ACLs > patch would work just fine for this case. Simply remove the superuser > checks and REVOKE EXECUTE from public in the script and we're done. > > Unfortunately, we aren't, and that's where things get complicated. The > usual pg_upgrade case will, quite correctly, dump out the objects > exactly as they exist from the 9.5-or-earlier system and restore them > into the 9.6 system, however, the new .so will be installed and that .so > won't have the superuser checks in it. > > The only approach to addressing this which I can think of offhand would > be to have the new .so library check the version of the extension and, > for the 1.3 (pre-9.6) and previous versions, keep the superuser check, > but skip it for 1.4 (9.6) and later versions. At the C level, have a pgstattuple function and a pgstattuple_v1_4 function. Let them differ only in that the former has a superuser check. Binary upgrades will use the former, and fresh CREATE EXTENSION shall use the latter. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
Fujii,
* Fujii Masao (masao.fu...@gmail.com) wrote:
> On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost wrote:
> > Possibly, but I'd need to look at them more closely than I have time to
> > right now. Can you provide a use-case? That would certainly help.
>
> I have seen the monitoring system which periodically executes
> the statement like
>
> SELECT * FROM pgstattuple('hoge');
>
> to monitor the relation's physical length, the number of dead
> tuples, etc. Then, for example, if the number of dead tuples is
> increased unexpectedly and the relation becomes bloated, DBA tries
> to find out the cause and execute the maintenance commands
> if necessary to alleviate the situation. The monitoring system calls
> pgstattuple() at off-peak times because pgstattuple() needs to
> scan all the pages in the relation and its performance penalty
> might be big.
[...]
> Currently only superusers can call pgstattuple().
I started looking into this.
If we were starting from a green field, the pg_dump dump catalog ACLs
patch would work just fine for this case. Simply remove the superuser
checks and REVOKE EXECUTE from public in the script and we're done.
Unfortunately, we aren't, and that's where things get complicated. The
usual pg_upgrade case will, quite correctly, dump out the objects
exactly as they exist from the 9.5-or-earlier system and restore them
into the 9.6 system, however, the new .so will be installed and that .so
won't have the superuser checks in it.
The only approach to addressing this which I can think of offhand would
be to have the new .so library check the version of the extension and,
for the 1.3 (pre-9.6) and previous versions, keep the superuser check,
but skip it for 1.4 (9.6) and later versions.
I'm certainly open to other suggestions, of course. Would be great to
remove those superuser() checks and allow non-superusers to be GRANT'd
the right to run those functions, as discussed.
Thanks!
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Default Roles
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
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 reserved roles * 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: add regression 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-absurd rolename 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost 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
From 090994ca5a9ae0c64f8752b43801141582f31af7 Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Mon, 29 Feb 2016 21:27:46 -0500
Subject: [PATCH 1/2] Reserve the "
Re: [HACKERS] Default Roles
On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Wed, Nov 18, 2015 at 5:36 AM, Stephen Frost wrote: > Michael, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> Will there be any work on this patch for this commit fest or not? This >> is being carried around for a couple of months now with not much >> progress. This thread is idle for 4 months now. > > This thread and the other one kind of merged. The last update on the > overall discussion is here: > > http://www.postgresql.org/message-id/2015093020.gm3...@tamriel.snowman.net Right. The CF entry links to two threads, and I somehow missed the first one. > Which was closer to 1.5 months ago and was the requested split of the > patch, which mainly needs to get review and/or buy-in from others on the > reservation of the role prefix 'pg_' (which is the first patch). > I'm happy to update the patches if they don't apply, of course, but > they're relatively straight-forward and we just need to agree that > reservation of the prefix is acceptable and then I can just commit > them. I'll reply directly on the other thread, there is no meaning to mess up things here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > Will there be any work on this patch for this commit fest or not? This > is being carried around for a couple of months now with not much > progress. This thread is idle for 4 months now. This thread and the other one kind of merged. The last update on the overall discussion is here: http://www.postgresql.org/message-id/2015093020.gm3...@tamriel.snowman.net Which was closer to 1.5 months ago and was the requested split of the patch, which mainly needs to get review and/or buy-in from others on the reservation of the role prefix 'pg_' (which is the first patch). I'm happy to update the patches if they don't apply, of course, but they're relatively straight-forward and we just need to agree that reservation of the prefix is acceptable and then I can just commit them. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Default Roles (was: Additional role attributes)
Stephen,
On Tue, Jul 14, 2015 at 9:22 PM, Fujii Masao wrote:
>
> On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost wrote:
> > Fujii,
> >
> > * Fujii Masao (masao.fu...@gmail.com) wrote:
> >> he documents of the functions which the corresponding default roles
> >> are added by this patch need to be updated. For example, the description
> >> of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
> >> to superusers).". I think that the description needs to mention
> >> the corresponding default role "pg_replay". Otherwise, it's difficult for
> >> users to see which default role is related to the function they want to
> >> use.
> >> Or probably we can add the table explaining all the relationships between
> >> default roles and corresponding operations. And it's useful.
> >
> > Certainly, totally agree that we need to make it clear in the function
> > descriptions also.
> >
> >> Why do we allow users to change the attributes of default roles?
> >> For example, ALTER ROLE default_role or GRANT ... TO default_role.
> >> Those changes are not dumped by pg_dumpall. So if users change
> >> the attributes for some reasons but they disappear via pg_dumpall,
> >> maybe the system goes into unexpected state.
> >
> > Good point. I'm fine with simply disallowing that completely; does
> > anyone want to argue that we should allow superusers to ALTER or GRANT
> > to these roles? I have a hard time seeing the need for that and it
> > could make things quite ugly.
> >
> >> I think that it's better to allow the roles with pg_monitor to
> >> execute pgstattuple functions. They are usually used for monitoring.
> >> Thought?
> >
> > Possibly, but I'd need to look at them more closely than I have time to
> > right now. Can you provide a use-case? That would certainly help.
>
> I have seen the monitoring system which periodically executes
> the statement like
>
> SELECT * FROM pgstattuple('hoge');
>
> to monitor the relation's physical length, the number of dead
> tuples, etc. Then, for example, if the number of dead tuples is
> increased unexpectedly and the relation becomes bloated, DBA tries
> to find out the cause and execute the maintenance commands
> if necessary to alleviate the situation. The monitoring system calls
> pgstattuple() at off-peak times because pgstattuple() needs to
> scan all the pages in the relation and its performance penalty
> might be big.
>
> > Also, we are mostly focused on things which are currently superuser-only
> > capabilities, if you don't need to be superuser today then the
> > monitoring system could be granted access using the normal mechanisms.
>
> Currently only superusers can call pgstattuple().
Will there be any work on this patch for this commit fest or not? This
is being carried around for a couple of months now with not much
progress. This thread is idle for 4 months now.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost wrote:
> Fujii,
>
> * Fujii Masao (masao.fu...@gmail.com) wrote:
>> he documents of the functions which the corresponding default roles
>> are added by this patch need to be updated. For example, the description
>> of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
>> to superusers).". I think that the description needs to mention
>> the corresponding default role "pg_replay". Otherwise, it's difficult for
>> users to see which default role is related to the function they want to use.
>> Or probably we can add the table explaining all the relationships between
>> default roles and corresponding operations. And it's useful.
>
> Certainly, totally agree that we need to make it clear in the function
> descriptions also.
>
>> Why do we allow users to change the attributes of default roles?
>> For example, ALTER ROLE default_role or GRANT ... TO default_role.
>> Those changes are not dumped by pg_dumpall. So if users change
>> the attributes for some reasons but they disappear via pg_dumpall,
>> maybe the system goes into unexpected state.
>
> Good point. I'm fine with simply disallowing that completely; does
> anyone want to argue that we should allow superusers to ALTER or GRANT
> to these roles? I have a hard time seeing the need for that and it
> could make things quite ugly.
>
>> I think that it's better to allow the roles with pg_monitor to
>> execute pgstattuple functions. They are usually used for monitoring.
>> Thought?
>
> Possibly, but I'd need to look at them more closely than I have time to
> right now. Can you provide a use-case? That would certainly help.
I have seen the monitoring system which periodically executes
the statement like
SELECT * FROM pgstattuple('hoge');
to monitor the relation's physical length, the number of dead
tuples, etc. Then, for example, if the number of dead tuples is
increased unexpectedly and the relation becomes bloated, DBA tries
to find out the cause and execute the maintenance commands
if necessary to alleviate the situation. The monitoring system calls
pgstattuple() at off-peak times because pgstattuple() needs to
scan all the pages in the relation and its performance penalty
might be big.
> Also, we are mostly focused on things which are currently superuser-only
> capabilities, if you don't need to be superuser today then the
> monitoring system could be granted access using the normal mechanisms.
Currently only superusers can call pgstattuple().
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
Fujii, * Fujii Masao (masao.fu...@gmail.com) wrote: > he documents of the functions which the corresponding default roles > are added by this patch need to be updated. For example, the description > of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted > to superusers).". I think that the description needs to mention > the corresponding default role "pg_replay". Otherwise, it's difficult for > users to see which default role is related to the function they want to use. > Or probably we can add the table explaining all the relationships between > default roles and corresponding operations. And it's useful. Certainly, totally agree that we need to make it clear in the function descriptions also. > Why do we allow users to change the attributes of default roles? > For example, ALTER ROLE default_role or GRANT ... TO default_role. > Those changes are not dumped by pg_dumpall. So if users change > the attributes for some reasons but they disappear via pg_dumpall, > maybe the system goes into unexpected state. Good point. I'm fine with simply disallowing that completely; does anyone want to argue that we should allow superusers to ALTER or GRANT to these roles? I have a hard time seeing the need for that and it could make things quite ugly. > I think that it's better to allow the roles with pg_monitor to > execute pgstattuple functions. They are usually used for monitoring. > Thought? Possibly, but I'd need to look at them more closely than I have time to right now. Can you provide a use-case? That would certainly help. Also, we are mostly focused on things which are currently superuser-only capabilities, if you don't need to be superuser today then the monitoring system could be granted access using the normal mechanisms. Actually logging systems won't log in directly as "pg_monitor" anyway, they'll log in as "nagios" or similar, which has been GRANT'd "pg_monitor" and could certainly be GRANT'd other rights also. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Wed, May 13, 2015 at 12:07 PM, Stephen Frost wrote: > All, > > This patch gets smaller and smaller. > > Upon reflection I realized that, with default roles, it's entirely > unnecssary to change how the permission checks happen today- we can > simply add checks to them to be looking at role membership also. That's > removed the last of my concerns regarding any API breakage for existing > use-cases and has greatly simplified things overall. > > This does change the XLOG functions to require pg_monitor, as discussed > on the other thread where it was pointed out by Heikki that the XLOG > location information could be used to extract sensitive information > based on what happens during compression. Adding docs explaining that > is on my to-do list for tomorrow. > > * Stephen Frost (sfr...@snowman.net) wrote: >> Andres suggested that we drop the REPLICATION role attribute and just >> use membership in pg_replication instead. That's turned out quite >> fantastically as we can now handle upgrades without breaking anything- >> CREATE ROLE and ALTER ROLE still accept the attribute but simply grant >> pg_replication to the role instead, and postinit.c has been changed to >> check role membership similar to other pg_hba role membership checks >> when a replication connection comes in. Hat's off to Andres for his >> suggestion. > > It's also unnecessary to change how the REPLICATION role attribute > functions today. This patch does add the pg_replication role, but it's > only allowed to execute the various pg_logical and friends functions and > not to actually connect as a REPLICATION user. Connecting as a > REPLICATION user allows you to stream the entire contents of the > backend, after all, so it makes sense to have that be independent. > > I added another default role which allows the user to view > pg_show_file_settings, as that seemed useful to me. The diffstat for > that being something like 4 additions without docs and maybe 10 with. > More documentation would probably be good though and I'll look at adding > to it. > > Most of the rest of what I've done has simply been reverting back to > what we had. The patch is certainly far easier to verify by reading > through it now, as the changes are right next to each other, and the > regression output changes are much smaller. > > Thoughts? Comments? Suggestions? he documents of the functions which the corresponding default roles are added by this patch need to be updated. For example, the description of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted to superusers).". I think that the description needs to mention the corresponding default role "pg_replay". Otherwise, it's difficult for users to see which default role is related to the function they want to use. Or probably we can add the table explaining all the relationships between default roles and corresponding operations. And it's useful. Why do we allow users to change the attributes of default roles? For example, ALTER ROLE default_role or GRANT ... TO default_role. Those changes are not dumped by pg_dumpall. So if users change the attributes for some reasons but they disappear via pg_dumpall, maybe the system goes into unexpected state. I think that it's better to allow the roles with pg_monitor to execute pgstattuple functions. They are usually used for monitoring. Thought? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
Robert Haas writes: > Now, if six people who are all well-known PostgreSQL contributors show > up and they all say "I looked at the latest version of this carefully > and I'm confident you've got it right", then go ahead: push it. But > don't make the mistake of thinking that because you're confident that > you've now got it right everybody else will like it too. Even since > you posted the last version, Heikki expressed a concern that resulted > in (surprise!) more revisions. There comes a point where a patch that > is still heavily in flux is just too late for the release cycle, and > we're well past that point at this stage of the game. FWIW, I agree that we're past the point where we should be committing features whose external definition hasn't been stable for awhile. Fixing bugs post-feature-freeze is one thing, but if there's a significant chance that you'll be having to adjust the feature definition, then it's probably too late for 9.5. And this particular item sure looks like it's in that category. There's always another release cycle. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Wed, May 13, 2015 at 11:50 AM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> Yes: let's punt this to 9.6. The decisions you're making here are way >> too significant to be making a couple of days before feature freeze, >> and this patch has changed massively since it was first submitted. >> There isn't time now for people who want to have an opinion on this to >> form an educated one. > > Perhaps I'm missing something, but the patch has been simplified down to > the point where the only question seems to be "should we have default > roles or not?", which I had asked about two weeks ago and again last > week on a new thread. I feel like we're waiting for the silent majority > to chime in. The thing is, right now, there are many, many patches in flight and everybody is really, really busy with them. What we should be trying to push in right now are the patches that we know we want, and there are at most a few minor implementation details to sort out. We should not be trying to push in any patches where we are not confident in the design. I really don't see how you can be confident that this design will have the backing of the community at this point. It's changed in major ways, multiple times. The latest version, again majorly revised, was posted TWO DAYS before feature freeze. Two days is not enough time to get meaningful feedback on significant design decisions under the best of circumstances, and even less so when those two days are the last remaining days before feature freeze. Now, if six people who are all well-known PostgreSQL contributors show up and they all say "I looked at the latest version of this carefully and I'm confident you've got it right", then go ahead: push it. But don't make the mistake of thinking that because you're confident that you've now got it right everybody else will like it too. Even since you posted the last version, Heikki expressed a concern that resulted in (surprise!) more revisions. There comes a point where a patch that is still heavily in flux is just too late for the release cycle, and we're well past that point at this stage of the game. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
* Robert Haas (robertmh...@gmail.com) wrote: > Yes: let's punt this to 9.6. The decisions you're making here are way > too significant to be making a couple of days before feature freeze, > and this patch has changed massively since it was first submitted. > There isn't time now for people who want to have an opinion on this to > form an educated one. Perhaps I'm missing something, but the patch has been simplified down to the point where the only question seems to be "should we have default roles or not?", which I had asked about two weeks ago and again last week on a new thread. I feel like we're waiting for the silent majority to chime in. Put another way, I'm afraid that posting this next week, next month, or next year is going to garner just as many responses as it's seen in the past 2 weeks, while I continue to field questions on -bugs, -admin, and IRC about "how do I set up Nagios with a non-superuser account?" and similar issues. It's not a novel idea, certainly; Magnus suggested it back in December on the thread, Tom made a similar comment that it might make sense to have them later on and it's come up quite a few times previously as it's something other RDBMS's have and we don't. Clearly, others have read the proposal, at least (You and Alvaro on the other thread, Heikki on this one). It's my fault that I didn't follow up on their suggestions earlier and instead spent a bunch of time fighting with pg_dump, but it doesn't seem like there is a lot of disagreement about the idea. I'd offer to simplify it down, but it seems like the obvious change in that direction would be to reserve pg_ as a role prefix and not actually create any default roles, but that doesn't gain us anything and makes a potential headache for users without any feature to go with it. Bruce's point is a better one, except that all of the changes have been about reducing changes to core, down to an almost trivial level. I wish it had been a smoother ride to get here from the original proposal six months ago, but I've certainly got a better understanding of the level of effort involved and changes required for the other approaches and this certainly seems like the best and simplest. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Wed, May 13, 2015 at 10:16:39AM -0400, Robert Haas wrote: > On Tue, May 12, 2015 at 11:07 PM, Stephen Frost wrote: > > Thoughts? Comments? Suggestions? > > Yes: let's punt this to 9.6. The decisions you're making here are way > too significant to be making a couple of days before feature freeze, > and this patch has changed massively since it was first submitted. > There isn't time now for people who want to have an opinion on this to > form an educated one. Yeah, pretty much any patch that needs significant redesign at this point should be kept for 9.6. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
On Tue, May 12, 2015 at 11:07 PM, Stephen Frost wrote: > Thoughts? Comments? Suggestions? Yes: let's punt this to 9.6. The decisions you're making here are way too significant to be making a couple of days before feature freeze, and this patch has changed massively since it was first submitted. There isn't time now for people who want to have an opinion on this to form an educated one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
All,
* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> On 05/13/2015 06:07 AM, Stephen Frost wrote:
> >This does change the XLOG functions to require pg_monitor, as discussed
> >on the other thread where it was pointed out by Heikki that the XLOG
> >location information could be used to extract sensitive information
> >based on what happens during compression.
>
> That seems like an orthogonal issue, not something that should be
> bundled in this patch. IIRC we didn't reach a consensus on what to
> do about the compression-leaks-information issue. One idea was to
> make it configurable on a per-table basis, and if we do that,
> perhaps we don't need to restrict access to
> pg_current_xlog_location() and friends.
Updated patch attached which removes the changes to the XLOG location
functions and adds checks for AlterRole and RenameRole to prevent
altering or renaming the default roles. Also adds '\duS'/'\dgS'
support to psql, to show default roles only when asked.
Thanks!
Stephen
From 88615d712892220cfe4c338317842e368f1fb62e Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Thu, 7 May 2015 23:35:03 -0400
Subject: [PATCH] Create default roles for administrative functions
To reduce the number of users on a system who are superusers,
create a set of roles by default during initdb which are allowed to run
certain functions that allow non-superusers to perform specific
administrative tasks and have access to privileged information.
The prefix "pg_" is reserved for default system roles, similar to
schemas and tablespaces. pg_upgrade is modified to check for any roles
which start with "pg_" and complain if they exist. pg_dumpall is
modified to not dump out roles starting with "pg_" on 9.5-and-above
systems. CreateRole is modified to refuse creation of roles which start
with "pg_", similar to CreateSchema. psql only shows default roles when
called with \duS or \dgS, similar to other system objects.
Roles created are: pg_backup, pg_monitor, pg_replay, pg_replication,
pg_rotate_logfile, pg_signal_backend, pg_file_settings and pg_admin.
---
contrib/test_decoding/expected/permissions.out | 8 +--
doc/src/sgml/catalogs.sgml | 9 +++
doc/src/sgml/user-manag.sgml | 91 ++
src/backend/access/transam/xlogfuncs.c | 30 +
src/backend/catalog/catalog.c | 5 +-
src/backend/catalog/system_views.sql | 6 +-
src/backend/commands/user.c| 30 +++--
src/backend/replication/logical/logicalfuncs.c | 17 ++---
src/backend/replication/slotfuncs.c| 29
src/backend/replication/walsender.c| 8 ++-
src/backend/utils/adt/misc.c | 12 ++--
src/backend/utils/adt/pgstatfuncs.c| 25 ---
src/backend/utils/misc/guc.c | 7 ++
src/bin/pg_dump/pg_dumpall.c | 2 +
src/bin/pg_upgrade/check.c | 40 ++-
src/bin/psql/command.c | 4 +-
src/bin/psql/describe.c| 5 +-
src/bin/psql/describe.h| 2 +-
src/bin/psql/help.c| 4 +-
src/include/catalog/pg_authid.h| 19 +-
20 files changed, 280 insertions(+), 73 deletions(-)
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index 212fd1d..79a7f86 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,13 @@ RESET ROLE;
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: must be superuser or member of pg_replication to use replication slots
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for relation lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: must be superuser or member of pg_replication to use replication slots
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: must be superuser or member of pg_replication to use replication slots
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE lr_superuser;
@@ -90,7 +90,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
RESET ROLE;
SET ROLE lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: must be superuser or member of pg_replication to use replication slots
RESET ROLE;
-- all users can see existing slots
SET
Re: [HACKERS] Default Roles
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 05/13/2015 06:07 AM, Stephen Frost wrote: > >This does change the XLOG functions to require pg_monitor, as discussed > >on the other thread where it was pointed out by Heikki that the XLOG > >location information could be used to extract sensitive information > >based on what happens during compression. > > That seems like an orthogonal issue, not something that should be > bundled in this patch. IIRC we didn't reach a consensus on what to > do about the compression-leaks-information issue. One idea was to > make it configurable on a per-table basis, and if we do that, > perhaps we don't need to restrict access to > pg_current_xlog_location() and friends. Alright, I'll pull it out. I see it's already been added to the open-items list, so we shouldn't forget about it. For my 2c, I'd much rather have the information restricted to a privileged role instead of having to disable the feature. Further, all tables need to be considered as having privileged information, not just systems ones like pg_authid, as the user might not have rights on the other columns or rows in the table. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Default Roles
On 05/13/2015 06:07 AM, Stephen Frost wrote: This does change the XLOG functions to require pg_monitor, as discussed on the other thread where it was pointed out by Heikki that the XLOG location information could be used to extract sensitive information based on what happens during compression. That seems like an orthogonal issue, not something that should be bundled in this patch. IIRC we didn't reach a consensus on what to do about the compression-leaks-information issue. One idea was to make it configurable on a per-table basis, and if we do that, perhaps we don't need to restrict access to pg_current_xlog_location() and friends. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles (was: Additional role attributes)
All,
This patch gets smaller and smaller.
Upon reflection I realized that, with default roles, it's entirely
unnecssary to change how the permission checks happen today- we can
simply add checks to them to be looking at role membership also. That's
removed the last of my concerns regarding any API breakage for existing
use-cases and has greatly simplified things overall.
This does change the XLOG functions to require pg_monitor, as discussed
on the other thread where it was pointed out by Heikki that the XLOG
location information could be used to extract sensitive information
based on what happens during compression. Adding docs explaining that
is on my to-do list for tomorrow.
* Stephen Frost (sfr...@snowman.net) wrote:
> Andres suggested that we drop the REPLICATION role attribute and just
> use membership in pg_replication instead. That's turned out quite
> fantastically as we can now handle upgrades without breaking anything-
> CREATE ROLE and ALTER ROLE still accept the attribute but simply grant
> pg_replication to the role instead, and postinit.c has been changed to
> check role membership similar to other pg_hba role membership checks
> when a replication connection comes in. Hat's off to Andres for his
> suggestion.
It's also unnecessary to change how the REPLICATION role attribute
functions today. This patch does add the pg_replication role, but it's
only allowed to execute the various pg_logical and friends functions and
not to actually connect as a REPLICATION user. Connecting as a
REPLICATION user allows you to stream the entire contents of the
backend, after all, so it makes sense to have that be independent.
I added another default role which allows the user to view
pg_show_file_settings, as that seemed useful to me. The diffstat for
that being something like 4 additions without docs and maybe 10 with.
More documentation would probably be good though and I'll look at adding
to it.
Most of the rest of what I've done has simply been reverting back to
what we had. The patch is certainly far easier to verify by reading
through it now, as the changes are right next to each other, and the
regression output changes are much smaller.
Thoughts? Comments? Suggestions?
Thanks!
Stephen
From 698a74ea2b627bf1a75babe177817da8dfcae464 Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Thu, 7 May 2015 23:35:03 -0400
Subject: [PATCH] Create default roles for administrative functions
To reduce the number of users on a system who are superusers,
create a set of roles by default during initdb which are allowed to run
certain functions that allow non-superusers to perform specific
administrative tasks and have access to privileged information.
The prefix "pg_" is reserved for default system roles, similar to
schemas and tablespaces. pg_upgrade is modified to check for any roles
which start with "pg_" and complain if they exist. pg_dumpall is
modified to not dump out roles starting with "pg_" on 9.5-and-above
systems. CreateRole is modified to refuse creation of roles which start
with "pg_", similar to CreateSchema.
Roles created are: pg_backup, pg_monitor, pg_replay, pg_replication,
pg_rotate_logfile, pg_signal_backend, pg_file_settings and pg_admin.
The XLOG location information functions now requires the pg_monitor role
as the compression rate can be used to derive sensitive information.
---
contrib/test_decoding/expected/permissions.out | 8 +--
doc/src/sgml/catalogs.sgml | 9 +++
doc/src/sgml/user-manag.sgml | 91 ++
src/backend/access/transam/xlogfuncs.c | 50 ++
src/backend/catalog/catalog.c | 5 +-
src/backend/catalog/system_views.sql | 6 +-
src/backend/commands/user.c| 13 +++-
src/backend/replication/logical/logicalfuncs.c | 17 ++---
src/backend/replication/slotfuncs.c| 29
src/backend/replication/walsender.c| 8 ++-
src/backend/utils/adt/misc.c | 12 ++--
src/backend/utils/adt/pgstatfuncs.c| 25 ---
src/backend/utils/misc/guc.c | 7 ++
src/bin/pg_dump/pg_dumpall.c | 2 +
src/bin/pg_upgrade/check.c | 40 ++-
src/include/catalog/pg_authid.h| 19 +-
16 files changed, 277 insertions(+), 64 deletions(-)
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index 212fd1d..79a7f86 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,13 @@ RESET ROLE;
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: must be superuser or member of pg_replication to use replicat
Re: [HACKERS] Default Roles (was: Additional role attributes)
All, * Stephen Frost (sfr...@snowman.net) wrote: > Starting a new thread, as suggested by Robert, for consideration of > adding default roles for sets of administrative functions, therefore > removing the need for superuser-level roles in many use-cases. [...] > This is part 2 and really the "guts" of the changes proposed. Part 1 > was the patch sent earlier today to change pg_stat_get_activity() to use > a tuplestore, and this patch depends on that one. I'll rebase and > resend after that's gone in. I did notice that Andres just pushed > upsert though, and it wouldn't surprise me if there are now merge > conflicts. Will address all of that tomorrow, in any case. Here's a rebase with a few additional items as follows. Andres suggested that we drop the REPLICATION role attribute and just use membership in pg_replication instead. That's turned out quite fantastically as we can now handle upgrades without breaking anything- CREATE ROLE and ALTER ROLE still accept the attribute but simply grant pg_replication to the role instead, and postinit.c has been changed to check role membership similar to other pg_hba role membership checks when a replication connection comes in. Hat's off to Andres for his suggestion. I've added two more default roles, since it was pointed out to me that I hadn't exactly mimicked the role attributes originally proposed. These are pg_rotate_logfile and pg_signal_backend. This also removes any direct object grants to pg_admin; it now means only "all of the other roles combined" without anything additional. Documentation and regression tests updated. Comments and suggestions are most welcome, as always. Thanks! Stephen From 381a3e619b8450a0f3a5225d70098fcb8291fd52 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 7 May 2015 23:35:03 -0400 Subject: [PATCH] Create default roles for administrative functions To reduce the number of users on a system who are superusers, create a set of roles by default during initdb which are granted rights to certain functions and views that allow non-superusers to perform specific administrative tasks and have access to privileged information. The prefix "pg_" is reserved for default system roles, similar to schemas and tablespaces. pg_upgrade is modified to check for any roles which start with "pg_" and complain if they exist. pg_dumpall is modified to not dump out roles starting with "pg_" on 9.5-and-above systems. CreateRole is modified to refuse creation of roles which start with "pg_", similar to CreateSchema. Roles created are: pg_backup, pg_monitor, pg_replay, pg_replication, pg_rotate_logfile, pg_signal_backend and pg_admin. Behavior of existing system views is unchanged. Views and functions are added for pg_stat_activity_all and pg_stat_replication_all, to provide unfiltered results for users granted the pg_monitor role. is_superuser() checks are removed and EXECUTE revoked from public and instead granted to the appropriate role for appropriate administrative functions. Role attribute REPLICATION is superseded by the pg_replication role and therefore removed. CREATE ROLE and ALTER ROLE will still accept the option and transform it into a GRANT pg_replication TO role; to facilitate upgrades from older versions. --- contrib/test_decoding/expected/permissions.out | 8 +- doc/src/sgml/catalogs.sgml | 32 +--- doc/src/sgml/ref/alter_role.sgml | 5 +- doc/src/sgml/ref/create_role.sgml | 16 -- doc/src/sgml/ref/createuser.sgml | 22 --- doc/src/sgml/ref/pg_basebackup.sgml| 4 +- doc/src/sgml/ref/pg_receivexlog.sgml | 4 +- doc/src/sgml/user-manag.sgml | 87 ++ src/backend/access/transam/xlogfuncs.c | 30 src/backend/catalog/catalog.c | 5 +- src/backend/catalog/system_views.sql | 134 ++- src/backend/commands/user.c| 61 +-- src/backend/replication/logical/logicalfuncs.c | 11 -- src/backend/replication/slotfuncs.c| 15 -- src/backend/replication/walsender.c| 92 -- src/backend/utils/adt/misc.c | 64 +-- src/backend/utils/adt/pgstatfuncs.c| 103 +++- src/backend/utils/init/miscinit.c | 18 -- src/backend/utils/init/postinit.c | 2 +- src/bin/pg_dump/pg_dumpall.c | 17 +- src/bin/pg_upgrade/check.c | 40 - src/bin/psql/describe.c| 2 +- src/bin/psql/tab-complete.c| 16 +- src/bin/scripts/createuser.c | 15 -- src/include/catalog/pg_authid.h| 31 +++- src/include/catalog/pg_proc.h | 6 + src/include/miscadmin.h| 1 - src/include/replication/walsender.h| 1 + src/include/utils/builtins.h | 1 +
