Re: Additional role attributes && superuser review

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Jan 4, 2016 at 12:55 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > 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
> >
> > pgbackrest:
> >
> > To run pgbackrest as a non-superuser and not the 'postgres' system
> > user, grant the pg_backup role to the backrest user and ensure the
> > backrest system user has read access to the database files (eg: by
> > having the system user be a member of the 'postgres' group):
> >
> > GRANT pg_backup to backrest;
> >
> > For information regarding the pg_backup role, see:
> > http://www.postgresql.org/docs/current/static/roles/database-roles.html
>
> So I have two comments on this.
>
> 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.

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

> 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? If we do, then we'll
> be giving excess permissions to all the other backup tools that don't
> need that new right, and maybe surprising users who upgrade without
> realizing that some of their roles now have new rights. If we don't,
> then that tool won't be able to work without some additional
> twiddling. I just find it incredibly unlikely that every monitoring
> tool, or every backup tool, or every admin tool will require exactly
> the same set of permissions.

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.

This discussion started out with the idea of adding more role
attributes to address this need to break out superuser rights, as we
have done in the past, and then moved to discussing default roles
instead because it's a better solution for abstracting permissions as
roles are more easily grantable, delegation of role granting can be
done, and role membership works with inheritance.

The arguments you raise here would apply nearly the same to the
replication role attribute, but in practice, we don't seem to have any
question regarding how that's handled, nor have we gotten complaints
about it. I expect the same would be true with default roles and don't
see any particular reason to fear otherwise.

> > I can see similar bits of documentation being included in pgAdmin or
> > other tools.
>
> ...and pgAdmin is a particularly good example. The permissions that
> pgAdmin requires depend on what you want to do with it, and it does a
> lot of things, and it might do more or fewer things in the future.
> You can't even fairly assume that everyone wants to give the same
> permissions to pgAdmin, let alone that some competing tool will
> require the same set.

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.

> >> Being narrowly tied to a specific function, it's just a suboptimal spelling of
> >> GRANT. The gap in GRANT has distorted the design for these predefined roles.
> >> I do not anticipate a sound design discussion about specific predefined roles
> >> so long as the state of GRANT clouds the matter.
> >
> > I'm loathe to encourage any direct modification of catalog objects,
> > even if it's just ACLs. I've seen too many cases, as I imagine others
> > have, of users destroying their databases or running into unexpected
> > results when modifying the catalog. The catalog modifications supported
> > should be explicitly provided through other means rather than direct
> > commands against the catalog objects. I see the default roles approach
> > as being similar to having:
> >
> > ALTER DATABASE db WITH CONNECTION LIMIT 5;
> >
> > instead of suggesting users issue:
> >
> > UPDATE DATABASE SET datconnlimit = 5 WHERE datname = 'db';
>
> This doesn't make any sense to me. Nobody was proposing issuing an
> UPDATE against pg_database directly (and it would have to be
> pg_database, not just database as you wrote here). We're talking
> about whether the user is going to GRANT pg_rotate_logfile TO ... or
> GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO ... which is not the
> same sort of thing at all.

I was showing them broadly under "making changes to catalog objects."
Granting a default role isn't making a modification to a catalog object
at all and would be explicitly supported. GRANT'ing EXECUTE on a
catalog function works today, but isn't really supported. The pg_dump
change would be explicitly changing that to be a supported operation.

> >> 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.
>
> This doesn't make any sense to me, either. Calling a function from
> the SQL checks the permissions on that function. This works just fine
> today:

Yes, it does, I didn't mean to imply otherwise. The above discussion
was in regards to SysCache, as used internally by various commands.

> We can do that by default and it will Just Work. All we need to have
> a complete solution here is (1) remove the superuser check from the C
> code and (2) make pg_dump dump and restore the ACL properly.
>
> Syscaches really have nothing to do with this.

Adding pg_dump dump and restore support for catalog ACLs implies that
we're supporting ACLs on all catalog objects- including tables.
Otherwise, we're going to have to figure out exactly what objects we
allow setting ACLs on and which ones we don't.

REVOKE'ing SELECT access to pg_class isn't going to make DROP TABLE
stop working, even though it clearly is going to be accessing pg_class.
That's the point which I was attempting to make with regard to SysCache.

Another consideration is that the catalog changes pretty regularly and
we'd have to make sure that we are dumping out the correct set of
privileges and applying them correctly to the new catalog- and that's
only going to work reliably if the user is using the newer version of
pg_dump; we'll have no hope if they're using an old version. While
that's what we've always recommended, restoring an old dump into a newer
version, by-and-large, does work today and is certainly commonly done.

I'm not excited about the complaints we'd get if we implemented a change
which reliably broke that and I'm afraid this might and we wouldn't
realize it until we're on our second major release with that capability
and have already signed off on supporting catalog ACLs.

These complications and considerations are exactly the kind of
additional complexity that adding pg_dump dump/restore support for
catalog ACLs implies and that's why, while it sounds pretty interesting,
I don't believe it's necessairly a good idea. The default roles
approach is quite simple and similar to, while being better than, the
existing role attributes approach, in my view.

Thanks!

Stephen