> On 27 Nov 2022, at 06:21, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> 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).
The spaces-before-tabs that git is complaining about are gone but there are
still whitespace issues like scram_build_secret_sha256() which has a mix of
two-space and tab indentation. I recommend taking it for a spin with pgindent.
Sorry for not noticing the thread earlier, below are some review comments and
+ SCRAM secret equilvaent to what is stored in
s/equilvaent/equivalent/
+ <literal>SELECT scram_build_secret_sha256('secret password', '\xabba5432');</literal>
+ <returnvalue></returnvalue>
+<programlisting>
+
SCRAM-SHA-256$4096:q7pUMg==$05Nb9QHwHkMA0CRcYaEfwtgZ+3kStIefz8fLMjTEtio=:P126h1ycyP938E69yxktEfhoAILbiwL/UMsMk3Efb6o=
+</programlisting>
Shouldn't the function output be inside <returnvalue></returnvalue>? IIRC the
use if <programlisting> with an empty <returnvalue> is for multiline output,
but I'm not 100% sure there.
+ if (iterations <= 0)
+ iterations = SCRAM_DEFAULT_ITERATIONS;
According to the RFC, the iteration-count "SHOULD be at least 4096", so we can
reduce it, but do we gain anything by allowing users to set it lower? If so,
scram_build_secret() is already converting (iterations <= 0) to the default so
there is no need to duplicate that logic.
Personally I'd prefer if we made 4096 the minimum and only allowed higher
regardless of the fate of this patch, but thats for another thread.
+ Assert(secret != NULL);
I don't think there are any paths where this is possible to trigger, did you
see any?
On the whole I tend to agree with Jacob upthread, while this does provide
consistency it doesn't seem to move the needle for best practices. Allowing
scram_build_secret_sha256('password', 'a', 1); with the password potentially
going in cleartext over the wire and into the logs doesn't seem like a great
tradeoff for the (IMHO) niche usecases it would satisfy.
--
Daniel Gustafsson https://vmware.com/
