Re: User functions for building SCRAM secrets

[Jonathan S. Katz]

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:



SELECT ab();
 ab
----
  0
(1 row)

SELECT ab(NULL::int);
 ab
----

(1 row)

CREATE FUNCTION ab (a int DEFAULT 0) RETURNS int AS $$ SELECT a; $$ LANGUAGE SQL; 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!



Thanks,

Jonathan

[1] https://www.postgresql.org/docs/current/functions-uuid.html

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.

OpenPGP_signature
Description: OpenPGP digital signature