Added all missing ops listed in Service API that Mbed Crypto support … #48
Conversation
|
While this one is somewhat okay, I strongly recommend adding them in smaller groups in the other repos, large PRs are very difficult to review. |
sbailey-arm
force-pushed
the
add-all-missing-operations
branch
2 times, most recently
from
63ea5a9
to
a6d032d
Compare
Ok. I was asked if it'd be possible to do them in one to make it easier to review, but perhaps this is a bit overboard...! I'll cut them up into smaller groups in the rest of the repos to make them more bitesize (infact @joechrisellis might be taking on some of the operations (e.g. the hash operations) too). |
|
My problem with large PRs is summarised by: But then again, everyone has their style of reviewing! If the change is really simple it could work, I guess |
Yep I see your point 😄. Maybe I should try reviewing some PRs 😅! Anyway, these changes shouldn't be too complex (if the multi-part ops are, let me know and I'll pull them out) and I'll split it up for the rest of the repos 🙂. |
| .to_result()?; | ||
| operation.alg.apply_inputs_to_op(&mut op) | ||
| } { | ||
| operation.abort(op)?; |
There was a problem hiding this comment.
Do you think we should abort on every possible exit step once we did the init?
There was a problem hiding this comment.
The key derivation setup, input bytes and input key state that if they return an error, you have to call abort.
There was a problem hiding this comment.
I see. That would mean that we have to add abort calls above in psa_key_derivation_setup fails
There was a problem hiding this comment.
It should do already, I think? The call to psa_key_derivation_setup and to apply_inputs_to_op (that method sort out calling the correct input functions) are wrapped in the same block that listens for an error. I was going to do it separately, then realised I could block them together.
There was a problem hiding this comment.
What about the error line 238, can't we escape the function without abort if we leave it at:
.to_result()??
psa-crypto/src/types/algorithm.rs
Outdated
| /// Enumeration of supported input data for different input steps | ||
| #[derive(Debug, Clone)] | ||
| pub enum Input<'a> { | ||
| /// Byte input for key derivation | ||
| Bytes(&'a [u8]), | ||
| /// Key input for key derivation | ||
| Key(Id), | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this used only in key derivation operations or also somewhere else? If not it is maybe better located inside the key derivation file.
There was a problem hiding this comment.
It already is 🤔. Not sure how it ended up in both! I've removed the one in algorithm.rs
| fn try_from(alg: psa_crypto_sys::psa_algorithm_t) -> Result<Self> { | ||
| if psa_crypto_sys::PSA_ALG_IS_FULL_LENGTH_MAC(alg) { | ||
| Ok(Mac::FullLength(alg.try_into()?)) | ||
| } else { |
There was a problem hiding this comment.
What if alg is not a MAC algorithm? What if it is a RSA algorithm for example? Do we need to check if it is a MAC algorithm, truncated or not before?
There was a problem hiding this comment.
Bit confused what you mean here. If it ends up in this method to begin with, it has to be a MAC algorithm, otherwise the code above this method isn't working correctly. The block starting on line 544 (the TryFrom<psa_crypto_sys::psa_algorithm_t> for Algorithm is what directs MAC algorithms to this particular method.
There was a problem hiding this comment.
it has to be a MAC algorithm
impls are "public", though - if someone has a psa_algorithm_t and they do Mac::try_from(alg), this gets called, regardless of what that alg was. You can throw in a PSA_ALG_IS_MAC to begin with
There was a problem hiding this comment.
Ah, I see what you mean. There should be a fail case for TryFrom, otherwise you'd use From.
| Mac::Truncated { | ||
| mac_alg: alg, | ||
| mac_length: length, | ||
| // The following call is NOT currently checked. If length is invalid, the return of this call is unspecified |
There was a problem hiding this comment.
I think it would make sense to restrict the Truncated instances to only the safe ones. Probably in another PR.
| // Copyright 2020 Contributors to the Parsec project. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! # PSA Operation types |
There was a problem hiding this comment.
Changed to PSA Key Derivation Operation Types.
| }; | ||
| let mut handle_for_new_key = 0; | ||
|
|
||
| let mut op: psa_crypto_sys::psa_key_derivation_operation_t = operation.clone().try_into()?; |
There was a problem hiding this comment.
Could operation be Copy? Is it that big?
There was a problem hiding this comment.
The reason for not implementing Copy on this was to stop the user ending up with an operation that they could use after it was aborted. The current way it is setup, once the user calls the key derivation operation, they cannot use that operation again.
When you call abort of the C struct, it resets it to default values, which are not 'valid'. I didn't want to make the types on operation Option<T> as I don't think it fits, so there is no way to reset the Rust operation struct, you have to start again, specifying the inputs you want at each stage for whatever algorithm you're using.
Another way of doing it would be to allow the user to keep their operation struct and re-use it. Its just with the abort in Mbed Crypto, I was trying to replicate it. Now that I think about it, I can't think of a reason why it shouldn't be done like that.
This is more of a Rust thing but I find that clone a bit annoying. I played around with making the TryFrom method take a reference, but then it got upset when I had the abort there as there was still a living borrow. I get why its complaining, but I know the borrow isn't used (e.g. stored) for use after TryFrom returns!
There was a problem hiding this comment.
to stop the user ending up with an operation that they could use after it was aborted.
They can still clone it, right? An alternative is to implement your own internal clone that's only visible to this crate.
Or a TryFrom<&KeyDerivationOperation> if that's possible, I think that's a thing
There was a problem hiding this comment.
Or a TryFrom<&KeyDerivationOperation> if that's possible, I think that's a thing
That's what I was referring to in this comment. You can have a TryFrom with a borrow, but that doesn't solve this problem
This is more of a Rust thing but I find that clone a bit annoying. I played around with making the TryFrom method take a reference, but then it got upset when I had the abort there as there was still a living borrow. I get why its complaining, but I know the borrow isn't used (e.g. stored) for use after TryFrom returns!
There was a problem hiding this comment.
What will happen for the slices when cloning? Will that make deep copies of them?
There was a problem hiding this comment.
Hmmm good question. Slice doesn't seem to implement Clone. It could copy the reference but then you could end up with a dangling pointer, which Rust doesn't allow 🤔.
| ) | ||
| }) | ||
| .to_result()?; | ||
| operation.abort(op)?; |
There was a problem hiding this comment.
Same question as previously, do you think we need to abort on every exit path?
There was a problem hiding this comment.
Yes. If you look at the last key derivation step, you have to call abort to clean up its resources.
| /// let my_key = key_management::generate(attributes, None).unwrap(); | ||
| /// let my_key_copy = key_management::copy_key(my_key, attributes, None); | ||
| /// ``` | ||
| pub fn copy_key(key_id_to_copy: Id, attributes: Attributes, id: Option<u32>) -> Result<Id> { |
There was a problem hiding this comment.
When thinking about it, we removed the "key" from all function names: this should probably be copy. Similar for export_key above, it should maybe be export. And purge below.
| /* | ||
| pub fn purge_key(key_id: Id) -> Result<()> { | ||
| initialized()?; | ||
| let handle = key_id.handle()?; | ||
| Status::from(psa_crypto_sys::psa_purge_key(handle)).to_result() | ||
| }*/ |
There was a problem hiding this comment.
Maybe it is better to still keep the function but have an empty implementation for now? Using a log to notice it, with the same comment that you have above.
There was a problem hiding this comment.
I've commented out the code and the example so now all it does is use the error macro. Is this what you meant?
psa-crypto/src/operations/mac.rs
Outdated
| /// # sign_message: true, | ||
| /// # ..Default::default() | ||
| /// # }, | ||
| /// # permitted_algorithms: Mac::FullLength(FullLengthMac::Hmac{hash_alg: Hash::Sha256}).into(), |
There was a problem hiding this comment.
Would be nice to have a From<FullLength> implementation on FullLengthMac to directly do:
FullLengthMac::Hmac{hash_alg: Hash::Sha256}.into()
There was a problem hiding this comment.
Thanks a lot for your work, that is amazing. I put some comments to start a discussion but I really appreciate the care you took making things look nice.
I have reviewed the last 8 files, I believe @ionut-arm will review the 7 first.
| /// Key derivation algorithm and inputs | ||
| pub alg: KeyDerivationWithInputs<'a>, | ||
| /// Maximum capacity of a key derivation operation | ||
| pub capacity: Option<usize>, |
There was a problem hiding this comment.
In the specs I can read:
For some algorithms, this step is mandatory because the output depends on the maximum capacity.
If we know for which ones this is mandatory, we can force it for them and leave it as Option for the other ones?
There was a problem hiding this comment.
I did look into doing exactly that but couldn't find any documentation that states for which algorithms capacity is mandatory 😞.
| /// Salt, used in the "extract" step. It is optional; if omitted, the derivation uses an empty salt. | ||
| /// Typically a direct input, can also be a key of type `RawData`. | ||
| salt: Option<Input<'a>>, | ||
| /// Secret, used in the "extract" step. Must be `Input::Key(Id)` if used with `psa_key_derivation_output_key`. |
There was a problem hiding this comment.
You write in multiple places:
Must be
Input::Key(Id)if used withpsa_key_derivation_output_key
Would there be a way to force that in the type system?
There was a problem hiding this comment.
I couldn't think of a clean way of doing it. An option would be to have it as a parameter on the method to force Input::Key(Id) but that seems a bit janky. I can have another think about it after everything else is sorted.
There was a problem hiding this comment.
I'm not sure I follow... KeyDerivationOperation is only usable with psa_key_derivation_output_key - or is there more to come?
|
|
||
| /// Enumeration of the step of a key derivation. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum DerivationStep { |
There was a problem hiding this comment.
I don't know about this one - not sure if Step is so common a term in this kind of context to be fully clear what it means without the Derivation part of the name
| /// HKDF algorithm. | ||
| Hkdf { | ||
| /// A hash algorithm to use | ||
| hash_alg: Hash, |
There was a problem hiding this comment.
Where is the hash alg actually used?
There was a problem hiding this comment.
When converting to FullLengthMac::Hkdf as it takes a hash algorithm (as does the C macro to build a Hkdf derivation algorithm).
psa-crypto/src/operations/mac.rs
Outdated
| let close_handle_res = key_id.close_handle(key_handle); | ||
| let final_mac_verify_res = match mac_verify_res { | ||
| Status::Success => Ok(true), | ||
| Status::Error(Error::InvalidSignature) => Ok(false), // Indicates operation successful but compare did not match |
There was a problem hiding this comment.
Since there is an InvalidSignature error, it could make sense to just return Result<()> from this function!
There was a problem hiding this comment.
I originally did that but thought since the operation is 'successful' in the event of an InvalidSignature error, it should tell the user that the operation completed successfully but the comparison did not match. I guess it doesn't actually matter that much and just returning the error is slightly simpler 🤔.
There was a problem hiding this comment.
I guess Result<Result<()>> is more correct, but fugly
| #[derive(Debug, Clone)] | ||
| pub struct KeyDerivationOperation<'a> { | ||
| /// Key derivation algorithm and inputs | ||
| pub alg: KeyDerivationWithInputs<'a>, |
There was a problem hiding this comment.
alg could be named inputs, KeyDerivationWithInputs could be named Inputs
|
|
||
| /// Key derivation operation for deriving keys from existing sources | ||
| #[derive(Debug, Clone)] | ||
| pub struct KeyDerivationOperation<'a> { |
There was a problem hiding this comment.
Still about naming: would it be nicer if this file is named key_derivation and this structure Operation?
There was a problem hiding this comment.
I think so, the shorter file name still conveys what's there. Just calling it Operation could be confusing for people if they have multiple different types of Operation in the same place, but they can just rename it themselves in the import statement.
psa-crypto-sys/src/c/shim.c
Outdated
| shim_PSA_ALG_IS_FULL_LENGTH_MAC (psa_algorithm_t alg) { | ||
| return PSA_ALG_IS_BLOCK_CIPHER_MAC (alg); |
There was a problem hiding this comment.
Is this a copy-pasta error or are cipher MACs defined to be full-length only? I can't see anything in that sense in the spec
There was a problem hiding this comment.
Yes, I think that is an error. The conversion code is going to need re-jigging if we don't have access to a macro that does something like the name of this one suggests 🤔.
There was a problem hiding this comment.
There is a macro PSA_ALG_FULL_LENGTH_MAC that converts any MAC algorithm (truncated or not) to its full length version - you could return something like PSA_ALG_FULL_LENGTH_MAC(alg) == alg?
There was a problem hiding this comment.
Ah yes, that's nice an elegant!
psa-crypto-sys/src/utils.rs
Outdated
| pub fn PSA_ALG_AEAD_TAG_TRUNCATED_LENGTH(aead_alg: psa_algorithm_t) -> usize { | ||
| const TAG_LENGTH_MASK: u8 = 0b00111111; // tag lengths are 6 bits in length | ||
| const PSA_V1_0_0_TAG_LENGTH_START_BIT: u32 = 16; // tag length at bit position [21:16] | ||
|
|
||
| let pre_mask_tag_length = aead_alg >> PSA_V1_0_0_TAG_LENGTH_START_BIT; | ||
|
|
||
| (pre_mask_tag_length as u8 & TAG_LENGTH_MASK) as usize | ||
| } |
There was a problem hiding this comment.
Could you add a simple test just below this?
psa-crypto/src/operations/hash.rs
Outdated
| Status::Success => Ok(true), | ||
| Status::Error(Error::InvalidSignature) => Ok(false), // Indicates operation successful but compare did not match | ||
| Status::Error(error) => Err(error), |
There was a problem hiding this comment.
Hmm, is this expected? As in, to differentiate between InvalidSignature and other errors, as I'm assuming that's the reason for returning a bool.
There was a problem hiding this comment.
Changed it to return Result<()> as with the other occurrence above.
There was a problem hiding this comment.
Did you 👀 ? Sorry if you did not push everything yet 😅
psa-crypto/src/operations/hash.rs
Outdated
| /// # use psa_crypto::operations::hash::{hash_compute, hash_compare}; | ||
| /// # use psa_crypto::types::algorithm::Hash; | ||
| /// # const MESSAGE: [u8; 32] = [ | ||
| /// # 0x69, 0x3E, 0xDB, 0x1B, 0x22, 0x79, 0x03, 0xF4, 0xC0, 0xBF, 0xD6, 0x91, 0x76, 0x37, 0x84, 0xA2, | ||
| /// # 0x94, 0x8E, 0x92, 0x50, 0x35, 0xC2, 0x8C, 0x5C, 0x3C, 0xCA, 0xFE, 0x18, 0xE8, 0x81, 0x37, 0x78, | ||
| /// # ]; |
There was a problem hiding this comment.
Any reason why these are "hidden" here but similar lines are visible in the AEAD examples?
There was a problem hiding this comment.
Not really! I'm not sure which bits should be hidden and which shouldn't. I was trying to make all 'obvious' code hidden with the less obvious code not. However, what 'obvious' actually means will depend on who you ask. E.g. I tried to make sure the bits that took a while to get working (e.g. correct key types, sizes, policy flags and algorithms) weren't hidden.
There was a problem hiding this comment.
I think it makes sense to hide the actual values/buffers, as the type really isn't something unusual or hard to grasp. Maybe the imports are actually useful is someone wants to just copy-paste code straight from the example, but yeah, really not sure.
As long as they're consistent, choose whatever makes sense to you!
psa-crypto/src/operations/aead.rs
Outdated
| /// let length = aead::aead_encrypt(my_key, alg, &NONCE, &ADDITIONAL_DATA, &INPUT_DATA, &mut output_buffer).unwrap(); | ||
| /// output_buffer.resize(length, 0); | ||
| /// ``` | ||
| pub fn aead_encrypt( |
There was a problem hiding this comment.
As Hugues suggested - these are already namespaced, you can just call them encrypt, decrypt - similarly for the hash ones
psa-crypto-sys/src/utils.rs
Outdated
|
|
||
| let pre_mask_tag_length = aead_alg >> PSA_V1_0_0_TAG_LENGTH_START_BIT; | ||
|
|
||
| (pre_mask_tag_length as u8 & TAG_LENGTH_MASK) as usize |
There was a problem hiding this comment.
Why is there a need for converting to u8 since you convert to usize immediately after? I think just & TAG_LENGTH_MASK is enough to keep only the lowest 6 bits set.
There was a problem hiding this comment.
Originally had that but compiler said no -
14 | (pre_mask_tag_length & TAG_LENGTH_MASK) as usize
| ^^^^^^^^^^^^^^^ expected `u32`, found `u8`
There was a problem hiding this comment.
And if you make const TAG_LENGTH_MASK: u32 = 0b111111 ?
|
Actually, a more primordial question - no tests? |
| pub(crate) fn abort( | ||
| self, | ||
| mut op: psa_crypto_sys::psa_key_derivation_operation_t, | ||
| ) -> Result<()> { | ||
| Status::from(unsafe { psa_crypto_sys::psa_key_derivation_abort(&mut op) }).to_result() | ||
| } |
There was a problem hiding this comment.
Why is this a method (instead of just a function on the type)? self isn't used anywhere, it would remove your need to clone the instance in some places
There was a problem hiding this comment.
So it consumes self, stopping the user from trying to reuse something that has been cleared. Think it might be better just to do it like attributes, let the user keep their un-cleared Rust operation struct while clearing the C version. I think it deviates from the PSA spec as the struct is supposed to be cleared (and we can't do that for reasons already mentioned) but I can't think of any major problem with it.
sbailey-arm
force-pushed
the
add-all-missing-operations
branch
from
a6d032d
to
040f48b
Compare
|
I've pulled |
Not yet. Didn't want to make this PR even bigger, but can add them here if required. |
sbailey-arm
force-pushed
the
add-all-missing-operations
branch
from
040f48b
to
6f20b1d
Compare
No, another PR with tests makes sense |
psa-crypto-sys/src/utils.rs
Outdated
| // Copyright 2020 Contributors to the Parsec project. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| #![allow(non_snake_case)] | ||
| /// Additional functionality required that Mbed Crypto does not provide |
There was a problem hiding this comment.
Maybe it is "PSA Crypto" does not provide?
Also it might be good to rename this file extra.rs. utils.rs often ends up being a pot-pourri of non-sorted things 😃
psa-crypto/src/operations/hash.rs
Outdated
| Status::Success => Ok(true), | ||
| Status::Error(Error::InvalidSignature) => Ok(false), // Indicates operation successful but compare did not match | ||
| Status::Error(error) => Err(error), |
There was a problem hiding this comment.
Did you 👀 ? Sorry if you did not push everything yet 😅
psa-crypto/src/operations/mac.rs
Outdated
| /// mac.resize(size, 0); | ||
| /// assert!(verify_mac(my_key, mac_alg, &MESSAGE, &mac)); | ||
| /// ``` | ||
| pub fn verify_mac(key_id: Id, mac_alg: Mac, input_message: &[u8], expected_mac: &[u8]) -> Result<bool> { |
There was a problem hiding this comment.
I guess this also should return Result<()>
There was a problem hiding this comment.
Yeah, sorry, I have changed these but my virtual machine has been taking the last few days off, must have been in its saved state which was lost. I'll re-change them 🙂
| ).to_result(); | ||
| let close_handle_res = key_id.close_handle(key_handle); | ||
| mac_verify_res?; | ||
| close_handle_res |
There was a problem hiding this comment.
How can this even compile ❓ Isn't close_handle_res of type Result<()>?
There was a problem hiding this comment.
Ah, it's not added as a module because Mbed Crypto does not support it!
…(plus some it doesn't) Signed-off-by: Samuel Bailey <[email protected]>
sbailey-arm
force-pushed
the
add-all-missing-operations
branch
from
6f20b1d
to
f4af888
Compare
…(plus some it doesn't) found here.
The goal of this PR is to add all operations listed in the Single-part Service API table that are currently supported by Mbed Crypto. Since there are more than a couple of additions, here is a breakdown:
psa_key_derivation_output_key- supported, added, *psa_copy_key- supported, addedpsa_purge_key- unsupported, addedpsa_hash_compute- supported, addedpsa_hash_compare- supported, addedpsa_mac_compute- unspported, added, ***psa_mac_verify- unsupported, added, ***psa_cipher_encrypt- unsupported, not addedpsa_cipher_decrypt- unsupported, not addedpsa_aead_encrypt- supported, added, ***psa_aead_decrypt- supported, added, ***psa_sign_message- unsupported, not addedpsa_verify_message- unsupported, not addedpsa_key_derivation_key_agreement- supported, added, *, **psa_raw_key_agreement- supported, addedsupported/unsupported - whether Mbed Crypto has support for this operation
added/not added - whether the operation has been added to psa-crypto in this PR
* - these don't actually appear to be single-part operations and probably shouldn't be in that Single-part operation API table. However,
psa_key_derivation_output_keywas almost complete before @hug-dev and I realised that so I completed them anyway.** -
psa_key_derivation_key_agreementappears as a separate operation in the API table. I took a different approach with it. It looks like just another input step so I've integrated it into thepsa_key_derivation_output_keyoperation.To give a quick overview of what goes on with key derivation in the PSA spec, you have to create an operation struct and then call multiple
inputfunctions on it in order to give the operation data (either byte buffer or a key) to use at each stepin the derivation process (which steps you need to provide data for and the format required is dependant on the derivation algorithm. After you've set it up, you call
psa_key_derivation_output_keyto perform the actual derivation.The general design of this particular multi-part operation is instead of having the user call multiple 'sub' operations, they provide all of the data upfront. A benefit of this is you can check before the operation is called
whether the correct data for the requested algorithm has been provided (or rather, you can't call it without providing the correct data). It started with just allowing byte buffers or key IDs, but as I mentioned, I expanded
it to include data required for a key agreement too.
*** - There is a potential problem with a truncated MAC. Creating a truncated MAC with the C macro is unspecified if 'mac_length is too small or too large for the specified MAC algorithm.'. The requirements for mac_length are 'This must be at most the full length of the MAC and must be at least an implementation-specified minimum. The implementation-specified minimum must not be zero.'.
The first requirement (must be at most the full length of the algorithm) should be easy enough to look up, but I can't find any indicators as to the min value that Mbed Crypto supports. The tests indicate that this value is 1, but I can't see any documentation for it.
We also don't currently do any checking on the values a user could put into our
Mac::Truncatedenum struct, which is what is passed to the macro. We could declare a separate Truncated struct, make the members private (outside of psa-crypto) to force users to useTruncated::newwhere we could do a runtime check on length, and have the enum hold that struct, but we still don't have anything to check againstThe same is true for Aead. This should perhaps be listed as an issue, but since I noticed it yesterday and the PR was ready, I included it here.
Signed-off-by: Samuel Bailey [email protected]