IronCoreLabs / ironoxide

Rust SDK for IronCore Privacy Platform
https://docs.rs/ironoxide
GNU Affero General Public License v3.0
10 stars 3 forks source link

Don't clone plaintext on aes encryption #246

Closed giarc3 closed 2 years ago

giarc3 commented 2 years ago

PR really has 2 parts, so I broke them into 2 commits you can view separately. I think we should definitely keep 1, but we could decide to not do 2.

  1. crypto::aes::encrypt now takes Vec<u8> plaintext instead of &[u8]. The call sites were all already doing &plaintext.to_vec(), so they had owned data. And now we don't have to .to_owned() when encrypting.
  2. Changes our public functions like document_encrypt() to take Vec<u8> instead of &[u8]. Since this is public this would be a breaking change. But essentially if we need owned data, we should take owned data (instead of just internally cloning with .to_vec()). It'll instead be up to the caller whether or not they need to clone their data (and I think they frequently won't need to be using their unencrypted_data again after encrypting it).

Closes #3

BobWall23 commented 2 years ago

It looks like the change necessitated a lot of clone calls in code that was encrypting the data - so maybe it didn't remove the problem, it just moved it up to the consumer?

In some of the tests, instead of cloning the vector, the code could have just grabbed the length before calling encrypt, so maybe the clones could be avoided in more places.

giarc3 commented 2 years ago

After a few attempts to make the encrypt/decrypt functions more generic (InOut: AsMut<[u8]> + for<'in_out> Extend<&'in_out u8>), I was not able to make this work (especially not in an async context). However, I think that this PR is still an improvement as it stands now and is still worth merging. Previously on encrypt, we would take a borrowed byte slice, clone it into a Vec, then clone it again when encrypt extends the Vec. Now we take a Vec, and still have the clone due to the extend. If the consumer already has a Vec, we saved a clone of the plaintext. If the consumer has a byte slice or other byte representation, they will have to clone it themselves before passing it in, and we're in the same place as when we started. No better, no worse.

@coltfred @clintfred Please take another look and see if you agree with my assessment.

coltfred commented 2 years ago

I agree with your assessment. It is better and we can continue to make it better if we find a way to make the api more ergonomic in the future. For now it's an improvement where we use it (and will be for most people I suspect).