refactor(x509-cert): decompose AsExtension into Criticality + AsExtension

[npmccallum]

The existing AsExtension trait has two design problems:

1. Criticality is coupled to the type. The critical() method attempts to determine criticality from subject and extensions, but these are not the only factors. Policy decisions, certificate profiles, or issuer preferences may also influence criticality. Some extensions (like BasicConstraints) "MAY appear as critical or non-critical" per RFC 5280.

2. Requires der::Encode. The trait bounds force all extensions to be DER-encodable, but an extension's extnValue is just an OCTET STRING - the encoding could theoretically be anything.

This refactor extracts a new Criticality trait and simplifies AsExtension:

pub trait Criticality {
    fn criticality(&self, subject: &Name, extensions: &[Extension]) -> bool;
}

pub trait AsExtension {
    type Error;
    fn to_extension(&self, subject: &Name, extensions: &[Extension])
        -> Result<Extension, Self::Error>;
}

A blanket impl provides AsExtension for T: Criticality + AssociatedOid + der::Encode, so most existing extension types just need to rename their impl.

New capabilities:

1. Override criticality at the call site using a tuple:

// Use default criticality.
builder.add_extension(&SubjectAltName(names)))?;

// Override default criticality.
builder.add_extension(&(true, SubjectAltName(names)))?;

2. Implement AsExtension directly for non-DER-encoded extensions:

impl AsExtension for MyCustomExtension {
    type Error = MyError;
    fn to_extension(&self, ..) -> Result<Extension, MyError> {
        // Custom encoding logic
    }
}

3. Builders return E::Error directly, letting callers decide error handling:

builder.add_extension(&ext)?;           // propagate E::Error
builder.add_extension(&ext).map_err(…)? // convert to another type

Migration: Replace impl AsExtension with impl Criticality and rename critical() to criticality(). The blanket impl provides AsExtension automatically.

[carl-wallace]

One comment inline… From: Nathaniel McCallum ***@***.***> Reply-To: RustCrypto/formats ***@***.***> Date: Sunday, December 7, 2025 at 12:02 PM To: RustCrypto/formats ***@***.***> Cc: Subscribed ***@***.***> Subject: [RustCrypto/formats] refactor(x509-cert): decompose AsExtension into Criticality + AsExtension (PR #2109) The existing AsExtension trait has two design problems: Criticality is coupled to the type. The critical() method attempts to determine criticality from subject and extensions, but these are not the only factors. Policy decisions, certificate profiles, or issuer preferences may also influence criticality. Some extensions (like BasicConstraints) "MAY appear as critical or non-critical" per RFC 5280. Requires der::Encode. The trait bounds force all extensions to be DER-encodable, but an extension's extnValue is just an OCTET STRING - the encoding could theoretically be anything. [CW] Even though some certs feature “anything” in the OCTET STRING, the contents of the OCTET STRING are supposed to be DER-encodable. RFC 5280 states:        Each extension includes an OID and an ASN.1 structure. When an    extension appears in a certificate, the OID appears as the field    extnID and the corresponding ASN.1 DER encoded structure is the value    of the octet string extnValue. RFC5912 defines Extension this way with a CONTAINING clause: Extension{EXTENSION:ExtensionSet} ::= SEQUENCE {      extnID      EXTENSION.&id({ExtensionSet}),      critical    BOOLEAN

…

--                     ***@***.***}))                       DEFAULT FALSE,      extnValue   OCTET STRING (CONTAINING                  ***@***.***}))                  --  contains the DER encoding of the ASN.1 value                  --  corresponding to the extension type identified                  --  by extnID  } This refactor extracts a new Criticality trait and simplifies AsExtension: pub trait Criticality {     fn criticality(&self, subject: &Name, extensions: &[Extension]) -> bool; } pub trait AsExtension {     type Error;     fn to_extension(&self, subject: &Name, extensions: &[Extension])         -> Result<Extension, Self::Error>; } A blanket impl provides AsExtension for T: Criticality + AssociatedOid + der::Encode, so most existing extension types just need to rename their impl. New capabilities: Override criticality at the call site using a tuple: ```rust // Use default criticality. builder.add_extension(&SubjectAltName(names)))?; // Override default criticality. builder.add_extension(&(true, SubjectAltName(names)))?; ``` Implement AsExtension directly for non-DER-encoded extensions: rust impl AsExtension for MyCustomExtension { type Error = MyError; fn to_extension(&self, ..) -> Result<Extension, MyError> { // Custom encoding logic } } Builders return E::Error directly, letting callers decide error handling: ```rust builder.add_extension(&ext)?; // propagate E::Error builder.add_extension(&ext).map_err(…)? // convert to another type 4. Migration: Replace impl AsExtension with impl Criticality and rename critical() to criticality(). The blanket impl provides AsExtension automatically. You can view, comment on, or merge this pull request online at: #2109 Commit Summary 4313964 refactor(x509-cert): decompose AsExtension into Criticality + AsExtension File Changes (11 files) M x509-cert/src/builder.rs (3) M x509-cert/src/ext.rs (72) M x509-cert/src/ext/pkix.rs (4) M x509-cert/src/ext/pkix/constraints/basic.rs (4) M x509-cert/src/macros.rs (6) M x509-cert/src/request/builder.rs (2) M x509-ocsp/src/basic.rs (2) M x509-ocsp/src/builder/request.rs (2) M x509-ocsp/src/builder/response.rs (2) M x509-ocsp/src/ext.rs (6) M x509-ocsp/src/request.rs (2) Patch Links: https://github.com/RustCrypto/formats/pull/2109.patch https://github.com/RustCrypto/formats/pull/2109.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[npmccallum]

I found the formatting of the email from @carl-wallace hard to read. So I'm extracting what I think is his comment to make it easier to see. Please let me know if I got it wrong.

[CW] Even though some certs feature “anything” in the OCTET STRING,
the contents of the OCTET STRING are supposed to be DER-encodable.
RFC 5280 states:

   Each extension includes an OID and an ASN.1 structure. When an
   extension appears in a certificate, the OID appears as the field
   extnID and the corresponding ASN.1 DER encoded structure is the
   value of the octet string extnValue.

RFC5912 defines Extension this way with a CONTAINING clause:

  Extension{EXTENSION:ExtensionSet} ::= SEQUENCE {
      extnID      EXTENSION.&id({ExtensionSet}),
      critical    BOOLEAN
  --                     (EXTENSION.&Critical({ExtensionSet}{@extnID}))
                       DEFAULT FALSE,
      extnValue   OCTET STRING (CONTAINING
                  EXTENSION.&ExtnType({ExtensionSet}{@extnID}))
                  --  contains the DER encoding of the ASN.1 value
                  --  corresponding to the extension type identified
                  --  by extnID
  }

[carl-wallace]

That's it. The snip was copied from here: Yubico/yubico-piv-tool#181). I don't much care if there is an accommodation for mis-encoded certs (given they exist in the wild) but we ought not to encourage them.

[tarcieri]

Sidebar: I find it a bit odd that the trait is called AsExtension but has one method to_extension. Should it be called ToExtension instead? As* usually suggests borrowing.