x509-cert: Document (or improve) how to construct custom extensions

[str4d]

CertificateBuilder::add_extension has the following public interface:

formats/x509-cert/src/builder.rs

Lines 196 to 197 in e0c3e08

	/// Add an extension to this certificate
	pub fn add_extension<E: AsExtension>(&mut self, extension: &E) -> Result<()> {

If I follow the breadcrumb trail to the AsExtension trait, I get this as the public API that needs implementing for a custom extension:

formats/x509-cert/src/ext.rs

Lines 47 to 51 in e0c3e08

	/// Trait to be implemented by extensions to allow them to be formatted as x509 v3 extensions by
	/// builder.
	pub trait AsExtension: AssociatedOid + der::Encode {
	/// Should the extension be marked critical
	fn critical(&self, subject: &crate::name::Name, extensions: &[Extension]) -> bool;

• AssociatedOid makes enough sense to me.

• der::Encode is somewhat confusing, because its documentation is just:

  Encoding trait.

  The method documentation is a little better:

  Encode this value as ASN.1 DER using the provided Writer.

  However, it is unclear how the output of this method will be treated. My particular example is the YubiKey PIV attestation policy extension, which is documented as:

  Extensions in the generated certificate:

  • ...
  • 1.3.6.1.4.1.41482.3.8: Two bytes, the first encoding pin policy and the second touch policy

  Should that map to two raw ASN.1 bytes? Is there some other ASN.1 DER wrapper around it that I need to know about? This should be handled in the yubikey crate (which I will upstream my impl to), but it is unclear what kind of verification happens on the output of der::Encode::encode.

• AsExtension::critical makes zero sense to me. What is a "critical extension" and how should I decide whether mine is? There is no description here to guide me, or any references to external documentation I should read before deciding what to return here.

It would be helpful to improve documentation across the board here, but in particular:

• CertificateBuilder::add_extension should document where to find existing extensions (a direct link to the list of AsExtension implementors would be ideal).
• Either CertificateBuilder::add_extension or AsExtension should document how to go about implementing a custom extension, with considerations about correctness (how to ensure everyone can parse your extension), best practices, and whatever criticality is.

[carl-wallace]

It may be worth noting that at least some YubiKey extensions are not encoded per RFC5280, i.e., they hold raw bytes not an ASN.1 type. I am not sure offhand how well this would play with any profile implementation. Some details are here: Yubico/yubico-piv-tool#181.

[str4d]

Indeed; that is my primary concern with the required der::Encode (and der::Decode for #1491) traits, as I mentioned above:

it is unclear what kind of verification happens on the output of der::Encode::encode.

I have no idea what ASN.1 DER correctness constraints those are placing on the custom extensions, or if there is any way to work around those constraints for "legacy" extensions that did not follow whatever RFC says that extension values are ASN.1 DER encoded.

[baloo]

There isn't much I can do about yubico using misformatted extensions.
Otherwise I hope I addressed the documentation for AsExtension::critical in #1497

[wiktor-k]

AsExtension::critical makes zero sense to me. What is a "critical extension" and how should I decide whether mine is? There is no description here to guide me, or any references to external documentation I should read before deciding what to return here.

Just for completeness sake "critical" means "if your program reading extensions doesn't recognize this extension then the validation should fail". This is a way to indicate that some particular extension is not optional but rather important.

It's rarely used but one example in the wild is the "poison extension" in pre-certificates: https://certificate.transparency.dev/howctworks/ Which causes the pre-certificate to be rejected (because of the critical bit in the poison extension).