> On 17 Dec 2022, at 04:27, Michael Paquier <mich...@paquier.xyz> wrote: > > On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote: >>> On 15 Dec 2022, at 00:52, Michael Paquier <mich...@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.
ok, renamed. >>> @@ -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? Fixed. > + time of encryption. In order to make use of a changed value, new > + password must be set. > "A new password must be set". Fixed. > Superuser-only GUCs should be documented as such, or do you intend to > make it user-settable like I suggested upthread :) ? I don't really have strong feelings, so I reverted to being user-settable since I can't really present a strong argument for superuser-only. The attached is a rebase on top of master with no other additional hacking done on top of the above review comments. -- Daniel Gustafsson
v4-0001-Make-SCRAM-iteration-count-configurable.patch
Description: Binary data