Re: Raising the SCRAM iteration count

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

>>> So, you mean that the GUC should be named like password_iterations,
>>> taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?
>> 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.
>
> 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.

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

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.
> */
>
> 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)
>
> 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") == 0)
> + {
> + conn->scram_iterations = atoi(value);
> + }
>
> 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/