threads / patch / 58107

patchsha256: add support for Nettle

Subject: [PATCH] sha256: add support for Nettle

## tl;dr

17 messages between Jul 5, 2022 and Aug 31, 2022. Diffs are folded; open one to read it.

replies: 16people: 6as markdown or json

brian m. carlson· Jul 5, 2022, 23:05 UTC · lore

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
Show changes to 3 files +38 −1

Makefile, hash.h, 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
Ramsay Jones· Jul 6, 2022, 01:00 UTC · re: brian m. carlson · lore

Re: [PATCH] sha256: add support for Nettle

On Tue, Jul 05, 2022 at 11:05:18PM +0000, brian m. carlson wrote:
[snip]
Show 8 quoted lines
> 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

Show 26 quoted lines
> +
> +#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
brian m. carlson· Jul 6, 2022, 01:08 UTC · re: Ramsay Jones · lore

Re: [PATCH] sha256: add support for Nettle

On 2022-07-06 at 01:00:29, Ramsay Jones wrote:
Show 14 quoted lines
> 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
Jeff King· Jul 6, 2022, 08:23 UTC · re: brian m. carlson · lore

Re: [PATCH] sha256: add support for Nettle

On Tue, Jul 05, 2022 at 11:05:18PM +0000, brian m. carlson wrote:
Show 6 quoted lines
> 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.

Show 5 quoted lines
>  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
Ævar Arnfjörð Bjarmason· Jul 6, 2022, 08:45 UTC · re: brian m. carlson · lore

Re: [PATCH] sha256: add support for Nettle

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?

Show 6 quoted lines
> 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.

Show 26 quoted lines
> -#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;
Show 15 quoted lines
> +{
> +	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...

Jeff King· Jul 6, 2022, 09:23 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] sha256: add support for Nettle

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
brian m. carlson· Jul 6, 2022, 10:39 UTC · re: Jeff King · lore

Re: [PATCH] sha256: add support for Nettle

On 2022-07-06 at 09:23:18, Jeff King wrote:
Show 11 quoted lines
> 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.

Show 6 quoted lines
> 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
Junio C Hamano· Jul 6, 2022, 17:49 UTC · re: brian m. carlson · lore

Re: [PATCH] sha256: add support for Nettle

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 15 quoted lines
> 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.

Show 13 quoted lines
> 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?

Show 8 quoted lines
> 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>
Junio C Hamano· Jul 6, 2022, 23:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] sha256: add support for Nettle

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> 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.

Show changes to diff +5 −5
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);
 }
Junio C Hamano· Jul 7, 2022, 06:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] sha256: add support for Nettle

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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).

Show changes to 2 files +6 −3

Makefile, sha256/nettle.h

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);
 }
Junio C Hamano· Jul 7, 2022, 16:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] sha256: add support for Nettle

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> 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
Show changes to 3 files +41 −1

Makefile, hash.h, 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
brian m. carlson· Jul 10, 2022, 13:29 UTC · re: brian m. carlson · lore

[PATCH v2] 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, 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
Show changes to 3 files +44 −1

Makefile, hash.h, 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
Ævar Arnfjörð Bjarmason· Jul 10, 2022, 14:41 UTC · re: brian m. carlson · lore

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

On Sun, Jul 10 2022, brian m. carlson wrote:
Show 25 quoted lines
> @@ -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.

Show 64 quoted lines
>  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":

Show changes to sha256/nettle.h +1 −6
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
 
Junio C Hamano· Jul 10, 2022, 16:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

brian m. carlson· Jul 10, 2022, 20:12 UTC · re: Junio C Hamano · lore

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

On 2022-07-10 at 16:39:18, Junio C Hamano wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Jul 10, 2022, 20:37 UTC · re: brian m. carlson · lore

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

"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.
Reza Mahdi· Aug 31, 2022, 05:34 UTC · re: Junio C Hamano · lore

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

On 7/11/2022 1:07 AM, Junio C Hamano wrote:
Show 12 quoted lines
>   (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.

← back to recent threads