Skip to content

Commit

Permalink
Merge pull request cli#9877 from malancas/simplify-sigstore-verify-re…
Browse files Browse the repository at this point in the history 
…sult-handling

Simplify Sigstore verification result handling in `gh attestation verify`
  • Loading branch information
@malancas
malancas authored Nov 6, 2024
2 parents 446a0d5 + f376ac1 commit 2f47d1c
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 49 deletions.
10 changes: 5 additions & 5 deletions pkg/cmd/attestation/inspect/inspect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ func runInspect(opts *Options) error {
return fmt.Errorf("failed to build policy: %v", err)
}

res := opts.SigstoreVerifier.Verify(attestations, policy)
if res.Error != nil {
return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", res.Error)
results, err := opts.SigstoreVerifier.Verify(attestations, policy)
if err != nil {
return fmt.Errorf("at least one attestation failed to verify against Sigstore: %v", err)
}

opts.Logger.VerbosePrint(opts.Logger.ColorScheme.Green(
Expand All @@ -152,7 +152,7 @@ func runInspect(opts *Options) error {

// If the user provides the --format=json flag, print the results in JSON format
if opts.exporter != nil {
details, err := getAttestationDetails(opts.Tenant, res.VerifyResults)
details, err := getAttestationDetails(opts.Tenant, results)
if err != nil {
return fmt.Errorf("failed to get attestation detail: %v", err)
}
Expand All @@ -165,7 +165,7 @@ func runInspect(opts *Options) error {
}

// otherwise, print results in a table
details, err := getDetailsAsSlice(opts.Tenant, res.VerifyResults)
details, err := getDetailsAsSlice(opts.Tenant, results)
if err != nil {
return fmt.Errorf("failed to parse attestation details: %v", err)
}
Expand Down
12 changes: 4 additions & 8 deletions pkg/cmd/attestation/verification/mock_verifier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type MockSigstoreVerifier struct {
t *testing.T
}

func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults {
func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
statement := &in_toto.Statement{}
statement.PredicateType = SLSAPredicateV1

Expand All @@ -41,9 +41,7 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve

results := []*AttestationProcessingResult{&result}

return &SigstoreResults{
VerifyResults: results,
}
return results, nil
}

func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier {
Expand All @@ -52,8 +50,6 @@ func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier {

type FailSigstoreVerifier struct{}

func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults {
return &SigstoreResults{
Error: fmt.Errorf("failed to verify attestations"),
}
func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
return nil, fmt.Errorf("failed to verify attestations")
}
25 changes: 7 additions & 18 deletions pkg/cmd/attestation/verification/sigstore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ type AttestationProcessingResult struct {
VerificationResult *verify.VerificationResult `json:"verificationResult"`
}

type SigstoreResults struct {
VerifyResults []*AttestationProcessingResult
Error error
}

type SigstoreConfig struct {
TrustedRoot string
Logger *io.Handler
Expand All @@ -42,7 +37,7 @@ type SigstoreConfig struct {
}

type SigstoreVerifier interface {
Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults
Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error)
}

type LiveSigstoreVerifier struct {
Expand Down Expand Up @@ -172,7 +167,7 @@ func getLowestCertInChain(ca *root.CertificateAuthority) (*x509.Certificate, err
return nil, fmt.Errorf("certificate authority had no certificates")
}

func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) *SigstoreResults {
func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) {
// initialize the processing apResults before attempting to verify
// with multiple verifiers
apResults := make([]*AttestationProcessingResult, len(attestations))
Expand All @@ -192,9 +187,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve
// determine which verifier should attempt verification against the bundle
verifier, issuer, err := v.chooseVerifier(apr.Attestation.Bundle)
if err != nil {
return &SigstoreResults{
Error: fmt.Errorf("failed to find recognized issuer from bundle content: %v", err),
}
return nil, fmt.Errorf("failed to find recognized issuer from bundle content: %v", err)
}

v.config.Logger.VerbosePrintf("Attempting verification against issuer \"%s\"\n", issuer)
Expand All @@ -206,9 +199,7 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve
"Failed to verify against issuer \"%s\" \n\n", issuer,
))

return &SigstoreResults{
Error: fmt.Errorf("verifying with issuer \"%s\"", issuer),
}
return nil, fmt.Errorf("verifying with issuer \"%s\"", issuer)
}

// if verification is successful, add the result
Expand All @@ -221,12 +212,10 @@ func (v *LiveSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve
}

if atLeastOneVerified {
return &SigstoreResults{
VerifyResults: apResults,
}
} else {
return &SigstoreResults{Error: ErrNoAttestationsVerified}
return apResults, nil
}

return nil, ErrNoAttestationsVerified
}

func newCustomVerifier(trustedRoot *root.TrustedRoot) (*verify.SignedEntityVerifier, error) {
Expand Down
24 changes: 12 additions & 12 deletions pkg/cmd/attestation/verification/sigstore_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ func TestLiveSigstoreVerifier(t *testing.T) {
Logger: io.NewTestHandler(),
})

res := verifier.Verify(tc.attestations, publicGoodPolicy(t))
results, err := verifier.Verify(tc.attestations, publicGoodPolicy(t))

if tc.expectErr {
require.Error(t, res.Error, "test case: %s", tc.name)
require.ErrorContains(t, res.Error, tc.errContains, "test case: %s", tc.name)
require.Nil(t, res.VerifyResults, "test case: %s", tc.name)
require.Error(t, err, "test case: %s", tc.name)
require.ErrorContains(t, err, tc.errContains, "test case: %s", tc.name)
require.Nil(t, results, "test case: %s", tc.name)
} else {
require.Equal(t, len(tc.attestations), len(res.VerifyResults), "test case: %s", tc.name)
require.NoError(t, res.Error, "test case: %s", tc.name)
require.Equal(t, len(tc.attestations), len(results), "test case: %s", tc.name)
require.NoError(t, err, "test case: %s", tc.name)
}
}

Expand All @@ -77,9 +77,9 @@ func TestLiveSigstoreVerifier(t *testing.T) {
Logger: io.NewTestHandler(),
})

res := verifier.Verify(attestations, githubPolicy)
require.Len(t, res.VerifyResults, 1)
require.NoError(t, res.Error)
results, err := verifier.Verify(attestations, githubPolicy)
require.Len(t, results, 1)
require.NoError(t, err)
})

t.Run("with custom trusted root", func(t *testing.T) {
Expand All @@ -90,9 +90,9 @@ func TestLiveSigstoreVerifier(t *testing.T) {
TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"),
})

res := verifier.Verify(attestations, publicGoodPolicy(t))
require.Len(t, res.VerifyResults, 2)
require.NoError(t, res.Error)
results, err := verifier.Verify(attestations, publicGoodPolicy(t))
require.Len(t, results, 2)
require.NoError(t, err)
})
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/cmd/attestation/verify/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,14 @@ func runVerify(opts *Options) error {
return err
}

sigstoreRes := opts.SigstoreVerifier.Verify(attestations, sp)
if sigstoreRes.Error != nil {
verifyResults, err := opts.SigstoreVerifier.Verify(attestations, sp)
if err != nil {
opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed"))
return sigstoreRes.Error
return err
}

// Verify extensions
if err := verification.VerifyCertExtensions(sigstoreRes.VerifyResults, ec); err != nil {
if err := verification.VerifyCertExtensions(verifyResults, ec); err != nil {
opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Policy verification failed"))
return err
}
Expand All @@ -292,7 +292,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, sigstoreRes.VerifyResults); err != nil {
if err = opts.exporter.Write(opts.Logger.IO, verifyResults); err != nil {
opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output"))
return err
}
Expand All @@ -302,7 +302,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, sigstoreRes.VerifyResults)
tableContent, err := buildTableVerifyContent(opts.Tenant, verifyResults)
if err != nil {
opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results"))
return err
Expand Down

0 comments on commit 2f47d1c

Please sign in to comment.