From 67724e3bedabbc62f93c7a17706d0a7900d7db49 Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Thu, 16 Jul 2026 16:29:46 -0500 Subject: [PATCH 1/2] Add subject DN components to certificate load URIs Support selecting certificates in the Apple Keychain by subject distinguished name components using the new "cn", "o", "ou", "l", "st", and "c" URI attributes, e.g. "mackms:cn=My+Cert;ou=Engineering". The keychain cannot match individual subject components, and it stores normalized (partially uppercased) subject values, so the components are used to filter the parsed certificates after retrieving the candidates from the keychain. All the given components must match using exact, case-sensitive comparisons, and the multi-valued components can be repeated, requiring every value to be present in the subject. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P3Nxd2zBbULcRAG6HrzQgR --- kms/mackms/mackms.go | 145 +++++++++++++++++++--- kms/mackms/mackms_test.go | 254 +++++++++++++++++++++++++++++++++++++- 2 files changed, 380 insertions(+), 19 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index c2c28003..c3e438cb 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -92,6 +92,63 @@ type certAttributes struct { serialNumber *big.Int keychain string useDataProtectionKeychain bool + + // Subject distinguished name components (cn, o, ou, l, st, c) used to + // filter certificates after retrieving them from the keychain. The + // keychain cannot match individual subject components, and it stores some + // of the subject values in uppercase. + commonName string + organization []string + organizationalUnit []string + locality []string + province []string + country []string +} + +// hasSubjectQuery reports whether at least one subject distinguished name +// component (cn, o, ou, l, st, c) is set. +func (u *certAttributes) hasSubjectQuery() bool { + return u.commonName != "" || len(u.organization) > 0 || + len(u.organizationalUnit) > 0 || len(u.locality) > 0 || + len(u.province) > 0 || len(u.country) > 0 +} + +// matchesSubject reports whether the certificate subject contains all the +// subject distinguished name components in u. Comparisons are exact and +// case-sensitive; for multi-valued components every requested value must be +// present in the certificate subject. It returns true if no components are +// set. +func (u *certAttributes) matchesSubject(cert *x509.Certificate) bool { + if u.commonName != "" && cert.Subject.CommonName != u.commonName { + return false + } + return containsAll(cert.Subject.Organization, u.organization) && + containsAll(cert.Subject.OrganizationalUnit, u.organizationalUnit) && + containsAll(cert.Subject.Locality, u.locality) && + containsAll(cert.Subject.Province, u.province) && + containsAll(cert.Subject.Country, u.country) +} + +// containsAll reports whether values contains all the elements in want. It +// returns true if want is empty. +func containsAll(values, want []string) bool { + for _, w := range want { + if !slices.Contains(values, w) { + return false + } + } + return true +} + +// nonEmptyValues returns the values in vs that are not the empty string. +func nonEmptyValues(vs []string) []string { + var values []string + for _, v := range vs { + if v != "" { + values = append(values, v) + } + } + return values } type algorithmAttributes struct { @@ -376,16 +433,31 @@ func (k *MacKMS) CreateSigner(req *apiv1.CreateSignerRequest) (crypto.Signer, er }, nil } -// LoadCertificate returns an [x509.Certificate] by its label and/or serial -// number. If multiple certificates match, it will return the certificate with -// the most recent NotBefore date among those that have already become valid. -// Future certificates maintain their original order. Expiration is not checked. -// By default Apple Keychain will use the certificate common name as the label. +// LoadCertificate returns an [x509.Certificate] by its label, serial number, +// and/or subject distinguished name components. If multiple certificates +// match, it will return the certificate with the most recent NotBefore date +// among those that have already become valid. Future certificates maintain +// their original order. Expiration is not checked. By default Apple Keychain +// will use the certificate common name as the label. // // Valid names (URIs) are: // - mackms:label=test@example.com // - mackms:serial=2c273934eda8454d2595a94497e2395a // - mackms:label=test@example.com;serial=2c273934eda8454d2595a94497e2395a +// - mackms:cn=My+Cert +// - mackms:cn=My+Cert;o=Smallstep;ou=Engineering;ou=Platform +// - mackms:label=test@example.com;ou=Engineering +// +// The "cn" (common name), "o" (organization), "ou" (organizational unit), "l" +// (locality), "st" (state or province), and "c" (country) attributes select +// certificates by their subject. All the given components must match using +// exact, case-sensitive comparison. The "cn" attribute is single-valued; the +// others can be repeated (e.g. "ou=a;ou=b"), and every given value must be +// present in the certificate subject. Values must be URL-encoded; spaces are +// encoded as "+" or "%20". Subject components can be combined with "label" and +// "serial", which narrow the keychain query. Note that a URI with just an +// attribute name and no value (e.g. "mackms:cn") is the shorthand for a label +// with that name, so subject components always require a value. // // On code-signed applications, it is possible to use the Data Protection // Keychain by default if [UseDataProtectionKeychain] is set to true. You can @@ -399,7 +471,7 @@ func (k *MacKMS) LoadCertificate(req *apiv1.LoadCertificateRequest) (*x509.Certi return nil, fmt.Errorf("loadCertificateRequest 'name' cannot be empty") } - // Require label or serial + // Require label, serial, or a subject component u, err := parseCertURI(req.Name, k.useDataProtectionKeychain, true) if err != nil { return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) @@ -424,6 +496,9 @@ func (k *MacKMS) LoadCertificate(req *apiv1.LoadCertificateRequest) (*x509.Certi // - "mackms:label=my-label" will use "my-label" // - "mackms:my-label" will use the "my-label" // +// The "serial" attribute and the subject components ("cn", "o", "ou", "l", +// "st", "c") are ignored when storing a certificate. +// // On code-signed applications, it is possible to use the Data Protection // Keychain by default if [UseDataProtectionKeychain] is set to true. You can // also select the keychain using the "keychain" attribute: @@ -464,23 +539,29 @@ func (k *MacKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error { return nil } -// LoadCertificateChain returns the leaf [x509.Certificate] by its label and/or -// serial number and its intermediate certificates. If multiple certificates -// match, it will return the certificate with the most recent NotBefore date -// among those that have already become valid. Future certificates maintain -// their original order. Expiration is not checked. By default Apple Keychain -// will use the certificate common name as the label. +// LoadCertificateChain returns the leaf [x509.Certificate] by its label, +// serial number, and/or subject distinguished name components, together with +// its intermediate certificates. If multiple certificates match, it will +// return the certificate with the most recent NotBefore date among those that +// have already become valid. Future certificates maintain their original +// order. Expiration is not checked. By default Apple Keychain will use the +// certificate common name as the label. // // Valid names (URIs) are: // - mackms:label=test@example.com // - mackms:serial=2c273934eda8454d2595a94497e2395a // - mackms:label=test@example.com;serial=2c273934eda8454d2595a94497e2395a +// - mackms:cn=My+Cert;ou=Engineering +// +// Subject components select the leaf certificate only; intermediates are +// located using the authority key identifier. See [MacKMS.LoadCertificate] for +// the subject component matching rules. func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([]*x509.Certificate, error) { if req.Name == "" { return nil, fmt.Errorf("loadCertificateChainRequest 'name' cannot be empty") } - // Require label or serial + // Require label, serial, or a subject component u, err := parseCertURI(req.Name, k.useDataProtectionKeychain, true) if err != nil { return nil, fmt.Errorf("mackms LoadCertificateChain failed: %w", err) @@ -1090,6 +1171,22 @@ func loadCertificate(u *certAttributes, subjectKeyID []byte) (*x509.Certificate, return nil, err } } + + // Filter the certificates by the subject distinguished name components + // (cn, o, ou, l, st, c) in the uri. The keychain cannot match individual + // subject components, and it normalizes the subject values it stores, so + // the parsed certificates are filtered instead. + if u.hasSubjectQuery() { + certs = slices.DeleteFunc(certs, func(cert *x509.Certificate) bool { + return !u.matchesSubject(cert) + }) + if len(certs) == 0 { + return nil, apiv1.NotFoundError{ + Message: "certificate not found", + } + } + } + if len(certs) == 1 { return certs[0], nil } @@ -1283,16 +1380,28 @@ func parseCertURI(rawuri string, useDataProtectionKeychain, requireValue bool) ( if err != nil { return nil, fmt.Errorf("error parsing %q: %w", rawuri, err) } - if requireValue && label == "" && serialNumber == nil { - return nil, fmt.Errorf("error parsing %q: label or serial is required", rawuri) - } - return &certAttributes{ + // Subject distinguished name components (cn, o, ou, l, st, c) are used to + // filter the certificates retrieved from the keychain. Empty values are + // ignored, and o, ou, l, st, and c can be repeated (e.g. "ou=a;ou=b"). + values := uri.Values(u) + attrs := &certAttributes{ label: label, serialNumber: serialNumber, useDataProtectionKeychain: isDataProtectionKeychain(keychain, useDataProtectionKeychain), keychain: keychain, - }, nil + commonName: u.Get("cn"), + organization: nonEmptyValues(values["o"]), + organizationalUnit: nonEmptyValues(values["ou"]), + locality: nonEmptyValues(values["l"]), + province: nonEmptyValues(values["st"]), + country: nonEmptyValues(values["c"]), + } + if requireValue && label == "" && serialNumber == nil && !attrs.hasSubjectQuery() { + return nil, fmt.Errorf("error parsing %q: label, serial, or a subject component (cn, o, ou, l, st, c) is required", rawuri) + } + + return attrs, nil } func parseSearchURI(rawuri string) (*keySearchAttributes, error) { diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index adca47ec..ba756b92 100644 --- a/kms/mackms/mackms_test.go +++ b/kms/mackms/mackms_test.go @@ -578,6 +578,125 @@ func Test_parseURI(t *testing.T) { } } +func Test_parseCertURI(t *testing.T) { + type args struct { + rawuri string + useDataProtectionKeychain bool + requireValue bool + } + tests := []struct { + name string + args args + want *certAttributes + assertion assert.ErrorAssertionFunc + }{ + {"ok label", args{"mackms:label=the-label", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok bare label", args{"the-label", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok label simple uri", args{"mackms:the-label", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok uppercase scheme", args{"MACKMS:label=the-label", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok serial hex", args{"mackms:serial=0x0102", false, true}, &certAttributes{serialNumber: big.NewInt(0x0102)}, assert.NoError}, + {"ok serial decimal", args{"mackms:serial=123456", false, true}, &certAttributes{serialNumber: big.NewInt(123456)}, assert.NoError}, + {"ok label and serial", args{"mackms:label=the-label;serial=0x01", false, true}, &certAttributes{label: "the-label", serialNumber: big.NewInt(1)}, assert.NoError}, + {"ok cn", args{"mackms:cn=My+Cert", false, true}, &certAttributes{commonName: "My Cert"}, assert.NoError}, + {"ok cn percent encoded", args{"mackms:cn=My%20Cert", false, true}, &certAttributes{commonName: "My Cert"}, assert.NoError}, + {"ok subject components", args{"mackms:cn=leaf;o=Smallstep;ou=Eng;l=San+Francisco;st=California;c=US", false, true}, &certAttributes{ + commonName: "leaf", + organization: []string{"Smallstep"}, + organizationalUnit: []string{"Eng"}, + locality: []string{"San Francisco"}, + province: []string{"California"}, + country: []string{"US"}, + }, assert.NoError}, + {"ok repeated ou", args{"mackms:ou=a;ou=b", false, true}, &certAttributes{organizationalUnit: []string{"a", "b"}}, assert.NoError}, + {"ok label serial and cn", args{"mackms:label=the-label;serial=0x01;cn=leaf", false, true}, &certAttributes{label: "the-label", serialNumber: big.NewInt(1), commonName: "leaf"}, assert.NoError}, + {"ok empty subject values", args{"mackms:label=the-label;o=;ou=", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok empty cn with label", args{"mackms:cn=;label=the-label", false, true}, &certAttributes{label: "the-label"}, assert.NoError}, + {"ok keychain", args{"mackms:label=the-label;keychain=dataProtection", false, true}, &certAttributes{label: "the-label", keychain: "dataProtection", useDataProtectionKeychain: true}, assert.NoError}, + {"ok default data protection", args{"mackms:label=the-label", true, true}, &certAttributes{label: "the-label", useDataProtectionKeychain: true}, assert.NoError}, + {"ok no require value", args{"mackms:", false, false}, &certAttributes{}, assert.NoError}, + {"ok bare cn is a label", args{"mackms:cn", false, true}, &certAttributes{label: "cn"}, assert.NoError}, + {"ok bare cn equal is a label", args{"mackms:cn=", false, true}, &certAttributes{label: "cn"}, assert.NoError}, + {"fail require value", args{"mackms:", false, true}, nil, assert.Error}, + {"fail keychain only", args{"mackms:keychain=login", false, true}, nil, assert.Error}, + {"fail bad serial", args{"mackms:serial=010a020b030z", false, true}, nil, assert.Error}, + {"fail parse", args{"mackms:%label=the-label", false, true}, nil, assert.Error}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseCertURI(tt.args.rawuri, tt.args.useDataProtectionKeychain, tt.args.requireValue) + tt.assertion(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func Test_certAttributes_hasSubjectQuery(t *testing.T) { + tests := []struct { + name string + attrs *certAttributes + want bool + }{ + {"empty", &certAttributes{}, false}, + {"label and serial only", &certAttributes{label: "the-label", serialNumber: big.NewInt(1), keychain: "login"}, false}, + {"cn", &certAttributes{commonName: "leaf"}, true}, + {"o", &certAttributes{organization: []string{"Smallstep"}}, true}, + {"ou", &certAttributes{organizationalUnit: []string{"Eng"}}, true}, + {"l", &certAttributes{locality: []string{"San Francisco"}}, true}, + {"st", &certAttributes{province: []string{"California"}}, true}, + {"c", &certAttributes{country: []string{"US"}}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.attrs.hasSubjectQuery()) + }) + } +} + +func Test_certAttributes_matchesSubject(t *testing.T) { + cert := &x509.Certificate{Subject: pkix.Name{ + CommonName: "leaf", + Organization: []string{"Smallstep"}, + OrganizationalUnit: []string{"Eng", "Platform"}, + Locality: []string{"San Francisco"}, + Province: []string{"California"}, + Country: []string{"US"}, + }} + + tests := []struct { + name string + attrs *certAttributes + cert *x509.Certificate + want bool + }{ + {"ok empty query", &certAttributes{label: "the-label"}, cert, true}, + {"ok cn", &certAttributes{commonName: "leaf"}, cert, true}, + {"ok all components", &certAttributes{ + commonName: "leaf", + organization: []string{"Smallstep"}, + organizationalUnit: []string{"Eng", "Platform"}, + locality: []string{"San Francisco"}, + province: []string{"California"}, + country: []string{"US"}, + }, cert, true}, + {"ok ou subset", &certAttributes{organizationalUnit: []string{"Platform"}}, cert, true}, + {"fail cn", &certAttributes{commonName: "other"}, cert, false}, + {"fail cn case sensitive", &certAttributes{commonName: "Leaf"}, cert, false}, + {"fail ou case sensitive", &certAttributes{organizationalUnit: []string{"eng"}}, cert, false}, + {"fail and semantics", &certAttributes{commonName: "leaf", organizationalUnit: []string{"Marketing"}}, cert, false}, + {"fail ou superset", &certAttributes{organizationalUnit: []string{"Eng", "Marketing"}}, cert, false}, + {"fail o", &certAttributes{organization: []string{"Other"}}, cert, false}, + {"fail l", &certAttributes{locality: []string{"New York"}}, cert, false}, + {"fail st", &certAttributes{province: []string{"New York"}}, cert, false}, + {"fail c", &certAttributes{country: []string{"ES"}}, cert, false}, + {"fail empty subject", &certAttributes{commonName: "leaf"}, &x509.Certificate{}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.attrs.matchesSubject(tt.cert)) + }) + } +} + func Test_parseECDSAPublicKey(t *testing.T) { p256, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) @@ -954,6 +1073,127 @@ func TestMacKMS_LoadCertificate_sort(t *testing.T) { assert.Equal(t, cert2, cert) } +func TestMacKMS_LoadCertificate_bySubject(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + label := "test-" + suffix + + now := time.Now().Truncate(time.Second) + cert1, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{ + CommonName: testName + "-1-" + suffix, + Organization: []string{"TestOrg"}, + OrganizationalUnit: []string{"Engineering"}, + Locality: []string{"San Francisco"}, + Province: []string{"California"}, + Country: []string{"US"}, + }, + PublicKey: key.Public(), + NotBefore: now, + }) + require.NoError(t, err) + + cert2, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{ + CommonName: testName + "-2-" + suffix, + Organization: []string{"TestOrg"}, + OrganizationalUnit: []string{"Engineering", "Platform"}, + Locality: []string{"New York"}, + Province: []string{"New York"}, + Country: []string{"US"}, + }, + PublicKey: key.Public(), + NotBefore: now.Add(-time.Second), + }) + require.NoError(t, err) + + cert3, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{ + CommonName: testName + "-3-" + suffix, + Organization: []string{"OtherOrg"}, + OrganizationalUnit: []string{"Sales"}, + Country: []string{"ES"}, + }, + PublicKey: key.Public(), + NotBefore: now.Add(-2 * time.Second), + }) + require.NoError(t, err) + + kms := &MacKMS{} + for _, crt := range []*x509.Certificate{cert1, cert2, cert3} { + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:label=" + label, Certificate: crt, + })) + t.Cleanup(func() { deleteCertificate(t, label, crt) }) + } + + isNotFound := func(t assert.TestingT, err error, i ...any) bool { + return assert.ErrorIs(t, err, apiv1.NotFoundError{}, i...) + } + + type args struct { + req *apiv1.LoadCertificateRequest + } + tests := []struct { + name string + args args + want *x509.Certificate + assertion assert.ErrorAssertionFunc + }{ + {"ok cn", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";cn=" + cert2.Subject.CommonName, + }}, cert2, assert.NoError}, + {"ok ou", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Platform", + }}, cert2, assert.NoError}, + {"ok ou repeated", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Engineering;ou=Platform", + }}, cert2, assert.NoError}, + {"ok ou multiple matches", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Engineering", + }}, cert1, assert.NoError}, + {"ok o", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";o=OtherOrg", + }}, cert3, assert.NoError}, + {"ok l with spaces", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";l=San+Francisco", + }}, cert1, assert.NoError}, + {"ok st and c", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";st=New+York;c=US", + }}, cert2, assert.NoError}, + {"ok cn only", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:cn=" + cert1.Subject.CommonName, + }}, cert1, assert.NoError}, + {"ok serial and o", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert3.SerialNumber.Bytes()) + ";o=OtherOrg", + }}, cert3, assert.NoError}, + {"fail serial and o", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert3.SerialNumber.Bytes()) + ";o=TestOrg", + }}, nil, isNotFound}, + {"fail ou", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Marketing", + }}, nil, isNotFound}, + {"fail ou case sensitive", args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label + ";ou=engineering", + }}, nil, isNotFound}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k := &MacKMS{} + got, err := k.LoadCertificate(tt.args.req) + tt.assertion(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + func TestMacKMS_StoreCertificate(t *testing.T) { testName := t.Name() @@ -1151,7 +1391,10 @@ func TestMacKMS_LoadCertificateChain(t *testing.T) { require.NoError(t, err) cert, err := ca.Sign(&x509.Certificate{ - Subject: pkix.Name{CommonName: testName + "@example.com"}, + Subject: pkix.Name{ + CommonName: testName + "@example.com", + OrganizationalUnit: []string{"Engineering"}, + }, EmailAddresses: []string{testName + "@example.com"}, PublicKey: key.Public(), }) @@ -1192,12 +1435,21 @@ func TestMacKMS_LoadCertificateChain(t *testing.T) { {"ok label and serial", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ Name: "mackms:labeld=" + cert.Subject.CommonName + ";serial=" + hex.EncodeToString(cert.SerialNumber.Bytes()), }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, + {"ok cn", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:cn=" + cert.Subject.CommonName, + }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, + {"ok label and ou", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:label=" + cert.Subject.CommonName + ";ou=Engineering", + }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, {"ok self-signed", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ Name: "mackms:label=" + ca.Root.Subject.CommonName, }}, []*x509.Certificate{ca.Root}, assert.NoError}, {"fail name", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{}}, nil, assert.Error}, {"fail uri", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{Name: "mackms:"}}, nil, assert.Error}, {"fail missing", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{Name: "mackms:label=missing-" + testName}}, nil, assert.Error}, + {"fail ou", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:label=" + cert.Subject.CommonName + ";ou=Marketing", + }}, nil, assert.Error}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 50e137d1088b84adea5a4e8f6c31753cc211969d Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Thu, 16 Jul 2026 16:32:11 -0500 Subject: [PATCH 2/2] Support subject DN components in DeleteCertificate When subject components ("cn", "o", "ou", "l", "st", "c") are present in the URI, find the matching certificate first, using the same selection rules as LoadCertificate, and delete it by its serial number and, when available, its subject key identifier. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P3Nxd2zBbULcRAG6HrzQgR --- kms/mackms/mackms.go | 38 +++++++++++-- kms/mackms/mackms_test.go | 111 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index c3e438cb..af63e9ae 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -698,7 +698,18 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error { } // DeleteCertificate deletes the certificate referenced by the URI in the -// request name. +// request name. It deletes at most one certificate. +// +// Valid names (URIs) are: +// - mackms:label=test@example.com +// - mackms:serial=2c273934eda8454d2595a94497e2395a +// - mackms:label=test@example.com;serial=2c273934eda8454d2595a94497e2395a +// - mackms:cn=My+Cert;ou=Engineering +// +// When subject components ("cn", "o", "ou", "l", "st", "c") are present, the +// certificate is first located using the same selection rules as +// [MacKMS.LoadCertificate], and then deleted by its serial number and, when +// available, its subject key identifier. // // # Experimental // @@ -718,6 +729,27 @@ func (k *MacKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error { security.KSecClass: security.KSecClassCertificate, security.KSecMatchLimit: security.KSecMatchLimitOne, } + + serialNumber := u.serialNumber + if u.hasSubjectQuery() { + // The keychain cannot match individual subject components. Find the + // matching certificate first, using the same selection rules as + // LoadCertificate, and delete it by its serial number and, when + // available, its subject key identifier. + cert, err := loadCertificate(u, nil) + if err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", apiv1Error(err)) + } + serialNumber = cert.SerialNumber + if len(cert.SubjectKeyId) > 0 { + cfSubjectKeyID, err := cf.NewData(cert.SubjectKeyId) + if err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", err) + } + defer cfSubjectKeyID.Release() + query[security.KSecAttrSubjectKeyID] = cfSubjectKeyID + } + } if u.label != "" { cfLabel, err := cf.NewString(u.label) if err != nil { @@ -726,8 +758,8 @@ func (k *MacKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error { defer cfLabel.Release() query[security.KSecAttrLabel] = cfLabel } - if u.serialNumber != nil { - cfSerial, err := cf.NewData(encodeSerialNumber(u.serialNumber)) + if serialNumber != nil { + cfSerial, err := cf.NewData(encodeSerialNumber(serialNumber)) if err != nil { return fmt.Errorf("mackms DeleteCertificate failed: %w", err) } diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index ba756b92..b6ea3c3d 100644 --- a/kms/mackms/mackms_test.go +++ b/kms/mackms/mackms_test.go @@ -1698,6 +1698,117 @@ func TestMacKMS_DeleteCertificate(t *testing.T) { } } +func TestMacKMS_DeleteCertificate_bySubject(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + label := "test-del-" + suffix + + certA, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{ + CommonName: testName + "-a-" + suffix, + OrganizationalUnit: []string{"Platform"}, + }, + PublicKey: key.Public(), + }) + require.NoError(t, err) + require.NotEmpty(t, certA.SubjectKeyId) + + certB, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{ + CommonName: testName + "-b-" + suffix, + OrganizationalUnit: []string{"Engineering"}, + }, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + // Create a self-signed certificate without a subject key identifier to + // exercise the delete by serial number only. Go only generates the subject + // key identifier automatically for CAs. + now := time.Now() + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 64)) + require.NoError(t, err) + template := &x509.Certificate{ + SerialNumber: serial.Add(serial, big.NewInt(1)), + Subject: pkix.Name{ + CommonName: testName + "-c-" + suffix, + OrganizationalUnit: []string{"NoSKID"}, + }, + NotBefore: now.Add(-time.Hour), + NotAfter: now.Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, key.Public(), key) + require.NoError(t, err) + certC, err := x509.ParseCertificate(der) + require.NoError(t, err) + require.Empty(t, certC.SubjectKeyId) + + serialURI := func(cert *x509.Certificate) string { + return "mackms:serial=0x" + hex.EncodeToString(cert.SerialNumber.Bytes()) + } + existsCheck := func(cert *x509.Certificate) { + kms := &MacKMS{} + _, err := kms.LoadCertificate(&apiv1.LoadCertificateRequest{Name: serialURI(cert)}) + assert.NoError(t, err) + } + notExistsCheck := func(cert *x509.Certificate) { + kms := &MacKMS{} + _, err := kms.LoadCertificate(&apiv1.LoadCertificateRequest{Name: serialURI(cert)}) + assert.ErrorIs(t, err, apiv1.NotFoundError{}) + } + + kms := &MacKMS{} + for _, crt := range []*x509.Certificate{certA, certB, certC} { + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:label=" + label, Certificate: crt, + })) + } + t.Cleanup(func() { + // The certificates are deleted by the test itself; this only removes + // the leftovers if the test fails early. + kms := &MacKMS{} + for _, crt := range []*x509.Certificate{certA, certB, certC} { + _ = kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{Name: serialURI(crt)}) + } + }) + + // Delete by subject removes only the matching certificate. + require.NoError(t, kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Platform", + })) + notExistsCheck(certA) + existsCheck(certB) + existsCheck(certC) + + // Delete a certificate without a subject key identifier. + require.NoError(t, kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=" + label + ";ou=NoSKID", + })) + notExistsCheck(certC) + existsCheck(certB) + + // Fail to delete if no certificate matches the subject. + err = kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=" + label + ";ou=Marketing", + }) + assert.ErrorIs(t, err, apiv1.NotFoundError{}) + existsCheck(certB) + + // Delete using only subject components. + require.NoError(t, kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: "mackms:cn=" + certB.Subject.CommonName, + })) + notExistsCheck(certB) +} + func Test_apiv1Error(t *testing.T) { type args struct { err error