Improve error message when verifying certificate identity (#218)

* Improve error message when verifying certificate identify Signed-off-by: Cody Soyland <[email protected]> * Test err unwrap Signed-off-by: Cody Soyland <[email protected]> --------- Signed-off-by: Cody Soyland <[email protected]>
sigstore · Jun 27, 2024 · 9554a92 · 9554a92
1 parent 04843b5
commit 9554a92
Show file tree

Hide file tree

Showing 5 changed files with 122 additions and 44 deletions.
diff --git a/.gitignore b/.gitignore
@@ -4,3 +4,4 @@
 /sigstore-go
 /tufdata
 /conformance
+/pkg/tuf/testing.local.json
diff --git a/pkg/fulcio/certificate/summarize.go b/pkg/fulcio/certificate/summarize.go
@@ -17,6 +17,7 @@ package certificate
 import (
 	"crypto/x509"
 	"errors"
+	"fmt"
 	"reflect"
 )
 
@@ -34,7 +35,7 @@ const (
 )
 
 type SubjectAlternativeName struct {
-	Type  SubjectAlternativeNameType `json:"type,omitempty""`
+	Type  SubjectAlternativeNameType `json:"type,omitempty"`
 	Value string                     `json:"value,omitempty"`
 }
 
@@ -44,6 +45,16 @@ type Summary struct {
 	Extensions
 }
 
+type ErrCompareExtensions struct {
+	field    string
+	expected string
+	actual   string
+}
+
+func (e *ErrCompareExtensions) Error() string {
+	return fmt.Sprintf("expected %s to be \"%s\", got \"%s\"", e.field, e.expected, e.actual)
+}
+
 func SummarizeCertificate(cert *x509.Certificate) (Summary, error) {
 	extensions, err := ParseExtensions(cert.Extensions)
 
@@ -68,10 +79,10 @@ func SummarizeCertificate(cert *x509.Certificate) (Summary, error) {
 	return Summary{CertificateIssuer: cert.Issuer.String(), SubjectAlternativeName: san, Extensions: extensions}, nil
 }
 
-// CompareExtensions compares two Extensions structs and returns true if their
-// public fields are equal. It returns false otherwise. Empty fields in the
+// CompareExtensions compares two Extensions structs and returns an error if
+// any set values in the expected struct not equal. Empty fields in the
 // expectedExt struct are ignored.
-func CompareExtensions(expectedExt, actualExt Extensions) bool {
+func CompareExtensions(expectedExt, actualExt Extensions) error {
 	expExtValue := reflect.ValueOf(expectedExt)
 	actExtValue := reflect.ValueOf(actualExt)
 
@@ -84,11 +95,11 @@ func CompareExtensions(expectedExt, actualExt Extensions) bool {
 			actualFieldVal := actExtValue.FieldByName(field.Name)
 			if actualFieldVal.IsValid() {
 				if expectedFieldVal.Interface() != actualFieldVal.Interface() {
-					return false
+					return &ErrCompareExtensions{field.Name, fmt.Sprintf("%v", expectedFieldVal.Interface()), fmt.Sprintf("%v", actualFieldVal.Interface())}
 				}
 			}
 		}
 	}
 
-	return true
+	return nil
 }
diff --git a/pkg/fulcio/certificate/summarize_test.go b/pkg/fulcio/certificate/summarize_test.go
@@ -117,21 +117,23 @@ func TestCompareExtensions(t *testing.T) {
 	}
 
 	// Only the specified fields are expected to match
-	assert.True(t, certificate.CompareExtensions(expectedExt, actualExt))
+	assert.NoError(t, certificate.CompareExtensions(expectedExt, actualExt))
 
 	// Blank fields are ignored
 	expectedExt = certificate.Extensions{
 		Issuer:             "https://token.actions.githubusercontent.com",
 		GithubWorkflowName: "",
 	}
 
-	assert.True(t, certificate.CompareExtensions(expectedExt, actualExt))
+	assert.NoError(t, certificate.CompareExtensions(expectedExt, actualExt))
 
 	// but if any of the fields don't match, it should return false
 	expectedExt = certificate.Extensions{
 		Issuer:             "https://token.actions.githubusercontent.com",
 		GithubWorkflowName: "Final",
 	}
 
-	assert.False(t, certificate.CompareExtensions(expectedExt, actualExt))
+	errCompareExtensions := &certificate.ErrCompareExtensions{}
+	assert.ErrorAs(t, certificate.CompareExtensions(expectedExt, actualExt), &errCompareExtensions)
+	assert.Equal(t, errCompareExtensions.Error(), "expected GithubWorkflowName to be \"Final\", got \"Release\"")
 }
diff --git a/pkg/verify/certificate_identity.go b/pkg/verify/certificate_identity.go
@@ -17,6 +17,7 @@ package verify
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"regexp"
 
 	"github.com/sigstore/sigstore-go/pkg/fulcio/certificate"
@@ -34,6 +35,48 @@ type CertificateIdentity struct {
 
 type CertificateIdentities []CertificateIdentity
 
+type ErrSANTypeMismatch struct {
+	expected string
+	actual   string
+}
+
+func (e *ErrSANTypeMismatch) Error() string {
+	return fmt.Sprintf("expected SAN type %s, got %s", e.expected, e.actual)
+}
+
+type ErrSANValueMismatch struct {
+	expected string
+	actual   string
+}
+
+func (e *ErrSANValueMismatch) Error() string {
+	return fmt.Sprintf("expected SAN value \"%s\", got \"%s\"", e.expected, e.actual)
+}
+
+type ErrSANValueRegexMismatch struct {
+	regex string
+	value string
+}
+
+func (e *ErrSANValueRegexMismatch) Error() string {
+	return fmt.Sprintf("expected SAN value to match regex \"%s\", got \"%s\"", e.regex, e.value)
+}
+
+type ErrNoMatchingCertificateIdentity struct {
+	errors []error
+}
+
+func (e *ErrNoMatchingCertificateIdentity) Error() string {
+	if len(e.errors) > 0 {
+		return fmt.Sprintf("no matching CertificateIdentity found, last error: %v", e.errors[len(e.errors)-1])
+	}
+	return "no matching CertificateIdentity found"
+}
+
+func (e *ErrNoMatchingCertificateIdentity) Unwrap() []error {
+	return e.errors
+}
+
 // NewSANMatcher provides an easier way to create a SubjectAlternativeNameMatcher.
 // If the regexpStr fails to compile into a Regexp, an error is returned.
 func NewSANMatcher(sanValue string, sanType string, regexpStr string) (SubjectAlternativeNameMatcher, error) {
@@ -63,31 +106,22 @@ func (s *SubjectAlternativeNameMatcher) MarshalJSON() ([]byte, error) {
 
 // Verify checks if the actualCert matches the SANMatcher's Type, Value, and
 // Regexp – if those values have been provided.
-func (s SubjectAlternativeNameMatcher) Verify(actualCert certificate.Summary) bool {
-	var typeMatches bool
-	var valueMatches bool
-	var regexMatches bool
-
-	// if a {SAN Type, Value, Regexp} was not specified, default to true
-	if s.SubjectAlternativeName.Type != "" {
-		typeMatches = s.Type == actualCert.SubjectAlternativeName.Type
-	} else {
-		typeMatches = true
+func (s SubjectAlternativeNameMatcher) Verify(actualCert certificate.Summary) error {
+	if s.SubjectAlternativeName.Type != "" &&
+		actualCert.SubjectAlternativeName.Type != s.SubjectAlternativeName.Type {
+		return &ErrSANTypeMismatch{string(s.SubjectAlternativeName.Type), string(actualCert.SubjectAlternativeName.Type)}
 	}
 
-	if s.SubjectAlternativeName.Value != "" {
-		valueMatches = s.Value == actualCert.SubjectAlternativeName.Value
-	} else {
-		valueMatches = true
+	if s.SubjectAlternativeName.Value != "" &&
+		actualCert.SubjectAlternativeName.Value != s.SubjectAlternativeName.Value {
+		return &ErrSANValueMismatch{string(s.SubjectAlternativeName.Value), string(actualCert.SubjectAlternativeName.Value)}
 	}
 
-	if s.Regexp.String() != "" {
-		regexMatches = s.Regexp.MatchString(actualCert.SubjectAlternativeName.Value)
-	} else {
-		regexMatches = true
+	if s.Regexp.String() != "" &&
+		!s.Regexp.MatchString(actualCert.SubjectAlternativeName.Value) {
+		return &ErrSANValueRegexMismatch{string(s.Regexp.String()), string(actualCert.SubjectAlternativeName.Value)}
 	}
-
-	return typeMatches && valueMatches && regexMatches
+	return nil
 }
 
 func NewCertificateIdentity(sanMatcher SubjectAlternativeNameMatcher, extensions certificate.Extensions) (CertificateIdentity, error) {
@@ -116,21 +150,31 @@ func NewShortCertificateIdentity(issuer, sanValue, sanType, sanRegex string) (Ce
 	return NewCertificateIdentity(sanMatcher, certificate.Extensions{Issuer: issuer})
 }
 
+// Verify verifies the CertificateIdentities, and if ANY of them match the cert,
+// it returns the CertificateIdentity that matched. If none match, it returns an
+// error.
 func (i CertificateIdentities) Verify(cert certificate.Summary) (*CertificateIdentity, error) {
+	multierr := &ErrNoMatchingCertificateIdentity{}
+	var err error
 	for _, ci := range i {
-		if ci.Verify(cert) {
+		if err = ci.Verify(cert); err == nil {
 			return &ci, nil
 		}
+		multierr.errors = append(multierr.errors, err)
 	}
-
-	return nil, errors.New("no matching certificate identity found")
+	return nil, multierr
 }
 
 // Verify checks if the actualCert matches the CertificateIdentity's SAN and
 // any of the provided OID extension values. Any empty values are ignored.
-func (c CertificateIdentity) Verify(actualCert certificate.Summary) bool {
-	sanMatches := c.SubjectAlternativeName.Verify(actualCert)
-	extensionsMatch := certificate.CompareExtensions(c.Extensions, actualCert.Extensions)
+func (c CertificateIdentity) Verify(actualCert certificate.Summary) error {
+	var err error
+	if err = c.SubjectAlternativeName.Verify(actualCert); err != nil {
+		return err
+	}
+	if err = certificate.CompareExtensions(c.Extensions, actualCert.Extensions); err != nil {
+		return err
+	}
 
-	return sanMatches && extensionsMatch
+	return nil
 }
diff --git a/pkg/verify/certificate_identity_test.go b/pkg/verify/certificate_identity_test.go
@@ -58,36 +58,56 @@ func TestCertificateIdentityVerify(t *testing.T) {
 
 	// First, let's test happy paths:
 	issuerOnlyID, _ := certIDForTesting("", "", "", ActionsIssuerValue, "")
-	assert.True(t, issuerOnlyID.Verify(actualCert))
+	assert.NoError(t, issuerOnlyID.Verify(actualCert))
 
 	sanValueOnly, _ := certIDForTesting(SigstoreSanValue, "", "", "", "")
-	assert.True(t, sanValueOnly.Verify(actualCert))
+	assert.NoError(t, sanValueOnly.Verify(actualCert))
 
 	sanRegexOnly, _ := certIDForTesting("", "", SigstoreSanRegex, "", "")
-	assert.True(t, sanRegexOnly.Verify(actualCert))
+	assert.NoError(t, sanRegexOnly.Verify(actualCert))
 
 	// multiple values can be specified
 	sanRegexAndIssuer, _ := certIDForTesting("", "", SigstoreSanRegex, ActionsIssuerValue, "github-hosted")
-	assert.True(t, sanRegexAndIssuer.Verify(actualCert))
+	assert.NoError(t, sanRegexAndIssuer.Verify(actualCert))
 
 	// unhappy paths:
 	// wrong issuer
 	sanRegexAndWrongIssuer, _ := certIDForTesting("", "", SigstoreSanRegex, "https://token.actions.example.com", "")
-	assert.False(t, sanRegexAndWrongIssuer.Verify(actualCert))
+	errCompareExtensions := &certificate.ErrCompareExtensions{}
+	assert.ErrorAs(t, sanRegexAndWrongIssuer.Verify(actualCert), &errCompareExtensions)
+	assert.Equal(t, "expected Issuer to be \"https://token.actions.example.com\", got \"https://token.actions.githubusercontent.com\"", errCompareExtensions.Error())
+
+	// bad san regex
+	badRegex, _ := certIDForTesting("", "", "^badregex.*", "", "")
+	errSANValueRegexMismatch := &ErrSANValueRegexMismatch{}
+	assert.ErrorAs(t, badRegex.Verify(actualCert), &errSANValueRegexMismatch)
+	assert.Equal(t, "expected SAN value to match regex \"^badregex.*\", got \"https://github.com/sigstore/sigstore-js/.github/workflows/release.yml@refs/heads/main\"", errSANValueRegexMismatch.Error())
 
 	// right san value, wrong san type
+	errSANTypeMismatch := &ErrSANTypeMismatch{}
 	sanValueAndWrongType, _ := certIDForTesting(SigstoreSanValue, "DNS", "", "", "")
-	assert.False(t, sanValueAndWrongType.Verify(actualCert))
+	assert.ErrorAs(t, sanValueAndWrongType.Verify(actualCert), &errSANTypeMismatch)
+	assert.Equal(t, "expected SAN type DNS, got URI", errSANTypeMismatch.Error())
 
 	// if we have an array of certIDs, only one needs to match
 	ci, err := CertificateIdentities{sanRegexAndWrongIssuer, sanRegexAndIssuer}.Verify(actualCert)
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	assert.Equal(t, *ci, sanRegexAndIssuer)
 
 	// if none match, we fail
 	ci, err = CertificateIdentities{sanValueAndWrongType, sanRegexAndWrongIssuer}.Verify(actualCert)
-	assert.NotNil(t, err)
+	assert.Error(t, err)
+	assert.Equal(t, "no matching CertificateIdentity found, last error: expected Issuer to be \"https://token.actions.example.com\", got \"https://token.actions.githubusercontent.com\"", err.Error())
 	assert.Nil(t, ci)
+	// test err unwrap for previous error
+	errCompareExtensions = &certificate.ErrCompareExtensions{}
+	assert.ErrorAs(t, err, &errCompareExtensions)
+	assert.Equal(t, "expected Issuer to be \"https://token.actions.example.com\", got \"https://token.actions.githubusercontent.com\"", errCompareExtensions.Error())
+
+	// if no certIDs are specified, we fail
+	_, err = CertificateIdentities{}.Verify(actualCert)
+	assert.Error(t, err)
+	assert.Equal(t, "no matching CertificateIdentity found", err.Error())
 }
 
 func TestThatCertIDsAreFullySpecified(t *testing.T) {