Re: Additional role attributes && superuser review

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> 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.

As David notes, they're actually quite related. Note that in our
documentation pg_switch_xlog() is listed in the "Backup Control
Functions" table.

I can think of a use-case for a user who can call pg_switch_xlog, but
not pg_start_backup()/pg_stop_backup(), but I have to admit that it
seems rather limited and I'm on the fence about it being a worthwhile
distinction.

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.

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

The file was generated using 'git format-patch', but sent to one file
instead of multiple. 'git am' understands that format and will add the
independent commits to the current branch. Note that the git
documentation (see 'man git-format-patch' and 'man git-apply', at least
on Debian-based systems) specifically recommends using 'git am' to
create commits from patches generated by 'git format-patch'.

The workflow you're describing would require saving off each patch,
doing a 'patch' or 'git apply' run for each, then committing the changes
with your own commit message and only then would you have the entire
series. That doesn't seem like a better approach.

> >> 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 made those changes in this patch set. Note that these functions
can now only be called by roles which are members of pg_monitor,
pg_backup, or pg_switch_xlog (or superuser, of course).

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

Perhaps, but...

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

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

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

Updated patchset attached.

Thanks!

Stephen