Re: User functions for building SCRAM secrets

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