Re: Modern SHA2- based password hashes for pgcrypto

Hi Alvaro,

Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera:
> Hello
>
> I was passing by and I noticed that this needs badly pgindent, and
> some
> comments are enumerations that would lose formatting once through it.
> For example, this would happen which is not good:
>
>     /*
> -    * 1. Start digest A
> -    * 2. Add the password string to digest A
> -    * 3. Add the salt to digest A
> +    * 1. Start digest A 2. Add the password string to digest A 3.
> Add the
> +    * salt to digest A
>      */

I didn't think about testing pg_ident, sorry.

>
> I suggest you can fix this by adding a "-" sign to the opening "/*"
> line
> so that pgindent doesn't mangle it (so it becomes /*- ).  This
> appears
> in several places.

Will try.

>
> It's been said in my presence that pgcrypto is obsolete and shouldn't
> be
> used anymore.  I'm not sure I believe that, but even if that's true,
> it's clear that there's plenty of people who has an interest on it,
> so I
> don't see that as an objection to reject this work.  So let's move
> on.
>

Oh, that's news to me. Is there a plan for it somewhere? I agree that
pgcrypto is widley used and needs a proper replacement when we decide
to deprecate it.

>
> Your test data (crypt-shacrypt.sql) looks a bit short.  I noticed
> that
> Drepper's SHA-crypt.txt file has a bunch of test lines (starting with
> the "Test vectors from FIPS 180-2: appendix B.1." comment line, as
> well
> as "appendix C.1" at the bottom) which perhaps could be incorporated
> into the .sql script, to ensure correctness (or at least,
> bug-compatibility with the reference implementation).  I'd also add a
> note that Drepper's implementation is public domain in crypt-sha.c's
> license block.

Good idea. Will do.

>
> I think the "ascii_dollar" variable is a bit useless.  Why not just
> use the
> literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding
> a

> magic value there)?  Also, I wonder if it would be better to use a
> StringInfo instead of a fixed-size buffer, which would probably make
> some string manipulations easier ... Or maybe not, but let's not mix
> strlcat calls with strncat calls with no comments about why.
>

I am going to give it a try, probably helps to get rid of the
strncat()/strlcat() mess.

I originally thought about StringInfo but went with just the fixed
length character buffers since we access them directly anyways (and the
px_*/OpenSSL API needs char * ).

> Some of your elog(ERROR)s should probably be ereport(), and I'm not
> sure
> we want all the elog(DEBUG1)s.
>

I added them during development. I am not married to them, but found
them useful during testing. If we come to the conclusion they're not
really that important, i drop them entirely.

Thanks for your comments!

Bernd