Re: Additional role attributes && superuser review

On Wed, Nov 18, 2015 at 10:06 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

>
>
> 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.
>

Bonus:
-static void
-check_permissions(void)
-{
- if (!superuser() && !has_rolreplication(GetUserId()))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser or replication
role to use replication slots"))));
-}
I would have let check_permissions and directly done the checks on
has_rolreplication and has_privs_of_role in it, the same code being
duplicated three times.
--
Michael