Re: Raising the SCRAM iteration count

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 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].
>
> I do have mixed feelings on the 'x1=y1,x2=y2' style GUC, but we do have
> 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).
>
> [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 sensitive?
>
> 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.
>
> 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)
>
> 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") == 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?

What would that change? This needs an equal match.

conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_iterations = 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] = '\0';

*salt = encoded_salt;
- *iterations = SCRAM_DEFAULT_ITERATIONS;
+ *iterations = 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