Skip to content

x509-cert: Rename and document TbsCertificateInner::{get, filter} #1491

Closed
@str4d

Description

@str4d

After writing a custom extension (#1490), I wanted to parse it from a certificate. The AsExtension trait has no decoding-related constraints, and neither it nor CertificateBuilder::add_extension document what to do.

After writing my own custom parsing logic, I then (while digging through the x509-cert code for an unrelated issue) discovered TbsCertificateInner::{get, filter}. TbsCertificateInner::get is exactly what I needed. However, it was impossible to discover due to its very opaque method name and being buried well below the top-level Certificate type.

I am guessing that the method names get and filter were chosen to follow a similar naming scheme to the standard Rust container types. However, certificates are not pure containers, and some certificate versions don't even support extensions, so having the untyped get method be how you get certificate extensions is very unintuitive. I think the methods should be renamed to e.g. get_extension and filter_extensions.

The methods should also be extensively documented, both on themselves and on Certificate (which is where a user will be looking for functionality, particularly after it is made opaque in #1486). In particular, these two aspects should be documented:

  • The currently-named TbsCertificateInner::filter should be documented as invalid / unsafe to use with the RFC 5280 profile, because RFC5280 forbids having duplicate extensions, but does not specify how errors should be handled. If Rust had specialization yet then I'd recommend an alternative implementation for the Rfc5280 concrete type that always returns an error (or better yet, prevents the method from being accessible at all), but at a minimum the documentation has to make this explicitly clear to the user.
  • The return value (bool, T) is currently called "the extension", but given that T is the user-requested extension type, the bool is completely undocumented.

It would also be beneficial to either update the AsExtension trait documentation, or somewhere else where custom extensions are defined, to document or give examples of the full "custom extension" process, trait impls, and interaction patterns. (In particular, someone who impls AsExtension and then goes looking for how to use it for parsing will be out of luck, because AsExtension is not used anywhere on the parsing side. Also, my concerns about der::Encode in #1490 also apply to der::Decode now that I know it is necessary, plus things like "what happens if there is excess data left over after I've parsed what I expect?")

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions