Re: Granting SET and ALTER SYSTE privileges for GUCs
I wrote: > Here's v17 rebased up to HEAD. Pushed after fooling around with the docs. I have a couple of followup ideas in mind (\dcp and another one), which I'll start separate threads about. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Apr 4, 2022, at 5:12 PM, Tom Lane wrote: > > Wrote it already, no need for you to do it. Thanks! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > On Apr 4, 2022, at 2:26 PM, Tom Lane wrote: >> So I think that instead of what you've got here, we should >> (1) apply the map_old_guc_names[] mapping, which is constant >> (for any one PG release anyway) >> (2) smash to lower case >> (3) verify validity per valid_variable_name. >> >> This also simplifies life on the lookup side, where it's sufficient >> to do steps (1) and (2) before performing a catalog search. >> >> Thoughts? > That sounds right. Do you already have something like that coded, or would > you like me to post a patch? Wrote it already, no need for you to do it. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Apr 4, 2022, at 2:26 PM, Tom Lane wrote: > > Thanks. As I'm working through this, I'm kind of inclined to drop > the has_parameter_privilege() variants that take an OID as object > identifier. This gets back to the fact that I don't think > pg_parameter_acl OIDs have any outside use; we wouldn't even have > them except that we need a way to track their role dependencies > in pg_shdepend. AFAICS users will only ever be interested in > looking up a GUC by name. Any objections? None. > Another thought here is that I see you're expending some code > to store the canonical name of a GUC in pg_parameter_acl, but > I think that's probably going too far. In particular, for the > legacy mixed-case names like "DateStyle", what ends up in the > table is the mixed-case name, which seems problematic. It would > definitely be problematic if an extension used such a name, > because we might or might not be aware of the idiosyncratic > casing at the time a GRANT is issued. I'm thinking that we > really want to avoid looking up custom GUCs at all during GRANT, > because that can't do anything except create hazards. Yikes. It took a few tries to see what you mean. Yes, if the GRANT happens before the LOAD, that can have bad consequences. So I agree something should be changed. > So I think that instead of what you've got here, we should > (1) apply the map_old_guc_names[] mapping, which is constant >(for any one PG release anyway) > (2) smash to lower case > (3) verify validity per valid_variable_name. > > This also simplifies life on the lookup side, where it's sufficient > to do steps (1) and (2) before performing a catalog search. > > Thoughts? That sounds right. Do you already have something like that coded, or would you like me to post a patch? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 4/4/22 17:26, Tom Lane wrote: > Mark Dilger writes: >> [ v15 patch ] > Thanks. As I'm working through this, I'm kind of inclined to drop > the has_parameter_privilege() variants that take an OID as object > identifier. This gets back to the fact that I don't think > pg_parameter_acl OIDs have any outside use; we wouldn't even have > them except that we need a way to track their role dependencies > in pg_shdepend. AFAICS users will only ever be interested in > looking up a GUC by name. Any objections? Not from me cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > [ v15 patch ] Thanks. As I'm working through this, I'm kind of inclined to drop the has_parameter_privilege() variants that take an OID as object identifier. This gets back to the fact that I don't think pg_parameter_acl OIDs have any outside use; we wouldn't even have them except that we need a way to track their role dependencies in pg_shdepend. AFAICS users will only ever be interested in looking up a GUC by name. Any objections? Another thought here is that I see you're expending some code to store the canonical name of a GUC in pg_parameter_acl, but I think that's probably going too far. In particular, for the legacy mixed-case names like "DateStyle", what ends up in the table is the mixed-case name, which seems problematic. It would definitely be problematic if an extension used such a name, because we might or might not be aware of the idiosyncratic casing at the time a GRANT is issued. I'm thinking that we really want to avoid looking up custom GUCs at all during GRANT, because that can't do anything except create hazards. So I think that instead of what you've got here, we should (1) apply the map_old_guc_names[] mapping, which is constant (for any one PG release anyway) (2) smash to lower case (3) verify validity per valid_variable_name. This also simplifies life on the lookup side, where it's sufficient to do steps (1) and (2) before performing a catalog search. Thoughts? regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Apr 4, 2022, at 8:36 AM, Tom Lane wrote: > > Mark Dilger writes: >> If we want to backtrack to v8, that's fine. I can rebase that, port >> some of the other changes from v14 to it, and repost it as v15. > > Are you working on that? I've set aside time this week to hopefully > get this over the finish line, but I don't want to find out that > I've been duplicating your effort. Yes, I expect to be posting the latest in maybe an hour? I believe the latest patch (just reviewing, adjusting code comments, etc.) that I'm preparing to post has all the changes we've discussed, aside from your parameterId renaming. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > If we want to backtrack to v8, that's fine. I can rebase that, port > some of the other changes from v14 to it, and repost it as v15. Are you working on that? I've set aside time this week to hopefully get this over the finish line, but I don't want to find out that I've been duplicating your effort. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Mar 30, 2022 at 8:46 AM Tom Lane wrote: > I don't want to do that with > a blunderbuss, but perhaps there's an argument to do it for specific > cases (search_path comes to mind, though the performance cost could be > significant, since I think setting that in function SET clauses is > common). > I suspect it became considerably moreso when we fixed the search_path CVE since we basically told people that doing so, despite the possible performance hit, was the easiest solution to their immediate dump/restore failures. But ISTM that because that SET has a function invocation context it could bypass any such check. Though maybe the DO command exposes a flaw in that idea. David J.
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > Your proposal to just punt on supporting revocation of set on userset from > public seems fine. We could revisit that in the next development cycle if > anyone really wants to defend it. In particular, I don't see that committing > this feature without that part would create any additional backward > compatibility problems when implementing that later. Yeah. Also, as you noted, we could mark some individual built-in variables as SUSET and add a default GRANT. I don't want to do that with a blunderbuss, but perhaps there's an argument to do it for specific cases (search_path comes to mind, though the performance cost could be significant, since I think setting that in function SET clauses is common). For now, though, saying that you can't restrict SET for USERSET variables seems fine --- there's certainly no loss of capability compared to where we stand today. I'd prefer to get the feature committed in that form and then look at whether we want to tighten things around the margins. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Mar 30, 2022 at 8:12 AM Andrew Dunstan wrote: > > On 3/30/22 09:26, Tom Lane wrote: > > > > > What this loses is the ability to revoke public SET permissions > > on USERSET GUCs. I claim that that is not so valuable as to > > justify all the complication needed to deal with it. Agreed, and in line with my thinking from last night. These default public set grants are indeed the complication and I'm good with the status quo where they are non-revocable. I'm finding it curious that we are choosing to document every (all 6) context that doesn't have this default privilege instead of saying that only the user context variables are granted this default, and now irrevocable, default set privilege. This is in addition to making sure we distinguish between parameter and context in my earlier email. > > Avoiding > > a permissions lookup in the default SET code path seems like > > a pretty important benefit, too. If we force that to happen > > it's going to be a noticeable drag on functions with SET clauses. > > > > > The last point is telling, so +1 > > Indeed. +1 David J.
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/30/22 09:26, Tom Lane wrote: > After sleeping on it, I have a modest proposal for simplifying > these issues. Consider this design: > > 1. In the SET code path, we assume (without any catalog lookup) > that USERSET GUCs can be set. Only for SUSET GUCs do we perform > a permissions lookup. (ALTER SYSTEM does a lookup in both cases.) > > 2. Given this, the default ACL for any GUC can be empty, greatly > simplifying all these management issues. Superusers could do what > they want anyway, so modeling an "owner's default grant" becomes > unnecessary. > > What this loses is the ability to revoke public SET permissions > on USERSET GUCs. I claim that that is not so valuable as to > justify all the complication needed to deal with it. (If a GUC > seems to require some defenses, why is it USERSET?) Avoiding > a permissions lookup in the default SET code path seems like > a pretty important benefit, too. If we force that to happen > it's going to be a noticeable drag on functions with SET clauses. > > The last point is telling, so +1 cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 30, 2022, at 6:59 AM, Mark Dilger wrote: > > We should review the conversation from December and January which included > some arguments for allowing revokes of SET on USERSET from PUBLIC. I don't > want to keep going around in circles on this. Hmm, I guess that conversation was mostly off-list at the PGConn in December. I made a reference to it upthread: > On Mar 6, 2022, at 2:40 PM, Mark Dilger wrote: > > Userset variables are implicitly settable by any user. There was a request, > off-list as I recall, to make it possible to revoke the privilege to set > variables such as "work_mem". To make that possible, but not change the > default behavior vis-a-vis prior releases, I upgraded most userset variables > to suset with a corresponding grant to public on the variable. Sites which > wish to have a more restrictive policy on such variables can revoke that > privilege from public and instead issue more restrictive grants. There were > a few variables where such treatment didn't seem sensible, such as ones to do > with client connections, and I left them alone. I didn't insist on a defense > for why any particular setting needed to be revocable in order to apply this > treatment. My assumption was that sites should be allowed to determine their > own security policies per setting unless there is a technical difficulty for > a given setting that would make it overly burdensome to implement. Your proposal to just punt on supporting revocation of set on userset from public seems fine. We could revisit that in the next development cycle if anyone really wants to defend it. In particular, I don't see that committing this feature without that part would create any additional backward compatibility problems when implementing that later. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 30, 2022, at 6:26 AM, Tom Lane wrote: > > Consider this design: Isn't that just the design I had implemented in v8 several months ago? Subject: [PATCH v8] Allow GRANT of SET and ALTER SYSTEM SET for gucs Allow granting of privilege to set or alter system set variables which otherwise can only be managed by superusers. Each (role,variable,privilege) triple is independently grantable, so a user may be granted privilege to SET but not to ALTER SYSTEM SET on a variable, or vice versa. The privilege to SET a userset variable may be granted, though doing so has no practical effect, since any role can set userset variables anyway. Worse, there is no way to revoke the privilege to SET a userset variable. To remedy that, most core userset variables have been changed to suset, with explicit grants to set the variable to public. I don't think v9 ever got posted to the list, but v10 has: Subject: [PATCH v10] Allow grant and revoke of privileges on settings Allow grant and revoke of privileges to set or alter system set configuration variables. Each (role,variable,privilege) triple can be independently granted or revoked, so a user may be granted privilege to SET but not to ALTER SYSTEM SET on a variable, or vice versa. Privilege to SET a userset variable is implicitly granted to public, but may be revoked. If we want to backtrack to v8, that's fine. I can rebase that, port some of the other changes from v14 to it, and repost it as v15. We should review the conversation from December and January which included some arguments for allowing revokes of SET on USERSET from PUBLIC. I don't want to keep going around in circles on this. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
After sleeping on it, I have a modest proposal for simplifying these issues. Consider this design: 1. In the SET code path, we assume (without any catalog lookup) that USERSET GUCs can be set. Only for SUSET GUCs do we perform a permissions lookup. (ALTER SYSTEM does a lookup in both cases.) 2. Given this, the default ACL for any GUC can be empty, greatly simplifying all these management issues. Superusers could do what they want anyway, so modeling an "owner's default grant" becomes unnecessary. What this loses is the ability to revoke public SET permissions on USERSET GUCs. I claim that that is not so valuable as to justify all the complication needed to deal with it. (If a GUC seems to require some defenses, why is it USERSET?) Avoiding a permissions lookup in the default SET code path seems like a pretty important benefit, too. If we force that to happen it's going to be a noticeable drag on functions with SET clauses. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Mar 30, 2022 at 12:00 AM Mark Dilger wrote: > > > On Mar 28, 2022, at 3:31 PM, Tom Lane wrote: > > > > This is what I meant by saying that you can't just refuse to GRANT on > > unknown GUCs. It makes custom GUCs into a time bomb for dump/restore. > > And that means you need a strategy for dealing with the possibility > > that you don't know whether the GUC is USERSET or not. I think though > > that it might work to just assume that it isn't, in which case dumps > > on unrecognized GUCs that really are USERSET will end up issuing an > > explicit GRANT SET TO PUBLIC that we didn't actually need to do, but it > > won't hurt anything. (Testing that assertion would be a good thing > > to do.) > > Ok, I returned to the idea upthread for a solution to this problem. A grant > or revoke on an unrecognized custom parameter will create a SUSET > placeholder, which is not quite right in some cases. However, the > installation scripts for modules have been updated to manually grant SET > privilege on their custom USERSET parameters, which cleans up the problem, > with one exception: if the user executes a "revoke set on parameter > some.such from public" prior to loading the module which defines parameter > some.such, that revoke won't be retained. That doesn't seem entirely wrong > to me, since no privilege to set the parameter existed when the revoke was > performed, but rather was granted along with the creation of the parameter, > but it also doesn't seem entirely right. Maybe revoke commands (but not > grant commands) should error on unrecognized custom parameters? I didn't > implement that here, but can do so if you think that makes more sense than > this new behavior. > > I changed add_placeholder_variable() to take a GucContext argument. It > previously always used PGC_USERSET, which is what all pre-existing call sites > now pass into it, but that seems a bit inappropriate where we're creating a > placeholder that we intend to treat as a SUSET variable until such time as a > module gets installed saying otherwise. Not changing > add_placeholder_variable in this fashion seems to work just fine. I just > didn't feel comfortable with doing it that way. But if you feel it generates > needless code churn, I could be talked out of doing this. > > I also changed the patch to use the ...HookStr functions for parameters. I > would really like a comment on this from Joshua, to be sure what I'm doing > comports with what he wanted. In particular, I'm uncertain that simply > passing the AclMode (in other words, the istmt->privileges field) for the > grant/revoke to the hook is sufficient. For one, how does the hook want to > distinguish grants from revokes? Do we want a bit for that? And what about > distinguishing WITH GRANT OPTION? I think the hooks are usable right now, > but they might be made better. I had not even been thinking about hooking grant/revoke TBH. In SELinux-type MAC systems we don't always try to control DAC permission changes, and when we do it's not granular, all permission changes just boil down to setattr permission or something of that nature. Thank you.
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Mar 29, 2022 at 9:00 PM Mark Dilger wrote: > A grant or revoke on an unrecognized custom parameter will create a SUSET > placeholder, [...] > which cleans up the problem, with one exception: if the user executes a > "revoke set on parameter some.such from public" prior to loading the module > which defines parameter some.such, that revoke won't be retained. That > doesn't seem entirely wrong to me, since no privilege to set the parameter > existed when the revoke was performed, but rather was granted along with > the creation of the parameter, but it also doesn't seem entirely right. > Maybe revoke commands (but not grant commands) should error on unrecognized > custom parameters? The only revoke role target that makes sense here is the default grant given to public. Aside from that technicality the grant system is purely additive and so only the GRANTS end up retained so far as perpetual state is concerned. For a revoke that doesn't target public we should remove the corresponding unrecognized setting grant if one is present. We don't generally/always raise a notice if a revoke doesn't actually cause something to be ungranted - though I'm partial to being so informed. I suppose we could distinguish the cases where the not-yet-loaded setting name is unrecognized by the system from the one where it is recognized but the grant is actually on a different role (or a revoke entry from public for the setting name is present). For a revoke of an unknown setting from public we should keep an entry somewhere that tells the system that the default grant to public for that setting has been revoked. Maybe there isn't the same timing concern here as there is for GRANT, but if only for symmetry it seems like a good thing to implement. I have the impression I'm missing something in what I wrote above but cannot quite figure out what. In any case as a first pass at this the behavior described is kinda what I'm expecting. David J. P.S. Skimming the patch we are, to my agreement, not touching the ALTER DEFAULT PRIVILEGES command to work with this feature. Should the omission be noted explicitly? At least in the commit message I would think. Though the sentence in [1] "Also, these default privilege settings can be overridden using the ALTER DEFAULT PRIVILEGES command." is rendered only partially correct. [1] https://www.postgresql.org/docs/current/ddl-priv.html P.P.S. + The default privileges for a user parameter allow + PUBLIC to SET and + RESET the assigned value. By default, + PUBLIC has no privileges on + postmaster, superuser-backend, + internal, backend, + sighup, and superuser parameters. Can we rephrase this to something like: By default, PUBLIC has no privileges on parameters in the postmaster, ..., and superuser contexts. pg_settings.context exists and those are the values found there. My initial interpretation of the wording was the postmaster, etc..., were themselves parameters, not containers for many parameters.
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > I was about to write another patch using the HookStr form, but if you are > already editing, then I'll let you make the change. I don't see a problem > with what you are proposing. Don't sweat about that, I can easily rebase what I've done so far over your updates. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
I'm going to be mostly unavailable till Wednesday, but I'll leave you with another thing to chew on: regression=# create user joe; CREATE ROLE regression=# grant set on parameter plpgsql.extra_warnings to joe; ERROR: unrecognized configuration parameter "plpgsql.extra_warnings" This is problematic, because once plpgsql is loaded it works: regression=# load 'plpgsql'; LOAD regression=# grant set on parameter plpgsql.extra_warnings to joe; GRANT If we then do $ pg_dumpall -g it falls over: pg_dumpall: error: query failed: ERROR: unrecognized configuration parameter "plpgsql.extra_warnings" apparently because aclparameterdefault() is allergic to being asked about unknown GUCs, and plpgsql is not loaded in pg_dumpall's session. But if pg_dumpall hadn't failed, it'd produce a dump script containing that same command, which would fail at load time (because, again, plpgsql isn't going to be loaded in the backend reading the restore script). This is what I meant by saying that you can't just refuse to GRANT on unknown GUCs. It makes custom GUCs into a time bomb for dump/restore. And that means you need a strategy for dealing with the possibility that you don't know whether the GUC is USERSET or not. I think though that it might work to just assume that it isn't, in which case dumps on unrecognized GUCs that really are USERSET will end up issuing an explicit GRANT SET TO PUBLIC that we didn't actually need to do, but it won't hurt anything. (Testing that assertion would be a good thing to do.) regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 28, 2022, at 2:54 PM, Tom Lane wrote: > > Yeah, I know it's *possible* to make this work. The question is why is > it good to do it like this rather than to use the string API, now that > we have the latter. AFAICS this way just guarantees that the hook must > do a catalog lookup in order to figure out what you're talking about. Ok, thanks for clarifying. I took the *HookStr versions of the hooks to be an alternative to be used when no Oid was present, something of a last resort. I never thought much about using them under other circumstances. > The core point here is that the actual identity of a GUC is its name. > Any OID that may exist in pg_parameter_acl is just a nonce alias that > means nothing to anybody. Anyone who's trying to, say, enforce that > Joe Blow can't change shared_buffers is going to need to see the GUC > name. (I am, btw, busy doing a lot of renaming in the patch to try > to clarify that these OIDs are not identifiers for GUCs; imagining > that they are just risks confusion.) I was about to write another patch using the HookStr form, but if you are already editing, then I'll let you make the change. I don't see a problem with what you are proposing. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: >> On Mar 28, 2022, at 2:16 PM, Tom Lane wrote: >> I just came across something odd in v12 that is still there in v13: >> ExecGrant_Parameter uses InvokeObjectPostAlterHook not >> InvokeObjectPostAlterHookArgStr. This seems pretty inconsistent. >> Is there a good argument for it? > For SET and ALTER SYSTEM, the target of the action may not have an entry > in pg_parameter_acl, nor an assigned Oid anywhere, so the only > consistent way to pass the argument to the hook is by name. For > GRANT/REVOKE, the parameter must have an Oid, at least by the time the > hook gets called. Yeah, I know it's *possible* to make this work. The question is why is it good to do it like this rather than to use the string API, now that we have the latter. AFAICS this way just guarantees that the hook must do a catalog lookup in order to figure out what you're talking about. The core point here is that the actual identity of a GUC is its name. Any OID that may exist in pg_parameter_acl is just a nonce alias that means nothing to anybody. Anyone who's trying to, say, enforce that Joe Blow can't change shared_buffers is going to need to see the GUC name. (I am, btw, busy doing a lot of renaming in the patch to try to clarify that these OIDs are not identifiers for GUCs; imagining that they are just risks confusion.) regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 28, 2022, at 2:16 PM, Tom Lane wrote: > > I just came across something odd in v12 that is still there in v13: > ExecGrant_Parameter uses InvokeObjectPostAlterHook not > InvokeObjectPostAlterHookArgStr. This seems pretty inconsistent. > Is there a good argument for it? > For SET and ALTER SYSTEM, the target of the action may not have an entry in pg_parameter_acl, nor an assigned Oid anywhere, so the only consistent way to pass the argument to the hook is by name. For GRANT/REVOKE, the parameter must have an Oid, at least by the time the hook gets called. Upthread there was some discussion of a hook not being able to assume a snapshot and working transaction, and hence not being able to query the catalogs. I would think that in a GRANT or REVOKE that hasn't already errored, the hook would have a transaction and could look up whatever it likes? There is a CommandCounterIncrement() call issued in objectNamesToOids() for new parameters, so by the time the hook is running it should be able to see the parameter. Am I reasoning about this the wrong way? > ... or, for that matter, why is there any such call at all? > No other GRANT/REVOKE operation calls such a hook. I think ALTER DEFAULT PRIVILEGES does, though that's not quite the same thing. I don't have a strong opinion on this. Joshua, what's your take? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
I just came across something odd in v12 that is still there in v13: ExecGrant_Parameter uses InvokeObjectPostAlterHook not InvokeObjectPostAlterHookArgStr. This seems pretty inconsistent. Is there a good argument for it? ... or, for that matter, why is there any such call at all? No other GRANT/REVOKE operation calls such a hook. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > On Mar 28, 2022, at 11:31 AM, Tom Lane wrote: >> I think we probably have to trash the core-regression-tests part of >> the patch altogether and instead use a TAP test for whatever testing >> we want to do. It might be all right to test SET privileges without >> testing ALTER SYSTEM, but I'm not sure there's much point in that. > How about putting them under src/test/modules/unsafe_tests ? Ah, that could work; I'd forgotten about that subdirectory. It'd still be a good idea if they didn't fail when run twice in a row. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > Version 12 also introduces \dcp (pneumonic, "Describe Configuration > Parameter") for listing parameters, with \dcp+ also showing the acl, like: The fact that that code is not dry behind the ears is painfully obvious. It's not documented in psql-ref, not tested anywhere AFAICS, and its handling of the pattern parameter is inconsistent with every other \d command. The wildcard character should be * not %. It only accidentally fails to dump core if no pattern is given, too. > \dcp[+] only shows "user" and "superuser" parameters: Why make that restriction? Also, I find it astonishing that this doesn't show the GUC's value by default. The non-plus form of the command seems useless as it stands, or at least its intended use-case is so narrow I can't see it. If we're to have it at all, it seems like it ought to be a reasonably useful shortcut for interrogating pg_settings. I'd expect the base set of columns to be name, value, and possibly unit; then add ACL with +. I'm not sure that GucContext belongs in this at all, but if it does, it's a + column. On the whole perhaps this should be taken out again; it's a bit late in the cycle to be introducing new features, especially ones as subject to bikeshedding as a \d command is. My ideas about what columns to show probably wouldn't match anyone else's ... and we haven't even gotten to whether \dcp is an okay choice of name. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 28, 2022, at 11:31 AM, Tom Lane wrote: > > I think we probably have to trash the core-regression-tests part of > the patch altogether and instead use a TAP test for whatever testing > we want to do. It might be all right to test SET privileges without > testing ALTER SYSTEM, but I'm not sure there's much point in that. How about putting them under src/test/modules/unsafe_tests ? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
I've started reviewing this patch in earnest, and almost immediately hit a serious problem: these regression tests aren't even a little bit committable. For one thing, they fail if you do "make installcheck" twice in a row. This seems to be because the first run leaves some cruft behind in pg_parameter_acl that affects the results of subsequent runs. But the bigger problem is that it is ABSOLUTELY NOT OKAY to test ALTER SYSTEM during an "installcheck" run. That would be true even if you cleaned up all the settings by the end of the run, as the patch fails to do (and for extra demerit, it leaves a superuser role laying around). Even transient effects on the behavior of sessions in other DBs aren't acceptable in installcheck mode, IMO. I think we probably have to trash the core-regression-tests part of the patch altogether and instead use a TAP test for whatever testing we want to do. It might be all right to test SET privileges without testing ALTER SYSTEM, but I'm not sure there's much point in that. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 24, 2022, at 12:06 PM, Andrew Dunstan wrote:
>
>
> On 3/24/22 12:49, Mark Dilger wrote:
>>
>>> On Mar 17, 2022, at 8:41 AM, Andrew Dunstan wrote:
>>>
>>> If we abandoned that for this form of GRANT/REVOKE I think we could
>>> probably get away with
>>>
>>>
>>>GRANT { SET | ALTER SYSTEM } ON setting_name ...
>>>
>>>
>>> I haven't tried it, so I could be all wrong.
>> Version 12 of the patch uses SET and ALTER SYSTEM as the names of the
>> privileges, and PARAMETER as the name of the thing on which the privilege is
>> granted. The catalog table which tracks these grants is now named
>> pg_parameter_acl, and various other parts of the patch have been adjusted to
>> use a "parameter" based, rather than a "setting" based, naming scheme. One
>> exception to this rule is the "setacl" column in pg_parameter_acl, which is
>> much more compact than the "parameteracl" name would be, so that remains
>> under the old name.
>
>
> I can live with it I guess, but it seems perverse to me to have
> pg_settings but pg_paramater_acl effectively referring to the same set
> of things. If we're going to do this perhaps we should create a
> pg_parameters view which is identical to pg_settings and deprecate
> pg_settings. I don;t want to hold up this patch, I think this can
> probably be managed as a follow up item.
Right, the version 12 patch was following Peter's and Tom's comments upthread:
> On Mar 17, 2022, at 7:47 AM, Tom Lane wrote:
>
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt). But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
>
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think. So it'd
> be
>
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
>
> and the new catalog would be pg_parameter_acl, and so on.
We could debate that again, but it seems awfully late in the development cycle.
I'd rather just get this committed, barring any objections?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/24/22 12:49, Mark Dilger wrote:
>
>> On Mar 17, 2022, at 8:41 AM, Andrew Dunstan wrote:
>>
>> If we abandoned that for this form of GRANT/REVOKE I think we could
>> probably get away with
>>
>>
>> GRANT { SET | ALTER SYSTEM } ON setting_name ...
>>
>>
>> I haven't tried it, so I could be all wrong.
> Version 12 of the patch uses SET and ALTER SYSTEM as the names of the
> privileges, and PARAMETER as the name of the thing on which the privilege is
> granted. The catalog table which tracks these grants is now named
> pg_parameter_acl, and various other parts of the patch have been adjusted to
> use a "parameter" based, rather than a "setting" based, naming scheme. One
> exception to this rule is the "setacl" column in pg_parameter_acl, which is
> much more compact than the "parameteracl" name would be, so that remains
> under the old name.
I can live with it I guess, but it seems perverse to me to have
pg_settings but pg_paramater_acl effectively referring to the same set
of things. If we're going to do this perhaps we should create a
pg_parameters view which is identical to pg_settings and deprecate
pg_settings. I don;t want to hold up this patch, I think this can
probably be managed as a follow up item.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Mar 17, 2022 at 12:36 PM Mark Dilger wrote: > > On Mar 17, 2022, at 9:29 AM, Joshua Brindle > > wrote: > > > > I hope this patch can be rolled > > into the contribution from Mark. > > Working on it Thanks for the patch! Great, thanks. I missed one objectId reference (InvokeObjectDropHookStr), fixed version attached. v2-0001-Add-String-object-access-hooks.patch Description: Binary data
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 17, 2022, at 9:29 AM, Joshua Brindle > wrote: > > I hope this patch can be rolled > into the contribution from Mark. Working on it Thanks for the patch! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Mar 17, 2022 at 12:30 PM Joshua Brindle wrote: > I agree with this view, outside of the mixup between MAC and DAC (DAC > is in-core, MAC is via hooks) An excellent point! Exactly why we need expert-level help with this stuff! :-) > So there should be some committers or contributors that keep an eye > out and make sure new objects have appropriate hooks, and since an Oid > based hook for GUCs is inappropriate, I hope this patch can be rolled > into the contribution from Mark. I'm not going to take a position on this patch right this minute, but I appreciate you providing it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Mar 17, 2022 at 12:04 PM Robert Haas wrote:
>
> On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle
> wrote:
> >
> >
> > > I remain of the opinion that this
> > > patch should not concern itself with that, though.
> >
> > So you are saying that people can add new object types to PG with DAC
> > permissions and not concern themselves with MAC capable hooks? Is that
> > an official PG community stance?
>
> I don't know that the community has an official position on that
> topic, but I do not think it's reasonable to expect everyone who
> tinkers with MAC permissions to try to make a corresponding equivalent
> for DAC. The number of people using PostgreSQL with DAC is relatively
> small, and the topic is extremely complicated, and a lot of hackers
> don't really understand it well enough to be sure that whatever they
> might do is right. I think it's reasonable to expect people who
> understand DAC and care about it to put some energy into the topic,
> and not just in terms of telling other people how they have to write
> their patches.
>
> I *don't* think it's appropriate for a patch that touches MAC to
> deliberately sabotage the existing support we have for DAC or to just
> ignore it where the right thing to do is obvious. But maintaining a
> million lines of code is a lot of work, and I can't think of any
> reason why the burden of maintaining relatively little-used features
> should fall entirely on people who don't care about them.
I agree with this view, outside of the mixup between MAC and DAC (DAC
is in-core, MAC is via hooks)
So there should be some committers or contributors that keep an eye
out and make sure new objects have appropriate hooks, and since an Oid
based hook for GUCs is inappropriate, I hope this patch can be rolled
into the contribution from Mark.
The attached patch adds a set of hooks for strings, and changes the
GUC patch from Mark to use it, I tested with this code:
void set_user_object_access_str(ObjectAccessType access, Oid classId,
const char *objName, int subId, void *arg)
{
if (next_object_access_hook_str)
{
(*next_object_access_hook_str)(access, classId, objName, subId, arg);
}
switch (access)
{
case OAT_POST_ALTER:
if (classId == SettingAclRelationId)
{
Oid userid = GetUserId();
bool is_superuser = superuser_arg(userid);
char *username = GETUSERNAMEFROMID(GetUserId());
const char *setting = "setting value";
const char *altering = "altering system";
const char *unknown = "unknown";
const char *action;
if (subId && ACL_SET_VALUE)
action = setting;
else if (subId && ACL_ALTER_SYSTEM)
action = altering;
else
action = unknown;
elog(WARNING, "%s is %s %s (%d)", username, action, objName, is_superuser);
if (strcmp(objName, "log_statement") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Setting %s blocked", objName)));
}
}
default:
break;
}
}
static void
set_user_object_access (ObjectAccessType access, Oid classId, Oid
objectId, int subId, void *arg)
{
if (next_object_access_hook)
{
(*next_object_access_hook)(access, classId, objectId, subId, arg);
}
switch (access)
{
case OAT_FUNCTION_EXECUTE:
{
/* Update the `set_config` Oid cache if necessary. */
set_user_cache_proc(InvalidOid);
/* Now see if this function is blocked */
set_user_block_set_config(objectId);
break;
}
/* fallthrough */
case OAT_POST_CREATE:
{
if (classId == ProcedureRelationId)
{
set_user_cache_proc(objectId);
}
break;
}
default:
break;
}
}
0001-Add-String-object-access-hooks.patch
Description: Binary data
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Mar 17, 2022 at 12:19 PM Mark Dilger wrote: > Joshua helped test the DAC portion of this patch, and answered a number of my > questions on the topic, including in-person back in December. I take your > point, Robert, on the general principle, but the archives should reflect that > Joshua did contribute to this specific patch. I wasn't intending to say otherwise, and if the changes needed for DAC here are straightforward and don't make the patch significantly harder to finish, then I would say Tom is wrong and we should just go ahead and make them. But if they do, then I think it's perfectly fine to say that we're going to leave that alone and let someone with subject-matter expertise sort it out when they have time. We do that all the time with other things, most notably MSVC builds, where we just can't expect every hacker or even every committer to understand exactly what's required to make it all work. I try my best to commit things that don't break it, but sometimes I do, and then Andrew helps sort it out, because he understands it and I don't. I think DAC should fall into that kind of category as well. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 17, 2022, at 9:04 AM, Robert Haas wrote: > > not just in terms of telling other people how they have to write > their patches. ... > the burden of maintaining relatively little-used features > should fall entirely on people who don't care about them. Joshua helped test the DAC portion of this patch, and answered a number of my questions on the topic, including in-person back in December. I take your point, Robert, on the general principle, but the archives should reflect that Joshua did contribute to this specific patch. Joshua, should we drop the DAC portion for postgres 15 and revisit the issue for postgres 16? I think it's getting late in the development cycle to attempt what Tom described upthread, and I'd hate to see the rest of this patch punted. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Mar 16, 2022 at 2:47 PM Tom Lane wrote: > Stepping back a bit ... do we really want to institutionalize the > term "setting" for GUC variables? I realize that the view pg_settings > exists, but the documentation generally prefers the term "configuration > parameters". Where config.sgml uses "setting" as a noun, it's usually > talking about a specific concrete value for a parameter, and you can > argue that the view's name comports with that meaning. But you can't > GRANT a parameter's current value. I agree that the lack of a good user-friendly term for GUCs is a real problem. Here at EDB I've observed even relatively non-technical people using that term, which appears nowhere in the documentation and is utterly unintelligible to a typical end-user. Somebody gets on the phone and tells the customer that they need to set a GUC and the customer is like "what's a guck?" except that they probably don't actually ask that question but are just confused and fail to understand that a postgresql.conf change is being proposed. I hate it. It sucks. I have sort of been trying to promote the use of the word "setting" and use it in my own writing, especially to end-users. That is definitely more intelligible to random users, but it's admittedly also awkward. "Set a setting" just sounds redundant. But "set a configuration variable" sounds wordy, so I don't know. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle wrote: > > > > I remain of the opinion that this > > patch should not concern itself with that, though. > > So you are saying that people can add new object types to PG with DAC > permissions and not concern themselves with MAC capable hooks? Is that > an official PG community stance? I don't know that the community has an official position on that topic, but I do not think it's reasonable to expect everyone who tinkers with MAC permissions to try to make a corresponding equivalent for DAC. The number of people using PostgreSQL with DAC is relatively small, and the topic is extremely complicated, and a lot of hackers don't really understand it well enough to be sure that whatever they might do is right. I think it's reasonable to expect people who understand DAC and care about it to put some energy into the topic, and not just in terms of telling other people how they have to write their patches. I *don't* think it's appropriate for a patch that touches MAC to deliberately sabotage the existing support we have for DAC or to just ignore it where the right thing to do is obvious. But maintaining a million lines of code is a lot of work, and I can't think of any reason why the burden of maintaining relatively little-used features should fall entirely on people who don't care about them. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/17/22 10:47, Tom Lane wrote:
> Peter Eisentraut writes:
>> On 16.03.22 19:47, Tom Lane wrote:
>>> ... Perhaps we could just use "SET" and
>>> "ALTER", or "SET" and "SYSTEM"?
>> I think Oracle and MS SQL Server have many multi-word privilege names.
>> So users are quite used to that. And if we want to add more complex
>> privileges, we might run out of sensible single words eventually. So I
>> would not exclude this approach.
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt). But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
>
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think. So it'd
> be
>
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
>
> and the new catalog would be pg_parameter_acl, and so on.
>
>
The upside of this is that it avoids the inelegant
GRANT SET ON SETTING ...
But I was just looking again at the grammar, and the only reason we need
this keyword at all AFAICS is to disambiguate ALL [PRIVILEGES] cases.
If we abandoned that for this form of GRANT/REVOKE I think we could
probably get away with
GRANT { SET | ALTER SYSTEM } ON setting_name ...
I haven't tried it, so I could be all wrong.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Peter Eisentraut writes:
> On 16.03.22 19:47, Tom Lane wrote:
>> ... Perhaps we could just use "SET" and
>> "ALTER", or "SET" and "SYSTEM"?
> I think Oracle and MS SQL Server have many multi-word privilege names.
> So users are quite used to that. And if we want to add more complex
> privileges, we might run out of sensible single words eventually. So I
> would not exclude this approach.
Well, I still say that "SET" is sufficient for the one privilege name
(unless we really can't make Bison handle that, which I doubt). But
I'm willing to yield on using "ALTER SYSTEM" for the other.
If we go with s/SETTING/PARAMETER/ as per your other message, then
that would be adequately consistent with the docs I think. So it'd
be
GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
and the new catalog would be pg_parameter_acl, and so on.
regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 16.03.22 19:47, Tom Lane wrote: I'm also fairly allergic to the way that this patch has decided to assign multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There is no existing precedent for that, and I think it's going to break client-side code that we don't need to break. It's not coincidental that this forces weird changes in rules about whitespace in the has_privilege functions, for example; and if you think that isn't going to cause problems I think you are wrong. Perhaps we could just use "SET" and "ALTER", or "SET" and "SYSTEM"? I think Oracle and MS SQL Server have many multi-word privilege names. So users are quite used to that. And if we want to add more complex privileges, we might run out of sensible single words eventually. So I would not exclude this approach.
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 16.03.22 19:59, Mark Dilger wrote: Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the two obvious choices. I preferred "configuration parameter" originally and was argued out of it. My take on "setting" was also that it more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter "work_mem" has the setting "8192". "The current setting of the work_mem parameter is 8192." I think something based on "parameter" is good. We also use that language in the protocol (e.g., ParameterStatus).
Re: Granting SET and ALTER SYSTE privileges for GUCs
> I remain of the opinion that this > patch should not concern itself with that, though. So you are saying that people can add new object types to PG with DAC permissions and not concern themselves with MAC capable hooks? Is that an official PG community stance?
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/16/22 16:53, Tom Lane wrote: >> Personally I don't have problem with the use of SETTING. I think the >> meaning is pretty plain in context and unlikely to produce any confusion. > I'm just unhappy about the disconnect with the documentation. I wonder > if we could get away with s/configuration parameter/setting/g in the docs. > > I don't think we need to fix that here though. If you can live with SETTING for now I will undertake to fix the docs post-feature-freeze if necessary. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Andrew Dunstan writes: > On 3/16/22 14:47, Tom Lane wrote: >> I'm also fairly allergic to the way that this patch has decided to assign >> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There >> is no existing precedent for that, and I think it's going to break >> client-side code that we don't need to break. It's not coincidental that >> this forces weird changes in rules about whitespace in the has_privilege >> functions, for example; and if you think that isn't going to cause >> problems I think you are wrong. Perhaps we could just use "SET" and >> "ALTER", or "SET" and "SYSTEM"? > That's going to look weird, ISTM. This is less clear about what it's > granting. > GRANT ALTER ON SOMETHING shared_buffers TO myuser; True. I think "GRANT SET" is clear enough, and it fits with the custom of using the name of the SQL statement that the privilege allows you to invoke. (I gather from Mark's comments that Bison gave him problems with that, but maybe that can be dealt with.) But I concede that "ALTER" by itself is pretty vague. > If you don't like that maybe ALTER_SYSTEM and SET_VALUE would work, > although mostly we have avoided things like that. > How about MODIFY instead of SET VALUE and CONFIGURE instead of ALTER SYSTEM? I thought about ALTER_SYSTEM too. It's not great but maybe the best we can do. Not sure that CONFIGURE is better. > Personally I don't have problem with the use of SETTING. I think the > meaning is pretty plain in context and unlikely to produce any confusion. I'm just unhappy about the disconnect with the documentation. I wonder if we could get away with s/configuration parameter/setting/g in the docs. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/16/22 14:47, Tom Lane wrote: > Andrew Dunstan writes: >> Generally I think this is now in fairly good shape, I've played with it >> and it seems to do what I expect in every case, and the things I found >> surprising are gone. > Stepping back a bit ... do we really want to institutionalize the > term "setting" for GUC variables? I realize that the view pg_settings > exists, but the documentation generally prefers the term "configuration > parameters". Where config.sgml uses "setting" as a noun, it's usually > talking about a specific concrete value for a parameter, and you can > argue that the view's name comports with that meaning. But you can't > GRANT a parameter's current value. > > I don't have a better name to offer offhand --- I surely am not proposing > that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x", > because that's way too wordy. But now is the time to bikeshed if we're > gonna bikeshed, or else admit that we're not interested in precise > vocabulary. > > I'm also fairly allergic to the way that this patch has decided to assign > multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There > is no existing precedent for that, and I think it's going to break > client-side code that we don't need to break. It's not coincidental that > this forces weird changes in rules about whitespace in the has_privilege > functions, for example; and if you think that isn't going to cause > problems I think you are wrong. Perhaps we could just use "SET" and > "ALTER", or "SET" and "SYSTEM"? That's going to look weird, ISTM. This is less clear about what it's granting. GRANT ALTER ON SOMETHING shared_buffers TO myuser; If you don't like that maybe ALTER_SYSTEM and SET_VALUE would work, although mostly we have avoided things like that. How about MODIFY instead of SET VALUE and CONFIGURE instead of ALTER SYSTEM? Personally I don't have problem with the use of SETTING. I think the meaning is pretty plain in context and unlikely to produce any confusion. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Joshua Brindle writes: > On Wed, Mar 16, 2022 at 3:06 PM Tom Lane wrote: >> It's going to be hard to do anything useful in a hook that (a) does >> not know which GUC is being assigned to and (b) cannot do catalog >> accesses for fear that we're not inside a transaction. (b), in >> particular, seems like a rather thorough API break; up to now >> ObjectPostAlter hooks could assume that catalog accesses are OK. > Can you elaborate on this point? Hmm, I glossed over too many details there perhaps. I was thinking about the restrictions on GUC check_hooks, which can be run outside a transaction altogether. But that's not quite relevant here. ExecSetVariableStmt can assume it's inside a transaction, but what it *can't* assume is that we've set a transaction snapshot as yet (cf. PlannedStmtRequiresSnapshot). If we call an ObjectPostAlter hook there, and it does a catalog access, that's going to break things for modifications of GUCs that are supposed to be modifiable without freezing the transaction snapshot. So the net result is the same: catalog access not okay, at least not in general. Between that and the fact that an OID-based API is largely useless here, I don't think it's sane to try to use the existing ObjectPostAlter API. Maybe there is a case for inventing a separate hook API that could pass the GUC name as a string, instead. I remain of the opinion that this patch should not concern itself with that, though. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Mar 16, 2022 at 3:06 PM Tom Lane wrote:
>
> Mark Dilger writes:
> > On Mar 16, 2022, at 11:47 AM, Tom Lane wrote:
> >> ... I therefore judge the
> >> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
> >> to be 100% useless, in fact probably counterproductive because they
> >> introduce a boatload of worries about whether the right things happen
> >> if the hook errors out or does something guc.c isn't expecting.
>
> > I think Joshua was planning to use these hooks for security purposes. The
> > hooks are supposed to check whether the Oid is valid, and if not, still be
> > able to make choices based on the other information. Joshua, any comment
> > on this?
>
> It's going to be hard to do anything useful in a hook that (a) does
> not know which GUC is being assigned to and (b) cannot do catalog
> accesses for fear that we're not inside a transaction. (b), in
> particular, seems like a rather thorough API break; up to now
> ObjectPostAlter hooks could assume that catalog accesses are OK.
>
Can you elaborate on this point? This is perhaps an area where I don't
know the rules of the road, to test this hook I modified the set_user
extension, which normally uses the parse tree to figure out if someone
is trying to SET log_statement or ALTER SYSTEM log_statement and
replaced it with:
case OAT_POST_ALTER:
if (classId == SettingAclRelationId)
{
ObjectAddress object;
object.classId = SettingAclRelationId;
object.objectId = objectId;
object.objectSubId = 0;
if (strcmp(getObjectIdentity(&object),
"log_statement") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Setting %s blocked",
getObjectIdentity(&object;
}
}
Is that inherently unsafe?
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > On Mar 16, 2022, at 11:47 AM, Tom Lane wrote: >> ... I therefore judge the >> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile >> to be 100% useless, in fact probably counterproductive because they >> introduce a boatload of worries about whether the right things happen >> if the hook errors out or does something guc.c isn't expecting. > I think Joshua was planning to use these hooks for security purposes. The > hooks are supposed to check whether the Oid is valid, and if not, still be > able to make choices based on the other information. Joshua, any comment on > this? It's going to be hard to do anything useful in a hook that (a) does not know which GUC is being assigned to and (b) cannot do catalog accesses for fear that we're not inside a transaction. (b), in particular, seems like a rather thorough API break; up to now ObjectPostAlter hooks could assume that catalog accesses are OK. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> I suggest that what might be saner is to consider that the "objects" > that the hook calls are concerned with are the pg_setting_acl entries, > not the underlying GUCs, and thus that the hooks need be invoked only > when creating, destroying or altering those entries. If we do have > a need for a hook editorializing on GUC value settings, that would > have to be an independent API --- but I have heard no calls for > the ability to have such a hook, and I don't think that this patch > is the place to introduce one. I requested it here: https://www.postgresql.org/message-id/CAGB%2BVh5pVFAqw8YzeXy4xxmEt_4Hq_8pEUHdCQvv3mCjvC-S-w%40mail.gmail.com with compromises here: https://www.postgresql.org/message-id/CAGB%2BVh6wLJ3FKsno62fi54pfg0FDrZRWcpuuCJBkHcCj-G1ndw%40mail.gmail.com https://www.postgresql.org/message-id/0A3D3CBA-6548-4C9E-9F46-59D5C51A1F31%40enterprisedb.com https://www.postgresql.org/message-id/CAGB%2BVh65R5vKC4rEt7r2_pK3kMZd-VY0n99RJwcP8Bic7xvOxQ%40mail.gmail.com and the new version here: https://www.postgresql.org/message-id/C8DF19D8-C15D-4C2D-91CA-391390F1E421%40enterprisedb.com which I wrote. a toy module to test and was satisfied with it, despite the limitations. If we are adding a DAC Grant for a type of object it seems untenable that it would not come with analogous MAC capable hooks. Thank you.
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 16, 2022, at 11:47 AM, Tom Lane wrote: > > Stepping back a bit ... do we really want to institutionalize the > term "setting" for GUC variables? I realize that the view pg_settings > exists, but the documentation generally prefers the term "configuration > parameters". Where config.sgml uses "setting" as a noun, it's usually > talking about a specific concrete value for a parameter, and you can > argue that the view's name comports with that meaning. But you can't > GRANT a parameter's current value. > > I don't have a better name to offer offhand --- I surely am not proposing > that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x", > because that's way too wordy. But now is the time to bikeshed if we're > gonna bikeshed, or else admit that we're not interested in precise > vocabulary. Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the two obvious choices. I preferred "configuration parameter" originally and was argued out of it. My take on "setting" was also that it more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter "work_mem" has the setting "8192". I'm happy to change the patch along these lines, but I vaguely recall it being Robert who liked the "setting" language. Robert? (Sorry if I misremember that...) > I'm also fairly allergic to the way that this patch has decided to assign > multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There > is no existing precedent for that, and I think it's going to break > client-side code that we don't need to break. It's not coincidental that > this forces weird changes in rules about whitespace in the has_privilege > functions, for example; and if you think that isn't going to cause > problems I think you are wrong. Perhaps we could just use "SET" and > "ALTER", or "SET" and "SYSTEM"? I chose those particular multi-word names to avoid needing to promote any keywords. That's been a while ago, and I don't recall exactly where the shift-reduce or reduce-reduce errors were coming from. I'll play with it to see what I can find. > I also agree with the upthread criticism that this is abusing the > ObjectPostAlterHook API unreasonably much. In particular, it's > trying to use pg_setting_acl OIDs as identities for GUCs, which > they are not, first because they're unstable and second because > most GUCs won't even have an entry there. I therefore judge the > hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile > to be 100% useless, in fact probably counterproductive because they > introduce a boatload of worries about whether the right things happen > if the hook errors out or does something guc.c isn't expecting. I think Joshua was planning to use these hooks for security purposes. The hooks are supposed to check whether the Oid is valid, and if not, still be able to make choices based on the other information. Joshua, any comment on this? > I suggest that what might be saner is to consider that the "objects" > that the hook calls are concerned with are the pg_setting_acl entries, > not the underlying GUCs, and thus that the hooks need be invoked only > when creating, destroying or altering those entries. That's certainly a thing we could do, but I got the impression that Joshua wanted to hook into SET, RESET, and ALTER SYSTEM SET, not merely into GRANT and REVOKE. > If we do have > a need for a hook editorializing on GUC value settings, that would > have to be an independent API --- but I have heard no calls for > the ability to have such a hook, and I don't think that this patch > is the place to introduce one. Well, the call for it was in this thread, but I'm ok with yanking out that part of the patch if it seems half-baked. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Andrew Dunstan writes: > Generally I think this is now in fairly good shape, I've played with it > and it seems to do what I expect in every case, and the things I found > surprising are gone. Stepping back a bit ... do we really want to institutionalize the term "setting" for GUC variables? I realize that the view pg_settings exists, but the documentation generally prefers the term "configuration parameters". Where config.sgml uses "setting" as a noun, it's usually talking about a specific concrete value for a parameter, and you can argue that the view's name comports with that meaning. But you can't GRANT a parameter's current value. I don't have a better name to offer offhand --- I surely am not proposing that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x", because that's way too wordy. But now is the time to bikeshed if we're gonna bikeshed, or else admit that we're not interested in precise vocabulary. I'm also fairly allergic to the way that this patch has decided to assign multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There is no existing precedent for that, and I think it's going to break client-side code that we don't need to break. It's not coincidental that this forces weird changes in rules about whitespace in the has_privilege functions, for example; and if you think that isn't going to cause problems I think you are wrong. Perhaps we could just use "SET" and "ALTER", or "SET" and "SYSTEM"? I also agree with the upthread criticism that this is abusing the ObjectPostAlterHook API unreasonably much. In particular, it's trying to use pg_setting_acl OIDs as identities for GUCs, which they are not, first because they're unstable and second because most GUCs won't even have an entry there. I therefore judge the hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile to be 100% useless, in fact probably counterproductive because they introduce a boatload of worries about whether the right things happen if the hook errors out or does something guc.c isn't expecting. I suggest that what might be saner is to consider that the "objects" that the hook calls are concerned with are the pg_setting_acl entries, not the underlying GUCs, and thus that the hooks need be invoked only when creating, destroying or altering those entries. If we do have a need for a hook editorializing on GUC value settings, that would have to be an independent API --- but I have heard no calls for the ability to have such a hook, and I don't think that this patch is the place to introduce one. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 3/15/22 16:59, Mark Dilger wrote: >> On Mar 6, 2022, at 3:27 PM, Tom Lane wrote: >> >> Mark Dilger writes: >>> The existing patch allows grants on unknown gucs, because it can't know >>> what guc an upgrade script will introduce, and the grant statement may need >>> to execute before the guc exists. >> Yeah, that's the problematic case. It might mostly work to assume that >> an unknown GUC has an empty default ACL. This could fail to retain the >> default PUBLIC SET permission if it later turns out the GUC is USERSET > On further reflection, I concluded this isn't needed. No current extension, > whether in-core or third party, expects to be able to create a new GUC and > then grant or revoke permissions on it. They can already specify the guc > context (PGC_USERS, etc). Introducing a feature that depends on the dubious > assumption that unrecognized GUCs will turn out to be USERSET doesn't seem > warranted. Agreed. > > The patch attributes all grants of setting privileges to the bootstrap > superuser. Only superusers can grant or revoke privileges on settings, and > all settings are implicitly owned by the bootstrap superuser because there is > no explicit owner associated with settings. Consequently, > select_best_grantor(some_superuser, ..., BOOTSTRAP_SUPERUSERID, ...) always > chooses the bootstrap superuser. I don't see a problem with this, but > wouldn't mind a second opinion. Some people might find it surprising when > viewing the pg_setting_acl.setacl field. I think it's OK as long as we document it. An alternative might be to invent a pseudo-superuser called, say, 'postgres_system', but that seems like overkill to solve what is in effect a cosmetic problem. Generally I think this is now in fairly good shape, I've played with it and it seems to do what I expect in every case, and the things I found surprising are gone. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > On Mar 6, 2022, at 2:57 PM, Tom Lane wrote: >> I don't think this is materially different from what we do with >> permissions on (say) functions. If you want to revoke the public >> SET privilege on some USERSET variable, you instantiate the default >> and then revoke. You end up with an empty ACL stored in pg_setting_acl, >> and voila. > I assume you mean the implementation of REVOKE does this, not that the user > needs to do both a grant and a revoke. Right. Again, look at what happens when you create a function and then revoke its default PUBLIC EXECUTE permission. >> It'd likely be necessary to refuse to record a grant/revoke on >> an unknown GUC, since if we don't know the GUC then we can't know >> what the relevant default ACL ought to be. But I bet your existing >> patch has some dubious behavior in that case too. > The existing patch allows grants on unknown gucs, because it can't know what > guc an upgrade script will introduce, and the grant statement may need to > execute before the guc exists. Yeah, that's the problematic case. It might mostly work to assume that an unknown GUC has an empty default ACL. This could fail to retain the default PUBLIC SET permission if it later turns out the GUC is USERSET ... but I suspect in most cases anybody who's messing with the permissions would've started out by revoking that anyway. We could make this definitely work in pg_dump if we teach pg_dump to explicitly grant or revoke the PUBLIC SET permission anytime it's emitting anything for a GUC, even if it thinks that would be the default state anyway. Extension scripts that want to modify permissions for their GUCs could follow that same principle to be sure. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 6, 2022, at 2:57 PM, Tom Lane wrote: > > I don't think this is materially different from what we do with > permissions on (say) functions. If you want to revoke the public > SET privilege on some USERSET variable, you instantiate the default > and then revoke. You end up with an empty ACL stored in pg_setting_acl, > and voila. I assume you mean the implementation of REVOKE does this, not that the user needs to do both a grant and a revoke. > It'd likely be necessary to refuse to record a grant/revoke on > an unknown GUC, since if we don't know the GUC then we can't know > what the relevant default ACL ought to be. But I bet your existing > patch has some dubious behavior in that case too. The existing patch allows grants on unknown gucs, because it can't know what guc an upgrade script will introduce, and the grant statement may need to execute before the guc exists. That opens a window for granting privileges on non-existent gucs. That sounds bad, but I don't know of any actual harm that it does, beyond just being ugly. With your proposal, it sounds like we could avoid that ugliness, so I'm inclined to at least try doing it as you propose. Thanks for the suggestion! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: >> On Mar 6, 2022, at 2:13 PM, Tom Lane wrote: >> ... Or, if that's our position, why are there >> per-GUC changes at all, rather than just redefining what the >> context values mean? (That is, why not redefine USERSET and >> SUSET as simply indicating the default ACL to be applied if there's >> no entry in the catalog.) > To my knowledge, there is no mechanism to revoke an implicit privilege. You > can revoke a privilege explicitly listed in an aclitem[], but only if the > privilege is being tracked that way. So? What I'm suggesting is along the lines of (1) pg_setting_acl starts out empty, or at least mostly empty (maybe there are a few GUCs that need custom values). (2) If there's a pg_setting_acl entry for a GUC that's to be set, we apply it: either it grants the desired permission or it doesn't. (3) If there's no entry, then for a USERSET GUC we assume that the entry would be like "=s/postgres", while for any other context value we assume the ACL grants nothing. I don't think this is materially different from what we do with permissions on (say) functions. If you want to revoke the public SET privilege on some USERSET variable, you instantiate the default and then revoke. You end up with an empty ACL stored in pg_setting_acl, and voila. It'd likely be necessary to refuse to record a grant/revoke on an unknown GUC, since if we don't know the GUC then we can't know what the relevant default ACL ought to be. But I bet your existing patch has some dubious behavior in that case too. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Mar 6, 2022, at 2:13 PM, Tom Lane wrote: > > 1. If we need to change these two contrib modules, doesn't that imply > a lot of changes forced on external modules as well? What are the > security implications if somebody doesn't make such a change? > > 2. It looks to me like if someone installs the updated postgres_fdw.so, > but doesn't run ALTER EXTENSION UPDATE, they are going to be left with a > rather broken extension. This seems not good either, especially if it's > multiplied by a boatload of third-party extensions requiring updates. > > So I think some more thought is needed to see if we can't avoid > the need to touch extension modules in this patch. Or at least > avoid the need for synchronized C-level and SQL-level updates, > because that is going to create a lot of pain for end users. > > I'm also fairly distressed by the number of changes in guc.c, > mainly because I fear that that means that pending patches that > add GUC variables will be subtly incorrect/insecure if they're not > updated to account for this. Frankly, I also reject the apparent > position that we need to support forbidding users from touching > many of these GUCs. Or, if that's our position, why are there > per-GUC changes at all, rather than just redefining what the > context values mean? (That is, why not redefine USERSET and > SUSET as simply indicating the default ACL to be applied if there's > no entry in the catalog.) To my knowledge, there is no mechanism to revoke an implicit privilege. You can revoke a privilege explicitly listed in an aclitem[], but only if the privilege is being tracked that way. Userset variables are implicitly settable by any user. There was a request, off-list as I recall, to make it possible to revoke the privilege to set variables such as "work_mem". To make that possible, but not change the default behavior vis-a-vis prior releases, I upgraded most userset variables to suset with a corresponding grant to public on the variable. Sites which wish to have a more restrictive policy on such variables can revoke that privilege from public and instead issue more restrictive grants. There were a few variables where such treatment didn't seem sensible, such as ones to do with client connections, and I left them alone. I didn't insist on a defense for why any particular setting needed to be revocable in order to apply this treatment. My assumption was that sites should be allowed to determine their own security policies per setting unless there is a technical difficulty for a given setting that would make it overly burdensome to implement. It seemed more complete to do the same thing for contrib modules, so I did. If a third-party module doesn't do the equivalent operation, they would simply continue with their userset variables working as before. I don't see a specific security concern with that, though I'd be happy to consider arguments to the contrary. I believe there are also some issues with this patch relating to pg_dump/pg_restore and to pg_upgrade, which EDB is working to address. We intend to repost something soon. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Thu, Dec 16, 2021 at 12:53 PM Mark Dilger wrote: > > > > > On Dec 16, 2021, at 7:43 AM, Joshua Brindle > > wrote: > > > > Ah, I understand now. Would it be possible to pass the > > SettingAclRelationId if it exists or InvalidOid if not? > > SettingAclRelationId is always defined, so we can always pass that value. > But the settingId itself may sometimes be InvalidOid. Yes, that is what I meant. > > That way if a > > MAC implementation cares about a particular GUC it'll ensure it's in > > pg_setting_acl. > > A much cleaner solution would be to create new ObjectAccessTypes with a > corresponding new Invoke macro and Run function. Those could take setting > names, not Oids, and include additional information about whether the > operation is SET, RESET or ALTER SYSTEM, what the new value is (if any), what > kind of setting it is (bool, int, ...), etc. I don't think such a patch > would even be all that hard to write. > > What do you think? Personally, I would be happy with that, but since it's a whole new hooking method I suspect it'll be an uphill battle. That definitely seems like another patchset though, if you do submit this I will test and review. Thank you.
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Dec 16, 2021, at 7:43 AM, Joshua Brindle > wrote: > > Ah, I understand now. Would it be possible to pass the > SettingAclRelationId if it exists or InvalidOid if not? SettingAclRelationId is always defined, so we can always pass that value. But the settingId itself may sometimes be InvalidOid. > That way if a > MAC implementation cares about a particular GUC it'll ensure it's in > pg_setting_acl. A much cleaner solution would be to create new ObjectAccessTypes with a corresponding new Invoke macro and Run function. Those could take setting names, not Oids, and include additional information about whether the operation is SET, RESET or ALTER SYSTEM, what the new value is (if any), what kind of setting it is (bool, int, ...), etc. I don't think such a patch would even be all that hard to write. What do you think? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Dec 15, 2021 at 1:18 PM Mark Dilger wrote: > > > On Dec 15, 2021, at 10:02 AM, Joshua Brindle > > wrote: > > > > Ah, I was actually requesting a hook where the acl check was done for > > setting a GUC, such that we could deny setting them in a hook, > > something that would be useful for the set_user extension > > (github.com/pgaudit/set_user) > > Hmm, this seems orthogonal to the patch under discussion. This patch only > adds a pg_setting_acl_aclcheck in ExecSetVariableStmt() for settings which > have been explicitly granted, otherwise it works the traditional way > (checking whether the setting is suset/userset). I don't think you'd get MAC > support without finding a way to fire the hook for all settings, regardless > of their presence in the new pg_setting_acl table. That is hard, because > InvokeObjectPostAlterHook expects the classId (SettingAclRelationId) and the > objectId (pg_setting_acl.oid), but you don't have those for many (most?) > settings. As discussed upthread, we *do not* want to force an entry into the > table for all settings, only for ones that have been explicitly granted. > > Do you agree? I'm happy to support MAC in this patch if can explain a simple > way of doing so. Ah, I understand now. Would it be possible to pass the SettingAclRelationId if it exists or InvalidOid if not? That way if a MAC implementation cares about a particular GUC it'll ensure it's in pg_setting_acl. I don't know if others will object to that but it seems like an okay compromise. > > but having a hook for grant/revoke is > > also helpful. > > Yes, I see no reason to rip this out. >
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Dec 15, 2021, at 10:02 AM, Joshua Brindle > wrote: > > Ah, I was actually requesting a hook where the acl check was done for > setting a GUC, such that we could deny setting them in a hook, > something that would be useful for the set_user extension > (github.com/pgaudit/set_user) Hmm, this seems orthogonal to the patch under discussion. This patch only adds a pg_setting_acl_aclcheck in ExecSetVariableStmt() for settings which have been explicitly granted, otherwise it works the traditional way (checking whether the setting is suset/userset). I don't think you'd get MAC support without finding a way to fire the hook for all settings, regardless of their presence in the new pg_setting_acl table. That is hard, because InvokeObjectPostAlterHook expects the classId (SettingAclRelationId) and the objectId (pg_setting_acl.oid), but you don't have those for many (most?) settings. As discussed upthread, we *do not* want to force an entry into the table for all settings, only for ones that have been explicitly granted. Do you agree? I'm happy to support MAC in this patch if can explain a simple way of doing so. > but having a hook for grant/revoke is > also helpful. Yes, I see no reason to rip this out. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Dec 15, 2021 at 10:56 AM Mark Dilger wrote: > > > > > On Dec 14, 2021, at 2:26 PM, Joshua Brindle > > wrote: > > > > currently there is a failure in check-world (not sure if it's known): > > That one is definitely my fault. 'en_US.UTF-8' exists on my platform, so I > hadn't noticed. I've changed it to use 'C', which should be portable. > > > One thing that seems like an omission to me is the absence of a > > InvokeObjectPostAlterHook in pg_setting_acl_aclcheck or > > pg_setting_acl_aclmask so that MAC extensions can also block this, > > InvokeObjectPostCreateHook is already in the create path so a > > PostAlter hook seems appropriate. > > Good catch, but that seems like a strange place to put a PostAlterHook, so I > added it to ExecGrant_Setting for v6, instead. This seems more consistent > with the hook in SetDefaultACL. Ah, I was actually requesting a hook where the acl check was done for setting a GUC, such that we could deny setting them in a hook, something that would be useful for the set_user extension (github.com/pgaudit/set_user) but having a hook for grant/revoke is also helpful. > (If you are really trying to do Managed Access Control (MAC), wouldn't that > be a separate patch which adds security hooks into all *_aclcheck functions?) MAC is mandatory access controls, so something that can be layered on top of DAC and can only be enforced even on object owners and superuser. sepgsql is a MAC extension, for example. It uses the object access hooks to enforce SELinux policy on PG objects.
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Mon, Dec 13, 2021 at 5:34 PM Mark Dilger wrote: > > > > > On Dec 13, 2021, at 1:33 PM, Mark Dilger > > wrote: > > > > but will repost in a few hours > > ... and here it is: currently there is a failure in check-world (not sure if it's known): diff -U3 /src/postgres/src/test/regress/expected/guc_privs.out /src/postgres/src/test/regress/results/guc_privs.out --- /src/postgres/src/test/regress/expected/guc_privs.out 2021-12-14 14:11:45.0 + +++ /src/postgres/src/test/regress/results/guc_privs.out 2021-12-14 15:50:52.219583421 + @@ -39,8 +39,10 @@ ALTER SYSTEM RESET autovacuum; -- ok -- PGC_SUSET SET lc_messages = 'en_US.UTF-8'; -- ok +ERROR: invalid value for parameter "lc_messages": "en_US.UTF-8" RESET lc_messages; -- ok ALTER SYSTEM SET lc_messages = 'en_US.UTF-8'; -- ok +ERROR: invalid value for parameter "lc_messages": "en_US.UTF-8" ALTER SYSTEM RESET lc_messages; -- ok -- PGC_SU_BACKEND SET jit_debugging_support = OFF; -- fail, cannot be set after connection start Aside from that I've tested this and it seems to function as advertised and in my view is worth adding to PG. One thing that seems like an omission to me is the absence of a InvokeObjectPostAlterHook in pg_setting_acl_aclcheck or pg_setting_acl_aclmask so that MAC extensions can also block this, InvokeObjectPostCreateHook is already in the create path so a PostAlter hook seems appropriate. Thank you.
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Dec 13, 2021, at 12:56 PM, Andrew Dunstan wrote:
>
> This patch had bit-rotted slightly, and I was attempting to remedy it.
I have that already, and getting ready to post. Give me a few minutes and I'll
repost.
> However, I got a failure running the TAP tests because of this change:
>
>
> diff --git a/src/test/modules/test_pg_dump/t/001_base.pl
> b/src/test/modules/test_pg_dump/t/001_base.pl
> index 16f7610883..7fbf2d871b 100644
> --- a/src/test/modules/test_pg_dump/t/001_base.pl
> +++ b/src/test/modules/test_pg_dump/t/001_base.pl
> @@ -9,7 +9,12 @@ use PostgreSQL::Test::Cluster;
> use PostgreSQL::Test::Utils;
> use Test::More;
>
> -my $tempdir = PostgreSQL::Test::Utils::tempdir;
> +# my $tempdir = PostgreSQL::Test::Utils::tempdir;
> +my $tempbase = '/tmp/test_pg_dump';
> +my $subdir = 0;
> +$subdir++ while (-e "$tempbase/$subdir");
> +my $tempdir = "$tempbase/$subdir";
> +system("mkdir $tempdir");
>
>
>
> What's going on here?
Yeah, I hit that, too. That was an accidentally committed bit of local
testing. Please ignore it for now.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/23/21 21:14, Mark Dilger wrote:
>
>> On Nov 23, 2021, at 8:07 AM, Robert Haas wrote:
>>
>> It's my impression that information_schema is a child of the SQL
>> standard, and that inventions specific to PG go in pg_catalog.
>>
>> Also, I think the user-facing name for GUCs is "settings".
> Thanks. These issues should be fixed in v4.
>
> Along the way, I also added has_setting_privilege() functions overlooked in
> v3 and before.
This patch had bit-rotted slightly, and I was attempting to remedy it.
However, I got a failure running the TAP tests because of this change:
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl
b/src/test/modules/test_pg_dump/t/001_base.pl
index 16f7610883..7fbf2d871b 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -9,7 +9,12 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
-my $tempdir = PostgreSQL::Test::Utils::tempdir;
+# my $tempdir = PostgreSQL::Test::Utils::tempdir;
+my $tempbase = '/tmp/test_pg_dump';
+my $subdir = 0;
+$subdir++ while (-e "$tempbase/$subdir");
+my $tempdir = "$tempbase/$subdir";
+system("mkdir $tempdir");
What's going on here?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Mon, Nov 22, 2021 at 7:21 PM Mark Dilger wrote: > There is a new information_schema.guc_privileges view, not present in v2. It's my impression that information_schema is a child of the SQL standard, and that inventions specific to PG go in pg_catalog. Also, I think the user-facing name for GUCs is "settings". -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 7:03 AM, Robert Haas wrote: > > It's also going to be important to think about what happens with > extension GUCs. If somebody installs an extension, we can't ask them > to perform a manual step in order to be able to grant privileges. And > if somebody then loads up a different .so for that extension, the set > of GUCs that it provides can change without any DDL being executed. > New GUCs could appear, and old GUCs could vanish. The v3 patch allows grants on unrecognized guc names. This should allow a grant statement to precede the loading of a new .so which provides the named guc. > instead just adjust the GRANT command to automatically insert a new > row into the relevant catalog if there isn't one already. That seems > nicer for extensions, and also nicer for core GUCs, since it avoids > bloating the catalog with a bunch of entries that aren't needed. Grants on GUCs create a new catalog entry if necessary, or update the existing catalog entry if found. There is a new information_schema.guc_privileges view, not present in v2. v3-0001-Allow-GRANT-of-SET-and-ALTER-SYSTEM-for-variables.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/17/21 12:12, Mark Dilger wrote: > >> On Nov 17, 2021, at 9:06 AM, Andrew Dunstan wrote: >> >> I agree it's not ideal. At the time I suggested a more flexible approach >> I hadn't really thought about the problems of upgrading. If you can come >> up with something that works there then I'll be all ears. > Are you talking about upgrades preserving revocations of privileges on gucs > from predefined roles, or merely preserving grants and revocation of > privileges on gucs to regular roles? I think the former problem is easily > handled by not shipping any predefined roles with such privileges. The > latter problem would seem to be a mere matter of programming, something I'm > working on but don't have finished. (But maybe you see dragons ahead for me > that I'm not seeing yet?) > Yes, if we don't ship with any preset privileges then the problem I was thinking about disappears. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 17, 2021, at 9:06 AM, Andrew Dunstan wrote: > > I agree it's not ideal. At the time I suggested a more flexible approach > I hadn't really thought about the problems of upgrading. If you can come > up with something that works there then I'll be all ears. Are you talking about upgrades preserving revocations of privileges on gucs from predefined roles, or merely preserving grants and revocation of privileges on gucs to regular roles? I think the former problem is easily handled by not shipping any predefined roles with such privileges. The latter problem would seem to be a mere matter of programming, something I'm working on but don't have finished. (But maybe you see dragons ahead for me that I'm not seeing yet?) — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/17/21 11:07, Mark Dilger wrote: > >> On Nov 17, 2021, at 6:31 AM, Andrew Dunstan wrote: >> >> Well, I was trying (perhaps not very well) to imagine how to deal with >> someone modifying the permissions of one of the predefined roles. Say >> pg_foo has initial permission to set bar and baz, and the DBA removes >> permission to set baz. How is pg_dump going to emit the right commands >> to allow a safe pg_upgrade? Maybe we should say that the permissions for >> the predefined roles are immutable, so only permissions sets for user >> defined roles are mutable. > I find this somewhat amusing. When you suggested off-list that I make gucs > individually grantable rather than creating predefined roles with privileges, > that was a great idea precisely because sites could define their own security > policies using their own site-defined roles: > > CREATE ROLE admin_type_a NOLOGIN NOSUPERUSER; > CREATE ROLE admin_type_b NOLOGIN NOSUPERUSER; > ... > GRANT ALTER SYSTEM ON guc_a1, guc_a2, guc_a3, ... TO admin_type_a; > GRANT ALTER SYSTEM ON guc_b1, guc_b2, guc_b3, ... TO admin_type_b; > ... > > That has all the power of a system based on predefined roles, but with > site-specific flexibility, which is better. So it amuses me that we'd now be > talking about granting some of these to predefined roles, as that is a > regression in flexibility. (How would a site revoke it from one of those > predefined roles if they wanted a different policy?) > I agree it's not ideal. At the time I suggested a more flexible approach I hadn't really thought about the problems of upgrading. If you can come up with something that works there then I'll be all ears. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Wed, Nov 17, 2021 at 9:31 AM Andrew Dunstan wrote: > Well, I was trying (perhaps not very well) to imagine how to deal with > someone modifying the permissions of one of the predefined roles. Say > pg_foo has initial permission to set bar and baz, and the DBA removes > permission to set baz. How is pg_dump going to emit the right commands > to allow a safe pg_upgrade? Maybe we should say that the permissions for > the predefined roles are immutable, so only permissions sets for user > defined roles are mutable. That's a great question, but it isn't a new problem. If I create a brand new database and do thIs: rhaas=# revoke execute on function pg_ls_waldir() from pg_monitor; REVOKE And then I do this: [rhaas pgsql]$ pg_dump Then the output includes this: REVOKE ALL ON FUNCTION pg_catalog.pg_ls_waldir(OUT name text, OUT size bigint, OUT modification timestamp with time zone) FROM pg_monitor; I recommend looking at how that works and making this work the same way. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 17, 2021, at 6:31 AM, Andrew Dunstan wrote: > > Well, I was trying (perhaps not very well) to imagine how to deal with > someone modifying the permissions of one of the predefined roles. Say > pg_foo has initial permission to set bar and baz, and the DBA removes > permission to set baz. How is pg_dump going to emit the right commands > to allow a safe pg_upgrade? Maybe we should say that the permissions for > the predefined roles are immutable, so only permissions sets for user > defined roles are mutable. I find this somewhat amusing. When you suggested off-list that I make gucs individually grantable rather than creating predefined roles with privileges, that was a great idea precisely because sites could define their own security policies using their own site-defined roles: CREATE ROLE admin_type_a NOLOGIN NOSUPERUSER; CREATE ROLE admin_type_b NOLOGIN NOSUPERUSER; ... GRANT ALTER SYSTEM ON guc_a1, guc_a2, guc_a3, ... TO admin_type_a; GRANT ALTER SYSTEM ON guc_b1, guc_b2, guc_b3, ... TO admin_type_b; ... That has all the power of a system based on predefined roles, but with site-specific flexibility, which is better. So it amuses me that we'd now be talking about granting some of these to predefined roles, as that is a regression in flexibility. (How would a site revoke it from one of those predefined roles if they wanted a different policy?) — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 17, 2021, at 5:32 AM, Robert Haas wrote: > >> I was aware of that, but figured not all GUCs have to be grantable. If it >> doesn't fit in a NameData, you can't grant on it. > > Such restrictions are rather counterintuitive for users, and here it > doesn't even buy anything. Using 'text' rather than 'name' as the data > type isn't going to cost any meaningful amount of performance. That sounds fine. >> If we want to be more accommodating than that, we can store it as text, just >> like pg_db_role_names does, but then we need more code complexity to look it >> up and to verify that it is unique. (We wouldn't want multiple records for >> the same pair.) > > If you're verifying that it's unique in any way other than using a > unique index, I think you're doing it wrong. No, I'm using a unique index. I was overthinking it, concerned about changing from name_ops to text_ops and needing the toast table, but that's silly, because I need one for the acl anyway. > Also, maybe I'm confused here, but why isn't the schema: > > gucoid > gucname > gucacl It is, both in v2 already posted, and in the v3, written but not yet posted, as I haven't finished the pg_dump work, and also I'm waiting to see how this discussion gets resolved before asking for a review of v3. > IOW, I don't understand why this table has as the primary > key rather than just guc. I was responding to Tom's recommendation that I follow the pattern in pg_db_role_setting, and speculating how that would work. I was not proposing to do it that way. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/16/21 17:12, Tom Lane wrote: > >>> To support pg_dump and pg_upgrade, it might be better to have an >>> enabled/disabled flag rather than to delete rows. >> I'm not really sure what this means. > I didn't see the point of this either. We really need to KISS here. > Every bit of added complexity in the catalog representation is another > opportunity for bugs-of-omission, not to mention a detail that you > have to provide mechanisms to dump and restore. > > Well, I was trying (perhaps not very well) to imagine how to deal with someone modifying the permissions of one of the predefined roles. Say pg_foo has initial permission to set bar and baz, and the DBA removes permission to set baz. How is pg_dump going to emit the right commands to allow a safe pg_upgrade? Maybe we should say that the permissions for the predefined roles are immutable, so only permissions sets for user defined roles are mutable. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/17/21 08:32, Robert Haas wrote: > On Tue, Nov 16, 2021 at 5:45 PM Mark Dilger > wrote: >> I was aware of that, but figured not all GUCs have to be grantable. If it >> doesn't fit in a NameData, you can't grant on it. > Such restrictions are rather counterintuitive for users, and here it > doesn't even buy anything. Using 'text' rather than 'name' as the data > type isn't going to cost any meaningful amount of performance. indeed >> If we want to be more accommodating than that, we can store it as text, just >> like pg_db_role_names does, but then we need more code complexity to look it >> up and to verify that it is unique. (We wouldn't want multiple records for >> the same pair.) > If you're verifying that it's unique in any way other than using a > unique index, I think you're doing it wrong. yeah > > Also, maybe I'm confused here, but why isn't the schema: > > gucoid > gucname > gucacl > > IOW, I don't understand why this table has as the primary > key rather than just guc. Everywhere else, we have a single ACL array > for the all privileges on an object. Why wouldn't we do the same thing > here? > Yes, that should work, It seems like a better scheme. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Nov 16, 2021 at 5:45 PM Mark Dilger wrote: > I was aware of that, but figured not all GUCs have to be grantable. If it > doesn't fit in a NameData, you can't grant on it. Such restrictions are rather counterintuitive for users, and here it doesn't even buy anything. Using 'text' rather than 'name' as the data type isn't going to cost any meaningful amount of performance. > If we want to be more accommodating than that, we can store it as text, just > like pg_db_role_names does, but then we need more code complexity to look it > up and to verify that it is unique. (We wouldn't want multiple records for > the same pair.) If you're verifying that it's unique in any way other than using a unique index, I think you're doing it wrong. Also, maybe I'm confused here, but why isn't the schema: gucoid gucname gucacl IOW, I don't understand why this table has as the primary key rather than just guc. Everywhere else, we have a single ACL array for the all privileges on an object. Why wouldn't we do the same thing here? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Nov 16, 2021 at 3:38 PM Andrew Dunstan wrote: > Your original and fairly simple set of patches used hardcoded role names > and sets of GUCs they could update via ALTER SYSTEM. I suggested to you > privately that a more flexible approach would be to drive this from a > catalog table. I had in mind a table of more or less . > You could prepopulate it with the roles / GUCs from your original patch > set. I don't think it needs to be initially empty. But DBAs would be > able to modify and extend the settings. I agree with Tom that we > shouldn't try to cover all GUCs in the table - any GUC without an entry > can only be updated by a superuser. I simply can't understand the point of this. You're basically proposing that somebody has to execute one SQL statement to make a GUC grantable, and then a second SQL statement to actually grant access to it. What is the value in that? It is the same person doing both things, and the system can work out automatically what needs to be done. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Nov 16, 2021 at 2:48 PM Mark Dilger wrote: > I'm preparing a new version of the patch that has the catalog empty to begin > with, and only adds values in response to GRANT commands. That also handles > the issues of extension upgrades, which I think the old patch handled better > than the discussion on this thread suggested, but no matter. The new > behavior will allow granting privileges on non-existent gucs, just as ALTER > ROLE..SET allows registering settings on non-existent gucs. > > The reason I had resisted allowing grants of privileges on non-existent gucs > is that you can get the following sort of behavior (note the last two lines): > > DROP USER regress_priv_user7; > ERROR: role "regress_priv_user7" cannot be dropped because some objects > depend on it > DETAIL: privileges for table persons2 > privileges for configuration parameter sort_mem > privileges for configuration parameter no_such_param > > Rejecting "no_such_param" in the original GRANT statement seemed cleaner to > me, but this discussion suggests pretty strongly that I can't do it that way. I think it's perfectly fine to refuse a GRANT statement on a GUC that doesn't exist, and I don't believe I ever said otherwise. What I don't think is OK is to require a preparatory statement like CREATE CONFIGURATION PARAMETER to be executed before you can grant permissions on it. There's no reason for that. If somebody tries to grant privileges on a GUC that does not exist, fail. If the GUC does exist but there's no catalog entry, make one. If the GUC exists and the catalog entry also exists, update it. At REVOKE time, don't check whether the GUC exists - only check the catalog. That way people can remove privileges on GUCs that don't exist any more. If somebody issues a REVOKE and there's no catalog entry, do nothing. If somebody issues a REVOKE and there is a catalog entry, remove stuff from it; but if that would leave it completely empty, instead delete it. Whenever you create a catalog entry, also add dependency entries pointing to it. When you delete one, remove those entries. > Changing the historical "sort_mem" to the canonical "work_mem" name also > seems better to me, as otherwise you could have different grants on the same > GUC under different names. I'm inclined to keep the canonicalization of > names where known, but maybe that runs afoul the rule that these grants > should not be tied very hard to the GUC? No. If somebody grants privileges on an old name, record the grant under the canonical name. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 2:12 PM, Tom Lane wrote: > > BTW, another objection to pg_config_param as designed here is that > a "name" is not an appropriate way to store possibly-qualified > custom GUC names. It's not long enough (cf. valid_custom_variable_name). I was aware of that, but figured not all GUCs have to be grantable. If it doesn't fit in a NameData, you can't grant on it. If we want to be more accommodating than that, we can store it as text, just like pg_db_role_names does, but then we need more code complexity to look it up and to verify that it is unique. (We wouldn't want multiple records for the same pair.) — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 2:12 PM, Tom Lane wrote: > > The question is why you need pg_config_param at all, then. > AFAICS it just adds maintenance complexity we could do without. > I think we'd be better off with a catalog modeled on the design of > pg_db_role_setting, which would have entries for roles and lists > of GUC names that those roles could set. Originally, I was trying to have dependency linkage between two proper types of objects, so that DROP ROLE and DROP CONFIGURATION PARAMETER would behave as expected, complaining about privileges remaining rather than dropping an object and leaving a dangling reference. I've deleted the whole CREATE CONFIGURATION PARAMETER and DROP CONFIGURATION PARAMETER syntax and implementation, but it still seems odd to me that: CREATE ROLE somebody; GRANT SELECT ON TABLE sometable TO ROLE somebody; GRANT ALTER SYSTEM ON someguc TO ROLE somebody; DROP ROLE somebody; ERROR: role "somebody" cannot be dropped because some objects depend on it DETAIL: privileges for table sometable would not mention privileges for "someguc" as well. That's why I want configuration parameters to be proper objects with OIDs and with entries in pg_depend and/or pg_shdepend. Maybe there is some better way to do it, but that's why I've been doing this. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > The patch already posted on this thread already works that way. Robert and > Tom seemed to infer that all gucs need to be present in the catalog, but in > fact, not entering them in the catalog simply means that they aren't > grantable. I think the confusion arose from the fact that I created a > mechanism for extension authors to enter additional gucs into the catalog, > which gave the false impression that such entries were required. They are > not. I didn't bother explaining that before, because Robert and Tom both > seem to expect all gucs to be grantable, an expectation you do not appear to > share. The question is why you need pg_config_param at all, then. AFAICS it just adds maintenance complexity we could do without. I think we'd be better off with a catalog modeled on the design of pg_db_role_setting, which would have entries for roles and lists of GUC names that those roles could set. BTW, another objection to pg_config_param as designed here is that a "name" is not an appropriate way to store possibly-qualified custom GUC names. It's not long enough (cf. valid_custom_variable_name). >> To support pg_dump and pg_upgrade, it might be better to have an >> enabled/disabled flag rather than to delete rows. > I'm not really sure what this means. I didn't see the point of this either. We really need to KISS here. Every bit of added complexity in the catalog representation is another opportunity for bugs-of-omission, not to mention a detail that you have to provide mechanisms to dump and restore. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 12:38 PM, Andrew Dunstan wrote: > > Your original and fairly simple set of patches used hardcoded role names > and sets of GUCs they could update via ALTER SYSTEM. I suggested to you > privately that a more flexible approach would be to drive this from a > catalog table. I had in mind a table of more or less . Right. I prefer a table of matching the structure of most of the rest of the grantable objects, so that entries from pg_depend or pg_shdepend can track the dependencies in the usual way and prevent dangling references when DROP ROLE is executed. Is there a reason to prefer an ad hoc tables structure? > You could prepopulate it with the roles / GUCs from your original patch > set. I don't think it needs to be initially empty. But DBAs would be > able to modify and extend the settings. I agree with Tom that we > shouldn't try to cover all GUCs in the table - any GUC without an entry > can only be updated by a superuser. The patch already posted on this thread already works that way. Robert and Tom seemed to infer that all gucs need to be present in the catalog, but in fact, not entering them in the catalog simply means that they aren't grantable. I think the confusion arose from the fact that I created a mechanism for extension authors to enter additional gucs into the catalog, which gave the false impression that such entries were required. They are not. I didn't bother explaining that before, because Robert and Tom both seem to expect all gucs to be grantable, an expectation you do not appear to share. > To support pg_dump and pg_upgrade, it might be better to have an > enabled/disabled flag rather than to delete rows. I'm not really sure what this means. Are you suggesting that the (or in your formulation ) should have a "bool enabled" field, and when the guc gets dropped, the "enabled" field gets set to false? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On 11/16/21 14:48, Mark Dilger wrote: > >> On Nov 16, 2021, at 8:44 AM, Tom Lane wrote: >> >> My concern is not about performance, it's about the difficulty of >> maintaining a catalog that expects to be a more-or-less exhaustive >> list of GUCs. I think you need to get rid of that expectation. > I'm preparing a new version of the patch that has the catalog empty to begin > with, and only adds values in response to GRANT commands. That also handles > the issues of extension upgrades, which I think the old patch handled better > than the discussion on this thread suggested, but no matter. The new > behavior will allow granting privileges on non-existent gucs, just as ALTER > ROLE..SET allows registering settings on non-existent gucs. Your original and fairly simple set of patches used hardcoded role names and sets of GUCs they could update via ALTER SYSTEM. I suggested to you privately that a more flexible approach would be to drive this from a catalog table. I had in mind a table of more or less . You could prepopulate it with the roles / GUCs from your original patch set. I don't think it needs to be initially empty. But DBAs would be able to modify and extend the settings. I agree with Tom that we shouldn't try to cover all GUCs in the table - any GUC without an entry can only be updated by a superuser. To support pg_dump and pg_upgrade, it might be better to have an enabled/disabled flag rather than to delete rows. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 8:44 AM, Tom Lane wrote: > > My concern is not about performance, it's about the difficulty of > maintaining a catalog that expects to be a more-or-less exhaustive > list of GUCs. I think you need to get rid of that expectation. I'm preparing a new version of the patch that has the catalog empty to begin with, and only adds values in response to GRANT commands. That also handles the issues of extension upgrades, which I think the old patch handled better than the discussion on this thread suggested, but no matter. The new behavior will allow granting privileges on non-existent gucs, just as ALTER ROLE..SET allows registering settings on non-existent gucs. The reason I had resisted allowing grants of privileges on non-existent gucs is that you can get the following sort of behavior (note the last two lines): DROP USER regress_priv_user7; ERROR: role "regress_priv_user7" cannot be dropped because some objects depend on it DETAIL: privileges for table persons2 privileges for configuration parameter sort_mem privileges for configuration parameter no_such_param Rejecting "no_such_param" in the original GRANT statement seemed cleaner to me, but this discussion suggests pretty strongly that I can't do it that way. Changing the historical "sort_mem" to the canonical "work_mem" name also seems better to me, as otherwise you could have different grants on the same GUC under different names. I'm inclined to keep the canonicalization of names where known, but maybe that runs afoul the rule that these grants should not be tied very hard to the GUC? > In the analogy to ALTER DATABASE/USER SET, we don't expect that > pg_db_role_setting catalog entries will exist for all, or even > very many, GUCs. Also, the fact that pg_db_role_setting entries > aren't tied very hard to the actual existence of a GUC is a good > thing from the maintenance and upgrade standpoint. Doing it that way > BTW, if we did create such a catalog, there would need to be > pg_dump and pg_upgrade support for its entries, and you'd have to > think about (e.g.) whether pg_upgrade would attempt to preserve > the same OIDs. I don't see any indication that the patch has > addressed that infrastructure ... which is probably just as well, > since it's work that I'd be wanting to reject. Yeah, that's why I didn't write it. I wanted feedback on the basic implementation before doing that work. > (Hm, but actually, > doesn't pg_dump need work anyway to dump this new type of GRANT?) Yes, if the idea of this kind of grant isn't being outright rejected, then I'll need to write that. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > On Nov 16, 2021, at 7:28 AM, Tom Lane wrote: >> True; as long as the expectation is that entries will exist for only >> a tiny subset of GUCs, it's probably fine. > I understand that bloating a frequently used catalog can be pretty > harmful to performance. I wasn't aware that the size of an infrequently > used catalog was critical. My concern is not about performance, it's about the difficulty of maintaining a catalog that expects to be a more-or-less exhaustive list of GUCs. I think you need to get rid of that expectation. In the analogy to ALTER DATABASE/USER SET, we don't expect that pg_db_role_setting catalog entries will exist for all, or even very many, GUCs. Also, the fact that pg_db_role_setting entries aren't tied very hard to the actual existence of a GUC is a good thing from the maintenance and upgrade standpoint. BTW, if we did create such a catalog, there would need to be pg_dump and pg_upgrade support for its entries, and you'd have to think about (e.g.) whether pg_upgrade would attempt to preserve the same OIDs. I don't see any indication that the patch has addressed that infrastructure ... which is probably just as well, since it's work that I'd be wanting to reject. (Hm, but actually, doesn't pg_dump need work anyway to dump this new type of GRANT?) regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > You are talking about mismatches in the other direction, aren't you? I was > responding to Robert's point that new gucs could appear, and old gucs > disappear. That seems analogous to new functions appearing and old functions > disappearing. If you upgrade (not downgrade) the .so, the new gucs and > functions will be in the .so, but won't be callable/grantable from sql until > the upgrade script runs. True, but in general users might not care about getting access to new features (or at least, they might not care until much later, at which point they'd bother to run ALTER EXTENSION UPDATE). > The old gucs and functions will be missing from the .so, and attempts to call > them/grant them from sql before the upgrade will fail. What am I missing > here? Here, you are describing behavior that's against project policy and would be rejected immediately if anyone submitted a patch that made an extension do that. Newer .so versions are expected to run successfully, not fail, with an older version of their SQL objects. Were this not so, it's almost inevitable that a binary upgrade would fail, because the extension is likely to be called into action somewhere before there is any opportunity to issue ALTER EXTENSION UPDATE. Even in-place updates of extensions would become problematic: you can't assume that a rollout of new executables will be instantly followed by ALTER EXTENSION UPDATE. Basically, I think the proposed new system catalog is both unworkable and entirely unnecessary. Maybe it would have been okay if Peter had designed GUCs like that a couple of decades ago, but at this point we have a ton of infrastructure --- much of it not under the core project's control --- that assumes that GUCs can be created with just a bit of code. I do not think that this feature is worth the cost of changing that. Or IOW: ALTER SYSTEM SET has gotten along fine without such a catalog; why can't this feature? I also notice that the patch seems to intend to introduce tracking of which user "owns" a GUC, which I think is even more unworkable. They are all going to wind up owned by the bootstrap superuser (extension GUCs too), so why bother? regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Nov 16, 2021 at 10:45 AM Mark Dilger wrote: > You are talking about mismatches in the other direction, aren't you? I was > responding to Robert's point that new gucs could appear, and old gucs > disappear. That seems analogous to new functions appearing and old functions > disappearing. If you upgrade (not downgrade) the .so, the new gucs and > functions will be in the .so, but won't be callable/grantable from sql until > the upgrade script runs. The old gucs and functions will be missing from the > .so, and attempts to call them/grant them from sql before the upgrade will > fail. What am I missing here? It's true that we could impose such a restriction, but I don't think we should. If I install a different .so, I want the new GUCs to be grantable immediately, without running any separate DDL. I also don't think we should burden extension authors with putting stuff in their upgrade scripts for this. We should solve the problem in our code rather than forcing them to do so in theirs. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 7:28 AM, Tom Lane wrote: > > True; as long as the expectation is that entries will exist for only > a tiny subset of GUCs, it's probably fine. I understand that bloating a frequently used catalog can be pretty harmful to performance. I wasn't aware that the size of an infrequently used catalog was critical. This new catalog would be used during GRANT SET ... and GRANT ALTER SYSTEM commands, which should be rare, and potentially consulted when SET or ALTER SYSTEM commands are issued. Is there a more substantial performance impact to this than I'm aware? It can be a bit challenging to run performance tests on such things, given the way everything interacts with everything else. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 7:32 AM, Tom Lane wrote: > > Mark Dilger writes: >> I don't think we support using a .so that is mismatched against the >> version of the extension that is installed. > > You are entirely mistaken. That's not only "supported", it's *required*. > Consider binary upgrades, where the SQL objects are transferred as-is > but the receiving installation may have a different (hopefully newer) > version of the .so. That .so has to cope with the older SQL object > definitions; it doesn't get to assume that the upgrade script has been > run yet. You are talking about mismatches in the other direction, aren't you? I was responding to Robert's point that new gucs could appear, and old gucs disappear. That seems analogous to new functions appearing and old functions disappearing. If you upgrade (not downgrade) the .so, the new gucs and functions will be in the .so, but won't be callable/grantable from sql until the upgrade script runs. The old gucs and functions will be missing from the .so, and attempts to call them/grant them from sql before the upgrade will fail. What am I missing here? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
Mark Dilger writes: > I don't think we support using a .so that is mismatched against the > version of the extension that is installed. You are entirely mistaken. That's not only "supported", it's *required*. Consider binary upgrades, where the SQL objects are transferred as-is but the receiving installation may have a different (hopefully newer) version of the .so. That .so has to cope with the older SQL object definitions; it doesn't get to assume that the upgrade script has been run yet. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
Robert Haas writes: > On Tue, Nov 16, 2021 at 10:17 AM Tom Lane wrote: >> Right. I think that any design that involves per-GUC catalog entries >> is going to be an abysmal failure, precisely because the set of GUCs >> is not stable enough. > In practice it's pretty stable. I think it's just a matter of having a > plan that covers the cases where it isn't stable reasonably elegantly. Um. Really the point comes down to having sane default behavior when there's no entry, which ought to eliminate any need to do the sort of "run over all the entries at startup" processing that you seemed to be proposing. So I guess I don't understand why such a thing would be needed. > We already embed GUC names in catalog entries when someone runs ALTER > USER SET or ALTER DATABASE SET, so this proposal doesn't seem to be > moving the goalposts in any meaningful way. True; as long as the expectation is that entries will exist for only a tiny subset of GUCs, it's probably fine. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
> On Nov 16, 2021, at 7:03 AM, Robert Haas wrote: > > It's also going to be important to think about what happens with > extension GUCs. If somebody installs an extension, we can't ask them > to perform a manual step in order to be able to grant privileges. The burden isn't on the installer of an extension. As implemented, it's the extension's installation .sql file that sets it up, and the upgrade .sql files that make adjustments, if necessary. > And > if somebody then loads up a different .so for that extension, the set > of GUCs that it provides can change without any DDL being executed. > New GUCs could appear, and old GUCs could vanish. Well, the same is true for functions, right? If you add, remove, or redefine functions in the extension, you need an upgrade script that defines the new functions, removes the old ones, changes function signatures, or whatever. The same is true here for GUCs. I don't think we support using a .so that is mismatched against the version of the extension that is installed. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Tue, Nov 16, 2021 at 10:17 AM Tom Lane wrote: > Right. I think that any design that involves per-GUC catalog entries > is going to be an abysmal failure, precisely because the set of GUCs > is not stable enough. In practice it's pretty stable. I think it's just a matter of having a plan that covers the cases where it isn't stable reasonably elegantly. We already embed GUC names in catalog entries when someone runs ALTER USER SET or ALTER DATABASE SET, so this proposal doesn't seem to be moving the goalposts in any meaningful way. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Granting SET and ALTER SYSTE privileges for GUCs
Robert Haas writes: > It's also going to be important to think about what happens with > extension GUCs. If somebody installs an extension, we can't ask them > to perform a manual step in order to be able to grant privileges. And > if somebody then loads up a different .so for that extension, the set > of GUCs that it provides can change without any DDL being executed. > New GUCs could appear, and old GUCs could vanish. Right. I think that any design that involves per-GUC catalog entries is going to be an abysmal failure, precisely because the set of GUCs is not stable enough. So I'm suspicious of this entire proposal. Maybe there's a way to make it work, but that way isn't how. regards, tom lane
Re: Granting SET and ALTER SYSTE privileges for GUCs
On Mon, Nov 15, 2021 at 3:37 PM Mark Dilger wrote: > One disadvantage of this approach is that the GUC variables are represented > both in the list of C structures in guc.c and in the new system catalog > pg_config_param's .dat file. Failure to enter a GUC in the .dat file will > result in the inability to grant privileges on the GUC, at least unless/until > you run CREATE CONFIGURATION PARAMETER on the GUC. (This is, in fact, how > extension scripts deal with the issue.) It would perhaps be better if the > list of GUCs were not duplicated, but I wasn't clever enough to find a clean > way to do that without greatly expanding the patch (nor did I complete > prototyping any such thing.) I think this imposes an unnecessary burden on developers. It seems like it would be easy to write some code that lives inside guc.c and runs during bootstrap, and it could just iterate over each ConfigureNamesWhatever array and insert catalog entries for everything it finds. It's also going to be important to think about what happens with extension GUCs. If somebody installs an extension, we can't ask them to perform a manual step in order to be able to grant privileges. And if somebody then loads up a different .so for that extension, the set of GUCs that it provides can change without any DDL being executed. New GUCs could appear, and old GUCs could vanish. So I wonder if we should instead not do what I proposed above, and instead just adjust the GRANT command to automatically insert a new row into the relevant catalog if there isn't one already. That seems nicer for extensions, and also nicer for core GUCs, since it avoids bloating the catalog with a bunch of entries that aren't needed. -- Robert Haas EDB: http://www.enterprisedb.com
