Offline verification: refactor to make it clear no signature checks a…

…re happening. (#319)

- Rename rekor.VerifyOffline -> Rekor.VerifyInclusion to make it
  clear that is is not checking the signature content.
- Require callers to pass in the certificate instead of trying to infer
  it from the signature to make it more likely that users have verified
  the commit already.

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

internal/gitsign/gitsign_test.go

@@ -154,7 +154,7 @@ func (fakeRekor) Verify(ctx context.Context, commitSHA string, cert *x509.Certif
 	return nil, nil
 }
 
-func (fakeRekor) VerifyOffline(ctx context.Context, sig []byte) (*models.LogEntryAnon, error) {
+func (fakeRekor) VerifyInclusion(ctx context.Context, sig []byte, cert *x509.Certificate) (*models.LogEntryAnon, error) {
 	return nil, nil
 }
 

pkg/git/verify.go

@@ -74,7 +74,7 @@ func Verify(ctx context.Context, git Verifier, rekor rekor.Verifier, data, sig [
 	}
 	claims = append(claims, NewClaim(ClaimValidatedSignature, true))
 
-	if tlog, err := rekor.VerifyOffline(ctx, sig); err == nil {
+	if tlog, err := rekor.VerifyInclusion(ctx, sig, cert); err == nil {
 		return &VerificationSummary{
 			Cert:     cert,
 			LogEntry: tlog,

pkg/rekor/rekor.go

@@ -49,7 +49,7 @@ import (
 // Verifier represents a mechanism to get and verify Rekor entries for the given Git data.
 type Verifier interface {
 	Verify(ctx context.Context, commitSHA string, cert *x509.Certificate) (*models.LogEntryAnon, error)
-	VerifyOffline(ctx context.Context, sig []byte) (*models.LogEntryAnon, error)
+	VerifyInclusion(ctx context.Context, sig []byte, cert *x509.Certificate) (*models.LogEntryAnon, error)
 }
 
 // Writer represents a mechanism to write content to Rekor.
@@ -179,7 +179,7 @@ func rekorPubsFromClient(rekorClient *client.Rekor) (*cosign.TrustedTransparency
 // 1. Searching Rekor for an entry matching the commit SHA + cert.
 // 2. Use the same cert to verify the commit content.
 //
-// Note: While not truly deprecated, Client.VerifyOffline is generally preferred.
+// Note: While not truly deprecated, using offline verification is generally preferred.
 // This function relies on non-GA behavior of Rekor, and remains for backwards
 // compatibility with older signatures.
 func (c *Client) Verify(ctx context.Context, commitSHA string, cert *x509.Certificate) (*models.LogEntryAnon, error) {
@@ -245,9 +245,10 @@ func (c *Client) PublicKeys() *cosign.TrustedTransparencyLogPubKeys {
 	return c.publicKeys
 }
 
-// VerifyOffline verifies a signature using offline verification.
-// Unlike Client.Verify, only the commit content is considered during verification.
-func (c *Client) VerifyOffline(ctx context.Context, sig []byte) (*models.LogEntryAnon, error) {
+// VerifyInclusion verifies a signature's inclusion in Rekor using offline verification.
+// NOTE: This does **not** verify the correctness of the signature against the content.
+// Prefer using [git.Verify] instead for complete verification.
+func (c *Client) VerifyInclusion(ctx context.Context, sig []byte, cert *x509.Certificate) (*models.LogEntryAnon, error) {
 	// Try decoding as PEM
 	var der []byte
 	if blk, _ := pem.Decode(sig); blk != nil {
@@ -261,27 +262,30 @@ func (c *Client) VerifyOffline(ctx context.Context, sig []byte) (*models.LogEntr
 		return nil, fmt.Errorf("failed to parse signature: %w", err)
 	}
 
-	// Generate verification options.
-	certs, err := sd.GetCertificates()
-	if err != nil {
-		return nil, fmt.Errorf("error getting signature certs: %w", err)
-	}
-	if len(certs) == 0 {
-		return nil, fmt.Errorf("no certificates found in signature")
-	}
-	cert := certs[0]
-
-	// Recompute HashedRekord body by rehashing the authenticated attributes.
+	// Extract signer from signature. The spec allows for multiple signers, but in
+	// practice for Git commits there's typically only ever 1 signer (the committer).
 	r := sd.Raw()
 	if len(r.SignerInfos) == 0 {
 		return nil, fmt.Errorf("no signers found in signature")
 	}
 	si := r.SignerInfos[0]
-	message, err := si.SignedAttrs.MarshaledForVerification()
+
+	// Double check cert matches the signer. This technically isn't needed
+	// since if this didn't match then the verify below should also fail,
+	// but this helps us distinguish the error in the unlikely event this does happen.
+	if _, err := si.FindCertificate([]*x509.Certificate{cert}); err != nil {
+		return nil, fmt.Errorf("signer certificate mismatch: %w", err)
+	}
+
+	// Get HashedRekord body from the signature.
+	// We are assuming here that the signature has already been authenticated against the
+	// cert, so it is okay to rely the precomputed checksum in the SignerInfo.
+	message, err := si.GetMessageDigestAttribute()
 	if err != nil {
 		return nil, err
 	}
 
+	// Reassemble the tlog entry from the signature pieces.
 	tlog, err := rekoroid.ToLogEntry(ctx, message, si.Signature, cert, si.UnsignedAttrs)
 	if err != nil {
 		return nil, err