Introduced a rusty AES256 module with API similar to the one I introduced in web/native for encrypted media.
It's needed for ENG-4812 (next diff).
Details
Unit test, also tested in tne next diff.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Adding tomek as blocking because of new deps. Left some comments regarding the rust code side, but I'm not really familiar with crypto so hoping someone else will also look at it
services/comm-services-lib/src/crypto.rs | ||
---|---|---|
1–2 | I would personally move this submodule into a separate file | |
12–13 | I see that there are some traits with definitions for these values in aes-gcm. Is there maybe a way for these to use constants from the crate instead of hardcoding numbers here? | |
36–39 | Should we additionaly add/ change it into a EncryptionKey::new? | |
51–52 | I might be missing something but I don't think it matches the code | |
77–78 | pub fn decrypt<'buf>( ciphertext: &'buf mut [u8], key: &EncryptionKey, ) -> Result<&'buf mut [u8], aes_gcm::Error> { if ciphertext.len() < NONCE_LEN + TAG_LEN { return Err(aes_gcm::Error); } let cipher = Aes256Gcm::new(key.as_ref()); let (rest, tag) = ciphertext.split_at_mut(ciphertext.len() - TAG_LEN); let (nonce, buffer) = rest.split_at_mut(NONCE_LEN); let nonce = GenericArray::from_slice(nonce); let tag = GenericArray::from_slice(tag); cipher.decrypt_in_place_detached(nonce, b"", buffer, tag)?; Ok(buffer) } This passes the tests, but I'm not really familiar with crypto and I'm just following the types and your code comments so it might be completely wrong 😅 (also it's a bit weird to use) |
actual code looks fine to me but i don't really understand the motivation for this work. i've left a question on ENG-4812; we can discuss there
services/comm-services-lib/src/crypto.rs | ||
---|---|---|
74 | are we avoiding introducing a custom error type to prevent a side channel attack? |
services/comm-services-lib/src/crypto.rs | ||
---|---|---|
1–2 | good idea | |
12–13 | I couldn't figure it out, these crypto types are weird | |
36–39 | good idea | |
51–52 | good catch | |
74 | No, I haven't considered any "side channel attacks", whatever they are. It's a zero-sized error type, it gives no information about failure reason. It doesn't matter if we reuse it or introduce a custom one. | |
77–78 | Yeah, it's weird to use. I'd stick with the current approach |