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