Re: [PATCH 03/15] cache: add an algo member to struct object_id
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Apr 14, 2021, 01:08 UTC
- Message-ID
- <YHZApIpPBhpmwscP@camp.crustytoothpaste.net>
- In-Reply-To
- <38f48634-077b-6351-5285-f8ecc6f552d0@gmail.com>
On 2021-04-13 at 12:12:21, Derrick Stolee wrote:
Show 27 quoted lines
> On 4/10/2021 11:21 AM, brian m. carlson wrote:
> > Now that we're working with multiple hash algorithms in the same repo,
> > it's best if we label each object ID with its algorithm so we can
> > determine how to format a given object ID. Add a member called algo to
> > struct object_id.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > hash.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/hash.h b/hash.h
> > index 3fb0c3d400..dafdcb3335 100644
> > --- a/hash.h
> > +++ b/hash.h
> > @@ -181,6 +181,7 @@ static inline int hash_algo_by_ptr(const struct git_hash_algo *p)
> >
> > struct object_id {
> > unsigned char hash[GIT_MAX_RAWSZ];
> > + int algo;
> > };
>
> What are the performance implications of adding this single bit
> (that actually costs us 4 to 8 bytes, based on alignment)? Later
> in the series you add longer hash comparisons, too. These seem
> like they will affect performance for existing SHA-1 repos, and
> it would be nice to know how much we are paying for this support.I will do some performance numbers on these patches, but it will likely be the weekend before I can get to it. I think this will add 4 bytes on most platforms, since int is typically 32 bits, and the alignment requirement would be for the most strictly aligned member, which is the int, so a 4-byte alignment. I don't think the alignment requirements are especially onerous here.
Show 6 quoted lines
> I assume that we already checked what happened when GIT_MAX_RAWSZ > increased, but that seemed worth the cost so we could have SHA-256 > at all. I find the justification for this interoperability mode to > be less significant, and potentially adding too much of a tax onto > both SHA-1 repos that will never upgrade, and SHA-256 repos that > upgrade all at once (or start as SHA-256).
The entire goal of the interoperability is to let people seamlessly and transparently move from SHA-1 to SHA-256. Currently, the only way people can move a SHA-1 repository to a SHA-256 repository is with fast-import and fast-export, which loses all digital signatures and tags to blobs. This also requires a flag day.
SHA-1 can now be attacked for USD 45,000. That means it is within the budget of a dedicated professional and virtually all medium or large corporations, including even most municipal governments, to create a SHA-1 collision. Unfortunately, the way we deal with this is to die, so as soon as this happens, the repository fails closed. While an attacker cannot make use of the collisions to spread malicious objects, because of the way Git works, they can effectively DoS a repository, which is in itself a security issue. Fixing this requires major surgery.
We need the interoperability code to let people transition their repositories away from SHA-1, even if it has some performance impact, because without that most SHA-1 repositories will never transition. That's what's outlined in the transition plan, and why that approach was proposed, even though it would be nicer to avoid having to implement it at all.
I will endeavor to make the performance impact as small as possible, of course, and ideally there will be none. I am sensitive to the fact that people do run absurdly large workloads on Git, as we both know, and I do want to support that.
-- brian m. carlson (he/him or they/them) Houston, Texas, US