Add verification function with custom EKU

[sietseringers]

This adds a new method called verify_is_valid_cert_with_eku() to EndEntityCert, similar to verify_is_valid_tls_server_cert() and verify_is_valid_tls_client_cert() but allowing the caller to specify its own EKU.

We intend to use this crate in our upcoming implementation of the mdoc standard (ISO 18013-5), for use in the EUDI NL wallet. That standard uses its own EKUs, such as 1.0.18013.5.1.2. This change would allow us to use this crate to verify certificate chains having such EKUs.

This PR is loosely based on this older PR before this project was forked.

[codecov]

Codecov Report

Merging #119 (091588a) into main (3313270) will increase coverage by 0.04%.
The diff coverage is 97.53%.

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   95.69%   95.74%   +0.04%     
==========================================
  Files          15       15              
  Lines        3346     3431      +85     
==========================================
+ Hits         3202     3285      +83     
- Misses        144      146       +2     

Impacted Files	Coverage Δ
src/trust_anchor.rs	91.07% <0.00%> (-1.66%)	⬇️
src/verify_cert.rs	95.88% <96.55%> (-0.11%)	⬇️
src/end_entity.rs	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[cpu]

Hi @sietseringers, thanks for the PR :-)

I had a couple initial comments. It would also be nice to see a unit test added that covers this new validation function if that's something you would be willing to implement.

[sietseringers]

Thanks for your comments! I'll answer them, and I'll also be sure to write some unit tests.

[djc]

@sietseringers cool to see you're using this project, presumably for parts of IRMA?

Wondering about the use case. Is the OID you're using registered with IANA or are you using a private namespace?

Are you using webpki to build a verifier for rustls or for some other purpose?

Apart from the EKU, is there other stuff that you're using x509-parser for?

[sietseringers]

No, this is not for IRMA, but for the EUDI NL wallet, which is a separate project. It aims to provide an implementation of mdoc credentials (among others), that the user can (1) obtain from a trusted issuer, (2) store in their EUDI NL wallet app, and then (3) use them to authenticate themselves to relying parties (a website, say).

Wondering about the use case. Is the OID you're using registered with IANA or are you using a private namespace?

Not as far as I know, no. oidref.com is unaware of it, anyway.

Are you using webpki to build a verifier for rustls or for some other purpose?

No, we have a different purpose. The relying party (RP) that receives an mdoc credential from the user has to verify it against the public key of the organization that issued the mdoc, which comes in the form of an X509 certificate (chain). The RP has a trust root, and so we need to verify that certificate chain against the trust root. That's the purpose in our case.

Apart from the EKU, is there other stuff that you're using x509-parser for?

I used it to get the subject of the certificate, as well as the public key to verify the mdoc with -- although for the latter I may decide to use EndEntityCert::verify_signature() instead; back when I made that I was not yet aware of the existence of this crate.

I see that the CI is still not happy with the PR; I'll have a look at that.

[cpu]

Thanks for the iteration! This is shaping up nicely :-)

I had another round of small comments but I think the overall shape looks great.

[sietseringers]

Thanks for the review! Good suggestions. I implemented them and added some more tests; I hope the CI is happy now.

[sietseringers]

Thanks for the review! Good suggestions. I implemented them and added some more tests; I hope the CI is happy now.

Well, almost. Looks like the main branch also has the problem it indicates now.

[cpu]

I hope the CI is happy now.

Almost! Looks like there's a couple constants you'll need to gate on the alloc feature to avoid them being considered dead code in the no-default-features build.

FWIW I think if you allow GitHub actions on your fork you can iterate on CI in separate branches as needed. Just mentioning in case one of us misses pressing the "Allow workflows to run" button here after an update.

[cpu]

Looks like the main branch also has the problem it indicates now.

Hmmm the errors I see in this failed task are specific to tests/custom_ekus.rs which is new to this branch.

[sietseringers]

FWIW I think if you allow GitHub actions on your fork you can iterate on CI in separate branches as needed.

Oh right, that's very useful, thanks! Didn't know that 😅

Hmmm the errors I see in this failed task are specific to tests/custom_ekus.rs which is new to this branch.

Ah yes, I read it wrong. Should be all good now.

[cpu]

This looks good to me, thanks for addressing my earlier feedback.

@ctz Could you give this a pass and see if you have any reservations?

I think we'll want to squash-merge this in-tree since the commit history has gotten a little bit long from the iteration.

[djc]

Ideally we would land this with a clean commit history that has a separate commit introducing the new data type and refactoring needed to pass that around before in another commit adding the new high-level API (and changing existing API to rely on it).

I noticed (reviewing on mobile so somewhat constrained by the number of available pixels) that the new method has a different way of taking the trust anchors. Is that well-motivated? In the absence of explicit motivation otherwise I'd be inclined to have them receive arguments the same way.

Not for this PR, but I do wonder if we shouldn't change the top level API here to build a verifier object using a builder API which then gets the EndEntityCert as an argument to a verify method, similar to what @jsha is proposing for the client verifier API in rustls (someone please link in the relevant issue/comment) here, potentially taking ownership of an Arc<[TrustAnchor]> or similar.

[cpu]

Thanks @djc (I didn't tag you explicitly to review only because I knew you're on vacation 🤪)

I noticed (reviewing on mobile so somewhat constrained by the number of available pixels) that the new method has a different way of taking the trust anchors. Is that well-motivated? In the absence of explicit motivation otherwise I'd be inclined to have them receive arguments the same way.

This was motivated by my feedback to implement verify_is_valid_tls_server_cert and verify_is_valid_tls_client_cert via a call to verify_is_valid_cert_with_eku. I made that suggestion when I thought the bodies of each were identical save for the eku but in retrospect the trust_anchor arguments in the function signatures were different to have the type system identify the purpose of the anchors. It might be better to undo that change at the cost of a bit of duplication.

Not for this PR, but I do wonder if we shouldn't change the top level API here to build a verifier object using a builder API which then gets the EndEntityCert as an argument to a verify method, similar to what @jsha is proposing for the client verifier API in rustls (someone please link in the relevant issue/comment) here, potentially taking ownership of an Arc<[TrustAnchor]> or similar.

rustls/rustls#1360

[sietseringers]

I noticed (reviewing on mobile so somewhat constrained by the number of available pixels) that the new method has a different way of taking the trust anchors. Is that well-motivated? In the absence of explicit motivation otherwise I'd be inclined to have them receive arguments the same way.

My motivation was that while the other two functions take TlsClientTrustAnchors and TlsServerTrustAnchors, in the case of verify_is_valid_cert_with_eku(), we have trust anchors that are not (generally) of either kind. They are just trust anchors, so I made the function accept a slice directly. I would be happy to add a TrustAnchors newtype wrapping
&[TrustAnchor], however; that would indeed be more consistent with the other two functions.

This was motivated by my feedback to implement verify_is_valid_tls_server_cert and verify_is_valid_tls_client_cert via a call to verify_is_valid_cert_with_eku. I made that suggestion when I thought the bodies of each were identical save for the eku but in retrospect the trust_anchor arguments in the function signatures were different to have the type system identify the purpose of the anchors. It might be better to undo that change at the cost of a bit of duplication.

Couldn't I do what I suggested above, and keep the function structure as is by wrapping the anchors in the new TrustAnchors type when invoking verify_is_valid_cert_with_eku()? That seems reasonable to me and keeps the code deduplication that your suggestion resulted into.

Ideally we would land this with a clean commit history that has a separate commit introducing the new data type and refactoring needed to pass that around before in another commit adding the new high-level API (and changing existing API to rely on it).

I could close this PR, and open a new one with the commit structure you suggest. While we're at this, am I right in that you prefix the commit messages with the module name that had all or at least most of the changes?

[cpu]

Couldn't I do what I suggested above, and keep the function structure as is by wrapping the anchors in the new TrustAnchors type when invoking verify_is_valid_cert_with_eku()? That seems reasonable to me and keeps the code deduplication that your suggestion resulted into.

Sounds reasonable to me 👍

I could close this PR, and open a new one with the commit structure you suggest.

I think you could also rebase your work in this branch and force push an update, but if a new PR is easier for you I don't think anyone would mind. It's slightly nicer IMO to have all the discussion history in one place.

am I right in that you prefix the commit messages with the module name that had all or at least most of the changes?

I think that's a nice habit but I don't think it's a practice we apply universally.

[sietseringers]

Alright, done (with force push). I also rebased onto the current main.

[cpu]

ci / coverage (pull_request) Failing after 1m

Looks like a transient error on coveralls side. Can be ignored

[cpu]

Expected — Waiting for status to be reported

Apologies, in #118 I moved around some CI tasks. If you rebase this branch on main the run+expected checks should line up again.

[cpu]

@ctz Did you have any additional feedback for this branch?

[djc]

This is looking good to me. A few minor suggestions.

[sietseringers]

@djc Thanks! I implemented your suggestions.

[cpu]

Thanks!

[sietseringers]

Great, thanks! 🎉