Re: Raising the SCRAM iteration count

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:
>>
>> On Tue, Dec 13, 2022 at 12:17:58PM +0100, Daniel Gustafsson wrote:
>>> It does raise an interesting point though, if we in the future add suppprt for
>>> SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
>>> single GUC for SCRAM iterations; we'd need to be able to set the iteration
>>> count per algorithm. I'll account for that when updating the patch downthread.
>>
>> 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].

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.

(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/

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

> The default in this version is rolled back to 4096 as there was pushback
> against raising it, and the lower limit is one in order to potentially assist
> situations like the one Andres mentioned where md5 is used.

Reviewing patch as is.

Suggestion on text:

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

/*
- * 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. Perhaps:

/*
* Default number of iterations when generating secret. RFC 7677
* recommend 4096 for SCRAM-SHA-256, which we set as the default,
* but we allow users to select their own values.
*/

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

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

Thanks,

Jonathan