Added helper methods and changed safety of size macros #55
Conversation
psa-crypto-sys/src/extras.rs
Outdated
| i.e. the bit size of the order of the curve's coordinate field. When m is not a multiple of 8, the byte containing the most | ||
| significant bit of the shared secret is padded with zero bits. | ||
| */ | ||
| (key_bits - 1) / 9 // Round the division up |
There was a problem hiding this comment.
What if key_bits is 0? This might panic in debug mode and overflow in release mode. I propose the following:
(key_bits + 7) / 8
or even better using checked_add for overflows. We can choose and document to return 0 if an overflow would happen.
There was a problem hiding this comment.
Didn't think of that. Though, will a valid key ever have 0 bits? I like your proposal, nice and fast 🚀
| /// This does not match any PSA macro, it will be replaces by PSA_RAW_KEY_AGREEMENT_OUTPUT_SIZE once | ||
| /// mbedTLS adds support for it. | ||
| pub unsafe fn PSA_RAW_ECDH_KEY_AGREEMENT_OUTPUT_SIZE( | ||
| _key_type: psa_key_type_t, |
There was a problem hiding this comment.
Are you keeping it because PSA_RAW_KEY_AGREEMENT_OUTPUT_SIZE will use it?
There was a problem hiding this comment.
I was told by the psa crypto api guys to only bother implementing the ECDH one as Mbed TLS will gain the full macro before it gains support for FFDH. When Mbed TLS adds the macro, this will be removed from psa-crypto-sys.
There was a problem hiding this comment.
oh I meant specifically about the _key_type parameter that is not used, sorry.
There was a problem hiding this comment.
aaah ok. Yes you are right, PSA_RAW_KEY_AGREEMENT_OUTPUT_SIZE does require key_type so I thought I'd keep the parameters the same (even though the name is slightly different).
psa-crypto/src/types/key.rs
Outdated
| pub fn mac_length(self, mac_alg: Mac) -> Result<usize> { | ||
| self.compatible_with_alg(mac_alg.into())?; |
There was a problem hiding this comment.
Are you sure no-one is using this method?
| pub fn raw_key_agreement_output_size(self, alg: RawKeyAgreement) -> Result<usize> { | ||
| self.compatible_with_alg(KeyAgreement::Raw(alg).into())?; | ||
| Ok(unsafe { | ||
| psa_crypto_sys::PSA_RAW_ECDH_KEY_AGREEMENT_OUTPUT_SIZE( |
There was a problem hiding this comment.
Should we check that alg Is only ECDH?
sbailey-arm
force-pushed
the
master
branch
2 times, most recently
from
ccd520f
to
01c06e3
Compare
Signed-off-by: Samuel Bailey <[email protected]>
| self.compatible_with_alg(alg.into())?; | ||
| Ok(unsafe { | ||
| psa_crypto_sys::PSA_AEAD_DECRYPT_OUTPUT_SIZE( | ||
| /*self.key_type.try_into() PSA API specifies including this parameter*/ |
There was a problem hiding this comment.
I don't understand very well what this comment is for?
There was a problem hiding this comment.
I thought it'd make it easier to see what's going on in the future when Mbed TLS is brought up to date with the spec. If it stops compiling, the person looking into it should see it and have a strong lead? I can remove it if its redundant or creates clutter.
|
|
||
| /// Sufficient buffer size for an encrypted message using the given aead algorithm | ||
| #[cfg(feature = "interface")] | ||
| pub fn aead_decrypt_output_size(self, alg: Aead, ciphertext_len: usize) -> Result<usize> { |
There was a problem hiding this comment.
Shouldn't this just be a method on Aead? I'm not sure why we'd have a compatibility check on this kind of method (at least, looking at the name of the method I wouldn't expect it to do anything like that), and the resulting size isn't for the alg in the policy either
There was a problem hiding this comment.
(I know there are other methods doing that already, I just noticed it now)
There was a problem hiding this comment.
I thought about doing that but I think I remember having this sort of conversation a while for a different helper method and it was decided that sticking it on Attributes was the most appropriate option at the time.
The PSA spec version does require key_type that is on Attributes and not on Aead, which is another reason I put it on Attributes (so it doesn't have to be moved in future to keep it in line with the existing methods).
There was a problem hiding this comment.
Ok, that makes sense if there's stuff from Attributes needed (or, will be)!
Added temporary implementation for getting output size of shared secret with EDCH algorithm (will be replaced with call to mbed TLS once it supports the macro).
Changed safety of some output size macro shim declarations as they have undefined return.
Signed-off-by: Samuel Bailey [email protected]