On 11/16/22 10:09 PM, Michael Paquier wrote:
> On Thu, Nov 10, 2022 at 11:14:34PM -0500, Jonathan S. Katz wrote:
>> On 10/31/22 8:56 PM, Michael Paquier wrote:
>>> Well, one could pass a salt based on something generated by random()
>>> to emulate what we currently do in the default case, as well. The
>>> salt length is an extra possibility, letting it be randomly generated
>>> by pg_strong_random().
>>
>> Sure, this is a good point. From a SQL level we can get that from pgcrypto
>> "gen_random_bytes".
>
> Could it be something we could just push into core? FWIW, I've used
> that quite a bit in the last to cheaply build long random strings of
> data for other things. Without pgcrypto, random() with
> generate_series() has always been kind of.. fun.
I would be a +1 for moving that into core, given we did something
similar with gen_random_uuid[1]. Separate patch, of course :)
> +SELECT scram_build_secret_sha256(NULL);
> +ERROR: password must not be null
> +SELECT scram_build_secret_sha256(NULL, NULL);
> +ERROR: password must not be null
> +SELECT scram_build_secret_sha256(NULL, NULL, NULL);
> +ERROR: password must not be null
>
> This is just testing three times the same thing as per the defaults.
> I would cut the second and third cases.
AFAICT it's not returning the defaults. Quick other example:
CREATE FUNCTION ab (a int DEFAULT 0) RETURNS int AS $$ SELECT a; $$
LANGUAGE SQL;
SELECT ab();
ab
----
0
(1 row)
SELECT ab(NULL::int);
ab
----
(1 row)
Given scram_build_secret_sha256 is not a strict function, I'd prefer to
test all of the NULL cases in case anything in the underlying code
changes in the future. It's a cheap cost to be a bit more careful.
> git diff --check reports some whitespaces.
Ack. Will fix on the next pass. (I've been transitioning editors, which
could have resulted in that),
> scram_build_secret_sha256_internal() is missing SASLprep on the
> password string. Perhaps the best thing to do here is just to extend
> pg_be_scram_build_secret() with more arguments so as callers can
> optionally pass down a custom salt with its length, leaving the
> responsibility to pg_be_scram_build_secret() to create a random salt
> if nothing has been given?
Ah, good catch!
I think if we go with passing down the salt, we'd also have to allow for
the passing down of the iterations, too, and we're close to rebuilding
"scram_build_secret". I'll stare a bit at this on the next pass and
either 1/ just SASLprep the string in the new
"scram_build_secret_sha256_internal" func, or 2/ change the definition
of "pg_be_scram_build_secret" to accommodate more overrides.
Thanks,
Jonathan
[1] https://www.postgresql.org/docs/current/functions-uuid.html
