On 11/26/22 2:53 PM, Jonathan S. Katz wrote:
> On 11/16/22 10:09 PM, Michael Paquier wrote:
>> git diff --check reports some whitespaces.
>
> Ack. Will fix on the next pass. (I've been transitioning editors, which
> could have resulted in that),
Fixed (and have run that check subsequently).
>> 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.
In the end I went with your suggested approach as it limited the amount
of code duplication. I did keep in all the permutations of the tests as
it did help me catch an error in my code that let to a panic.
As this seems to be closer to completion, I did include docs in this
patch. I added this function as part of the "string functions" section
of the docs as "md5" was already there. If we continue to add more
authentication helper functions, perhaps we should consider breaking
those out into their own documentation section.
Thanks,
Jonathan