-
Notifications
You must be signed in to change notification settings - Fork 54
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
support for pure-rust make credentials #563
base: main
Are you sure you want to change the base?
Conversation
75b7750
to
3671522
Compare
3671522
to
75fa54b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really cool. Of course error handling and other niceties are missing but that's not the point. Happy to see tests, I've skimmed them.
Thanks! 👍
tss-esapi/tests/integration_tests/abstraction_tests/credential_tests.rs
Outdated
Show resolved
Hide resolved
55774ad
to
573d67e
Compare
let cred = vec![1, 2, 3, 4, 5]; | ||
let expected = Digest::try_from(vec![1, 2, 3, 4, 5]).unwrap(); | ||
|
||
let (credential_blob, secret) = utils::make_credential_ecc::<_, sha2::Sha256, aes::Aes128>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sha2::Sha256, aes::Aes128
here codes for EKHash
and EkCipher
.
Those should be read from the template of the EK ideally.
Although in reality, the template would have been dropped already and we're only working with a PEM encoded public key, and there should be some kind of default value.
https://github.com/tpm2-software/tpm2-tools/blob/master/tools/tpm2_makecredential.c#L340
Anyone with an opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your question about this test in particular, or about the interface of make_credential_ecc
, and whether we can deduce the type params from the inputs?
I think generally you should be able to deduce the hash and the cipher for the EK if you know the nature of the public key, for example by doing the reverse of the mapping done here: https://github.com/parallaxsecond/rust-tss-esapi/blob/main/tss-esapi/src/abstraction/ek.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that was an API question.
I know how to get the parameters from a Public, but I don't expect the public or its template to always available.
9b21b16
to
89021e0
Compare
451ae4f
to
c652a60
Compare
I've finished support for both RSA and ECC, and there is now error management. |
7b03a64
to
969e006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had a brief look, will come back for more :)
Cargo.toml
Outdated
[patch.crates-io] | ||
p192 = { git = "https://github.com/RustCrypto/elliptic-curves.git" } | ||
p224 = { git = "https://github.com/RustCrypto/elliptic-curves.git" } | ||
sm2 = { git = "https://github.com/RustCrypto/elliptic-curves.git" } | ||
|
||
# https://github.com/RustCrypto/KDFs/pull/108 | ||
concat-kdf = { git = "https://github.com/RustCrypto/KDFs.git" } | ||
|
||
cfb-mode = { git = "https://github.com/RustCrypto/block-modes.git" } | ||
|
||
# https://github.com/RustCrypto/RSA/pull/467 | ||
rsa = { git = "https://github.com/baloo/RSA.git", branch = "baloo/oaep/non-string-label" } | ||
|
||
# https://github.com/RustCrypto/traits/issues/1738 | ||
# https://github.com/RustCrypto/traits/pull/1742 | ||
# Pending release of crypto-common 0.2.0-rc.2 | ||
# Pending release of digest 0.11.0-rc.0 | ||
crypto-common = { git = "https://github.com/RustCrypto/traits.git" } | ||
digest = { git = "https://github.com/RustCrypto/traits.git" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we waiting for these to be released on crates.io?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm relying on a bunch of pre-releases crates here. The kbkdf we just finished and released the first version of, but that relies on a bunch of pre-release or rc crates.
I've requested a new feature in the trait system for like detecting weak keys and be in compliance with the spec, probably safe to ignore that for now (and not merge this PR) until we have stable release.
The last huge pain point in the release cycle is a major rewrite of RSA, but I expect it to merge soon, and for the whole ecosystem to bump up.
let cred = vec![1, 2, 3, 4, 5]; | ||
let expected = Digest::try_from(vec![1, 2, 3, 4, 5]).unwrap(); | ||
|
||
let (credential_blob, secret) = utils::make_credential_ecc::<_, sha2::Sha256, aes::Aes128>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your question about this test in particular, or about the interface of make_credential_ecc
, and whether we can deduce the type params from the inputs?
I think generally you should be able to deduce the hash and the cipher for the EK if you know the nature of the public key, for example by doing the reverse of the mapping done here: https://github.com/parallaxsecond/rust-tss-esapi/blob/main/tss-esapi/src/abstraction/ek.rs
fe9fdd5
to
75825af
Compare
75825af
to
e26f3d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of a skim-review, most of this looks like a really good change, especially going in the direction of rustcrypto.
The biggest issue with the rustcrypto ecosystem is the use of tiny crates, so finding a way to keep all the versions in lock-step will be a thing to consider in future.
I'm fairly involved in RustCrypto, would happily handle the versions. |
Realistically the issue is an external consumer of tss-esapi here - they need to keep everything in lockstep, and over what ... 5, 10, 15 crates? It's a huge ask. rust-crypto desperately needs a glue crate that keeps everything in version, especially the traits and to aid discoverability of what is / isn't possible. I think that's what's needed here. Imagine I was a consumer tss-esapi, well now suddenly I need to keep all my crates in lockstep to tss-esapi versions, and wait for them to upgrade, but what if I have something conflicting? |
Signed-off-by: Arthur Gautier <[email protected]>
Signed-off-by: Arthur Gautier <[email protected]>
e26f3d0
to
406f62f
Compare
This sits on top of #537, please ignore the first commits.
This brings support for a pure rust implementation of make credentials which will not involve the TPM or
tpm2-tss
.Fixes #160