Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x509-cert: add certificate builder #764

Merged
merged 8 commits into from
Apr 10, 2023

Conversation

baloo
Copy link
Member

@baloo baloo commented Nov 19, 2022

This is an attempt at a certificate builder.

See: #418

@baloo
Copy link
Member Author

baloo commented Nov 19, 2022

This is very much a work in progress, but sharing early to get feedback. I think I want to attached the extensions to the CertificateVersion::V3.

I don't exactly know how much we want to force in that API. I'm inclined to force the inclusion of:

  • X509v3 CRL Distribution Points
  • X509v3 Extended Key Usage
  • X509v3 Key Usage (make it critical)
  • X509v3 Basic Constraints (make it critical)
  • X509v3 Subject Alternative Name (probably needs a builder itself)

I'm not sure about:

  • Authority Information Access (ocsp uri)
  • X509v3 Certificate Policies (not sure if that's expected for a private CA).
  • X509v3 Subject Key Identifier / X509v3 Authority Key Identifier (not sure how they are built)

Pretty convinced I can't force:

  • CT Precertificate SCTs
    the CT infrastructure is unlikely for a private ca.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 2 times, most recently from 2c89de5 to ad06b76 Compare November 19, 2022 16:11
@tarcieri
Copy link
Member

@baloo there was a little bit of previous discussion on iqlusioninc/yubikey.rs#348 as linked from #418.

One thing I'd recommend is running the generated certificates through a tool like certlint or zlint (probably zlint these days?). I can help out with automating CI setup for that as need be.

@tarcieri
Copy link
Member

Also @str4d's x509 crate includes a certificate builder and might provide some inspiration here.

I think if we make a good enough replacement he'll also give us the x509 crate name.

@baloo
Copy link
Member Author

baloo commented Nov 19, 2022

I've started a zlint integration, but it's not as easy as I thought (need to parse the output json, I can't just rely on the status code), I also need to remember how to setup github actions and pull a binary. Got the integration for it in nixos already (to run on my machine): NixOS/nixpkgs#201537

What are your thoughts on https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html. Wondering whether I should keep both the UIntRef and it's backing buffer (Vec<u8>) in the builder as to not have the fn build have to "reparse" from buffer.

@tarcieri
Copy link
Member

I also need to remember how to setup github actions and pull a binary.

There's a binary here:

https://github.com/zmap/zlint/releases/download/v3.4.0/zlint_3.4.0_Linux_x86_64.tar.gz

You can look at this as an example for how to download it in a GitHub Actions workflow:

https://github.com/RustCrypto/actions/blob/master/cargo-hack-install/action.yml

It's fine to just put it in the x509-cert workflow for now.

What are your thoughts on https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html.

I played with trying to integrate ouroboros into der in the past. I don't think it was a good fit.

See #734 where I proposed yoke as an alternative.

Alternatively we could eliminate the lifetimes on the various types and move to entirely owned types. I opened #765 to discuss that.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 2 times, most recently from a44db20 to 111c526 Compare November 20, 2022 06:56
@baloo
Copy link
Member Author

baloo commented Nov 20, 2022

I think I like the yoke a bit better than going to owned types.

@tarcieri
Copy link
Member

tarcieri commented Nov 20, 2022

The owned types are actually nice when decoding from PEM, since they avoid an intermediate PEM -> DER pass and allow the DER to be decoded from PEM on-the-fly as needed to parse document.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch from a8c8cb6 to cd1bf94 Compare November 21, 2022 17:07
@baloo
Copy link
Member Author

baloo commented Nov 21, 2022

test basic_certificate ... ok

Got the root CA profile to work with zero lint error on zlint.
I guess now I need to get the other profiles working and then clean up that whole mess I made.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 2 times, most recently from c00659b to 240c0f9 Compare November 23, 2022 04:00
@baloo
Copy link
Member Author

baloo commented Nov 23, 2022

(minimal-versions error is a regression in nightly: rust-lang/rust#104759)

@baloo baloo marked this pull request as draft November 28, 2022 06:03
@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch from 240c0f9 to 61fe940 Compare December 31, 2022 06:11
@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 3 times, most recently from aeb2cf8 to 1e4b92e Compare January 3, 2023 18:30
@brandonweeks
Copy link

I don't exactly know how much we want to force in that API. I'm inclined to force the inclusion of:

  • X509v3 CRL Distribution Points
  • X509v3 Extended Key Usage
  • X509v3 Key Usage (make it critical)
  • X509v3 Basic Constraints (make it critical)
  • X509v3 Subject Alternative Name (probably needs a builder itself)

It feels like doing so would inhibit the usage of this API for use cases other than Web PKI. Which would be a shame because I suspect Web PKI will be one of the last environments where this crate will be adopted.

Go's crypto/x509 offers one of the best APIs for parsing and generating X.509 certificate for arbitrary use cases. It'd be great if this crate could analogous functionality.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch from 1e4b92e to 74dcf8e Compare March 30, 2023 21:11
@baloo
Copy link
Member Author

baloo commented Mar 30, 2023

I don't exactly know how much we want to force in that API. I'm inclined to force the inclusion of:

  • X509v3 CRL Distribution Points
  • X509v3 Extended Key Usage
  • X509v3 Key Usage (make it critical)
  • X509v3 Basic Constraints (make it critical)
  • X509v3 Subject Alternative Name (probably needs a builder itself)

It feels like doing so would inhibit the usage of this API for use cases other than Web PKI. Which would be a shame because I suspect Web PKI will be one of the last environments where this crate will be adopted.

Go's crypto/x509 offers one of the best APIs for parsing and generating X.509 certificate for arbitrary use cases. It'd be great if this crate could analogous functionality.

@brandonweeks I implemented an hazmat feature that let you opt out of the default extensions (via Profile::Manual) and that allows you to build your own certificate without webpki extensions.

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 2 times, most recently from f803168 to 37b45da Compare April 1, 2023 16:05
@baloo baloo changed the title x509-cert: wip: add certificate builder x509-cert: add certificate builder Apr 1, 2023
@tarcieri
Copy link
Member

tarcieri commented Apr 9, 2023

Finished a readthrough pass. Looking good so far. It's a lot of code!

@@ -0,0 +1,195 @@
use serde::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I snagged https://crates.io/crates/zlint

Might be interesting to extract this into a separate crate (after landing this PR perhaps)

@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch from ce7fe7c to 3086be6 Compare April 10, 2023 16:17
@baloo baloo mentioned this pull request Apr 10, 2023
@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch 3 times, most recently from 497ad4f to 7a91c7b Compare April 10, 2023 20:10
@baloo baloo force-pushed the baloo/x509-cert/cert-builder branch from 7a91c7b to 794e080 Compare April 10, 2023 21:17
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to go ahead and land this

@tarcieri tarcieri merged commit 98957ae into RustCrypto:master Apr 10, 2023
@baloo baloo deleted the baloo/x509-cert/cert-builder branch April 10, 2023 22:08
lumag added a commit to lumag/formats that referenced this pull request Apr 16, 2023
top-level Cargo.toml patches crates.io to point to the in-tree versions
of several crates. Revert some of the changes introduced by the commit
98957ae ("x509-cert: add certificate builder (RustCrypto#764)") which gets
overriden again by the patch.crates.io config.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
lumag added a commit to lumag/formats that referenced this pull request Apr 16, 2023
top-level Cargo.toml patches crates.io to point to the in-tree versions
of several crates. Revert some of the changes introduced by the commit
98957ae ("x509-cert: add certificate builder (RustCrypto#764)") which gets
overriden again by the patch.crates.io config.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
baloo added a commit to baloo/formats that referenced this pull request May 3, 2023
Added
- Certificate builder ([RustCrypto#764])
- Support for `RandomizedSigner` in builder ([RustCrypto#1007])
- Provide parsing profiles ([RustCrypto#987])
- Support for `Time::INFINITY` ([RustCrypto#1024])
- Conversion from `std::net::IpAddr` ([RustCrypto#1035])
- `CertReq` builder ([RustCrypto#1034])

Changed
- use `ErrorKind::Value` for overlength serial ([RustCrypto#988])
- Bump `hex-literal` to v0.4.1 ([RustCrypto#999])
- Builder updates ([RustCrypto#1001])
- better debug info when `zlint` isn't installed ([RustCrypto#1018])
- make SKI optional in leaf certificate ([RustCrypto#1028])
- bump rsa from 0.9.0-pre.2 to 0.9.0 ([RustCrypto#1033])

Fixed
- fix `KeyUsage` bit tests ([RustCrypto#993])
- extraneous PhantomData in `TbsCertificate` ([RustCrypto#1019])
@baloo baloo mentioned this pull request May 3, 2023
baloo added a commit to baloo/formats that referenced this pull request May 3, 2023
Added
- Certificate builder ([RustCrypto#764])
- Support for `RandomizedSigner` in builder ([RustCrypto#1007])
- Provide parsing profiles ([RustCrypto#987])
- Support for `Time::INFINITY` ([RustCrypto#1024])
- Conversion from `std::net::IpAddr` ([RustCrypto#1035])
- `CertReq` builder ([RustCrypto#1034])

Changed
- use `ErrorKind::Value` for overlength serial ([RustCrypto#988])
- Bump `hex-literal` to v0.4.1 ([RustCrypto#999])
- Builder updates ([RustCrypto#1001])
- better debug info when `zlint` isn't installed ([RustCrypto#1018])
- make SKI optional in leaf certificate ([RustCrypto#1028])
- bump rsa from 0.9.0-pre.2 to 0.9.0 ([RustCrypto#1033])

Fixed
- fix `KeyUsage` bit tests ([RustCrypto#993])
- extraneous PhantomData in `TbsCertificate` ([RustCrypto#1017])
baloo added a commit to baloo/formats that referenced this pull request May 10, 2023
Added
- Certificate builder (RustCrypto#764)
- Support for `RandomizedSigner` in builder (RustCrypto#1007)
- Provide parsing profiles (RustCrypto#987)
- Support for `Time::INFINITY` (RustCrypto#1024)
- Conversion from `std::net::IpAddr` (RustCrypto#1035)
- `CertReq` builder (RustCrypto#1034)
- missing extension implementations (RustCrypto#1050)
- notes about `UTCTime` range being 1970-2049 (RustCrypto#1052)

Changed
- use `ErrorKind::Value` for overlength serial (RustCrypto#988)
- Bump `hex-literal` to v0.4.1 (RustCrypto#999)
- Builder updates (RustCrypto#1001)
- better debug info when `zlint` isn't installed (RustCrypto#1018)
- make SKI optional in leaf certificate (RustCrypto#1028)
- bump rsa from 0.9.0-pre.2 to 0.9.0 (RustCrypto#1033)
- bump rsa from 0.9.1 to 0.9.2 (RustCrypto#1056)

Fixed
- fix `KeyUsage` bit tests (RustCrypto#993)
- extraneous PhantomData in `TbsCertificate` (RustCrypto#1017)
- CI flakiness (RustCrypto#1042)
- usage of ecdsa signer (RustCrypto#1043)
baloo added a commit to baloo/formats that referenced this pull request May 11, 2023
Added
- Certificate builder (RustCrypto#764)
- Support for `RandomizedSigner` in builder (RustCrypto#1007)
- Provide parsing profiles (RustCrypto#987)
- Support for `Time::INFINITY` (RustCrypto#1024)
- Conversion from `std::net::IpAddr` (RustCrypto#1035)
- `CertReq` builder (RustCrypto#1034)
- missing extension implementations (RustCrypto#1050)
- notes about `UTCTime` range being 1970-2049 (RustCrypto#1052)
- consume the `SignatureBitStringEncoding` trait (RustCrypto#1048)

Changed
- use `ErrorKind::Value` for overlength serial (RustCrypto#988)
- Bump `hex-literal` to v0.4.1 (RustCrypto#999)
- Builder updates (RustCrypto#1001)
- better debug info when `zlint` isn't installed (RustCrypto#1018)
- make SKI optional in leaf certificate (RustCrypto#1028)
- bump rsa from 0.9.0-pre.2 to 0.9.0 (RustCrypto#1033)
- bump rsa from 0.9.1 to 0.9.2 (RustCrypto#1056)

Fixed
- fix `KeyUsage` bit tests (RustCrypto#993)
- extraneous PhantomData in `TbsCertificate` (RustCrypto#1017)
- CI flakiness (RustCrypto#1042)
- usage of ecdsa signer (RustCrypto#1043)
baloo added a commit that referenced this pull request May 19, 2023
Added
- Certificate builder (#764)
- Support for `RandomizedSigner` in builder (#1007)
- Provide parsing profiles (#987)
- Support for `Time::INFINITY` (#1024)
- Conversion from `std::net::IpAddr` (#1035)
- `CertReq` builder (#1034)
- missing extension implementations (#1050)
- notes about `UTCTime` range being 1970-2049 (#1052)
- consume the `SignatureBitStringEncoding` trait (#1048)

Changed
- use `ErrorKind::Value` for overlength serial (#988)
- Bump `hex-literal` to v0.4.1 (#999)
- Builder updates (#1001)
- better debug info when `zlint` isn't installed (#1018)
- make SKI optional in leaf certificate (#1028)
- bump rsa from 0.9.0-pre.2 to 0.9.0 (#1033)
- bump rsa from 0.9.1 to 0.9.2 (#1056)

Fixed
- fix `KeyUsage` bit tests (#993)
- extraneous PhantomData in `TbsCertificate` (#1017)
- CI flakiness (#1042)
- usage of ecdsa signer (#1043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants