Add value field to ParsedExtension::UnsupportedExtension

[jean-airoldie]

This is the simplest solution I could think off. However this is a breaking change, which isn't ideal.

Closes #205.

[cpu]

👋 Thanks for the PR!

However this is a breaking change, which isn't ideal.

I believe @chifflier is preparing a new semver breaking release, so that might not be a non-starter.

However, I also left a comment showing a way to accomplish this without a code change so perhaps it's better to not add new API surface at all?

[jean-airoldie]

Your comment addressed my issue, so I will be closing this.

[jean-airoldie]

See my comment.

[cpu]

I think the remaining clippy failures are addressed in #203.

[jean-airoldie]

@cpu Alright, I guess I'll merge when that PR eventually gets merged.

[jean-airoldie]

While not strictly required, this API is much nicer than using X509CertificationRequestInfo::attributes etc. methods because we don't need to manually decode the OID_PKCS_9_AT_EXTENSION_REQUEST extension. See my comment for more details.

Here's a specific usage example: https://github.com/jean-airoldie/rcgen/blob/30f01893a8002e5c7dd12a4a224ce3f1b70449ba/rcgen/src/csr.rs?pain=#L209-L218

[chifflier]

@jean-airoldie Thank you for the PR and the discussion
The breaking PR has been sent in #207 and is currently under discussion. Please wait for this before changing code, since there is a code refactor that will result in conflicts otherwise.

About the extensions and API, I am totally in favor of updating the API if this makes things easier.
Generally speaking, extensions are stored in both unparsed (reference to bytes) and parsed form in internal objects, so it is possible to expose everything.

I also realize that there is no pattern visitor (like for certificates and CRL). Would it be useful for your use case?

[jean-airoldie]

@chifflier

@jean-airoldie Thank you for the PR and the discussion
The breaking PR has been sent in #207 and is currently under discussion. Please wait for this before changing code, since there is a code refactor that will result in conflicts otherwise.

Alright, I will wait for that then.

I also realize that there is no pattern visitor (like for certificates and CRL). Would it be useful for your use case?

If the CSR visitor pattern walk method is recursive, meaning it would walk over each X509Extension found in X509CriAttribute with OID_PKCS_9_AT_EXTENSION_REQUEST (which as far as I understand is already parsed as ParsedCriAttribute::ExtensionRequest), then this would indeed also work for my use case. I just want to avoid having to manually parse the X509CriAttribute in the case unsupported extensions (in my case prioprietary) are encountered.

[chifflier]

@chifflier

@jean-airoldie Thank you for the PR and the discussion
The breaking PR has been sent in #207 and is currently under discussion. Please wait for this before changing code, since there is a code refactor that will result in conflicts otherwise.

Alright, I will wait for that then.

Hi @jean-airoldie
#207 has been merged and code has now changed quite a lot. In particular, extensions/mod.rs has been split into multiple files to make code easier. I know this will cause some conflicts and am sorry for it 😢

Can you check how you code can be adapted? Looking at your code, I think it shouldn't be too complicated

I also realize that there is no pattern visitor (like for certificates and CRL). Would it be useful for your use case?

If the CSR visitor pattern walk method is recursive, meaning it would walk over each X509Extension found in X509CriAttribute with OID_PKCS_9_AT_EXTENSION_REQUEST (which as far as I understand is already parsed as ParsedCriAttribute::ExtensionRequest), then this would indeed also work for my use case. I just want to avoid having to manually parse the X509CriAttribute in the case unsupported extensions (in my case prioprietary) are encountered.

I see. I wonder if this is more a performance problem (avoiding deep-parsing unknown extensions) or an API problem, because if using the visitor the extensions will be parsed anyway, so not improving performance.

[jean-airoldie]

@chifflier Alright I rebased into master. I also squashed my commits so I'm gonna need one review to see if CI passes (forgot about that quirk).

I feel that this API is satisfactory, but as far as I understand a visitor API would also do the trick. For my use case, I think the visitor API would result in slightly less code, so I don't feel its necessarily needed.

As a side note, I'm not a big fan of ans1_rs::Input::as_bytes2 naming. Wouldn't it make sense to simply name it as_bytes and the impl nom AsBytes traits as Input::as_bytes(self) to avoid the stack overflow?

[chifflier]

@chifflier Alright I rebased into master. I also squashed my commits so I'm gonna need one review to see if CI passes (forgot about that quirk).

It seems the CI passes, I'll review the PR as soon as possible

I feel that this API is satisfactory, but as far as I understand a visitor API would also do the trick. For my use case, I think the visitor API would result in slightly less code, so I don't feel its necessarily needed.

You can have a look at the visitor API in #209
You can have access to the raw values, so it could work for you. That said, storing the data in UnsupportedExtension looks good to me.

As a side note, I'm not a big fan of ans1_rs::Input::as_bytes2 naming. Wouldn't it make sense to simply name it as_bytes and the impl nom AsBytes traits as Input::as_bytes(self) to avoid the stack overflow?

Agreed, the naming is not satisfying, but I had to use a different function name than the traits and I was not very inspired.
The reason behind is that the default traits (AsRef in std and AsBytes in nom) have a lifetime problem: they constrain the lifetime of data to be the same as the one from the object, which breaks things in parsers where data can outlive intermediate objects. See this rust forum entry for the entire explanation.

[jean-airoldie]

You can have a look at the visitor API in #209
You can have access to the raw values, so it could work for you. That said, storing the data in UnsupportedExtension looks good to me.

Alright I'll try that.

Agreed, the naming is not satisfying, but I had to use a different function name than the traits and I was not very inspired.

Yeah I guess what I meant is I didn't understand why you wanted to use a different name than the trait. If the nom::AsBytes trait isn't used explicitely, the name clash doesn't matter. Specificically, I did sed -i s/as_bytes2/as_bytes/g **/**.rs in your asn1-rs codebase and everything seems to work fine, except compiler warnings about the nom::AsBytes import being unneeded.

[chifflier]

Merged, thanks!