Date: Tue, 3 Nov 2020 13:06:42 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Wolfgang Walther <walther@technowledgy.de>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: User with BYPASSRLS privilege can't change password
Message-ID: <20201103180641.GX16415@tamriel.snowman.net>
References: <a5548a9f-89ee-3167-129d-162b5985fcf8@technowledgy.de>
 <923005.1604420579@sss.pgh.pa.us>
 <956994.1604425451@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Lqu9UxmKHIR7DYMI"
Content-Disposition: inline
In-Reply-To: <956994.1604425451@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--Lqu9UxmKHIR7DYMI
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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
>=20
> > 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.
>=20
> 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.
>=20
> (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 =3D authform->oid;
> =20
>  	/*
> -	 * 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 allo=
wed
> +	 * to change your own password.
>  	 */
>  	if (authform->rolsuper || issuper >=3D 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 >=3D 0)
> +	else if (bypassrls >=3D 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

--Lqu9UxmKHIR7DYMI
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJfoZwxAAoJEO1sijiDR2RVvqUQALy7j9BsB2jOXZTlpdtY1KxV
KpiHhHM5KRRcc+ZnzHxcX8oTPLKsUPhxudSWeldrLUT4USZiQRR97AOQWv2x46HX
NVUKJ4BWd+eyLHSVn40NkfZBfOAcJ97/t83c7Y3Nr09wfKLM8DkwdkS9KfKmQUKw
Sok22yhLBvT0wcHym6ofXVdQf65J9RLX61AzpV9ik8yLB7OACYEsu2PpIZZ5UX9r
kJRfMnKVKJ1WBIK7Kuuy+Z2JNBSYhiDFmsCnI61030+vXM4O1mc+72M8b94SpjtS
jF4qU+XkzjoBtuKgvikqbhTQ8kv21+PDD+H8KIyM0yX9BBgSMYbn9mcuOWjMGLMy
ItCDEwPjZZwwdzTZSW4qrjV59VS7myzLaaP6qK7ekmxefn4wR65RZdDEjisxvUqZ
w98bTnpF17LhD3XIzDTRiqE/XlxlnyMG8pXCvT2uR/6/JvgJr05ZqG8lZhbyyW4c
YpgQDMv4SQR8LK5qUiQ8TTfz6ujWBVuYacV9Me8Wkg8qkpw4Sx4T6ZyQmZWOBJ2m
yeOnn0P0qFmzAz+cuOUOtFoJS2KQk6CufuWpb8oV0CTGOeYMKGOJO/+gn3Ut9FYy
YocIonaTaH5nRgFGyKNNDR/a3cGsDgwe3i312ozkh4dwM1BgxJD5OgVls5o42EsE
rKd2Xmi2mCf5KRZQthd/
=cOmG
-----END PGP SIGNATURE-----

--Lqu9UxmKHIR7DYMI--