Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 16 Jan 2023 14:22:27 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jelte Fennema <postgres@jeltef.nl>
Cc: Jelte Fennema <Jelte.Fennema@microsoft.com>,
	"isaac.morland@gmail.com" <isaac.morland@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	Nathan Bossart <nathandbossart@gmail.com>,
	Andrew Dunstan <andrew@dunslane.net>
Subject: Re: [EXTERNAL] Re: [PATCH] Support using "all" for the db user in
 pg_ident.conf
Message-ID: <Y8TfE699nFya24fp@paquier.xyz>
References: <DBBPR83MB0507FEC2E8965012990A80D0F7FC9@DBBPR83MB0507.EURPRD83.prod.outlook.com>
 <Y76YEHolPL4LIAtO@paquier.xyz>
 <CAGECzQTkwELHUOAKhvdA+m3tWbUQySHHkExJV8GAZ1pwgbEgXg@mail.gmail.com>
 <Y7+bXzP5ubRHgoyf@paquier.xyz>
 <CAGECzQRNow4MwkBjgPxywXdJU_K3a9+Pm78JB7De3yQwwkTDew@mail.gmail.com>
 <Y8C9R4dIv5iHH0gF@paquier.xyz>
 <CAGECzQQ_h_-kUg_xy=mm8b6wQ=TSMV=-5h37T32=qODxi+GOMA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="tNmOO70C/8EOxd4r"
Content-Disposition: inline
In-Reply-To: <CAGECzQQ_h_-kUg_xy=mm8b6wQ=TSMV=-5h37T32=qODxi+GOMA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Y8TfE699nFya24fp%40paquier.xyz>
Precedence: bulk


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

On Fri, Jan 13, 2023 at 09:19:10AM +0100, Jelte Fennema wrote:
>> Even if folks applying quotes
>> would not be able anymore to replace the pattern, the risk seems a bit
>> remote?
>=20
> Yeah I agree the risk is remote. To be clear, the main pattern I'm
> worried about breaking is simply "\1". Where people had put
> quotes around \1 for no reason. All in all, I'm fine if 0003 gets
> merged, but I'd also be fine with it if it doesn't. Both the risk
> and the advantage seem fairly small.

Still, I am having a few second thoughts about 0003 after thinking
about it over the weekend.  Except if I am missing something, there
are no issues with 0004 if we keep the current behavior of always
replacing \1 even if pg-user is quoted?  I would certainly add a new
test case either way.

>> I don't see how much that's different from the recent discussion with
>> regexps added for databases and users to pg_hba.conf.  And consistency
>> sounds pretty good to me here.
>=20
> It's not much different, except that here also all and + change their mea=
ning
> (for pg_hba.conf those special cases already existed). Mainly I called it=
 out
> because I realised this discussion was called out in that commit too.
>=20
>> Regexps can have commas
>=20
> That's a really good reason to allow quoted regexes indeed. Even for pg_i=
dent
> entries, commas in unquoted regexes would cause the AuthToken parsing to =
fail.
>=20
> Is there anything you still want to see changed about any of the patches?

+           /*
+            * Mark the token as quoted, so it will only be compared litera=
lly
+            * and not for special meanings like, such as "all" and members=
hip
+            * checks using the + prefix.
+            */
+           expanded_pg_user_token =3D make_auth_token(expanded_pg_user, tr=
ue);
It is critical to quote this AuthToken after the replacement, indeed.
Or we are in big trouble.

-           /* no substitution, so copy the match */
-           expanded_pg_user =3D pstrdup(identLine->pg_user->string);
+           expanded_pg_user_token =3D identLine->pg_user;
Perhaps it would be simpler to use copy_auth_token() in this code path
and always free the resulting token?

In the code path where system-user is a regexp, could it be better
to skip the replacement of \1 in the new AuthToken if pg-user is
itself a regexp?  The compiled regexp would be the same, but it could
be considered as a bit confusing, as it can be thought that the
compiled regexp of pg-user happened after the replacement?

No issues with 0002 after a second look, so applied to move on.
--
Michael

--tNmOO70C/8EOxd4r
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmPE3xMACgkQnvQgOdby
QH3fdhAAiyNOOmg4cO5YjyjfQ/6LhW4hSXAVetKQPjBbmzC26Xuhr//csydFFkGx
caC7h81FRKr22clQgFI9j7EfinPtid2QabFexcGEeSP/wO0C0kFIqJpvfa+/0Kni
JhpC9o2onQJZOfkn22MvR6MRlEHZ2K7OFx7vRy7zDEbEWdftbFsemp/CVOqUzPjc
EsZFTBzPSiWA52gK+ysZUCfSp45dgc/apwro3ahHhqgQ7wyhjpO6typnf1zuU9zp
G6TRF8sBVFVZUZIklbEOamZr4LsBDLkyNu5ecKuKa55gjkh2729De1iE8c+vtvY7
LObCM1Eu3yWI/L1mGMwtI8STrjInLNz9z/w3NkELdkvda3dPSGn6OQOTJWjL3rp8
Ztb0/ZeqPWLUllwzZBy92tI6wga8RJgrkKawPCyZMN10yOV7aftlpyccw07+j0a8
v8yT7nPISdyL2ROMkuhICoZNKus+cjwHu0l3QPd364RfwZMlmPfBKZh/zbpSxmHv
yDkVf/DNTrx8Z7HxVcxPyQbjs8zFbIEcWHLSVjpvfC7u4Wb7Jdz6g6VINEJ3EMV7
CtpnJXvgYbWMYZtasEV9msAscC3RGefLR6tDCbPImosK7iTI0KLIGcOgaKRW/fcG
t/J+NOQXYJzRa60uhPESCjuYH6h/tsqN4EkbdgvNANWsCLpUNQo=
=vlkp
-----END PGP SIGNATURE-----

--tNmOO70C/8EOxd4r--