On Mon, 20 Jan 2025 at 18:41, Bernd Helmle <maili...@oopsware.de> wrote:
> Hi,
>
> Please find attached a reworked patch according Alvaro's comments.
>
> 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 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.
>>
>
> This is even documented:
>
> https://www.postgresql.org/docs/devel/source-format.html
>
> I ran pgindent locally and verified it works like you suggested.
>>
>
> [...]
>
>> 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.
>>
>
> These FIPS cases tests explicit against sha{256|512}_process_bytes() in
> Drepper's code, which seem to be the equivalent to OpenSSL's
> EVP_Digest*() API we're using. I am not sure it makes sense to adapt
> them.
>
> I left them out for now but adapted all the testcases for the password
> hashes defined in the 2nd test cases to make sure we create the same
> hashes.
>
>> 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.
>
> Result buffer via out_buf is wrapped into a StringInfo now.
>
>>
>> Some of your elog(ERROR)s should probably be ereport(), and I'm not
>> sure
>> we want all the elog(DEBUG1)s.
>
> Done, but i kept some of the DEBUG output for now. I am not sure if i
> really should set the errcode for ereport() explicitly, but i did on
> some places where i thought it might be useful.
>
> This patch version also changes the original behavior of crypt() when
> called for sha{256|512}crypt hashes. Before we didn't accept values for
> the rounds parameter exceeding the minimum and maximum values. This was
> along with gen_salt(), which also errors out in such cases. But
> Drepper's code accepts them and changes them behind the scenes either
> to the minimum or maximum and calculates the password hash based on
> them, depending on the exceeded boundary.
>
> So when passing a user defined salt string to crypt(), we do the same
> now but i added a NOTICE to indicate that we changed the user submitted
> parameter behind the scene. This also enables us to stress
> px_crypt_shacrypt() with the same test cases as Drepper's code.
>
>
Hi, Bernd Helmle
Thanks for updating the patch. Here are some comments for v3 patch.
1.
+char *
+_crypt_gensalt_sha256_rn(unsigned long count,
+ const char *input, int size,
+ char *output, int output_size)
+{
+ memset(output, 0, output_size);
+ /* set magic byte for sha512crypt */
s/sha512crypt/sha256crypt/g
2.
Both PX_SHACRYPT_BUF_LEN and PX_SHACRYPT_DIGEST_MAX_LENGTH are macros for
length; however, they use different suffixes. How about unifying them?
I prefer to use the LEN suffix.
3.
+ if ((dec_salt_binary[0] != '$')
+ && (dec_salt_binary[2] != '$'))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid format of salt")),
+ errhint("magic byte format for shacrypt is
either \"$5$\" or \"$6$\""));
+ }
...
+ if (strncmp(dec_salt_binary, magic_bytes[0], strlen(magic_bytes[0])) ==
0)
...
+ else if (strncmp(dec_salt_binary, magic_bytes[1],
strlen(magic_bytes[1])) == 0)
IMO, we could change the above code as:
+ if (strncmp(dec_salt_binary, magic_bytes[0], strlen(magic_bytes[0])) ==
0)
...
+ else if (strncmp(dec_salt_binary, magic_bytes[1],
strlen(magic_bytes[1])) == 0)
...
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid format of salt")),
+ errhint("magic byte format for shacrypt is
either \"$5$\" or \"$6$\""));
+ }
4.
+/* Default number of rounds of shacrypt if not explicitly specified. */
+#define PX_SHACRYPT_ROUNDS_DEFAULT 5000
It seems you were missing the `l` suffix, since both PX_SHACRYPT_ROUNDS_MIN and
PX_SHACRYPT_ROUNDS_MAX have this suffix.
5.
Does the following work as expected?
postgres=# select crypt('hello', '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
crypt
-------------------------------------------------
$5$$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
(1 row)
postgres=# select crypt('hello', '$5$rounds=10000$/Zg436s2vmTwsoSz');
crypt
------------------------------------------------------------------------------
$5$rounds=10000$/Zg436s2vmTwsoSz$N4Ad0oGzE6ak30z4EGSEXOc2OXQOpmauicPT3560lY2
(1 row)
According to the documentation, I think the first should output as following:
$5$rounds=5000$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
--
Regrads,
Japin Li
