Re: Additional role attributes && superuser review

Robert, Noah,

I just wanted to start off by saying thank you for taking the time read
and comment with your thoughts on this concept. I was a bit frustrated
about it feeling rather late, but appreciate the comments which have
been made as they've certainly been constructive.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Jan 4, 2016 at 4:56 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> First, it's not really going to matter to users very much whether the
> >> command to enable one of these features is a single GRANT command or a
> >> short sequence of GRANT commands executed one after another. So even
> >> if we don't have roles that bundle together multiple permissions, you
> >> will still be able to do this; you just might need more than one
> >> command. Probably, I'm guessing, not very many commands, but more
> >> than one.
> >
> > I disagree. I find that it does make a difference to users to be using
> > a well documented and clear approach over one which involves fiddling
> > with the individual pieces to get everything to work, and if a new
> > function comes along that is useful for backup users, that would have to
> > also be granted, even if it would clearly be useful to a backup role.
>
> How is that a fair way to characterize the discussion here? Just
> because you have to execute several SQL commands instead of one
> doesn't turn a "well-documented and clear approach" into something
> that involves "fiddling with individual pieces". Cutting and pasting
> a sequence of 3 or 4 SQL commands into a psql window is not a lot
> harder than copying and pasting a single one, and does not turn a good
> approach into a shambles.

I was looking at it from a perspective of what we have currently vs.
what the future state with default roles would be. That's not an
entirely fair characterization. If we supported ACLs on catalog objects
via pg_dump, then we could add documentation along the lines of "backups
generally need access to functions X, Y, Z, here's an example of how to
create such a role: blah, blah."

Of course, that documentation would likely have to be repeated in the
various backup tools, though it's possible they could tailor those, if
there was something different about their particular tool.

> >> Second, I think it's really unlikely that you're going to maintain
> >> perfect alignment between your predefined roles and the permissions
> >> that third-party tools need over the course of multiple releases. I
> >> think the chances of that working out are just about zero. Sure, you
> >> can make the initial set of permissions granted to pg_backup match
> >> exactly what pgbackrest needs, but it's a good bet that one of the six
> >> or eight widely-used backup tools uses something that's not included
> >> in that set.
> >
> > There may be something I've missed with the proposed pg_backup role, but
> > I don't believe you're correct that there isn't a set of privileges
> > which all of those backup tools need and which could be provided through
> > the pg_backup role.
>
> Well, there's certainly some set of privileges that will make them all
> work. But it might include more than some of them want. If you done
> any analysis of this, I have not seen it posted to this thread.

I can certainly work up a formal analysis and submit it for
consideration.

> >> And even if not, it doesn't require very much
> >> imagination to suppose that some tool, maybe pgbackrest or maybe
> >> something else, that comes along over the next few releases will
> >> require some extra permission. When that happens, will we include add
> >> that permission to the pg_backup role, or not?
> >
> > As I pointed out previously, we've already been doing this with the
> > replication role attribute and I don't recall any complaining about it.
>
> 1. This doesn't really seem like the same thing. You're introducing
> something quite new here: these are not role attributes that apply
> only to the role itself, but inheritable role attributes.

This approach started out by adding role attributes to handle these
kinds of rights, but in discussion with Tom and Magnus, iirc (no, I
don't have the specific links or threads, though I have asked Magnus to
take a look at this thread, as he has time), the idea of default roles
seemed better specifically because they would then be inheritable and
granting access could also be delegated.

> 2. I believe that the discussion about what the replication role
> should and should not allow involved considerably more people than
> have discussed any of the specific roles you propose to add here.

I didn't intend to dispute that, but...

> 3. It was clear from the outset that the replication role's basic
> purpose was to be sufficient privilege for a streaming standby and no
> more. The remit of these roles is a lot less clear, at least to me.

I've certainly intended the intention of these roles to be clear and
documented. The point I was trying to address above is that we
certainly appear fine to add additional privileges as new capabilities
are added to existing role attributes (the entire logical replication
system was added after the replication role attribute).

> > I wasn't suggesting that we give everyone the same privileges to some
> > default 'pgAdmin' role but rather that providing an abstraction of the
> > set of privileges possible against the catalog objects, into sets that
> > make sense together, is a useful simplification for users and that it'd
> > be a better approach than asking users to figure out what sets make
> > sense on their own.
>
> I have no objection to that *in theory*. What's not clear to me is
> that the way that you have broken it up actually meets the bona fide
> needs of actual tools in a useful way.

That's certainly been my goal, but I've not been as clear as I should
have been regarding the analysis and background work which I have done.

> > Adding pg_dump dump and restore support for catalog ACLs implies that
> > we're supporting ACLs on all catalog objects- including tables.
>
> Not to me it doesn't. I think we could support it just for functions,
> and have it continue to be as weird as it is currently for other types
> of objects until somebody gets around to straightening that out. If
> we want to support it for more object types, great, but I don't think
> that's a hard requirement.

Alright, that would certainly simplify things if we're talking about
only functions. The only concern which I have there is if there are any
non-function cases that we'll end up coming across, and I'm still a bit
nervous about the "old pg_dump / new database" restore concern, but
perhaps that's an acceptable issue.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Jan 4, 2016 at 5:22 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> So, is this another case where the support is all in off-list fora and
> >> thus invisible, or can you point to specific on-list discussions where
> >> it was supported, and to the opinions offered in support? I don't
> >> really remember many opinions that were any more positive than "I
> >> wouldn't be strongly opposed to this" or "If we're going to do this
> >> then we ought to do it in X way". I'm happy to be corrected if I'm
> >> misrepresenting the record, but I'd characterize the overall reaction
> >> to this proposal as tepid: nobody hated it, but nobody really loved it
> >> either, and a bunch of mild concerns were offered.
> >
> > I agree that this has largely been the on-list reaction. To be fair,
> > it's been largely the off-list reaction also, which I've expressly
> > tried to seek out, as mentioned above. I'm not asking anyone to love
> > it, I'm not entirely convinced it's lovable myself, but I do feel it's
> > useful and worth making an effort for.
>
> I think the question of whether the specific proposals on the table
> are in fact useful is one that deserves more study. I am not
> convinced of that. I believe something like this could be useful, but
> I don't see a lot of evidence that the particular roles you're arguing
> for actually are.

Based on Noah's response (which I respond to below), we seem to still
be debating the whole concept of default roles. I'm happy to provide
detailed analysis if we're able to agree on the concept.

> > I'd love to have folks from other companies involved in these
> > discussions. I'll even reach out explicitly to seek their comment, as
> > I've done with other hackers at conferences, and try to get them to
> > voice their opinions here.
>
> Great, thanks.

I've reached out to Magnus and Greg S-M so far. I've discussed this
quite a bit with David Steele already. I'll reach out to Marco
regarding barman.

> > I'm not sure about the references you use above to Slony or pgBouncer or
> > pgPool as those aren't backup tools, to my mind.. I would expect barman
> > and other backup tools to also use pg_start/stop_backup and
> > pg_switch_xlog. I'm not sure that there's a way to cater to one backup
> > role when it comes to how filesystem-level backups are handled in PG,
> > but perhaps I've missed something there that barman uses and which isn't
> > included currently.
>
> Oh, sure: they are not backup tools specifically. But anything that
> might need elevated privileges deserves consideration here: what sort
> of subdivision of the superuser role would make that need go away?

The general approach which I've been using for the default roles is that
they should grant rights which aren't able to be used to trivially make
oneself a superuser.

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Mon, Jan 04, 2016 at 12:55:16PM -0500, Stephen Frost wrote:
> > * Noah Misch (noah(at)leadboat(dot)com) wrote:
> > > On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:
>
> > I'm approaching this largely from a 3rd-party application perspective.
> > There are two examples off-hand which I'm considering:
> >
> > check_postgres
> > pgbackrest
> >
> > I'd like to be able to include, in both of those, a simple set of
> > instructions for granting the necessary rights to the user who is
> > running those processes. A set of rights which an administrator can go
> > look up and easily read and understand the result of those grants. For
> > example:
> >
> > check_postgres:
> >
> > Most check_postgres sub-commands can be run without superuser
> > privileges by granting the pg_monitor role to the monitoring user:
> >
> > GRANT pg_monitor TO monitor;
> >
> > For information regarding the pg_monitor role, see:
> > http://www.postgresql.org/docs/current/static/roles/database-roles.html
>
> Stop. Even if PostgreSQL introduces pg_monitor, check_postgres will do itself
> a favor by not using it. The moment the two projects' sense of monitoring
> privileges falls out of lockstep, benefits from using pg_monitor go negative.
> check_postgres should instead furnish a script that creates a role holding
> required privileges and/or SECURITY DEFINER helpers. If check_postgres starts
> using an object, say pgstattuple, it will wish to use it in all versions.
> Since PostgreSQL will change pg_monitor privileges in major releases only,
> check_postgres would wait 6-18 months to use a privilege in just one of five
> supported versions. If PostgreSQL hackers ever disagree with check_postgres
> hackers about whether a privilege belongs in the pg_monitor role, then
> check_postgres will wish it had never used pg_monitor. For a sample
> controversy, some monitoring tool may well call pg_read_file, but that's
> arguably too much power to be giving _every_ monitoring tool.

Greg and I have chatted previously, and recently again yesterday, about
the idea of providing SECURITY DEFINER functions and neither of us care
for that approach. A role which is granted the necessary rights might
be alright, though I would still contend that a default role is better
and I don't particularly agree with the slow turn-around time-
check_postgres is quite mature and does not change very much any more.

> By the way, the pg_monitor role you were ready to commit does not cover
> today's check_postgres needs. Among restricted objects, check_postgres uses
> at least pg_ls_dir, pg_stats, pg_settings, and pg_stat_activity. Having said
> that, it remains premature to design predefined roles. It would be a waste to
> mobilize such a design effort with GRANT's limitation clouding the issue.

I reviewed check_postgres and wished to avoid functions that were overly
broad for what check_postgres actually needed and those which would
allow the monitoring role to be able to become superuser easily. That
is where the set of permissions actually granted to pg_monitor came
from. The checks of things like "number of WAL segments" are cases
where I'd rather we provide an explicit function that only provided that
information and then allow pg_monitor to use it rather than simply grant
pg_ls_dir access. I don't think that waiting until 9.7 or 10.0 for that
would be any problem at all for check_postgres, which is why I had
wished to get the basic default roles concept completed before moving on
to add such additional functions.

To put it another way, I'd rather *not* tell check_postgres users "sure,
just grant access to pg_ls_dir to the monitoring user" as that really
grants more access than the check_postgres check requires.

> > and, to reiterate what I said above, I'd rather have one abstraction for
> > these kinds of permissions instead of a mish-mash of instructions. The
> > difference I can imagine being between:
> >
> > For backups and monitoring, you can use default roles:
> >
> > GRANT pg_backup,pg_monitor to new_admin;
> >
> > but for other regular privileges such as rotating logfiles, or sending
> > signals to other processes, you have to explicitly GRANT permissions:
> >
> > GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO new_admin;
> > GRANT EXECUTE ON FUNCTION pg_signal_backend() TO new_admin;
>
> I don't mind having those two ways to transfer privilege. If I have to settle
> for one or the other, I pick the latter.

Of course, if we did provide both, then any difference might be able to
be addressed as a delta against what is provided by the default role.

> > > > Further, I'm not convinced that adding support for dumping ACLs or, in
> > > > general, encouraging users to define their own ACLs on catalog objects
> > > > is a good idea. We certainly have no mechanism in place today for those
> > > > ACLs to be respected by SysCache and encouraging their use when we won't
> > > > actually respect them is likely to be confusing.
> > >
> > > What's this problem with syscache? It sounds important.
> >
> > CREATE TABLE and DROP TABLE aren't going to care one bit if you have
> > access to pg_class or not. The same goes for basically everything else.
> >
> > If we really want to support ACLs on the catalog, we'd have to either
> > caveat that none of the internal lookups will respect them or revamp
> > SysCache and any other direct catalog access to do permission checks
> > first, which I don't think we really want to do.
>
> Oh, that. Having internal lookups ignore the ACLs is more good than bad, and
> users have little cause to expect something different. You don't need INSERT
> on pg_attribute to add a column, so why expect lack of SELECT on pg_attribute
> to prevent dropping one? I might document it like so:
>
> While GRANT can modify the privileges of a system catalog table, that
> affects only queries that address the catalog as an SQL table. Internal,
> system access to the same underlying data will proceed normally. For
> example, "REVOKE SELECT ON pg_proc FROM PUBLIC" does not preclude calling
> functions or even preclude passing them to pg_get_functiondef. It does
> block queries that name pg_proc in a FROM clause.

That's a particularly good example, as it is frequently requested that
prosrc be hidden. Though what is really desired is for users to be able
to see all functions they can call, but only see prosrc for the
functions they own. That kind of policy is where I believe we should be
taking CREATE POLICY, in the future. That's discussion for another
thread though, by and large.

> > This entire discussion of privileges-on-catalog-objects should really
> > also consider the ongoing discussion about providing policies for the
> > catalog via RLS. If we start pg_dump'ing the ACLs of catalog objects
> > then we'd, presumably, also want to pg_dump out any policies defined
> > against catalog objects.
>
> I would have no qualms supporting ACL changes while not supporting added
> policies, indexes, triggers, rules, inheritance children, extra columns, etc.

Alright.

> Both Oracle and SQL Server invite system object GRANTs, but they don't invite
> system object mutations generally.

Interesting. I hadn't assessed how system object GRANTs in those
systems were handled, so that's good to know.

Thanks!

Stephen