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

On Wed, Jan 6, 2016 at 11:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
> 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.

Thanks.

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

I would be in favor of that.

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

I agree that predefined roles are better than additional role
attributes.  I don't agree that predefined roles are better than GRANT
EXECUTE ON FUNCTION pg_catalog.blah().  I think full support for GRANT
EXECUTE ON FUNCTION pg_catalog.blah() is undeniably useful and will be
a very clear improvement over what we have now.  I think predefined
roles are a reasonable thing for somebody to want, but I don't think
it's nearly as clear-cut, and I'm very much unconvinced that we know
that the ones you're proposing are the right ones.

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

Mmmph.  I think we got lucky there as much as anything.  replication
already allows sufficient privilege to extract all data in the
database, so allowing it to also request a logical change stream isn't
really a privilege escalation.  That's not going to be true for what
you are proposing here.

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

I wouldn't worry about that a bit.  We recommend that users always
pg_dump using the newest version of pg_dump, even if the dumped server
is older.  That advice will be entirely satisfactory in this case
also.

> 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 think you need to provide detailed analysis SO THAT we can agree on
the concept.

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

That's a good principle, but not a sufficient guide.

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

I am pretty much entirely in agreement with Noah's analysis here and I
think he put it better than I ever could have done.  The rate of
change of check_postgres is utterly irrelevant.  We are not going to
have a role called pg_check_postgres built into the database.  We
would have, at most, a role called pg_monitor, which would be expected
to be generic across all monitoring tools.  But all monitoring tools
will not have the same permissions needs.  And they will certainly not
all develop equally quickly.  EnterpriseDB has an entire team of
people working on our monitoring tool (PEM).  Maybe it's not growing
any new features that require additional permissions at this point,
but that can change as fast as somebody can rewrite the roadmap.
Somebody could offer Greg a million dollars tomorrow to add a whole
bunch of new features to check_postgres.pl, and he might agree to take
on the work.

The point is that with the GRANT EXECUTE ON FUNCTION proposal, authors
of monitoring tools enjoy various really noteworthy advantages.  They
can have monitoring roles which have *exactly* the privileges that
their tool needs, not whatever set of permissions (larger or smaller)
the core project has decide the pg_monitor role should have.  They can
have optional features requiring extra permissions and those extra
permissions can be granted in precisely those shops where those extra
features are in use.  They can deploy a new versions of their
monitoring tool that requires an extra privilege on an existing
PostgreSQL release without requiring any core modifications, which
shaves years of time off the deployment schedule and avoids
contentious arguments with the lovable folks who populate this mailing
list.  That sounds *terrific* to me compared to the alternative you
are proposing.

> I don't think that waiting until 9.7 or 10.0 for that
> would be any problem at all for check_postgres,

I think it's nuts to design a solution on the theory that people won't
mind waiting 2 or 3 years to be able to enable a new feature in their
product.  Maybe Greg won't mind personally, but if users don't mind
not having the feature for 2 or 3 years, it wasn't a very important
feature in the first place.  No software product I've ever worked on
personally could afford to wait an extra 2 or 3 years to deploy a
feature that was truly needed, and most managers I've worked for would
pop a gasket.

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

Sure, that's clearly right, but it doesn't making the pg_monitor
approach better than individual function grants.  If you provide a
better alternative there, people can start granting access to that
instead, and in the meantime they can decide whether or not they want
to take the risk of granting access to pg_ls_dir().

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

You can grant extra rights, but you can't revoke some of them away AFAIK.

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