Re: fixing CREATEROLE
On Sun, 15 Jan 2023 at 06:02, Robert Haas wrote: > > On Sat, Jan 14, 2023 at 2:26 AM vignesh C wrote: > > I'm not sure if any work is left here, if there is nothing more to do, > > can we close this? > > There's a discussion on another thread about some follow-up > documentation adjustments, but feel free to close the CF entry for > this patch. Thanks, I have marked the CF entry as committed. Regards, Vignesh
Re: fixing CREATEROLE
On Sat, Jan 14, 2023 at 2:26 AM vignesh C wrote: > I'm not sure if any work is left here, if there is nothing more to do, > can we close this? There's a discussion on another thread about some follow-up documentation adjustments, but feel free to close the CF entry for this patch. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Tue, 10 Jan 2023 at 23:16, Robert Haas wrote: > > On Thu, Jan 5, 2023 at 2:53 PM Robert Haas wrote: > > On Tue, Jan 3, 2023 at 3:11 PM Robert Haas wrote: > > > Committed and back-patched 0001 with fixes for the issues that you > > > pointed out. > > > > > > Here's a trivial rebase of the rest of the patch set. > > > > I committed 0001 and 0002 after improving the commit messages a bit. > > Here's the remaining two patches back. I've done a bit more polishing > > of these as well, specifically in terms of fleshing out the regression > > tests. I'd like to move forward with these soon, if nobody's too > > vehemently opposed to that. > > Done now. I'm not sure if any work is left here, if there is nothing more to do, can we close this? Regards, Vignesh
Re: fixing CREATEROLE
On Thu, Jan 5, 2023 at 2:53 PM Robert Haas wrote: > On Tue, Jan 3, 2023 at 3:11 PM Robert Haas wrote: > > Committed and back-patched 0001 with fixes for the issues that you pointed > > out. > > > > Here's a trivial rebase of the rest of the patch set. > > I committed 0001 and 0002 after improving the commit messages a bit. > Here's the remaining two patches back. I've done a bit more polishing > of these as well, specifically in terms of fleshing out the regression > tests. I'd like to move forward with these soon, if nobody's too > vehemently opposed to that. Done now. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Tue, Jan 3, 2023 at 3:11 PM Robert Haas wrote: > Committed and back-patched 0001 with fixes for the issues that you pointed > out. > > Here's a trivial rebase of the rest of the patch set. I committed 0001 and 0002 after improving the commit messages a bit. Here's the remaining two patches back. I've done a bit more polishing of these as well, specifically in terms of fleshing out the regression tests. I'd like to move forward with these soon, if nobody's too vehemently opposed to that. Previous feedback, especially from Tom but also others, was that the role-level properties the final patch was creating were not good. Now it doesn't create any new role-level properties, and in fact it has nothing to say about role-level properties in any way. That might not be the right thing. Right now, if you have CREATEROLE, you can create new roles with any combination of attributes you like, except that you cannot set the SUPERUSER, REPLICATION, or BYPASSRLS properties. While I think it makes sense that a CREATEROLE user can't hand out SUPERUSER or REPLICATION privileges, it is really not obvious to me why a CREATEROLE user shouldn't be permitted to hand out BYPASSRLS, at least if they have it themselves, and right now there's no way to allow that. On the other hand, I think that some superusers might want to restrict a CREATEROLE user's ability to hand out CREATEROLE or CREATEDB to the users they create, and right now there's no way to prohibit that. I don't have a great idea about what a system for handling this problem ought to look like. In a vacuum, I think it would be reasonable to change CREATEROLE to only allow CREATEDB, BYPASSRLS, and similar to be given to new users if the creating user possesses them, but that approach does not work for CREATEROLE, because if you didn't have that, you couldn't create any new users at all. It's also pretty weird for, say, CONNECTION LIMIT. I doubt that there's any connection between the CONNECTION LIMIT of the CREATEROLE user and the values that they ought to be able to set for users that they create. Probably you just want to allow setting CONNECTION LIMIT for downstream users, or not. Or maybe it's not even worth worrying about -- I think there might be a decent argument that limiting the ability to set CONNECTION LIMIT just isn't interesting. If someone else has a good idea what we ought to do about this part of the problem, I'd be interested to hear it. Absent such a good idea -- or if that good idea is more work to implement that can be done in the near term -- I think it would be OK to ship as much as I've done here and revisit the topic at some later point when we've had a chance to absorb user feedback. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com v4-0001-Restrict-the-privileges-of-CREATEROLE-users.patch Description: Binary data v4-0002-Add-new-GUC-createrole_self_grant.patch Description: Binary data
Re: fixing CREATEROLE
On Fri, Dec 23, 2022 at 4:55 PM Robert Haas wrote: > On Thu, Dec 22, 2022 at 9:14 AM Alvaro Herrera > wrote: > > The contents looks good to me other than that problem, and I agree to > > backpatch it. > > Cool. Thanks for the review. > > > Why did you choose to use two dots for ellipses in some command > > s rather than three? I know I've made that choice too on > > occassion, but there aren't many such cases and maybe we should put a > > stop to it (or a period) before it spreads too much. > > Honestly, I wasn't aware that we had some other convention for it. Committed and back-patched 0001 with fixes for the issues that you pointed out. Here's a trivial rebase of the rest of the patch set. -- Robert Haas EDB: http://www.enterprisedb.com v3-0001-Refactor-permissions-checking-for-role-grants.patch Description: Binary data v3-0004-Add-new-GUC-createrole_self_grant.patch Description: Binary data v3-0002-Pass-down-current-user-ID-to-AddRoleMems-and-DelR.patch Description: Binary data v3-0003-Restrict-the-privileges-of-CREATEROLE-users.patch Description: Binary data
Re: fixing CREATEROLE
On Thu, Dec 22, 2022 at 9:14 AM Alvaro Herrera wrote: > The contents looks good to me other than that problem, and I agree to > backpatch it. Cool. Thanks for the review. > Why did you choose to use two dots for ellipses in some command > s rather than three? I know I've made that choice too on > occassion, but there aren't many such cases and maybe we should put a > stop to it (or a period) before it spreads too much. Honestly, I wasn't aware that we had some other convention for it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
Reading 0001: +However, CREATEROLE does not convey the ability to +create SUPERUSER roles, nor does it convey any +power over SUPERUSER roles that already exist. +Furthermore, CREATEROLE does not convey the power +to create REPLICATION users, nor the ability to +grant or revoke the REPLICATION privilege, nor the +ability to the role properties of such users. "... nor the ability to the role properties ..." I think a verb is missing here. The contents looks good to me other than that problem, and I agree to backpatch it. Why did you choose to use two dots for ellipses in some command s rather than three? I know I've made that choice too on occassion, but there aren't many such cases and maybe we should put a stop to it (or a period) before it spreads too much. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 8:33 PM Robert Haas wrote: > Hmm, that's an interesting alternative to what I actually implemented. > Some people might like it better, because it puts the behavior fully > under the control of the CREATEROLE user, which a number of you seem > to favor. Here's an updated patch set. 0001 adds more precise and extensive documentation for the current (broken) state of affairs. I propose to back-patch this to all supported branches. It also removes a suggesting that you should use a CREATEDB & CREATEROLE role instead of a superuser, because that is pretty pointless as things stand, and is too simplistic for the new system that I'm proposing to put in place, too. 0002 and 0003 are refactoring, unchanged from v1. 0004 is the core fix to CREATEROLE. It has been updated from the previous version with documentation and some bug fixes. 0005 adopts David's suggestion: instead of giving the superuser a way to control the options on the implicit grant, give CREATEROLE users a way to grant newly-created roles to themselves automatically. I made this a GUC, which means that the person setting up the system could configure a default in postgresql.conf, but a user who doesn't prefer that default can also override it using ALTER ROLE .. SET or ~/.psqlrc or whatever. This is simpler than what I had before, doesn't involve a catalog change, makes it clear that the behavior is not security-critical, and puts the decision fully in the hands of the CREATEROLE user rather than being partly controlled by that user and partly by the superuser. Hopefully that's an improvement. Comments? -- Robert Haas EDB: http://www.enterprisedb.com v2-0001-Improve-documentation-of-the-CREATEROLE-attibute.patch Description: Binary data v2-0002-Refactor-permissions-checking-for-role-grants.patch Description: Binary data v2-0003-Pass-down-current-user-ID-to-AddRoleMems-and-DelR.patch Description: Binary data v2-0004-Restrict-the-privileges-of-CREATEROLE-users.patch Description: Binary data v2-0005-Add-new-GUC-createrole_self_grant.patch Description: Binary data
Re: fixing CREATEROLE
On Tue, Nov 29, 2022 at 2:32 AM wrote: > I propose a slightly different syntax instead: > > ALTER DEFAULT PRIVILEGES GRANT CREATED ROLE TO role_specification WITH ...; > > This, together with the proposal above regarding the grantor, should be > consistent. I think that is more powerful than what I proposed but less fit for purpose. If alice is a CREATEROLE user and issues CREATE ROLE bob, my proposal allows alice to automatically obtain access to bob's privileges. Your proposal would allow that, but it would also allow alice to automatically confer bob's privileges on some third user, say charlie. Maybe that's useful to somebody, I don't know. But one significant disadvantage of this is that every CREATEROLE user must have their own configuration. If we have CREATE ROLE users alice, dave, and ellen, then allice needs to execute ALTER DEFAULT PRIVILEGES GRANT CREATED ROLE TO alice WITH ...; dave needs to do the same thing with dave instead of alice; and ellen needs to do the same thing with ellen instead of alice. There's no way to apply a system-wide configuration that applies nicely to all CREATEROLE users. A GUC would of course allow that, because it could be set in postgresql.conf and then overridden for particular databases, users, or sessions. David claims that "these aren't privileges, they are memberships." I don't entirely agree with that, because I think that we're basically using memberships as a pseudonym for privileges where roles are concerned. However, it is true that there's no precedent for referring to role grants using the keyword PRIVILEGES at the SQL level, and the fact that the underlying works in somewhat similar ways doesn't necessarily mean that it's OK to conflate the two concepts at the SQL level. So I'm still not very sold on this idea. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Tue, Nov 29, 2022 at 12:32 AM wrote: > > Is there any other argument to be made against ADP? > These aren't privileges, they are memberships. The pg_default_acl catalog is also per-data while these settings should be present in a catalog which, like pg_authid, is catalog-wide. This latter point, for me, disqualifies the command itself from being used for this purpose. If we'd like to create ALTER DEFAULT MEMBERSHIP (and a corresponding cluster-wide catalog) then maybe the rest of the design would work within that. > > Note, that ADP allows much more than just creating a grant for the > CREATEROLE user, which would be the case if the default GRANT was made > TO the_create_role_user. But it could be made towards *other* users as > well, so you could do something like this: > > CREATE ROLE alice CREATEROLE; > CREATE ROLE bob; > > ALTER DEFAULT PRIVILEGES FOR alice GRANT CREATED ROLE TO bob WITH SET > TRUE, INHERIT FALSE; > What does that accomplish? bob cannot create roles to actually exercise his privilege. > This is much more flexible than role attributes or GUCs. > > The main advantage of GUC over a role attribute is that you can institute layers of defaults according to a given cluster's specific needs. ALTER ROLE SET (pg_db_role_setting - also cluster-wide) also comes into play; maybe alice wants auto-inherit while in db-a but not db-b (this would/will be more convincing if we end up having per-database roles). If we accept that some external configuration knowledge is going to influence the result of executing this command (Tom?) then it seems that all the features a GUC provides are desirable in determining how the final execution context is configured. Which makes sense as this kind of thing is precisely what the GUC subsystem was designed to handle - session context environments related to the user and database presently connected. David J.
Re: fixing CREATEROLE
Robert Haas: 1) Automatically install an additional membership grant, with the CREATEROLE user as the grantor, specifying INHERIT OR SET as TRUE (I personally favor attaching these to ALTER ROLE, modifiable only by oneself) Hmm, that's an interesting alternative to what I actually implemented. Some people might like it better, because it puts the behavior fully under the control of the CREATEROLE user, which a number of you seem to favor. +1 I suppose if we did it that way, it could even be a GUC, like create_role_automatic_grant_options. I don't think using GUCs for that is any better. ALTER DEFAULT PRIVILEGES is the correct way to do it. The only argument against it was, so far, that it's easy to confuse with default options for newly created role grants, due to the abbreviated grant syntax. I propose a slightly different syntax instead: ALTER DEFAULT PRIVILEGES GRANT CREATED ROLE TO role_specification WITH ...; This, together with the proposal above regarding the grantor, should be consistent. Is there any other argument to be made against ADP? Note, that ADP allows much more than just creating a grant for the CREATEROLE user, which would be the case if the default GRANT was made TO the_create_role_user. But it could be made towards *other* users as well, so you could do something like this: CREATE ROLE alice CREATEROLE; CREATE ROLE bob; ALTER DEFAULT PRIVILEGES FOR alice GRANT CREATED ROLE TO bob WITH SET TRUE, INHERIT FALSE; This is much more flexible than role attributes or GUCs. Best, Wolfgang
Re: fixing CREATEROLE
Robert Haas: And the result is that I've got like five people, some of whom particulated in those discussions, showing up to say "hey, we don't need the ability to set defaults." Well, if that's the case, then why did we have hundreds and hundreds of emails within the last 12 months arguing about which way it should work? For me: "Needed" as in "required". I don't think we *require* defaults to make this useful, just as David said as well. Personally, I don't need defaults either, at least I didn't have a use-case for it, yet. I'm not objecting to introduce defaults, but I do object to *how* they were introduced in your patch set, so far. It just wasn't consistent with the other stuff that already exists. Best, Wolfgang
Re: fixing CREATEROLE
Mark Dilger: Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That should allow role administration, without actually granting membership in that role, yet, right? Can you clarify what you mean here? Are you inventing a new syntax? +GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE; +ERROR: syntax error at or near "SET" +LINE 1: GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE... This is valid syntax on latest master. Best, Wolfgang
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 6:32 PM David G. Johnston wrote: > Desirable follow-on patches include: > > 1) Automatically install an additional membership grant, with the CREATEROLE > user as the grantor, specifying INHERIT OR SET as TRUE (I personally favor > attaching these to ALTER ROLE, modifiable only by oneself) Hmm, that's an interesting alternative to what I actually implemented. Some people might like it better, because it puts the behavior fully under the control of the CREATEROLE user, which a number of you seem to favor. I suppose if we did it that way, it could even be a GUC, like create_role_automatic_grant_options. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 4:55 PM Robert Haas wrote: > But so far nobody has actually reviewed anything, ... Actually this isn't true. Mark did review. Thanks, Mark. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 2:55 PM Robert Haas wrote: > On Mon, Nov 28, 2022 at 4:19 PM David G. Johnston > wrote: > > That's fine, but are you saying this patch is incapable (or simply > undesirable) of having the parts about handling defaults separated out from > the parts that define how the system works with a given set of permissions; > and the one implementation detail of having the bootstrap superuser > automatically grant admin to any roles a createuser role creates? If you > and others feel strongly about defaults I'm sure that the suggested other > thread focused on that will get attention and be committed in a timely > manner. But the system will work, and not be broken, if that got stalled, > and it could be added in later. > > The topics are so closely intertwined that I don't believe that trying > to have separate discussions will be useful or productive. There's no > hope of anybody understanding 0004 or having an educated opinion about > it without first understanding the earlier patches, and there's no > requirement that someone has to review 0004, or like it, just because > they review or like 0001-0003. > > But so far nobody has actually reviewed anything > Well, if that's the case, then why > did we have hundreds and hundreds of emails within the last 12 months > arguing about which way it should work? > > When ya'll come to some final conclusion on how you want the defaults to look, come tell the rest of us. You already have 4 people debating the matter, I don't really see the point of adding more voices to that cachopany. As you noted - voicing an opinion about 0004 is optional. I'll reiterate my review from before, with a bit more confidence this time. 0001-0003 implements a desirable behavior change. In order for someone to make some other role a member in some third role that someone must have admin privileges on both other roles. CREATEROLE is not exempt from this rule. A user with CREATEROLE will, upon creating a new role, be granted admin privilege on that role by the bootstrap superuser. The consequence of 0001-0003 in the current environment is that since the newly created CREATEROLE user will not have admin rights on any existing roles in the cluster, while they can create new roles in the system they are unable to grant those new roles membership in any other roles not also created by them. The ability to assign attributes to newly created roles is unaffected. As a unit of work, those are "ready-to-commit" for me. I'll leave it to you and others to judge the technical quality of the patch and finishing up the FIXMEs that have been noted. Desirable follow-on patches include: 1) Automatically install an additional membership grant, with the CREATEROLE user as the grantor, specifying INHERIT OR SET as TRUE (I personally favor attaching these to ALTER ROLE, modifiable only by oneself) 2) Convert Attributes into default roles David J.
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 4:19 PM David G. Johnston wrote: > That's fine, but are you saying this patch is incapable (or simply > undesirable) of having the parts about handling defaults separated out from > the parts that define how the system works with a given set of permissions; > and the one implementation detail of having the bootstrap superuser > automatically grant admin to any roles a createuser role creates? If you and > others feel strongly about defaults I'm sure that the suggested other thread > focused on that will get attention and be committed in a timely manner. But > the system will work, and not be broken, if that got stalled, and it could be > added in later. The topics are so closely intertwined that I don't believe that trying to have separate discussions will be useful or productive. There's no hope of anybody understanding 0004 or having an educated opinion about it without first understanding the earlier patches, and there's no requirement that someone has to review 0004, or like it, just because they review or like 0001-0003. But so far nobody has actually reviewed anything, and all that's happened is people have complained about 0004 for reasons which in my opinion are pretty nebulous and largely ignore the factors that caused it to exist in the first place. We had about 400 emails during the last release cycle arguing about a whole bunch of topics related to user management, and it became absolutely crystal clear in that discussion that Stephen Frost and David Steele wanted to have roles that could create other roles but not immediately be able to access their privileges. Mark and I, on the other hand, wanted to have roles that could create other roles WITH immediate access to their privileges. That argument was probably the main thing that derailed that entire patch set, which represented months of work by Mark. Now, I have come up with a competing patch set that for the price of 100 lines of code and a couple of slightly ugly option names can do either thing. So Stephen and David and any like-minded users can have what they want, and Mark and I and any like-minded users can have what we want. And the result is that I've got like five people, some of whom particulated in those discussions, showing up to say "hey, we don't need the ability to set defaults." Well, if that's the case, then why did we have hundreds and hundreds of emails within the last 12 months arguing about which way it should work? -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 1:28 PM Robert Haas wrote: > On Mon, Nov 28, 2022 at 3:02 PM Mark Dilger > wrote: > > You can argue that a grant with INHERIT FALSE, SET FALSE, ADMIN TRUE > still grants membership, and I think formally that's true, but I also > think it's just picking something to bicker about. The need isn't to > separate membership per se from administration. It's to separate > privilege inheritance and the ability to SET ROLE from role > administration. And I've done that. > We seem to now be in agreement on this design choice, and the related bit about bootstrap superuser granting admin on newly created roles by the createrole user. This seems like a patch in its own right. It still leaves open the default membership behavior as well as whether we want to rework the attributes into predefined roles. > I strongly disagree with the idea that the ability for users to > control defaults here isn't needed. That's fine, but are you saying this patch is incapable (or simply undesirable) of having the parts about handling defaults separated out from the parts that define how the system works with a given set of permissions; and the one implementation detail of having the bootstrap superuser automatically grant admin to any roles a createuser role creates? If you and others feel strongly about defaults I'm sure that the suggested other thread focused on that will get attention and be committed in a timely manner. But the system will work, and not be broken, if that got stalled, and it could be added in later. David J.
Re: fixing CREATEROLE
> On Nov 28, 2022, at 12:33 PM, Mark Dilger > wrote: > >> Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That >> should allow role administration, without actually granting membership in >> that role, yet, right? > > Can you clarify what you mean here? Are you inventing a new syntax? Nevermind. After reading Robert's email, it's clear enough what you mean here. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
> On Nov 28, 2022, at 12:08 PM, walt...@technowledgy.de wrote: > > Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That > should allow role administration, without actually granting membership in > that role, yet, right? Can you clarify what you mean here? Are you inventing a new syntax? +GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE; +ERROR: syntax error at or near "SET" +LINE 1: GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE... — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 3:02 PM Mark Dilger wrote: > Robert's patch tries to deal with the (possibly unwanted) role membership by > setting up defaults to mitigate the effects, but that is more confusing to me > than just de-conflating role membership from role administration, and giving > role creators administration over roles they create, without in so doing > giving them role membership. I don't recall enough details about how hard it > is to de-conflate role membership from role administration, and maybe that's > a non-starter for reasons I don't recall at the moment. I expect Robert has > already contemplated that idea and instead proposed this patch for good > reasons. Robert? "De-conflating role membership from role administration" isn't really a specific proposal that someone can go out and implement. You have to make some decision about *how* you are going to separate those concepts. And that's what I did: I made INHERIT and SET into grant-level options. That allows you to give someone access to the privileges of a role without the ability to administer it (at least one of INHERIT and SET true, and ADMIN false) or the ability to administer a role without having any direct access to its privileges (INHERIT FALSE, SET FALSE, ADMIN TRUE). I don't see that we can, or need to, separate things any more than that. You can argue that a grant with INHERIT FALSE, SET FALSE, ADMIN TRUE still grants membership, and I think formally that's true, but I also think it's just picking something to bicker about. The need isn't to separate membership per se from administration. It's to separate privilege inheritance and the ability to SET ROLE from role administration. And I've done that. I strongly disagree with the idea that the ability for users to control defaults here isn't needed. You can set a default tablespace for your database, and a default tablespace for your session, and a default tablespace for new partitions of an existing partition table. You can set default privileges for every type of object you can create, and a default search path to find objects in the database. You can set defaults for all of your connection parameters to the database using environment variables, and the default data directory for commands that need one. You can set defaults for all of your psql settings in ~/.psqlrc. You can set defaults for the character sets, locales and collations of new databases. You can set the default version of an extension in the control file, so that the user doesn't have to specify a version. And so on and so on. There's absolutely scads of things for which it is useful to be able to set defaults and for which we give people the ability to set defaults, and I don't think anyone is making a real argument for why that isn't also true here. The argument that has been made is essentially that you could get by without it, but that's true of *every* default. Yet we keep adding the ability to set defaults for new things, and to set the defaults for existing things in new ways, and there's a very good reason for that: it's extremely convenient. And that's true here, too. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
Mark Dilger: Robert's patch tries to deal with the (possibly unwanted) role membership by setting up defaults to mitigate the effects, but that is more confusing to me than just de-conflating role membership from role administration, and giving role creators administration over roles they create, without in so doing giving them role membership. I don't recall enough details about how hard it is to de-conflate role membership from role administration, and maybe that's a non-starter for reasons I don't recall at the moment. Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That should allow role administration, without actually granting membership in that role, yet, right? Best, Wolfgang
Re: fixing CREATEROLE
> On Nov 28, 2022, at 11:34 AM, David G. Johnston > wrote: > > No Defaults needed: David J., Mark?, Tom? As Robert has the patch organized, I think defaults are needed, but I see that as a strike against the patch. > Defaults needed - attached to role directly: Robert > Defaults needed - defined within Default Privileges: Walther? > The capability itself seems orthogonal to the rest of the patch to track > these details better. I think we can "Fix CREATEROLE" without any feature > regarding optional default behaviors and would suggest this patch be so > limited and that another thread be started for discussion of (assuming a > default specifying mechanism is wanted overall) how it should look. Let's > not let a usability debate distract us from fixing a real problem. In Robert's initial email, he wrote, "It seems to me that the root of any fix in this area must be to change the rule that CREATEROLE can administer any role whatsoever." The obvious way to fix that is to revoke that rule and instead automatically grant ADMIN OPTION to a creator over any role they create. That's problematic, though, because as things stand, ADMIN OPTION is granted to somebody by granting them membership in the administered role WITH ADMIN OPTION, so membership in the role and administration of the role are conflated. Robert's patch tries to deal with the (possibly unwanted) role membership by setting up defaults to mitigate the effects, but that is more confusing to me than just de-conflating role membership from role administration, and giving role creators administration over roles they create, without in so doing giving them role membership. I don't recall enough details about how hard it is to de-conflate role membership from role administration, and maybe that's a non-starter for reasons I don't recall at the moment. I expect Robert has already contemplated that idea and instead proposed this patch for good reasons. Robert? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
Robert Haas: In my proposal, the "object" is not the GRANT of that role. It's the role itself. So the default privileges express what should happen when the role is created. The default privileges would NOT affect a regular GRANT role TO role_spec command. They only run that command when a role is created. I agree that this is what you are proposing, but it is not what your proposed syntax says. Your proposed syntax simply says ALTER DEFAULT PRIVILEGES .. GRANT. Users who read that are going to think it controls the default behavior for all grants, because that's what the syntax says. If the proposed syntax mentioned CREATE ROLE someplace, maybe that would have some potential. A proposal to make a command that controls CREATE ROLE and only CREATE ROLE and mentions neither CREATE nor ROLE anywhere in the syntax is never going to be acceptable. Yes, I agree - the abbreviated GRANT syntax is confusing/misleading in that case. Consistent with the other syntaxes, but easily confused nonetheless. It's no. Well, OK, you can do it, but it doesn't revoke anything, because you can only revoke your own grant, not the bootstrap superuser's grant. Ah, I see. I didn't get that difference regarding the bootstrap superuser, so far. So in that sense, the ADP GRANT would be an additional GRANT issued by the user that created the role in addition to the bootstrap superuser's grant. You can't revoke the bootstrap superuser's grant - but you can't modify it either. And there is no need to add SET or INHERIT to the boostrap superuser's grant, because you can grant the role yourself again, with those options. I think it would be very strange to have a default for that bootstrap superuser's grant. Or rather: A different default than the minimum required - and that's just ADMIN, not SET, not INHERIT. When you have the minimum, you can always choose to grant SET and INHERIT later on yourself - and revoke it, too! But when the SET and INHERIT are on the boostrap superuser's grant - then there is no way for you to revoke SET or INHERIT on that grant anymore later. Why should the superuser, who gave you CREATEROLE, insist on you having SET or INHERIT forever and disallow to revoke it from yourself? Best, Wolfgang
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 12:42 PM wrote: > David G. Johnston: > > A quick tally of the thread so far: > > > > No Defaults needed: David J., Mark?, Tom? > > Defaults needed - attached to role directly: Robert > > Defaults needed - defined within Default Privileges: Walther? > > s/Walther/Wolfgang > Sorry 'bout that, I was just reading the To: line in my email reply. > > Personally, I'm in the No Defaults needed camp, too. > I kinda thought so from your final comments, thanks for clarifying. David J.
Re: fixing CREATEROLE
David G. Johnston: A quick tally of the thread so far: No Defaults needed: David J., Mark?, Tom? Defaults needed - attached to role directly: Robert Defaults needed - defined within Default Privileges: Walther? s/Walther/Wolfgang The capability itself seems orthogonal to the rest of the patch to track these details better. I think we can "Fix CREATEROLE" without any feature regarding optional default behaviors and would suggest this patch be so limited and that another thread be started for discussion of (assuming a default specifying mechanism is wanted overall) how it should look. Let's not let a usability debate distract us from fixing a real problem. +1 I didn't argue for whether defaults are needed in this case or not. I just said that ADP is better for defaults than role attributes are. Or the other way around: I think role attributes are not a good way to express those. Personally, I'm in the No Defaults needed camp, too. Best, Wolfgang
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 1:56 PM wrote: > And now this reason is gone - there is no reason NOT to implement it as > DEFAULT PRIVILEGES. I think there is, and it's this, which you wrote further down: > In my proposal, the "object" is not the GRANT of that role. It's the > role itself. So the default privileges express what should happen when > the role is created. The default privileges would NOT affect a regular > GRANT role TO role_spec command. They only run that command when a role > is created. I agree that this is what you are proposing, but it is not what your proposed syntax says. Your proposed syntax simply says ALTER DEFAULT PRIVILEGES .. GRANT. Users who read that are going to think it controls the default behavior for all grants, because that's what the syntax says. If the proposed syntax mentioned CREATE ROLE someplace, maybe that would have some potential. A proposal to make a command that controls CREATE ROLE and only CREATE ROLE and mentions neither CREATE nor ROLE anywhere in the syntax is never going to be acceptable. > With how you implemented it right now, is it possible to do the following? > > CREATE ROLE alice; > REVOKE ADMIN OPTION FOR alice FROM CURRENT_USER; > > If the answer is yes, then there is no reason to allow a user to set a > shortcut for SET and INHERIT, but not for ADMIN. > > If the answer is no, then you could just not allow specifying the ADMIN > option in the ALTER DEFAULT PRIVILEGES statement and always force it to > be TRUE. It's no. Well, OK, you can do it, but it doesn't revoke anything, because you can only revoke your own grant, not the bootstrap superuser's grant. > attributes vs. default privileges. Or we could just decide not to, > because is it really that hard to just issue a GRANT statement > immediately after CREATE ROLE, when you want to have SET or INHERIT > options on that role? It's not difficult in the sense that climbing Mount Everest is difficult, but it makes the user experience as a CREATEROLE non-superuser quite noticeably different from being a superuser. Having a way to paper over such differences is, in my opinion, an important usability feature. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Mon, Nov 28, 2022 at 11:57 AM wrote: > Robert Haas: > > I don't know if changing the syntax from A to B is really getting us > > anywhere. I generally agree that the ALTER DEFAULT PRIVILEGES syntax > > looks nicer than the CREATE/ALTER ROLE syntax, but I'm not sure that's > > a sufficient reason to move the control over this behavior to ALTER > > DEFAULT PRIVILEGES. > > Your patch is introducing a new category of role attributes - those that > are affecting default behavior. But there is already a way to express > this right now, and that's ALTER DEFAULT PRIVILEGES in this case. I do not like ALTER DEFAULT PRIVILEGES (ADP) for this. I don't really like defaults, period, for this. The role doing the creation and the role being created are both in scope when the command is executed and if anything it is the role doing to the creation that is receiving the privileges not the role being created. For ADP, the role being created gets the privileges and it is objects not in the scope of the executed command that are being affected. > > One thing to consider is that, as I've designed > > this, whether or not ADMIN is included in the grant is non-negotiable. > > I am, at least at present, inclined to think that was the right call, > > partly because Mark Dilger expressed a lot of concern about the > > CREATEROLE user losing control over the role they'd just created, and > > allowing ADMIN to be turned off would have exactly that effect. Plus a > > grant with INHERIT FALSE, SET FALSE, ADMIN FALSE would end up being > > almost identical to no great at all, which seems pointless. Basically, > > without ADMIN, the implicit GRANT fails to accomplish its intended > > purpose, so I don't like having that as a possibility. > > With how you implemented it right now, is it possible to do the following? > > CREATE ROLE alice; > REVOKE ADMIN OPTION FOR alice FROM CURRENT_USER; > > If the answer is yes, then there is no reason to allow a user to set a > shortcut for SET and INHERIT, but not for ADMIN. > > If the answer is no, then you could just not allow specifying the ADMIN > option in the ALTER DEFAULT PRIVILEGES statement and always force it to > be TRUE. > A prior email described that the creation of a role by a CREATEROLE role results in the necessary creation of an ADMIN grant from the creator to the new role granted by the bootstrap superuser (or, possibly, whichever superuser granted CREATEROLE). That REVOKE will not work as there would be no existing "grant by current_user over alice granted by current_user" immediately after current_user creates alice. Or we could just decide not to, > because is it really that hard to just issue a GRANT statement > immediately after CREATE ROLE, when you want to have SET or INHERIT > options on that role? > > The answer to that question was "yes it is too hard" a while back and as > such DEFAULT PRIVILEGES were introduced. > > A quick tally of the thread so far: No Defaults needed: David J., Mark?, Tom? Defaults needed - attached to role directly: Robert Defaults needed - defined within Default Privileges: Walther? The capability itself seems orthogonal to the rest of the patch to track these details better. I think we can "Fix CREATEROLE" without any feature regarding optional default behaviors and would suggest this patch be so limited and that another thread be started for discussion of (assuming a default specifying mechanism is wanted overall) how it should look. Let's not let a usability debate distract us from fixing a real problem. David J.
Re: fixing CREATEROLE
Robert Haas: I don't know if changing the syntax from A to B is really getting us anywhere. I generally agree that the ALTER DEFAULT PRIVILEGES syntax looks nicer than the CREATE/ALTER ROLE syntax, but I'm not sure that's a sufficient reason to move the control over this behavior to ALTER DEFAULT PRIVILEGES. The list of role attributes can currently be roughly divided into the following categories: - Settings with role-specific values: CONNECTION LIMIT, PASSWORD, VALID UNTIL. It's hard to imagine storing them anywhere else, because they need to have a different value for each role. Those are not just "flags" like all the other attributes. - Two special attributes in INHERIT and BYPASSRLS regarding security/privileges. Those were invented because there was no other syntax to do the same thing. Those could be interpreted as privileges to do something, too - but lacking the ability to do that explicit. There is no SET BYPASSRLS ON/OFF or SET INHERIT ON/OFF. Of course the INHERIT case is now a bit different, because there is the inherit grant option you introduced. - Cluster-wide privileges: SUPERUSER, CREATEDB, CREATEROLE, LOGIN, REPLICATION. Those can't be granted on some kind of object, because there is no such global object. You could imagine inventing some kind of global CLUSTER object and do something like GRANT SUPERUSER ON CLUSTER TO alice; instead. Turning those into role attributes was the choice made instead. Most likely it would have been only a syntactic difference anyway: Even if there was something like GRANT .. ON CLUSTER, you'd most likely implement that as... storing those grants as role attributes. Your patch is introducing a new category of role attributes - those that are affecting default behavior. But there is already a way to express this right now, and that's ALTER DEFAULT PRIVILEGES in this case. Imho, the question asked should not be "why change from syntax A to B?" but rather: Why introduce a new category of role attributes, when there is a way to express the same concept already? I can't see any compelling reason for that, yet. Or not "yet", but rather "anymore". When I understood and remember correctly, you implemented it in a way that a user could not change those new attributes on their own role. This is in fact different to how ALTER DEFAULT PRIVILEGES works, so you could have made an argument that this was better expressed as role attributes. But I think this was asked and agreed on to act differently, so that the user can change this default behavior of what happens when they create a role for themselves. And now this reason is gone - there is no reason NOT to implement it as DEFAULT PRIVILEGES. One thing to consider is that, as I've designed this, whether or not ADMIN is included in the grant is non-negotiable. I am, at least at present, inclined to think that was the right call, partly because Mark Dilger expressed a lot of concern about the CREATEROLE user losing control over the role they'd just created, and allowing ADMIN to be turned off would have exactly that effect. Plus a grant with INHERIT FALSE, SET FALSE, ADMIN FALSE would end up being almost identical to no great at all, which seems pointless. Basically, without ADMIN, the implicit GRANT fails to accomplish its intended purpose, so I don't like having that as a possibility. With how you implemented it right now, is it possible to do the following? CREATE ROLE alice; REVOKE ADMIN OPTION FOR alice FROM CURRENT_USER; If the answer is yes, then there is no reason to allow a user to set a shortcut for SET and INHERIT, but not for ADMIN. If the answer is no, then you could just not allow specifying the ADMIN option in the ALTER DEFAULT PRIVILEGES statement and always force it to be TRUE. The other thing that's a little weird about the syntax which you propose is that it's not obviously related to CREATE ROLE. The intent of the patch as implemented is to allow control over only the implicit GRANT that is created when a new role is created, not all grants that might be created by or to a particular user. Setting defaults for all grants doesn't seem like a particularly good idea to me, but it's definitely a different idea than what the patch proposes to do. Before I proposed that I was confused for a moment about this, too - but it turns out to be wrong. ALTER DEFAULT PRIVILEGES in general works as: When object A is created, issue a GRANT ON A automatically. In my proposal, the "object" is not the GRANT of that role. It's the role itself. So the default privileges express what should happen when the role is created. The default privileges would NOT affect a regular GRANT role TO role_spec command. They only run that command when a role is created. I did spend some time thinking about trying to tie this to the CREATEROLE syntax itself. For example, instead of CREATE ROLE alice CREATEROLE INHERITCREATEDROLES SETCREATEDROLES you could write CREATE ROLE alic
Re: fixing CREATEROLE
On Thu, Nov 24, 2022 at 2:41 AM wrote: > INHERITCREATEDROLES and SETCREATEDROLES behave much like DEFAULT > PRIVILEGES. What about something like: > > ALTER DEFAULT PRIVILEGES FOR alice > GRANT TO alice WITH INHERIT FALSE, SET TRUE, ADMIN TRUE > > The "abbreviated grant" is very much abbreviated, because the original > syntax GRANT a TO b is already quite short to begin with, i.e. there is > no ON ROLE or something like that in it. > > The initial DEFAULT privilege would be INHERIT FALSE, SET FALSE, ADMIN > TRUE, I guess? I don't know if changing the syntax from A to B is really getting us anywhere. I generally agree that the ALTER DEFAULT PRIVILEGES syntax looks nicer than the CREATE/ALTER ROLE syntax, but I'm not sure that's a sufficient reason to move the control over this behavior to ALTER DEFAULT PRIVILEGES. One thing to consider is that, as I've designed this, whether or not ADMIN is included in the grant is non-negotiable. I am, at least at present, inclined to think that was the right call, partly because Mark Dilger expressed a lot of concern about the CREATEROLE user losing control over the role they'd just created, and allowing ADMIN to be turned off would have exactly that effect. Plus a grant with INHERIT FALSE, SET FALSE, ADMIN FALSE would end up being almost identical to no great at all, which seems pointless. Basically, without ADMIN, the implicit GRANT fails to accomplish its intended purpose, so I don't like having that as a possibility. The other thing that's a little weird about the syntax which you propose is that it's not obviously related to CREATE ROLE. The intent of the patch as implemented is to allow control over only the implicit GRANT that is created when a new role is created, not all grants that might be created by or to a particular user. Setting defaults for all grants doesn't seem like a particularly good idea to me, but it's definitely a different idea than what the patch proposes to do. I did spend some time thinking about trying to tie this to the CREATEROLE syntax itself. For example, instead of CREATE ROLE alice CREATEROLE INHERITCREATEDROLES SETCREATEDROLES you could write CREATE ROLE alice CREATEROLE WITH (INHERIT TRUE, SET TRUE) or something like this. That would avoid introducing new, lengthy keywords that are just concatenations of other English words, a kind of syntax that doesn't look particularly nice to me and probably is less friendly to non-English speakers as well. I didn't do it that way because the parser support would be more complex, but I could. CREATEROLE would have to become a keyword again, but that's not a catastrophe. Another idea would be to break the CREATEROLE stuff off from CREATE ROLE entirely and put it all into GRANT. You could imagine syntax like GRANT CREATEROLE (or CREATE ROLE?) TO role_specification WITH (INHERIT TRUE/FALSE, SET TRUE/FALSE). There are a few potential issues with this direction. One, if we did this, then CREATEROLE probably ought to become inheritable, because that's the way grants work in general, and this likely shouldn't be an exception, but this would be a behavior change. However, if it is the consensus that such a behavior change would be an improvement, that might be OK. Two, I wonder what we'd do about the GRANTED BY role_specification clause. We could leave it out, but that would be asymmetric with other GRANT commands. We could also support it and record that information and make this work more like other cases, including, I suppose, the possibility of dependent grants. We'd have to think about what that means exactly. If you revoke CREATEROLE from someone who has granted CREATEROLE to others, I suppose that's a clear dependent grant and needs to be recursively revoked. But what about the implicit grants that were created because the person had CREATEROLE? Are those also dependent grants? And what about the roles themselves? Should revoking CREATEROLE drop the roles that the user in question created? That gets complicated, because those roles might own objects. That's scary, because you might not expect revoking a role permission to result in tables getting dropped. It's also problematic, because those tables might be in some other database where they are inaccessible to the current session. All in all I'm inclined to think that recursing to the roles themselves is a bad plan, but it's debatable. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
Robert Haas: I have to admit that when I realized that was the natural place to put them to make the patch work, my first reaction internally was "well, that can't possibly be right, role properties suck!". But I didn't and still don't see where else to put them that makes any sense at all, so I eventually decided that my initial reaction was misguided. So I can't really blame you for not liking it either, and would be happy if we could come up with something else that feels better. I just don't know what it is: at least as of this moment in time, I believe these naturally ARE properties of the role [...] That might be the wrong view. As I say, I'm open to other ideas, and it's possible there's some nicer way to do it that I just don't see right now. INHERITCREATEDROLES and SETCREATEDROLES behave much like DEFAULT PRIVILEGES. What about something like: ALTER DEFAULT PRIVILEGES FOR alice GRANT TO alice WITH INHERIT FALSE, SET TRUE, ADMIN TRUE The "abbreviated grant" is very much abbreviated, because the original syntax GRANT a TO b is already quite short to begin with, i.e. there is no ON ROLE or something like that in it. The initial DEFAULT privilege would be INHERIT FALSE, SET FALSE, ADMIN TRUE, I guess? Best, Wolfgang
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 4:18 PM Tom Lane wrote: > To be clear, I'm not saying that I know a better answer. But the fact > that these end up so different from other role-property bits seems to > me to suggest that making them role-property bits is the wrong thing. > They aren't privileges in any usual sense of the word --- if they > were, allowing Alice to flip her own bits would obviously be silly. > But all the existing role-property bits, with the exception of > rolinherit, certainly are in the nature of privileges. I think that's somewhat true, but I don't completely agree. I don't think that INHERIT, LOGIN, CONNECTION LIMIT, PASSWORD, or VALID UNTIL are privileges either. I think they're just properties. I would put these in the same category: properties, not privileges. I think that SUPERUSER, CREATEDB, CREATEROLE, REPLICATION, and BYPASSRLS are privileges. > CREATEDB and CREATEROLE don't particularly bother me. We've talked before > about replacing them with memberships in predefined roles, and that would > be fine. But the reason nobody's got around to that (IMNSHO) is that it > won't really add much. I agree, although I'm not sure that means that we don't need to do anything about them as we evolve the system. > The thing that I think is a big wart is > rolinherit. I don't know quite what to do about it. One option is to nuke it from orbit. Now that you can set that property on a per-grant basis, the per-role basis serves only to set the default. I think that's of dubious value, and arguably backwards, because ISTM that in a lot of cases whether you want a role grant to be inherited will depend on the nature of the role being granted rather than the role to which it is being granted. The setting we have works the other way around, and I can never keep in my head what the use case for that is. I think there must be one, though, because Peter Eisentraut seemed to like having it around. I don't understand why, but I respect Peter. :-) > But these two new > proposed bits seem to be much the same kind of wart, so I'd rather not > invent them, at least not in the form of role properties. I have to admit that when I realized that was the natural place to put them to make the patch work, my first reaction internally was "well, that can't possibly be right, role properties suck!". But I didn't and still don't see where else to put them that makes any sense at all, so I eventually decided that my initial reaction was misguided. So I can't really blame you for not liking it either, and would be happy if we could come up with something else that feels better. I just don't know what it is: at least as of this moment in time, I believe these naturally ARE properties of the role, and therefore I'm inclined to view my initial reluctance to implement it that way as a reflex rather than a well-considered opinion. That is, the CREATE ROLE syntax is clunky, and it controls some things that are properties and others that are permissions, but they're not inherited like regular permissions, so it stinks, and thus adding things to it also feels stinky. But if the existing command weren't such a mess I'm not sure adding this stuff to it would feel bad either. That might be the wrong view. As I say, I'm open to other ideas, and it's possible there's some nicer way to do it that I just don't see right now. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
"David G. Johnston" writes: > On Wed, Nov 23, 2022 at 2:18 PM Robert Haas wrote: >> Either way, I'm not quite sure what the benefit of converting these >> things to predefined roles is. > Specifically, you gain inheritance/set and "admin option" for free. Right: the practical issue with CREATEROLE/CREATEDB is that you need some mechanism for managing who can grant those privileges. The current answer isn't very flexible, which has been complained of repeatedly. If they become predefined roles then we get a lot of already-built-out infrastructure to solve that, instead of having to write even more single-purpose logic. I think it's a sensible future path, but said lack of flexibility hasn't yet spurred anyone to do it. regards, tom lane
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 2:18 PM Robert Haas wrote: > On Wed, Nov 23, 2022 at 3:59 PM David G. Johnston > wrote: > > I haven't yet formed a complete thought here but is there any reason we > cannot convert the permission-like attributes to predefined roles? > > > > pg_login > > pg_replication > > pg_bypassrls > > pg_createdb > > pg_createrole > > pg_haspassword (password and valid until) > > pg_hasconnlimit > > > > Presently, attributes are never inherited, but having that be controlled > via the INHERIT property of the grant seems desirable. > > I think that something like this might be possible, but I'm not > convinced that it's a good idea. > > Either way, I'm not quite sure what the benefit of converting these > things to predefined roles is. Specifically, you gain inheritance/set and "admin option" for free. So whether I have an ability and whether I can grant it are separate concerns. > A password is a fine example of that. You should never > inherit someone else's password. Whether we've chosen the right set of > things to treat as per-role properties rather than predefined roles is > very much debatable, though, as are a number of other aspects of the > role system. > You aren't inheriting a specific password, you are inheriting the right to have a password stored in the database, with an optional expiration date. > > For instance, I'm pretty well unconvinced that merging users and > groups into a uniformed thing called roles was a good idea. I agree. No one was interested in the, admittedly complex, psql queries I wrote the other month but I decided to undo some of that decision there. David J.
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 2:01 PM Robert Haas wrote: > In the latter case there are two, one with > > grantor=bootstrap_supeuser/admin_option=true/set_option=false/inherit_option=false > and a second with > grantor=alice/admin_option=false/set_option=true/inherit_option=true. > This, IMO, is preferable. And I'd probably typically want to hide the first grant from the user in typical cases - it is an implementation detail. We have to grant the creating role membership in the created role, with admin option, as a form of bookkeeping. If the creating role really wants to be a member of the created role for other reasons that should be done explicitly and granted by the creating role. This patch series need not be concerned about how easy or difficult it is to get the additional grant entry into the database. The ability to refine the permissions in the data model is there so there should be no complaints that "it is impossible to set up this combination of permissions". We've provided a detailed model and commands to alter it - the users can build their scripts to glue those things together. David J.
Re: fixing CREATEROLE
Robert Haas writes: > But having said that, I could certainly change the patches so that any > user, or maybe just a createrole user since it's otherwise irrelevant, > can flip the INHERITCREATEDROLE and SETCREATEDROLE bits on their own > account. There would be no harm in that from a security or auditing > perspective AFAICS. It would, however, make the handling of those > flags different from the handling of most other role-level properties. > That is in fact why things ended up the way that they did: I just made > the new role-level properties which I added work like most of the > existing ones. To be clear, I'm not saying that I know a better answer. But the fact that these end up so different from other role-property bits seems to me to suggest that making them role-property bits is the wrong thing. They aren't privileges in any usual sense of the word --- if they were, allowing Alice to flip her own bits would obviously be silly. But all the existing role-property bits, with the exception of rolinherit, certainly are in the nature of privileges. > What I would > particularly like to hear in such an argument, though, is a theory > that goes beyond those two particular properties and addresses what > ought to be done with all the other ones, especially CREATEDB and > CREATEROLE. CREATEDB and CREATEROLE don't particularly bother me. We've talked before about replacing them with memberships in predefined roles, and that would be fine. But the reason nobody's got around to that (IMNSHO) is that it won't really add much. The thing that I think is a big wart is rolinherit. I don't know quite what to do about it. But these two new proposed bits seem to be much the same kind of wart, so I'd rather not invent them, at least not in the form of role properties. regards, tom lane
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 3:59 PM David G. Johnston wrote: > I haven't yet formed a complete thought here but is there any reason we > cannot convert the permission-like attributes to predefined roles? > > pg_login > pg_replication > pg_bypassrls > pg_createdb > pg_createrole > pg_haspassword (password and valid until) > pg_hasconnlimit > > Presently, attributes are never inherited, but having that be controlled via > the INHERIT property of the grant seems desirable. I think that something like this might be possible, but I'm not convinced that it's a good idea. I've always felt that the role-level properties seemed kind of like warts, but in studying these issues recently, I've come to the conclusion that in some ways that's just a visual impression. The syntax LOOKS outdated and clunky, whereas granting someone a predefined role feels clean and modern. But the reality is that the predefined roles system is full of really unpleasant warts. For example, in talking through the now-committed patch to allow control over SET ROLE, we had some fairly extensive discussion of the fact that there was previously no way to avoid having a user who has been granted the pg_read_all_stats predefined role to create objects owned by pg_read_all_stats, or to alter existing objects. That's really pretty grotty. We now have a way to prevent that, but perhaps we should have something even better. I'm also not really sure that's the only problem here, but maybe it is. Either way, I'm not quite sure what the benefit of converting these things to predefined roles is. I think actually the strongest argument would be to do this for the superuser property! Make the bootstrap superuser the only real superuser, and anyone else who wants to be a superuser has to inherit that from that role. It's really unclear to me why inheriting a lot of permissions is allowable, but inheriting all of them is not allowable. Doing it for something like pg_hasconnlimit seems pretty unappealing by contrast, because that's an integer-valued property, not a Boolean, and it's not at all clear to me why that should be inherited or what the semantics ought to be. Really, I'm not that tempted to try to rejigger this kind of stuff around because it seems like a lot of work for not a whole lot of benefit. I think there's a perfectly reasonable case for setting some things on a per-role basis that are actually per-role and not inherited. A password is a fine example of that. You should never inherit someone else's password. Whether we've chosen the right set of things to treat as per-role properties rather than predefined roles is very much debatable, though, as are a number of other aspects of the role system. For instance, I'm pretty well unconvinced that merging users and groups into a uniformed thing called roles was a good idea. I think it makes all of this machinery a LOT harder to understand, which may be part of the reason why this area doesn't seem to have had much TLC in quite a long time. But I think it's too late to revisit that decision, and I also think it's too late to revisit the question of having predefined roles at all. For better or for worse, that's what we did, and what remains now is to find a way to make the best of it in light of those decisions. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 3:33 PM Tom Lane wrote: > > Because they are role-level properties, they can be set by whoever has > > ADMIN OPTION on the role. That always includes every superuser, and it > > never includes Alice herself (except if she's a superuser). > > That is just bizarre. Alice can do X, and she can do Y, but she > can't control a flag that says which of those happens by default? > How is that sane (disregarding the question of whether the existence > of the flag is a good idea, which I'm now even less sold on)? Look, I admitted later in that same email that I don't really know what the rules for setting role-level properties ought to be. If you have an idea, I'd love to hear it, but I'd rather if you didn't just label things into which I have put quite a bit of work as insane without giving any constructive feedback, especially if you haven't yet fully understood the proposal. Your description of the behavior here is not quite accurate. Regardless of how the flags are set, alice, as a CREATEROLE user, can gain access to all the privileges of the target role, and she can arrange to have a grant of permissions on that role with INHERIT TRUE and SET TRUE. However, there's a difference between the case where (a) INHERITCREATEDROLE and SETCREATEDROLE are set, and alice gets the permissions of the role by default and the one where (b) NOINHERITCREATEDROLE and NOSETCREATEDROLE are set, and therefore alice gets the permissions only if she does GRANT created_role TO ALICE WITH INHERIT TRUE, SET TRUE. In the former case, there is only one grant, and it has grantor=bootstrap_superuser/admin_option=true/inherit_option=true/set_option=true. In the latter case there are two, one with grantor=bootstrap_supeuser/admin_option=true/set_option=false/inherit_option=false and a second with grantor=alice/admin_option=false/set_option=true/inherit_option=true. That is pretty nearly equivalent, but it is not the same, and it will not, for example, be dumped in the same way. Furthermore, it's not equivalent in the other direction at all. If the superuser gives alice INHERITCREATEDROLES and SETCREATEDROLES, she can't renounce those permissions in the patch as written. All of which is to say that I don't think your characterization of this as "Alice can do X, and she can do Y, but she can't control a flag that says which of those happens by default?" is really correct. It's subtler than that. But having said that, I could certainly change the patches so that any user, or maybe just a createrole user since it's otherwise irrelevant, can flip the INHERITCREATEDROLE and SETCREATEDROLE bits on their own account. There would be no harm in that from a security or auditing perspective AFAICS. It would, however, make the handling of those flags different from the handling of most other role-level properties. That is in fact why things ended up the way that they did: I just made the new role-level properties which I added work like most of the existing ones. I don't think that's insane at all. I even think it might be the right decision. But it's certainly arguable. If you think it should work differently, make an argument for that. What I would particularly like to hear in such an argument, though, is a theory that goes beyond those two particular properties and addresses what ought to be done with all the other ones, especially CREATEDB and CREATEROLE. If we can't come up with such a grand unifying theory but are confident we know what to do about this case, so be it, but we shouldn't make an idiosyncratic rule for this case without at least thinking about the overall picture. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 1:04 PM Robert Haas wrote: > > I'm not very certain about any of that stuff; I don't have a clear > mental model of how it should work, or even what exact problem we're > trying to solve. To me, the patches that I posted make sense as far as > they go, but I'm not under the illusion that they solve all the > problems in this area, or even that I understand what all of the > problems are. > > I haven't yet formed a complete thought here but is there any reason we cannot convert the permission-like attributes to predefined roles? pg_login pg_replication pg_bypassrls pg_createdb pg_createrole pg_haspassword (password and valid until) pg_hasconnlimit Presently, attributes are never inherited, but having that be controlled via the INHERIT property of the grant seems desirable. WITH ADMIN controls passing on of membership to other roles. Example: I have pg_createrole (set, noinherit, no with admin), pg_password (no set, inherit, no with admin), and pg_createdb (set, inherit, with admin), pg_login (no set, inherit, with admin) Roles I create cannot be members of pg_createrole or pg_password but can be given pg_createdb and pg_login (this would be a way to enforce external authentication for roles created by me) I can execute CREATE DATABASE due to inheriting pg_createdb I must set role to pg_createrole in order to execute CREATE ROLE Since I don't have admin on pg_createrole I cannot change my own set/inherit, but I could do that for pg_createdb David J.
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 3:11 PM Mark Dilger wrote: > Ok, so the critical part of this proposal is that auditing tools can tell > when Alice circumvents these settings. Without that bit, the whole thing is > inane. Why make Alice jump through hoops that you are explicitly allowing > her to jump through? Apparently the answer is that you can point a high > speed camera at the hoops. Well put. Also, it's a bit like 'su', right? Typically you don't just log in as root and do everything a root, even if you have access to root privileges. You log in as 'mdilger' or whatever and then when you want to exercise elevated privileges you use 'su' or 'sudo' or something. Similarly here you can make an argument that it's a lot cleaner to give Alice the potential to access all of these privileges than to make her have them all the time. But on the flip side, one big advantage of having 'alice' have the privileges all the time is that, for example, she can probably restore a database dump that might otherwise be restorable only with superuser privileges. As long as she has been granted all the relevant roles with INHERIT TRUE, SET TRUE, the kinds of locutions that pg_dump spits out should pretty much work fine, whereas if Alice is firewalled from the privileges of the roles she manages, that is not going to work well at all. To me, that is a pretty huge advantage, and it's a major reason why I initially thought that alice should just categorically, always inherit the privileges of the roles she creates. But having been burned^Wenlightened by previous community discussion, I can now see both sides of the argument, which is why I am now proposing to let people pick the behavior they happen to want. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
Robert Haas writes: > On Wed, Nov 23, 2022 at 2:28 PM Mark Dilger > wrote: >> I had incorrectly imagined that if the bootstrap superuser granted >> CREATEROLE to Alice with particular settings, those settings would >> limit the things that Alice could do when creating role Bob, >> specifically limiting how much she could administer/inherit/set role >> Bob thereafter. Apparently, your proposal only configures what happens >> by default, and Alice can work around that if she wants to. > Right. Okay ... >> But if that's the case, did I misunderstand upthread that these are >> properties the superuser specifies about Alice? Can Alice just set >> these properties about herself, so she gets the behavior she wants? >> I'm confused now about who controls these settings. > Because they are role-level properties, they can be set by whoever has > ADMIN OPTION on the role. That always includes every superuser, and it > never includes Alice herself (except if she's a superuser). That is just bizarre. Alice can do X, and she can do Y, but she can't control a flag that says which of those happens by default? How is that sane (disregarding the question of whether the existence of the flag is a good idea, which I'm now even less sold on)? regards, tom lane
Re: fixing CREATEROLE
> On Nov 23, 2022, at 12:04 PM, Robert Haas wrote: > >> But if that's the case, did I misunderstand upthread that these are >> properties the superuser specifies about Alice? Can Alice just set these >> properties about herself, so she gets the behavior she wants? I'm confused >> now about who controls these settings. > > Because they are role-level properties, they can be set by whoever has > ADMIN OPTION on the role. That always includes every superuser, and it > never includes Alice herself (except if she's a superuser). It could > include other users depending on the system configuration. For > example, under this proposal, the superuser might create alice and set > her account to CREATEROLE, configuring the INHERITCREATEDROLES and > SETCREATEDROLES properties on Alice's account according to preference. > Then, alice might create another user, say bob, and make him > CREATEROLE as well. In such a case, either the superuser or alice > could set these properties for role bob, because alice enjoys ADMIN > OPTION on role bob. Ok, so the critical part of this proposal is that auditing tools can tell when Alice circumvents these settings. Without that bit, the whole thing is inane. Why make Alice jump through hoops that you are explicitly allowing her to jump through? Apparently the answer is that you can point a high speed camera at the hoops. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 2:28 PM Mark Dilger wrote: > > On Nov 23, 2022, at 11:02 AM, Robert Haas wrote: > > For me, this > > clearly falls into the "good" category: it's configuration that you > > put into the database that makes things happen the way you want, not a > > behavior-changing setting that comes along and ruins somebody's day. > > I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE > to Alice with particular settings, those settings would limit the things that > Alice could do when creating role Bob, specifically limiting how much she > could administer/inherit/set role Bob thereafter. Apparently, your proposal > only configures what happens by default, and Alice can work around that if > she wants to. Right. > But if that's the case, did I misunderstand upthread that these are > properties the superuser specifies about Alice? Can Alice just set these > properties about herself, so she gets the behavior she wants? I'm confused > now about who controls these settings. Because they are role-level properties, they can be set by whoever has ADMIN OPTION on the role. That always includes every superuser, and it never includes Alice herself (except if she's a superuser). It could include other users depending on the system configuration. For example, under this proposal, the superuser might create alice and set her account to CREATEROLE, configuring the INHERITCREATEDROLES and SETCREATEDROLES properties on Alice's account according to preference. Then, alice might create another user, say bob, and make him CREATEROLE as well. In such a case, either the superuser or alice could set these properties for role bob, because alice enjoys ADMIN OPTION on role bob. Somewhat to one side of the question you were asking, but related to the above, I believe there is an opportunity, and perhaps a need, to modify the scope of CREATEROLE in terms of what role-level options a CREATEROLE user can set. For instance, if a CREATEROLE user doesn't have CREATEDB, they can still create users and give them that privilege, even with these patches, and likewise these two new properties. This patch is only concerned about which roles you can manipulate, not what role-level properties you can set. Somebody might feel that's a serious problem, and they might even feel that this patch set ought to something about it. In my view, the issues are somewhat severable. I don't think you can do anything as evil by setting role-level properties (except for SUPERUSER, of course) as what you can do by granting predefined roles, so I don't find restricting those capabilities to be as urgent as doing something to restrict role grants. Also, and this definitely plays into it too, I think there's some debate about what the model ought to look like there. For instance, you could simply stipulate that you can't give what you don't have, but that would mean that every CREATEROLE user can create additional CREATEROLE users, and I suspect some people might like to restrict that. We could add a new CREATECREATEROLE property to decide whether a user can make CREATEROLE users, but by that argument we'd also need a new CREATECREATECREATEROLE property to decide whether a role can make CREATECREATEROLE users, and then it just recurses indefinitely from there. Similarly for CREATEDB. Also, what if anything should you restrict about how the new INHERITCREATEDROLES and SETCREATEDROLES properties should be set? You could argue that they ought to be superuser-only (because the implicit grant is performed by the bootstrap superuser) or that it's fine for them to be set by a CREATEROLE user with ADMIN OPTION (because it's not all that security-critical how they get set) or maybe even that a user ought to be able to set those properties on his or her own role. I'm not very certain about any of that stuff; I don't have a clear mental model of how it should work, or even what exact problem we're trying to solve. To me, the patches that I posted make sense as far as they go, but I'm not under the illusion that they solve all the problems in this area, or even that I understand what all of the problems are. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
> On Nov 23, 2022, at 11:02 AM, Robert Haas wrote: > > For me, this > clearly falls into the "good" category: it's configuration that you > put into the database that makes things happen the way you want, not a > behavior-changing setting that comes along and ruins somebody's day. I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE to Alice with particular settings, those settings would limit the things that Alice could do when creating role Bob, specifically limiting how much she could administer/inherit/set role Bob thereafter. Apparently, your proposal only configures what happens by default, and Alice can work around that if she wants to. But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice? Can Alice just set these properties about herself, so she gets the behavior she wants? I'm confused now about who controls these settings. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 1:11 PM Tom Lane wrote: > I haven't thought about these issues hard enough to form an overall > opinion (though I agree that making CREATEROLE less tantamount > to superuser would be an improvement). However, I share Mark's > discomfort about making these commands act differently depending on > context. We learned long ago that allowing GUCs to affect query > semantics was a bad idea. Basing query semantics on properties > of the issuing role (beyond success-or-permissions-failure) doesn't > seem a whole lot different from that. It still means that > applications can't just issue command X and expect Y to happen; > they have to inquire about context in order to find out that Z might > happen instead. That's bad in any case, but it seems especially bad > for security-critical behaviors. I'm not sure that this behavior qualifies as security-critical. If INHERITCREATEDROLES and SETCREATEDROLES are both true, then the grant has INHERIT TRUE and SET TRUE and there are no more rights to be gained. If not, the createrole user can do something like GRANT new_role TO my_own_account WITH INHERIT TRUE, SET TRUE. Even if we somehow disallowed that, they could gain access to the privilege of the created role in a bunch of other ways, such as granting the rights to someone else, or just changing the password and using the new password to log into the account. When I started working in this area, I thought non-inherited grants were pretty useless, because you can so easily work around it. However, other people did not agree. From what I can gather, I think the reason why people like non-inherited grants is that they prevent mistakes. A user who has ADMIN OPTION on another role but does not inherit its privileges can break into that account and do whatever they want, but they cannot ACCIDENTALLY perform an operation that makes use of that user's privileges. They will have to SET ROLE, or GRANT themselves something, and those actions can be logged and audited if desired. Because of the potential for that sort of logging and auditing, you can certainly make an argument that this is a security-critical behavior, but it's not that clear cut, because it's making assumptions about the behavior of other software, and of human beings. Strictly speaking, looking just at PostgreSQL, these options don't affect security. On the more general question of configurability, I tend to agree that it's not great to have the behavior of commands depend too much on context, especially for security-critical things. A particularly toxic example IMHO is search_path, which I think is an absolute disaster in terms of security that I believe we will never be able to fully fix. Yet there are plenty of examples of configurability that no one finds problematic. No one agitates against the idea that a database can have a default tablespace, or that you can ALTER USER or ALTER DATABASE to configure an setting on a user-specific or database-specific setting, even a security-critical one like search_path, or one that affects query behavior like work_mem. No one is outraged that a data type has a default btree operator class that is used for indexes unless you specify another one explicitly. What people mostly complain about IME is stuff like standard_conforming_strings, or bytea_output, or datestyle. Often, when proposal come up on pgsql-hackers and get shot down on these grounds, the issue is that they would essentially make it impossible to write SQL that will run portably on PostgreSQL. Instead, you'd need to write your application to issue different SQL depending on the value of settings on the local system. That's un-fun at best, and impossible at worst, as in the case of extension scripts, whose content has to be static when you ship the thing. But it's not exactly clear to me what the broader organizing principle is here, or ought to be. I think it would be ridiculous to propose -- and I assume that you are not proposing -- that no command should have behavior that in any way depends on what SQL commands have been executed previously. Taken to a silly extreme, that would imply that CREATE TABLE ought to be removed, because the behavior of SELECT * FROM something will otherwise depend on whether someone has previously issued CREATE TABLE something. Obviously that is a stupid argument. But on the other hand, it would also be ridiculous to propose the reverse, that it's fine to add arbitrary settings that affect the behavior of any command whatsoever in arbitrary ways. Simon's proposal to add a GUC that would make vacuum request a background vacuum rather than performing one in the foreground is a good example of a proposal that did not sit well with either of us. But I don't know on what basis exactly we put a proposal like this in one category rather than the other. I'm not sure I can really articulate the general principle in a sensible way. For me, this clearly falls into the "good" category: it's configuration th
Re: fixing CREATEROLE
Robert Haas writes: > On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger > wrote: >> Yes, this all makes sense, but does it entail that the CREATE ROLE command >> must behave differently on the basis of a setting? > Well, we certainly don't HAVE to add those new role-level properties; > that's why they are in a separate patch. But I think they add a lot of > useful functionality for a pretty minimal amount of extra code > complexity. I haven't thought about these issues hard enough to form an overall opinion (though I agree that making CREATEROLE less tantamount to superuser would be an improvement). However, I share Mark's discomfort about making these commands act differently depending on context. We learned long ago that allowing GUCs to affect query semantics was a bad idea. Basing query semantics on properties of the issuing role (beyond success-or-permissions-failure) doesn't seem a whole lot different from that. It still means that applications can't just issue command X and expect Y to happen; they have to inquire about context in order to find out that Z might happen instead. That's bad in any case, but it seems especially bad for security-critical behaviors. regards, tom lane
Re: fixing CREATEROLE
On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger wrote: > Oh, I don't mean that it will be poorly documented. I mean that the way the > command is written won't advertise what it is going to do. That's concerning > if you fat-finger a CREATE ROLE command, then realize you need to drop and > recreate the role, only to discover that a property you weren't thinking > about, and which you are accustomed to being set the opposite way, is set > such that you can't drop the role you just created. That doesn't ever happen. No matter how the properties are set, you end up with ADMIN OPTION on the newly-created role and can drop it. The flags control things like whether you can select from the newly created role's tables even if you otherwise lack permissions on them (INHERIT), and whether you can SET ROLE to it (SET). You can always administer it, i.e. grant rights on it to others, change its password, drop it. > I think if you're going to create-and-disown something, you should have to > say so, to make sure you mean it. Reasonable, but not relevant, since that isn't what's happening. > Why not make this be a permissions issue, rather than a default behavior > issue? Instead of a single CREATEROLE privilege, grant roles privileges to > CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, > CREATE-SANS-ADMIN, and thereby limit which forms of the command they may > execute. That way, the semantics of the command do not depend on some > property external to the command. Users (and older scripts) will expect the > traditional syntax to behave consistent with how CREATE ROLE has worked in > the past. The behaviors can be specified explicitly. Perhaps if we get the confusion above cleared up you won't be as concerned about this, but let me just say that this patch is absolutely breaking backward compatibility. I don't feel bad about that, either. I think it's a good thing in this case, because the current behavior is abjectly broken and horrible. What we've been doing for the last several years is shipping a product that has a built-in exploit that a clever 10-year-old could use to escalate privileges from CREATEROLE to SUPERUSER. We should not be OK with that, and we should be OK with changing the behavior however much is required to fix it. I'm personally of the opinion that this patch set does a rather clever job minimizing that blast radius, but that might be my own bias as the patch author. Regardless, I don't think there's any reasonable argument for maintaining the current behavior. I don't entirely follow exactly what you have in mind in the sentence above, but if it involves keeping the current CREATEROLE behavior around in any form, -1 from me. > > But since this implicit grant has, and must have, the bootstrap > > superuser as grantor, it is also only reasonable that superusers get > > to determine what options are used when creating that grant, rather > > than leaving that up to the CREATEROLE user. > > Yes, this all makes sense, but does it entail that the CREATE ROLE command > must behave differently on the basis of a setting? Well, we certainly don't HAVE to add those new role-level properties; that's why they are in a separate patch. But I think they add a lot of useful functionality for a pretty minimal amount of extra code complexity. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
> On Nov 23, 2022, at 9:01 AM, Robert Haas wrote: > >> That's not to say that I wouldn't rather that it always work one way or >> always the other. It's just to say that I don't want it to work differently >> based on some poorly advertised property of the role executing the command. > > That seems rather pejorative. If we stipulate from the outset that the > property is poorly advertised, then obviously anything at all > depending on it is not going to seem like a very good idea. But why > should we assume that it will be poorly-advertised? I clearly said > that the documentation needs a bunch of work, and that I planned to > work on it. As an initial matter, the documentation is where we > advertise new features, so I think you ought to take it on faith that > this will be well-advertised, unless you think that I'm completely > hopeless at writing documentation or something. Oh, I don't mean that it will be poorly documented. I mean that the way the command is written won't advertise what it is going to do. That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and recreate the role, only to discover that a property you weren't thinking about, and which you are accustomed to being set the opposite way, is set such that you can't drop the role you just created. I think if you're going to create-and-disown something, you should have to say so, to make sure you mean it. This consideration differs from the default schema or default tablespace settings. If I fat-finger the creation of a table, regardless of where it gets placed, I'm still the owner of the table, and I can still drop and recreate the table to fix my mistake. Why not make this be a permissions issue, rather than a default behavior issue? Instead of a single CREATEROLE privilege, grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and thereby limit which forms of the command they may execute. That way, the semantics of the command do not depend on some property external to the command. Users (and older scripts) will expect the traditional syntax to behave consistent with how CREATE ROLE has worked in the past. The behaviors can be specified explicitly. > On the actual issue, I think that one key question is who should > control what happens when a role is created. Is that the superuser's > decision, or the CREATEROLE user's decision? I think it's better for > it to be the superuser's decision. Consider first the use case where > you want to set up a user who "feels like a superuser" i.e. inherits > the privileges of users they create. You don't want them to have to > specify anything when they create a role for that to happen. You just > want it to happen. So you want to set up their account so that it will > happen automatically, not throw the complexity back on them. In the > reverse scenario where you don't want the privileges inherited, I > think it's a little less clear, possibly just because I haven't > thought about that scenario as much, but I think it's very reasonable > here too to want the superuser to set up a configuration for the > CREATEROLE user that does what the superuser wants, rather than what > the CREATEROLE user wants. > > Even aside from the question of who controls what, I think it is far > better from a usability perspective to have ways of setting up good > defaults. That is why we have the default_tablespace GUC, for example. > We could have made the CREATE TABLE command always use the database's > default tablespace, or we could have made it always use the main > tablespace. Then it would not be dependent on (poorly advertised?) > settings elsewhere. But it would also be really inconvenient, because > if someone is creating a lot of tables and wants them all to end up in > the same place, they don't want to have to specify the name of that > tablespace each time. They want to set a default and have that get > used by each command. > > There's another, subtler consideration here, too. Since > ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who > can validly be recorded as the grantor of a particular role grant, > just as we have always done for other types of grants. The grants have > to form a tree, with each grant having a grantor that was themselves > granted ADMIN OPTION by someone else, until eventually you get back to > the bootstrap superuser who is the source of all privileges. Thus, > today, when a CREATEROLE user grants membership in a role, the grantor > is recorded as the bootstrap superuser, because they might not > actually possess ADMIN OPTION on the role at all, and so we can't > necessarily record them as the grantor. But this patch changes that, > which I think is a significant improvement. The implicit grant that is > created when CREATE ROLE is issued has the bootstrap superuser as > grantor, because there is no other legal option, but then any further
Re: fixing CREATEROLE
On Tue, Nov 22, 2022 at 5:48 PM Mark Dilger wrote: > Whatever behavior is to happen in the CREATE ROLE statement should be spelled > out in that statement. "CREATE ROLE bob WITH INHERIT false WITH SET false" > doesn't seem too unwieldy, and has the merit that it can be read and > understood without reference to hidden parameters. Forcing this to be > explicit should be safer if these statements ultimately make their way into > dump/restore scripts, or into logical replication. > > That's not to say that I wouldn't rather that it always work one way or > always the other. It's just to say that I don't want it to work differently > based on some poorly advertised property of the role executing the command. That seems rather pejorative. If we stipulate from the outset that the property is poorly advertised, then obviously anything at all depending on it is not going to seem like a very good idea. But why should we assume that it will be poorly-advertised? I clearly said that the documentation needs a bunch of work, and that I planned to work on it. As an initial matter, the documentation is where we advertise new features, so I think you ought to take it on faith that this will be well-advertised, unless you think that I'm completely hopeless at writing documentation or something. On the actual issue, I think that one key question is who should control what happens when a role is created. Is that the superuser's decision, or the CREATEROLE user's decision? I think it's better for it to be the superuser's decision. Consider first the use case where you want to set up a user who "feels like a superuser" i.e. inherits the privileges of users they create. You don't want them to have to specify anything when they create a role for that to happen. You just want it to happen. So you want to set up their account so that it will happen automatically, not throw the complexity back on them. In the reverse scenario where you don't want the privileges inherited, I think it's a little less clear, possibly just because I haven't thought about that scenario as much, but I think it's very reasonable here too to want the superuser to set up a configuration for the CREATEROLE user that does what the superuser wants, rather than what the CREATEROLE user wants. Even aside from the question of who controls what, I think it is far better from a usability perspective to have ways of setting up good defaults. That is why we have the default_tablespace GUC, for example. We could have made the CREATE TABLE command always use the database's default tablespace, or we could have made it always use the main tablespace. Then it would not be dependent on (poorly advertised?) settings elsewhere. But it would also be really inconvenient, because if someone is creating a lot of tables and wants them all to end up in the same place, they don't want to have to specify the name of that tablespace each time. They want to set a default and have that get used by each command. There's another, subtler consideration here, too. Since ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who can validly be recorded as the grantor of a particular role grant, just as we have always done for other types of grants. The grants have to form a tree, with each grant having a grantor that was themselves granted ADMIN OPTION by someone else, until eventually you get back to the bootstrap superuser who is the source of all privileges. Thus, today, when a CREATEROLE user grants membership in a role, the grantor is recorded as the bootstrap superuser, because they might not actually possess ADMIN OPTION on the role at all, and so we can't necessarily record them as the grantor. But this patch changes that, which I think is a significant improvement. The implicit grant that is created when CREATE ROLE is issued has the bootstrap superuser as grantor, because there is no other legal option, but then any further grants performed by the CREATE ROLE user rely on that user having that grant, and thus record the OID of the CREATEROLE user as the grantor, not the bootstrap superuser. That, in turn, has a number of rather nice consequences. It means in particular that the CREATEROLE user can't remove the implicit grant, nor can they alter it. They are, after all, not the grantor, who is the bootstrap superuser, nor do they any longer have the authority to act as the bootstrap superuser. Thus, if you have two or more CREATEROLE users running around doing stuff, you can tell who did what. Every role that those users created is linked back to the creating role in a way that the creator can't alter. A CREATEROLE user is unable to contrive a situation where they no longer control a role that they created. That also means that the superuser, if desired, can revoke all membership grants performed by any particular CREATEROLE user by revoking the implicit grants with CASCADE. But since this implicit grant has, and must have, the bootstrap superuser as gr
Re: fixing CREATEROLE
> On Nov 22, 2022, at 2:02 PM, Robert Haas wrote: > >> Patch 0004 feels like something that won't get committed. The >> INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky. > > I think role properties are kind of clunky in general, the way we've > implemented them in PostgreSQL, but I don't really see why these are > worse than anything else. I think we need some way to control the > behavior, and I don't really see a reasonable place to put it other > than a per-role property. And if we're going to do that then they > might as well look like the other properties that we've already got. > > Do you have a better idea? Whatever behavior is to happen in the CREATE ROLE statement should be spelled out in that statement. "CREATE ROLE bob WITH INHERIT false WITH SET false" doesn't seem too unwieldy, and has the merit that it can be read and understood without reference to hidden parameters. Forcing this to be explicit should be safer if these statements ultimately make their way into dump/restore scripts, or into logical replication. That's not to say that I wouldn't rather that it always work one way or always the other. It's just to say that I don't want it to work differently based on some poorly advertised property of the role executing the command. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
On Tue, Nov 22, 2022 at 3:01 PM Mark Dilger wrote: > > On Nov 21, 2022, at 12:39 PM, Robert Haas wrote: > > I have drafted a few patches to try to improve the situation. > > The 0001 and 0002 patches appear to be uncontroversial refactorings. Patch > 0003 looks on-point and a move in the right direction. The commit message in > that patch is well written. Thanks. > Patch 0004 feels like something that won't get committed. The > INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky. I think role properties are kind of clunky in general, the way we've implemented them in PostgreSQL, but I don't really see why these are worse than anything else. I think we need some way to control the behavior, and I don't really see a reasonable place to put it other than a per-role property. And if we're going to do that then they might as well look like the other properties that we've already got. Do you have a better idea? -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
> On Nov 21, 2022, at 12:39 PM, Robert Haas wrote: > > I have drafted a few patches to try to improve the situation. The 0001 and 0002 patches appear to be uncontroversial refactorings. Patch 0003 looks on-point and a move in the right direction. The commit message in that patch is well written. Patch 0004 feels like something that won't get committed. The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: fixing CREATEROLE
On 11/21/22 15:39, Robert Haas wrote: I'm curious to hear what other people think of these proposals, but let me first say what I think about them. First, I think it's clear that we need to do something, because things right now are pretty badly broken and in a way that affects security. Although these patches are not back-patchable, they at least promise to improve things as older versions go out of use. +1 Second, it's possible that we should look for back-patchable fixes here, but I can't really see that we're going to come up with anything much better than just telling people not to use this feature against older releases, because back-patching catalog changes or dramatic behavior changes seems like a non-starter. In other words, I think this is going to be a master-only fix. Yep, seems highly likely Third, someone could well have a better or just different idea how to fix the problems in this area than what I'm proposing here. This is the best that I've been able to come up with so far, but that's not to say it's free of problems or that no improvements are possible. On quick inspection I like what you have proposed and no significantly "better" ideas jump to mind. I will try to think on it though. Finally, I think that whatever we do about the code, the documentation needs quite a bit of work, because the code is doing a lot of stuff that is security-critical and entirely non-obvious from the documentation. I have not in this version of these patches included any documentation changes and the regression test changes that I have included are quite minimal. That all needs to be fixed up before there could be any thought of moving forward with these patches. However, I thought it best to get rough patches and an outline of the proposed direction on the table first, before doing a lot of work refining things. I have looked at, and even done some doc improvements in this area in the past, and concluded that it is simply hard to describe it in a clear, straightforward way. There are multiple competing concepts (privs on objects, attributes of roles, membership, when things are inherited versus not, settings bound to roles, etc). I don't know what to do about it, but yeah, fixing the documentation would be a noble goal. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: fixing CREATEROLE
walt...@technowledgy.de writes: >> No, we don't support partial indexes on catalogs, and I don't think >> we want to change that. Partial indexes would require expression >> evaluations occurring at very inopportune times. > I see. Is that the same for indexes *on* an expression? Or would those > be ok? Right, we don't support those on catalogs either. > Now, you are going to tell me that EXCLUDE constraints are not supported > on catalogs either, right? ;) Nor those. regards, tom lane
Re: fixing CREATEROLE
Wolfgang Walther: Tom Lane: No, we don't support partial indexes on catalogs, and I don't think we want to change that. Partial indexes would require expression evaluations occurring at very inopportune times. I see. Is that the same for indexes *on* an expression? Or would those be ok? With a custom operator, an EXCLUDE constraint on the ROW(reldatabase, relname) expression could work. The operator would compare: - (0, name1) and (0, name2) as name1 == name2 - (db1, name1) and (0, name2) as name1 == name2 - (0, name1) and (db2, name2) as name1 == name2 - (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2 or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2. Does it even need to be on the expression? I don't think so. It would be enough to just make it compare relname WITH = and reldatabase WITH the custom operator (db1 == 0 || db2 == 0 || db1 == db2), right? Best, Wolfgang
Re: fixing CREATEROLE
Tom Lane: No, we don't support partial indexes on catalogs, and I don't think we want to change that. Partial indexes would require expression evaluations occurring at very inopportune times. I see. Is that the same for indexes *on* an expression? Or would those be ok? With a custom operator, an EXCLUDE constraint on the ROW(reldatabase, relname) expression could work. The operator would compare: - (0, name1) and (0, name2) as name1 == name2 - (db1, name1) and (0, name2) as name1 == name2 - (0, name1) and (db2, name2) as name1 == name2 - (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2 or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2. Now, you are going to tell me that EXCLUDE constraints are not supported on catalogs either, right? ;) Best, Wolfgang
Re: fixing CREATEROLE
walt...@technowledgy.de writes: > Robert Haas: >> 2. There are some serious implementation challenges because the >> constraints on duplicate object names must be something which can be >> enforced by unique constraints on the relevant catalogs. Off-hand, I >> don't see how to do that. > For each database created, create a partial unique index: > CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0, > ); > Is that possible on catalogs? No, we don't support partial indexes on catalogs, and I don't think we want to change that. Partial indexes would require expression evaluations occurring at very inopportune times. Also, we don't support creating shared indexes post-initdb. The code has hard-wired lists of which relations are shared, besides which there's no way to update other databases' pg_class. Even without that, the idea of a shared catalog ending up with 1 indexes after you create 1 databases (requiring 10^8 pg_class entries across the whole cluster) seems ... unattractive. regards, tom lane
Re: fixing CREATEROLE
Robert Haas: 2. There are some serious implementation challenges because the constraints on duplicate object names must be something which can be enforced by unique constraints on the relevant catalogs. Off-hand, I don't see how to do that. It would be easy to make the cluster roles all have unique names, and it would be easy to make the database roles have unique names within each database, but I have no idea how you would keep a database role from having the same name as a cluster role. For anyone to try to implement this, we'd need to have a solution to that problem. For each database created, create a partial unique index: CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0, ); Is that possible on catalogs? Best, Wolfgang
Re: fixing CREATEROLE
On Tue, Nov 22, 2022 at 3:02 AM wrote: > My suggestion to $subject and the namespace problem would be to > introduce database-specific roles, i.e. add a database column to > pg_authid. Having this column set to 0 will make the role a cluster-wide > role ("cluster role") just as currently the case. But having a database > oid set will make the role exist in the context of that database only > ("database role"). Then, the following principles should be enforced: > > - database roles can not share the same name with a cluster role. > - database roles can have the same name as database roles in other > databases. > - database roles can not be members of database roles in **other** > databases. > - database roles with CREATEROLE can only create or alter database roles > in their own database, but not roles in other databases and also not > cluster roles. > - database roles with CREATEROLE can GRANT all database roles in the > same database, but only those cluster roles they have ADMIN privilege on. > - database roles with CREATEROLE can not set SUPERUSER. > > To be able to create database roles with a cluster role, there needs to > be some syntax, e.g. something like > > CREATE ROLE name IN DATABASE dbname ... I have three comments on this: 1. It's a good idea and might make for some interesting followup work. 2. There are some serious implementation challenges because the constraints on duplicate object names must be something which can be enforced by unique constraints on the relevant catalogs. Off-hand, I don't see how to do that. It would be easy to make the cluster roles all have unique names, and it would be easy to make the database roles have unique names within each database, but I have no idea how you would keep a database role from having the same name as a cluster role. For anyone to try to implement this, we'd need to have a solution to that problem. 3. I don't want to sidetrack this thread into talking about possible future features or followup work. There is enough to do just getting consensus on the design ideas that I proposed without addressing the question of what else we might do later. I do not think there is any reasonable argument that we can't clean up the CREATEROLE mess without also implementing database-specific roles, and I have no intention of including that in this patch series. Whether I or someone else might work on it in the future is a question we can leave for another day. -- Robert Haas EDB: http://www.enterprisedb.com
Re: fixing CREATEROLE
Robert Haas: It seems to me that the root of any fix in this area must be to change the rule that CREATEROLE can administer any role whatsoever. Agreed. Instead, I propose to change things so that you can only administer roles for which you have ADMIN OPTION. [...] > I'm curious to hear what other people think of these proposals, [...] Third, someone could well have a better or just different idea how to fix the problems in this area than what I'm proposing here. Once you can restrict CREATEROLE to only manage "your own" (no matter how that is defined, e.g. via ADMIN or through some "ownership" concept) roles, the possibility to "namespace" those roles somehow will become a lot more important. For example in a multi-tenant setup in the same cluster, where each tenant has their own database and admin user with a restricted CREATEROLE privilege, it will very quickly be at least quite annoying to have conflicts with other tenants' role names. I'm not sure whether it could even be a serious problem, because I should still be able to GRANT my own roles to other roles from other tenants - and that could affect matching of +group records in pg_hba.conf? My suggestion to $subject and the namespace problem would be to introduce database-specific roles, i.e. add a database column to pg_authid. Having this column set to 0 will make the role a cluster-wide role ("cluster role") just as currently the case. But having a database oid set will make the role exist in the context of that database only ("database role"). Then, the following principles should be enforced: - database roles can not share the same name with a cluster role. - database roles can have the same name as database roles in other databases. - database roles can not be members of database roles in **other** databases. - database roles with CREATEROLE can only create or alter database roles in their own database, but not roles in other databases and also not cluster roles. - database roles with CREATEROLE can GRANT all database roles in the same database, but only those cluster roles they have ADMIN privilege on. - database roles with CREATEROLE can not set SUPERUSER. To be able to create database roles with a cluster role, there needs to be some syntax, e.g. something like CREATE ROLE name IN DATABASE dbname ... A database role with CREATEROLE should not need to use that syntax, though - every CREATE ROLE should be IN DATABASE anyway. With database roles, it would be possible to hand out CREATEROLE without the ability to grant SUPERUSER or any of the built-in roles. It would be much more useful on top of that, too. Not only is the namespace problem mentioned above solved, but it would also be possible to let pg_dump dump a whole database, including the database roles and their memberships. This would allow dumping (and restoring) a single tenant/application including the relevant roles and privileges - without dumping all roles in the cluster. Plus, it's backwards compatible because without creating database roles, everything stays exactly the same. Best, Wolfgang
fixing CREATEROLE
The CREATEROLE permission is in a very bad spot right now. The biggest problem that I know about is that it allows you to trivially access the OS user account under which PostgreSQL is running, which is expected behavior for a superuser but simply wrong behavior for any other user. This is because CREATEROLE conveys powerful capabilities not only to create roles but also to manipulate them in various ways, including granting any non-superuser role in the system to any new or existing user, including themselves. Since v11, the roles that can be granted include pg_execute_server_program and pg_write_server_files which are trivially exploitable. Perhaps this should have been treated as an urgent security issue and a fix back-patched, although it is not clear to me exactly what such a fix would look like. Since we haven't done that, I went looking for a way to improve things in a principled way going forward, taking advantage also of recent master-only work to improve various aspects of the role grant system. Here, I feel it important to point out that I think the current system would be broken even if we didn't have predefined roles that are trivially exploitable to obtain OS user access. We would still lack any way to restrict the scope of the CREATEROLE privilege. Sure, the privilege doesn't extend to superusers, but that's not really good enough. Consider: rhaas=# create role alice createrole; CREATE ROLE rhaas=# create role bob password 'known_only_to_bob'; CREATE ROLE rhaas=# set session authorization alice; SET rhaas=> alter role bob password 'known_to_alice'; ALTER ROLE Assuming that some form of password authentication is supported, alice is basically empowered to break into any non-superuser account on the system and assume all of its privileges. That's really not cool: it's OK, I think, to give a non-superuser the right to change somebody else's passwords, but it should be possible to limit it in some way, e.g. to the users that alice creates. Also, while the ability to make this sort of change seems to be the clear intention of the code, it's not documented on the CREATE ROLE page. The problems with pg_execute_server_program et. al. are not documented either; all it says is that you should "regard roles that have the CREATEROLE privilege as almost-superuser-roles," which seems to me to be understating the extent of the problem. I have drafted a few patches to try to improve the situation. It seems to me that the root of any fix in this area must be to change the rule that CREATEROLE can administer any role whatsoever. Instead, I propose to change things so that you can only administer roles for which you have ADMIN OPTION. Administering a role here includes changing the password for a role, renaming a role, dropping a role, setting the comment or security label on a role, or granting membership in that role to another role, whether at role creation time or later. All of these options are treated in essentially two places in the code, so it makes sense to handle them all in a symmetric way. One problem with this proposal is that, if we did exactly this much, then a CREATEROLE user wouldn't be able to administer the roles which they themselves had just created. That seems like it would be restricting the privileges of CREATEROLE users too much. To fix that, I propose when a non-superuser creates a role, the role be implicitly granted back to the creator WITH ADMIN OPTION. This arguably doesn't add any fundamentally new capability because the CREATEROLE user could do something like "CREATE ROLE some_new_role ADMIN myself" anyway, but that's awkward to remember and doing it automatically seems a lot more convenient. However, there's a little bit of trickiness here, too. Granting the new role back to the creator might, depending on whether the INHERIT or SET flags are true or false for the new grant, allow the CREATEROLE user to inherit the privileges of, or set role to, the target role, which under current rules would not be allowed. We can minimize behavior changes from the status quo by setting up the new, implicit grant with SET FALSE, INHERIT FALSE. However, that might not be what everyone wants. It's definitely not what *I* want. I want a way for non-superusers to create new roles and automatically inherit the privileges of those roles just as a superuser automatically inherits everyone's privileges. I just don't want the users who can do this to also be able to break out to the OS as if they were superusers when they're not actually supposed to be. However, it's clear from previous discussion that other people do NOT want that, so I propose to make it configurable. Thus, this patch series also proposes to add INHERITCREATEDROLES and SETCREATEDROLES properties to roles. These have no meaning if the role is not marked CREATEROLE. If it is, then they control the properties of the implicit grant that happens when a CREATEROLE user who is not a superuser creates a role. If INHERITCREATEDROLES is