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

On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.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