Skip to content

verification: add skeleton for revocation checking#14501

Open
tnytown wants to merge 16 commits intopyca:mainfrom
trail-of-forks:tnytown/crl-skeleton
Open

verification: add skeleton for revocation checking#14501
tnytown wants to merge 16 commits intopyca:mainfrom
trail-of-forks:tnytown/crl-skeleton

Conversation

@tnytown
Copy link
Copy Markdown
Contributor

@tnytown tnytown commented Mar 18, 2026

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@tnytown tnytown Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Copy Markdown
Contributor Author

@tnytown tnytown Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@tnytown tnytown Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@tnytown tnytown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex this is ready for another round of review :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +46 to +56
@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.
"""
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that we now have an ABC that inherits from the Rust class that implements the CheckRevocation trait

@alex
Copy link
Copy Markdown
Member

alex commented Mar 24, 2026

What would it be useful to review for? Do we have x509-limbo test cases for this yet?

@tnytown
Copy link
Copy Markdown
Contributor Author

tnytown commented Mar 24, 2026

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 CRLRevocationChecker.

@alex
Copy link
Copy Markdown
Member

alex commented Mar 24, 2026

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.

@tnytown
Copy link
Copy Markdown
Contributor Author

tnytown commented Mar 27, 2026

@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 CheckRevocation in cryptography-x509-verification. This is the basic interface that the path building logic uses, and instances of it are provided to the public API via a new argument in cryptography_x509_verification::verify. The CRL revocation checker CrlRevocationChecker also lives in cryptography-x509-verification and implements CheckRevocation. We can implement other (OCSP?) revocation checkers in Rust in the future with the same basic pattern, they just need to implement CheckRevocation. Constructing a CRL revocation checker from Rust and using it with verification is pretty straightforward, to wit:

// `::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 CrlRevocationChecker to Python and make it constructible as a Python class. Luckily, this was done before with PyStore, so I just constructed a PyCrlRevocationChecker patterned off of that API. The new API PolicyBuilder.revocation_checker accepts instances of these, so we can construct a verifier with a revocation checker like so:

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 PolicyBuilder.revocation_checker be? If we want to allow for further instantiations like an OCSP revocation checker, we can't just accept PyCrlRevocationChecker. I decided to make a marker class, PyRevocationChecker, that PyCrlRevocationChecker inherits from. This allows us to be stricter in the signature of revocation_checker, which pyo3 enforces:

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 PolicyBuilder.revocation_checker, just like Rust-implemented checkers. I started with a RevocationChecker ABC that roughly mirrors the CheckRevocation trait. This ABC inherits from our PyCrlRevocationChecker marker class, which allows PolicyBuilder.revocation_checker to take instances of it.

The question now becomes, how do we bridge the Python ABC and the Rust CheckRevocation trait? Unfortunately, PyO3 doesn't do this for us yet. The solution I settled on was implementing the CheckRevocation trait on Py<PyRevocationChecker>. This allows us to call the concrete is_revoked method implemented in Python with call_method1 from Rust.

Now, from the perspective of the cryptography-rust bindings, we can get a CheckRevocation trait object by trying to cast to PyCrlRevocationChecker and falling through to the implementation on Py<PyRevocationChecker>. This is what build_rust_revocation_checker does. If we wanted to separate things out more, I could add another pyclass for the ABC to subclass from instead of having it subclass from the PyRevocationChecker.

Sorry for all the complexity, hopefully this clears things up a bit. Let me know if there's anything I can clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants