On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
On 14 Dec 2022, at 02:00, Michael Paquier <mich...@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
OpenPGP_signature
Description: OpenPGP digital signature
