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

Expand the public API slightly #35

Merged
merged 3 commits into from
Jul 7, 2020
Merged

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Jul 6, 2020

For X509Name, exposes parsing as well as accessing the raw data after parsing.

For parsed certificate extensions, this fixes an unnecessarily short lifetime.

Copy link
Member

@chifflier chifflier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR!
I just have a few questions (added as comments)

@@ -48,7 +48,7 @@ impl<'a> X509Extension<'a> {
}

/// Return the extension type or `None` if the extension is not implemented.
pub fn parsed_extension(&self) -> &ParsedExtension {
pub fn parsed_extension(&self) -> &ParsedExtension<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand - the compiler was inferring a wrong lifetime without the explicit one?

Copy link
Contributor Author

@g2p g2p Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Lifetime elision was picking up the anonymous lifetime on the &self reference, and not the 'a lifetime on the impl block.

My usage goes like this:
I have a long-lived ('a) object that carries the entire DER of the certificate.
A function parses it and returns &str data that is sliced from the same DER.
The &str should be able to live as long as the parent DER, but the X509Extension and ParsedExtension structs are local to the function; having the reference lifetime (function local) distinct from the 'a lifetime used inside the structs allows me to return the right &'a str.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now, thank you for the details.
I also encountered this kind of limitations within DER, where the compiler did not apply "transitive" lifetimes to inner objects when returned, but agrees when I give it explicitly - maybe I will be able to fix this now!


impl<'a> X509Name<'a> {
// Not using the AsRef trait, as that would not give back the full 'a lifetime
pub fn as_raw(&self) -> &'a [u8] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm also curious about the comment on lifetimes (which seems similar to the above one)
  • Could you share the use case where you needed the raw bytes? I'm asking mostly because, if this is useful for several fields, the as_raw method could be added to all parsed components

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above; the AsRef trait takes no lifetime parameter, and the as_ref function can only return data with the lifetime on the X509Name; if the parsing is done in an inner function, the inner function won't be able to return slices from that.

My use case is sending OCSP requests and validating the reponses; the raw data is sometimes hashed (building the CertID for the OCSP protocol), it can also be copied (some parts of the request should exactly match the certificates that are being checked), and compared (to check that the response really matches the request).

@@ -51,14 +51,23 @@ fn parse_rdn(i: &[u8]) -> BerResult<RelativeDistinguishedName> {
.map(|(rem, x)| (rem, x.1))
}

pub(crate) fn parse_name(i: &[u8]) -> BerResult<X509Name> {
pub fn parse_name(i: &[u8]) -> BerResult<X509Name> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with making this public, but FYI I think I'll just change to a better name (parse_x509_name) by adding a commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@g2p
Copy link
Contributor Author

g2p commented Jul 7, 2020

I think having as_raw everywhere would prove useful.
Maybe it should be supported in parse_der_struct?

@chifflier
Copy link
Member

I think having as_raw everywhere would prove useful.
Maybe it should be supported in parse_der_struct?

I'll see if this can be added easily, but that will be a different issue. Just thinking of rewriting parse_der_struct gives me headaches :p

@chifflier chifflier merged commit 1ad3e68 into rusticata:master Jul 7, 2020
@chifflier
Copy link
Member

Merged, thanks!

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 this pull request may close these issues.

None yet

2 participants