Add offline verification #220
Conversation
b528d89 to
5548553
Compare
|
Marked as draft for now since we still need to define the Rekor OIDs in the Rekor repo itself, but wanted to throw this up for feedback. cc @znewman01 @haydentherapper @asraa who might be interested. |
5548553 to
c309c66
Compare
| // offline - Hashed commit content is stored in Rekor, with Rekor attributes | ||
| // necessary for offline verification being stored in the commit itself. | ||
| // Note: online verification will be deprecated in favor of offline in the future. | ||
| RekorMode string |
There was a problem hiding this comment.
I'm not against this, but it feels like it would be a good standalone refactor.
9a31d38 to
00dd3f0
Compare
| git cat-file commit HEAD | sed -n '/BEGIN/, /END/p' | sed 's/^ //g' | sed 's/gpgsig //g' | sed 's/SIGNED MESSAGE/PKCS7/g' | openssl pkcs7 -print -print_certs -text | ||
| - name: Test Sign and Verify commit - offline verification | ||
| env: | ||
| GITSIGN_REKOR_MODE: "offline" |
There was a problem hiding this comment.
Hm, is there any way to double check that validation actually happens offline? If you typo'd GITSIGN_REKOR_MODE this test wouldn't check the right thing.
You could turn off network access for the gitsign verify commands
https://github.com/orgs/community/discussions/24975
There was a problem hiding this comment.
I can look into doing this in another PR. The long term plan is to move all signing to offline verification. This is mainly here as a feature flag to maintain compatibility.
| Unfortunately this is a bit complex to query manually. Roughly this is: | ||
|
|
||
| ``` | ||
| sha256(der(sort(system time | commit data | content type))) |
There was a problem hiding this comment.
"commit data" is what, the output of git cat-file commit <SHA>?
What's content type? Any why is the time in there if there's already a commit time?
What's the "sort" for?
More generally, what's the motivation for the design here (esp because you could just use the commit SHA)? Does this format come from Git?
There was a problem hiding this comment.
This is trying to be a high level overview describing what's going on behind the scenes here - https://github.com/github/smimesign/blob/3564e86011859c28b315328027abebb954b6bf6f/ietf-cms/protocol/protocol.go#L700-L731. These are the pieces of data that go into the verification checksum.
"commit data" is what, the output of git cat-file commit ?
Kinda. It's the marshalled data used for signing, which is basically git cat-file but without the signature.
What's content type?
Content type is part of the PKCS7 spec - https://datatracker.ietf.org/doc/html/rfc2315#section-8. It's effectively a constant for our usage, but it's needed to generate the correct checksum.
Any why is the time in there if there's already a commit time?
I'm not 100% sure about this - it's carried over from smimesign. In general it doesn't seem harmful to have a separate signing time from commit time (since hypothetically the commit body is generated before the signing operation, so there can be skew).
What's the "sort" for?
Stabilization.
More generally, what's the motivation for the design here (esp because you could just use the commit SHA)?
The motivation here is we want to move away from the commit SHA approach - the current online verification flow relies on the Rekor search API since we're signing 2 different pieces of data with the same key. This changes it so we're always looking at the same data.
internal/commands/root/sign.go
Outdated
| @@ -88,18 +88,21 @@ func commandSign(o *options, s *gsio.Streams, args ...string) error { | |||
| opts.UserName = o.Config.CommitterName | |||
| opts.UserEmail = o.Config.CommitterEmail | |||
| } | |||
| sig, cert, tlog, err := git.Sign(ctx, rekor, userIdent, dataBuf.Bytes(), opts) | |||
| if o.Config.RekorMode == "offline" { | |||
| opts.RekorAddr = o.Config.Rekor | |||
There was a problem hiding this comment.
Hmm, I find this a little confusing. If we're "offline", we provide the "RekorAddr"? That's the opposite of what I'd expect.
IIUC this basically does the same signing process, it just additionally stuffs offline verification materials into the git repo.
There was a problem hiding this comment.
Oh, I see. If we pass opts.RekorAddr, then signature.Sign will do the new Rekor log stuff. Otherwise, git.Sign will use the rekor we pass in.
That feels...a little fraught. I'd rather make the signing mode more explicit in the arguments, or even have separate git.SignOffline and git.SignOnline.
There was a problem hiding this comment.
Yeah I thought about this - the tricky part is for git.Sign rekor is required, since it does the sign + rekor upload, but for signature.Sign that acts on the git signature directly it's optional. I'd rather plumb the sign options through than have a pseudo option that tries to inject it later.
If/when we transition to always offline signing, this is something we can clean up because it should be always set.
| "github.com/sigstore/rekor/pkg/generated/models" | ||
| ) | ||
|
|
||
| // This file contains helper functions from going to/from Rekor types <-> protobuf-specs. |
There was a problem hiding this comment.
nice work on this!
This adds the ability for gitsign commits to include rekor log entry details to enable offline verification of commits. - Adds new unauthenticated attributes corresponding to Rekor log entry values. - Adds support for offline commit verification along side existing SHA based online verification. - Adds config option for users to select offline or online storage options. Defaults to existing online behavior to allow users to opt-in, this will be changed to offline after a release. - Adds e2e test for offline verification. Signed-off-by: Billy Lynch <billy@chainguard.dev>
00dd3f0 to
a56af77
Compare
Signed-off-by: Billy Lynch <billy@chainguard.dev>
a56af77 to
2b31102
Compare
Summary
This adds the ability for gitsign commits to include rekor log entry details to enable offline verification of commits.
Fixes #219
Release Note
Adds new
rekorModeconfig option to allow configuring signature format to allow for offline verification of commits. This is opt-in for this release. This will become the default in the future.Documentation
✅