You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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?")
The text was updated successfully, but these errors were encountered:
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 norCertificateBuilder::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) discoveredTbsCertificateInner::{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-levelCertificate
type.I am guessing that the method names
get
andfilter
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 untypedget
method be how you get certificate extensions is very unintuitive. I think the methods should be renamed to e.g.get_extension
andfilter_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: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 theRfc5280
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.(bool, T)
is currently called "the extension", but given thatT
is the user-requested extension type, thebool
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 implsAsExtension
and then goes looking for how to use it for parsing will be out of luck, becauseAsExtension
is not used anywhere on the parsing side. Also, my concerns aboutder::Encode
in #1490 also apply toder::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?")The text was updated successfully, but these errors were encountered: