use x509 from RustCrypto/formats by carl-wallace · Pull Request #348 · iqlusioninc/yubikey.rs

carl-wallace · 2022-02-05T20:47:13Z

No description provided.

…o.toml references an x509 crate not in formats repo, this should change as some PRs there are procecessed)

tony-iqlusion · 2022-02-05T20:49:31Z

tony-iqlusion · 2022-02-05T20:52:41Z

+fn oid_lookup(oid: &ObjectIdentifier) -> String {
+    let oidstr = oid.to_string();
+    if oidstr == "2.5.4.41" {
+        return "name".to_string();
+    } else if oidstr == "2.5.4.4" {
+        return "sn".to_string();
+    } else if oidstr == "2.5.4.42" {
+        return "givenName".to_string();
+    } else if oidstr == "2.5.4.43" {
+        return "initials".to_string();
+    } else if oidstr == "2.5.4.44" {
+        return "generationQualifier".to_string();
+    } else if oidstr == "2.5.4.3" {
+        return "cn".to_string();
+    } else if oidstr == "2.5.4.7" {
+        return "l".to_string();
+    } else if oidstr == "2.5.4.8" {
+        return "st".to_string();
+    } else if oidstr == "2.5.4.9" {
+        return "street".to_string();
+    } else if oidstr == "2.5.4.11" {
+        return "ou".to_string();
+    } else if oidstr == "2.5.4.10" {
+        return "o".to_string();
+    } else if oidstr == "2.5.4.12" {
+        return "title".to_string();
+    } else if oidstr == "2.5.4.46" {
+        return "dnQualifier".to_string();
+    } else if oidstr == "2.5.4.6" {
+        return "c".to_string();
+    } else if oidstr == "2.5.4.5" {
+        return "serialNumber".to_string();
+    } else if oidstr == "2.5.4.65" {
+        return "pseudonym".to_string();
+    } else if oidstr == "0.9.2342.19200300.100.1.25" {
+        return "dc".to_string();
+    } else if oidstr == "1.2.840.113549.1.9.1" {
+        return "emailAddress".to_string();
+    }


You should be able to make OID constants for each of these and compare those, and return a &'static str, something like:

const NAME_OID: ObjectIdentifier = ObjectIdentifier::new("2.5.4.41"); fn oid_lookup(oid: &ObjectIdentifier) -> &'static str { match oid { NAME_OID => "name", ... } }

OK. I will likely move these OIDs and lookup to x509. As noted in the comment, they are duplicated here from certval (but that's probably not the right spot for them either).

As I was implementing this I recalled why String was used. It was to accommodate the case where the OID does not match and the dot notation form is returned. I will add two functions, one that returns a &'static str with empty on a miss and one that calls that and returns a String with dot notation instead of empty.

str4d

Gave this a brief scan. The API changes from the new x509 crate look nice!

str4d · 2022-02-06T01:35:50Z


-/// Certificates
-#[derive(Clone, Debug)]
-pub struct Certificate {


We should not be removing the owned types and replacing them in the API with Vec<u8>; this loses significant type safety. If the issue is that these names conflict with the ones in the x509 crate, then the latter should be referenced directly as x509::Certificate etc. If the issue is that we want to optimise the parsing case, then we should use a Cow or similar.

The issue was that the Certificate (or really the Decoder below it) maintains a reference to the buffer. I could add a new type that binds the buffer and an Option together and return that.

What parts of the certificate borrow? It should probably be an owned data structure

Decoder has a ByteSlice: https://github.com/RustCrypto/formats/blob/master/der/src/decoder.rs#L15. ByteSlice has a reference here: https://github.com/RustCrypto/formats/blob/master/der/src/byte_slice.rs#L16.

Yes, but once the certificate is decoded everything should be owned and no longer reference the input buffer.

Where does the lifetime of borrowed data come into play?

All of the buffers in the structure reference points in the original buffer. So this code snip:

let buf = <read buffer>; let cert = Certificate::from_der(buf.as_slice());

Yields these addresses in a sample run just now:

0x00007fa5cb009200 - buffer (1281 bytes total)
0x00007fa5cb009337 - spki
0x00007fa5cb009459 - first extension
0x00007fa5cb009601 - signature buffer

Aah yeah, an owned form of SPKI seems needed.

There's the PublicKeyDocument document type which we could potentially make work for this application. I think it's kind of like what you're describing where it's a newtype for a Vec<u8> which impls the der::Document trait for decoding.

I will give a look at preparing a CertificateDocument class and return that instead of the buffer in the spots where I moved from struct to buffer. I'll put that in x509 and move the OIDs and nametostr while at it.

We should not be removing the owned types and replacing them in the API with Vec; this loses significant type safety

There's a "document" type convention for this in the other https://github.com/RustCrypto/formats/ and this PR adds a CertificateDocument type: RustCrypto/formats#393

These types are effectively a newtype wrapper around a Vec<u8> which is guaranteed to parse successfully as a particular type, i.e. a Certificate in this case. They impl the der::Document trait which handles serializing/deserializing DER-encoded documents including PEM (de)serialization as well as reading from and writing to DER or PEM-encoded files.

str4d · 2022-02-06T01:58:24Z

-    }
-}
+    let tbs_certificate = TBSCertificate {
+        version: 2,


I thought there was a bug here: version is set to 2, but extensions may be Some, and according to RFC 5280:

issuerUniqueID [1] IMPLICIT UniqueIdentifier OPTIONAL, -- If present, version MUST be v2 or v3 subjectUniqueID [2] IMPLICIT UniqueIdentifier OPTIONAL, -- If present, version MUST be v2 or v3 extensions [3] EXPLICIT Extensions OPTIONAL -- If present, version MUST be v3

It turns out this is not a bug, because the new x509 crate doesn't have a type-safe version field, and this value is the raw encoding of the v3 version, not the numeric version v2.

More generally, I dislike this serialization API, enabled as it is by raw-construcing the same struct used as the output of parsing. It is a brittle approach, prone to exactly the kind of mistake I thought was being made here (and could easily be made by someone else trying to use the crate), because no relative constraints are being placed on any of the fields. In particular, I see no handling in the new x509 crate of the above MUST requirements on version; it looks like the user could construct and serialize a certificate that violates them.

You are correct there is nothing here that addresses the version requirement (or relative requirements in general). I will give a look at that (easiest way in this case is likely to disallow non-v3 at decode/encode time).

FWIW, many of the other crates in https://github.com/rustcrypto/formats use a Version enum, e.g:

https://docs.rs/pkcs8/latest/pkcs8/enum.Version.html

It's also computed implicitly from the structure of the document, eliminating the possibility of setting it to the wrong value:

https://docs.rs/pkcs8/latest/pkcs8/struct.PrivateKeyInfo.html#method.version

At decode time, the implicitly computed version is checked against the explicit one in the document to ensure it's set correctly.

I will give a look and do something similar (and follow suit in the OCSP and CMS bits as the evolve). Thanks.

More generally, I dislike this serialization API, enabled as it is by raw-construcing the same struct used as the output of parsing. It is a brittle approach, prone to exactly the kind of mistake I thought was being made here (and could easily be made by someone else trying to use the crate), because no relative constraints are being placed on any of the fields.

Regarding this in general, it would be good to add something like a CertificateBuilder type which enforces X.509 best practices. We could even use tools like certlint or zlint to ensure that certificates generated with a CertificateBuilder are following those best practices correctly.

The builder type could own the relevant data for various parts of the certificate, and in doing so, that also solves the lifetime-related concerns around a zero-copy certificate parser/serializer which borrows data.

@tony-iqlusion @carl-wallace My latest PRs both add Version types and make it trivial to derive Version types.

carl-wallace added 2 commits January 30, 2022 07:53

migrate to x509 crate from RustCrypto/formats (note - at present Carg…

0e9e00c

…o.toml references an x509 crate not in formats repo, this should change as some PRs there are procecessed)

align with upstream changes (i.e., moving things from x509 to x501)

1dd3aa3

tony-iqlusion reviewed Feb 5, 2022

View reviewed changes

str4d suggested changes Feb 6, 2022

View reviewed changes

str4d reviewed Feb 6, 2022

View reviewed changes

This was referenced Feb 10, 2022

Fix/351/certificate issuer returns subject #352

Closed

Generating CSR #349

Open

Certificate issuer returns subject #351

Closed

tarcieri mentioned this pull request Feb 12, 2022

[WIP] der: generic Document type RustCrypto/formats#427

Closed

tarcieri mentioned this pull request Nov 19, 2022

x509-cert: add certificate builder RustCrypto/formats#764

Merged

carl-wallace closed this Jul 1, 2023

carl-wallace deleted the x509_update branch August 1, 2025 09:52

Uh oh!

Conversation

carl-wallace commented Feb 5, 2022

Uh oh!

tony-iqlusion commented Feb 5, 2022

Uh oh!

tony-iqlusion Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d 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!

tony-iqlusion Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

5 participants

tony-iqlusion Feb 5, 2022 •

edited

Loading

carl-wallace Feb 7, 2022 •

edited

Loading

tony-iqlusion Feb 10, 2022 •

edited

Loading