Re: Additional role attributes && superuser review

On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> 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.

Agreed. I am not actually getting why that's part of the backup
actually. That would be more related to archiving, both being
unrelated concepts. But at this point I guess that's mainly a
philosophical split.

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

Yeah that 's what I meant. Thanks, I should have been more precise
with my words.

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

I see, that's why I got confused. I just downloaded your file and
applied it blindly with git apply or patch (don't recall which). Other
folks tend to publish that as a separate set of files generated by
git-format-patch.

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

My vote goes into hardening them a bit this way.

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

It is at least possible to check that the error code paths are
working. For the code paths where a backend would be actually running,
you could use the following:
SET client_min_messages TO 'error';
-- This should return "1" and not an ERROR nor a WARNING if PID does not exist.
select count(pg_terminate_backend(0));
But that's ugly depending on your taste.

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

Fine for me. Usually people point out such code duplications are bad
things, and here we just have a static routine being used for a set of
functions interacting with the same kind of object, here a replication
slot. But I'm fine either way.

Do you think that it makes sense to add tests as well in the second
patch to check that restrictions pg_* are in place? Except that I
don't have much more to say about patches 1 and 2 which are rather
straight-forward.

Regarding patch 3, the documentation of psql should mention the new
subommands \dgS and \dgS+. That's a one-liner.

+GRANT pg_backup TO regressuser1;
+SET SESSION AUTHORIZATION regressuser1;
+SELECT pg_start_backup('abc'); -- fail
+ERROR: WAL level not sufficient for making an online backup
+HINT: wal_level must be set to "archive", "hot_standby", or
"logical" at server start.
make install would wal on a server with something else than wal_level
= minimal. Just checking that errors kick correctly looks fine to me
here.

+GRANT pg_backup TO pg_monitor; -- error
+ERROR: role "pg_monitor" is reserved
+DETAIL: Roles starting with "pg_" are reserved.
+GRANT testrol0 TO pg_backup; -- error
+ERROR: role "pg_backup" is reserved
We would gain with verbose error messages here, like, "cannot GRANT
somerole to a system role", and "cannot GRANT system role to
somerole".
--
Michael