x501+x509: `CertificateDocument`, `X509Version`, and more additions #393

carl-wallace · 2022-02-07T17:20:54Z

Modifications resulting from code review of changes to yubikey library and client tool to use x509 crate:

moved additional OID definitions from certval into x509
moved, refactored and expanded OID lookup function
added CertificateDocument implementation
added X509Version enum and improved decode and encode for TBSCertificate relative to version-specific fields

…ic str option, and expand to cover more OIDs

… to version number

x501/src/time.rs

x509/src/certificate.rs

tarcieri · 2022-02-07T20:05:35Z

x509/src/certificate.rs

@@ -25,6 +28,57 @@ pub fn default_zero() -> u32 {
 /// Version  ::=  INTEGER  {  v1(0), v2(1), v3(2)  }
 pub const X509_CERT_VERSION: u8 = 2;

+/// Version identifier for X.509 certificates. In practice, only v3 is used.
+#[derive(Clone, Debug, Copy, PartialEq, Eq, PartialOrd)]
+pub enum X509Version {


In the spirit of RFC356 I'd suggest simply calling this Version.

That also follows the convention used by other version types in this repo.

ok. will change as i mop up cargo hack findings.

#410 allows you to derive this type.

x509/src/certificate_traits.rs

x509/src/certificate.rs

…ubikey_mods

x509/src/certificate.rs

tarcieri · 2022-02-07T22:15:33Z

x509/src/certificate.rs

        #[allow(unused_imports)]
        use core::convert::TryFrom;
        f(&[
            &ContextSpecific {
                tag_number: VERSION_TAG,
                tag_mode: TagMode::Explicit,
-                value: self.version,
+                value: version_value,


Seems like just self.version should work here, especially if you make it Copy

Addressed here (initially I though you meant replace that production with self.version but landed as shown): carl-wallace@c226d37

…conversion to u8 when encoding context specific tag for version field.

…dd a v1 test case, add an invalid version (v7) test case

x509/src/certificate.rs

… for Certificate type.

x509/src/certificate_document.rs

npmccallum · 2022-02-11T05:58:29Z

@tarcieri I would like to review this after my derive fixes and other cleanup has been merged.

npmccallum · 2022-02-11T06:27:51Z

x509/src/certificate_document.rs

+/// it will parse successfully according to this crate's parsing rules.
+#[derive(Clone)]
+#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
+pub struct CertificateDocument(Vec<u8>);


@tarcieri I know I've implemented one of these document types for pkcs10. However:

What is the purpose of them? I don't understand how they are intended to be used. It seems that PEM decoding is implemented through the document types. However, in order to instantiate a document, you have to fully parse the DER. So why not just have the fully parsed type?

If there is a good reason for the document types, surely these can be genericized. They are all exactly the same code and differ only by the type of the value decoded therein. So Document<T: Decodable + Encodable> seems like an obvious win here.

This seems like a complicated and important enough issue I did a long-form write up on it, so as not to lose the answers to these questions in a PR comment: #421

Some short answers though:

The purpose of these types is discussed at length in der: *Document type improvements #421, but in regard to PEM specifically, since Decode<'a> is zero-copy and needs to borrow from a slice containing DER, parsing PEM needs to decode DER into a buffer that Decode<'a> can borrow from. If we had something like DecodeOwned which didn't need to borrow, documents could be decoded directly from PEM. As it were, this is how the ssh-key crate works, and I just added support for incremental/buffered PEM decoding to the pem-rfc7468 crate (pem-rfc7468: buffered Base64 decoder #406) and changed the ssh-key crate to use that (ssh-key: use pem-rfc7468 for private key PEM parser #407). But that only works because the ssh-key crate is built entirely around owned types (and doesn't use ASN.1 DER).

The problem with Document<T: Decodable + Encodable> is you omitted the lifetime on Decodable<'a>. Since it's a 'self borrow and Rust doesn't actually have a way to notate a 'self lifetime you can't notate that on a struct definition, however as I noted in der: *Document type improvements #421 it can probably be done by moving the bounds to individual methods so Decodable<'a> can hang off pub fn decode<'a>(&'a self) -> T where T: Decode<'a>

npmccallum · 2022-02-11T06:28:35Z

x509/src/certificate.rs

+            _ => Err(Self::TAG.value_error()),
+        }
+    }
+}


@carl-wallace All the changes in this file are obsolete if we merge #410 instead.

Sure. Further review of this until after alignment with changes to dependencies is likely not worthwhile.

npmccallum · 2022-02-11T06:30:01Z

x509/src/pkix_oids.rs

+/// id-sha512  OBJECT IDENTIFIER  ::=
+///        { joint-iso-itu-t(2) country(16) us(840) organization(1) gov(101)
+///        csor(3) algorithms(4) hashalgs(2) 3 }
+pub const PKIXALG_SHA512: ObjectIdentifier = ObjectIdentifier::new("2.16.840.1.101.3.4.2.3");


I really don't like the "big pile of OID constants" approach. There has to be a way to organize these better.

The OIDs are grouped by module with names derived by capitalization, insertion of underscores and addition of a prefix. I'm OK with this approach, but if there is something desired please let me know.

npmccallum · 2022-02-11T06:31:47Z

x509/src/pkix_oids.rs

+        PKIXALG_SHA512 => "sha512",
+        _ => "",
+    }
+}


I really don't like this function. It smells like OpenSSL. What is the purpose of it?

Logging and display.

npmccallum · 2022-02-11T06:32:11Z

x509/tests/certificate.rs

-
+pub fn default_zero_u8() -> u8 {
+    0
+}


Please don't. Use Default::default.

As part of the realignment as noted above.

carl-wallace · 2022-03-03T20:54:55Z

outdated

carl-wallace added 7 commits February 5, 2022 14:48

add Display impl for Time

f2ab456

refactor Display to use new to_date_time

213e9a2

move algorithm OID definitions to x509 from certval

945bfe9

move OID lookup from certval (and yubikey), refactor to have a &'stat…

c2d3b40

…ic str option, and expand to cover more OIDs

add version enum and add constraints to govern decode/encode relative…

40cf4aa

… to version number

clippy suggestions

7504101

address unused import on alloc feature

30c77d0

tarcieri reviewed Feb 7, 2022

View reviewed changes

x501/src/time.rs Outdated Show resolved Hide resolved

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

carl-wallace and others added 3 commits February 7, 2022 13:18

add links to doc comment

66e6b9a

use const length of 1 vs calculated

89dc750

Merge branch 'master' into yubikey_mods

b34f48b

tarcieri changed the title ~~Yubikey mods~~ x501+x509: CertificateDocument, X509Version, and more additions Feb 7, 2022

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate_traits.rs Outdated Show resolved Hide resolved

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

carl-wallace added 3 commits February 7, 2022 16:09

address cargo-hack findings

bc391a7

Merge branch 'yubikey_mods' of github.com:carl-wallace/formats into y…

c2f9377

…ubikey_mods

rename new version enum, add a default, delete an unused const

1e44e55

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

tarcieri reviewed Feb 7, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

tarcieri reviewed Feb 7, 2022

View reviewed changes

carl-wallace added 4 commits February 7, 2022 18:17

reorder derive attributes on version

fb203b6

use EncodeValue/DecodeValue instead of Encodable/Decodable for Version

ee9f1a3

fix bug in value_len (was still implemented as if encoded_len). drop …

c226d37

…conversion to u8 when encoding context specific tag for version field.

refactor version handling (including adding proper support for v1), a…

b600b99

…dd a v1 test case, add an invalid version (v7) test case

tarcieri reviewed Feb 8, 2022

View reviewed changes

x509/src/certificate.rs Outdated Show resolved Hide resolved

carl-wallace added 2 commits February 8, 2022 09:02

fix visibility on version default function

6fe650f

drop traits for certificate as unnecessary. move PEM support to impls…

a9dab3e

… for Certificate type.

tarcieri reviewed Feb 9, 2022

View reviewed changes

x509/src/certificate_document.rs Outdated Show resolved Hide resolved

remove redundant impls

b7a19de

tony-iqlusion mentioned this pull request Feb 10, 2022

use x509 from RustCrypto/formats iqlusioninc/yubikey.rs#348

Closed

npmccallum suggested changes Feb 11, 2022

View reviewed changes

carl-wallace closed this Mar 3, 2022

carl-wallace deleted the yubikey_mods branch January 23, 2023 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x501+x509: `CertificateDocument`, `X509Version`, and more additions #393

x501+x509: `CertificateDocument`, `X509Version`, and more additions #393

carl-wallace commented Feb 7, 2022

tarcieri Feb 7, 2022

carl-wallace Feb 7, 2022

npmccallum Feb 11, 2022

tarcieri Feb 7, 2022

carl-wallace Feb 8, 2022

npmccallum commented Feb 11, 2022

npmccallum Feb 11, 2022

tarcieri Feb 11, 2022 •

edited

Loading

npmccallum Feb 11, 2022

carl-wallace Feb 11, 2022

npmccallum Feb 11, 2022

carl-wallace Feb 11, 2022

npmccallum Feb 11, 2022

carl-wallace Feb 11, 2022

npmccallum Feb 11, 2022

carl-wallace Feb 11, 2022

carl-wallace commented Mar 3, 2022

x501+x509: CertificateDocument, X509Version, and more additions #393

x501+x509: CertificateDocument, X509Version, and more additions #393

Conversation

carl-wallace commented Feb 7, 2022

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

npmccallum commented Feb 11, 2022

Choose a reason for hiding this comment

tarcieri Feb 11, 2022 • 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

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

carl-wallace commented Mar 3, 2022

x501+x509: `CertificateDocument`, `X509Version`, and more additions #393

x501+x509: `CertificateDocument`, `X509Version`, and more additions #393

tarcieri Feb 11, 2022 •

edited

Loading