Re: Raising the SCRAM iteration count - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Raising the SCRAM iteration count
Date
Msg-id Y503IsOdgSrUP6Se@paquier.xyz
Whole thread Raw
In response to Re: Raising the SCRAM iteration count  (Daniel Gustafsson <daniel@yesql.se>)
Responses Re: Raising the SCRAM iteration count  (Daniel Gustafsson <daniel@yesql.se>)
List pgsql-hackers
On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>> On 15 Dec 2022, at 00:52, Michael Paquier <michael@paquier.xyz> wrote:
>>    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?
>
> Distinct members in the conn object is only of interest if there is a way for
> the user to select a different password method in \password right?  I can
> rename it now but I think doing too much here is premature, awaiting work on
> \password (should that materialize) seems reasonable no?

You could do that already, somewhat indirectly, with
password_encryption, assuming that it supports more than one mode
whose password build is influenced by it.  If you wish to keep it
named this way, this is no big deal for me either way, so feel free to
use what you think is best based on the state of HEAD.  I think that
I'd value more the consistency with the backend in terms of naming,
though.

>> @@ -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.
>
> For avoiding revealing anything, I think a case can be argued for both.  I've
> reverted back to the default though.
>
> I also renamed the GUC sha_256 to match terminology we use.

+   "SET password_encryption='scram-sha-256';
+    SET scram_sha_256_iterations=100000;
Maybe use a lower value to keep the test cheap?

+        time of encryption. In order to make use of a changed value, new
+        password must be set.
"A new password must be set".

Superuser-only GUCs should be documented as such, or do you intend to
make it user-settable like I suggested upthread :) ?
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Refactor SCRAM code to dynamically handle hash type and key length
Next
From: Nathan Bossart
Date:
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX