-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add support for elliptic curves for transient context #69
Conversation
src/utils/mod.rs
Outdated
/// Builder for `TPMS_ECC_PARMS` values. | ||
// Most of the field types are from bindgen which does not implement Debug on them. | ||
#[allow(missing_debug_implementations)] |
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.
This is no longer the case.
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.
Nice! Does that mean we could remove this allow
?
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.
Yes, and put Debug
in derive
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.
Thanks a lot for adding support for elliptic curves! That is a huge improvements!
I put some comments on what I can understand.
@@ -5,3 +5,4 @@ Cargo.lock | |||
*DS_Store | |||
*.patch | |||
*NVChip | |||
.vscode |
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.
You wouldn't need that with vim
😁
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.
Sorry, I can't hear you over the sound of my IDE checking for and displaying any cargo issues automatically 😛
/// Size of key in bits | ||
/// | ||
/// Can only be one of: 1024, 2048, 3072 or 4096 | ||
size: u16, |
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.
Could we have a RsaSize
enumeration for those four possible values so that the check line 104 is no longer needed?
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 know, tried that with Cipher
some time ago, but it feels overkill tbh... Could change if you want. Also the variant names feel weird af because you can't start a name with digits.
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.
I think it is worth to do it!
We could write the number in clear letters like:
enum RsaSize {
OneThousandTwentyFour,
TwoThousandFortyEight,
...
(joking)
Actually it could be possible
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, for the symmetric ones I went for Bits128
, Bits192
... but anyway... I'll skip it for now
src/utils/mod.rs
Outdated
/// Builder for `TPMS_ECC_PARMS` values. | ||
// Most of the field types are from bindgen which does not implement Debug on them. | ||
#[allow(missing_debug_implementations)] |
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.
Nice! Does that mean we could remove this allow
?
|
||
impl TpmsEccParmsBuilder { | ||
/// Create parameters for a restricted decryption key (i.e. a storage key) | ||
pub fn new_restricted_decryption_key(symmetric: Cipher, curve: EllipticCurve) -> Self { |
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.
What is the justification for the decryption one to be restricted and the signing one to be unrestricted?
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.
Restricted decryption keys are essentially Parent keys - they could be either used for derivation or storage of other object underneath it. The ECC one isn't used like that, so I could remove this
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.
From the spec:
A hierarchy is rooted in a secret seed key, kept in the TPM. To create a hierarchy of keys, the seed key (Primary Seed) is used to generate a key that uses a specific set of algorithms. If this key is a restricted decryption key, then it is a Parent Key. If it is not a Derivation Parent, then it is a Storage Parent under which other objects may be created or attached.
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.
Ahhh got it thanks!
if self.for_signing | ||
&& scheme.scheme != TPM2_ALG_ECDSA | ||
&& scheme.scheme != TPM2_ALG_ECDAA | ||
&& scheme.scheme != TPM2_ALG_SM2 | ||
&& scheme.scheme != TPM2_ALG_ECSCHNORR | ||
{ | ||
return Err(Error::local_error(WrapperErrorKind::InconsistentParams)); | ||
} | ||
|
||
if self.for_decryption | ||
&& scheme.scheme != TPM2_ALG_SM2 | ||
&& scheme.scheme != TPM2_ALG_ECDH | ||
&& scheme.scheme != TPM2_ALG_ECMQV | ||
&& scheme.scheme != TPM2_ALG_NULL | ||
{ | ||
return Err(Error::local_error(WrapperErrorKind::InconsistentParams)); | ||
} |
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.
I ask this mostly because I am not an expert but it seems like here we are checking that the appropriate algorithm is select for each key type. Does that mean that different algorithms are used for signing and verification?
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, for_signing
actually means for_signing_and_verification
and decryption is the same with encryption. It essentially specifies what the private key is used for.
This split here is because some scheme are for signing and others are for encryption. SM2
can do both
src/utils/mod.rs
Outdated
| AsymSchemeUnion::RSAES | ||
| AsymSchemeUnion::RSAOAEP(_) | ||
| AsymSchemeUnion::ECMQV(_) => { | ||
Err(Error::local_error(WrapperErrorKind::InconsistentParams)) |
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.
Should that be UnsupportedParam
?
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.
This is in a impl TryFrom<Signature> for TPMT_SIGNATURE
- meaning that only asymmetric schemes that are for signing make sense here; if the scheme on the Signature
is an encryption one then there's an inconsistency.
That does, however, bring the question of whether the AsymSchemeUnion
should be split into multiple enums - AsymSignScheme
and AsymEncryptScheme
, maybe even further, RsaSignScheme
, EccSignScheme
etc.
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.
This is in a impl TryFrom for TPMT_SIGNATURE - meaning that only asymmetric schemes that are for signing make sense here; if the scheme on the Signature is an encryption one then there's an inconsistency.
That makes sense!
That does, however, bring the question of whether the AsymSchemeUnion should be split into multiple enums - AsymSignScheme and AsymEncryptScheme, maybe even further, RsaSignScheme, EccSignScheme etc.
That would mean that the scheme
field of Signature
could be AsymSignScheme
so you would not even have to do that check?
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.
That would mean that the scheme field of Signature could be AsymSignScheme so you would not even have to do that check?
True, but I've also realised that this would only work if we keep separate methods for creating/using signing and decryption keys. Otherwise in the TransientKeyContext
, for example, we'd have to have different params types or variants for signing and encryption.
I'm not a huge fan of it, but we can keep the thing as is and simply have these checks here - we have to do a full match
in most cases anyway. I could implement methods is_signing
and is_decryption
on AsymSchemeUnion
to make checks faster.
pub fn create_unrestricted_signing_ecc_public( | ||
scheme: AsymSchemeUnion, | ||
curve: EllipticCurve, | ||
) -> Result<TPM2B_PUBLIC> { |
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 TPM2B_PUBLIC
the last structure that we use and do not abstract? (Just for my information)
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, TPM2B_PUBLIC
is really deeply nested and convoluted - I guess it could be made up of a hierarchy of Rust native types, but it would be a large effort to allow all the possibilities that the original interface allows, while making it easier for a developer to build a structure that just works™️.
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.
Got it.
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.
Approving! I'll let you decide if you want to make the changes discussed about creating new types, and if you do, if you want to make them in this PR or a new one.
This commit adds support for elliptic curve handling of the TransientKeyContext. It also adds and improves many of the underlying structures used to access or provision said keys. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
In order to execute the tests under different single providers, under all providers and for cross-compilation, split the tests directory into sub-directories. Each one contains a specific configuration file to run the tests under. Please check issue parallaxsecond#69 for details. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
This commit adds support for elliptic curve handling of the
TransientKeyContext.
It also adds and improves many of the underlying structures used to
access or provision said keys.
Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com