Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 15 Dec 2022 08:52:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Jonathan S. Katz" <jkatz@postgresql.org>
Cc: Daniel Gustafsson <daniel@yesql.se>, Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Raising the SCRAM iteration count
Message-ID: <Y5phwMQOMCZAAZYK@paquier.xyz>
References: <F72E7BC7-189F-4B17-BF47-9735EB72C364@yesql.se>
 <20221210001538.czikyvk3xtkphxfu@awork3.anarazel.de>
 <4880738a-cbf7-c9a1-4faf-ba861c731c63@postgresql.org>
 <75619A47-4CD0-4E0F-8A30-32F83FF593DD@yesql.se>
 <Y5kgHT43XqDEc3Ev@paquier.xyz>
 <FDBA5DA0-1015-4162-A8F5-E7BC1F620837@yesql.se>
 <869f3aee-5c46-998d-be42-b3b232415bef@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2NovYXgAkcdQF9sh"
Content-Disposition: inline
In-Reply-To: <869f3aee-5c46-998d-be42-b3b232415bef@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/Y5phwMQOMCZAAZYK%40paquier.xyz>
Precedence: bulk


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

On Wed, Dec 14, 2022 at 01:59:04PM -0500, Jonathan S. Katz wrote:
> On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>> I was thinking about it but opted for the simpler approach of a GUC name=
 with
>> the algorithm baked into it: scram_sha256_iterations.  It doesn't seem a=
ll that
>> likely that we'll have more than two versions of SCRAM (sha256/sha512) so
>> the additional complexity doesn't seem worth it.
>=20
> I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].
>=20
> I do have mixed feelings on the 'x1=3Dy1,x2=3Dy2' style GUC, but we do ha=
ve
> machinery to handle it and it gives a bit more flexibility over how many
> SCRAM hash methods get added. I'd like to hear more feedback.

Technically, I would put the logic to parse the GUC to scram-common.c
and let libpq and the backend use it.  Saying that, we are just
talking about what looks like one new hashing method, so a separate
GUC is fine by me.

> (I don't know if there will be a world if we ever let users BYO-hash, but
> that case may force separate GUCs anyway).
>=20
> [1] https://datatracker.ietf.org/doc/draft-melnikov-scram-sha3-512/

Still, the odds is that we are going to see one update to
SCRAM-SHA-256 that we will just need to pick up?

>> The attached v2 has the GUC rename and a change to GUC_REPORT such that =
the
>> frontend can use the real value rather than the default.  I kept it for =
super
>> users so far, do you think it should be a user setting being somewhat se=
nsitive?
>=20
> No, because a user can set the number of iterations today if they build
> their own SCRAM secret. I think it's OK if they change it in a session.
>=20
> If a superuser wants to enforce a minimum iteration count, they can write=
 a
> password_check_hook. (Or we could add another GUC to enforce that).

Hm?  check_password_hook does not allow one to recompile the password
given by the user, except if I am missing your point?

> -pg_fe_scram_build_secret(const char *password, const char **errstr)
> +pg_fe_scram_build_secret(const char *password, int iterations, const char
> **errstr)
>=20
> I have mild worry about changing this function definition for downstream
> usage, esp. for drivers. Perhaps it's not that big of a deal, and perhaps
> this will end up being needed for the work we've discussed around
> "\password" but I do want to note that this could be a breaking change.

FWIW, an extension would be required to enforce the type of hash
used, which is an extra parameter on top of the iteration number when
building the SCRAM verifier.

> +	else if (strcmp(name, "scram_sha256_iterations") =3D=3D 0)
> +	{
> +		conn->scram_iterations =3D atoi(value);
> +	}
>=20
> Maybe out of scope for this patch based on what else is in the patch, but=
 I
> was wondering why we don't use a "strncmp" here?

What would that change?  This needs an equal match.

    conn->in_hot_standby =3D PG_BOOL_UNKNOWN;
+   conn->scram_iterations =3D SCRAM_DEFAULT_ITERATIONS;

s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
s/scram_iterations/scram_sha_256_interations/ perhaps?  It does not
look like we'd have the same default across the various SHA variations
if we stick with the RFC definitions..

+#ifndef FRONTEND
+/*
+ * Number of iterations when generating new secrets.
+ */
+extern PGDLLIMPORT int scram_sha256_iterations;
+#endif

It looks like libpq/scram.h, which is backend-only, would be a better
location.

@@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations=
, char **salt,
    encoded_salt[encoded_len] =3D '\0';
=20
    *salt =3D encoded_salt;
-   *iterations =3D SCRAM_DEFAULT_ITERATIONS;
+   *iterations =3D scram_sha256_iterations;

This looks incorrect to me?  The mock authentication is here to
produce a realistic verifier, still it will fail.  It seems to me that
we'd better stick to the default in all the cases.

(FWIW, extending \password with custom options would have the
advantage to allow older server versions to use a custom iteration
number.  Perhaps that's not worth bothering about, just saying as a
separate thing to consider.)
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmOaYcAACgkQnvQgOdby
QH1G6w//YoH5BmVbsXkcHwpdZpyzUe4kup2+YaNQlkl3V8x2Z9B1lwRiGwL66JvP
SKMIdFgMrsRm5ftATYwaoXDORjue0EUL38DuH+IDa2gObrHUr4o6XXipbbGho/BV
lsvhRKzTsnYBrO0i8+GtrMoiwvaUiEZx9UniZVaIEcBgazhcR5trex6s/LIzoFeh
GilvVRLsWKgiTUtqbToKbP4qZXE0Xi1POzvJ1UYJCQ/tN3R8wkJ6ih5YwhobnXhp
4G6M6BKLjJmxmSxYFUGT82eXh5AHpOIfaXac6nxWT+O7KjJrtoFgVDEAanui5LwV
R8kxYxkqpDfR7VQ/mSnIPAlapW6VCjOdE/tQK3FlFN9BopfbRsW0AhhR3RZxB7Hx
qNpuFFyaBVZPE2n3bcxawAuBioy1Zqnxn6RkVdA9XeAoIKYPuCKzViKoMm+TMjaT
aohc2etW7JiJAkaqkmsnOfT8wlLphMyNKIDPM1morlXqHEGhUoQH6f39IZ68Vqi7
r8F/yDU5+w/4/FX7b637n9GqEOI2XUJ58l94hrJbTW57HaRguGaQzX8tP82etMBe
xAtWYnPA/buoU6R7C81XGXwMkYA9ca+P71fGrXrGt11vrU57hQR2ZHBySSRw6blA
vbUKOHyLepEvJuF5ceXtUHUBw3z2pEjfpt/NT5XHY7YJgSd9Q5I=
=EuLT
-----END PGP SIGNATURE-----

--2NovYXgAkcdQF9sh--