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

Use x509-cert certificate builder #495

Merged
merged 6 commits into from
Aug 15, 2023

Conversation

baloo
Copy link
Contributor

@baloo baloo commented Apr 1, 2023

makes use of RustCrypto/formats#764 for generating certificates.

@baloo baloo force-pushed the baloo/x509-cert-builder branch 2 times, most recently from d9e4d73 to 37bd854 Compare April 6, 2023 03:46
OID_NIST_P256 => Ok(AlgorithmId::EccP256),
OID_NIST_P384 => Ok(AlgorithmId::EccP384),
_ => Err(Error::AlgorithmError),
pub struct Rsa1024;
Copy link
Contributor Author

@baloo baloo Apr 6, 2023

Choose a reason for hiding this comment

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

Feels like I'm redefining AlgorithmId, which is not great. (I'm considering replacing it entirely, but it's a pretty major API breakage).

@baloo baloo force-pushed the baloo/x509-cert-builder branch 4 times, most recently from bc4850c to ee3e1a0 Compare April 10, 2023 22:22
@tony-iqlusion
Copy link
Member

@baloo looks like you need to update cli/src/terminal.rs:183 too, possibly replacing into_buffer with to_der

@baloo
Copy link
Contributor Author

baloo commented Apr 12, 2023

yes, I haven't got time to do that yet.

@tony-iqlusion
Copy link
Member

@baloo whoops, tried to bump the rsa version and now it's triggering a deprecation warning

@tony-iqlusion
Copy link
Member

Also I'm about to cut rsa v0.9.0, so maybe just update to that when it's released:

RustCrypto/RSA#318

@baloo baloo force-pushed the baloo/x509-cert-builder branch from 6e857e2 to 7a0de9c Compare April 27, 2023 17:36
@baloo
Copy link
Contributor Author

baloo commented Apr 27, 2023

Sorry I missed the RSA release train (fixed the deprecation notice)

@baloo baloo force-pushed the baloo/x509-cert-builder branch from 7a0de9c to e8e3a88 Compare April 27, 2023 17:39
@baloo
Copy link
Contributor Author

baloo commented Apr 27, 2023

ran the ignored tests on a dummy yubikey:

[Running 'cargo test -- --ignored  --nocapture ']
   Compiling yubikey v0.8.0-pre.0 (/home/arthur_gautier/work/dev/yubikey.rs)
    Finished test [unoptimized + debuginfo] target(s) in 1.29s
     Running unittests src/lib.rs (target/debug/deps/yubikey-40695a15af72431a)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s

     Running tests/integration.rs (target/debug/deps/integration-b52bb285bb799885)

running 10 tests
test test_slot_id_display ... ok
[INFO  yubikey::yubikey] connected to reader: Yubico YubiKey CCID 00 00
test test_parse_cert_from_der ... ok
test test_verify_pin ... ok
test test_get_cccid ... ok
test test_get_chuid ... ok
test test_get_config ... ok
test generate_self_signed_ec_cert ... ok
test test_list_keys ... ok
test test_read_metadata ... ok
test generate_self_signed_rsa_cert ... ok

test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.92s

   Doc-tests yubikey

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

tony-iqlusion
tony-iqlusion previously approved these changes Apr 27, 2023
Copy link
Member

@tony-iqlusion tony-iqlusion left a comment

Choose a reason for hiding this comment

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

Looks good. I especially like how it cleans up the dependency tree.

Seems like there's some potential refactoring possible but that seems fine as a followup to me.

Will leave a note or two inline but they're not blocking.

@tony-iqlusion
Copy link
Member

@str4d can you take a look at this when you get a chance?

@carl-wallace
Copy link
Contributor

The new function for Signer should be pub, not pub(crate). This would allow it to be used for things like generating SignedData. I'd also like to see the Attestation slot added to the list of SLOTS in piv.rs. These changes are here: carl-wallace@aef7c67. A gadget that uses this PR plus the suggested changes is here: https://github.com/carl-wallace/pbyk.

@tony-iqlusion
Copy link
Member

@str4d unless you have any objections I'd like to merge this

@tony-iqlusion tony-iqlusion merged commit 6a1e160 into iqlusioninc:main Aug 15, 2023
@baloo baloo deleted the baloo/x509-cert-builder branch August 15, 2023 04:01
tarcieri pushed a commit that referenced this pull request Aug 15, 2023
It's no longer used as of #495
tony-iqlusion added a commit that referenced this pull request Aug 15, 2023
It's no longer used as of #495
This was referenced Aug 16, 2023
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.

3 participants