|From:||"Jonathan S(dot) Katz" <jkatz(at)postgresql(dot)org>|
|To:||Daniel Gustafsson <daniel(at)yesql(dot)se>, Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: Raising the SCRAM iteration count|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
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).
> 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:
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
- * 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
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
+ 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?
|Next Message||Tom Lane||2022-12-14 19:02:58||Re: wake up logical workers after ALTER SUBSCRIPTION|
|Previous Message||Nikita Malakhov||2022-12-14 18:56:12||Re: collect_corrupt_items_vacuum.patch|