Feedback-ID: ia2694551:Fastmail
Date: Mon, 13 Jan 2025 17:06:16 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Bernd Helmle <mailings@oopsware.de>
Cc: Japin Li <japinli@hotmail.com>, 
	PostgreSQL Development <pgsql-hackers@postgresql.org>
Subject: Re: Modern SHA2- based password hashes for pgcrypto
Message-ID: <202501131606.h6ctz533qhy6@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7bde94157f8357a59a9d6de1f74a1b6d7705c5b0.camel@oopsware.de>
Archived-At: <https://www.postgresql.org/message-id/202501131606.h6ctz533qhy6%40alvherre.pgsql>
Precedence: bulk

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 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.

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.


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.

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.

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

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")