certificate: add a `get_extension` helper #8892

woodruffw · 2023-05-08T18:07:56Z

Breakout from #8873.

Needs tests, but just to get something up for review.

Signed-off-by: William Woodruff <[email protected]>

src/rust/cryptography-x509/src/certificate.rs

Signed-off-by: William Woodruff <[email protected]>

src/rust/cryptography-x509/src/certificate.rs

Signed-off-by: William Woodruff <[email protected]>

woodruffw · 2023-05-08T22:45:09Z

Should be good to go now, modulo coverage -- I've rewritten existing Extensions usage into RawExtensions (and added a raw_ prefix as appropriate), with the new Extensions type being a duplicate-checking wrapper.

The existing exception behavior is preserved by having the Err(...) variant carry the duplicate OID.

Signed-off-by: William Woodruff <[email protected]>

alex · 2023-05-08T23:05:25Z

src/rust/cryptography-x509/src/certificate.rs

@@ -13,6 +14,12 @@ pub struct Certificate<'a> {
    pub signature: asn1::BitString<'a>,
 }

+impl<'a> Certificate<'a> {
+    pub fn extensions(&'a self) -> Result<Option<Extensions<'a>>, asn1::ObjectIdentifier> {
+        self.tbs_cert.extensions()


Not sure it's worth it to have the method on TBS, is there a use case?

That was mostly for tbs_precertificate_bytes, since we need TbcCertificate.extensions() in that context to perform the poison extension filtering. But maybe there's another way I should be getting the extensions there?

~~Ah yep, I can get rid of this entirely.~~

Edit: Nope, I don't think I can directly get rid of this -- the TBS precert should have its extensions checked like the others but it doesn't go through the caching layer here (since all we're doing is munging the cert).

src/rust/cryptography-x509/src/certificate.rs

src/rust/src/x509/certificate.rs

alex · 2023-05-08T23:09:44Z

src/rust/src/x509/crl.rs

+        let extensions = match tbs_cert_list.extensions() {
+            Ok(exts) => exts,
+            Err(oid) => {
+                let oid_obj = oid_to_py_oid(py, &oid)?;
+                return Err(exceptions::DuplicateExtension::new_err((
+                    format!("Duplicate {} extension found", oid),
+                    oid_obj.into_py(py),
+                )));
+            }
+        };
+


This shouldn't be repeated in every extensions method, it should be in parse_and_cache_extensions which can take the result of calling extensions() on the cert.

Making sure I understand: in that case, parse_and_cache_extensions should take a Result<Option<Extensions>>? Because that'll be the return type from extensions().

(Or are you thinking it should take the RawExtensions, and then convert it to an Extensions internally? That would avoid passing a potential Err variant through as a parameter.)

I've done this with RawExtensions; the error handling is now pushed fully into parse_and_cache_extensions.

Signed-off-by: William Woodruff <[email protected]>

alex · 2023-05-09T03:02:47Z

clippy

Signed-off-by: William Woodruff <[email protected]>

woodruffw · 2023-05-09T15:07:24Z

GitHub is struggling today, but I've added some coverage for the new get_extension helper.

Signed-off-by: William Woodruff <[email protected]>

woodruffw · 2023-05-09T15:47:06Z

Remaining coverage items:

tbs_precertificate_bytes needs a testcase for duplicate extensions (these weren't previously handled)
~~- [ ] Certificate::extensions needs a testcase~~

Signed-off-by: William Woodruff <[email protected]>

woodruffw · 2023-05-09T19:53:18Z

This should be fully good to go: I've removed Certificate::extensions in favor of an extensional trait in that path validation library being scaffolded in #8873, which means we can punt on coverage.

woodruffw · 2023-05-09T20:02:02Z

src/rust/cryptography-x509/src/extensions.rs

+        raw: Option<&RawExtensions<'a>>,
+    ) -> Result<Option<Self>, asn1::ObjectIdentifier> {


I'm not super happy with this signature -- we end up pushing an Option through as a no-op to avoid having to destructure the optional extensions in multiple other places, but that results in more invalid states than are strictly needed here (and the smelly Result<Option<...>> type).

I think a reasonable follow up is, instead of as_raw(), we make Extensions into an iterator, and just handle the Option<> internally. But let's do that in a seperate PR, this one is already complicated enough.

Signed-off-by: William Woodruff <[email protected]>

alex · 2023-05-09T22:43:39Z

src/rust/cryptography-x509/src/extensions.rs

+        raw: Option<&RawExtensions<'a>>,
+    ) -> Result<Option<Self>, asn1::ObjectIdentifier> {


I think a reasonable follow up is, instead of as_raw(), we make Extensions into an iterator, and just handle the Option<> internally. But let's do that in a seperate PR, this one is already complicated enough.

alex · 2023-05-09T22:45:14Z

src/rust/src/x509/certificate.rs

+            Err(oid) => {
+                let oid_obj = oid_to_py_oid(py, &oid)?;
+                Err(exceptions::DuplicateExtension::new_err((
+                    format!("Duplicate {} extension found", oid),
+                    oid_obj.into_py(py),
+                ))
+                .into())
+            }


To avoid needing to repeat this stanza anywhere we touch extensions, can we make extensions return a more-specific error type and then we can do a From impl on that?

Yeah, good idea -- I'll do that now.

212b75f is my attempt at this -- it saves us the repetition of this error clause in exchange for a new error enum + From implementation.

alex · 2023-05-09T22:47:24Z

src/rust/src/x509/common.rs

@@ -391,27 +390,30 @@ pub(crate) fn parse_and_cache_extensions<
 >(
    py: pyo3::Python<'p>,
    cached_extensions: &mut Option<pyo3::PyObject>,
-    raw_exts: &Option<Extensions<'_>>,
+    raw_extensions: &Option<RawExtensions<'_>>,


I think we actually want this method to take an Option<Extensions> and then the callers can obtain the extensions, via an extensions() method on the various cryptography-x509 types. That way those types have the right methods that can be used from the validator crate.

That SGTM, although if I understand correctly doing that will require us to make the Iterator change mentioned in #8892 (comment): without that change, we'd need to perform the fallible conversion from RawExtensions -> Extensions before parse_and_cache_extensions, defeating the point of having it inside of that function. Or did I miss something?

Ok, let's leave this change for a seperate PR.

Signed-off-by: William Woodruff <[email protected]> extensions: unwrap -> expect Signed-off-by: William Woodruff <[email protected]>

This reverts commit 212b75f.

certificate: add a get_extension helper

351af50

Signed-off-by: William Woodruff <[email protected]>

woodruffw commented May 8, 2023

View reviewed changes

src/rust/cryptography-x509/src/certificate.rs Outdated Show resolved Hide resolved

alex reviewed May 8, 2023

View reviewed changes

src/rust/cryptography-x509/src/certificate.rs Outdated Show resolved Hide resolved

alex reviewed May 8, 2023

View reviewed changes

src/rust/cryptography-x509/src/certificate.rs Outdated Show resolved Hide resolved

woodruffw added 3 commits May 8, 2023 14:15

certificate: OID by ref

302eb18

Signed-off-by: William Woodruff <[email protected]>

certificate: syntax

3cd1a7d

Signed-off-by: William Woodruff <[email protected]>

x509, src: check_duplicate_extensions

c37e66f

Signed-off-by: William Woodruff <[email protected]>

woodruffw commented May 8, 2023

View reviewed changes

src/rust/cryptography-x509/src/certificate.rs Outdated Show resolved Hide resolved

woodruffw added 5 commits May 8, 2023 15:00

src: simplify

326634c

Signed-off-by: William Woodruff <[email protected]>

src: everyone loves newtypes

a900f6b

Signed-off-by: William Woodruff <[email protected]>

rust: refactor-o-rama

4c59f62

Signed-off-by: William Woodruff <[email protected]>

src: look upon my works

b393f1b

Signed-off-by: William Woodruff <[email protected]>

src: continue blasting the code

07e7cee

Signed-off-by: William Woodruff <[email protected]>

woodruffw added 2 commits May 8, 2023 18:47

src/rust: actually commit my changes

897161e

Signed-off-by: William Woodruff <[email protected]>

src: clippage

22b41c5

Signed-off-by: William Woodruff <[email protected]>

alex reviewed May 8, 2023

View reviewed changes

woodruffw added 8 commits May 8, 2023 19:21

relocate

0ce6afb

Signed-off-by: William Woodruff <[email protected]>

src: dedupe

b2a9e07

Signed-off-by: William Woodruff <[email protected]>

src: cleanup

2811cc8

Signed-off-by: William Woodruff <[email protected]>

clippage

e293f9c

Signed-off-by: William Woodruff <[email protected]>

src: dedupe

0893194

Signed-off-by: William Woodruff <[email protected]>

common: cleanup

2dbb3eb

Signed-off-by: William Woodruff <[email protected]>

src: unused impls

aa924dd

Signed-off-by: William Woodruff <[email protected]>

more deletion

5189ca7

Signed-off-by: William Woodruff <[email protected]>

woodruffw added 2 commits May 9, 2023 10:47

clippage

e9d8e9f

Signed-off-by: William Woodruff <[email protected]>

extensions: add a get_extension test

8ec095e

Signed-off-by: William Woodruff <[email protected]>

extensions: unused derives

e8ef3b7

Signed-off-by: William Woodruff <[email protected]>

woodruffw added 3 commits May 9, 2023 12:13

tests/x509: dup ext check for tbs_precertificate_bytes

ea5da30

Signed-off-by: William Woodruff <[email protected]>

Merge remote-tracking branch 'upstream/main' into tob-x509-get-extension

b9c14ac

certificate: remove extensions()

f6f0367

Signed-off-by: William Woodruff <[email protected]>

woodruffw commented May 9, 2023

View reviewed changes

woodruffw added 2 commits May 9, 2023 16:21

extensions: docs

fb585cf

Signed-off-by: William Woodruff <[email protected]>

extensions: newtype

cdd65d4

Signed-off-by: William Woodruff <[email protected]>

alex reviewed May 9, 2023

View reviewed changes

woodruffw added 2 commits May 9, 2023 23:46

rust: better error types, dedupe

212b75f

Signed-off-by: William Woodruff <[email protected]> extensions: unwrap -> expect Signed-off-by: William Woodruff <[email protected]>

Revert "rust: better error types, dedupe"

e2bb769

This reverts commit 212b75f.

alex approved these changes May 10, 2023

View reviewed changes

alex merged commit 1ff6208 into pyca:main May 10, 2023

woodruffw deleted the tob-x509-get-extension branch May 10, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

certificate: add a `get_extension` helper #8892

certificate: add a `get_extension` helper #8892

woodruffw commented May 8, 2023

woodruffw commented May 8, 2023

alex May 8, 2023

woodruffw May 8, 2023

woodruffw May 8, 2023 •

edited

Loading

alex May 8, 2023

woodruffw May 8, 2023

woodruffw May 8, 2023

alex commented May 9, 2023

woodruffw commented May 9, 2023

woodruffw commented May 9, 2023 •

edited

Loading

woodruffw commented May 9, 2023

woodruffw May 9, 2023

alex May 9, 2023

alex May 9, 2023

alex May 9, 2023

woodruffw May 10, 2023

woodruffw May 10, 2023

alex May 9, 2023

woodruffw May 10, 2023

alex May 10, 2023

		raw: Option<&RawExtensions<'a>>,
		) -> Result<Option<Self>, asn1::ObjectIdentifier> {

certificate: add a get_extension helper #8892

certificate: add a get_extension helper #8892

Conversation

woodruffw commented May 8, 2023

woodruffw commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented May 9, 2023

woodruffw commented May 9, 2023

woodruffw commented May 9, 2023 • edited Loading

woodruffw commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certificate: add a `get_extension` helper #8892

certificate: add a `get_extension` helper #8892

woodruffw May 8, 2023 •

edited

Loading

woodruffw commented May 9, 2023 •

edited

Loading