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

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