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

Adds a trait for crypto, porting EC first #606

Merged
merged 4 commits into from Apr 4, 2023

Conversation

kaczmarczyck
Copy link
Collaborator

This is the the first PR to introduce a Crypto API for our library.
Summary and thoughts:

  • The API tries to follow Rust Crypto. Names are similar where reasonable. To proof its compatiblity, you can switch the test env to the p256 crate (run
    cargo +nightly test --release --features std,ed25519,with_ctap1,vendor_hid,rust_crypto in libraries/opensk/
    ). I couldn't test it on hardware yet, I didn't find a Rust compiler version that works for both Tock v1 and p256 with its dependencies. Numbers on binary size will be meaningful only after a full port, since p256 uses sha2 for signing, so we currently have overlap.
  • The CoseKey type doesn't easily translate. I intend to keep all types that are used in parsing commands (rule of thumb: converts to CBOR) free of Env. CoseKey is one of them. The From and TryFrom replacements in this PR could be improved in a follow-up PR. Best case the compiler could again ensure that we don't create an ECDH CoseKey from a ECDSA public key and vice versa.
  • The crypto implementations in TestEnv and TockEnv are currently identical. I was wondering if they TockEnv should just import TestEnv, but kept them fully independent for now. This way I don't have to mess with the conditional compilation and it's easier to change only one of them later (e.g. hardware crypto).
  • I introduced shorthands like
    pub type EcdhSk<E> = <<<E as Env>::Crypto as Crypto>::Ecdh as Ecdh>::SecretKey;
    Seems to me to be the best way to have less noise. The trait imports still look a bit clunky, as SecretKey has the same name for ECDH and ECDSA. Which makes sense inside its namespace, but needs extra work when useing them. One option is to split the traits into multiple, like replacing the ECDSA SecretKey with a general SecretKey that has a random and a public_key function, together with a Signer trait. But I feels like unncecessary complexity to me.

Curious about your thoughts!

@kaczmarczyck kaczmarczyck requested a review from ia0 March 17, 2023 16:43
@coveralls
Copy link

coveralls commented Mar 17, 2023

Coverage Status

Coverage: 96.536% (+0.1%) from 96.403% when pulling 128d694 on kaczmarczyck:crypto-trait into 6d5ea16 on google:develop.

libraries/opensk/src/api/crypto/mod.rs Outdated Show resolved Hide resolved
libraries/opensk/src/api/crypto/mod.rs Outdated Show resolved Hide resolved
libraries/opensk/src/api/key_store.rs Outdated Show resolved Hide resolved
libraries/opensk/src/ctap/ctap1.rs Outdated Show resolved Hide resolved
libraries/opensk/src/ctap/data_formats.rs Outdated Show resolved Hide resolved
libraries/opensk/src/env/test/mod.rs Outdated Show resolved Hide resolved
libraries/opensk/src/env/test/rust_crypto.rs Outdated Show resolved Hide resolved
libraries/opensk/src/env/test/rust_crypto.rs Outdated Show resolved Hide resolved
ia0
ia0 previously approved these changes Mar 20, 2023
libraries/opensk/src/env/test/crypto.rs Outdated Show resolved Hide resolved
src/env/tock/crypto.rs Outdated Show resolved Hide resolved
@kaczmarczyck
Copy link
Collaborator Author

The failing tests are unrelated to this PR and seem to be related to a difference in compilation between debug and release mode. Locally, all tests pass.

@kaczmarczyck kaczmarczyck merged commit c168141 into google:develop Apr 4, 2023
25 checks passed
@kaczmarczyck kaczmarczyck deleted the crypto-trait branch April 4, 2023 11:54
@kaczmarczyck kaczmarczyck restored the crypto-trait branch April 4, 2023 11:54
@kaczmarczyck kaczmarczyck deleted the crypto-trait branch April 4, 2023 11:54
@kaczmarczyck kaczmarczyck restored the crypto-trait branch April 4, 2023 11:54
@kaczmarczyck kaczmarczyck deleted the crypto-trait branch April 4, 2023 11:54
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.

None yet

3 participants