Expose builder-like interface for path verification#486
Conversation
| // Use `'a` for lifetimes that we don't care about, `'p` for lifetimes that become a part of | ||
| // the `VerifiedPath`. | ||
| pub(crate) struct ChainOptions<'a, 'p> { | ||
| pub(crate) struct PathBuilder<'a, 'p> { |
There was a problem hiding this comment.
Not sure this is the best name? I wanted to evoke "path building" but maybe that's too jargony and it would be better to stick with "verification" or "verifier" language.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
==========================================
+ Coverage 96.97% 97.01% +0.03%
==========================================
Files 20 20
Lines 3936 3950 +14
==========================================
+ Hits 3817 3832 +15
+ Misses 119 118 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
LGTM! |
briansmith
left a comment
briansmith
left a comment
There was a problem hiding this comment.
I think that this is a good and necessary thing to do. But, I think that the new design now errs too far on making it too easy for the user to skip adding universally-essential inputs prior to calling build. It matters less now that there isn't a way to do things like AIA-based intermediate fetching, but if/when extension points are added that would allow that in the future (e.g. to support things like Firefox's preloaded intermediate allowlist), an application could easily end up "working" during testing but fail in usual cases.
| /// public key is not validated against this list. | ||
| /// * `trust_anchors` is the list of root CAs to trust in the built path. | ||
| pub fn new( | ||
| eku: &'p dyn ExtendedKeyUsageValidator, |
There was a problem hiding this comment.
ExtendedKeyUsageValidator is not a sealed trait so this supports arbitrary user-implemented EKU matching, including EKU matching that is totally broken (no-op), right?
If it is necessary to have such custom functionality, perhaps there could be a separate not_for_webpki() constructor that takes a &dyn ExtendedKeyUsageValidator, while having new take a concrete WebPKI-relevant OID for eku.
(Also, KeyUsage is a misleading name for the library-provided implementation of this type, because in X.509 KeyUsage is a different thing.)
| /// Set the sequence of intermediate certificates to use for path building. | ||
| /// | ||
| /// These should be sent by the peer. Defaults to empty. | ||
| pub fn with_intermediate_certs(mut self, intermediate_certs: &'p [CertificateDer<'p>]) -> Self { |
There was a problem hiding this comment.
In almost any real-world WebPKI implementation, failing to supply intermediate certificates as input into the validation is going to result in bad behavior, so IMO the intermediate certificates should be parameters to new or there should be some other mechanism to statically ensure that this step isn't skipped.
| /// | ||
| /// By default, revocation checking is disabled. | ||
| pub fn with_revocation(mut self, revocation: RevocationOptions<'a>) -> Self { | ||
| self.revocation = Some(revocation); |
There was a problem hiding this comment.
Every X.509 application has to make an explicit decision on what to do about revocation, so again, I think there should be some mechanism to ensure this isn't skipped.
3 participants
Avoids the many-argument
verify_for_usage()complexity bottleneck in favor of exposing the underlyingChainOptionsdirectly (under a different name). Should make it easier to evolve this API going forward (as suggested in #485).