Re: [PATCH 2/4] read-cache: add index.skipHash config option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 12, 2022, 18:01 UTC
Message-ID: <221212.868rjc5w8t.gmgdl@evledraar.gmail.com>
In-Reply-To: <5301dfa1-71c9-1ff4-6ba0-e37f25e8ef62@github.com>

On Mon, Dec 12 2022, Derrick Stolee wrote:

Show 65 quoted lines

> On 12/7/2022 6:06 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:
>> 
>>> From: Derrick Stolee <derrickstolee@github.com>
>>> [...]
>>> While older Git versions will not recognize the null hash as a special
>>> case, the file format itself is still being met in terms of its
>>> structure. Using this null hash will still allow Git operations to
>>> function across older versions.
>> 
>> That's good news, but...
>> 
>>> The one exception is 'git fsck' which checks the hash of the index file.
>>> This used to be a check on every index read, but was split out to just
>>> the index in a33fc72fe91 (read-cache: force_verify_index_checksum,
>>> 2017-04-14).
>> 
>> ...uh, what?
>> 
>> Is there an implied claim here that versions before v2.13.0 don't count
>> as "older versions"?
>> 
>> I.e. doesn't v2.12.0 hard fail the verification for all index writing?
>> It's only after v2.13.0 that we do it only for the fsck.
>> 
>> That seems like a rather significant caveat that we should be noting
>> prominently in the docs added in 4/4.
>
> I can add those details.
>  
>>> As a quick comparison, I tested 'git update-index --force-write' with
>>> and without index.computeHash=false on a copy of the Linux kernel
>>> repository.
>> 
>> It took me a bit to see why I was failing to reproduce this, before
>> finding that it's because you mention index.computeHash here, but it's
>> index.skipHash now.
>>>
>>> Benchmark 1: with hash
>>>   Time (mean ± σ):      46.3 ms ±  13.8 ms    [User: 34.3 ms, System: 11.9 ms]
>>>   Range (min … max):    34.3 ms …  79.1 ms    82 runs
>>>
>>> Benchmark 2: without hash
>>>   Time (mean ± σ):      26.0 ms ±   7.9 ms    [User: 11.8 ms, System: 14.2 ms]
>>>   Range (min … max):    16.3 ms …  42.0 ms    69 runs
>>>
>>> Summary
>>>   'without hash' ran
>>>     1.78 ± 0.76 times faster than 'with hash'
>> 
>> I suggested in
>> https://lore.kernel.org/git/221207.868rjiam86.gmgdl@evledraar.gmail.com/
>> earlier to benchmark this against not-sha1collisiondetection.
>
> Generally, I'm avoiding that benchmark because sha1dc is here to stay.
>
> If users want to go through the trouble of compiling to use the non-dc
> version, then I would expect the difference to be less noticeable, but
> still significant. However, I would strongly avoid considering compiling
> both into the client by default, letting certain paths use sha1dc and
> others using non-dc. Certain secure environments currently only use Git
> under exceptions that allow SHA1 for "non-cryptographic" reasons, but
> also with the understanding that sha1dc is used as a safety measure.
> Adding the non-dc version back in would put that understanding at risk.

Doesn't using a checksum for our own index count as a "non-cryptographic reason"? I.e. we control the .git/index file, and the context is that we're checking if bytes we wrote to disk are corrupt since we last saw them.

Even if hypothetically an attacker could craft files to go into the index (knowing our envelope) in such a way as to craft a collision between that index file and some other index file I don't see how that would give the attacker anything. We'd still have a valid index, and we'd probably be replacing that crafted index with a new one anyway.

I understand that some organizations have SHA-1 on some naughty list, and using it again in non-SHA1DC contexts might trigger some audit.

So it wouldn't be something for everyone, and it's orthagonal to the benefits of a new ref format or index format.

But if we're considering new formats, I think it's worth considering a non-format change which doesn't get us all of the way of no checksumming, but more than halfway there.

Maybe we'll still want the "don't do any checksumming", but maybe some would find that enough (particularly if SHA-1 HW acceleration is available).