Re: Additional role attributes && superuser review

On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Heikki Linnakangas (hlinnaka(at)iki(dot)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