> On 27 Nov 2022, at 06:21, Jonathan S. Katz <jk...@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/