Re: CREATEROLE and role ownership hierarchies - Mailing list pgsql-hackers

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Being able to create and drop users is, in fact, effectively a superuser-only task today.  We could throw out the
entireidea of role ownership, in fact, as being entirely unnecessary when talking about that specific task. 
>
> Wow, that's totally contrary to how I see this patch.  The heart and soul of this patch is to fix the fact that
CREATEROLEis currently overpowered.  Everything else is gravy. 

I agree that CREATEROLE is overpowered and that the goal of this should
be to provide a way for roles to be created and dropped that doesn't
give the user who has that power everything that CREATEROLE currently
does.  The point I was making is that the concept of role ownership
isn't intrinsically linked to that and is, therefore, as you say, gravy.
That isn't to say that I'm entirely against the role ownership idea but
I'd want it to be focused on the goal of providing ways of creating and
dropping users and otherwise performing that kind of administration and
that doesn't require the specific change to make owners be members of
all roles they own and automatically have all privileges of those roles
all the time.

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >
> > Superuser is a problem specifically because it gives people access to do absolutely anything, both for security and
safetyconcerns. Disallowing a way to curtail that same risk when it comes to role ownership invites exactly those same
problems.
>
> Before the patch, users with CREATEROLE can do mischief.  After the patch, users with CREATEROLE can do mischief.
Thedifference is that the mischief that can be done after the patch is a proper subset of the mischief that can be done
beforethe patch.  (Counter-examples highly welcome.) 
>
> Specifically, I claim that before the patch, non-superuser "bob" with CREATEROLE can interfere with *any*
non-superuser. After the patch, non-superuser "bob" with CREATEROLE can interfere with *some* non-superusers;
specifically,with non-superusers he created himself, or which have had ownership transferred to him. 
>
> Restricting the scope of bob's mischief is a huge win, in my view.
>
> The argument about whether owners should always implicitly inherit privileges from roles they own is a bit orthogonal
tomy point about mischief-making.  Do we at least agree on the mischief-abatement aspect of this patch set?   

I don't know how many bites at this particular apple we're going to get,
but I doubt folks are going to be happy if we change our minds every
release.  Further, I suspect we'll be better off going too far in the
direction of 'mischief reduction' than not far enough.  If we restrict
things too far then we can provide ways to add those things back, but
it's harder to remove things we didn't take away.

This particular case is even an oddity on that spectrum though-
CREATEROLE users, today, don't have access to all the objects created by
roles which they create.  Yes, they can get such access if they go
through some additional hoops, but that could then be caught by someone
auditing the logs, a consideration that I don't think we appreciate
enough today.

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >
> > To push back on the original “tenant” argument, consider that one of the bigger issues in cloud computing today is
exactlythe problem that the cloud managers can potentially gain access to the sensitive data of their tenants and
that’snot generally viewed as a positive thing. 
>
> +1.  This is a real problem.  I have been viewing this problem as separate from the one which role ownership is
intendedto fix.  Do you have a suggestion about how to tackle the problems together with less work than tackling them
separately?

I don't know about less work or not, but in this particular case I was
asking for a few lines to be removed from the patch.  I can believe that
doing so would create some issues in terms of the use-cases that you
want to solve with this and if we agree on those being sensible cases to
address then we'd need to implement something to address those, though
it's also possibly not the case and maybe removing those few lines
doesn't impact anything beyond then allowing owners to not automatically
inherit the rights of the roles they own if they don't wish to.

Instead of talking about those cases concretely though, it seems like
we've shifted to abstractly talking about ownership and landlords.
Maybe some of that is helpful, but it seems to increasingly be an area
that's causing more division than helping to move forward towards a
mutually agreeable result.

> >  This change would make it so that every landlord can go and SELECT from the tables of their tenants without so
muchas a by-your-leave. 
>
> I would expect that is already true.  A user with CREATEROLE can do almost everything.  This patch closes some
CREATEROLErelated security problems, but not this one you mention. 

Yes, such a role *can* do almost anything, but they can't do this today:

=> create role r3;
CREATE ROLE
=*> set role r3;
ERROR:  permission denied to set role "r3"

Nor, should 'r3' log in and create tables, can the creating role SELECT
from r3's tables or otherwise have any effect on them.  That has
positives and negatives- we do want the 'owning' role to be able to do
certain things, like DROP the role, and once a role has created objects
that isn't able to be done unless those objects are reassigned or
dropped themselves.  How do we allow explicitly that then?  That's the
general direction I would think we'd be wanting to go in, rather than
just blanketly giving the owner all privileges of the roles they create
without any further say by anyone.

> >  The tenants likely don’t like that idea
>
> +1
>
> > , and almost as likely the landlords in many cases aren’t thrilled with it either.
>
> +1

Glad we agree on those.

> >  Should the landlords be able to DROP the tenant due to the tenant not paying their bill?  Of course, and that
shouldthen eliminate the tenant’s tables and other objects which take up resources, but that’s not the same thing as
sayingthat a landlord should be able to unlock a tenant’s old phone that they left behind (and yeah, maybe the analogy
fallsapart a bit there, but the point I’m trying to get at is that it’s not as simple as it’s being made out to be here
andwe should think about these things and not just implicitly grant all access to the owner because that’s an easy
thingto do- and is exactly what viewing owners as “mini superusers” does and leads to many of the same issues we
alreadyhave with superusers). 
>
> This is a pretty interesting argument.  I don't believe it will work to do as you say unconditionally, as there is
stilla need to have CREATEROLE users who have privileges on their created roles' objects, even if for no other purpose
thanto be able to REASSIGN OWNED BY those objects before dropping roles.  But maybe there is also a need to have
CREATEROLEusers who lack that privilege?  Would that be a privilege bit akin to (but not the same as!) the INHERIT
privilege? Should I redesign for something like that? 

We have INHERIT today already for roles and I'm not really thrilled with
the idea of coming up with some new and independent way to make that
work, or having something that works effectively the same way as role
membership does today but is called something else (which is what this
patch set is doing with ownership, hence my concern).

There's a couple of thoughts I have about addressing things around DROP
and REASSIGN- one is that those could perhaps just be made to work for
owners, but another is to allow owners to manage the role memberships of
roles they own, to include allowing the role to be granted to
themselves, and maybe that's even the default?  With today's CREATEROLE,
that looks like:

=> create role r3 admin sfrost;
CREATE ROLE
=*> set role r3;
SET

but we could possibly change that to be the default, or maybe we don't,
since that isn't how it works today.

Either way, we likely would need to allow owners to modify the role
membership of roles they own, but that doesn't strike me as a terribly
difficult thing to allow.  A more interesting question is about if a
role can manage their *own* membership- something we allow today but, as
I've brought up before, we should probably curtail to some extent.

Ultimately, that makes it possible for this:

SELECT * FROM secret_table;

to fail when secret_table was created by a tenant and the query is run
by a landlord.  A landlord would still be able to get access to
secret_table, but they'd have to do:

GRANT tenant TO landlord;
SELECT * FROM secret_table;

which may not seem like a lot to us, but it shows clear forethought and
very likely that GRANT would be an audited statement.  If tenant also
doesn't have 'inherit' set then a SET ROLE might also be required.
Perhaps additional requirements could be added to the GRANT/SET ROLE to
make those operations not be trivial to do (certainly we've been asked
in the past for a way for SET ROLE to require a password, and, indeed,
some other database systems support that; consider that one day a
landlord might have to reset the PW for the role, GRANT themselves into
the role, and then SET ROLE with the reset password...).

I'd also like to share that while we talk about 'landlords' and
'tenants' here, the real world is more complicated- I'm sure the various
cloud providers have employees who have different levels of access,
perhaps some of whom are able to reset passwords for users, while others
are able to create new accounts, and yet others are able to authorize
access to customer data, something which hopefully most of the
organization isn't able to do and requires some additional hoops.

> I like that the current patch restricts CREATEROLE users from granting privileges they themselves lack.  Would such a
newprivilege bit work the same way?  Imagine that you, "stephen", have CREATEROLE but not this new bit, and you create
me,"mark" as a tenant with CREATEROLE.  Can you give me the bit?  Or does the fact that you lack the bit mean you can't
giveit to me, either? 
>
> Other suggestions?

As I mentioned in the patch review, having a particular bit set doesn't
necessarily mean you should be able to pass it on- the existing object
GRANT system distinguishes those two and it seems like we should too.
In other words, I'm saying that we should be able to explicitly say just
what privileges a CREATEROLE user is able to grant to some other role
rather than basing it on what that user themselves has.  This might
already be possible with the proposed patch by creating a role with
CREATEROLE that then has the privileges we want to be allowed to be
passed on, and then GRANT'ing that role to the user who we want to allow
to create roles, though they would then have to SET ROLE to that role to
run the CREATE ROLE since role attributes aren't inherited by role
memberships.  That doesn't seem like a terrible approach to solving that
particular issue, but then perhaps others feel differently.

Thanks,

Stephen