Re: [PATCH v5 10/12] Add a base implementation of SHA-256 support

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 5, 2018, 11:39 UTC
Message-ID: <87h8gv7nz1.fsf@evledraar.gmail.com>
In-Reply-To: <20181104234458.139223-11-sandals@crustytoothpaste.net>

On Sun, Nov 04 2018, brian m. carlson wrote:

Show 5 quoted lines

> SHA-1 is weak and we need to transition to a new hash function.  For
> some time, we have referred to this new function as NewHash.  Recently,
> we decided to pick SHA-256 as NewHash.  The reasons behind the choice of
> SHA-256 are outlined in the thread starting at [1] and in the commit
> history for the hash function transition document.

Nit: In some contradiction now to what's said in
hash-function-transition.txt, see 5988eb631a ("doc
hash-function-transition: clarify what SHAttered means", 2018-03-26).

> +	{
> +		"sha256",
> +		/* "s256", big-endian */

The existing entry/comment for sha1 is:

		"sha1",
		/* "sha1", big-endian */

So why the sha256/s256 difference in the code/comment? Wondering if I'm missing something and we're using "s256" for something.

>  const char *empty_tree_oid_hex(void)
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> [...]

I had a question before about whether we see ourselves perma-forking this implementation based off libtomcrypt, as I recall you said yes.

Still, I think it would be better to introduce this in at least two-four commits where the upstream code is added as-is, then trimmed down to size, then adapted to our coding style, and finally we add our own utility functions.

It'll make it easier to forward-port any future upstream changes.

Show 5 quoted lines

> +	perl -E "for (1..100000) { print q{aaaaaaaaaa}; }" | \
> +		test-tool sha256 >actual &&
> +	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
> +	perl -E "for (1..100000) { print q{abcdefghijklmnopqrstuvwxyz}; }" | \
> +		test-tool sha256 >actual &&

I've been wanting to make use depend on perl >= 5.10 (previous noises about that on-list), but for now we claim to support >=5.8, which doesn't have the -E switch.

But most importantly you aren't even using -E features here, and this isn't very idoimatic Perl. Instead do, respectively:

    perl -e 'print q{aaaaaaaaaa} x 100000'
    perl -e "print q{abcdefghijklmnopqrstuvwxyz} x 100000"