cli · phillmv · Nov 22, 2024 · Nov 19, 2024 · Nov 19, 2024 · Nov 19, 2024
@@ -13,25 +13,31 @@ var (
 	GitHubTenantOIDCIssuer = "https://token.actions.%s.ghe.com"
 )
 
-func VerifyCertExtensions(results []*AttestationProcessingResult, ec EnforcementCriteria) error {
+// VerifyCertExtensions allows us to perform case insensitive comparisons of certificate extensions
+func VerifyCertExtensions(results []*AttestationProcessingResult, ec EnforcementCriteria) ([]*AttestationProcessingResult, error) {
 	if len(results) == 0 {
-		return errors.New("no attestations proccessing results")
+		return nil, errors.New("no attestations processing results")
 	}
 
+	verified := make([]*AttestationProcessingResult, 0, len(results))
 	var lastErr error
 	for _, attestation := range results {
-		err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate)
-		if err == nil {
-			// if at least one attestation is verified, we're good as verification
-			// is defined as successful if at least one attestation is verified
-			return nil
+		if err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate); err != nil {
+			lastErr = err
+			// move onto the next attestation in the for loop if verification fails
+			continue
 		}
-		lastErr = err
+		// otherwise, add the result to the results slice and increment verifyCount
+		verified = append(verified, attestation)
 	}
 
-	// if we have exited the for loop without returning early due to successful
-	// verification, we need to return an error
-	return lastErr
+	// if we have exited the for loop without verifying any attestations,
+	// return the last error found
+	if len(verified) == 0 {
+		return nil, lastErr
+	}
+
+	return verified, nil
 }
 
 func verifyCertExtensions(given, expected certificate.Summary) error {

@@ -37,17 +37,19 @@ func TestVerifyCertExtensions(t *testing.T) {
 	}
 
 	t.Run("passes with one result", func(t *testing.T) {
-		err := VerifyCertExtensions(results, c)
+		verified, err := VerifyCertExtensions(results, c)
 		require.NoError(t, err)
+		require.Len(t, verified, 1)
 	})
 
 	t.Run("passes with 1/2 valid results", func(t *testing.T) {
 		twoResults := []*AttestationProcessingResult{createSampleResult(), createSampleResult()}
 		require.Len(t, twoResults, 2)
 		twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong"
 
-		err := VerifyCertExtensions(twoResults, c)
+		verified, err := VerifyCertExtensions(twoResults, c)
 		require.NoError(t, err)
+		require.Len(t, verified, 1)
 	})
 
 	t.Run("fails when all results fail verification", func(t *testing.T) {
@@ -56,35 +58,40 @@ func TestVerifyCertExtensions(t *testing.T) {
 		twoResults[0].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong"
 		twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong"
 
-		err := VerifyCertExtensions(twoResults, c)
+		verified, err := VerifyCertExtensions(twoResults, c)
 		require.Error(t, err)
+		require.Nil(t, verified)
 	})
 
 	t.Run("with wrong SourceRepositoryOwnerURI", func(t *testing.T) {
 		expectedCriteria := c
 		expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong"
-		err := VerifyCertExtensions(results, expectedCriteria)
+		verified, err := VerifyCertExtensions(results, expectedCriteria)
 		require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner")
+		require.Nil(t, verified)
 	})
 
 	t.Run("with wrong SourceRepositoryURI", func(t *testing.T) {
 		expectedCriteria := c
 		expectedCriteria.Certificate.SourceRepositoryURI = "https://github.com/foo/wrong"
-		err := VerifyCertExtensions(results, expectedCriteria)
+		verified, err := VerifyCertExtensions(results, expectedCriteria)
 		require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/owner/repo")
+		require.Nil(t, verified)
 	})
 
 	t.Run("with wrong OIDCIssuer", func(t *testing.T) {
 		expectedCriteria := c
 		expectedCriteria.Certificate.Issuer = "wrong"
-		err := VerifyCertExtensions(results, expectedCriteria)
+		verified, err := VerifyCertExtensions(results, expectedCriteria)
 		require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com")
+		require.Nil(t, verified)
 	})
 
 	t.Run("with partial OIDCIssuer match", func(t *testing.T) {
 		expectedResults := results
 		expectedResults[0].VerificationResult.Signature.Certificate.Extensions.Issuer = "https://token.actions.githubusercontent.com/foo-bar"
-		err := VerifyCertExtensions(expectedResults, c)
+		verified, err := VerifyCertExtensions(expectedResults, c)
 		require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer")
+		require.Nil(t, verified)
 	})
 }
@@ -6,17 +6,23 @@ import (
 
 	"github.com/cli/cli/v2/pkg/cmd/attestation/api"
 	"github.com/cli/cli/v2/pkg/cmd/attestation/test/data"
+	"github.com/sigstore/sigstore-go/pkg/bundle"
 	"github.com/sigstore/sigstore-go/pkg/fulcio/certificate"
 
 	in_toto "github.com/in-toto/attestation/go/v1"
 	"github.com/sigstore/sigstore-go/pkg/verify"
 )
 
 type MockSigstoreVerifier struct {
-	t *testing.T
+	t           *testing.T
+	mockResults []*AttestationProcessingResult
 }
 
-func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
+func (v *MockSigstoreVerifier) Verify([]*api.Attestation, verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
+	if v.mockResults != nil {
+		return v.mockResults, nil
+	}
+
 	statement := &in_toto.Statement{}
 	statement.PredicateType = SLSAPredicateV1
 
@@ -45,11 +51,51 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve
 }
 
 func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier {
-	return &MockSigstoreVerifier{t}
+	result := BuildSigstoreJsMockResult(t)
+	results := []*AttestationProcessingResult{&result}
+
+	return &MockSigstoreVerifier{t, results}
+}
+
+func NewMockSigstoreVerifierWithMockResults(t *testing.T, mockResults []*AttestationProcessingResult) *MockSigstoreVerifier {
+	return &MockSigstoreVerifier{t, mockResults}
 }
 
 type FailSigstoreVerifier struct{}
 
-func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
+func (v *FailSigstoreVerifier) Verify([]*api.Attestation, verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
 	return nil, fmt.Errorf("failed to verify attestations")
 }
+
+func BuildMockResult(b *bundle.Bundle, buildSignerURI, sourceRepoOwnerURI, sourceRepoURI, issuer string) AttestationProcessingResult {
+	statement := &in_toto.Statement{}
+	statement.PredicateType = SLSAPredicateV1
+
+	return AttestationProcessingResult{
+		Attestation: &api.Attestation{
+			Bundle: b,
+		},
+		VerificationResult: &verify.VerificationResult{
+			Statement: statement,
+			Signature: &verify.SignatureVerificationResult{
+				Certificate: &certificate.Summary{
+					Extensions: certificate.Extensions{
+						BuildSignerURI:           buildSignerURI,
+						SourceRepositoryOwnerURI: sourceRepoOwnerURI,
+						SourceRepositoryURI:      sourceRepoURI,
+						Issuer:                   issuer,
+					},
+				},
+			},
+		},
+	}
+}
+
+func BuildSigstoreJsMockResult(t *testing.T) AttestationProcessingResult {
+	bundle := data.SigstoreBundle(t)
+	buildSignerURI := "https://github.com/github/example/.github/workflows/release.yml@refs/heads/main"
+	sourceRepoOwnerURI := "https://github.com/sigstore"
+	sourceRepoURI := "https://github.com/sigstore/sigstore-js"
+	issuer := "https://token.actions.githubusercontent.com"
+	return BuildMockResult(bundle, buildSignerURI, sourceRepoOwnerURI, sourceRepoURI, issuer)
+}
@@ -48,3 +48,26 @@ func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestatio
 	msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation)
 	return attestations, msg, nil
 }
+
+func verifyAttestations(art artifact.DigestedArtifact, att []*api.Attestation, sgVerifier verification.SigstoreVerifier, ec verification.EnforcementCriteria) ([]*verification.AttestationProcessingResult, string, error) {
+	sgPolicy, err := buildSigstoreVerifyPolicy(ec, art)
+	if err != nil {
+		logMsg := "✗ Failed to build Sigstore verification policy"
+		return nil, logMsg, err
+	}
+
+	sigstoreVerified, err := sgVerifier.Verify(att, sgPolicy)
+	if err != nil {
+		logMsg := "✗ Sigstore verification failed"
+		return nil, logMsg, err
+	}
+
+	// Verify extensions
+	certExtVerified, err := verification.VerifyCertExtensions(sigstoreVerified, ec)
+	if err != nil {
+		logMsg := "✗ Policy verification failed"
+		return nil, logMsg, err
+	}
+
+	return certExtVerified, "", nil
+}
@@ -0,0 +1,117 @@
+//go:build integration
+
+package verify
+
+import (
+	"testing"
+
+	"github.com/cli/cli/v2/pkg/cmd/attestation/api"
+	"github.com/cli/cli/v2/pkg/cmd/attestation/artifact"
+	"github.com/cli/cli/v2/pkg/cmd/attestation/io"
+	"github.com/cli/cli/v2/pkg/cmd/attestation/test"
+	"github.com/cli/cli/v2/pkg/cmd/attestation/verification"
+	"github.com/sigstore/sigstore-go/pkg/fulcio/certificate"
+	"github.com/stretchr/testify/require"
+)
+
+func getAttestationsFor(t *testing.T, bundlePath string) []*api.Attestation {
+	t.Helper()
+
+	attestations, err := verification.GetLocalAttestations(bundlePath)
+	require.NoError(t, err)
+
+	return attestations
+}
+
+func TestVerifyAttestations(t *testing.T) {
+	sgVerifier := verification.NewLiveSigstoreVerifier(verification.SigstoreConfig{
+		Logger: io.NewTestHandler(),
+	})
+
+	certSummary := certificate.Summary{}
+	certSummary.SourceRepositoryOwnerURI = "https://github.com/sigstore"
+	certSummary.SourceRepositoryURI = "https://github.com/sigstore/sigstore-js"
+	certSummary.Issuer = verification.GitHubOIDCIssuer
+
+	ec := verification.EnforcementCriteria{
+		Certificate:   certSummary,
+		PredicateType: verification.SLSAPredicateV1,
+		SANRegex:      "^https://github.com/sigstore/",
+	}
+	require.NoError(t, ec.Valid())
+
+	artifactPath := test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz")
+	a, err := artifact.NewDigestedArtifact(nil, artifactPath, "sha512")
+	require.NoError(t, err)
+
+	t.Run("all attestations pass verification", func(t *testing.T) {
+		attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl")
+		require.Len(t, attestations, 2)
+		results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec)
+		require.NoError(t, err)
+		require.Zero(t, errMsg)
+		require.Len(t, results, 2)
+	})
+
+	t.Run("passes verification with 2/3 attestations passing Sigstore verification", func(t *testing.T) {
+		invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json")
+		attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl")
+		attestations = append(attestations, invalidBundle[0])
+		require.Len(t, attestations, 3)
+
+		results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec)
+		require.NoError(t, err)
+		require.Zero(t, errMsg)
+		require.Len(t, results, 2)
+	})
+
+	t.Run("fails verification when Sigstore verification fails", func(t *testing.T) {
+		invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json")
+		invalidBundle2 := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json")
+		attestations := append(invalidBundle, invalidBundle2...)
+		require.Len(t, attestations, 2)
+
+		results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec)
+		require.Error(t, err)
+		require.Contains(t, errMsg, "✗ Sigstore verification failed")
+		require.Nil(t, results)
+	})
+
+	t.Run("attestations fail to verify when cert extensions don't match enforcement criteria", func(t *testing.T) {
+		sgjAttestation := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl")
+		reusableWorkflowAttestations := getAttestationsFor(t, "../test/data/reusable-workflow-attestation.sigstore.json")
+		attestations := []*api.Attestation{sgjAttestation[0], reusableWorkflowAttestations[0], sgjAttestation[1]}
+		require.Len(t, attestations, 3)
+
+		rwfResult := verification.BuildMockResult(reusableWorkflowAttestations[0].Bundle, "", "https://github.com/malancas", "", verification.GitHubOIDCIssuer)
+		sgjResult := verification.BuildSigstoreJsMockResult(t)
+		mockResults := []*verification.AttestationProcessingResult{&sgjResult, &rwfResult, &sgjResult}
+		mockSgVerifier := verification.NewMockSigstoreVerifierWithMockResults(t, mockResults)
+
+		// we want to test that attestations that pass Sigstore verification but fail
+		// cert extension verification are filtered out properly in the second step
+		// in verifyAttestations. By using a mock Sigstore verifier, we can ensure
+		// that the call to verification.VerifyCertExtensions in verifyAttestations
+		// is filtering out attestations as expected
+		results, errMsg, err := verifyAttestations(*a, attestations, mockSgVerifier, ec)
+		require.NoError(t, err)
+		require.Zero(t, errMsg)
+		require.Len(t, results, 2)
+		for _, result := range results {
+			require.NotEqual(t, result.Attestation.Bundle, reusableWorkflowAttestations[0].Bundle)
+		}
+	})
+
+	t.Run("fails verification when cert extension verification fails", func(t *testing.T) {
+		attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl")
+		require.Len(t, attestations, 2)
+
+		expectedCriteria := ec
+		expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong"
+
+		results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, expectedCriteria)
+		require.Error(t, err)
+		require.Contains(t, errMsg, "✗ Policy verification failed")
+		require.Nil(t, results)
+	})
+}
@@ -244,21 +244,9 @@ func runVerify(opts *Options) error {
 
 	opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", ec.PredicateType)
 
-	sp, err := buildSigstoreVerifyPolicy(ec, *artifact)
+	verified, errMsg, err := verifyAttestations(*artifact, attestations, opts.SigstoreVerifier, ec)
 	if err != nil {
-		opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy"))
-		return err
-	}
-
-	verifyResults, err := opts.SigstoreVerifier.Verify(attestations, sp)
-	if err != nil {
-		opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed"))
-		return err
-	}
-
-	// Verify extensions
-	if err := verification.VerifyCertExtensions(verifyResults, ec); err != nil {
-		opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Policy verification failed"))
+		opts.Logger.Println(opts.Logger.ColorScheme.Red(errMsg))
 		return err
 	}
 
@@ -267,7 +255,7 @@ func runVerify(opts *Options) error {
 	// If an exporter is provided with the --json flag, write the results to the terminal in JSON format
 	if opts.exporter != nil {
 		// print the results to the terminal as an array of JSON objects
-		if err = opts.exporter.Write(opts.Logger.IO, verifyResults); err != nil {
+		if err = opts.exporter.Write(opts.Logger.IO, verified); err != nil {
 			opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output"))
 			return err
 		}
@@ -277,7 +265,7 @@ func runVerify(opts *Options) error {
 	opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg())
 
 	// Otherwise print the results to the terminal in a table
-	tableContent, err := buildTableVerifyContent(opts.Tenant, verifyResults)
+	tableContent, err := buildTableVerifyContent(opts.Tenant, verified)
 	if err != nil {
 		opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results"))
 		return err