[PATCH] sha256: add support for Nettle

[PATCH] sha256: add support for Nettle

[brian m. carlson]

For SHA-256, we currently have support for OpenSSL and libgcrypt because
these two libraries contain optimized implementations that can take
advantage of native processor instructions.  However, OpenSSL is not
suitable for linking against for Linux distros due to licensing
incompatibilities with the GPLv2, and libgcrypt has been less favored
by cryptographers due to some security-related implementation issues.

Let's add another option that's compatible with the GPLv2, which is
Nettle.  It also has recently gained support for Intel's SHA-NI
instructions, which compare very favorably to other implementations.
For example, using this implementation and SHA-1 DC on a machine with
SHA-NI, hashing a 2 GiB file with SHA-1 takes 7.582 seconds, while
hashing the same file with SHA-256 takes 2.278 seconds.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Makefile        |  7 +++++++
 hash.h          |  4 +++-
 sha256/nettle.h | 28 ++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 sha256/nettle.h

diff --git a/Makefile b/Makefile
index 04d0fd1fe6..ce99aecc31 100644
--- a/Makefile
+++ b/Makefile
@@ -182,6 +182,8 @@ include shared.mak
 #
 # Define BLK_SHA256 to use the built-in SHA-256 routines.
 #
+# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
+#
 # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
 #
 # Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
@@ -1842,6 +1844,10 @@ ifdef OPENSSL_SHA256
 	EXTLIBS += $(LIB_4_CRYPTO)
 	BASIC_CFLAGS += -DSHA256_OPENSSL
 else
+ifdef NETTLE_SHA256
+	BASIC_CFLAGS += -DSHA256_NETTLE
+	EXTLIBS += -lnettle
+else
 ifdef GCRYPT_SHA256
 	BASIC_CFLAGS += -DSHA256_GCRYPT
 	EXTLIBS += -lgcrypt
@@ -1850,6 +1856,7 @@ else
 	BASIC_CFLAGS += -DSHA256_BLK
 endif
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
 	LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index 5d40368f18..ea87ae9d92 100644
--- a/hash.h
+++ b/hash.h
@@ -16,7 +16,9 @@
 #include "block-sha1/sha1.h"
 #endif
 
-#if defined(SHA256_GCRYPT)
+#if defined(SHA256_NETTLE)
+#include "sha256/nettle.h"
+#elif defined(SHA256_GCRYPT)
 #define SHA256_NEEDS_CLONE_HELPER
 #include "sha256/gcrypt.h"
 #elif defined(SHA256_OPENSSL)
diff --git a/sha256/nettle.h b/sha256/nettle.h
new file mode 100644
index 0000000000..9b2845babc
--- /dev/null
+++ b/sha256/nettle.h
@@ -0,0 +1,28 @@
+#ifndef SHA256_GCRYPT_H
+#define SHA256_GCRYPT_H
+
+#include <nettle/sha2.h>
+
+typedef struct sha256_ctx nettle_SHA256_CTX;
+
+inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
+{
+	sha256_init(ctx);
+}
+
+inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
+{
+	sha256_update(ctx, len, data);
+}
+
+inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
+{
+	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
+}
+
+#define platform_SHA256_CTX nettle_SHA256_CTX
+#define platform_SHA256_Init nettle_SHA256_Init
+#define platform_SHA256_Update nettle_SHA256_Update
+#define platform_SHA256_Final nettle_SHA256_Final
+
+#endif

Re: [PATCH] sha256: add support for Nettle

[Ramsay Jones]

On Tue, Jul 05, 2022 at 11:05:18PM +0000, brian m. carlson wrote:

[snip]

> diff --git a/sha256/nettle.h b/sha256/nettle.h
> new file mode 100644
> index 0000000000..9b2845babc
> --- /dev/null
> +++ b/sha256/nettle.h
> @@ -0,0 +1,28 @@
> +#ifndef SHA256_GCRYPT_H
> +#define SHA256_GCRYPT_H

I'm guessing you had intended: s/GCRYPT/NETTLE/ here. :)

ATB,
Ramsay Jones

> +
> +#include <nettle/sha2.h>
> +
> +typedef struct sha256_ctx nettle_SHA256_CTX;
> +
> +inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
> +{
> +	sha256_init(ctx);
> +}
> +
> +inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
> +{
> +	sha256_update(ctx, len, data);
> +}
> +
> +inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
> +{
> +	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
> +}
> +
> +#define platform_SHA256_CTX nettle_SHA256_CTX
> +#define platform_SHA256_Init nettle_SHA256_Init
> +#define platform_SHA256_Update nettle_SHA256_Update
> +#define platform_SHA256_Final nettle_SHA256_Final
> +
> +#endif

Re: [PATCH] sha256: add support for Nettle

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On 2022-07-06 at 01:00:29, Ramsay Jones wrote:
> On Tue, Jul 05, 2022 at 11:05:18PM +0000, brian m. carlson wrote:
> 
> [snip]
> 
> > diff --git a/sha256/nettle.h b/sha256/nettle.h
> > new file mode 100644
> > index 0000000000..9b2845babc
> > --- /dev/null
> > +++ b/sha256/nettle.h
> > @@ -0,0 +1,28 @@
> > +#ifndef SHA256_GCRYPT_H
> > +#define SHA256_GCRYPT_H
> 
> I'm guessing you had intended: s/GCRYPT/NETTLE/ here. :)

Ah, yes.  I'll see if anyone has other comments and then send out a v2.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] sha256: add support for Nettle

[Jeff King]

On Tue, Jul 05, 2022 at 11:05:18PM +0000, brian m. carlson wrote:

> Let's add another option that's compatible with the GPLv2, which is
> Nettle.  It also has recently gained support for Intel's SHA-NI
> instructions, which compare very favorably to other implementations.
> For example, using this implementation and SHA-1 DC on a machine with
> SHA-NI, hashing a 2 GiB file with SHA-1 takes 7.582 seconds, while
> hashing the same file with SHA-256 takes 2.278 seconds.

Nifty. I was curious about the speed on my machine, since I still had
that sha256 import of linux.git. Sadly, it's not nearly as impressive:

  [blk_sha256]
  $ time git index-pack --verify objects/pack/*.pack
  real	2m40.164s
  user	12m30.626s
  sys	0m31.567s

  [nettle]
  $ time git index-pack --verify objects/pack/*.pack
  real	2m21.346s
  user	10m14.507s
  sys	0m30.943s

which I take to mean that it doesn't support the Intel SHA extensions
(and /proc/cpuinfo confirms that). Still, 12-18% speedup is a nice and
easy win.

>  Makefile        |  7 +++++++
>  hash.h          |  4 +++-
>  sha256/nettle.h | 28 ++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
>  create mode 100644 sha256/nettle.h

The patch itself looks good to me, modulo the guard typo already pointed
out.

-Peff

Re: [PATCH] sha256: add support for Nettle

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 05 2022, brian m. carlson wrote:

Do any of those security issues in libcrypt have to do with the narrow
"hash stuff" part of the API we're using?

> Let's add another option that's compatible with the GPLv2, which is
> Nettle.  It also has recently gained support for Intel's SHA-NI
> instructions, which compare very favorably to other implementations.
> For example, using this implementation and SHA-1 DC on a machine with
> SHA-NI, hashing a 2 GiB file with SHA-1 takes 7.582 seconds, while
> hashing the same file with SHA-256 takes 2.278 seconds.

Interesting, but as Jeff notes downthread we really should be comparing
the different sha256 backends for speed, i.e. the interesting thing
isn't sha1dc v.s. this sha256 ,but if this sha256 backend in particular
offers performance benefits.

> -#if defined(SHA256_GCRYPT)
> +#if defined(SHA256_NETTLE)
> +#include "sha256/nettle.h"
> +#elif defined(SHA256_GCRYPT)
>  #define SHA256_NEEDS_CLONE_HELPER
>  #include "sha256/gcrypt.h"
>  #elif defined(SHA256_OPENSSL)
> diff --git a/sha256/nettle.h b/sha256/nettle.h
> new file mode 100644
> index 0000000000..9b2845babc
> --- /dev/null
> +++ b/sha256/nettle.h
> @@ -0,0 +1,28 @@
> +#ifndef SHA256_GCRYPT_H
> +#define SHA256_GCRYPT_H
> +
> +#include <nettle/sha2.h>
> +
> +typedef struct sha256_ctx nettle_SHA256_CTX;
> +
> +inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
> +{
> +	sha256_init(ctx);
> +}
> +
> +inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)

Needs a line wrap;

> +{
> +	sha256_update(ctx, len, data);
> +}
> +
> +inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
> +{
> +	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
> +}
> +
> +#define platform_SHA256_CTX nettle_SHA256_CTX
> +#define platform_SHA256_Init nettle_SHA256_Init
> +#define platform_SHA256_Update nettle_SHA256_Update
> +#define platform_SHA256_Final nettle_SHA256_Final
> +
> +#endif

Would it be viable / at all sane to embed the part of the library we
need in our sources, similar to what we do for sha1dc? Or perhaps it's
not worth it at all...

Re: [PATCH] sha256: add support for Nettle

[Jeff King]

On Wed, Jul 06, 2022 at 10:45:06AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Would it be viable / at all sane to embed the part of the library we
> need in our sources, similar to what we do for sha1dc? Or perhaps it's
> not worth it at all...

I doubt it's worth it. It's a big library with tons of algorithms, most
of which we won't need. And the implementation has lots of asm and
platform-specific knobs. I wouldn't want to try extracting any of that
from their autoconf file, nor putting (more) autoconf inside our
repository.

For sha1dc, I think including a vendored copy was important for us
making it the default, and we wanted to do that for the security
implications. A 12% speedup is OK to leave on the able for the default
build, and people can easily link against the system libnettle if they
care enough. And other linkable implementations are in the same boat;
openssl is even faster than libnettle on my machine.

-Peff

Re: [PATCH] sha256: add support for Nettle

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]

On 2022-07-06 at 09:23:18, Jeff King wrote:
> On Wed, Jul 06, 2022 at 10:45:06AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > Would it be viable / at all sane to embed the part of the library we
> > need in our sources, similar to what we do for sha1dc? Or perhaps it's
> > not worth it at all...
> 
> I doubt it's worth it. It's a big library with tons of algorithms, most
> of which we won't need. And the implementation has lots of asm and
> platform-specific knobs. I wouldn't want to try extracting any of that
> from their autoconf file, nor putting (more) autoconf inside our
> repository.

Yeah, it requires doing a CPUID check on different platforms to
determine which CPU to use at runtime.  Depending on platform, CPUID
isn't always available, and sometimes you end up needing to use a
different method, which we wouldn't want to port here.

If we just want a basic option, we have that in the block SHA-256
algorithm, which is probably no better or worse than Nettle's
implementation.

> For sha1dc, I think including a vendored copy was important for us
> making it the default, and we wanted to do that for the security
> implications. A 12% speedup is OK to leave on the able for the default
> build, and people can easily link against the system libnettle if they
> care enough. And other linkable implementations are in the same boat;
> openssl is even faster than libnettle on my machine.

Right.  I'm not surprised that OpenSSL is faster here, and that's
expected, since OpenSSL tends to outperform other libraries.  Even with
SHA-NI extensions, it's slightly faster still (1.415 s vs. 1.529 s for a
2 GB file), but I'm sure with a small amount of tuning Nettle could
catch up, because it's essentially the same instructions.

For block SHA-256, the time taken for the same file is 7.296 seconds, so
the performance improvement is substantial if you have the extensions
(and maybe even if you don't since both Nettle and OpenSSL have SIMD
implementations as well). That's why I wanted to send this patch: I'd
like to see if Debian can link against libnettle in the future,
especially since it's already in at least some binaries due to GnuTLS.

For SHA-1 DC, we definitely want it to be the default for security, so
vendoring a copy matters.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] sha256: add support for Nettle

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/hash.h b/hash.h
> index 5d40368f18..ea87ae9d92 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -16,7 +16,9 @@
>  #include "block-sha1/sha1.h"
>  #endif
>  
> -#if defined(SHA256_GCRYPT)
> +#if defined(SHA256_NETTLE)
> +#include "sha256/nettle.h"
> +#elif defined(SHA256_GCRYPT)
>  #define SHA256_NEEDS_CLONE_HELPER
>  #include "sha256/gcrypt.h"
>  #elif defined(SHA256_OPENSSL)

When it does not make any semantic difference, it is preferrable to
add a new thing after existing things.  But this sequence is meant
to list them in the order of preference when multiple choices are
availble, so it is OK to prepend nettle IF our intention is to favor
it over all others.

I am OK with that design choice, and I think the first paragraph of
the proposed log message adequately justifies why, but I'd prefer to
see it a bit more explicitly stated in the log message.

> For SHA-256, we currently have support for OpenSSL and libgcrypt because
> these two libraries contain optimized implementations that can take
> advantage of native processor instructions.  However, OpenSSL is not
> suitable for linking against for Linux distros due to licensing
> incompatibilities with the GPLv2, and libgcrypt has been less favored
> by cryptographers due to some security-related implementation issues.
>
> Let's add another option that's compatible with the GPLv2, which is
> Nettle.  It also has recently gained support for Intel's SHA-NI
> instructions, which compare very favorably to other implementations.
> For example, using this implementation and SHA-1 DC on a machine with
> SHA-NI, hashing a 2 GiB file with SHA-1 takes 7.582 seconds, while
> hashing the same file with SHA-256 takes 2.278 seconds.

Perhaps "Let's add another option ..., which is Nettle, and give it
preference over all others when multiple libraries are availalble"
or something along that line?

> diff --git a/sha256/nettle.h b/sha256/nettle.h
> new file mode 100644
> index 0000000000..9b2845babc
> --- /dev/null
> +++ b/sha256/nettle.h
> @@ -0,0 +1,28 @@
> +#ifndef SHA256_GCRYPT_H
> +#define SHA256_GCRYPT_H

Not really ;-)

> +
> +#include <nettle/sha2.h>

Re: [PATCH] sha256: add support for Nettle

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps "Let's add another option ..., which is Nettle, and give it
> preference over all others when multiple libraries are availalble"
> or something along that line?
>
>> diff --git a/sha256/nettle.h b/sha256/nettle.h
>> new file mode 100644
>> index 0000000000..9b2845babc
>> --- /dev/null
>> +++ b/sha256/nettle.h
>> @@ -0,0 +1,28 @@
>> +#ifndef SHA256_GCRYPT_H
>> +#define SHA256_GCRYPT_H
>
> Not really ;-)

FWIW, I needed the following to successfully build with
NETTLE_SHA256=YesPlease defined.  The final linkage step complained
about missing nettle_SHA256_{Init,Update,Final}() functions without
the tweak.

diff --git c/sha256/nettle.h w/sha256/nettle.h
index 9b2845babc..8c93f29dda 100644
--- c/sha256/nettle.h
+++ w/sha256/nettle.h
@@ -1,21 +1,21 @@
-#ifndef SHA256_GCRYPT_H
-#define SHA256_GCRYPT_H
+#ifndef SHA256_NETTLE_H
+#define SHA256_NETTLE_H
 
 #include <nettle/sha2.h>
 
 typedef struct sha256_ctx nettle_SHA256_CTX;
 
-inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
+static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
 {
 	sha256_init(ctx);
 }
 
-inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
+static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
 {
 	sha256_update(ctx, len, data);
 }
 
-inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
+static inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
 {
 	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
 }

Re: [PATCH] sha256: add support for Nettle

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> FWIW, I needed the following to successfully build with
> NETTLE_SHA256=YesPlease defined.  The final linkage step complained
> about missing nettle_SHA256_{Init,Update,Final}() functions without
> the tweak.
> ...

Another glitch.

As "make hdr-check" is pretty much indiscriminatory, my build failed
on a box without libnettle-dev (hence /usr/include/nettle/sha2.h
missing).

diff --git a/Makefile b/Makefile
index ce99aecc31..52a9f97997 100644
--- a/Makefile
+++ b/Makefile
@@ -3098,6 +3098,9 @@ $(SP_OBJ): %.sp: %.c %.o
 sparse: $(SP_OBJ)
 
 EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
+ifndef NETTLE_SHA256
+	EXCEPT_HDRS += sha256/nettle.h
+endif
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
diff --git a/sha256/nettle.h b/sha256/nettle.h
index 159239a785..8c93f29dda 100644
--- a/sha256/nettle.h
+++ b/sha256/nettle.h
@@ -5,17 +5,17 @@
 
 typedef struct sha256_ctx nettle_SHA256_CTX;
 
-inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
+static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
 {
 	sha256_init(ctx);
 }
 
-inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
+static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
 {
 	sha256_update(ctx, len, data);
 }
 
-inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
+static inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
 {
 	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
 }

Re: [PATCH] sha256: add support for Nettle

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> FWIW, I needed the following to successfully build with
>> NETTLE_SHA256=YesPlease defined.  The final linkage step complained
>> about missing nettle_SHA256_{Init,Update,Final}() functions without
>> the tweak.
>> ...
>
> Another glitch.
> 
> As "make hdr-check" is pretty much indiscriminatory, my build failed
> on a box without libnettle-dev (hence /usr/include/nettle/sha2.h
> missing).
> ...

Taking all together, here is a copy I have in my tree right now (see
the range diff for what changed since your initial version).

Thanks.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From e8713290975d171764768bea99d686243e23ef8e Mon Sep 17 00:00:00 2001
From: "brian m. carlson" <sandals@crustytoothpaste.net>
Date: Tue, 5 Jul 2022 23:05:18 +0000
Subject: [PATCH] sha256: add support for Nettle

For SHA-256, we currently have support for OpenSSL and libgcrypt because
these two libraries contain optimized implementations that can take
advantage of native processor instructions.  However, OpenSSL is not
suitable for linking against for Linux distros due to licensing
incompatibilities with the GPLv2, and libgcrypt has been less favored
by cryptographers due to some security-related implementation issues.

Let's add another option that's compatible with the GPLv2, which is
Nettle, and give it preference over all others when Nettle and other
choices are possible.  It also has recently gained support for
Intel's SHA-NI instructions, which compare very favorably to other
implementations.  For example, using this implementation and SHA-1
DC on a machine with SHA-NI, hashing a 2 GiB file with SHA-1 takes
7.582 seconds, while hashing the same file with SHA-256 takes 2.278
seconds.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

1:  3bcd13c813 ! 1:  e871329097 sha256: add support for Nettle
    @@ Commit message
         by cryptographers due to some security-related implementation issues.
     
         Let's add another option that's compatible with the GPLv2, which is
    -    Nettle.  It also has recently gained support for Intel's SHA-NI
    -    instructions, which compare very favorably to other implementations.
    -    For example, using this implementation and SHA-1 DC on a machine with
    -    SHA-NI, hashing a 2 GiB file with SHA-1 takes 7.582 seconds, while
    -    hashing the same file with SHA-256 takes 2.278 seconds.
    +    Nettle, and give it preference over all others when Nettle and other
    +    choices are possible.  It also has recently gained support for
    +    Intel's SHA-NI instructions, which compare very favorably to other
    +    implementations.  For example, using this implementation and SHA-1
    +    DC on a machine with SHA-NI, hashing a 2 GiB file with SHA-1 takes
    +    7.582 seconds, while hashing the same file with SHA-256 takes 2.278
    +    seconds.
     
         Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    @@ Makefile: else
      
      ifdef SHA1_MAX_BLOCK_SIZE
      	LIB_OBJS += compat/sha1-chunked.o
    +@@ Makefile: $(SP_OBJ): %.sp: %.c %.o
    + sparse: $(SP_OBJ)
    + 
    + EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
    ++ifndef NETTLE_SHA256
    ++	EXCEPT_HDRS += sha256/nettle.h
    ++endif
    + ifndef GCRYPT_SHA256
    + 	EXCEPT_HDRS += sha256/gcrypt.h
    + endif
     
      ## hash.h ##
     @@
    @@ hash.h
     
      ## sha256/nettle.h (new) ##
     @@
    -+#ifndef SHA256_GCRYPT_H
    -+#define SHA256_GCRYPT_H
    ++#ifndef SHA256_NETTLE_H
    ++#define SHA256_NETTLE_H
     +
     +#include <nettle/sha2.h>
     +
     +typedef struct sha256_ctx nettle_SHA256_CTX;
     +
    -+inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
    ++static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
     +{
     +	sha256_init(ctx);
     +}
     +
    -+inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
    ++static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
     +{
     +	sha256_update(ctx, len, data);
     +}
     +
    -+inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
    ++static inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
     +{
     +	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
     +}



 Makefile        | 10 ++++++++++
 hash.h          |  4 +++-
 sha256/nettle.h | 28 ++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 sha256/nettle.h

diff --git a/Makefile b/Makefile
index 04d0fd1fe6..52a9f97997 100644
--- a/Makefile
+++ b/Makefile
@@ -182,6 +182,8 @@ include shared.mak
 #
 # Define BLK_SHA256 to use the built-in SHA-256 routines.
 #
+# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
+#
 # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
 #
 # Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
@@ -1842,6 +1844,10 @@ ifdef OPENSSL_SHA256
 	EXTLIBS += $(LIB_4_CRYPTO)
 	BASIC_CFLAGS += -DSHA256_OPENSSL
 else
+ifdef NETTLE_SHA256
+	BASIC_CFLAGS += -DSHA256_NETTLE
+	EXTLIBS += -lnettle
+else
 ifdef GCRYPT_SHA256
 	BASIC_CFLAGS += -DSHA256_GCRYPT
 	EXTLIBS += -lgcrypt
@@ -1850,6 +1856,7 @@ else
 	BASIC_CFLAGS += -DSHA256_BLK
 endif
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
 	LIB_OBJS += compat/sha1-chunked.o
@@ -3091,6 +3098,9 @@ $(SP_OBJ): %.sp: %.c %.o
 sparse: $(SP_OBJ)
 
 EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
+ifndef NETTLE_SHA256
+	EXCEPT_HDRS += sha256/nettle.h
+endif
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
diff --git a/hash.h b/hash.h
index 5d40368f18..ea87ae9d92 100644
--- a/hash.h
+++ b/hash.h
@@ -16,7 +16,9 @@
 #include "block-sha1/sha1.h"
 #endif
 
-#if defined(SHA256_GCRYPT)
+#if defined(SHA256_NETTLE)
+#include "sha256/nettle.h"
+#elif defined(SHA256_GCRYPT)
 #define SHA256_NEEDS_CLONE_HELPER
 #include "sha256/gcrypt.h"
 #elif defined(SHA256_OPENSSL)
diff --git a/sha256/nettle.h b/sha256/nettle.h
new file mode 100644
index 0000000000..8c93f29dda
--- /dev/null
+++ b/sha256/nettle.h
@@ -0,0 +1,28 @@
+#ifndef SHA256_NETTLE_H
+#define SHA256_NETTLE_H
+
+#include <nettle/sha2.h>
+
+typedef struct sha256_ctx nettle_SHA256_CTX;
+
+static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
+{
+	sha256_init(ctx);
+}
+
+static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx, const void *data, size_t len)
+{
+	sha256_update(ctx, len, data);
+}
+
+static inline void nettle_SHA256_Final(unsigned char *digest, nettle_SHA256_CTX *ctx)
+{
+	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
+}
+
+#define platform_SHA256_CTX nettle_SHA256_CTX
+#define platform_SHA256_Init nettle_SHA256_Init
+#define platform_SHA256_Update nettle_SHA256_Update
+#define platform_SHA256_Final nettle_SHA256_Final
+
+#endif
-- 
2.37.0-211-gafcdf5f063

[PATCH v2] sha256: add support for Nettle

[brian m. carlson]

For SHA-256, we currently have support for OpenSSL and libgcrypt because
these two libraries contain optimized implementations that can take
advantage of native processor instructions.  However, OpenSSL is not
suitable for linking against for Linux distros due to licensing
incompatibilities with the GPLv2, and libgcrypt has been less favored by
cryptographers due to some security-related implementation issues,
which, while not affecting our use of hash algorithms, has affected its
reputation.

Let's add another option that's compatible with the GPLv2, which is
Nettle.  This is an option which is generally better than libgcrypt
because on many distros GnuTLS (which uses Nettle) is used for HTTPS and
therefore as a practical matter it will be available on most systems.
As a result, prefer it over libgcrypt and our built-in implementation.

Nettle also has recently gained support for Intel's SHA-NI instructions,
which compare very favorably to other implementations, as well as
assembly implementations for when SHA-NI is not available.

A git gc on git.git sees a 12% performance improvement with Nettle over
our block SHA-256 implementation due to general assembly improvements.
With SHA-NI, the performance of raw SHA-256 on a 2 GiB file goes from
7.296 seconds with block SHA-256 to 1.523 seconds with Nettle.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---

Changes from v1:

* Improve commit message to provide better statistics and rationale.
* Fix include guard typo.
* Exclude header in Makefile unless we're building it.

 Makefile        | 10 ++++++++++
 hash.h          |  4 +++-
 sha256/nettle.h | 31 +++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 sha256/nettle.h

diff --git a/Makefile b/Makefile
index 04d0fd1fe6..52a9f97997 100644
--- a/Makefile
+++ b/Makefile
@@ -182,6 +182,8 @@ include shared.mak
 #
 # Define BLK_SHA256 to use the built-in SHA-256 routines.
 #
+# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
+#
 # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
 #
 # Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
@@ -1842,6 +1844,10 @@ ifdef OPENSSL_SHA256
 	EXTLIBS += $(LIB_4_CRYPTO)
 	BASIC_CFLAGS += -DSHA256_OPENSSL
 else
+ifdef NETTLE_SHA256
+	BASIC_CFLAGS += -DSHA256_NETTLE
+	EXTLIBS += -lnettle
+else
 ifdef GCRYPT_SHA256
 	BASIC_CFLAGS += -DSHA256_GCRYPT
 	EXTLIBS += -lgcrypt
@@ -1850,6 +1856,7 @@ else
 	BASIC_CFLAGS += -DSHA256_BLK
 endif
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
 	LIB_OBJS += compat/sha1-chunked.o
@@ -3091,6 +3098,9 @@ $(SP_OBJ): %.sp: %.c %.o
 sparse: $(SP_OBJ)
 
 EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
+ifndef NETTLE_SHA256
+	EXCEPT_HDRS += sha256/nettle.h
+endif
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
diff --git a/hash.h b/hash.h
index 5d40368f18..ea87ae9d92 100644
--- a/hash.h
+++ b/hash.h
@@ -16,7 +16,9 @@
 #include "block-sha1/sha1.h"
 #endif
 
-#if defined(SHA256_GCRYPT)
+#if defined(SHA256_NETTLE)
+#include "sha256/nettle.h"
+#elif defined(SHA256_GCRYPT)
 #define SHA256_NEEDS_CLONE_HELPER
 #include "sha256/gcrypt.h"
 #elif defined(SHA256_OPENSSL)
diff --git a/sha256/nettle.h b/sha256/nettle.h
new file mode 100644
index 0000000000..b63e1c8190
--- /dev/null
+++ b/sha256/nettle.h
@@ -0,0 +1,31 @@
+#ifndef SHA256_NETTLE_H
+#define SHA256_NETTLE_H
+
+#include <nettle/sha2.h>
+
+typedef struct sha256_ctx nettle_SHA256_CTX;
+
+static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
+{
+	sha256_init(ctx);
+}
+
+static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx,
+					const void *data,
+					size_t len)
+{
+	sha256_update(ctx, len, data);
+}
+
+static inline void nettle_SHA256_Final(unsigned char *digest,
+				       nettle_SHA256_CTX *ctx)
+{
+	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
+}
+
+#define platform_SHA256_CTX nettle_SHA256_CTX
+#define platform_SHA256_Init nettle_SHA256_Init
+#define platform_SHA256_Update nettle_SHA256_Update
+#define platform_SHA256_Final nettle_SHA256_Final
+
+#endif

Re: [PATCH v2] sha256: add support for Nettle

[Ævar Arnfjörð Bjarmason]

On Sun, Jul 10 2022, brian m. carlson wrote:

> @@ -182,6 +182,8 @@ include shared.mak
>  #
>  # Define BLK_SHA256 to use the built-in SHA-256 routines.
>  #
> +# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
> +#
>  # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
>  #
>  # Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
> @@ -1842,6 +1844,10 @@ ifdef OPENSSL_SHA256
>  	EXTLIBS += $(LIB_4_CRYPTO)
>  	BASIC_CFLAGS += -DSHA256_OPENSSL
>  else
> +ifdef NETTLE_SHA256
> +	BASIC_CFLAGS += -DSHA256_NETTLE
> +	EXTLIBS += -lnettle
> +else
>  ifdef GCRYPT_SHA256
>  	BASIC_CFLAGS += -DSHA256_GCRYPT
>  	EXTLIBS += -lgcrypt
> @@ -1850,6 +1856,7 @@ else
>  	BASIC_CFLAGS += -DSHA256_BLK
>  endif
>  endif
> +endif

This just carries forward existing technical debt, but it's unfortunate
that we don't catch OPENSSL_SHA256 overridding NETTLE_SHA256, and error
if both are defined.

>  ifdef SHA1_MAX_BLOCK_SIZE
>  	LIB_OBJS += compat/sha1-chunked.o
> @@ -3091,6 +3098,9 @@ $(SP_OBJ): %.sp: %.c %.o
>  sparse: $(SP_OBJ)
>  
>  EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
> +ifndef NETTLE_SHA256
> +	EXCEPT_HDRS += sha256/nettle.h
> +endif
>  ifndef GCRYPT_SHA256
>  	EXCEPT_HDRS += sha256/gcrypt.h
>  endif
> diff --git a/hash.h b/hash.h
> index 5d40368f18..ea87ae9d92 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -16,7 +16,9 @@
>  #include "block-sha1/sha1.h"
>  #endif
>  
> -#if defined(SHA256_GCRYPT)
> +#if defined(SHA256_NETTLE)
> +#include "sha256/nettle.h"
> +#elif defined(SHA256_GCRYPT)
>  #define SHA256_NEEDS_CLONE_HELPER
>  #include "sha256/gcrypt.h"
>  #elif defined(SHA256_OPENSSL)
> diff --git a/sha256/nettle.h b/sha256/nettle.h
> new file mode 100644
> index 0000000000..b63e1c8190
> --- /dev/null
> +++ b/sha256/nettle.h
> @@ -0,0 +1,31 @@
> +#ifndef SHA256_NETTLE_H
> +#define SHA256_NETTLE_H
> +
> +#include <nettle/sha2.h>
> +
> +typedef struct sha256_ctx nettle_SHA256_CTX;
> +
> +static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
> +{
> +	sha256_init(ctx);
> +}
> +
> +static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx,
> +					const void *data,
> +					size_t len)
> +{
> +	sha256_update(ctx, len, data);
> +}
> +
> +static inline void nettle_SHA256_Final(unsigned char *digest,
> +				       nettle_SHA256_CTX *ctx)
> +{
> +	sha256_digest(ctx, SHA256_DIGEST_SIZE, digest);
> +}
> +
> +#define platform_SHA256_CTX nettle_SHA256_CTX
> +#define platform_SHA256_Init nettle_SHA256_Init
> +#define platform_SHA256_Update nettle_SHA256_Update
> +#define platform_SHA256_Final nettle_SHA256_Final
> +
> +#endif

This looks good to me, except for the small nit that I think this should
be squashed in. For the others we need this inline wrappers, but not for
"init":

diff --git a/sha256/nettle.h b/sha256/nettle.h
index b63e1c81903..5c9811c309a 100644
--- a/sha256/nettle.h
+++ b/sha256/nettle.h
@@ -5,11 +5,6 @@
 
 typedef struct sha256_ctx nettle_SHA256_CTX;
 
-static inline void nettle_SHA256_Init(nettle_SHA256_CTX *ctx)
-{
-	sha256_init(ctx);
-}
-
 static inline void nettle_SHA256_Update(nettle_SHA256_CTX *ctx,
 					const void *data,
 					size_t len)
@@ -24,7 +19,7 @@ static inline void nettle_SHA256_Final(unsigned char *digest,
 }
 
 #define platform_SHA256_CTX nettle_SHA256_CTX
-#define platform_SHA256_Init nettle_SHA256_Init
+#define platform_SHA256_Init sha256_init
 #define platform_SHA256_Update nettle_SHA256_Update
 #define platform_SHA256_Final nettle_SHA256_Final
 

Re: [PATCH v2] sha256: add support for Nettle

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> This just carries forward existing technical debt, but it's unfortunate
> that we don't catch OPENSSL_SHA256 overridding NETTLE_SHA256, and error
> if both are defined.

You are mistaken, unless I am ;-)  

Allowing users to list whatever is available, instead of requiring
all users to choose only one, is a deliberate feature in the
arrangement, so it is not unfortunate and it would be breaking
end-user expectation if we gave an error when more than one is
given (and it would be easier to write and maintain autoconf rules
for the feature---we do not want to have two places that makes
decisions on precedence).

Re: [PATCH v2] sha256: add support for Nettle

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On 2022-07-10 at 16:39:18, Junio C Hamano wrote:
> Allowing users to list whatever is available, instead of requiring
> all users to choose only one, is a deliberate feature in the
> arrangement, so it is not unfortunate and it would be breaking
> end-user expectation if we gave an error when more than one is
> given (and it would be easier to write and maintain autoconf rules
> for the feature---we do not want to have two places that makes
> decisions on precedence).

Yeah, I think given the fact that many folks use autoconf, it's
beneficial to allow multiple options and just choose the one we like the
most.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2] sha256: add support for Nettle

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Yeah, I think given the fact that many folks use autoconf, it's
> beneficial to allow multiple options and just choose the one we like the
> most.

Not that we any of *_SHA256 set in the configure script ;-) It still
is worth noting that the Makefile is prepared to deal with configure
scripts written in the most simple way, i.e. test for any backend
independent of other possible backends.

There can be at least three possible arrangements, and I think the
current one is the most sensible.

 (1) We can allow multiple to be set and let Makefile define
     precedence.  "If you do not choose any, then we fall back to
     ..." will fall out as a natural consequence.  

     If somebody wants to write autoconf support, they do not have
     to reinvent the precedence order.  They can just check for the
     presence of individual libraries independently from each other.

     This is what we have today.

 (2) We can allow multiple to be set and let Makefile define
     precedence.  But a misguided autoconf author can invent their
     own precedence order that may not be compatible with what the
     Makefile has.

     This allows the autoconf author weird satisifaction for being
     different from what Makefile already decided for the sake of
     being different.

     This would be unfortunate, and we should watch out when we
     review patches to add autoconf support for *_SHA256 variables.

 (3) We can allow only one to be set and otherwise error out.

     If somebody wants to write autoconf support, they have to come
     up with the precedence order, or support the "--with[out]-X"
     thing to customize, in order to ensure that the configure
     script picks exactly one.

     Because not everybody has nor need to have enough knowledge to
     choose one among the multiple choices available to them, this
     is hostile to those who do not use configure.

     Not an improvement over what we have today.

Re: [PATCH v2] sha256: add support for Nettle

[Reza Mahdi]

On 7/11/2022 1:07 AM, Junio C Hamano wrote:
>   (3) We can allow only one to be set and otherwise error out.
> 
>       If somebody wants to write autoconf support, they have to come
>       up with the precedence order, or support the "--with[out]-X"
>       thing to customize, in order to ensure that the configure
>       script picks exactly one.
> 
>       Because not everybody has nor need to have enough knowledge to
>       choose one among the multiple choices available to them, this
>       is hostile to those who do not use configure.
> 
>       Not an improvement over what we have today.

I think this is the most used and familiar to folk. There is no need
to have multiple options since all of backends work perfectly.

And, of curse "--with-X" option must be meaningful when the sha256 is
enabled.