Add support for subject DN components in MacKMS certificate URIs - #1073
Conversation
|
|
| 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 { |
There was a problem hiding this comment.
If the whole keychain is searched, a single malformed certificate (as determined by the Go parser) can result in loadCertificate failing, as it returns early in the loop when failing to parse the cert. The error itself will likely also confuse the reader, as it may not even be clear which cert fails to parse.
Either change the early return to continue, skipping the malformed (certs), or provide an additional option in loadCertificate to explicitly ask to continue instead of the early return.
| useDataProtectionKeychain: isDataProtectionKeychain(keychain, useDataProtectionKeychain), | ||
| keychain: keychain, | ||
| }, nil | ||
| commonName: u.Get("cn"), |
There was a problem hiding this comment.
To make it even more robust, cn can be processed similarly to the other components, and then pick the first one. Even better would be to return an error if multiple cns are provided.
| // 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 |
There was a problem hiding this comment.
If I understand this correctly, a subject search could match multiple certificates, and then only a single one would get deleted? Although the comment mentions it, it could be somewhat surprising.
Given the function name, I think the behavior is OK, and I think it can still go in. I don't have a great alternative atm.
| 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() |
There was a problem hiding this comment.
Probably minor, as I believe the SKI will be filled, but if it's not, then the actual delete will be by serial only. While generally random, the serial is only unique in combination with the issuer.
I believe this behavior existed before, so it's not something new that breaks if this gets merged.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P3Nxd2zBbULcRAG6HrzQgR
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P3Nxd2zBbULcRAG6HrzQgR
When seaching based on subject components, ignore errors caused by parsing certificates from the Apple keychain.
10f57aa to
ddd2124
Compare
hslatman
changed the title
|
While testing this I found that certificates from the There's currently a switch in which we decide whether to use the data protection keychain vs. "the regular one", which is also chosen when it says |
maraino
left a comment
maraino
left a comment
There was a problem hiding this comment.
The changes look ok, but should we be deleting the oldest certificate instead of the new one.
48f3416 to
d6585c7
Compare
5 participants
What
Adds subject distinguished name components —
cn,o,ou,l,st,c— to the mackms certificate URI vocabulary, so certificates can be selected by subject inLoadCertificate,LoadCertificateChain, andDeleteCertificate:Constraint: subject matching happens in Go — we have to enumerate certificates
The Apple keychain cannot do this filtering for us:
kSecMatchSubjectContainsis a substring match over the whole subject and only works on legacy file-based keychains, not the data protection keychain.kSecAttrSubject/kSecAttrIssuerhold the entire name as an Apple-normalized (partially uppercased) DER blob. They are read-only, system-derived, exact-match columns, and there is no public API to produce the normalized bytes from user-supplied strings — theSecCertificateCopyNormalized*SequenceAPIs all require aSecCertificateRefin hand. The existing comment inLoadCertificateChainalready rejectscert.RawIssuer/RawSubjectmatching for this reason.So when a URI contains subject components, mackms fetches the candidates (
kSecMatchLimitAll), parses every candidate withx509.ParseCertificate, and matches the components againstcert.Subjectin Go.labeland/orserialin the same URI still narrow the keychain query natively first; a pure-DN query likemackms:ou=Engineeringenumerates and parses every certificate in the keychain. That's fine at keychain scale (tens to a few hundred items), but combining withlabelis recommended, and the docs say so. This mirrors what capi does on Windows, wherecnis matched in Go while iterating a native issuer-search cursor.Semantics
cnis single-valued;o/ou/l/st/ccan be repeated, and every given value must be present in the certificate subject (subset match on multi-valued RDNs).LoadCertificateChain: components select the leaf only; intermediates are still located via the authority key identifier.DeleteCertificate: with subject components present, the certificate is located first (same rules asLoadCertificate) and then deleted by serial number plus, when available, subject key identifier. Still deletes at most one certificate.Behavior change
Subject component attributes in certificate URIs were previously ignored; now they filter. For example,
mackms:label=x;cn=yused to load by label alone and can now return not found, andmackms:cn=yused to fail with "label or serial is required" and now performs a lookup.Testing
Test_parseCertURI(first unit coverage for this function),Test_certAttributes_hasSubjectQuery, andTest_certAttributes_matchesSubject— pure, no keychain access.TestMacKMS_LoadCertificate_bySubjectandTestMacKMS_DeleteCertificate_bySubject(including a no-SKID self-signed cert for the serial-only delete path), plus subject cases inTestMacKMS_LoadCertificateChain.gofmt,go build ./...,go vet, and thekms/uri/kms/platform/kms/apiv1tests. The package still needsgo test ./kms/mackms/...on a Mac before this merges — keeping as draft until that run happens.Possible follow-ups (out of scope)
issuer-cn, …) for disambiguating same-subject certs from different CAs.kSecAttrSubject == SecCertificateCopyNormalizedIssuerSequence(child)(macOS 10.12.4+) as an alternative to the SKID walk — with a cert in hand the framework computes the normalized bytes, so no reimplementation is needed;kSecAttrIssueris an indexed primary-key column, making this the one place issuer-based keychain-side narrowing is legitimate.🤖 Generated with Claude Code
https://claude.ai/code/session_01P3Nxd2zBbULcRAG6HrzQgR