verification: add skeleton for revocation checking#14501
verification: add skeleton for revocation checking#14501
Conversation
There was a problem hiding this comment.
I'd appreciate thoughts on the general approach here with revocation checker ownership. I assume that we eventually want to instantiate RevocationChecker for OCSP, so I patterned everything after the existing Subject type. It feels a little bit clunky.
There was a problem hiding this comment.
It's not clear to me that the RevocationCheckerOwner is actually doing any work here? It only ever lives on the stack AFAICT, which suggests it shouldn't be needed.
There was a problem hiding this comment.
Maybe there's a better name for it, but RevocationCheckerOwner helps us by holding the concrete PyCrlRevocationChecker, which we then can borrow a CrlRevocationChecker from to pass to cryptography-x509-verification. Another way we could do this is by holding a plain PyCrlRevocationChecker, but we'd have to figure something out in the future to accept multiple RevocationChecker impls. I originally wanted to directly borrow a CrlRevocationChecker out of the PyAny but couldn't find a way to; I'm not very handy with pyo3 😅
There was a problem hiding this comment.
Scratch the above, I had an idea that makes everything simpler and allows for custom Python revocation checkers :)
| class RevocationChecker(metaclass=abc.ABCMeta): | ||
| """ | ||
| An interface for revocation checkers. | ||
| """ |
There was a problem hiding this comment.
So right now the idea is that we won't actually have a Python API you can implement for revocation checking, it'll be handled internally and these are more markers, is that the idea?
There was a problem hiding this comment.
Correct, sorry that this isn't more clear—I experimented with having an extensible Python API but it was a bit complicated to shim over considering that the implementation for CRL is going to be in Rust. I can sketch out a shim if you'd like? see latest diff which supports Python-extensible revocation checkers
There was a problem hiding this comment.
It's not clear to me that the RevocationCheckerOwner is actually doing any work here? It only ever lives on the stack AFAICT, which suggests it shouldn't be needed.
There was a problem hiding this comment.
The idea here is to have CrlRevocationChecker and PyRevocationChecker both implement the trait that is used in the verification crate. PyRevocationChecker is a marker class that implements the CheckRevocation trait and dispatches to the underlying Python method for Python revocation checker implementations.
| @abc.abstractmethod | ||
| def is_revoked( | ||
| self, | ||
| leaf: rust_x509.Certificate, | ||
| issuer: rust_x509.Certificate, | ||
| policy: Policy, | ||
| ) -> bool | None: | ||
| """ | ||
| Returns whether the certificate is revoked. If the revocation status | ||
| cannot be determined, the revocation checker may return None. | ||
| """ |
There was a problem hiding this comment.
This signature is a bit subtle. There are situations in which a revocation checker cannot determine the revocation status for a given certificate: for the CRL checker, the checker might not contain a relevant CRL; and for OCSP, the responder may be offline. I figure that it's best to signal this instead of collapsing that error state into "not revoked".
| CRLRevocationChecker = rust_x509.CRLRevocationChecker | ||
|
|
||
|
|
||
| class RevocationChecker(rust_x509.RevocationChecker, metaclass=abc.ABCMeta): |
There was a problem hiding this comment.
note that we now have an ABC that inherits from the Rust class that implements the CheckRevocation trait
|
What would it be useful to review for? Do we have x509-limbo test cases for this yet? |
@alex I'd like your eyes on the approach wrt. bridging Python revocation checkers, just to make sure that I'm not totally off base. We do have a few Limbo tests for CRLs already (C2SP/x509-limbo#538), but I'm working on a few more and will get them up to cover the meat of the |
|
I briefly skimmed, and I think it'd be helpful if you wrote a few paragraphs/examples of what the design/goals are. Right now it's very unclear which parts of the inheritance/ABCs/etc. are incidental vs. the intended design. |
|
@alex Sure, so the whole implementation here is predicated on a couple assumptions, namely that we'd like to (1) implement the revocation checker for CRLs in Rust while (2) allowing for custom revocation checker implementations from Python. Let me know if we don't need the extensibility on the Python end, it'd allow us to get rid of a lot of this infrastructure. I started off by defining a revocation checker trait // `::new` accepts an iterator of `CertificateRevocationList`
let checker = CrlRevocationChecker::new(&crls);
cryptography_x509_verification::verify(&leaf, &intermediates, &policy, &checker, &store);At this point, we have to figure out how to expose builder = PolicyBuilder().store(store).time(validation_time)
builder = builder.revocation_checker(CRLRevocationChecker([crl]))
verifier = builder.build_client_verifier()The question then becomes, what should the signature of fn revocation_checker(
&self,
py: pyo3::Python<'_>,
revocation_checker: pyo3::Py<PyRevocationChecker>,
) -> CryptographyResult<PolicyBuilder>This is good enough for the Rust-implemented revocation checkers, but we also want Python extensibility. Starting with the user-facing API, we want to support passing Python-implemented checkers to The question now becomes, how do we bridge the Python ABC and the Rust Now, from the perspective of the Sorry for all the complexity, hopefully this clears things up a bit. Let me know if there's anything I can clarify. |
This is an outline of the CRL revocation API that was discussed in #10393. I'd like to get some feedback on the general approach before we go further and fill in the revocation algorithm.