| Message ID | 20220710132907.1499365-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e55573583685eda80dd33d85ab2cea59b86332c5 |
| Headers | show |
| Series | [v2] sha256: add support for Nettle | expand |
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
Æ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).
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" <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.
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.
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
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