Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
str4d opened this issue Sep 1, 2024 · 0 comments · Fixed by #1497
Closed

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

str4d opened this issue Sep 1, 2024 · 0 comments · Fixed by #1497
Assignees

Comments

@str4d
Copy link

str4d commented Sep 1, 2024

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?")

baloo added a commit to baloo/formats that referenced this issue Sep 4, 2024
As noted in RustCrypto#1491, the naming of the helpers to get or filter extensions
was confusing.

Fixes RustCrypto#1491.
baloo added a commit to baloo/formats that referenced this issue Sep 6, 2024
As noted in RustCrypto#1491, the naming of the helpers to get or filter extensions
was confusing.

Fixes RustCrypto#1491.
tarcieri pushed a commit that referenced this issue Sep 8, 2024
As noted in #1491, the naming of the helpers to get or filter extensions
was confusing.

Fixes #1491.
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 a pull request may close this issue.

3 participants