Re: Raising the SCRAM iteration count

On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>> On 15 Dec 2022, at 00:52, Michael Paquier <michael(at)paquier(dot)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