Skip to content

Continued extension representation rework - #454

Open
cpu wants to merge 22 commits into
mainfrom
ci/rework-exts-pt2
Open

cpu wants to merge 22 commits into
mainfrom
ci/rework-exts-pt2

Conversation

@cpu

@cpu cpu commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

This revives and continues the extension rework from #446 and continues where #449 left off by starting on breaking changes. Each extension is now its own type in a new ext module, with the presence decision living next to the encoding.

Serialization builds an Extensions collection 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).

Along the way this grew some related changes:

  • extensions borrow from params instead of cloning
  • the path length enum was renamed PathLenConstraint to free up the BasicConstraints name for the extension type and because it's more precise.
  • SKI is now emitted for end entity certs too (I think this is the right default), and the CustomExtension/ACME identifier API got a rework.
  • Some parameters (e.g. name constraints) were changed to prevent invalid states from being represented earlier than ext construction.

Resolves #155
Resolves #446

The next branch continuing from here will pick up some CSR + custom extensions improvements on top.

@cpu
cpu added this pull request to stack #455 September 18, 2026 18:04
@cpu cpu self-assigned this Sep 18, 2026
Base automatically changed from ci/rework-exts to main September 21, 2026 09:42
@djc
djc force-pushed the ci/rework-exts-pt2 branch from 0e1d34e to 33f178f Compare September 21, 2026 09:42
@djc djc mentioned this pull request Sep 21, 2026
@cpu
cpu force-pushed the ci/rework-exts-pt2 branch from 33f178f to 6171866 Compare September 21, 2026 15:43
@cpu
cpu marked this pull request as draft September 21, 2026 15:48
@cpu
cpu force-pushed the ci/rework-exts-pt2 branch from 6171866 to c624890 Compare September 21, 2026 16:00
@cpu
cpu marked this pull request as ready for review September 21, 2026 16:32
@cpu

cpu commented Sep 21, 2026

Copy link
Copy Markdown
Member Author

cpu marked this pull request as ready for review now

Rebased on main and carried forward the tweaks from #456 throughout.

@djc djc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest we split this once more, before the AcmeIdentifier inclusion?

Two concerns remaining (somewhat from the previous PR):

  • It feels like in a number of cases, this is wrapping a local type in another local type, when we could arguably implement Extension for the existing type rather than wrapping it in another layer.
  • The from_params(params: ..) -> Option<Self> pattern feels somewhat overwrought? Especially to the extent we can reuse existing types, can we just have write() yield bool instead of having this indirection between something that exists, then is written?

Comment thread rcgen/src/certificate.rs Outdated
Comment thread rcgen/src/crl.rs
Comment thread rcgen/src/extension.rs Outdated
Comment thread rcgen/src/extension.rs Outdated
@djc

djc commented Sep 22, 2026

Copy link
Copy Markdown
Member
  • The from_params(params: ..) -> Option<Self> pattern feels somewhat overwrought? Especially to the extent we can reuse existing types, can we just have write() yield bool instead of having this indirection between something that exists, then is written?

Oh, I guess this is necessary because we only want to write the extensions wrapper when we know there are 1+ extensions to write? Still wondering if there isn't a more light-weight pattern at least for some of the extensions.

@cpu

cpu commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Still wondering if there isn't a more light-weight pattern at least for some of the extensions.

I will noodle on this when I split it up & address your other feedback. This part of the solution is more or less as-is from 2023 (time flies!) and I wouldn't be surprised if in 2026 I have a better idea 😆

@cpu
cpu marked this pull request as draft September 22, 2026 17:23
@cpu
cpu force-pushed the ci/rework-exts-pt2 branch from c624890 to 05e1156 Compare October 9, 2026 16:55
cpu added 14 commits October 9, 2026 12:55
Use the name of the optional `pathLenConstraint` field to distinguish
the CA chain length limit from the basic constraints extension itself.
Introduce `NonEmptySlice` with a first item and remaining slice.
Implement `StaticExtension` on `NonEmptySlice<KeyUsagePurpose>` and
remove the redundant extension-specific `KeyUsage` wrapper type. Reuse
the encoder for borrowed extensions through the shared implementation.
Implement `StaticExtension` on `NonEmptySlice<ExtendedKeyUsagePurpose>`
and remove the redundant extension-specific `ExtendedKeyUsage` wrapper
type. Certificate and CSR writers omit empty usage lists and borrow
nonempty lists for encoding.
Store SAN names in `NonEmptySlice`. Retain the `SubjectAlternativeName`
extension wrapper to carry criticality derived from the subject
distinguished name.
Require at least one subtree and ASCII DNS and email names. Make the
lists private, provide read-only access and checked additions, and
migrate callers and parsing.
Implement `StaticExtension` directly on the checked `NameConstraints`
parameter type. Remove the redundant extension-specific
`NameConstraintsExt` wrapper type and its absence check. Encode each
present subtree list through `NonEmptySlice`.
Require one or more ASCII URI names. Make the names private with read-
only access and checked additions, and migrate callers. Use the checked
parameter encoder in certificates and CRLs instead of checking for empty
names during serialization.
Move the URI name encoder into `CrlDistributionPoint::write_name()`
without changing its encoding. Remove the forwarding method and free
function so the encoder receives a checked distribution point directly.
Implement `StaticExtension` on `NonEmptySlice<CrlDistributionPoint>` and
remove the redundant extension-specific `CrlDistributionPoints` wrapper
type. Each point already guarantees nonempty ASCII URI names.
Move `IsCa`, its parsing helpers, and `PathLenConstraint` unchanged into
`extension`. Update imports, re-exports, and the parsing helper
visibility required by the module boundary. Keep the basic constraints
writer unchanged for the following refactoring commit.
Replace `IsCa` with `BasicConstraints` and
`CertificateParams::basic_constraints: Option<BasicConstraints>`. Keep
omission separate from valid extension values, update parsing and
callers, and preserve the existing default and inline encoding.
Implement `StaticExtension` directly on `BasicConstraints` and remove
the inline certificate/CSR encoder. Keep omission in the `Option`-valued
certificate parameter.
Move `CustomExtension` and its existing methods unchanged into
`extension` before implementing `Extension`. Preserve the `bool`
criticality and original constructors and accessors. Adjust imports, re-
exports, and field visibility for the existing certificate writer.
Implement `Extension` for `&CustomExtension` next to the type. Route
certificate and CSR serialization through the trait, borrowing custom
extensions from params. Store `Criticality` instead of `bool` and retain
the existing public `bool` accessors.
cpu added 8 commits October 9, 2026 12:55
Keep CRL-only types and encoders in `crl`. Implement `StaticExtension`
on the existing issuing distribution point type and introduce a borrowed
`CrlNumber` for the CRL-specific meaning of `SerialNumber`. Replace the
inline extension writers without moving the existing types.
Implement `StaticExtension` directly on `RevocationReason` and add an
`InvalidityDate` encoder in `crl`. Select optional values in the entry
writer, omitting unspecified reason codes and preserving unconditional
`GeneralizedTime` for invalidity dates. Remove the unused
`write_x509_extension` helper without moving the existing revocation
reason enum.
…riting

Add `Extensions` with duplicate-OID rejection and generic
`push()`/`push_if_some()` insertion. Build certificate extensions before
starting encoding, and omit the outer field for an empty collection.
Build CSR extension requests through `Extensions`, omitting empty lists
and borrowing nonempty values for encoding. Let collection emptiness
determine whether to include the attribute.
Build CRL extensions before starting encoding and encode CRL and entry
extensions through `Extensions`. Use checked issuing distribution points
directly and `push_if_some()` for optional fields; omit empty extension
collections.
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.
Make `Criticality` public and keep its two boolean values exhaustive.
Give `CustomExtension` public `oid`, `criticality`, and `der_value`
fields, and remove its redundant accessors. Keep the struct
non-exhaustive and provide `new()` to construct an extension with its
OID, criticality, and DER encoded value.
@cpu
cpu force-pushed the ci/rework-exts-pt2 branch from 05e1156 to 12c3d72 Compare October 9, 2026 16:56
@cpu

cpu commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Still wondering if there isn't a more light-weight pattern at least for some of the extensions.

I think I arrived at a solution that's less heavy weight and let me drop a bunch of types that were just wrappers on top of existing types. More of the extension code is now directly implemented on the pre-existing params types. The only cases where that isn't true we're imposing some additional context on top in a way that felt justified.

As a result the branch grew some new commits in two categories:

  1. I undid some of the wrapper types that were added in Rework X.509 cert/csr/crl extension representation #449 - sorry for the churn there.
  2. I shifted left some of the validation that was being done in wrapper types to be part of the parameter types directly. This mainly involves making illegal states impossible to represent at the param stage instead of catching it at the ext serialization stage. I tried to avoid doing anything that made the params cumbersome to construct.

Suggest we split this once more, before the AcmeIdentifier inclusion?

Done. I'll rework the remaining bits that were dropped from this branch after this one is ready to go.

I think this is ready for another review pass when you have time.

@cpu
cpu marked this pull request as ready for review October 9, 2026 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rework X509 extension code Enforce extension uniqueness

2 participants