Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

From: Luke Shumaker <lukeshu@lukeshu.com>
Date: Apr 20, 2021, 17:15 UTC
Message-ID: <87tuo0q3ma.wl-lukeshu@lukeshu.com>
In-Reply-To: <YH4xY/oSwYIUmJyL@camp.crustytoothpaste.net>

On Mon, 19 Apr 2021 19:41:55 -0600, brian m. carlson wrote:

Show 34 quoted lines

> 
> [1  <text/plain; utf-8 (quoted-printable)>]
> On 2021-04-19 at 22:54:41, Luke Shumaker wrote:
> > From: Luke Shumaker <lukeshu@datawire.io>
> > 
> > fast-export has an existing --signed-tags= flag that controls how to
> > handle tag signatures.  However, there is no equivalent for commit
> > signatures; it just silently strips the signature out of the commit
> > (analogously to --signed-tags=strip).
> > 
> > While signatures are generally problematic for fast-export/fast-import
> > (because hashes are likely to change), if they're going to support tag
> > signatures, there's no reason to not also support commit signatures.
> > 
> > So, implement signed-commits.
> > 
> > On the fast-export side, try to be as much like signed-tags as possible,
> > in both implementation and in user-interface; with the exception that
> > the default should be `--signed-commits=strip` (compared to the default
> > `--signed-tags=abort`), in order to continue defaulting to the
> > historical behavior.  Only bother implementing "gpgsig", not
> > "gpgsig-sha256"; the existing signed-tag support doesn't implement
> > "gpgsig-sha256" either.
> 
> I would appreciate it if we did in fact implement it.  I would like to
> use this functionality to round-trip objects between SHA-1 and SHA-256,
> and it would be nice if both worked.
> 
> The situation with tags is different: the signature using the current
> algorithm is always trailing, and the signature for the other algorithm
> is in the header.  That wasn't how we intended it to be, but that's how
> it ended up being.
> 
> As a result, tag output can support SHA-256 data,

I don't believe that's true? With SHA-1-signed tags, the signature gets included in the fast-import stream as part of the tag message (the `data` line in the BNF). Since SHA-256-signed tags have their signature as a header (rather than just appending it to the message), we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command (like I've done to the 'commit' top-level-command).

Show 5 quoted lines

>                                                   but with your
> proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
> wildly insecure and therefore signing SHA-1 commits adds very little
> security, whereas SHA-256 is presently considered strong, I'd argue that
> only supporting SHA-1 isn't the right move here.

The main reason I didn't implement SHA-256 support (well, besides that the repo I'm working on turned out to not have any SHA-256-signed commits in it) is that I had questions about SHA-256 that I didn't know/couldn't find the answers to.

However, looking again, I see a few of the answers in t7510-signed-commit.sh, so I'll have a go at it. If I get stuck, I'll go ahead and implement the below "gpgsig sha1" suggestion, and leave the sha256 implementation to someone else.

> Provided we do that and the test suite passes under both algorithms, I'm
> strongly in favor of this feature.  In fact, I had been thinking about
> implementing this feature myself just the other day, so I'm delighted
> you decided to do it.

That's one of the big reasons I didn't implement both--I wasn't sure how to test sha256 (within the test harness, `git commit -S` gives a sha1 signature).

I see that t7510-signed-commit.sh 'verify-commit verifies multiply signed commits' tests sha256 by hard-coding a raw commit object in the test itself, and feeding that to `git hash-object`. I'd prefer to figure out how to get `git commit` itself to generate a sha256 signature rather than a sha1 signature, so that I can _know_ that I'm getting the ordering of headers the same as `git commit`. But I don't think that needs to be a blocker; if the test doesn't do the same ordering as `git commit`, I guess that can just be a bugfix later?

Show 13 quoted lines

> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Could we emit this as "gpgsig sha1 data" and "gpgsig sha256 data"?  That
> would allow us to consider the future case where the hash algorithm
> changes again without requiring a change of format.

I like that idea.  I'll implement it.

FWIW, I thought about instead adding a fast-import command to insert arbitrary headers in to the commit object, rather than having to add a new command for every new header we want to be able to round-trip. But it's like, if we're exposing that much of the low-levels of a commit object, why are we keeping up the facade fast-import stream at all, instead of streaming raw Git objects around?

-- 
Happy hacking,
~ Luke Shumaker