Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfr...@snowman.net> wrote: > > Being able to create and drop users is, in fact, effectively a > > superuser-only task today. We could throw out the entire idea 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 CREATEROLE is 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.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfr...@snowman.net> wrote: > > > > Superuser is a problem specifically because it gives people access to do > > absolutely anything, both for security and safety concerns. 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. The difference is that the mischief > that can be done after the patch is a proper subset of the mischief that can > be done before the 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 to my 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.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost <sfr...@snowman.net> wrote: > > > > To push back on the original “tenant” argument, consider that one of the > > bigger issues in cloud computing today is exactly the problem that the > > cloud managers can potentially gain access to the sensitive data of their > > tenants and that’s not 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 intended to 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 much as a by-your-leave. > > I would expect that is already true. A user with CREATEROLE can do almost > everything. This patch closes some CREATEROLE related 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 should then eliminate the tenant’s > > tables and other objects which take up resources, but that’s not the same > > thing as saying that a landlord should be able to unlock a tenant’s old > > phone that they left behind (and yeah, maybe the analogy falls apart 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 and we should think about these things and > > not just implicitly grant all access to the owner because that’s an easy > > thing to do- and is exactly what viewing owners as “mini superusers” does > > and leads to many of the same issues we already have with superusers). > > This is a pretty interesting argument. I don't believe it will work to do as > you say unconditionally, as there is still a need to have CREATEROLE users > who have privileges on their created roles' objects, even if for no other > purpose than to be able to REASSIGN OWNED BY those objects before dropping > roles. But maybe there is also a need to have CREATEROLE users 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 new privilege 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 give it 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
signature.asc
Description: PGP signature