From 547ee877657cf407012e540c60a6f10070a99193 Mon Sep 17 00:00:00 2001 From: Hayden Blauzvern Date: Wed, 28 Sep 2022 16:12:32 +0000 Subject: [PATCH 1/3] Add support for Fulcio username identity in SAN We have changed the format of the username identity to not look like an email, and so we also needed to change which SAN the username was set in. Now using OtherName, I've added some custom unmarshalling to extract the OtherName SAN, because Go doesn't support this SAN type. Note in verify.go, I had to handle when the extension was critical. Since Go doesn't handle the extension, but it must be marked critical according to RFC5280, the cert will fail verification. We can simply remove the extension from the list of unhandled extensions before verifying. Signed-off-by: Hayden Blauzvern --- pkg/cosign/certextensions.go | 112 +++++++++++++++++++- pkg/cosign/certextensions_test.go | 165 ++++++++++++++++++++++++++++++ pkg/cosign/verify.go | 19 ++++ pkg/cosign/verify_test.go | 60 ++++++++++- pkg/signature/keys.go | 4 + pkg/signature/keys_test.go | 33 ++++++ 6 files changed, 391 insertions(+), 2 deletions(-) diff --git a/pkg/cosign/certextensions.go b/pkg/cosign/certextensions.go index 376a2add8bd..f1bc7ba66fb 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", @@ -82,3 +90,105 @@ 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"` +} + +// MarshalSANS 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 MarshalSANS(name string, critical bool) (*pkix.Extension, error) { + var rawValues []asn1.RawValue + o := OtherName{ + ID: OIDOtherName, + Value: name, + } + bytes, err := asn1.MarshalWithParams(o, "tag:0") + if err != nil { + return nil, err + } + rawValues = append(rawValues, asn1.RawValue{FullBytes: bytes}) + + sans, err := asn1.Marshal(rawValues) + if err != nil { + return nil, err + } + return &pkix.Extension{ + Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: critical, + Value: sans, + }, nil +} + +// UnmarshalSANs extracts a UTF-8 string from the OtherName +// field in the Subject Alternative Name extension. +func UnmarshalSANS(exts []pkix.Extension) (string, error) { + var otherNames []string + + for _, e := range exts { + if !e.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) { + 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 != 16 || seq.Class != 0 { + 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: %v", 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..80f12d5ab2f 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 TestMarshalAndUnmarshalSANS(t *testing.T) { + otherName := "foo!example.com" + critical := true + + ext, err := MarshalSANS(otherName, critical) + if err != nil { + t.Fatalf("unexpected error for MarshalSANs: %v", err) + } + if ext.Critical != critical { + t.Fatalf("expected extension to be critical") + } + if !ext.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) { + 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 := UnmarshalSANS([]pkix.Extension{*ext}) + if err != nil { + t.Fatalf("unexpected error for UnmarshalSANs: %v", err) + } + if on != otherName { + t.Fatalf("unexpected OtherName, expected %s, got %s", otherName, on) + } +} + +func TestUnmarshalSANsFailures(t *testing.T) { + var err error + + // failure: no SANs extension + ext := &pkix.Extension{ + Id: asn1.ObjectIdentifier{}, + Critical: true, + Value: []byte{}, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: []byte{}, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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 => 00000010 -> 02) + b, _ = hex.DecodeString("0221a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + ext = &pkix.Extension{ + Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Critical: true, + Value: b, + } + _, err = UnmarshalSANS([]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..26632f8e8fb 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(asn1.ObjectIdentifier{2, 5, 29, 17}) { + 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,10 @@ func getSubjectAlternateNames(cert *x509.Certificate) []string { for _, uri := range cert.URIs { sans = append(sans, uri.String()) } + otherName, _ := UnmarshalSANS(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..7127ff3d658 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 := MarshalSANS(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 := MarshalSANS("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..05ba61150eb 100644 --- a/pkg/signature/keys.go +++ b/pkg/signature/keys.go @@ -242,5 +242,9 @@ func CertSubject(c *x509.Certificate) string { case c.URIs != nil: return c.URIs[0].String() } + otherName, _ := cosign.UnmarshalSANS(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..8b36c14037d 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.MarshalSANS("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) + } +} From dd7eb8e1a589061c82ed5c2c61cf9b0c124dd853 Mon Sep 17 00:00:00 2001 From: Hayden Blauzvern Date: Wed, 28 Sep 2022 16:40:43 +0000 Subject: [PATCH 2/3] fix lint Signed-off-by: Hayden Blauzvern --- pkg/cosign/certextensions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cosign/certextensions.go b/pkg/cosign/certextensions.go index f1bc7ba66fb..5814030e550 100644 --- a/pkg/cosign/certextensions.go +++ b/pkg/cosign/certextensions.go @@ -174,7 +174,7 @@ func UnmarshalSANS(exts []pkix.Extension) (string, error) { var other OtherName _, err := asn1.UnmarshalWithParams(v.FullBytes, &other, "tag:0") if err != nil { - return "", fmt.Errorf("could not parse requested OtherName SAN: %v", err) + 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) From 1c3bf136f122e1a6c3bb9c591baa67d27bf145af Mon Sep 17 00:00:00 2001 From: Hayden Blauzvern Date: Thu, 29 Sep 2022 04:53:47 +0000 Subject: [PATCH 3/3] address comments Signed-off-by: Hayden Blauzvern --- pkg/cosign/certextensions.go | 21 ++++++------ pkg/cosign/certextensions_test.go | 56 +++++++++++++++---------------- pkg/cosign/verify.go | 5 +-- pkg/cosign/verify_test.go | 4 +-- pkg/signature/keys.go | 3 +- pkg/signature/keys_test.go | 2 +- 6 files changed, 47 insertions(+), 44 deletions(-) diff --git a/pkg/cosign/certextensions.go b/pkg/cosign/certextensions.go index 5814030e550..feb6a60f7d3 100644 --- a/pkg/cosign/certextensions.go +++ b/pkg/cosign/certextensions.go @@ -46,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 { @@ -105,7 +108,7 @@ type OtherName struct { Value string `asn1:"utf8,explicit,tag:0"` } -// MarshalSANS creates a Subject Alternative Name extension +// MarshalOtherNameSAN creates a Subject Alternative Name extension // with an OtherName sequence. RFC 5280, 4.2.1.6: // // SubjectAltName ::= GeneralNames @@ -114,8 +117,7 @@ type OtherName struct { // // otherName [0] OtherName, // ... } -func MarshalSANS(name string, critical bool) (*pkix.Extension, error) { - var rawValues []asn1.RawValue +func MarshalOtherNameSAN(name string, critical bool) (*pkix.Extension, error) { o := OtherName{ ID: OIDOtherName, Value: name, @@ -124,26 +126,25 @@ func MarshalSANS(name string, critical bool) (*pkix.Extension, error) { if err != nil { return nil, err } - rawValues = append(rawValues, asn1.RawValue{FullBytes: bytes}) - sans, err := asn1.Marshal(rawValues) + sans, err := asn1.Marshal([]asn1.RawValue{{FullBytes: bytes}}) if err != nil { return nil, err } return &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: critical, Value: sans, }, nil } -// UnmarshalSANs extracts a UTF-8 string from the OtherName +// UnmarshalOtherNameSAN extracts a UTF-8 string from the OtherName // field in the Subject Alternative Name extension. -func UnmarshalSANS(exts []pkix.Extension) (string, error) { +func UnmarshalOtherNameSAN(exts []pkix.Extension) (string, error) { var otherNames []string for _, e := range exts { - if !e.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) { + if !e.Id.Equal(SANOID) { continue } @@ -154,7 +155,7 @@ func UnmarshalSANS(exts []pkix.Extension) (string, error) { } else if len(rest) != 0 { return "", fmt.Errorf("trailing data after X.509 extension") } - if !seq.IsCompound || seq.Tag != 16 || seq.Class != 0 { + if !seq.IsCompound || seq.Tag != asn1.TagSequence || seq.Class != asn1.ClassUniversal { return "", asn1.StructuralError{Msg: "bad SAN sequence"} } diff --git a/pkg/cosign/certextensions_test.go b/pkg/cosign/certextensions_test.go index 80f12d5ab2f..51ae1af18c1 100644 --- a/pkg/cosign/certextensions_test.go +++ b/pkg/cosign/certextensions_test.go @@ -67,18 +67,18 @@ func TestCertExtensions(t *testing.T) { } } -func TestMarshalAndUnmarshalSANS(t *testing.T) { +func TestMarshalAndUnmarshalOtherNameSAN(t *testing.T) { otherName := "foo!example.com" critical := true - ext, err := MarshalSANS(otherName, critical) + ext, err := MarshalOtherNameSAN(otherName, critical) if err != nil { - t.Fatalf("unexpected error for MarshalSANs: %v", err) + t.Fatalf("unexpected error for MarshalOtherNameSAN: %v", err) } if ext.Critical != critical { t.Fatalf("expected extension to be critical") } - if !ext.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) { + if !ext.Id.Equal(SANOID) { t.Fatalf("expected extension's OID to be SANs OID") } // https://lapo.it/asn1js/#MCGgHwYKKwYBBAGDvzABB6ARDA9mb28hZXhhbXBsZS5jb20 @@ -99,16 +99,16 @@ func TestMarshalAndUnmarshalSANS(t *testing.T) { t.Fatalf("unexpected ASN.1 encoding") } - on, err := UnmarshalSANS([]pkix.Extension{*ext}) + on, err := UnmarshalOtherNameSAN([]pkix.Extension{*ext}) if err != nil { - t.Fatalf("unexpected error for UnmarshalSANs: %v", err) + t.Fatalf("unexpected error for UnmarshalOtherNameSAN: %v", err) } if on != otherName { t.Fatalf("unexpected OtherName, expected %s, got %s", otherName, on) } } -func TestUnmarshalSANsFailures(t *testing.T) { +func TestUnmarshalOtherNameSANFailures(t *testing.T) { var err error // failure: no SANs extension @@ -117,18 +117,18 @@ func TestUnmarshalSANsFailures(t *testing.T) { Critical: true, Value: []byte{}, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: []byte{}, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -136,11 +136,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: extra data after valid sequence b, _ := hex.DecodeString("3021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d" + "30") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -148,11 +148,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: non-universal class (Change last two bits: 30 = 00110000 => 10110000 -> B0) b, _ = hex.DecodeString("B021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -160,23 +160,23 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: not compound sequence (Change 6th bit: 30 = 00110000 => 00010000 -> 10) b, _ = hex.DecodeString("1021a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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 => 00000010 -> 02) - b, _ = hex.DecodeString("0221a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") + // failure: non-sequence tag (Change lower 5 bits: 30 = 00110000 => 00100010 -> 12) + b, _ = hex.DecodeString("1221a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -184,11 +184,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: no GeneralName with tag=0 (Change lower 5 bits of first sequence field: 3021a01f -> 3021a11f) b, _ = hex.DecodeString("3021a11f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -196,11 +196,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: invalid OtherName (Change tag of UTF8String field to 1: a0110c0f -> a1110c0f) b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a1110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -208,11 +208,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: OtherName has wrong OID (2b0601040183bf300107 -> 2b0601040183bf300108) b, _ = hex.DecodeString("3021a01f060a2b0601040183bf300108a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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) } @@ -220,11 +220,11 @@ func TestUnmarshalSANsFailures(t *testing.T) { // failure: multiple OtherName fields (Increase sequence size from 0x21 -> 0x42, duplicate OtherName) b, _ = hex.DecodeString("3042a01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6da01f060a2b0601040183bf300107a0110c0f666f6f216578616d706c652e636f6d") ext = &pkix.Extension{ - Id: asn1.ObjectIdentifier{2, 5, 29, 17}, + Id: SANOID, Critical: true, Value: b, } - _, err = UnmarshalSANS([]pkix.Extension{*ext}) + _, 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 26632f8e8fb..52b0a988276 100644 --- a/pkg/cosign/verify.go +++ b/pkg/cosign/verify.go @@ -177,7 +177,7 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver if len(cert.UnhandledCriticalExtensions) > 0 { var unhandledExts []asn1.ObjectIdentifier for _, oid := range cert.UnhandledCriticalExtensions { - if !oid.Equal(asn1.ObjectIdentifier{2, 5, 29, 17}) { + if !oid.Equal(SANOID) { unhandledExts = append(unhandledExts, oid) } } @@ -347,7 +347,8 @@ func getSubjectAlternateNames(cert *x509.Certificate) []string { for _, uri := range cert.URIs { sans = append(sans, uri.String()) } - otherName, _ := UnmarshalSANS(cert.Extensions) + // ignore error if there's no OtherName SAN + otherName, _ := UnmarshalOtherNameSAN(cert.Extensions) if len(otherName) > 0 { sans = append(sans, otherName) } diff --git a/pkg/cosign/verify_test.go b/pkg/cosign/verify_test.go index 7127ff3d658..58c25c69e5c 100644 --- a/pkg/cosign/verify_test.go +++ b/pkg/cosign/verify_test.go @@ -861,7 +861,7 @@ func TestValidateAndUnpackCertWithIdentities(t *testing.T) { 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 := MarshalSANS(tc.otherName, true) + ext, err := MarshalOtherNameSAN(tc.otherName, true) if err != nil { t.Fatalf("error marshalling SANs: %v", err) } @@ -994,7 +994,7 @@ func Test_getSubjectAltnernativeNames(t *testing.T) { subCert, subKey, _ := test.GenerateSubordinateCa(rootCert, rootKey) // generate with OtherName, which will override other SANs - ext, err := MarshalSANS("subject-othername", true) + ext, err := MarshalOtherNameSAN("subject-othername", true) if err != nil { t.Fatalf("error marshalling SANs: %v", err) } diff --git a/pkg/signature/keys.go b/pkg/signature/keys.go index 05ba61150eb..51995b9904a 100644 --- a/pkg/signature/keys.go +++ b/pkg/signature/keys.go @@ -242,7 +242,8 @@ func CertSubject(c *x509.Certificate) string { case c.URIs != nil: return c.URIs[0].String() } - otherName, _ := cosign.UnmarshalSANS(c.Extensions) + // ignore error if there's no OtherName SAN + otherName, _ := cosign.UnmarshalOtherNameSAN(c.Extensions) if len(otherName) > 0 { return otherName } diff --git a/pkg/signature/keys_test.go b/pkg/signature/keys_test.go index 8b36c14037d..b07ecd3d138 100644 --- a/pkg/signature/keys_test.go +++ b/pkg/signature/keys_test.go @@ -176,7 +176,7 @@ func TestCertSubject(t *testing.T) { subCert, subKey, _ := test.GenerateSubordinateCa(rootCert, rootKey) // generate with OtherName, which will override other SANs - ext, err := cosign.MarshalSANS("subject-othername", true) + ext, err := cosign.MarshalOtherNameSAN("subject-othername", true) if err != nil { t.Fatalf("error marshalling SANs: %v", err) }