Add offline verification #220

wlynch · 2023-01-13T21:52:36Z

Summary

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.

Fixes #219

Release Note

Adds new rekorMode config 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

✅

wlynch · 2023-01-13T22:02:01Z

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.

znewman01

Sorry for the delay here!

LGTM (pending the Rekor OIDs upstream). If everyone wrote docs like this code review would be a lot less onerous, thanks. Some suggestions but take or leave 'em

docs/verification.md

internal/commands/root/sign.go

znewman01 · 2023-02-08T20:23:07Z

internal/config/config.go

+	// 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


Nit: I'd usually make this an enum

I'm not against this, but it feels like it would be a good standalone refactor.

internal/signature/sign.go

pkg/rekor/rekor.go

znewman01

I wrote a bunch of comments but you should interpret that as rambling to myself in order to figure out what's going on. If you find any suggestions helpful, feel free to address them but my approval is not contingent on any of them.

znewman01 · 2023-04-19T16:00:59Z

.github/workflows/e2e.yaml

+          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"


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

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.

docs/verification.md

znewman01 · 2023-04-19T16:11:04Z

docs/verification.md

+Unfortunately this is a bit complex to query manually. Roughly this is:
+
+```
+sha256(der(sort(system time | commit data | content type)))


"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?

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.

znewman01 · 2023-04-19T16:14:25Z

internal/commands/root/sign.go

@@ -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


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.

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.

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.

internal/signature/sign.go

internal/git/git.go

docs/verification.md

internal/signature/sign.go

docs/verification.md

haydentherapper · 2023-04-19T18:07:18Z

internal/rekor/oid/pbcompat.go

+	"github.com/sigstore/rekor/pkg/generated/models"
+)
+
+// This file contains helper functions from going to/from Rekor types <-> protobuf-specs.


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>

Signed-off-by: Billy Lynch <billy@chainguard.dev>

wlynch force-pushed the offline-verification branch 3 times, most recently from b528d89 to 5548553 Compare January 13, 2023 22:01

wlynch force-pushed the offline-verification branch from 5548553 to c309c66 Compare January 17, 2023 16:54

znewman01 reviewed Feb 8, 2023

View reviewed changes

wlynch mentioned this pull request Mar 16, 2023

Add Rekor OID info. sigstore/rekor#1390

Merged

wlynch force-pushed the offline-verification branch 3 times, most recently from 9a31d38 to 00dd3f0 Compare April 17, 2023 19:27

wlynch marked this pull request as ready for review April 17, 2023 19:52

wlynch requested a review from znewman01 April 17, 2023 19:52

znewman01 previously approved these changes Apr 19, 2023

View reviewed changes

haydentherapper reviewed Apr 19, 2023

View reviewed changes

wlynch dismissed znewman01’s stale review via a56af77 May 18, 2023 14:22

wlynch force-pushed the offline-verification branch from 00dd3f0 to a56af77 Compare May 18, 2023 14:22

wlynch requested a review from znewman01 May 18, 2023 14:23

Refactor Sign func into 2 separate functions, rework verification doc.

2b31102

Signed-off-by: Billy Lynch <billy@chainguard.dev>

wlynch force-pushed the offline-verification branch from a56af77 to 2b31102 Compare May 18, 2023 14:25

wlynch requested a review from priyawadhwa May 19, 2023 21:02

priyawadhwa approved these changes May 20, 2023

View reviewed changes

wlynch merged commit 5dd6092 into sigstore:main May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add offline verification #220

Add offline verification #220

wlynch commented Jan 13, 2023

wlynch commented Jan 13, 2023

znewman01 left a comment

znewman01 Feb 8, 2023

wlynch Apr 17, 2023

znewman01 left a comment

znewman01 Apr 19, 2023

wlynch May 17, 2023

znewman01 Apr 19, 2023

wlynch May 17, 2023

znewman01 Apr 19, 2023

znewman01 Apr 19, 2023

wlynch May 18, 2023

haydentherapper Apr 19, 2023

Add offline verification #220

Add offline verification #220

Conversation

wlynch commented Jan 13, 2023

Summary

Release Note

Documentation

wlynch commented Jan 13, 2023

znewman01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znewman01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment