Rework X.509 cert/csr/crl extension representation - #449
Open
cpu wants to merge 28 commits into
Open
Conversation
Add an `Extension` trait (OID, criticality, DER value) and a single `write_extension()` serializer so each extension's encoding lives beside its presence logic as types are migrated. Extensions whose OID and criticality are fixed by the profile defining them implement the `StaticExtension` trait instead, receiving `Extension` through a blanket impl. Move authority key identifier writing into the module as the first static extension, replacing `write_x509_authority_key_identifier()` in the certificate and CRL paths.
Port the SAN extension into an `ext::SubjectAlternativeName` type whose `from_params` constructor owns the presence decision, replacing `write_subject_alt_names()` in the certificate and CSR paths. The RFC 5280 §4.1.2.6 criticality rule (critical if the subject DN is empty) is now unit tested.
Port the KeyUsage extension into an `ext::KeyUsage` static extension whose `from_params` constructor owns the presence decision, replacing `write_key_usage()` in the certificate and CSR paths. The minimal-length BIT STRING encoding (including the 9-bit `decipherOnly` case) remains covered by the existing certificate tests.
Port the EKU extension into an `ext::ExtendedKeyUsage` static extension whose `from_params` constructor owns the presence decision, replacing `write_extended_key_usage()` in the certificate and CSR paths.
Port the NameConstraints extension into an `ext::NameConstraints` static extension whose `from_params` constructor owns the presence decision (including omitting the extension when both subtrees are empty), replacing the inline writer and `write_general_subtrees()` in the certificate path.
Port the certificate CRLDistributionPoints extension into an `ext::CrlDistributionPoints` static extension whose `from_params` constructor owns the presence decision, replacing the inline writer in the certificate path. The empty-URIs rejection in `serialize_der_with_signer` is unchanged.
Port the SKI extension into an `ext::SubjectKeyIdentifier` static extension constructed from a `KeyIdMethod` and the subject public key info. The current behavior of only emitting SKI for `IsCa::Ca`/`ExplicitNoCa` certificates (and never for CSRs) is preserved.
The following commits make semver-incompatible changes to the public extension API.
The enum represents the optional pathLenConstraint field of the basic constraints extension, not the extension itself. Renaming it frees the `BasicConstraints` name for the extension type introduced next, without needing to disambiguate between the two.
Port the BasicConstraints extension into an `ext::BasicConstraints` static extension whose `from_params` constructor owns the presence decision (extension omitted entirely for `IsCa::NoCa`), removing `write_ca_extensions()`.
Implement `Extension` for `&CustomExtension` so the certificate and CSR paths write user-supplied extensions through `ext::write_extension()` like the built-in ones, borrowing them from the params rather than cloning. `CustomExtension` now stores a `Criticality` instead of a `bool`, converting at the public accessors.
Port both CRL-level extensions to static extensions: a `CrlNumber` type in the ext module built via `From<&SerialNumber>`, and a `StaticExtension` impl directly on the existing `CrlIssuingDistributionPoint` params type, replacing the inline writers in the CRL serialization path.
Port the reasonCode and invalidityDate CRL entry extensions into `ext::ReasonCode` and `ext::InvalidityDate` static extensions. The `from_params` constructors own the presence decisions (filtering `unspecified(0)` per RFC 5280 §5.3.1), so the crlEntryExtensions presence gate and the writers now share a single source of truth. The unconditional GeneralizedTime encoding for invalidityDate (RFC 5280 §5.3.2) is preserved and remains covered by the existing CRL tests. With no callers left, the `write_x509_extension()` helper is deleted.
…riting Add an `ext::Extensions` collection that preserves insertion order and rejects duplicate OIDs with a new `Error::DuplicateExtension` variant. The collection borrows the extensions (and through them, the params payloads) for the duration of one serialization. The certificate path now builds the full collection via `CertificateParams::extensions()` and only emits the extensions field of the certificate when the built collection is non-empty. This deletes the `should_write_exts` predicate whose drift from the writers previously caused requested extensions to be silently dropped. Presence is now observed from what was built rather than predicted from the params, removing that bug class structurally. Params that request two extensions with the same OID (previously serialized as an invalid duplicate) now fail with `Error::DuplicateExtension`.
Replace `write_extension_request_attribute()` and the `write_extension_request` predicate with a `csr_extensions()` collection builder. Like the certificate extensions field, the PKCS #9 extensionRequest attribute elides itself when the built collection is empty, claiming a slot in the attributes SET only when there is something to write (yasna rejects set elements that produce no output).
Build the crlExtensions field (AKI, CRL number, optional IDP) and each entry's crlEntryExtensions through `ext::Extensions`, eliding the fields when the built collections are empty. The reasonCode/invalidityDate presence gate is gone: the collection's emptiness is the single source of truth. With all writers converted, `write_extension()` becomes private to the ext module.
Previously SKI was only written for `IsCa::Ca`/`ExplicitNoCa` certificates. RFC 5280 §4.2.1.2 describes the SKI as a MUST for CA certificates and a SHOULD for end entity certificates, so emit it unconditionally. CSRs are unchanged (no SKI is requested).
CSRs previously requested extensions as KU, SAN, EKU while certificates emit SAN, KU, EKU. Use the certificate order for the CSR extension request attribute so both paths serialize extensions consistently.
Move the type next to its `Extension` impl, adding whitespace between the impl's methods; the crate root re-export path is unchanged.
Make `Criticality` public and restructure `CustomExtension` with public `oid`, `criticality` and `der_value` fields, dropping the `set_criticality()`/`criticality()`/`content()` accessors. `from_oid_content()` now takes the criticality directly instead of defaulting to non-critical with later mutation.
Add a dedicated `AcmeIdentifier` type for RFC 8737 TLS-ALPN-01 challenge response extensions. It converts into a `CustomExtension` for use in `CertificateParams::custom_extensions`, always critical per RFC 8737 §3. It stays a `CustomExtension` conversion rather than becoming a first-class params field: the extension never appears in CSRs, and nearly every `CertificateParams` would carry a `None` for it. The `TryFrom<&[u8]>` constructor returns the new `Error::InvalidAcmeIdentifierLength` instead of panicking on wrong-length digests like `new_acme_identifier` did.
Move the CSR extension parsing into `from_parsed` constructors on the ext types (`KeyUsage`, `SubjectAlternativeName`, `ExtendedKeyUsage`, `BasicConstraints`), symmetric with their `from_params` serializing counterparts. Unknown requested extensions still yield `Error::UnsupportedExtension`. Custom EKU purpose OIDs now parse into `ExtendedKeyUsagePurpose::Other` and round-trip, where `from_der` previously rejected them with `Error::UnsupportedExtension`.
Rewrite the test-only `CertificateParams::from_ca_cert_der()` as a single pass over the parsed extensions using the shared `from_parsed` constructors, adding test-only `from_parsed` for `NameConstraints` and `SubjectKeyIdentifier`. This deletes the per-field `from_x509` helpers that each re-scanned the certificate (`SanType`, `ExtendedKeyUsagePurpose`, `NameConstraints`, `IsCa`). `KeyUsagePurpose::from_x509` and `KeyIdMethod::from_x509` remain for `Issuer::from_ca_cert_der`.
Serialize params exercising every certificate extension writer, then assert the exact extension count and that parsing recovers what was requested. A certificate missing a requested extension is still well-formed, so presence regressions can only be caught by comparing the output against the requested params.
`CertificateSigningRequestParams::from_der` previously returned `Error::UnsupportedExtension` for any requested extension other than SAN/KU/EKU/BasicConstraints - including custom extensions rcgen itself wrote via `serialize_request`. Iterate the raw extension request attribute (rather than x509-parser's pre-parsed view, which discards OIDs) and recover anything unhandled into `CertificateParams::custom_extensions` via a new `CustomExtension::from_parsed` constructor, preserving OID, criticality and value so that serializing the recovered params reproduces the request.
RFC 5280 §4.2 forbids multiple instances of the same extension. A CSR requesting an extension twice previously either merged both instances into the recovered `CertificateParams` (for natively handled types) or preserved both as custom extensions, deferring the failure to re-serialization. Reject the duplicate up front with `Error::DuplicateExtension`, mirroring the write-side collection invariant.
Like the CSR extension request path, `from_ca_cert_der()` previously merged duplicate instances of natively handled extensions into params silently. Reject them with `Error::DuplicateExtension` instead, mirroring the write-side collection invariant.
Clippy's new `chunks_exact_to_as_chunks` lint (denied in CI) flags `chunks_exact` calls with a constant chunk size. `slice::as_chunks` stabilized in Rust 1.88, the crate MSRV, and yields `[u8; N]` chunks directly, resolving the FIXMEs that waited on `array_chunks`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This revives and finishes the extension rework from #446. Each extension is now its own type in a new ext module, with the presence decision living next to the encoding.
Serialization builds an
Extensionscollection first and only writes what was actually built, so the bug class that produced silently dropped extensions is addressed more structurally.Throughout duplicate OIDs are rejected instead of silently emitted, on both the write and parse sides (#155).
CSR parsing also got friendlier. Requested extensions rcgen doesn't handle natively used to fail the whole parse with
UnsupportedExtension. Now they're preserved as custom extensions, so a CSR round-trips throughfrom_derwithout losing anything (#150). Same deal for extended key usages with purpose OIDs rcgen doesn't know, those now parse intoExtendedKeyUsagePurpose::Otherinstead of erroring.Along the way this grew some related changes:
PathLenConstraintto free up theBasicConstraintsname for the extension type and because it's more precise.CustomExtension/ACME identifier API got a rework.For most users this will all be fairly uneventful and require no code changes since the parameters+issuance APIs remain mostly the same, but some changes are semver-breaking so this bumps to 0.15.0 so we can use
mainto prepare the next major release (#418).Best reviewed commit by commit. Each one moves a single extension or makes one focused change, and every commit builds and passes tests on its own. I also tacked one on the end that fixes a clippy CI failure - I might pull that out into a separate PR since this will take longer to review.
Resolves #150
Resolves #155
Resolves #446
Resolves #122