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

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Wed, Nov 18, 2015 at 10:06 PM, Michael
Paquier<span dir="ltr"><<a href="mailto:michael.paquier@gmail.com"
target="_blank">michael.paquier@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0px
0px0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span class=""><br /><br />On Wed,
Sep30, 2015 at 8:11 PM, Stephen Frost <<a href="mailto:sfrost@snowman.net"
target="_blank">sfrost@snowman.net</a>>wrote:<br />> * Heikki Linnakangas (<a href="mailto:hlinnaka@iki.fi"
target="_blank">hlinnaka@iki.fi</a>)wrote:<br />>> I agree with Robert's earlier point that this needs to be
splitinto<br />>> multiple patches, which can then be reviewed and discussed<br />>> separately. Pending
that,I'm going to mark this as "Waiting on<br />>> author" in the commitfest.<br />><br />> Attached is an
initialsplit which divides up reserving the role names<br />> from actually adding the initial set of default
roles.<br/><br /></span>I have begun looking at this patch, and here are some comments after screening it.<br /><br
/>Patchneeds a rebase, some catalog OIDs and there was a conflict in misc.c (see attached for the rebase. none of the
commentsmentioning issues are fixed by it).<br /><br />=# grant pg_replay to pg_backup  ;<br />GRANT ROLE<br />=# \du
pg_backup<br/>             List of roles<br /> Role name |  Attributes  |  Member of<br
/>-----------+--------------+-------------<br/> pg_backup | Cannot login | {pg_replay}<br />Perhaps we should restrict
grantinga default role to another default role?<br /><br />+  <para><br />+   Also note that changing the
permissionson objects in the system<br />+   catalogs, while possible, is unlikely to have the desired effect as<br />+
 the internal lookup functions use a cache and do not check the<br />+   permissions nor policies of tables in the
systemcatalog.  Further,<br />+   permission changes to objects in the system catalogs are not<br />+   preserved by
pg_dumpor across upgrades.<br />+  </para><br />This bit could be perhaps applied as a separate patch.<br /><br
/>+     <row><br />+       <entry>pg_backup</entry><br />+       <entry>Start and stop backups,
switchxlogs, and create restore points.</entry><br />+      </row><br />s/switch xlogs/switch to a new
transactionlog file/<br />It seems weird to not have a dedicated role for pg_switch_xlog.<br /><br />In func.sgml, the
descriptionsof pg_switch_xlog, pg_xlog_replay_pause and pg_xlog_replay_resume still mention that those functions are
restrictedonly to superusers. The documentation needs an update. It would be good to double-check if there are similar
mistakesfor the other functions.<br /><br />+       <entry>pg_montior</entry><br />Typo, pg_monitor.<br
/><br/>+       <entry>Pause and resume xlog replay on replicas.</entry><br />Those descriptions should be
completesentences or not? Both styles are mixed in user-manag.sgml.<br /><br />+      <row><br />+      
<entry>pg_replication</entry><br/>+       <entry>Create, destroy, and work with replication
slots.</entry><br/>+      </row><br />pg_replication_slots would be more adapted?<br /><br />+    
 <row><br/>+       <entry>pg_file_settings</entry><br />+       <entry>Allowed to view config
settingsfrom all config files</entry><br />+      </row><br />I would say "configuration files" instead. An
abbreviationis not adapted.<br /><br />+       <entry>pg_admin</entry><br />+       <entry><br />+  
    Granted pg_backup, pg_monitor, pg_reply, pg_replication,<br />+        pg_rotate_logfile, pg_signal_backend and
pg_file_settingsroles.<br />+       </entry><br />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.<br /><br />+    
 if (IsReservedName(stmt->role))<br />+               ereport(ERROR,<br />+                              
(errcode(ERRCODE_RESERVED_NAME),<br/>+                                errmsg("role name \"%s\" is reserved",<br />+    
                                  stmt->role),<br />+                                errdetail("Role names starting
with\"pg_\" are reserved.")));<br />Perhaps this could be a separate change? It seems that restricting roles with pg_
onthe cluster is not a bad restriction in any case. You may want to add regression tests to trigger those errors as
well.<br/><br />Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, pg_last_xlog_receive_location and
pg_last_xlog_replay_locationfall under a restriction category like pg_monitor? I recall of course that we discussed
thatsome 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_compressionis enabled.<br /><br />It would be nice to have regression tests as well to check that role restrictions
areapplied where they should.<span class=""></span></div></blockquote></div><br /></div><div
class="gmail_extra">Bonus:<br/>-static void<br />-check_permissions(void)<br />-{<br />-       if (!superuser()
&&!has_rolreplication(GetUserId()))<br />-               ereport(ERROR,<br />-                              
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),<br/>-                                (errmsg("must be superuser or
replicationrole to use replication slots"))));<br />-}<br /></div><div class="gmail_extra">I would have let
check_permissionsand directly done the checks on has_rolreplication and has_privs_of_role in it, the same code being
duplicatedthree times.<br />-- <br /><div class="gmail_signature">Michael<br /></div></div></div>