Re: Additional role attributes && superuser review

On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> Even so, in the interest of having more fine-grained permission
> controls, I've gone ahead and added a pg_switch_xlog default role.
> Note that this means that pg_switch_xlog() can be called by both
> pg_switch_xlog roles and pg_backup roles. I'd be very much against
> removing the ability to call pg_switch_xlog from the pg_backup role as
> that really is a capability which is needed by users running backups and
> it'd just add unnecessary complexity to require users setting up backup
> tools to grant two different roles to get the backup to work.

There is going to be many opinions regarding the granularity of this
control, each one of us having a different opinion at the end. I don't
think this should be a stopper for this patch, hence I am fine with the
judgement you think is good. We could still more finely tune those default
roles later in the dev cycle of 9.6 (10.0?).

>> 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.
>
> I really dislike that.
>
>> 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.
>
> Ah, yes. I've now moved those hunks to the second patch where they
> really should have been.
>
>> Regarding patch 3, the documentation of psql should mention the new
>> subommands \dgS and \dgS+. That's a one-liner.
>
> Ah, right. I've updated the psql SGML documentation for \duS and \dgS.
> The '\h' output had already been updated. Was there something else
> which you meant above that I've missed? Note that these fixes went into
> the second patch.

Thanks, this looks good to me.

>> +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.
>
> I've removed those checks as they could get annoying on the buildfarm or
> for people doing make installcheck, as you say, but I'm not really
> thrilled that we're only testing the failure paths.

I guess that's better than nothing.

>> +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".
>
> Alright, I've changed these to have better detail messages.

@@ -183,6 +190,13 @@ pg_current_xlog_location(PG_FUNCTION_ARGS)
{
XLogRecPtr current_recptr;

+ if (!has_privs_of_role(GetUserId(), DEFAULT_ROLE_BACKUPID) &&
+ !has_privs_of_role(GetUserId(), DEFAULT_ROLE_MONITORID) &&
+ !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SWITCH_XLOGID))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser or member of
pg_monitor, pg_backup, or pg_switch_xlog to switch transaction log
files")));

I don't think you mean to refer to the switch of segments files here. Same
comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location
and pg_last_xlog_replay_location.

+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser or member of
pg_file_settings to see all config file settings")));
Should avoid abbreviations => "all configuration file settings".

<varlistentry>
- <term><literal>\dg[+] [ <link
linkend="APP-PSQL-patterns"><replaceable
class="parameter">pattern</replaceable></link> ]</literal></term>
+ <term><literal>\dgS[+] [ <link
linkend="APP-PSQL-patterns"><replaceable
class="parameter">pattern</replaceable></link> ]</literal></term>
<listitem>
I'm picky here, but that should be "\dg[S+]". Same for \du[S+].

The rest looks fine.
Regards,
--
Michael