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 asymmetric encrypt and decrypt to Mbed Crypto provider #196

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

sbailey-arm
Copy link
Contributor

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

@sbailey-arm
Copy link
Contributor Author

This PR won't pass CI until parsec-interface-rs and parsec-client-rust have been updated on creates.io. Will mark as ready for review once they have been updated 😃.

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.

Looking good!

@@ -12,3 +12,4 @@ This file aims to acknowledge the specific contributors referred to in the "Cont
* Ionut Mihalcea (@ionut-arm)
* Hugues de Valon (@hug-dev)
* Jesper Brynolf (@Superhepper)
* Samuel Bailey (@sbailey-arm)
Copy link
Member

Choose a reason for hiding this comment

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

👌

Copy link
Member

Choose a reason for hiding this comment

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

🌞

Cargo.toml Outdated
zeroize = { version = "1.1.0", features = ["zeroize_derive"] }
secrecy = { version = "0.6.0", features = ["serde"] }
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? I didn't see any direct uses of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I have removed it. Wonder if there's a clippy setting to catch that...

Copy link
Member

Choose a reason for hiding this comment

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

WIP it seems

Copy link

Choose a reason for hiding this comment

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

That PR only provides the functionality of cargo-udeps in upstream tools. You can install it today!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @est31! We should definitely add that on our CIs!

Comment on lines 33 to 36
let salt_buff = match &op.salt {
Some(salt) => Some(salt.as_slice()),
None => None,
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this what you're trying to achieve here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's quite it. op.salt is an Option<Zeroizing<Vec<u8>>> and I need salt_buff to be Option<&[u8]>. There may still be some more concise way of writing it that I'm not aware of though.

Copy link
Member

Choose a reason for hiding this comment

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

Huh! I thought that since Zeroizing implements Deref, it would work - actually, it seems as_deref is what I should've recommended, though it doesn't solve the whole problem either - op.salt.as_deref() yields Option<&Vec<u8>>

}
Err(error) => {
let error = ResponseStatus::from(error);
format_error!("Encrypt status: {}", error);
Copy link
Member

Choose a reason for hiding this comment

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

The format_error macro doesn't need the {} part of the "format string", that gets added inside the macro!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's why the log outputs have been a bit odd...!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about in this case?

format_error!(
       "Error {} when opening a persistent Mbed Crypto key.",
       e
);

Copy link
Member

Choose a reason for hiding this comment

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

Ooops! I didn't notice that one when it went in... I wrote format_error to always log in the same format "Brief description of problem; error: {}" - the last bit is only written when the config flag for verbose logging is on. You can probably change that to ("Failed to open persistent Mbed Crypto key", e)

}
Err(error) => {
// When dropped, will zero buffer incase anything was written before error
let _ = Secret::new(plaintext);
Copy link
Member

Choose a reason for hiding this comment

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

Easier/quicker to just plaintext.zeroize() here (though you need to import the Zeroize trait).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh, I didn't know you could do that 🤯! How does it work? plaintext is a regular Vec<u8>' and I can't see anything obvious in the documentation that states Zeroizableis implemented forVec. Is it u8implementsDefaultIsZeros, so implements Zeroizablein the blanket implementation andVec` 'inherits' (derefs?) that?

Copy link
Member

Choose a reason for hiding this comment

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

Zeroize is implemented on Vecs - it's actually what you were using in the other PR when doing plaintext.zeroize() for the protobuf types - those types don't use Zeroizing or anything from the crate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah... 🤦‍♂️

Comment on lines 97 to 180
fn asym_encrypt_verify_decrypt_with_rsa_crate() {
let key_name = String::from("asym_encrypt_verify_decrypt_with_rsa_crate");
let mut client = TestClient::new();

client.generate_rsa_encryption_keys_rsapkcs1v15crypt(key_name.clone()).unwrap();
let pub_key = client.export_public_key(key_name.clone()).unwrap();

let rsa_pub_key = RSAPublicKey::from_pkcs1(&pub_key).unwrap();
let ciphertext = rsa_pub_key.encrypt(&mut OsRng, PaddingScheme::new_pkcs1v15_encrypt(), &PLAINTEXT_MESSAGE).unwrap();

let plaintext = client.asymmetric_decrypt_message_with_rsapkcs1v15(
key_name.clone(),
ciphertext,
).unwrap();

assert_eq!(&PLAINTEXT_MESSAGE[..], &plaintext[..]);

}
Copy link
Member

Choose a reason for hiding this comment

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

🙆‍♂️ niiiiiice!

Comment on lines 19 to 45
if client.provider() != Some(ProviderID::MbedCrypto) {
// Not supported by current provider
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a neater way of doing this?
Would be nice if the tests could check which operations the current provider supports and only test those.

Copy link
Member

Choose a reason for hiding this comment

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

You can always put an #[cfg(feature = "mbed-crypto-provider")] on the test

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is in the e2e crate... Hmmm

Copy link
Member

@ionut-arm ionut-arm Jul 7, 2020

Choose a reason for hiding this comment

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

I don't think the if is 100% correct anyway, because you could have multiple providers set up and the client chooses a different one, doesn't mean the Mbed Crypto one isn't available. Maybe we should change the set_provider method (on the test client) to check if the provider actually exists, and returns an error if not. Then we can check on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

client chooses a different one

Are the tests not done in isolation? As in, there will only ever be one provider at once during testing?

Maybe we should change the set_provider method (on the test client) to check if the provider actually exists, and returns an error if not. Then we can check on that.

So have set_provider check basic_client.list_providers to see if that provider exists (in this case, MbedCrypto) and if it does, it returns Ok and we can run the tests?

Copy link
Member

Choose a reason for hiding this comment

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

Are the tests not done in isolation? As in, there will only ever be one provider at once during testing?

Yeah, that's true, this is in the per_provider test thing - forget that.

I think we can do it in another way, actually, makes it more scalable - call list_opcodes and check if the operation you want is supported. If it isn't, return. It's not ideal because it will still appear as a test even when it's not supported, but there's no need to do anything to enable the test when you implement it in some new provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can do it in another way, actually, makes it more scalable - call list_opcodes and check if the operation you want is supported. If it isn't, return. It's not ideal because it will still appear as a test even when it's not supported, but there's no need to do anything to enable the test when you implement it in some new provider.

That sounds more like what I was thinking. The other two solutions don't deal with the fact that the tests are still run either. Could do with a way of forcing text output even if --show-output isn't specified, so we can at least say 'Hey, this provider doesn't actually support this operation'. I can have a poke tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it might be worth to change this check to:

if client.list_opcodes(client.provider().unwrap())?.contains(Opcode::PsaAsymmetricEncrypt) {
    return;
}

We could even create a method out of that snippet in the client:

fn is_operation_supported(&self, op: Opcode) -> bool {
}

@sbailey-arm sbailey-arm force-pushed the add-asym-encryption branch 3 times, most recently from 1f9f841 to 4b2fa81 Compare July 9, 2020 09:42
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 a lot! That looks very good. A few comments but otherwise it's ok.

@@ -12,3 +12,4 @@ This file aims to acknowledge the specific contributors referred to in the "Cont
* Ionut Mihalcea (@ionut-arm)
* Hugues de Valon (@hug-dev)
* Jesper Brynolf (@Superhepper)
* Samuel Bailey (@sbailey-arm)
Copy link
Member

Choose a reason for hiding this comment

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

🌞

Comment on lines 19 to 45
if client.provider() != Some(ProviderID::MbedCrypto) {
// Not supported by current provider
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it might be worth to change this check to:

if client.list_opcodes(client.provider().unwrap())?.contains(Opcode::PsaAsymmetricEncrypt) {
    return;
}

We could even create a method out of that snippet in the client:

fn is_operation_supported(&self, op: Opcode) -> bool {
}

app_name: ApplicationName,
op: psa_asymmetric_encrypt::Operation,
) -> Result<psa_asymmetric_encrypt::Result> {
info!("Mbed Provider - Asym Encrypt");
Copy link
Member

Choose a reason for hiding this comment

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

I think those info! are not needed now that we have the ingress/egress trace everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should they be removed from the other operations too?

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind 👀

})
}
Err(error) => {
plaintext.zeroize();
Copy link
Member

Choose a reason for hiding this comment

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

Ah good point! Do you think, even in case of error, it might contain some decrypted data? Does the spec say anything about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm it doesn't mention it in the spec that I can see. A quick look at the mbedtls code shows that it only copies into plaintext buffer if successful.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I reckon that is the safe, default, behaviour that you would expect. We can remove the zeroize here I think.

@sbailey-arm sbailey-arm force-pushed the add-asym-encryption branch 2 times, most recently from 44f6795 to e45d0e0 Compare July 9, 2020 11:11
Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>
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 adressing the comments!

@@ -25,8 +25,6 @@ impl Pkcs11Provider {
app_name: ApplicationName,
op: psa_sign_hash::Operation,
) -> Result<psa_sign_hash::Result> {
info!("Pkcs11 Provider - Asym Sign");
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks for removing those as well.

@hug-dev hug-dev merged commit 0c61762 into parallaxsecond:master Jul 9, 2020
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.

4 participants