add support for CRLs, OCSP and certs-only SignedData messages. #462

carl-wallace · 2022-03-03T18:28:13Z

The ocsp crate is new and features encoders/decoders for the OCSP protocol.
the pkcs7 crate now features a new module that provides a partial implementation of SignedData support. This is intended to provide parsers for certs-only PKCS7 messages in support of dynamic path building in the new certval library. This adds a new dependency on the x509 crate. ASN.1 definitions of structures that were not implemented are included as comments.
The spki crate was modified because the signer_infos field in SignedData generated the need to add PartialOrd to AlgorithmIdentifier.
The x509 crate received a new crl module that features encoders/decoders for CRLs. The Rust version in Cargo.toml was modified to match README and sibling modules. A couple of escape characters were added to keyusage.rs to quiet warnings from cargo doc. Typos were fixed in oids.rs and new changed to new_unwrap. Certificate tests were modified to feature new_unwrap. Only a simple CRL parsing test is included here. The certval library more thoroughly exercises CRLs via executing PKITS for validation and revocation status determination.

- The ocsp crate is new and features encoders/decoders for the OCSP protocol. - the pkcs7 crate now features a new module that provides a partial implementation of SignedData support. This is intended to provided parsing certs-only PKCS7 messages in support of dynamic path building in the new certval library. This adds a new dependency on the x509 crate. ASN.1 definitions of structures that were not implemented are included as comments. - The spki crate was modified because the signer_infos field in SignedData generated the need to add PartialOrd to AlgorithmIdentifier. - The x509 crate received a new crl module that features encoders/decoders for CRLs. The Rust version in Cargo.toml was modified to match README and sibling modules. A couple of escape characters were added to keyusage.rs to quiet warnings from cargo doc. Typos were fixed in oids.rs and new changed to new_unwrap. Certificate tests were modified to feature new_unwrap. Only a simple CRL parsing test is included here. The certval library more thoroughly exercises CRLs via executing PKITS for validation and revocation status determination.

npmccallum

This PR needs to be split into multiple PRs. At least the following:

OIDDB changes.
ocsp crate changes.
pkcs7 crate changes.
x509 crate changes.
Other small changes.

npmccallum · 2022-03-04T14:50:35Z

ocsp/src/ocsp.rs

+/// [RFC 6960 Section 4.1.1]: https://datatracker.ietf.org/doc/html/rfc6960#section-4.1.1
+#[derive(Clone, Debug, Eq, PartialEq, Sequence)]
+pub struct TbsRequest<'a> {
+    ///    version             \[0\] EXPLICIT Version DEFAULT v1,


Please do not use documentation comments this way. I would prefer for fields like this to have no documentation rather than a random dump of an ASN.1 fragment.

The above applies to all similar doc comments in this PR.

Re: chunking things up, I asked that very question yesterday before sending the PR and it was agreed not to do that. The reason for sending in one chunk is certval tests cover a lot of territory but need the full feature range.

Re: documentation, we could modify to align with your tastes now or later. I'd like to get this backlog that's been growing since December cleared then address minor nits personally. There will be a few suggestions noted in chat to make as well to get at parsing errors.

@carl-wallace I don't see this conversation. Was it a private conversation between you and @tarcieri ? I don't have any context on what certval tests means.

Regarding the documentation comments: just fix them. It is a 5 minute job to go through all the types and remove these doc comments.

Yes, it was a private chat. Re: certval, this is the library I have mentioned many times and provided a link to. The currently posted one is well out of date (late January). I aim to update it once this clears.

I can go through and delete the doc comments to match your preferred style. I would prefer to leave the CMS comments (i.e., unimpl'ed structs) but don't want to hold anything up over it.

Will be out for a bit. Expect delays in further replies.

npmccallum · 2022-03-04T14:52:31Z

ocsp/src/ocsp.rs

+///    requestList             SEQUENCE OF Request,
+///    requestExtensions   [2] EXPLICIT Extensions OPTIONAL }
+/// ```
+/// [RFC 6960 Section 4.1.1]: https://datatracker.ietf.org/doc/html/rfc6960#section-4.1.1


This link won't actually appear in documentation because nothing refers to i.

The above applies to many similar structures below.

npmccallum · 2022-03-04T15:01:05Z

ocsp/src/ocsp.rs

+// Object Identifiers
+//       id-pkix  OBJECT IDENTIFIER  ::=
+//                { iso(1) identified-organization(3) dod(6) internet(1)
+//                        security(5) mechanisms(5) pkix(7) }
+// id-kp OBJECT IDENTIFIER ::= { id-pkix 3 }
+
+/// id-kp-OCSPSigning            OBJECT IDENTIFIER ::= { id-kp 9 }
+pub const KP_OCSP_SIGNING: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.3.9");
+
+// id-ad OBJECT IDENTIFIER ::= { id-pkix 48 }
+// id-ad-ocsp OBJECT IDENTIFIER ::= { id-ad 1 }
+
+/// id-pkix-ocsp                 OBJECT IDENTIFIER ::= { id-ad-ocsp }
+pub const OCSP: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1");
+
+/// id-pkix-ocsp-basic           OBJECT IDENTIFIER ::= { id-pkix-ocsp 1 }
+pub const OCSP_BASIC: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.1");
+
+/// id-pkix-ocsp-nonce           OBJECT IDENTIFIER ::= { id-pkix-ocsp 2 }
+pub const OCSP_NONCE: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.2");
+
+/// id-pkix-ocsp-crl             OBJECT IDENTIFIER ::= { id-pkix-ocsp 3 }
+pub const OCSP_CRL: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.3");
+
+/// id-pkix-ocsp-response        OBJECT IDENTIFIER ::= { id-pkix-ocsp 4 }
+pub const OCSP_RESPONSE: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.4");
+
+/// id-pkix-ocsp-nocheck         OBJECT IDENTIFIER ::= { id-pkix-ocsp 5 }
+pub const OCSP_NOCHECK: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.5");
+
+/// id-pkix-ocsp-archive-cutoff  OBJECT IDENTIFIER ::= { id-pkix-ocsp 6 }
+pub const KP_OCSP_ARCHIVE_CUTOFF: ObjectIdentifier =
+    ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.6");
+
+/// id-pkix-ocsp-service-locator OBJECT IDENTIFIER ::= { id-pkix-ocsp 7 }
+pub const OCSP_SERVICE_LOCATOR: ObjectIdentifier =
+    ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.7");
+
+/// id-pkix-ocsp-pref-sig-algs   OBJECT IDENTIFIER ::= { id-pkix-ocsp 8 }
+pub const OCSP_PREF_SIG_ALGS: ObjectIdentifier =
+    ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.8");
+
+/// id-pkix-ocsp-extended-revoke OBJECT IDENTIFIER ::= { id-pkix-ocsp 9 }
+pub const OCSP_EXTENDED_REVOKE: ObjectIdentifier =
+    ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.48.1.9");


These don't belong here. They belong in the const-oid OID database.

npmccallum · 2022-03-04T15:05:29Z

ocsp/tests/ocsp.rs

+    // -reqout ~/Desktop/ocspdigicert.req -issuer ~/Desktop/DigiCertGlobalCAG2.crt
+    // -cert ~/Desktop/amazon.der -url http://ocsp.digicert.com -header "HOST" "ocsp.digicert.com" -text
+
+    pub const PKIXALG_SHA1: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.14.3.2.26");


npmccallum · 2022-03-04T15:05:39Z

ocsp/tests/ocsp.rs

+    pub const PKIXALG_SHA256_WITH_RSA_ENCRYPTION: ObjectIdentifier =
+        ObjectIdentifier::new_unwrap("1.2.840.113549.1.1.11");
+    pub const PKIXALG_SHA1: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.14.3.2.26");


npmccallum · 2022-03-04T15:17:02Z

pkcs7/src/cryptographic_message_syntax2004.rs

+}
+impl ValueOrd for SignerInfo<'_> {
+    fn value_cmp(&self, _other: &Self) -> der::Result<Ordering> {
+        todo!()


Pull requests must not contain outstanding todo!() panics.

This is known to be incomplete.

That means nothing to me. This code will still panic at runtime if this method is called. Runtime panics are a NO GO because they provide opportunities for DoS attacks.

npmccallum · 2022-03-04T15:20:29Z

pkcs7/src/cryptographic_message_syntax2004.rs

+     otherCert ANY DEFINED BY otherCertFormat }
+
+   CertificateSet ::= SET OF CertificateChoices
+*/


Please don't leave large comment blocks in the code.

These were left specifically because this incomplete and the comment highlights what has not been implemented. If preference is to drop, that's fine with me.

npmccallum · 2022-03-04T15:20:54Z

pkcs7/src/cryptographic_message_syntax2004.rs

+  Signature ::= BIT STRING
+
+  END -- of CryptographicMessageSyntax2004
+*/


Remove large comment chunks.

npmccallum · 2022-03-04T15:24:35Z

x509/src/ext/pkix/keyusage.rs

 #[derive(Clone, Debug, PartialEq, Eq, Sequence)]
 pub struct PrivateKeyUsagePeriod {
-    /// notBefore       [0]     GeneralizedTime OPTIONAL,
+    /// notBefore       \[0\]     GeneralizedTime OPTIONAL,


Just remove these doc comments.

npmccallum · 2022-03-04T15:25:12Z

x509/src/ext/pkix/oids.rs

-/// OID for OCSPSigning key purpose: 1.3.6.1.5.5.7.3.39
-pub const KP_OCSPSIGNING: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.3.39");
+/// OID for OCSPSigning key purpose: 1.3.6.1.5.5.7.3.9
+pub const KP_OCSPSIGNING: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.5.5.7.3.9");


Please remove this whole file and ensure any missing OIDs are in the OIDDB.

If it is acceptable, I would prefer to clear this then align. certval will need similar treatment.

@carl-wallace FYI #465

carl-wallace added 2 commits March 3, 2022 13:23

missed an Option<>

92e9c8f

npmccallum suggested changes Mar 4, 2022

View reviewed changes

carl-wallace added 2 commits March 4, 2022 11:46

remove field comments with ASN.1 definitions and remove spurious links.

969351f

add test case featuring CRL with an entry that has no entry extensions

bb1d035

npmccallum mentioned this pull request Mar 7, 2022

feat(x509): add CRL type support #475

Merged

carl-wallace closed this Mar 7, 2022

add support for CRLs, OCSP and certs-only SignedData messages. #462

add support for CRLs, OCSP and certs-only SignedData messages. #462

Uh oh!

Conversation

carl-wallace commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npmccallum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carl-wallace commented Mar 3, 2022 •

edited

Loading