diff --git a/pkg/cosign/certextensions.go b/pkg/cosign/certextensions.go index 376a2add8bd..feb6a60f7d3 100644 --- a/pkg/cosign/certextensions.go +++ b/pkg/cosign/certextensions.go @@ -15,7 +15,13 @@ package cosign -import "crypto/x509" +import ( + "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" + "errors" + "fmt" +) type CertExtensions struct { Cert *x509.Certificate @@ -30,6 +36,8 @@ var ( CertExtensionGithubWorkflowRepository = "1.3.6.1.4.1.57264.1.5" CertExtensionGithubWorkflowRef = "1.3.6.1.4.1.57264.1.6" + OIDOtherName = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 7} + CertExtensionMap = map[string]string{ CertExtensionOIDCIssuer: "oidcIssuer", CertExtensionGithubWorkflowTrigger: "githubWorkflowTrigger", @@ -38,6 +46,9 @@ var ( CertExtensionGithubWorkflowRepository: "githubWorkflowRepository", CertExtensionGithubWorkflowRef: "githubWorkflowRef", } + + // OID for Subject Alternative Name + SANOID = asn1.ObjectIdentifier{2, 5, 29, 17} ) func (ce *CertExtensions) certExtensions() map[string]string { @@ -82,3 +93,103 @@ func (ce *CertExtensions) GetCertExtensionGithubWorkflowRepository() string { func (ce *CertExtensions) GetCertExtensionGithubWorkflowRef() string { return ce.certExtensions()["githubWorkflowRef"] } + +// TODO: Move (un)marshalling to sigstore/sigstore +// OtherName describes a name related to a certificate which is not in one +// of the standard name formats. RFC 5280, 4.2.1.6: +// +// OtherName ::= SEQUENCE { +// type-id OBJECT IDENTIFIER, +// value [0] EXPLICIT ANY DEFINED BY type-id } +// +// OtherName for Fulcio-issued certificates only supports UTF-8 strings as values. +type OtherName struct { + ID asn1.ObjectIdentifier + Value string `asn1:"utf8,explicit,tag:0"` +} + +// MarshalOtherNameSAN creates a Subject Alternative Name extension +// with an OtherName sequence. RFC 5280, 4.2.1.6: +// +// SubjectAltName ::= GeneralNames +// GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName +// GeneralName ::= CHOICE { +// +// otherName [0] OtherName, +// ... } +func MarshalOtherNameSAN(name string, critical bool) (*pkix.Extension, error) { + o := OtherName{ + ID: OIDOtherName, + Value: name, + } + bytes, err := asn1.MarshalWithParams(o, "tag:0") + if err != nil { + return nil, err + } + + sans, err := asn1.Marshal([]asn1.RawValue{{FullBytes: bytes}}) + if err != nil { + return nil, err + } + return &pkix.Extension{ + Id: SANOID, + Critical: critical, + Value: sans, + }, nil +} + +// UnmarshalOtherNameSAN extracts a UTF-8 string from the OtherName +// field in the Subject Alternative Name extension. +func UnmarshalOtherNameSAN(exts []pkix.Extension) (string, error) { + var otherNames []string + + for _, e := range exts { + if !e.Id.Equal(SANOID) { + continue + } + + var seq asn1.RawValue + rest, err := asn1.Unmarshal(e.Value, &seq) + if err != nil { + return "", err + } else if len(rest) != 0 { + return "", fmt.Errorf("trailing data after X.509 extension") + } + if !seq.IsCompound || seq.Tag != asn1.TagSequence || seq.Class != asn1.ClassUniversal { + return "", asn1.StructuralError{Msg: "bad SAN sequence"} + } + + rest = seq.Bytes + for len(rest) > 0 { + var v asn1.RawValue + rest, err = asn1.Unmarshal(rest, &v) + if err != nil { + return "", err + } + + // skip all GeneralName fields except OtherName + if v.Tag != 0 { + continue + } + + var other OtherName + _, err := asn1.UnmarshalWithParams(v.FullBytes, &other, "tag:0") + if err != nil { + return "", fmt.Errorf("could not parse requested OtherName SAN: %w", err) + } + if !other.ID.Equal(OIDOtherName) { + return "", fmt.Errorf("unexpected OID for OtherName, expected %v, got %v", OIDOtherName, other.ID) + } + otherNames = append(otherNames, other.Value) + } + } + + if len(otherNames) == 0 { + return "", errors.New("no OtherName found") + } + if len(otherNames) != 1 { + return "", errors.New("expected only one OtherName") + } + + return otherNames[0], nil +} diff --git a/pkg/cosign/certextensions_test.go b/pkg/cosign/certextensions_test.go index d966c958b87..51ae1af18c1 100644 --- a/pkg/cosign/certextensions_test.go +++ b/pkg/cosign/certextensions_test.go @@ -18,6 +18,8 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/asn1" + "encoding/hex" + "strings" "testing" ) @@ -64,3 +66,166 @@ func TestCertExtensions(t *testing.T) { t.Fatal("CertExtension does not extract field 'githubWorkflowRef' correctly") } } + +func TestMarshalAndUnmarshalOtherNameSAN(t *testing.T) { + otherName := "foo!example.com" + critical := true + + ext, err := MarshalOtherNameSAN(otherName, critical) + if err != nil { + t.Fatalf("unexpected error for MarshalOtherNameSAN: %v", err) + } + if ext.Critical != critical { + t.Fatalf("expected extension to be critical") + } + if !ext.Id.Equal(SANOID) { + t.Fatalf("expected extension's OID to be SANs OID") + } + // https://lapo.it/asn1js/#MCGgHwYKKwYBBAGDvzABB6ARDA9mb28hZXhhbXBsZS5jb20 + // 30 - Constructed sequence + // 21 - length of sequence + // A0 - Context-specific (class 2) (bits 8,7) with Constructed bit (bit 6) and 0 tag + // 1F - length of context-specific field (OID) + // 06 - OID tag + // 0A - length of OID + // 2B 06 01 04 01 83 BF 30 01 07 - OID + // A0 - Context-specific (class 2) with Constructed bit and 0 tag + // (needed for EXPLICIT encoding, which wraps field in outer encoding) + // 11 - length of context-specific field (string) + // 0C - UTF8String tag + // 0F - length of string + // 66 6F 6F 21 65 78 61 6D 70 6C 65 2E 63 6F 6D - string + if hex.EncodeToString(ext.Value) != "3021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d" { + t.Fatalf("unexpected ASN.1 encoding") + } + + on, err := UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err != nil { + t.Fatalf("unexpected error for UnmarshalOtherNameSAN: %v", err) + } + if on != otherName { + t.Fatalf("unexpected OtherName, expected %s, got %s", otherName, on) + } +} + +func TestUnmarshalOtherNameSANFailures(t *testing.T) { + var err error + + // failure: no SANs extension + ext := &pkix.Extension{ + Id: asn1.ObjectIdentifier{}, + Critical: true, + Value: []byte{}, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "no OtherName found") { + t.Fatalf("expected error finding no OtherName, got %v", err) + } + + // failure: bad sequence + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: []byte{}, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "sequence truncated") { + t.Fatalf("expected error with invalid ASN.1, got %v", err) + } + + // failure: extra data after valid sequence + b, _ := hex.DecodeString("3021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d" + "30") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "trailing data after X.509 extension") { + t.Fatalf("expected error with extra data, got %v", err) + } + + // failure: non-universal class (Change last two bits: 30 = 00110000 => 10110000 -> B0) + b, _ = hex.DecodeString("B021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") { + t.Fatalf("expected error with non-universal class, got %v", err) + } + + // failure: not compound sequence (Change 6th bit: 30 = 00110000 => 00010000 -> 10) + b, _ = hex.DecodeString("1021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") { + t.Fatalf("expected error with non-compound sequence, got %v", err) + } + + // failure: non-sequence tag (Change lower 5 bits: 30 = 00110000 => 00100010 -> 12) + b, _ = hex.DecodeString("1221a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "bad SAN sequence") { + t.Fatalf("expected error with non-sequence tag, got %v", err) + } + + // failure: no GeneralName with tag=0 (Change lower 5 bits of first sequence field: 3021a01f -> 3021a11f) + b, _ = hex.DecodeString("3021a11f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "no OtherName found") { + t.Fatalf("expected error with no GeneralName, got %v", err) + } + + // failure: invalid OtherName (Change tag of UTF8String field to 1: a0110c0f -> a1110c0f) + b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a1110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "could not parse requested OtherName SAN") { + t.Fatalf("expected error with invalid OtherName, got %v", err) + } + + // failure: OtherName has wrong OID (2b0601040183bf300107 -> 2b0601040183bf300108) + b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "unexpected OID for OtherName") { + t.Fatalf("expected error with wrong OID, got %v", err) + } + + // failure: multiple OtherName fields (Increase sequence size from 0x21 -> 0x42, duplicate OtherName) + b, _ = hex.DecodeString("3042a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6da01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: SANOID, + Critical: true, + Value: b, + } + _, err = UnmarshalOtherNameSAN([]pkix.Extension{*ext}) + if err == nil || !strings.Contains(err.Error(), "expected only one OtherName") { + t.Fatalf("expected error with multiple OtherName fields, got %v", err) + } +} diff --git a/pkg/cosign/verify.go b/pkg/cosign/verify.go index a82736e1f49..52b0a988276 100644 --- a/pkg/cosign/verify.go +++ b/pkg/cosign/verify.go @@ -21,6 +21,7 @@ import ( "crypto/ecdsa" "crypto/sha256" "crypto/x509" + "encoding/asn1" "encoding/base64" "encoding/hex" "encoding/json" @@ -169,6 +170,20 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver return nil, fmt.Errorf("invalid certificate found on signature: %w", err) } + // Handle certificates where the Subject Alternative Name is not set to a supported + // GeneralName (RFC 5280 4.2.1.6). Go only supports DNS, IP addresses, email addresses, + // or URIs as SANs. Fulcio can issue a certificate with an OtherName GeneralName, so + // remove the unhandled critical SAN extension before verifying. + if len(cert.UnhandledCriticalExtensions) > 0 { + var unhandledExts []asn1.ObjectIdentifier + for _, oid := range cert.UnhandledCriticalExtensions { + if !oid.Equal(SANOID) { + unhandledExts = append(unhandledExts, oid) + } + } + cert.UnhandledCriticalExtensions = unhandledExts + } + // Now verify the cert, then the signature. chains, err := TrustedCert(cert, co.RootCerts, co.IntermediateCerts) if err != nil { @@ -332,6 +347,11 @@ func getSubjectAlternateNames(cert *x509.Certificate) []string { for _, uri := range cert.URIs { sans = append(sans, uri.String()) } + // ignore error if there's no OtherName SAN + otherName, _ := UnmarshalOtherNameSAN(cert.Extensions) + if len(otherName) > 0 { + sans = append(sans, otherName) + } return sans } diff --git a/pkg/cosign/verify_test.go b/pkg/cosign/verify_test.go index 3b23c165c7a..58c25c69e5c 100644 --- a/pkg/cosign/verify_test.go +++ b/pkg/cosign/verify_test.go @@ -22,6 +22,7 @@ import ( "crypto/rand" "crypto/sha256" "crypto/x509" + "crypto/x509/pkix" "encoding/base64" "encoding/hex" "encoding/json" @@ -794,6 +795,7 @@ func TestValidateAndUnpackCertWithIdentities(t *testing.T) { dnsSubjects := []string{"dnssubject.example.com"} ipSubjects := []net.IP{net.ParseIP("1.2.3.4")} uriSubjects := []*url.URL{u} + otherName := "email!example.com" oidcIssuer := "https://accounts.google.com" tests := []struct { @@ -803,6 +805,7 @@ func TestValidateAndUnpackCertWithIdentities(t *testing.T) { emailAddresses []string ipAddresses []net.IP uris []*url.URL + otherName string }{ {identities: nil /* No matches required, checks out */}, {identities: []Identity{ // Strict match on both @@ -846,10 +849,25 @@ func TestValidateAndUnpackCertWithIdentities(t *testing.T) { {SubjectRegExp: ".*url.examp.*", IssuerRegExp: ".*accounts.google.*"}}, uris: uriSubjects, wantErrSubstring: ""}, + {identities: []Identity{ // regex matches otherName + {SubjectRegExp: ".*example.com", IssuerRegExp: ".*accounts.google.*"}}, + otherName: otherName, + wantErrSubstring: ""}, } for _, tc := range tests { rootCert, rootKey, _ := test.GenerateRootCa() - leafCert, _, _ := test.GenerateLeafCertWithSubjectAlternateNames(tc.dnsNames, tc.emailAddresses, tc.ipAddresses, tc.uris, oidcIssuer, rootCert, rootKey) + var leafCert *x509.Certificate + if len(tc.otherName) == 0 { + leafCert, _, _ = test.GenerateLeafCertWithSubjectAlternateNames(tc.dnsNames, tc.emailAddresses, tc.ipAddresses, tc.uris, oidcIssuer, rootCert, rootKey) + } else { + // generate with OtherName, which will override other SANs + ext, err := MarshalOtherNameSAN(tc.otherName, true) + if err != nil { + t.Fatalf("error marshalling SANs: %v", err) + } + exts := []pkix.Extension{*ext} + leafCert, _, _ = test.GenerateLeafCert("unused", oidcIssuer, rootCert, rootKey, exts...) + } rootPool := x509.NewCertPool() rootPool.AddCert(rootCert) @@ -970,3 +988,43 @@ func TestTrustedCertSuccessChainFromRoot(t *testing.T) { t.Fatalf("expected no error verifying certificate, got %v", err) } } + +func Test_getSubjectAltnernativeNames(t *testing.T) { + rootCert, rootKey, _ := test.GenerateRootCa() + subCert, subKey, _ := test.GenerateSubordinateCa(rootCert, rootKey) + + // generate with OtherName, which will override other SANs + ext, err := MarshalOtherNameSAN("subject-othername", true) + if err != nil { + t.Fatalf("error marshalling SANs: %v", err) + } + exts := []pkix.Extension{*ext} + leafCert, _, _ := test.GenerateLeafCert("unused", "oidc-issuer", subCert, subKey, exts...) + + sans := getSubjectAlternateNames(leafCert) + if len(sans) != 1 { + t.Fatalf("expected 1 SAN field, got %d", len(sans)) + } + if sans[0] != "subject-othername" { + t.Fatalf("unexpected OtherName SAN value") + } + + // generate with all other SANs + leafCert, _, _ = test.GenerateLeafCertWithSubjectAlternateNames([]string{"subject-dns"}, []string{"subject-email"}, []net.IP{{1, 2, 3, 4}}, []*url.URL{{Path: "testURL"}}, "oidc-issuer", subCert, subKey) + sans = getSubjectAlternateNames(leafCert) + if len(sans) != 4 { + t.Fatalf("expected 1 SAN field, got %d", len(sans)) + } + if sans[0] != "subject-dns" { + t.Fatalf("unexpected DNS SAN value") + } + if sans[1] != "subject-email" { + t.Fatalf("unexpected email SAN value") + } + if sans[2] != "1.2.3.4" { + t.Fatalf("unexpected IP SAN value") + } + if sans[3] != "testURL" { + t.Fatalf("unexpected URL SAN value") + } +} diff --git a/pkg/signature/keys.go b/pkg/signature/keys.go index 6c89d22b974..51995b9904a 100644 --- a/pkg/signature/keys.go +++ b/pkg/signature/keys.go @@ -242,5 +242,10 @@ func CertSubject(c *x509.Certificate) string { case c.URIs != nil: return c.URIs[0].String() } + // ignore error if there's no OtherName SAN + otherName, _ := cosign.UnmarshalOtherNameSAN(c.Extensions) + if len(otherName) > 0 { + return otherName + } return "" } diff --git a/pkg/signature/keys_test.go b/pkg/signature/keys_test.go index fc1ed81f18f..b07ecd3d138 100644 --- a/pkg/signature/keys_test.go +++ b/pkg/signature/keys_test.go @@ -17,12 +17,16 @@ package signature import ( "context" "crypto" + "crypto/x509/pkix" "errors" + "net" + "net/url" "os" "testing" "github.com/sigstore/cosign/pkg/blob" "github.com/sigstore/cosign/pkg/cosign" + "github.com/sigstore/cosign/test" sigsignature "github.com/sigstore/sigstore/pkg/signature" "github.com/sigstore/sigstore/pkg/signature/kms" ) @@ -166,3 +170,32 @@ func pass(s string) cosign.PassFunc { return []byte(s), nil } } + +func TestCertSubject(t *testing.T) { + rootCert, rootKey, _ := test.GenerateRootCa() + subCert, subKey, _ := test.GenerateSubordinateCa(rootCert, rootKey) + + // generate with OtherName, which will override other SANs + ext, err := cosign.MarshalOtherNameSAN("subject-othername", true) + if err != nil { + t.Fatalf("error marshalling SANs: %v", err) + } + exts := []pkix.Extension{*ext} + leafCert, _, _ := test.GenerateLeafCert("unused", "oidc-issuer", subCert, subKey, exts...) + if otherName := CertSubject(leafCert); otherName != "subject-othername" { + t.Fatalf("unexpected otherName, got %s", otherName) + } + + // generate with email + leafCert, _, _ = test.GenerateLeafCert("subject-email", "oidc-issuer", subCert, subKey) + if email := CertSubject(leafCert); email != "subject-email" { + t.Fatalf("unexpected email address, got %s", email) + } + + // generate with URI + uri, _ := url.Parse("spiffe://domain/user") + leafCert, _, _ = test.GenerateLeafCertWithSubjectAlternateNames([]string{}, []string{}, []net.IP{}, []*url.URL{uri}, "oidc-issuer", subCert, subKey) + if uri := CertSubject(leafCert); uri != "spiffe://domain/user" { + t.Fatalf("unexpected URI, got %s", uri) + } +}