Re: Additional role attributes && superuser review - Mailing list pgsql-hackers

On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Heikki Linnakangas (hlinnaka@iki.fi) wrote:
>> I agree with Robert's earlier point that this needs to be split into
>> multiple patches, which can then be reviewed and discussed
>> separately. Pending that, I'm going to mark this as "Waiting on
>> author" in the commitfest.
>
> Attached is an initial split which divides up reserving the role names
> from actually adding the initial set of default roles.

I have begun looking at this patch, and here are some comments after screening it.

Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c (see attached for the rebase. none of the comments mentioning issues are fixed by it).

=# grant pg_replay to pg_backup  ;
GRANT ROLE
=# \du pg_backup
             List of roles
 Role name |  Attributes  |  Member of
-----------+--------------+-------------
 pg_backup | Cannot login | {pg_replay}
Perhaps we should restrict granting a default role to another default role?

+  <para>
+   Also note that changing the permissions on objects in the system
+   catalogs, while possible, is unlikely to have the desired effect as
+   the internal lookup functions use a cache and do not check the
+   permissions nor policies of tables in the system catalog.  Further,
+   permission changes to objects in the system catalogs are not
+   preserved by pg_dump or across upgrades.
+  </para>
This bit could be perhaps applied as a separate patch.

+      <row>
+       <entry>pg_backup</entry>
+       <entry>Start and stop backups, switch xlogs, and create restore points.</entry>
+      </row>
s/switch xlogs/switch to a new transaction log file/
It seems weird to not have a dedicated role for pg_switch_xlog.

In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and pg_xlog_replay_resume still mention that those functions are restricted only to superusers. The documentation needs an update. It would be good to double-check if there are similar mistakes for the other functions.

+       <entry>pg_montior</entry>
Typo, pg_monitor.

+       <entry>Pause and resume xlog replay on replicas.</entry>
Those descriptions should be complete sentences or not? Both styles are mixed in user-manag.sgml.

+      <row>
+       <entry>pg_replication</entry>
+       <entry>Create, destroy, and work with replication slots.</entry>
+      </row>
pg_replication_slots would be more adapted?

+      <row>
+       <entry>pg_file_settings</entry>
+       <entry>Allowed to view config settings from all config files</entry>
+      </row>
I would say "configuration files" instead. An abbreviation is not adapted.

+       <entry>pg_admin</entry>
+       <entry>
+        Granted pg_backup, pg_monitor, pg_reply, pg_replication,
+        pg_rotate_logfile, pg_signal_backend and pg_file_settings roles.
+       </entry>
Typo, s/pg_reply/pg_replay and I think that there should be <literal> markups around those role names. I am actually not convinced that we actually need it.

+       if (IsReservedName(stmt->role))
+               ereport(ERROR,
+                               (errcode(ERRCODE_RESERVED_NAME),
+                                errmsg("role name \"%s\" is reserved",
+                                        stmt->role),
+                                errdetail("Role names starting with \"pg_\" are reserved.")));
Perhaps this could be a separate change? It seems that restricting roles with pg_ on the cluster is not a bad restriction in any case. You may want to add regression tests to trigger those errors as well.

Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a restriction category like pg_monitor? I recall of course that we discussed that some months ago and that a lot of people were reluctant to harden this area to not break any existing monitoring tool, still this patch may be the occasion to restrict a bit those functions, particularly on servers where wal_compression is enabled.

It would be nice to have regression tests as well to check that role restrictions are applied where they should.
Regards,
--
Michael