Verify and validate certificate policies extensions

[kikuomax]

Summary

Deal with certificate policies extensions so that rustls-webpki won't fail when it faces a critical certificate policies extensions.

• fixes Support anyPolicy Certificate Policy #30
• a revision of Remember certificate policies extensions but do nothing further #211

With this PR:

• rustls-webpki no longer rejects certificates with critical certificate policies extensions as long as they conform to the definition described in RFC 5280, Section 4.2.1.4.
• rustls-webpki provides a utility function that applications can call to validate the policy tree.
• rustls-webpki introduces optional feature cert_policy that enables the above functionalities

How verification works

rustls-webpki first remembers a certificate policies extension in cert::Cert::certificate_policies.

rustls-webpki verifies the structure of the remembered certificate policies extension by calling cert_policy::check_certificate_policies from verify_cert::ChainOptions::build_chain_inner when it reaches a trust anchor.

cert_policy::check_certificate_policies verifies:

• the structure of the extension conforms to RFC 5280, Section 4.2.1.4
• policy OIDs do not duplicate in the extension
• if policy qualifiers are included in the extension, they are CPS Pointer (URI) or User Notice

cert_policy::check_certificate_policies does not validate the policy tree.

How validation works

Applications can call cert_policy::validate_policy_tree_paths function, exported as validate_policy_tree_paths, in verify_path callback of EndEntityCert::verify_for_usage to validate the policy tree.

Unlike the algorithm described in RFC 5280, Section 6.1, validate_policy_tree_paths does not build the entire policy tree but performs depth-first search. As long as requirements of applications meet all of the following conditions, what validate_policy_tree_paths validates should be equivalent to what they can do with the full policy tree:

• the policy tree must include at least one policy in the application-provided list (user-initial-policy-set)
• any policy (anyPolicy) is always allowed
• policy mappings extension is not supported
• policy constraints extension is not supported
• inhibit anyPolicy extension is not supported

You can find justification of my approach in the comments of cert_policy::validate_policy_tree_paths. I also excerpted it as a gist.

Why optional?

cert_policy feature is optional because additional computation and binary size may not be tolerated.

[cpu]

Thanks for the PR, I appreciate the detailed description 👍

I think you have some CI failures to sort out for the builds that activate the new feature flag.

rustls-webpki provides a utility function that applications can call to validate the policy tree.
rustls-webpki introduces optional feature cert_policy that enables the above functionalities

Quickly looking at the diff I'm not sure I'm convinced yet this should be included in webpki, even behind a feature flag. It looks like the meat of the policy validation is integrated by way of a fn to be called as a verify_path callback. Couldn't that be offered from a separate webpki-policy-assertions crate (or something named based on your use-case)?

I'm generally nervous about wading into the policy space. It's pretty gross and I think there aren't many great examples to validate against (e.g. I think Go's stdlib avoids doing anything other than representing policies in a cert). It's a big chunk of new code and complexity.

Unlike the algorithm described in RFC 5280, Section 6.1, validate_policy_tree_paths does not build the entire policy tree but performs depth-first search.

It seems like the algorithm in 5280 is outright dangerous (see draft-ietf-lamps-x509-policy-graph) which is another reason to be wary of this entire area IMO 😬

[kikuomax]

@cpu Thank you for your reply.

I think you have some CI failures to sort out for the builds that activate the new feature flag.

rustls-webpki provides a utility function that applications can call to validate the policy tree.
rustls-webpki introduces optional feature cert_policy that enables the above functionalities

Sorry, I have not tested building with the latest Rust.

Quickly looking at the diff I'm not sure I'm convinced yet this should be included in webpki, even behind a feature flag. It looks like the meat of the policy validation is integrated by way of a fn to be called as a verify_path callback. Couldn't that be offered from a separate webpki-policy-assertions crate (or something named based on your use-case)?

Separating the policy validation in a different crate makes sense as it is highly application specific.

It seems like the algorithm in 5280 is outright dangerous (see draft-ietf-lamps-x509-policy-graph) which is another reason to be wary of this entire area IMO 😬

Thanks for guiding me to the draft. I will look into it.

[kikuomax]

I think you have some CI failures to sort out for the builds that activate the new feature flag.

rustls-webpki provides a utility function that applications can call to validate the policy tree.
rustls-webpki introduces optional feature cert_policy that enables the above functionalities

Sorry, I have not tested building with the latest Rust.

@cpu I found my branch did not include the commit that fixes some clippy issues in the nightly build. I will rebase and force push my branch to sync main.

[kikuomax]

@cpu I removed validate_policy_tree_paths from the crate but exposed cert_policy module, and its types and methods necessary for applications to implement custom policy validation.

I also added a new row for cert_policy to the feature table in the documentation.

[codecov]

Codecov Report

Attention: Patch coverage is 96.38554% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 97.03%. Comparing base (c5e07ad) to head (eacdb47).

Files	Patch %	Lines
src/cert_policy.rs	97.78%	18 Missing ⚠️
src/cert.rs	30.00%	7 Missing ⚠️
src/error.rs	0.00%	4 Missing ⚠️
src/verify_cert.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
- Coverage   97.14%   97.03%   -0.11%     
==========================================
  Files          19       20       +1     
  Lines        4336     5166     +830     
==========================================
+ Hits         4212     5013     +801     
- Misses        124      153      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

I'm not sure what @cpu had in mind, but adding all of this code is definitely not what I had in mind when I was reading @cpu's suggestion to minimize the scope of the code added to this crate. Instead, I think we should keep your previous commit (that stores the policy extension) and make minimal extensions that exposes that information to downstream crates. Given the state as summarized previously, I'm not sure it makes sense for this crate's maintainers to maintain all this code.

[cpu]

Instead, I think we should keep your previous commit (that stores the policy extension) and make minimal extensions that exposes that information to downstream crates.

That matches what I was thinking as well. Thanks for making it explicit 👍

[kikuomax]

The function validate_policy_tree_path has already been removed from this PR. cert_policy.rs is still huge, though.

check_certificate_policies function might be another concern. The other part is just the structural definition of Certificate Policies Extension and tests.

[djc]

I think our point is that we don't want any of this code to live in the rustls-webpki crate. As long as the caller is able to effectively get to a &[u8] representing the policy extension's contents, it seems like everything else should be able to live in other crates.

[kikuomax]

I see. Now I understand my "previous commit" means the one I made in PR #211. But it would let certificates containing malformed certificate policies extensions marked as critical pass. I think that was the reason we rejected PR#211.

[djc]

Okay, that's fair. @ctz what do you think, should we:

1. Accept parsing code like in this PR to ensure we don't ignore invalid policies
2. Continue to not process/remember policies at all
3. Some middle ground where we do high-level parsing of the policy extension, but not in too much depth
4. Some other solution, perhaps involving configuration for another optional callback/trait

[ctz]

My concern is that we continue to reject critical extensions if we don't fully and unconditionally implement the constraints they specify. That is what critical means.

So solutions acceptable to me would bind together the acceptance of critical extensions with the validation and enforcement of their contents.

But I think a better overall solution is to use one of the more general certificate verification crates, rather than extending one that is designed to be specific to the webpki.

[kikuomax]

But I think a better overall solution is to use one of the more general certificate verification crates, rather than extending one that is designed to be specific to the webpki.

@ctz Do you have any recommended crates to look into? I briefly searched for some crates with keywords like x509 and pkix:

• x509-parser:
  • does not seem to expose any method to specifically parse certificate policies extensions
  • lacks types for policy qualifiers
• x509-cert:
  • seems to expose the way to parse most of the necessary types but lacks DisplayText variants for VisibleString and BMPString which I faced in the test cases I added
• pkix:
  • seems to expose the way to parse all the necessary types
• rasn-pkix:
  • similar to pkix

[cpu]

x509-parser
...

I would be open to reviewing a PR on x509-parser adding the required methods/types. I can't promise the primary maintainer would be receptive but I suspect it fits within the vision of the crate.

[cpu]

Sorry for letting this sit for a long while.

So solutions acceptable to me would bind together the acceptance of critical extensions with the validation and enforcement of their contents.

We discussed this a bit more in Discord (conversation link) back in Feb and it sounded like we all thought the approach Go's stdlib uses was sensible. Pulling out a couple quotes from that coversation:

looks like they record all extensions, plus a set of extension OIDs that were critical but not recognised by the core library. then chain verification fails if the unhandled-but-critical set is non-empty. but that means callers can potentially act on those extensions, delete them from the unhandled set, and then do verification afterwards?

also gives people the YOLO option if they like: x509.UnhandledCriticalExtensions = nil. Here's the commit that introduced it: https://groups.google.com/g/golang-codereviews/c/RMXCTmy-4ZA

I don't think any of the core maintainers have availability to implement this strategy, but I think we would be supportive of a PR matching CONTRIBUTING.md guidelines that explored bringing this model to the webpki crate. As mentioned I'm also willing to support upstream changes in x509-parser if they would be helpful to you.

Thanks again for the contribution.