Import private key support for TPM provider #243
Conversation
There was a problem hiding this comment.
Thanks for this! I like the scaffolding you did, something we need to do/coordinate on all our providers.
I left a few comments and I have a request: could you please add a e2e test importing a RSA key pair? There might already be existing tests on that which are not activated for the TPM provider.
|
|
||
| let (int_bytes, _) = private_key | ||
| .public_exponent() | ||
| .split_at(std::mem::size_of::<u32>()); |
There was a problem hiding this comment.
split_at could panic if the public exponent is smaller than 4 bytes but I guess that's fine because of the validate_ functions above.
src/providers/tpm_provider/utils.rs
Outdated
| pub fn validate_private_key(private_key: &RSAPrivateKey, attributes: &Attributes) -> Result<()> { | ||
| // NOTE: potentially incomplete, but any errors that aren't caught here should be caught | ||
| // further down the stack (i.e. in the tss crate). | ||
| let key_prime = &private_key.primes()[0]; |
There was a problem hiding this comment.
Maybe it is better to use the fields of RSAPrivateKey directly
| Type::RsaKeyPair => self.psa_import_key_internal_rsa_keypair(app_name, op), | ||
| _ => { | ||
| error!( | ||
| "The TPM provider does not support the {:?} key type.", |
There was a problem hiding this comment.
As far as I know, the TPM provider does not support importing these other key types but it might support generating keys of those types
joechrisellis
force-pushed
the
import_private_key_support
branch
from
d164d5d to
d7f4cbe
Compare
src/providers/tpm_provider/utils.rs
Outdated
| /// Validates an RSAPrivateKey against the attributes we expect. Returns ok on success, otherwise | ||
| /// returns an error. | ||
| pub fn validate_private_key(private_key: &RSAPrivateKey, attributes: &Attributes) -> Result<()> { | ||
| // NOTE: potentially incomplete, but any errors that aren't caught here should be caught | ||
| // further down the stack (i.e. in the tss crate). | ||
| let key_prime = &private_key.prime_1; | ||
| let key_len = key_prime.len(); | ||
| if key_len != attributes.bits / 2 { | ||
| error!( | ||
| "The key prime is not of the expected size (expected {}, got {})", | ||
| attributes.bits / 2, | ||
| key_len, | ||
| ); | ||
| return Err(ResponseStatus::PsaErrorInvalidArgument); | ||
| } | ||
| Ok(()) | ||
| } |
src/providers/tpm_provider/utils.rs
Outdated
| let len = key_data.len(); | ||
|
|
||
| let key_bits = attributes.bits; | ||
| if key_bits != 0 && len * 8 != key_bits { |
There was a problem hiding this comment.
I'm wondering - given that the modulus is given as a vector of bytes, the assumed size is always a multiple of 8 (i.e. len * 8). But the size of the modulus could theoretically be something in between, like 1245, in which case the MSBits of the last byte won't be set. Now, given that the TPM will most likely reject that anyway (I think only a few values are accepted by the spec), should we reject it from here? The log messages below wouldn't be very helpful in this (rather edge) case
src/providers/tpm_provider/utils.rs
Outdated
| if len != 128 && len != 256 { | ||
| if crate::utils::GlobalConfig::log_error_details() { | ||
| error!( | ||
| "The TPM provider only supports 1024 and 2048 bits RSA public keys ({} bits given).", |
There was a problem hiding this comment.
Is this limitation in the TransientKeyContext? I think TPMs support one of 1024, 2048, 3072 or 4096
joechrisellis
force-pushed
the
import_private_key_support
branch
2 times, most recently
from
2cb263c to
fb747ab
Compare
Signed-off-by: Joe Ellis <[email protected]>
Signed-off-by: Joe Ellis <[email protected]>
joechrisellis
force-pushed
the
import_private_key_support
branch
4 times, most recently
from
3be245a to
e10d335
Compare
Signed-off-by: Joe Ellis <[email protected]>
Signed-off-by: Joe Ellis <[email protected]>
joechrisellis
force-pushed
the
import_private_key_support
branch
from
e10d335 to
8197ce9
Compare
| @@ -284,3 +321,61 @@ fn asym_encrypt_with_crate() { | |||
|
|
|||
| assert_eq!(&initial_plaintext[..], &plaintext[..]); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn import_private_key() { | |||
There was a problem hiding this comment.
Thanks for adding the test!
@ionut-arm is it fine for this test to be here or should we add it as an e2e test?
There was a problem hiding this comment.
Leave it there! I'll rebase my changes, though I won't finish today
No description provided.