Improve CRL ergonomics, replace trait with enum #203
Conversation
Codecov Report
@@ Coverage Diff @@
## main #203 +/- ##
==========================================
+ Coverage 96.73% 96.77% +0.03%
==========================================
Files 20 20
Lines 4749 4776 +27
==========================================
+ Hits 4594 4622 +28
+ Misses 155 154 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cpu
changed the title
I think it would be possible to make an alternative to That seems a little bit cleaner, but we're still left having to make sure we don't use the WDYT? |
Hmm, yeah, doesn't seem like a big win. How sure are we of the utility of the public |
Yeah, that was the feeling I had too so I left it out of this branch initially.
Thinking on it more I suppose it's a pretty weak use-case. The rustls side isn't using it at all. We use it in I could make the SPKI part more ergonomic but I think in sum it's not worthwhile to have this as part of the public API. I think the choice to put it in the API originally was more an accident around having to use a trait to make it work with both kinds of CRLs, and the trait requiring the fn be pub. I'll go ahead and remove the |
Yup! Can always add something back later if a need comes up, but feels like we can err on the side of YAGNI here. |
Previously the `RevocationOptionsBuilder` type exposed mutation of its associated `UnknownStatusPolicy` using `allow_unknown_status(mut self)` or `forbid_unknown_status(mut self)` fns , but didn't offer a way to directly provide a `UnknownStatusPolicy` variant. Since the `UnknownStatusPolicy` enum is exposed in the public API it's more convenient for downstream users to be able to provide it directly. E.g. In Rustls we have a client verifier builder that wants to support its own `webpki::UnknownStatusPolicy` as part of a broader implementation for client certificate validation. We don't want to have to match one of the variants to decide which builder fn to call when we could just give the builder the enum directly. This commit replaces the existing fns with a new `with_status_policy(mut self, policy::UnknownStatusPolicy)` replacement that should be more ergonomic downstream.
Previously we used a sealed trait to represent a `CertRevocationList` in the general sense, implementing the trait for the concrete `BorrowedCertRevocationList` and `OwnedCertRevocationList` types. This allowed abstracting certain operations over a CRL (e.g. looking up a given serial number) so that the owned representation could handle this in a more performant way than the borrowed representation allowed. In practice, this way of representing the commonality between the owned and borrowed CRL representation had some downsides: 1. We couldn't easily associate common functionality to both CRL types without making it part of the `pub` trait interface, leading to free-standing functions like `crl_authoritative`. 2. Downstream, when holding a `Vec<OwnedCertRevocationList>` the caller had to do some awkward gymnastics to create a `&[&dyn CertRevocationList]`. Each value had to be converted individual with an explicit `as` cast[0]. 3. We had to seal the trait, since we didn't want external types implementing it. This commit replaces the `CertRevocationList` trait with an enum, having `Owned` and `Borrowed` variants (with the `Owned` variant and associated bits behind the `alloc` cfg flag). This new representation addresses the downsides we've encountered over time while still meeting our requirements for the webpki crate. [0]: https://users.rust-lang.org/t/how-to-pass-a-vec-t-as-dyn-trait/92274/4
Previously we had to use a free-standing crate-internal `crl_authoritative` fn for deciding if a CRL was authoritative for a given pathnode certificate to avoid adding the functionality to the `CertRevocationList` trait where it would become part of the public API. Now that we have an `enum` representation of the `CertRevocationList` we can add a `pub(crate)` fn to the type directly where it fits more naturally than a free-standing fn accepting a CRL as the first arg. This commit implements this and moves the associated unit tests.
Previously it was only possible to build an `OwnedCertRevocationList` by first parsing a `BorrowedCertRevocationList` and then calling `to_owned`. This is convenient if you already have a `BorrowedCertRevocationList`, but downstream consumers like Rustls may want to construct a `OwnedCertRevocationList` directly. This commit adds a new `OwnedCertRevocationList::from_der` fn that can perform the construction in one step, avoiding the caller having to deal with two `Result`'s (one from creating the `BorrowedCertRevocationList` and then one from calling `to_owned`).
Previously the `verify_signature` fn was part of the `CertRevocationList` trait, and so part of the public API. This meant we couldn't accept an `untrusted::Input` argument, or more importantly, a `Budget` to use to indicate we've consumed a signature validation operation. Now that the `CertRevocationList` type is an enum we can easily make this fn crate-internal. It doesn't seem especially valuable to external users, especially given that it's somewhat nuanced/cumbersome to build the right SPKI representation to use for validation outside of webpki. This commit makes the fn crate internal, adds the `Budget` argument, reworks the SPKI to be an `untrusted::Input` and folds-in the `signed_data` crate-internal fn since it wasn't being used anywhere except in `verify_signature`. The net result is that it's not impossible to verify a CRL signature without providing a budget to use. In the path building context in `verify_cert.rs` we pass through the budget we use for the overall accounting during path building.
cpu
force-pushed
the
cpu-crl-ergonomics
branch
from
4759083
to
7e5b62d
Compare
Done. I also added a commit incrementing the alpha version so that once this is merged I can publish that and use it in Rustls. |
|
@ctz Is this something you want to give a once-over? |
|
Thanks! |
|
While expanding the Rustls webpki verifier CRL support we found the code required to work with CRLs was becoming awkward e.g. see this review comment. This branch attempts to make things easier through a few changes to the existing CRL code. See rustls/rustls#1552 for a draft PR demonstrating how these changes end up making the Rustls code less convoluted.
See individual commit messages for more information.