fix(x509): reject RFC 5280 MUST-NOT builder constructions - #88
Conversation
Three builder gaps emitted RFC-invalid certificates. pathLenConstraint
now requires the keyUsage keyCertSign bit when a keyUsage extension is
present (§4.2.1.9); the encoder emitted a certificate this library's own
verifier rejects. An empty subject DN requires a subjectAltName that is
present and critical (§4.2.1.6), counting a customExtensions SAN only
when critical and an empty subjectAltNames array as absent; only
criticality was enforced, so subject: {} signed a certificate with no
identity at all. A relativeName distribution point rejects more than one
cRLIssuer distinguished name (§4.2.1.13). All three throw coded
ExtensionEncoderErrorCode errors on both the certificate and CSR paths.
Verifier fixtures that intentionally omit keyCertSign drop their
pathLength so the malformed-chain scenarios stay constructible.
Citation riders from the audit: the ParsedBitFlags docs no longer claim
non-canonical padding is reported for callers to judge (extension flag
decoding rejects it), and decodeBoolean's doc cites the X.690 11.1 DER
restriction instead of the BER any-non-zero rule.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
micro509 | 517fc1c | Commit Preview URL Branch Preview URL |
Jul 25 2026, 12:49 AM |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCertificate and CSR builders now enforce RFC 5280 constraints for path length, key usage, empty subjects, GeneralName values, criticality, OIDs, AIA, and CRL distribution points. Extension registries provide profile assertions, parsers reject empty IA5String GeneralNames, and tests can inject raw extensions to exercise malformed inputs. Tooling, documentation, changelog entries, and fixtures were also updated. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CertificateOrCSRBuilder
participant ExtensionRegistry
participant ExtensionProfileValidator
participant Parser
CertificateOrCSRBuilder->>ExtensionRegistry: resolve configured or custom extension
ExtensionRegistry->>ExtensionProfileValidator: validate payload and criticality
ExtensionProfileValidator-->>CertificateOrCSRBuilder: accept or return coded error
Parser->>Parser: decode GeneralName and extension DER
Parser-->>CertificateOrCSRBuilder: reject empty IA5String GeneralName
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
Comment |
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
pathLength/keyCertSign coupling, the empty-subject SAN rule, and relativeName cRLIssuer validation each admitted a bypass through public input. Resolve an effective basicConstraints/keyUsage/SAN view across typed fields and custom-known extensions before the cross-field checks, so a known extension smuggled through customExtensions no longer evades them (canonical OID match). Reject absent or empty keyUsage under pathLength, empty typed and malformed custom SAN under an empty subject, empty GeneralName string values on encode, and a non-directoryName or smuggled/multiple cRLIssuer under relativeName.
This comment was marked as resolved.
This comment was marked as resolved.
RFC 5280 §4.2.1.13 lines 88-91 make cRLIssuer DN-only a MUST for every case, not just nameRelativeToCRLIssuer. The builder now rejects any non-directoryName cRLIssuer entry unconditionally; relativeName keeps the extra at-most-one constraint. The parser and revocation scanner stay tolerant of non-DN cRLIssuer for real-world certificates, fed through a raw-DER test helper instead of the conformant builder. Cover every new branch: custom-basicConstraints/keyUsage decode paths (including malformed-value fallbacks), the canonical leading-zero OID match, and the non-string SAN identity forms. Replace the SAN identity switch with a ternary, dropping an unreachable exhaustiveness guard.
This comment was marked as resolved.
This comment was marked as resolved.
# Conflicts: # CHANGELOG.md
…loads Three escapes survived the first pass at the builder cross-field guards, all reachable through the public API with green CI. `pushExtension` keyed its duplicate set by raw OID text and `appendCustomExtensions` looked the registry up the same way. Since `2.5.029.17` and `2.5.29.17` encode to identical DER, a typed `subjectAltNames` plus a custom `2.5.029.17` emitted two wire-identical SAN OIDs that this library's own parser then rejected, and a custom `2.5.029.18` evaded the certificate-only restriction on `issuerAltName`. Both keys are now the canonical form; the diagnostic still quotes the OID as submitted, since that is the string the caller can find in their input. The effective-value resolvers swallowed DER decode failures and returned `undefined`, so a `customExtensions` entry carrying a known OID with a malformed payload passed every cross-field guard and reached the wire unchecked. Known payloads are now decoded up front, ahead of those guards, which also lets the three resolvers drop their catch clauses. `parseGeneralName` accepted a zero-length `dNSName`, `rfc822Name`, or `uniformResourceIdentifier`, which RFC 5280 §4.2.1.6 forbids. An external certificate could carry an empty subjectAltName value and parse, leaving chain verification to accept a certificate with no usable identity. Builder strictness means parser-strictness fixtures can no longer be built through the builder, so `test/helpers.ts` gains raw-extension variants of `createCertificate`, `createSelfSignedCertificate`, and `createCertificateSigningRequest` that splice extensions into the signed structure.
This comment was marked as resolved.
This comment was marked as resolved.
Decoding a known custom extension proves its structure, not the RFC profile the typed encoders enforce. A `cRLDistributionPoints` value supplied through `customExtensions` therefore still emitted the exact RFC 5280 §4.2.1.13 constructions this PR claims the builders reject: a non-directoryName cRLIssuer, and more than one cRLIssuer DN alongside nameRelativeToCRLIssuer. Both builder paths now run the same assertion the typed field runs, so the escape is closed on certificates and CSRs alike. `validateOid` checked decimal syntax only, so an OID such as `3.1` or `1.40` passed it and then failed inside `objectIdentifier` with an uncoded `Error`. It now rejects them with `invalid_oid`, matching the builder's coded-error contract. The parser stays tolerant of both, so the fixtures that feed it non-conformant values move to the raw-extension helpers. Parametrized cases become `it.each` so a failure names the case rather than the whole test.
Decoding proves a payload's structure, not that it obeys the RFC 5280 profile, so a `customExtensions` value under a known OID could still carry duplicate policy OIDs, an over-long DisplayText, a non-URI OCSP location, an empty nameConstraints, or a keyUsage with no bit set. `ExtensionDefinition` now requires `assertProfile`, which delegates to the encoder owning the rule. Builders run it over the decoded payload. Parsing never calls it, so the parser stays tolerant. The payload decodes once, inside the coded-error boundary. A profile violation keeps its own code. A decode failure maps to `malformed_known_extension_value`. `encodeExtension` and the `encodeCertificatePolicies` duplicate scan validate OIDs before encoding, so an unencodable arc yields `invalid_oid` instead of a bare `Error`. `encodeBasicConstraints` gains `path_length_requires_ca`. CRLDP rules move to `assertCrlDistributionPointsProfile`, shared by the encoder and the hook. `encodeDistributionPointName` now takes the resolved name choice, dropping its unreachable branch.
`getAuthorityInfoAccessMethodOid` and `getExtendedKeyUsageOid` returned the submitted OID, and `encodePolicyMappings` compared its input by string, so an alias that encodes to a forbidden OID dodged the rule. `1.3.6.1.5.5.7.048.1` accepted a dNSName OCSP location and `2.5.29.032.0` passed the anyPolicy check. All three now resolve through `validateOid`, which returns the canonical spelling. `assertProfile` had only the decoded payload, so a `customExtensions` entry could carry a criticality RFC 5280 forbids. It now also receives the flag, and nameConstraints (§4.2.1.10), policyConstraints (§4.2.1.11), and inhibitAnyPolicy (§4.2.1.14) require critical, while authorityKeyIdentifier (§4.2.1.1), subjectKeyIdentifier (§4.2.1.2), and authorityInfoAccess (§4.2.2.1) require non-critical. §4.2.1.9 conditions basicConstraints criticality on the certificate being a CA whose key signs certificates, which no single extension carries, so that check sits beside `assertPathLengthKeyUsage` where the effective keyUsage is already resolved. The name-constraints tolerance fixture emitted a non-critical extension through the builder and moves to the raw-extension helper.
RFC 5280 §4.2.1.9 covers any CA certificate whose key may validate signatures on certificates, and §4.2.1.3 restricts a key only through a keyUsage extension that reaches the wire. An absent keyUsage therefore leaves the key free to validate certificate signatures, as does an empty one, which the builder omits. Both were accepting a non-critical `cA=true` custom basicConstraints. Only an emitted keyUsage lacking keyCertSign takes the certificate out of the clause's scope.
`expectRejectedErrorCode` existed byte-identically in three suites; it
moves to `test/helpers.ts`.
The raw-extension helpers re-signed with `getSignatureAlgorithm(key)` and
no profile, so a fixture built with `signature: { kind: 'rsa-pss' }` came
back signed PKCS#1 v1.5 without complaint. `appendCertificateExtensions`
takes the profile, and both certificate wrappers plus the CSR wrapper
forward `input.signature`.
The duplicate-extension builder assertions in `parse.test.ts` matched the
message rather than `duplicate_extension_oid`, and neither awaited its
`.rejects` chain.
What
Resolves the three certificate/extension builder findings from the spec audit, all RFC 5280 MUST-NOT constructions the builders emitted.
pathLenConstraint without keyCertSign (§4.2.1.9)
"CAs MUST NOT include the pathLenConstraint field unless the cA boolean is asserted and the key usage extension asserts the keyCertSign bit." Only the cA conjunct was checked, so
{ basicConstraints: { ca: true, pathLength: 0 }, keyUsage: ['digitalSignature'] }emitted a certificate this library's own verifier rejects.assertPathLengthKeyUsagereturns early only whenpathLengthis absent; an absent, empty, orkeyCertSign-less keyUsage all throwpath_length_requires_key_cert_sign, on both the certificate path (buildCertificateExtensions) and the CSR path (appendConstraintExtensions).Empty subject requires a present, critical SAN (§4.2.1.6)
The clause carries two conjoined MUSTs; only criticality was implemented, so
subject: {}signed a certificate with no identity at all.buildCertificateExtensionsnow throwsempty_subject_requires_subject_alt_namewhensubjectIsEmptyand no SAN is configured. AcustomExtensionsSAN counts only whencritical: true, and a present-but-emptysubjectAltNamesarray counts as absent.createCertificatedelegates, so one guard covers both entry points;buildRequestedExtensionsis untouched.relativeName with multiple cRLIssuer DNs (§4.2.1.13)
"The DistributionPointName MUST NOT use the nameRelativeToCRLIssuer alternative when cRLIssuer contains more than one distinguished name." Lines 88-91 of the same clause make the DN-only requirement unconditional ("If present, the cRLIssuer MUST only contain the distinguished name"), so
encodeDistributionPointrejects any non-directoryNamecRLIssuerentry withdistribution_point_crl_issuer_not_directory_name, and additionally rejectsnameRelativeToCRLIssuerpast one DN withdistribution_point_relative_name_multiple_crl_issuers.Citation riders (audit Medium)
The
ParsedBitFlagsdocs claimed non-canonical padding is recorded "so verification layers can decide", but no caller can observe it becauserequireCanonicalBitFlagsthrows first; the docs now state rejection.decodeBoolean's doc cited the BER any-non-zero rule while the function enforces X.690 11.1 DER (0xff/0x00only). The audit's remaining two Medium citation errors (X.690 §11.2.2 six sites, name-constraints step labels) were verified already fixed on master.Test fixture correction
Three verifier tests built their deliberately-invalid intermediate (
ca: true, keyUsage without keyCertSign) withpathLength: 0, which the builder now refuses. DroppingpathLengthkeeps those chains constructible and verifier-rejected (key_cert_sign_required/ca_requiredunchanged).Tests
Encoder level:
path_length_requires_key_cert_sign(accepted with keyCertSign, rejected when keyUsage is absent or lacks it),empty_subject_requires_subject_alt_name(empty array rejected, populated accepted),distribution_point_crl_issuer_not_directory_name(a URIcRLIssuerrejected under bothfullNameandrelativeName),distribution_point_relative_name_multiple_crl_issuers(two DNs rejected, one accepted). Public API:createSelfSignedCertificateandcreateCertificateSigningRequestreject the pathLen case;createCertificaterejects an empty subject without a SAN and accepts one carried as a critical custom extension.Gate: typecheck, biome, docs:lint, 1482 tests (PKITS in-suite, 249 standalone), and 76 OpenSSL differential cases (11 deterministic in
test/differential.test.tsplus 65 generated intest/differential-fuzz.test.ts, both run bytest:differential). Green on stable Bun 1.3.14 and on thebun-35433build.Canonical extension OIDs and custom payload validation
pushExtensionkeyed its duplicate set by raw OID text andappendCustomExtensionslooked the registry up the same way.2.5.029.17and2.5.29.17encode to identical DER, so typedsubjectAltNamesplus a custom2.5.029.17emitted two wire-identical SAN OIDs that the parser then rejected, and a custom2.5.029.18evaded the certificate-only restriction onissuerAltName. Both keys are now the canonical form viacanonicalizeOid; the diagnostic still quotes the OID as submitted.The effective-value resolvers swallowed DER decode failures and returned
undefined, so acustomExtensionsentry carrying a known OID with a malformed payload passed every cross-field guard and reached the wire unchecked.assertCustomExtensionsValidnow decodes known payloads ahead of those guards and throwsmalformed_known_extension_value, which also lets the three resolvers drop their catch clauses.Empty IA5 GeneralNames reject on parse
parseGeneralNameaccepted a zero-lengthdNSName,rfc822Name, oruniformResourceIdentifier, which RFC 5280 §4.2.1.6 forbids. An external certificate could carry an empty subjectAltName value and parse, leaving chain verification to accept a certificate with no usable identity when no identity match was requested. Certificate and CRL parsing share the decoder, so this covers subjectAltName, issuerAltName, authorityInfoAccess locations, CRL distribution points,cRLIssuer, the issuing distribution point, andcertificateIssuer. Name constraints keep their own decoder, where an empty base is meaningful.Builder strictness means parser-strictness fixtures can no longer be built through the builder, so
test/helpers.tsgains raw-extension variants ofcreateCertificate,createSelfSignedCertificate, andcreateCertificateSigningRequestthat splice extensions into the signed structure and re-sign.Known-extension profiles apply to custom payloads
Decoding a payload proves its structure, not that it obeys the RFC 5280 profile, because the parser is deliberately tolerant.
ExtensionDefinitionnow requires anassertProfilehook that delegates to the encoder owning each rule, and the builders run it over the decoded value of anycustomExtensionsentry carrying a known OID. Parsing never calls it, so the parser and the CRL scanner stay tolerant.That closes every rule the decoder does not already carry: duplicate policy OIDs and an over-long
DisplayTextincertificatePolicies, a non-URI OCSP location inauthorityInfoAccess, anameConstraintswith neither subtree, akeyUsagewith no bit set, and the §4.2.1.13cRLIssuerrules. Every other registered extension (basicConstraints,extendedKeyUsage,subjectAltName,issuerAltName,policyMappings,policyConstraints,inhibitAnyPolicy, SKI, AKI) was audited: their decoders already reject what their encoders reject, and each still declares the hook so the two paths cannot drift apart later.The payload decodes once, inside the coded-error boundary. A profile violation keeps its own code. A decode failure maps to
malformed_known_extension_value.The §4.2.1.13 rules now live in
assertCrlDistributionPointsProfile, shared byencodeCrlDistributionPointsand the hook.encodeDistributionPointNameconsumes the resolved name choice, so it has no unreachable branch.Coded OID errors on every encoding path
encodeExtensionis public and calledobjectIdentifierdirectly, soencodeExtension('3.1', ...)threw a bareError. TheencodeCertificatePoliciesduplicate scan encoded each policy OID before validating it, with the same result. Both validate first and returninvalid_oid, alongside the builder paths, for an OID that parses as decimals but breaks the X.660 arc bounds.encodeBasicConstraintsgainspath_length_requires_cain place of its bare throw.Round-two regression matrices
cRLDistributionPoints: a URIcRLIssuerand arelativeNamebeside two wire-tagged ([4]) DNs are rejected as profile violations; aNULLpayload, an emptycRLIssuer, and an empty points SEQUENCE are rejected as malformed; afullNameURI, arelativeNamewith one DN, arelativeNamewith nocRLIssuer, and afullNamebeside a[4]DN are accepted. Each runs on the certificate and CSR paths under both2.5.29.31and2.5.029.31, and every profile-invalid payload is asserted to still parse.Profile hooks: one rejecting case per rule the decoder does not carry, one accepting case per registered extension, and one malformed case per rule the decoder does carry, all on both builder paths. Every new guard was reverted in turn to confirm its test fails without it.
Canonical OID comparison on the typed path
getAuthorityInfoAccessMethodOidandgetExtendedKeyUsageOidvalidated a custom OID and then returned the string as submitted, andencodePolicyMappingscompared its input by string. Both rules keyed on OID equality were therefore dodgeable by an alias that encodes to the same DER:{ method: { type: 'oid', value: '1.3.6.1.5.5.7.048.1' }, location: { type: 'dns', ... } }emitted a non-URI OCSP location, and2.5.29.032.0passed thepolicyMappingsanyPolicy check. All three now resolve throughvalidateOid, which returns the canonical spelling. The custom-payload route was already safe, since an OID decoded from DER is canonical by construction.Criticality is part of the profile
assertProfilesaw only the decoded payload, so acustomExtensionsentry could carry a criticality RFC 5280 forbids while the typed field could not. The hook now takes the flag as well. nameConstraints (§4.2.1.10), policyConstraints (§4.2.1.11), and inhibitAnyPolicy (§4.2.1.14) must be critical; authorityKeyIdentifier (§4.2.1.1), subjectKeyIdentifier (§4.2.1.2), and authorityInfoAccess (§4.2.2.1) must not.§4.2.1.9 conditions basicConstraints criticality on the certificate being a CA whose key validates signatures on certificates. Neither conjunct is in the extension itself, so that check sits beside
assertPathLengthKeyUsage, where the effective basicConstraints and keyUsage are already resolved across the typed field andcustomExtensions. §4.2.1.3 restricts a key only through a keyUsage extension that reaches the wire, so an absent keyUsage, and an empty one the builder omits, both leave the key free to validate certificate signatures and require a critical basicConstraints; only an emitted keyUsage lacking keyCertSign takes the certificate out of the clause's scope.A single test runs every registered definition's hook at its own
defaultCriticalwith a conformant payload, so a default that contradicts its profile fails, and a new extension added without a fixture fails too.test/verify.test.tsbuilt its non-critical name-constraints tolerance fixture through the builder; it moves tocreateSelfSignedCertificateWithRawExtensions.RFC 1421 PEM header parsing is tracked separately in #92; it is out of scope here.