Use infallible conversion into instead of try_into #747
Conversation
A new clippy lint https://rust-lang.github.io/rust-clippy/master/index.html#/unnecessary_fallible_conversions has been added to rust 1.75.0 that is causing the CI failure. The use of try_into when into is available will be reported by the lint now. Signed-off-by: Gowtham Suresh Kumar <[email protected]>
gowthamsk-arm
force-pushed
the
fix_tryinto
branch
from
0f591fd to
7ec6d4c
Compare
|
Nightly CI failure: https://github.com/parallaxsecond/parsec/actions/runs/7391951129 |
|
|
||
| impl Context { | ||
| pub fn asym_encrypt( | ||
| &self, | ||
| key_id: u32, | ||
| alg: AsymmetricEncryption, | ||
| mut plaintext: Vec<u8>, |
There was a problem hiding this comment.
I am assuming zeroisation is no longer required and we trust the vector data to be the responsibility of the caller?
There was a problem hiding this comment.
zeroisation was only happening when we would get an error from try_into(). With into() there is no error scenario so we would never zeroise. So it would be the caller's responsibility to clear as it is with the positive case.
There was a problem hiding this comment.
Hmm, what I find interesting is that we didn't zeroize it after sending the request, even though we don't pass ownership of the plaintext data, just a reference to it. Maybe we should do it before returning, regardless of whether it's an exception or not. Or maybe we didn't do it for some reason and I forgot the reason 🤔
Would it be possible for you to quickly check if zeroizing at the end breaks anything?
There was a problem hiding this comment.
I thought all the structs from ts_protobuf have Drop trail implemented here. But I can only see Drop being implemented for ImportKeyIn, ExportKeyOut, SignHashIn, VerifyHashIn. For these structs the drop method takes care of the zeroise part.
Not sure why AsymmetricEncryptIn and AsymmetricDecryptIn don't have them.
There was a problem hiding this comment.
The test patch seems to work fine and there are no failures reported in the CI jobs for trusted services backend.
There was a problem hiding this comment.
I would be more inclined for the Drop method of the involved types to handle the zeroization. Could you check if this is possible and easy to do? I'm approving in case it's not possible/easy.
gowthamsk-arm
force-pushed
the
fix_tryinto
branch
from
6bd36ea to
7ec6d4c
Compare
|
|
||
| impl Context { | ||
| pub fn asym_encrypt( | ||
| &self, | ||
| key_id: u32, | ||
| alg: AsymmetricEncryption, | ||
| mut plaintext: Vec<u8>, |
There was a problem hiding this comment.
I would be more inclined for the Drop method of the involved types to handle the zeroization. Could you check if this is possible and easy to do? I'm approving in case it's not possible/easy.
gowthamsk-arm
force-pushed
the
fix_tryinto
branch
2 times, most recently
from
cd2df29 to
b0b5b84
Compare
|
CI run failure for the Cargo deny job is expected and @tgonzalezorlandoarm has a fix for it. |
…etricDecryptIn The drop functions zeroise the ciphertext, plaintext and salt information to avoid leakage of any confifdential information. Signed-off-by: Gowtham Suresh Kumar <[email protected]>
gowthamsk-arm
force-pushed
the
fix_tryinto
branch
from
b0b5b84 to
7a5f3e4
Compare
A new clippy lint https://rust-lang.github.io/rust-clippy/master/index.html#/unnecessary_fallible_conversions has been added to rust 1.75.0 that is causing the CI failure. The use of try_into when into is available will be reported by the lint now.
Signed-off-by: Gowtham Suresh Kumar [email protected]