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

Added tests for all operations that were added that are supported by Mbed Crypto #50

Merged

Conversation

sbailey-arm
Copy link
Contributor

Tests added for:
psa_copy_key
psa_aead_encrypt
psa_aead_decrypt
psa_hash_compute
psa_hash_compare
psa_raw_key_agreement

Signed-off-by: Samuel Bailey samuel.bailey@arm.com

@sbailey-arm sbailey-arm force-pushed the add-all-missing-operations branch from c374842 to 21ca95a Compare August 6, 2020 09:52
@hug-dev hug-dev self-requested a review August 6, 2020 09:54
@hug-dev hug-dev added the enhancement New feature or request label Aug 6, 2020
psa-crypto/Cargo.toml Outdated Show resolved Hide resolved
psa-crypto/src/operations/key_management.rs Outdated Show resolved Hide resolved
psa-crypto/src/types/key.rs Show resolved Hide resolved
fn key_agreement() {
let alg = RawKeyAgreement::Ecdh;
let attributes = Attributes {
key_type: Type::EccKeyPair {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So key agreement can also be done on ECC keys? Maybe we need to update this code then.
Is it the case that Ecdh can be used with ECC keys where Ffdh can be used with DH keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ECC supports key agreement. Not sure about second question, maybe @ionut-arm has an idea? It also doesn't look like Mbed Crypto supports FFDH at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and yes. I actually asked a similar question back in the day and I think they've included clarification on this in subsequent releases of the spec - found it weird that they didn't explicitly specify that ECC keys are used for ECDH when they have a DH key type.

The "classical" Diffie-Hellman (over finite fields) uses DH keys and is represented by FFDH, the elliptic-curve variant uses ECC keys and is represented by ECDH.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that code seems wrong, good spot!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the clarification! In that case, could you update it as part of this PR @sbailey-arm please 😃 ?

@sbailey-arm sbailey-arm force-pushed the add-all-missing-operations branch from 21ca95a to b63ffba Compare August 6, 2020 13:54
..
}) => true,
Algorithm::AsymmetricSignature(sign_alg) => sign_alg.is_ecc_alg(),
Algorithm::AsymmetricEncryption(_) => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think I told you something wrong, there don't seem to be any asymmetric encryption algorithms for ECC keys.

@sbailey-arm sbailey-arm force-pushed the add-all-missing-operations branch from b63ffba to 730a3d7 Compare August 6, 2020 15:44
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests!

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting in the effort!

psa-crypto/src/operations/key_management.rs Outdated Show resolved Hide resolved
Comment on lines +262 to +277
Type::EccKeyPair { .. } | Type::EccPublicKey { .. } => match alg {
Algorithm::KeyAgreement(KeyAgreement::Raw(RawKeyAgreement::Ecdh))
| Algorithm::KeyAgreement(KeyAgreement::WithKeyDerivation {
ka_alg: RawKeyAgreement::Ecdh,
..
}) => true,
Algorithm::AsymmetricSignature(sign_alg) => sign_alg.is_ecc_alg(),
_ => false,
},
Type::DhKeyPair { .. } | Type::DhPublicKey { .. } => {
if let Algorithm::KeyAgreement(_) = alg {
if let Algorithm::KeyAgreement(KeyAgreement::Raw(RawKeyAgreement::Ffdh))
| Algorithm::KeyAgreement(KeyAgreement::WithKeyDerivation {
ka_alg: RawKeyAgreement::Ffdh,
..
}) = alg
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

…Mbed Crypto

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>
@sbailey-arm sbailey-arm force-pushed the add-all-missing-operations branch from 730a3d7 to 7444a77 Compare August 10, 2020 15:40
@ionut-arm ionut-arm merged commit a007a30 into parallaxsecond:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants