Allow placeholders in ALTER ROLE w/o superuser
Hello hackers,
Using placeholders for application variables allows the use of RLS for
application users as shown in this blog post
https://www.2ndquadrant.com/en/blog/application-users-vs-row-level-security/
.
SET my.username = 'tomas'
CREATE POLICY chat_policy ON chat
USING (current_setting('my.username') IN (message_from, message_to))
WITH CHECK (message_from = current_setting('my.username'))
This technique has enabled postgres sidecar services(PostgREST,
PostGraphQL, etc) to keep the application security at the database level,
which has worked great.
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"
Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
services to store metadata scoped to its pertaining role.
I've attached a patch that removes this restriction. From my testing, this
doesn't affect permission checking when an extension defines its custom GUC
variables.
DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
PGC_SUSET, ..);
Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".
---
Steve Chavez
Engineering at https://supabase.com/
Attachments:
0001-Allow-placeholders-in-ALTER-ROLE-w-o-superuser.patchtext/x-patch; charset=US-ASCII; name=0001-Allow-placeholders-in-ALTER-ROLE-w-o-superuser.patchDownload+1-24
On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
services to store metadata scoped to its pertaining role.I've attached a patch that removes this restriction. From my testing, this
doesn't affect permission checking when an extension defines its custom GUC
variables.DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
PGC_SUSET, ..);Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".
IIUC you are basically proposing to revert a6dcd19 [0]/messages/by-id/4090.1258042387@sss.pgh.pa.us, but it is not clear
to me why that is safe. Am I missing something?
[0]: /messages/by-id/4090.1258042387@sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
services to store metadata scoped to its pertaining role.I've attached a patch that removes this restriction. From my testing, this
doesn't affect permission checking when an extension defines its custom GUC
variables.DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
PGC_SUSET, ..);Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".IIUC you are basically proposing to revert a6dcd19 [0], but it is not clear
to me why that is safe. Am I missing something?
I spent some more time looking into this, and I think I've constructed a
simple example that demonstrates the problem with removing this
restriction.
postgres=# CREATE ROLE test CREATEROLE;
CREATE ROLE
postgres=# CREATE ROLE other LOGIN;
CREATE ROLE
postgres=# GRANT CREATE ON DATABASE postgres TO other;
GRANT
postgres=# SET ROLE test;
SET
postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
ALTER ROLE
postgres=> \c postgres other
You are now connected to database "postgres" as user "other".
postgres=> CREATE EXTENSION plperl;
CREATE EXTENSION
postgres=> SHOW plperl.on_plperl_init;
plperl.on_plperl_init
-----------------------
test
(1 row)
In this example, the non-superuser role sets a placeholder GUC for another
role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
record of who ran ALTER ROLE, we might be able to apply appropriate
permissions checking when the module is loaded, but this information
doesn't exist in pg_db_role_setting. IIUC we have the following options:
1. Store information about who ran ALTER ROLE. I think there are a
couple of problems with this. For example, what happens if the
grantor was dropped or its privileges were altered? Should we
instead store the context of the user (i.e., PGC_USERSET or
PGC_SUSET)? Do we need to add entries to pg_depend?
2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs. Since
we don't know who ran ALTER ROLE, we can't trust the value.
3. Require superuser to use ALTER ROLE for a placeholder. This is what
we do today. Since we know a superuser set the value, we can always
apply it when the custom GUC is finally defined.
If this is an accurate representation of the options, it seems clear why
the superuser restriction is in place.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Thanks a lot for the feedback Nathan.
Taking your options into consideration, for me the correct behaviour should
be:
- The ALTER ROLE placeholder should always be stored with a PGC_USERSET
GucContext. It's a placeholder anyway, so it should be the less restrictive
one. If the user wants to define it as PGC_SUSET or other this should be
done through a custom extension.
- When an extension claims the placeholder, we should check the
DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match,
then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can
WARN or ERR about the defined role GUCs.
What do you think?
On Mon, 18 Jul 2022 at 19:03, Nathan Bossart <nathandbossart@gmail.com>
wrote:
Show quoted text
On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allowsidecar
services to store metadata scoped to its pertaining role.
I've attached a patch that removes this restriction. From my testing,
this
doesn't affect permission checking when an extension defines its custom
GUC
variables.
DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom,
NULL,
PGC_SUSET, ..);
Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".IIUC you are basically proposing to revert a6dcd19 [0], but it is not
clear
to me why that is safe. Am I missing something?
I spent some more time looking into this, and I think I've constructed a
simple example that demonstrates the problem with removing this
restriction.postgres=# CREATE ROLE test CREATEROLE;
CREATE ROLE
postgres=# CREATE ROLE other LOGIN;
CREATE ROLE
postgres=# GRANT CREATE ON DATABASE postgres TO other;
GRANT
postgres=# SET ROLE test;
SET
postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
ALTER ROLE
postgres=> \c postgres other
You are now connected to database "postgres" as user "other".
postgres=> CREATE EXTENSION plperl;
CREATE EXTENSION
postgres=> SHOW plperl.on_plperl_init;
plperl.on_plperl_init
-----------------------
test
(1 row)In this example, the non-superuser role sets a placeholder GUC for another
role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
record of who ran ALTER ROLE, we might be able to apply appropriate
permissions checking when the module is loaded, but this information
doesn't exist in pg_db_role_setting. IIUC we have the following options:1. Store information about who ran ALTER ROLE. I think there are a
couple of problems with this. For example, what happens if the
grantor was dropped or its privileges were altered? Should we
instead store the context of the user (i.e., PGC_USERSET or
PGC_SUSET)? Do we need to add entries to pg_depend?
2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs.
Since
we don't know who ran ALTER ROLE, we can't trust the value.
3. Require superuser to use ALTER ROLE for a placeholder. This is
what
we do today. Since we know a superuser set the value, we can
always
apply it when the custom GUC is finally defined.If this is an accurate representation of the options, it seems clear why
the superuser restriction is in place.--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Jul 19, 2022 at 12:55:14AM -0500, Steve Chavez wrote:
Taking your options into consideration, for me the correct behaviour should
be:- The ALTER ROLE placeholder should always be stored with a PGC_USERSET
GucContext. It's a placeholder anyway, so it should be the less restrictive
one. If the user wants to define it as PGC_SUSET or other this should be
done through a custom extension.
- When an extension claims the placeholder, we should check the
DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match,
then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can
WARN or ERR about the defined role GUCs.What do you think?
Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed
by a superuser or a role with privileges via pg_parameter_acl. Storing all
placeholder GUC settings as PGC_USERSET would make things more restrictive
than they are today. For example, it would no longer be possible to apply
any ALTER ROLE settings from superusers for placeholders that later become
custom GUCS.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
On Tue, Jul 19, 2022 at 12:55:14AM -0500, Steve Chavez wrote:
Taking your options into consideration, for me the correct behaviour should
be:- The ALTER ROLE placeholder should always be stored with a PGC_USERSET
GucContext. It's a placeholder anyway, so it should be the less restrictive
one. If the user wants to define it as PGC_SUSET or other this should be
done through a custom extension.
- When an extension claims the placeholder, we should check the
DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match,
then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can
WARN or ERR about the defined role GUCs.What do you think?
Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed
by a superuser or a role with privileges via pg_parameter_acl. Storing all
placeholder GUC settings as PGC_USERSET would make things more restrictive
than they are today. For example, it would no longer be possible to apply
any ALTER ROLE settings from superusers for placeholders that later become
custom GUCS.
Currently placehoders are always created PGC_USERSET, thus
non-superuser can set it. But if loaded module defines the custom
variable as PGC_SUSET, the value set by the user is refused then the
value from ALTER-ROLE-SET or otherwise the default value from
DefineCustom*Variable is used. If the module defines it as
PGC_USERSET, the last value is accepted.
If a placehoders were created PGC_SUSET, non-superusers cannot set it
on-session. But that behavior is not needed since loadable modules
reject PGC_USERSET values as above.
Returning to the topic, that operation can be allowed in PG15, having
being granted by superuser using the GRANT SET ON PARMETER command.
=# GRANT SET ON PARAMETER my.username TO r1;
r1=> ALTER ROLE r1 SET my.username = 'hoge_user_x';
<success>
r1=> \c
r1=> => show my.username;
my.username
-------------
hoge_user_x
(1 row)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed
by a superuser or a role with privileges via pg_parameter_acl. Storing all
placeholder GUC settings as PGC_USERSET would make things more restrictive
than they are today. For example, it would no longer be possible to apply
any ALTER ROLE settings from superusers for placeholders that later become
custom GUCS.
Returning to the topic, that operation can be allowed in PG15, having
being granted by superuser using the GRANT SET ON PARMETER command.
I think that 13d838815 has completely changed the terms that this
discussion needs to be conducted under. It seems clear to me now
that if you want to relax this only-superusers restriction, what
you have to do is store the OID of the role that issued ALTER ROLE/DB SET,
and then apply the same checks that would be used in the ordinary case
where a placeholder is being filled in after being set intra-session.
That is, we'd no longer assume that a value coming from pg_db_role_setting
was set with superuser privileges, but we'd know exactly who did set it.
This might also tie into Nathan's question in another thread about
exactly what permissions should be required to issue ALTER ROLE/DB SET.
In particular I'm wondering if different permissions should be needed to
override an existing entry than if there is no existing entry. If not,
we could find ourselves downgrading a superuser-set entry to a
non-superuser-set entry, which might have bad consequences later
(eg, by rendering the entry nonfunctional because when we actually
load the extension we find out the GUC is SUSET).
Possibly related to this: I felt while working on 13d838815 that
PGC_SUSET and PGC_SU_BACKEND should be usable as GucContext
values for GUC variables, indicating that the GUC requires special
privileges to be set, but we should no longer use them as passed-in
GucContext values. That is, we should remove privilege tests from
the call sites, like this:
(void) set_config_option(stmt->name,
ExtractSetVariableArgs(stmt),
- (superuser() ? PGC_SUSET : PGC_USERSET),
+ PGC_USERSET,
PGC_S_SESSION,
action, true, 0, false);and instead put that behavior inside set_config_option_ext, which
would want to look at superuser_arg(srole) instead, and indeed might
not need to do anything because pg_parameter_aclcheck would subsume
the test. I didn't pursue this further because it wasn't essential
to fixing the bug. But it seems relevant here, because that line of
thought leads to the conclusion that storing PGC_SUSET vs PGC_USERSET
is entirely the wrong approach.
There is a bunch of infrastructure work that has to be done if anyone
wants to make this happen:
* redesign physical representation of pg_db_role_setting
* be sure to clean up if a role mentioned in pg_db_role_setting is dropped
* pg_dump would need to be taught to dump the state of affairs correctly.
regards, tom lane
On Wed, Jul 20, 2022 at 11:50:10AM -0400, Tom Lane wrote:
I think that 13d838815 has completely changed the terms that this
discussion needs to be conducted under. It seems clear to me now
that if you want to relax this only-superusers restriction, what
you have to do is store the OID of the role that issued ALTER ROLE/DB SET,
and then apply the same checks that would be used in the ordinary case
where a placeholder is being filled in after being set intra-session.
That is, we'd no longer assume that a value coming from pg_db_role_setting
was set with superuser privileges, but we'd know exactly who did set it.
I was imagining that the permissions checks would apply at ALTER ROLE/DB
SET time, not at login time. Otherwise, changing a role's privileges might
impact other roles' parameters, and it's not clear (at least to me) what
should happen when the role is dropped. Another reason I imagined it this
way is because that's basically how it works today. We assume that the
pg_db_role_setting entry was added by a superuser, but we don't check that
the user that ran ALTER ROLE/DB SET is still superuser every time you log
in.
This might also tie into Nathan's question in another thread about
exactly what permissions should be required to issue ALTER ROLE/DB SET.
In particular I'm wondering if different permissions should be needed to
override an existing entry than if there is no existing entry. If not,
we could find ourselves downgrading a superuser-set entry to a
non-superuser-set entry, which might have bad consequences later
(eg, by rendering the entry nonfunctional because when we actually
load the extension we find out the GUC is SUSET).
Yeah, this is why I suggested storing something that equates to PGC_SUSET
any time a role is superuser or has grantable GUC permissions.
Possibly related to this: I felt while working on 13d838815 that
PGC_SUSET and PGC_SU_BACKEND should be usable as GucContext
values for GUC variables, indicating that the GUC requires special
privileges to be set, but we should no longer use them as passed-in
GucContext values. That is, we should remove privilege tests from
the call sites, like this:(void) set_config_option(stmt->name, ExtractSetVariableArgs(stmt), - (superuser() ? PGC_SUSET : PGC_USERSET), + PGC_USERSET, PGC_S_SESSION, action, true, 0, false);and instead put that behavior inside set_config_option_ext, which
would want to look at superuser_arg(srole) instead, and indeed might
not need to do anything because pg_parameter_aclcheck would subsume
the test. I didn't pursue this further because it wasn't essential
to fixing the bug. But it seems relevant here, because that line of
thought leads to the conclusion that storing PGC_SUSET vs PGC_USERSET
is entirely the wrong approach.
Couldn't ProcessGUCArray() use set_config_option_ext() with the context
indicated by pg_db_role_setting? Also, instead of using PGC_USERSET in all
the set_config_option() call sites, shouldn't we remove the "context"
argument altogether? I am likely misunderstanding your proposal, but while
I think simplifying set_config_option() is worthwhile, I don't see why it
would preclude storing the context in pg_db_role_setting.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, Jul 21, 2022 at 10:56:41AM -0700, Nathan Bossart wrote:
Couldn't ProcessGUCArray() use set_config_option_ext() with the context
indicated by pg_db_role_setting? Also, instead of using PGC_USERSET in all
the set_config_option() call sites, shouldn't we remove the "context"
argument altogether? I am likely misunderstanding your proposal, but while
I think simplifying set_config_option() is worthwhile, I don't see why it
would preclude storing the context in pg_db_role_setting.
This thread has remained idle for a bit more than two months, so I
have marked its CF entry as returned with feedback.
--
Michael
Hi!
I'd like to resume this discussion.
On Wed, Jul 20, 2022 at 6:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed
by a superuser or a role with privileges via pg_parameter_acl. Storing all
placeholder GUC settings as PGC_USERSET would make things more restrictive
than they are today. For example, it would no longer be possible to apply
any ALTER ROLE settings from superusers for placeholders that later become
custom GUCS.Returning to the topic, that operation can be allowed in PG15, having
being granted by superuser using the GRANT SET ON PARMETER command.I think that 13d838815 has completely changed the terms that this
discussion needs to be conducted under. It seems clear to me now
that if you want to relax this only-superusers restriction, what
you have to do is store the OID of the role that issued ALTER ROLE/DB SET,
and then apply the same checks that would be used in the ordinary case
where a placeholder is being filled in after being set intra-session.
That is, we'd no longer assume that a value coming from pg_db_role_setting
was set with superuser privileges, but we'd know exactly who did set it.
This makes sense. But do we really need to store the OID of the role?
validate_option_array_item() already checks if the placeholder option
passes validation for PGC_SUSET. So, we can just save a flag
indicating that this check was not successful. If so, then the value
stored can be only used for PGC_USERSET. Do you think this would be
correct?
------
Regards,
Alexander Korotkov
Alexander Korotkov <aekorotkov@gmail.com> writes:
This makes sense. But do we really need to store the OID of the role?
validate_option_array_item() already checks if the placeholder option
passes validation for PGC_SUSET. So, we can just save a flag
indicating that this check was not successful. If so, then the value
stored can be only used for PGC_USERSET. Do you think this would be
correct?
Meh ... doesn't seem like much of an improvement. You still need
to store something that's not there now. This also seems to require
some shaky assumptions about decisions having been made when storing
still being valid later on. Given the possibility of granting or
revoking permissions for SET, I think we don't really want it to act
that way.
regards, tom lane
... BTW, re-reading the commit message for a0ffa885e:
One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege
--- one could wish that those were handled by a revocable grant to
PUBLIC, but they are not, because we couldn't make it robust enough
for GUCs defined by extensions.it suddenly struck me to wonder if the later 13d838815 changed the
situation enough to allow revisiting that problem, and/or if storing
the source role's OID in pg_db_role_setting would help.
I don't immediately recall all the problems that led us to leave USERSET
GUCs out of the feature, so maybe this is nuts; but maybe it isn't.
It'd be worth considering if we're trying to improve matters here.
regards, tom lane
On Sat, Nov 19, 2022 at 12:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alexander Korotkov <aekorotkov@gmail.com> writes:
This makes sense. But do we really need to store the OID of the role?
validate_option_array_item() already checks if the placeholder option
passes validation for PGC_SUSET. So, we can just save a flag
indicating that this check was not successful. If so, then the value
stored can be only used for PGC_USERSET. Do you think this would be
correct?Meh ... doesn't seem like much of an improvement. You still need
to store something that's not there now.
Yes, but it wouldn't be needed to track dependencies of pg_role
mentions in pg_db_role_setting. That seems to be a significant
simplification.
This also seems to require
some shaky assumptions about decisions having been made when storing
still being valid later on. Given the possibility of granting or
revoking permissions for SET, I think we don't really want it to act
that way.
Yes, it might be shaky. Consider user sets parameter
pg_db_role_setting, and that appears to be capable only for
PGC_USERSET. Next this user gets the SET permissions. Then this
parameter needs to be set again in order for the new permission to
take effect.
But consider the other side. How should we handle stored OID of a
role? Should the privilege checking be moved from "set time" to "run
time"? Therefore, revoking SET permission from role may affect
existing parameters in pg_db_role_setting. It feels like revoke of
SET permission also aborts changes previously made with that
permission. This is not how we normally do, and that seems confusing.
I think if we implement the flag and make it user-visible, e.g.
implement something like "ALTER ROLE ... SET ... USERSET;", then it
might be the lesser confusing option.
Thoughts?
------
Regards,
Alexander Korotkov
On Sat, Nov 19, 2022 at 12:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
... BTW, re-reading the commit message for a0ffa885e:
One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege --- one could wish that those were handled by a revocable grant to PUBLIC, but they are not, because we couldn't make it robust enough for GUCs defined by extensions.it suddenly struck me to wonder if the later 13d838815 changed the
situation enough to allow revisiting that problem, and/or if storing
the source role's OID in pg_db_role_setting would help.I don't immediately recall all the problems that led us to leave USERSET
GUCs out of the feature, so maybe this is nuts; but maybe it isn't.
It'd be worth considering if we're trying to improve matters here.
I think if we implement the user-visible USERSET flag for ALTER ROLE,
then we might just check permissions for such parameters from the
target role.
------
Regards,
Alexander Korotkov
On Sat, Nov 19, 2022 at 4:02 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Sat, Nov 19, 2022 at 12:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
... BTW, re-reading the commit message for a0ffa885e:
One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege --- one could wish that those were handled by a revocable grant to PUBLIC, but they are not, because we couldn't make it robust enough for GUCs defined by extensions.it suddenly struck me to wonder if the later 13d838815 changed the
situation enough to allow revisiting that problem, and/or if storing
the source role's OID in pg_db_role_setting would help.I don't immediately recall all the problems that led us to leave USERSET
GUCs out of the feature, so maybe this is nuts; but maybe it isn't.
It'd be worth considering if we're trying to improve matters here.I think if we implement the user-visible USERSET flag for ALTER ROLE,
then we might just check permissions for such parameters from the
target role.
I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET syntax.
These options are working only for USERSET GUC variables, but require
less privileges to set. I think there is no problem to implement
Also it seems that this approach doesn't conflict with future
privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be
available unless explicitly REVOKEd. That mean we should be able to
check those privileges during ALTER ROLE.
Opinions on the patch draft?
------
Regards,
Alexander Korotkov
Attachments:
0001-ALTER-ROLE-.-SET-.-TO-.-USER-SET-v1.patchapplication/octet-stream; name=0001-ALTER-ROLE-.-SET-.-TO-.-USER-SET-v1.patchDownload+108-24
.On Sun, Nov 20, 2022 at 8:48 PM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET syntax.
These options are working only for USERSET GUC variables, but require
less privileges to set. I think there is no problem to implementAlso it seems that this approach doesn't conflict with future
privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be
available unless explicitly REVOKEd. That mean we should be able to
check those privileges during ALTER ROLE.Opinions on the patch draft?
Uh, sorry for the wrong link. I meant
/messages/by-id/2271988.1668807706@sss.pgh.pa.us
------
Regards,
Alexander Korotkov
Hey Alexander,
Looks like your latest patch addresses the original issue I posted!
So now I can create a placeholder with the USERSET modifier without a
superuser, while non-USERSET placeholders still require superuser:
```sql
create role foo noinherit;
set role to foo;
alter role foo set prefix.bar to true user set;
ALTER ROLE
alter role foo set prefix.baz to true;
ERROR: permission denied to set parameter "prefix.baz"
set role to postgres;
alter role foo set prefix.baz to true;
ALTER ROLE
```
Also USERSET gucs are marked(`(u)`) on `pg_db_role_setting`:
```sql
select * from pg_db_role_setting ;
setdatabase | setrole | setconfig
-------------+---------+--------------------------------------
0 | 16384 | {prefix.bar(u)=true,prefix.baz=true}
```
Which I guess avoids the need for adding columns to `pg_catalog` and makes
the "fix" simpler.
So from my side this all looks good!
Best regards,
Steve
On Sun, 20 Nov 2022 at 12:50, Alexander Korotkov <aekorotkov@gmail.com>
wrote:
Show quoted text
.On Sun, Nov 20, 2022 at 8:48 PM Alexander Korotkov
<aekorotkov@gmail.com> wrote:I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET
syntax.
These options are working only for USERSET GUC variables, but require
less privileges to set. I think there is no problem to implementAlso it seems that this approach doesn't conflict with future
privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be
available unless explicitly REVOKEd. That mean we should be able to
check those privileges during ALTER ROLE.Opinions on the patch draft?
Links
1.Uh, sorry for the wrong link. I meant
/messages/by-id/2271988.1668807706@sss.pgh.pa.us------
Regards,
Alexander Korotkov
On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote:
So from my side this all looks good!
Thank you for your feedback.
The next revision of the patch is attached. It contains code
improvements, comments and documentation. I'm going to also write
sode tests. pg_db_role_setting doesn't seem to be well-covered with
tests. I will probably need to write a new module into
src/tests/modules to check now placeholders interacts with dynamically
defined GUCs.
------
Regards,
Alexander Korotkov
Attachments:
0001-ALTER-ROLE-.-SET-.-TO-.-USER-SET-v2.patchapplication/octet-stream; name=0001-ALTER-ROLE-.-SET-.-TO-.-USER-SET-v2.patchDownload+198-43
On Thu, Dec 1, 2022 at 6:14 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote:
So from my side this all looks good!
Thank you for your feedback.
The next revision of the patch is attached. It contains code
improvements, comments and documentation. I'm going to also write
sode tests. pg_db_role_setting doesn't seem to be well-covered with
tests. I will probably need to write a new module into
src/tests/modules to check now placeholders interacts with dynamically
defined GUCs.
Another revision of patch is attached. It's fixed now that USER SET
values can't be used for PGC_SUSET parameters. Tests are added. That
require new module test_pg_db_role_setting to check dynamically
defined GUCs.
------
Regards,
Alexander Korotkov
Attachments:
0001-USER-SET-parameters-for-pg_db_role_setting-v3.patchapplication/octet-stream; name=0001-USER-SET-parameters-for-pg_db_role_setting-v3.patchDownload+526-45
On Thu, Dec 1, 2022 at 6:14 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote:
So from my side this all looks good!
Thank you for your feedback.
The next revision of the patch is attached. It contains code
improvements, comments and documentation. I'm going to also write
sode tests. pg_db_role_setting doesn't seem to be well-covered with
tests. I will probably need to write a new module into
src/tests/modules to check now placeholders interacts with dynamically
defined GUCs.Another revision of patch is attached. It's fixed now that USER SET
values can't be used for PGC_SUSET parameters. Tests are added. That
require new module test_pg_db_role_setting to check dynamically
defined GUCs.
I've looked through the last version of a patch. The tests in v3
failed due to naming mismatches. I fixed this in v4 (PFA).
The other thing that may seem unexpected: is whether the value should
apply to the ordinary user only, encoded in the parameter name. The
pro of this is that it doesn't break catalog compatibility by a
separate field for GUC permissions a concept that doesn't exist today
(and maybe not needed at all). Also, it makes the patch more
minimalistic in the code. This is also fully compatible with the
previous parameters naming due to parentheses being an unsupported
symbol for the parameter name.
I've also tried to revise the comments and docs a little bit to
reflect the changes.
The CI-enabled build of patch v4 for reference is at
https://github.com/pashkinelfe/postgres/tree/placeholders-in-alter-role-v4
Overall the patch looks useful and good enough to be committed.
Kind regards,
Pavel Borisov,
Supabase
