Re: Additional role attributes && superuser review

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.

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

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

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

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

rhaas=# revoke execute on function pg_rotate_logfile() from public;
REVOKE
rhaas=# set role bob;
SET
rhaas=> select pg_rotate_logfile();
ERROR: permission denied for function pg_rotate_logfile

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company