Re: [PATCH v2 2/2] help: include unsafe SHA-1 build info in version

From: Justin Tobler <jltobler@gmail.com>
Date: Apr 2, 2025, 15:59 UTC
Message-ID: <46xnrbtafdg2iafownzujbpzjuygpns4g4r26wzxxv6xmi5mg7@m3dw6cfk6f7q>
In-Reply-To: <Z-zpau_DsOGVVtjj@pks.im>

On 25/04/02 09:38AM, Patrick Steinhardt wrote:

Show 19 quoted lines

> On Tue, Apr 01, 2025 at 03:36:30PM -0500, Justin Tobler wrote:
> > diff --git a/Documentation/git-version.adoc b/Documentation/git-version.adoc
> > index f06758a7cf..753794988c 100644
> > --- a/Documentation/git-version.adoc
> > +++ b/Documentation/git-version.adoc
> > @@ -25,6 +25,9 @@ OPTIONS
> >  +
> >  Note that the SHA1 options `SHA1_APPLE`, `SHA1_OPENSSL`, and `SHA1_BLK` do not
> >  have collision detection.
> > ++
> > +If built to use a faster SHA-1 implementation for non-cryptographic purposes,
> > +that implementation is denoted as "non-crypto-SHA-1".
> >  
> >  GIT
> >  ---
> 
> I got basically the same comment for this new paragraph as for the first
> one. I'd either drop it or expand it so that readers know what's going
> on.

Ya, this should also be expanded a bit. I think in combination with the expanded documentation for the prior patch, something like this might be a bit better.

"When a faster SHA-1 implementation without collision detection is used for only non-cryptographic purposes, the algorithm is diplayed in the form `non-collision-detecting-SHA-1: <option>`."

Show 16 quoted lines

> > diff --git a/help.c b/help.c
> > index 3aebfb3681..1238a962b0 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -772,6 +772,11 @@ char *help_unknown_cmd(const char *cmd)
> >  static void get_sha_impl(struct strbuf *buf)
> >  {
> >  	strbuf_addf(buf, "SHA-1: %s\n", SHA1_BACKEND);
> > +
> > +#if defined(SHA1_UNSAFE_BACKEND)
> > +	strbuf_addf(buf, "non-crypto-SHA-1: %s\n", SHA1_UNSAFE_BACKEND);
> > +#endif
> > +
> 
> Should we maybe print the equivalent of "none" in case no unsafe backend
> was selected?

It is suggested later to rename "non-crypto-SHA-1" to "SHA-1 without collision detection", which could lead to something like this:

    SHA-1: SHA1_OPENSSL (No collision detection)
    SHA-1 without collision detection: none

which could be a bit misleading IMO. It might be best to leave the option omitted if it is not defined.

> I also think we shouldn't name this "non-crypto". The backend still is
> SHA1, which is a proper cryptogtaphic hash function. It may be somewhat
> broken nowadays, but that doesn't change the fact that it's a
> cryptographic primitive.

I was trying to indicate that this SHA-1 backend was used only in non-cryptographic scenarios, but I agree that this name is not great. Calling it "SHA-1 used for non-cryptographic purposes" is a bit of a mouthful, but maybe that is fine?

Another idea I had was to call it "fast-SHA-1:" since it's intended as a performance optimization used in certain cases.

> How about we rename this to "SHA-1 without collision detection:"?

Being verbose here is probably best. I'll probably use something like "non-collision-detecting-SHA-1:" in the next version.

-Justin