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

Refactor key import function #128

Merged
merged 4 commits into from
Jun 10, 2020
Merged

Refactor key import function #128

merged 4 commits into from
Jun 10, 2020

Conversation

BlackHoleFox
Copy link
Contributor

@BlackHoleFox BlackHoleFox commented Jun 2, 2020

Following up on #26, I have refactored the import_key function out into two separate functions and have done some general cleanup.

As for the precomputations, I haven't done them yet as I don't know what is safe to adapt over from rsa's generate_multi_prime_key() and PrecomputedValues.

@BlackHoleFox
Copy link
Contributor Author

Seems like I didn't tag all the methods I needed to in #[cfg(feature = "untested")]. I only ran cargo check --features untested. locally, my bad there.

@tarcieri
Copy link
Collaborator

tarcieri commented Jun 2, 2020

Looks like an improvement so far.

@str4d any thoughts on this?

@BlackHoleFox
Copy link
Contributor Author

BlackHoleFox commented Jun 4, 2020

I took a stab at calculating the RSA pre-computation values but ran into a small snag. The BigUint rsa uses is from a forked crate, and on top of that, the ModInverse trait for the same reason. I tried switching the crate yubikey-priv uses to the forked version, but the X509 certs have the standard one.

Do you have a recommended approach for managing the two types of BigUints?

@tarcieri
Copy link
Collaborator

tarcieri commented Jun 8, 2020

@BlackHoleFox urgh, that kind of sucks. You may need to use the "semver trick"?

@BlackHoleFox
Copy link
Contributor Author

I looked into the semver trick and found this post, which worked just like I wanted.

For the precomputation values, I tried to follow what the RSA crate does as close as possible. I had to bring in a few more of the num- crates as well for some traits.

@tarcieri
Copy link
Collaborator

tarcieri commented Jun 9, 2020

Cool, glad you got it working.

It'd be good to maybe open some issues upstream (or comment on existing issues) to try to coordinate the versions so we don't need to use two of them in perpetuity.

src/key.rs Outdated Show resolved Hide resolved
@BlackHoleFox
Copy link
Contributor Author

As for the upstream issues, I opened one here on num-bigint-dig about upstreaming the ModInverse trait, hopefully its a reasonable thing and possible.

@BlackHoleFox
Copy link
Contributor Author

I tried to test that this works with my Yubikey, but I get an auth error every time I try. No clue if this is because the API is being used wrong or if because I did something with my Yubikey when I first got it and forgot. The official tool mentions something about the management key and how a default is provided, but I wasn't able to pinpoint exactly where that happens in the C code, nor in this library.

Would you mind giving it a try? This is the code I used in tests/integration.rs:

#[test]
#[cfg(feature = "untested")]
fn import_rsa_2048_key() {
    let mut p = [0u8; 32];
    let mut q = [0u8; 32];

    getrandom(&mut p).unwrap();
    getrandom(&mut q).unwrap();

    let key_data = RsaKeyData::new(&p, &q);

    let mut yubikey = YUBIKEY.lock().unwrap();
    let slot = SlotId::KeyManagement;
    let algorithm = AlgorithmId::Rsa2048;
    let touch_policy = TouchPolicy::Default;
    let pin_policy = PinPolicy::Default;

    import_rsa_key(&mut yubikey, slot, algorithm, key_data, touch_policy, pin_policy).unwrap();
}

There is a fair chance that you will have to run a few times to get past the RsaKeyData::new() because the random bytes aren't always primes, so the exp.mod_inverse(&totient) fails sometimes.

@tarcieri
Copy link
Collaborator

I'm going to go ahead and land this in the untested state as it's certainly an improvement over the existing code and the changes will help unblock some other upgrades we're trying to do anyway.

We're almost to the point I'm ready to actually consume this crate in a project, at which point I will try to debug whatever issues are occurring.

@tarcieri tarcieri merged commit 15f9e26 into iqlusioninc:develop Jun 10, 2020
@tony-iqlusion tony-iqlusion mentioned this pull request Oct 19, 2020
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.

2 participants