Re: SHOW ALL does not honor pg_read_all_settings membership
On Thu, 2018-03-01 at 16:22 +0100, I wrote: > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > to teach SHOW ALL to show all GUCs when the user belongs to > pg_read_all_settings. > > Patch attached; I think this should be backpatched. Now that the dust from the last commitfest is settling, I'll make a second attempt to attract attention for this small bug fix. The original commit was Simon's. Yours, Laurenz Albe
Re: SHOW ALL does not honor pg_read_all_settings membership
On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: > Now that the dust from the last commitfest is settling, I'll make a second > attempt to attract attention for this small bug fix. > > The original commit was Simon's. Thanks for the ping. This was new as of v10, so this cannot be listed as an open item still I have added that under the section for older bugs, because you are right as far as I can see. GetConfigOption is wrong by the way, as restrict_superuser means that all members of the group pg_read_all_settings can read GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at least needs a fix, the variable ought to be renamed as well. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
Michael Paquier wrote: > On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: > > Now that the dust from the last commitfest is settling, I'll make a second > > attempt to attract attention for this small bug fix. > > > > The original commit was Simon's. > > Thanks for the ping. > > This was new as of v10, so this cannot be listed as an open item still I > have added that under the section for older bugs, because you are right > as far as I can see. > > GetConfigOption is wrong by the way, as restrict_superuser means that > all members of the group pg_read_all_settings can read > GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at > least needs a fix, the variable ought to be renamed as well. Thanks for the review! I agree; here is a patch for that. Yours, Laurenz AlbeFrom cfe04ef974dba324c47510b854587de6c33e39a1 Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Fri, 20 Apr 2018 13:13:20 +0200 Subject: [PATCH] Fix comments and a parameter name Commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 changed the semantics of GetConfigOption, but did not update the comment. Also, the parameter name should better be changed. Noted by Michael Paquier. --- src/backend/utils/misc/guc.c | 10 +- src/include/utils/guc.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7acec5ea21..b5501fd949 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6869,15 +6869,15 @@ SetConfigOption(const char *name, const char *value, * this cannot be distinguished from a string variable with a NULL value!), * otherwise throw an ereport and don't return. * - * If restrict_superuser is true, we also enforce that only superusers can - * see GUC_SUPERUSER_ONLY variables. This should only be passed as true - * in user-driven calls. + * If restrict_privileged is true, we also enforce that only superusers and + * members of the pg_read_all_settings role can see GUC_SUPERUSER_ONLY + * variables. This should only be passed as true in user-driven calls. * * The string is *not* allocated for modification and is really only * valid until the next call to configuration related functions. */ const char * -GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser) +GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged) { struct config_generic *record; static char buffer[256]; @@ -6892,7 +6892,7 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser) errmsg("unrecognized configuration parameter \"%s\"", name))); } - if (restrict_superuser && + if (restrict_privileged && (record->flags & GUC_SUPERUSER_ONLY) && !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) ereport(ERROR, diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 3d13a33b94..f462eabe59 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -347,7 +347,7 @@ extern void DefineCustomEnumVariable( extern void EmitWarningsOnPlaceholders(const char *className); extern const char *GetConfigOption(const char *name, bool missing_ok, -bool restrict_superuser); +bool restrict_privileged); extern const char *GetConfigOptionResetString(const char *name); extern int GetConfigOptionFlags(const char *name, bool missing_ok); extern void ProcessConfigFile(GucContext context); -- 2.14.3
Re: SHOW ALL does not honor pg_read_all_settings membership
On Fri, Apr 20, 2018 at 01:21:46PM +0200, Laurenz Albe wrote: > I agree; here is a patch for that. Thanks for taking care of that as well this looks fine to me. Note to committer: this needs to be merged with the first patch actually fixing the SHOW ALL issue. All the four core callers of GetConfigOption() actually do not use restrict_superuser set to true... Modules may use this option, so of course let's not remove it. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
On 17 April 2018 at 04:28, Michael Paquier wrote: > On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: >> Now that the dust from the last commitfest is settling, I'll make a second >> attempt to attract attention for this small bug fix. >> >> The original commit was Simon's. > > Thanks for the ping. > > This was new as of v10, so this cannot be listed as an open item still I > have added that under the section for older bugs, because you are right > as far as I can see. OK, agreed, its a bug. Any objections to backpatch to v10? > GetConfigOption is wrong by the way, as restrict_superuser means that > all members of the group pg_read_all_settings can read > GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at > least needs a fix, the variable ought to be renamed as well. OK -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote: > Any objections to backpatch to v10? A backpatch is acceptable in my opinion. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-May-31, Michael Paquier wrote: > On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote: > > Any objections to backpatch to v10? > > A backpatch is acceptable in my opinion. Agreed on backpatching. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
BTW a further bug here seems to be that "select * from pg_settings()" does not show the source file/line for members of the role, which seems to be documented to occur. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-Jun-08, Alvaro Herrera wrote: > BTW a further bug here seems to be that "select * from pg_settings()" > does not show the source file/line for members of the role, which seems > to be documented to occur. And I think this fixes it. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From c71a14bdac838efc09d01b60383fa6e631606c57 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 8 Jun 2018 15:12:58 -0400 Subject: [PATCH] fix sourcefile/line display for pg_read_all_settings role --- src/backend/utils/misc/guc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 274034d88b..e84d56dae6 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8595,7 +8595,8 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) * security reasons, we don't show source file/line number for * non-superusers. */ - if (conf->source == PGC_S_FILE && superuser()) + if (conf->source == PGC_S_FILE && + is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) { values[14] = conf->sourcefile; snprintf(buffer, sizeof(buffer), "%d", conf->sourceline); -- 2.11.0
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-Mar-01, Laurenz Albe wrote: > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > to teach SHOW ALL to show all GUCs when the user belongs to > pg_read_all_settings. > > Patch attached; I think this should be backpatched. Done, with the further fixes downthread. Thanks! -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On Fri, Jun 08, 2018 at 03:13:57PM -0400, Alvaro Herrera wrote: > And I think this fixes it. -if (conf->source == PGC_S_FILE && superuser()) +if (conf->source == PGC_S_FILE && +is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) Thanks Alvaro! This bit in GetConfigOptionByNum() looks fine to me. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
Alvaro Herrera wrote: > On 2018-Mar-01, Laurenz Albe wrote: > > > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > > to teach SHOW ALL to show all GUCs when the user belongs to > > pg_read_all_settings. > > > > Patch attached; I think this should be backpatched. > > Done, with the further fixes downthread. Thanks! Thank you! Laurenz Albe
Re: SHOW ALL does not honor pg_read_all_settings membership
On Thu, 2018-03-01 at 16:22 +0100, I wrote: > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > to teach SHOW ALL to show all GUCs when the user belongs to > pg_read_all_settings. > > Patch attached; I think this should be backpatched. Now that the dust from the last commitfest is settling, I'll make a second attempt to attract attention for this small bug fix. The original commit was Simon's. Yours, Laurenz Albe
Re: SHOW ALL does not honor pg_read_all_settings membership
On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: > Now that the dust from the last commitfest is settling, I'll make a second > attempt to attract attention for this small bug fix. > > The original commit was Simon's. Thanks for the ping. This was new as of v10, so this cannot be listed as an open item still I have added that under the section for older bugs, because you are right as far as I can see. GetConfigOption is wrong by the way, as restrict_superuser means that all members of the group pg_read_all_settings can read GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at least needs a fix, the variable ought to be renamed as well. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
Michael Paquier wrote: > On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: > > Now that the dust from the last commitfest is settling, I'll make a second > > attempt to attract attention for this small bug fix. > > > > The original commit was Simon's. > > Thanks for the ping. > > This was new as of v10, so this cannot be listed as an open item still I > have added that under the section for older bugs, because you are right > as far as I can see. > > GetConfigOption is wrong by the way, as restrict_superuser means that > all members of the group pg_read_all_settings can read > GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at > least needs a fix, the variable ought to be renamed as well. Thanks for the review! I agree; here is a patch for that. Yours, Laurenz AlbeFrom cfe04ef974dba324c47510b854587de6c33e39a1 Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Fri, 20 Apr 2018 13:13:20 +0200 Subject: [PATCH] Fix comments and a parameter name Commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 changed the semantics of GetConfigOption, but did not update the comment. Also, the parameter name should better be changed. Noted by Michael Paquier. --- src/backend/utils/misc/guc.c | 10 +- src/include/utils/guc.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7acec5ea21..b5501fd949 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6869,15 +6869,15 @@ SetConfigOption(const char *name, const char *value, * this cannot be distinguished from a string variable with a NULL value!), * otherwise throw an ereport and don't return. * - * If restrict_superuser is true, we also enforce that only superusers can - * see GUC_SUPERUSER_ONLY variables. This should only be passed as true - * in user-driven calls. + * If restrict_privileged is true, we also enforce that only superusers and + * members of the pg_read_all_settings role can see GUC_SUPERUSER_ONLY + * variables. This should only be passed as true in user-driven calls. * * The string is *not* allocated for modification and is really only * valid until the next call to configuration related functions. */ const char * -GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser) +GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged) { struct config_generic *record; static char buffer[256]; @@ -6892,7 +6892,7 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser) errmsg("unrecognized configuration parameter \"%s\"", name))); } - if (restrict_superuser && + if (restrict_privileged && (record->flags & GUC_SUPERUSER_ONLY) && !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) ereport(ERROR, diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 3d13a33b94..f462eabe59 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -347,7 +347,7 @@ extern void DefineCustomEnumVariable( extern void EmitWarningsOnPlaceholders(const char *className); extern const char *GetConfigOption(const char *name, bool missing_ok, -bool restrict_superuser); +bool restrict_privileged); extern const char *GetConfigOptionResetString(const char *name); extern int GetConfigOptionFlags(const char *name, bool missing_ok); extern void ProcessConfigFile(GucContext context); -- 2.14.3
Re: SHOW ALL does not honor pg_read_all_settings membership
On Fri, Apr 20, 2018 at 01:21:46PM +0200, Laurenz Albe wrote: > I agree; here is a patch for that. Thanks for taking care of that as well this looks fine to me. Note to committer: this needs to be merged with the first patch actually fixing the SHOW ALL issue. All the four core callers of GetConfigOption() actually do not use restrict_superuser set to true... Modules may use this option, so of course let's not remove it. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
On 17 April 2018 at 04:28, Michael Paquier wrote: > On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote: >> Now that the dust from the last commitfest is settling, I'll make a second >> attempt to attract attention for this small bug fix. >> >> The original commit was Simon's. > > Thanks for the ping. > > This was new as of v10, so this cannot be listed as an open item still I > have added that under the section for older bugs, because you are right > as far as I can see. OK, agreed, its a bug. Any objections to backpatch to v10? > GetConfigOption is wrong by the way, as restrict_superuser means that > all members of the group pg_read_all_settings can read > GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at > least needs a fix, the variable ought to be renamed as well. OK -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote: > Any objections to backpatch to v10? A backpatch is acceptable in my opinion. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-May-31, Michael Paquier wrote: > On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote: > > Any objections to backpatch to v10? > > A backpatch is acceptable in my opinion. Agreed on backpatching. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
BTW a further bug here seems to be that "select * from pg_settings()" does not show the source file/line for members of the role, which seems to be documented to occur. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-Jun-08, Alvaro Herrera wrote: > BTW a further bug here seems to be that "select * from pg_settings()" > does not show the source file/line for members of the role, which seems > to be documented to occur. And I think this fixes it. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From c71a14bdac838efc09d01b60383fa6e631606c57 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 8 Jun 2018 15:12:58 -0400 Subject: [PATCH] fix sourcefile/line display for pg_read_all_settings role --- src/backend/utils/misc/guc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 274034d88b..e84d56dae6 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8595,7 +8595,8 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) * security reasons, we don't show source file/line number for * non-superusers. */ - if (conf->source == PGC_S_FILE && superuser()) + if (conf->source == PGC_S_FILE && + is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) { values[14] = conf->sourcefile; snprintf(buffer, sizeof(buffer), "%d", conf->sourceline); -- 2.11.0
Re: SHOW ALL does not honor pg_read_all_settings membership
On 2018-Mar-01, Laurenz Albe wrote: > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > to teach SHOW ALL to show all GUCs when the user belongs to > pg_read_all_settings. > > Patch attached; I think this should be backpatched. Done, with the further fixes downthread. Thanks! -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SHOW ALL does not honor pg_read_all_settings membership
On Fri, Jun 08, 2018 at 03:13:57PM -0400, Alvaro Herrera wrote: > And I think this fixes it. -if (conf->source == PGC_S_FILE && superuser()) +if (conf->source == PGC_S_FILE && +is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) Thanks Alvaro! This bit in GetConfigOptionByNum() looks fine to me. -- Michael signature.asc Description: PGP signature
Re: SHOW ALL does not honor pg_read_all_settings membership
Alvaro Herrera wrote: > On 2018-Mar-01, Laurenz Albe wrote: > > > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot > > to teach SHOW ALL to show all GUCs when the user belongs to > > pg_read_all_settings. > > > > Patch attached; I think this should be backpatched. > > Done, with the further fixes downthread. Thanks! Thank you! Laurenz Albe