Basic support for multiple SINGLERESP messages in one OCSP response, take 2 #6410

turettn · 2021-10-12T03:25:06Z

Second attempt at #6320, this time in Rust.

#5124 identified an issue where OCSP responses with multiple certificate blocks were rejected violently. #5316 cleaned this up with an intelligent error message.

Motivation-wise, there's at least one commonly used OCSP server that pre-generates OCSP responses in batches of 20, then when it receives a OCSP request it simply finds the correct response file and fires it back. These servers seem to be mostly used in environments with extremely large client certificate roll-outs (e.g. the NIPRNet, where every user has a physical token with a client certificate). On these networks, servers are constantly checking client certificate validity due to the high rate of revocations (e.g. cards are lost\stolen, eligibility for access changes, etc...). The original issue referenced above provided the ocsp-army.deps.mil-resp.der test vector, which appears to have come from one such network.

This patch does something very similar to what we had discussed in #6320. We had discussed trying to expose the individual single-response objects via an iterable to Python, but went down the rabbit-hole of lifetimes, and discovered that I clearly don't understand them as well as I should. Additionally, this (and most of things I could try that actually compiled) broke backward-compatibility with the existing API, which I was trying really hard to avoid.

I am by no means a Rust expert - if you can suggest a way to clean any of this up, I'm happy to submit follow up revisions.

Thanks for taking a look!

reaperhulk · 2021-10-16T23:45:23Z

I think we do need to figure out the problems you were seeing with the other approach because an iterable is the API we want here. If you have a gist of the code you were attempting to get working @alex and I can take a look.

turettn · 2021-10-17T14:24:06Z

I'll try to get something as clean as possible pushed in the next day or two.

turettn · 2021-10-25T22:02:27Z

Sorry for the delay - I haven't had much time to devote to this.

There were enough changes on main I ended up just starting again from origin/main.

In my perfect world, I would move move all the getter methods on OCSPResponse that refer just grab fields from SingleResponse (e.g. serial_number, certificate_status) onto SingleResponse, replace the OCSPResponse methods with pass-throughs for the "only one SINGLERESP" case, and then return a list of SingleRespones for OCSPResponse.responses.

If I try to make SingleResponse a pyclass, I get #[pyclass] cannot have generic paramters.

I can implement the IntoPy trait for that type, but have no idea what type to wrap it as. Any suggestions? Right now I'm just returning a placeholder since I can't figure out how to make SingleResponse Python-accessible.

alex · 2021-10-26T22:41:04Z

I suspect the right thing to look at is CRLs, which model basically this pattern of one structure containing a bunch of sub-structures from the ASN.1

…

On Mon, Oct 25, 2021 at 6:02 PM turettn ***@***.***> wrote: Sorry for the delay - I haven't had much time to devote to this. There were enough changes on main I ended up just starting again from origin/main. In my perfect world, I would move move all the getter methods on OCSPResponse that refer just grab fields from SingleResponse (e.g. serial_number, certificate_status) onto SingleResponse, replace the OCSPResponse methods with pass-throughs for the "only one SINGLERESP" case, and then return a list of SingleRespones for OCSPResponse.responses. If I try to make SingleResponse a pyclass, I get #[pyclass] cannot have generic paramters. I can implement the IntoPy trait for that type, but have no idea what type to wrap it as. Any suggestions? Right now I'm just returning a placeholder since I can't figure out how to make SingleResponse Python-accessible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBBC7HFDL436RQT2VDLUIXHX5ANCNFSM5FZRGKUA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- All that is necessary for evil to succeed is for good people to do nothing.

turettn · 2021-11-29T04:40:25Z

@alex Thanks for the pointer - it got me going down the right path.

I think this is the iterator API you wanted.

Everything passes CI, coverage looks good, etc... The only thing I can't figure out right now is getting the docs to build.

It complains that:

Warning, treated as error:
/home/neal/src/cryptography/docs/x509/ocsp.rst:628:py:class reference target not found: cryptography.x509.ocsp.OCSPResponseIterator

except I can't figure out why it thinks that:

(venv) neal@DESKTOP-S8J07MC:~/src/cryptography$ python3
Python 3.8.12 (default, Sep 10 2021, 00:16:05)
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import cryptography.x509.ocsp
>>> cryptography.x509.ocsp.OCSPResponseIterator
<class 'cryptography.x509.ocsp.OCSPResponseIterator'>
>>>

If you have any suggestions about the docs warning, or have any other thoughts about the code, please let me know.

alex

It's getting close! Thanks for continuing to work on this.

docs/x509/ocsp.rst

alex · 2021-11-30T12:55:04Z

src/rust/src/x509/ocsp_resp.rs

+#[pyo3::prelude::pyclass]
+struct OCSPResponseIterator {
+    contents: OwnedRawOCSPResponse,
+    cur_pos: usize,


Rather than indices, can we use the iterator, as seen in CRLs? That'll avoid the usize::MAX stuff as well as the O(n^2) behavior.

alex · 2021-11-30T12:55:34Z

src/cryptography/x509/ocsp.py

@@ -163,7 +163,73 @@ def extensions(self) -> x509.Extensions:
        """


+class OCSPResponseIterator(metaclass=abc.ABCMeta):


This type isn't actually an iterator, this seems to be a single response and then response_iter returns a typing.Iterator[ThisType]?

It's an iterator, in that it implements __next__ and __iter__. The root of all my pain is that my understanding of PyO3 is that I have to return a pyclass object, which can't contain any lifetimes. This means I can't just return SingleResponse<'_>, or turn it into a pyclass. To solve this problem, I have the iterator object with the index on it. Each time you call __next__, it increments the index, then when you call the property methods (e.g. serial_number), it burrows into contents, finds the appropriate SingleResponse, extracts the requested field in a way Python can handle, and returns it.

If I understand what you're suggesting, you want next do something like:

let next_resp = slf .contents .borrow_basic_response() .as_ref() .unwrap() .tbs_response_data .responses .unwrap_read() .next() .unwrap();

Then use next_resp to create a new pyclass object. I can get close to this, since the __next__ method has slf.py() to get the pyo3::Python object, but when I try to advance...tbs_response_data.responses it gives me an error about being unable to borrow the result of unwrap_read() as mutable.

If you have any advice on how to get tbs_response_data.responses into a state where I can next() through it, that would be great.

The ABC doesn't contain __next__, that's what's confusing me. This seems to be an ABC for SingleResponse.

docs/x509/ocsp.rst

alex · 2021-12-04T14:53:51Z

src/cryptography/x509/ocsp.py

@@ -163,7 +163,73 @@ def extensions(self) -> x509.Extensions:
        """


+class OCSPResponseIterator(metaclass=abc.ABCMeta):


The ABC doesn't contain __next__, that's what's confusing me. This seems to be an ABC for SingleResponse.

src/cryptography/x509/ocsp.py

src/rust/src/x509/ocsp_resp.rs

turettn · 2021-12-09T02:53:11Z

@alex Regarding class OCSPResponseIterator(metaclass=abc.ABCMeta):,

the right answer seems to involve collections.abc.Iterable. Now that I'm staring at the committed code, I'm wondering if it should be a normal super-class, not a metaclass. Thoughts?

alex · 2021-12-12T04:26:45Z

Why is the design to have an iterator where __next__ returns self? That's not the typical design for iterators, usually __next__ returns an instance of some other type.

turettn · 2021-12-12T04:43:41Z

The mutating iterator was the way I found that avoided a pyclass object with lifetimes, while also avoided a truly ridiculous amount of cloning.

The last change I made clearly broke CI badly - I assume this is something to do with the metaclass vs just subclassing. I'm on the road for a couple days and will fix it once I'm back.

alex · 2021-12-12T04:46:31Z

You may want to take a look at crl.rs, which has an iterator in Rust that yields that from an ASN.1 structure. It should be the exact same pattern as is required here.

…

On Sat, Dec 11, 2021 at 11:43 PM turettn ***@***.***> wrote: The mutating iterator was the way I found that avoided a pyclass object with lifetimes, while also avoided a truly ridiculous amount of cloning. The last change I made clearly broke CI badly - I assume this is something to do with the metaclass vs just subclassing. I'm on the road for a couple days and will fix it once I'm back. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBFWPBEYOVSB5SLF353UQQSAPANCNFSM5FZRGKUA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- All that is necessary for evil to succeed is for good people to do nothing.

alex

Very very close! Thanks for your work on this, and sorry I've been so slow to review.

docs/x509/ocsp.rst

src/cryptography/x509/ocsp.py

alex · 2021-12-29T01:45:35Z

Oh, please add an entry to the CHANGELOG for this.

Instead of throwing an exception when encountering a OCSP response with multiple SINGLERESPs, throw the exception when attempting to pull a single structure if multiple are present. Add a response_iter property to the OCSP Response object, which allows for iteration through all the SINGLERESPs, and properties to be individually accessed for each.

turettn · 2022-01-03T03:11:59Z

@alex No problem - it's that time of year. I made the requested changes. Let me know if it needs anything else.

alex · 2022-01-03T21:31:39Z

LGTM! Ping'd @reaperhulk to see if he wants to take a look before I merge.

Thanks for all your work on this!

alex reviewed Nov 30, 2021

View reviewed changes

alex reviewed Dec 4, 2021

View reviewed changes

reaperhulk added this to the Thirty Seventh Release milestone Dec 23, 2021

alex reviewed Dec 29, 2021

View reviewed changes

docs/x509/ocsp.rst Outdated Show resolved Hide resolved

src/cryptography/x509/ocsp.py Outdated Show resolved Hide resolved

src/cryptography/x509/ocsp.py Outdated Show resolved Hide resolved

turettn added 6 commits January 2, 2022 20:00

Small fixes to OCSP multiple response handling. Squash eventually

55186b5

Docs updates. Should be squashed

d61285e

Get rid of mutating iterator, replace with separate return object

20b9d4f

Fix versionadded strings in docs

0c29ff5

Cleanup docs & python bindings.

c6a715d

alex approved these changes Jan 3, 2022

View reviewed changes

reaperhulk approved these changes Jan 3, 2022

View reviewed changes

alex merged commit 1249776 into pyca:main Jan 3, 2022

h-vetinari mentioned this pull request Apr 3, 2022

Rebuild for PyPy3.8 and PyPy3.9 conda-forge/cryptography-feedstock#92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for multiple SINGLERESP messages in one OCSP response, take 2 #6410

Basic support for multiple SINGLERESP messages in one OCSP response, take 2 #6410

turettn commented Oct 12, 2021

reaperhulk commented Oct 16, 2021

turettn commented Oct 17, 2021

turettn commented Oct 25, 2021

alex commented Oct 26, 2021 via email

turettn commented Nov 29, 2021

alex left a comment

alex Nov 30, 2021

alex Nov 30, 2021

turettn Dec 3, 2021

alex Dec 4, 2021

alex Dec 4, 2021

turettn commented Dec 9, 2021

alex commented Dec 12, 2021

turettn commented Dec 12, 2021

alex commented Dec 12, 2021 via email

alex left a comment

alex commented Dec 29, 2021

turettn commented Jan 3, 2022

alex commented Jan 3, 2022

		@@ -163,7 +163,73 @@ def extensions(self) -> x509.Extensions:
		"""


		class OCSPResponseIterator(metaclass=abc.ABCMeta):

Basic support for multiple SINGLERESP messages in one OCSP response, take 2 #6410

Basic support for multiple SINGLERESP messages in one OCSP response, take 2 #6410

Conversation

turettn commented Oct 12, 2021

reaperhulk commented Oct 16, 2021

turettn commented Oct 17, 2021

turettn commented Oct 25, 2021

alex commented Oct 26, 2021 via email

turettn commented Nov 29, 2021

alex left a comment

Choose a reason for hiding this comment

alex Nov 30, 2021

Choose a reason for hiding this comment

alex Nov 30, 2021

Choose a reason for hiding this comment

turettn Dec 3, 2021

Choose a reason for hiding this comment

alex Dec 4, 2021

Choose a reason for hiding this comment

alex Dec 4, 2021

Choose a reason for hiding this comment

turettn commented Dec 9, 2021

alex commented Dec 12, 2021

turettn commented Dec 12, 2021

alex commented Dec 12, 2021 via email

alex left a comment

Choose a reason for hiding this comment

alex commented Dec 29, 2021

turettn commented Jan 3, 2022

alex commented Jan 3, 2022