Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\))
Subject: Re: Raising the SCRAM iteration count
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <869f3aee-5c46-998d-be42-b3b232415bef@postgresql.org>
Date: Thu, 15 Dec 2022 12:09:01 +0100
Cc: Michael Paquier <michael@paquier.xyz>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <93D5D4F1-F375-4B1E-84FE-B79FCC813DAB@yesql.se>
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>
To: "Jonathan S. Katz" <jkatz@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/93D5D4F1-F375-4B1E-84FE-B79FCC813DAB%40yesql.se>
Precedence: bulk

> On 14 Dec 2022, at 19:59, Jonathan S. Katz <jkatz@postgresql.org> =
wrote:
> On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>>> On 14 Dec 2022, at 02:00, Michael Paquier <michael@paquier.xyz> =
wrote:

>>> So, you mean that the GUC should be named like password_iterations,
>>> taking a grammar with a list like 'scram-sha-256=3D4096,algo2=3D5000'?=

>> 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 all 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].

Note that this draft is very far from RFC status, it has alredy expired =
twice
and hasn't been updated for a year.  The SCRAM-SHA-512 draft has an =
almost
identical history and neither are assigned a work group.  The author is =
also
drafting scram-bis which is setting up more context around these =
proposals,
this has yet to expire but is also very early.  The work on SCRAM-2FA =
seems the
most promising right now.

There might be additional versions of SCRAM published but it's looking =
pretty
distant now.

> Reviewing patch as is.

Thanks for review! Fixes coming downthread in an updated version.

> =3D=3Dsnip=3D=3D
> The number of computational iterations to perform when generating
> a SCRAM-SHA-256 secret. The default is <literal>4096</literal>. A
> higher number of iterations provides additional protection against
> brute-force attacks on stored passwords, but makes authentication
> slower. Changing the value has no effect on previously created
> SCRAM-SHA-256 secrets as the iteration count at the time of creation
> is fixed. A password must be re-hashed to use an updated iteration
> value.
> =3D=3Dsnip=3D=3D

I've rewritten to a version of this.  We don't use the terminology =
"SCRAM
secret" anywhere else so I used password instead.

> /*
> - * Default number of iterations when generating secret.  Should be at =
least
> - * 4096 per RFC 7677.
> + * Default number of iterations when generating secret.
>  */
>=20
> I don't think we should remove the RFC 7677 reference entirely.

Fixed.

> -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.

Not sure driver authors should be relying on this function..  Code scans
doesn't turn up any public consumers of it right now at least.  If we =
want to
support multiple SCRAM versions we'd still need to change it though as =
noted
downthread.

> +	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?

strncmp() would allow scram_sha256_iterations_foo to match, which we =
don't
want, we want an exact match.

--
Daniel Gustafsson		https://vmware.com/