Re: Additional role attributes && superuser review

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Additional role attributes && superuser review
Date: 2015-11-18 22:10:30
Message-ID: 20151118221030.GC3685@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Michael,

Thanks for the review!

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> 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).

Done (did it a bit differently from what you had, to hopefully avoid
future OID conflicts and also to allow us a bit of room to add
additional default roles later, if we choose, in nearby OID space).

> =# 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?

Done (in the second patch in the series, the one reserving 'pg_').

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

Done as a separate patch (first one in the series).

> + <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/

Fixed.

> It seems weird to not have a dedicated role for pg_switch_xlog.

I didn't add a pg_switch_xlog default role in this patch series, but
would be happy to do so if that's the consensus. It's quite easy to do.

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

Fixed. Also did a review and found a number of other places which
required updating, so those have also been fixed.

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

Fixed.

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

I didn't take any action on this.

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

Perhaps... I didn't make this change, but if others agree that adding
"_slots" would help, I'd be happy to do so. Personally, I'd like to
keep these names shorter, if possible, but I don't want it to be
confusing either.

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

Done.

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

I added <literal> markups around the role names, where used.

I'm guessing you were referring to pg_admin with your comment that you
were "not convinced that we actually need it." After thinking about
it for a bit, I tend to agree. A user could certainly create their own
role which combines these all together if they wanted to (or do any
other combinations, based on their particular needs). I've gone ahead
and removed pg_admin from the set of default roles.

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

I'm a bit confused- this is a separate change. Note that the previous
patch was actually a git patchset which had two patches- one to do the
reservation and a second to add the default roles. The attached
patchset is actually three patches:

1- Update to catalog documentation regarding permissions
2- Reserve 'pg_' role namespace
3- Add default roles

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

For my 2c, I believe they should. I've not modified them in that way in
this patchset, but I can certainly do so if others agree.

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

I've added regression tests for the default roles where it was
relatively straight-forward to do so. I don't see a trivial way to test
that the pg_signal_backend role works though, as an example.

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

For my 2c, I dislike the notion of "check_permissions()" functions being
added throughout the codebase as I'm afraid it'd get confusing, which is
why I got rid of it. I'm much happier seeing the actual permissions
check as it should be happening early on in the primary function which
is being called into. If there is really a push to go back to having a
'check_permissions()' like function, I'd argue that we should make the
function name more descriptive of what's actually being done and have
them be distinct from each other. As I recall, I discussed this change
with Andres and he didn't feel particularly strongly about this one way
or the other, therefore, I've not made this change for this patchset.

Thanks!

Stephen

Attachment Content-Type Size
default_roles_v7.patch text/x-diff 52.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2015-11-18 22:19:43 Re: Bug in numeric multiplication
Previous Message Robert Haas 2015-11-18 20:55:05 Re: Patch: Implement failover on libpq connect level.