Horton

Thread

Commits

  1. Improve error messages around REPLICATION and BYPASSRLS properties.

  2. Allow users with BYPASSRLS to alter their own passwords.

  3. Row-Level Security Policies (RLS)

  1. User with BYPASSRLS privilege can't change password

    Wolfgang Walther <walther@technowledgy.de> — 2020-11-03T15:51:07Z

    Hi,

observed on PG12.4:

CREATE USER alice;
SET ROLE alice;
ALTER USER alice PASSOWRD 'x';
-- works

RESET ROLE;

CREATE USER bob BYPASSRLS;
SET ROLE bob;
ALTER USER bob PASSWORD 'x';
-- ERROR: must be superuser to change bypassrls attribute

I would expect bob to be able to change his password here.

Best

Wolfgang

  2. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T16:22:59Z

    Wolfgang Walther <walther@technowledgy.de> writes:
> CREATE USER bob BYPASSRLS;
> SET ROLE bob;
> ALTER USER bob PASSWORD 'x';
> -- ERROR: must be superuser to change bypassrls attribute

Yeah, duplicated here on HEAD.  The error message seems to think
the command is trying to remove the BYPASSRLS privilege, which
suggests somebody forgot to copy that flag somewhere where it needs
to be copied.  Haven't dug further than that.

			regards, tom lane

  3. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T17:44:11Z

    I wrote:
> Wolfgang Walther <walther@technowledgy.de> writes:
>> CREATE USER bob BYPASSRLS;
>> SET ROLE bob;
>> ALTER USER bob PASSWORD 'x';
>> -- ERROR: must be superuser to change bypassrls attribute

> Yeah, duplicated here on HEAD.  The error message seems to think
> the command is trying to remove the BYPASSRLS privilege, which
> suggests somebody forgot to copy that flag somewhere where it needs
> to be copied.  Haven't dug further than that.

It's a little more subtle than that, but not much.  Commit 491c029db
copied-and-pasted the logic used to deny non-superusers the privilege
to change anything about a superuser role.  That was certainly not the
intention, because the error message was phrased differently from the
superuser case, but that was the effect.  I propose the attached.

(Hm, looks like this behavior is undocumented, too.)

			regards, tom lane

  4. Re: User with BYPASSRLS privilege can't change password

    Wolfgang Walther <walther@technowledgy.de> — 2020-11-03T18:05:32Z

    Tom Lane:
> It's a little more subtle than that, but not much.  Commit 491c029db
> copied-and-pasted the logic used to deny non-superusers the privilege
> to change anything about a superuser role.  That was certainly not the
> intention, because the error message was phrased differently from the
> superuser case, but that was the effect.  I propose the attached.

Wouldn't the following change allow a non-superuser with createrole 
privilege to grant the replication privilege to a role that does not 
have that privilege, yet? This should still be forbidden, I think.

@@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
  			  createrole < 0 &&
  			  createdb < 0 &&
  			  canlogin < 0 &&
-			  isreplication < 0 &&
  			  !dconnlimit &&
  			  !rolemembers &&
  			  !validUntil &&

This is because the "must be superuser to alter replication users" 
condition only triggers when the altered role already has isrepliaction, 
so isreplication could very well be >= 0 here.

The other change looks good.

Best

Wolfgang

  5. Re: User with BYPASSRLS privilege can't change password

    Stephen Frost <sfrost@snowman.net> — 2020-11-03T18:06:42Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I wrote:
> > Wolfgang Walther <walther@technowledgy.de> writes:
> >> CREATE USER bob BYPASSRLS;
> >> SET ROLE bob;
> >> ALTER USER bob PASSWORD 'x';
> >> -- ERROR: must be superuser to change bypassrls attribute
> 
> > Yeah, duplicated here on HEAD.  The error message seems to think
> > the command is trying to remove the BYPASSRLS privilege, which
> > suggests somebody forgot to copy that flag somewhere where it needs
> > to be copied.  Haven't dug further than that.
> 
> It's a little more subtle than that, but not much.  Commit 491c029db
> copied-and-pasted the logic used to deny non-superusers the privilege
> to change anything about a superuser role.  That was certainly not the
> intention, because the error message was phrased differently from the
> superuser case, but that was the effect.  I propose the attached.
> 
> (Hm, looks like this behavior is undocumented, too.)

The intent was certainly that non-superusers shouldn't be able to tweak
the BYPASSRLS bit, but it wasn't intended to prevent users who have
BYPASSRLS from being able to change their own password.

I certainly recall the discussion about that requirement and we should
document it.

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> index 9ce9a66921..5cd479a649 100644
> --- a/src/backend/commands/user.c
> +++ b/src/backend/commands/user.c
> @@ -709,8 +709,10 @@ AlterRole(AlterRoleStmt *stmt)
>  	roleid = authform->oid;
>  
>  	/*
> -	 * To mess with a superuser you gotta be superuser; else you need
> -	 * createrole, or just want to change your own password
> +	 * To mess with a superuser or replication role in any way you gotta be
> +	 * superuser.  We also insist on superuser to change the BYPASSRLS
> +	 * property.  Otherwise, if you don't have createrole, you're only allowed
> +	 * to change your own password.
>  	 */
>  	if (authform->rolsuper || issuper >= 0)
>  	{
> @@ -726,7 +728,7 @@ AlterRole(AlterRoleStmt *stmt)
>  					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>  					 errmsg("must be superuser to alter replication users")));
>  	}
> -	else if (authform->rolbypassrls || bypassrls >= 0)
> +	else if (bypassrls >= 0)
>  	{
>  		if (!superuser())
>  			ereport(ERROR,

This change looks correct, we shouldn't be worrying about what's already
been set on the role.

> @@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
>  			  createrole < 0 &&
>  			  createdb < 0 &&
>  			  canlogin < 0 &&
> -			  isreplication < 0 &&
>  			  !dconnlimit &&
>  			  !rolemembers &&
>  			  !validUntil &&

This seems like an independent change..?  Not saying it's wrong though.

Thanks,

Stephen

  6. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T18:17:23Z

    Stephen Frost <sfrost@snowman.net> writes:
>> @@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
>>			 createrole < 0 &&
>>			 createdb < 0 &&
>>			 canlogin < 0 &&
>> -			 isreplication < 0 &&
>>			 !dconnlimit &&
>>			 !rolemembers &&
>>			 !validUntil &&

> This seems like an independent change..?  Not saying it's wrong though.

That test is redundant, since we wouldn't be in this path at all if
isreplication >= 0.  You could alternatively argue that this should
redundantly test all three of issuper, isreplication, and bypassrls;
but testing just one of them is confusing and hence bug-prone.

			regards, tom lane

  7. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T18:19:01Z

    Wolfgang Walther <walther@technowledgy.de> writes:
> This is because the "must be superuser to alter replication users" 
> condition only triggers when the altered role already has isrepliaction, 
> so isreplication could very well be >= 0 here.

How do you figure that?  This is in an "else" path after

	else if (authform->rolreplication || isreplication >= 0)

so AFAICS it's impossible to get there.  If it isn't impossible,
we have a much bigger hole with respect to issuper.

			regards, tom lane

  8. Re: User with BYPASSRLS privilege can't change password

    Wolfgang Walther <walther@technowledgy.de> — 2020-11-03T18:26:13Z

    Tom Lane:
> How do you figure that?  This is in an "else" path after
> 
> 	else if (authform->rolreplication || isreplication >= 0)
> 
> so AFAICS it's impossible to get there.  If it isn't impossible,
> we have a much bigger hole with respect to issuper.

Yes, you're right. I read the || as &&. And also missed the ! in else if 
(!have_createrole_privilege()) btw. :)

I guess the error message "must be superuser to alter replication users" 
led me on the wrong path. I would be more precise as "must be superuser 
to alter replication users or change replication attribute" to cover the 
change-non-replication-to-replication user case, I think. The same thing 
for superusers.

Best

Wolfgang

  9. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T18:51:52Z

    Wolfgang Walther <walther@technowledgy.de> writes:
> Tom Lane:
>> so AFAICS it's impossible to get there.  If it isn't impossible,
>> we have a much bigger hole with respect to issuper.

> Yes, you're right. I read the || as &&. And also missed the ! in else if 
> (!have_createrole_privilege()) btw. :)

Actually the right way to deal with this potential confusion is to
add a comment, as below.  I fixed the docs too.

> I guess the error message "must be superuser to alter replication users" 
> led me on the wrong path. I would be more precise as "must be superuser 
> to alter replication users or change replication attribute" to cover the 
> change-non-replication-to-replication user case, I think. The same thing 
> for superusers.

I'd be okay with changing the error text in HEAD, but less so in the back
branches, since that'd cause thrashing of translatable strings.

			regards, tom lane

  10. Re: User with BYPASSRLS privilege can't change password

    David G. Johnston <david.g.johnston@gmail.com> — 2020-11-03T18:58:39Z

    On Tue, Nov 3, 2020 at 11:06 AM Stephen Frost <sfrost@snowman.net> wrote:

>
> > diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> > index 9ce9a66921..5cd479a649 100644
> > --- a/src/backend/commands/user.c
> > +++ b/src/backend/commands/user.c
> > @@ -709,8 +709,10 @@ AlterRole(AlterRoleStmt *stmt)
> >       roleid = authform->oid;
> >
> >       /*
> > -      * To mess with a superuser you gotta be superuser; else you need
> > -      * createrole, or just want to change your own password
> > +      * To mess with a superuser or replication role in any way you
> gotta be
> > +      * superuser.  We also insist on superuser to change the BYPASSRLS
> > +      * property.  Otherwise, if you don't have createrole, you're only
> allowed
> > +      * to change your own password.
> >        */
> >       if (authform->rolsuper || issuper >= 0)
> >       {
> > @@ -726,7 +728,7 @@ AlterRole(AlterRoleStmt *stmt)
> >
>  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> >                                        errmsg("must be superuser to
> alter replication users")));
> >       }
> > -     else if (authform->rolbypassrls || bypassrls >= 0)
> > +     else if (bypassrls >= 0)
> >       {
> >               if (!superuser())
> >                       ereport(ERROR,
>
> This change looks correct, we shouldn't be worrying about what's already
> been set on the role.
>
>
Is the nuance that in reality a non-superuser cannot specify BypassRLS even
if the effective value is unchanged unimportant here?

David J.

  11. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T19:06:50Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> Is the nuance that in reality a non-superuser cannot specify BypassRLS even
> if the effective value is unchanged unimportant here?

I was wondering about that.  You could definitely make a case for a
weaker set of rules here.  However, the superuser restriction has been
like that for 15-ish years without much complaint, so it doesn't seem
that it bothers people in practice.

			regards, tom lane

  12. Re: User with BYPASSRLS privilege can't change password

    David G. Johnston <david.g.johnston@gmail.com> — 2020-11-03T19:13:13Z

    On Tue, Nov 3, 2020 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > Is the nuance that in reality a non-superuser cannot specify BypassRLS
> even
> > if the effective value is unchanged unimportant here?
>
> I was wondering about that.  You could definitely make a case for a
> weaker set of rules here.  However, the superuser restriction has been
> like that for 15-ish years without much complaint, so it doesn't seem
> that it bothers people in practice.
>
>
Agreed that the behavior seems fine to keep.  Was more about the
documentation saying "change" instead of "specify".

David J.

  13. Re: User with BYPASSRLS privilege can't change password

    Stephen Frost <sfrost@snowman.net> — 2020-11-03T20:02:38Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Wolfgang Walther <walther@technowledgy.de> writes:
> > Tom Lane:
> >> so AFAICS it's impossible to get there.  If it isn't impossible,
> >> we have a much bigger hole with respect to issuper.
> 
> > Yes, you're right. I read the || as &&. And also missed the ! in else if 
> > (!have_createrole_privilege()) btw. :)
> 
> Actually the right way to deal with this potential confusion is to
> add a comment, as below.  I fixed the docs too.
> 
> > I guess the error message "must be superuser to alter replication users" 
> > led me on the wrong path. I would be more precise as "must be superuser 
> > to alter replication users or change replication attribute" to cover the 
> > change-non-replication-to-replication user case, I think. The same thing 
> > for superusers.
> 
> I'd be okay with changing the error text in HEAD, but less so in the back
> branches, since that'd cause thrashing of translatable strings.

I tend to agree with that.

> diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
> index aef30521bc..5aa5648ae7 100644
> --- a/doc/src/sgml/ref/alter_role.sgml
> +++ b/doc/src/sgml/ref/alter_role.sgml
> @@ -71,7 +71,9 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
>     Attributes not mentioned in the command retain their previous settings.
>     Database superusers can change any of these settings for any role.
>     Roles having <literal>CREATEROLE</literal> privilege can change any of these
> -   settings, but only for non-superuser and non-replication roles.
> +   settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
> +   and <literal>BYPASSRLS</literal>; but only for non-superuser and
> +   non-replication roles.
>     Ordinary roles can only change their own password.
>    </para>

This looks good, though should we also mention that you have to be a
superuser to set BYPASSRLS on a role in the CREATE ROLE documentation
too..?  The error message there could also be improved, it looks like.
Also, we don't seem to document in CREATE ROLE that you have to be a
superuser to create a REPLICATION role either, even though the code
clearly says that.

Attached is a patch for all of those.  As mentioned, the error message
probably only makes sense to change in HEAD, though improving the
documentation could be back-patched.

Thanks,

Stephen

  14. Re: User with BYPASSRLS privilege can't change password

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-03T20:05:39Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'd be okay with changing the error text in HEAD, but less so in the back
>> branches, since that'd cause thrashing of translatable strings.

> I tend to agree with that.

OK, will do it that way.

> Attached is a patch for all of those.  As mentioned, the error message
> probably only makes sense to change in HEAD, though improving the
> documentation could be back-patched.

Check.  I'll merge this with what I was doing and push it all.

			regards, tom lane