Page MenuHomePhabricator

[services-lib] Introduce crypto module
ClosedPublic

Authored by bartek on Aug 30 2023, 1:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 4:08 AM
Unknown Object (File)
Sun, Nov 10, 12:39 PM
Unknown Object (File)
Thu, Nov 7, 6:40 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:46 PM
Subscribers

Details

Summary

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).

Test Plan

Unit test, also tested in tne next diff.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Aug 30 2023, 2:26 PM
bartek added inline comments.
services/comm-services-lib/src/crypto.rs
77–78 ↗(On Diff #30606)

I tried to avoid copying here too, but I wasn't able to do so without unsafe code or accessing crate-private APIs.

services/reports/Cargo.toml
19 ↗(On Diff #30606)

I should've done this in the next diff. I'll move it before landing

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 ↗(On Diff #30606)

I would personally move this submodule into a separate file

12–13 ↗(On Diff #30606)

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 ↗(On Diff #30606)

Should we additionaly add/ change it into a EncryptionKey::new?

51–52 ↗(On Diff #30606)

I might be missing something but I don't think it matches the code

77–78 ↗(On Diff #30606)
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)

Dependencies look ok

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 ↗(On Diff #30606)

are we avoiding introducing a custom error type to prevent a side channel attack?

This revision is now accepted and ready to land.Aug 31 2023, 5:49 PM
bartek marked an inline comment as done.

Rebase, apply feedback

services/comm-services-lib/src/crypto.rs
1–2 ↗(On Diff #30606)

good idea

12–13 ↗(On Diff #30606)

I couldn't figure it out, these crypto types are weird

36–39 ↗(On Diff #30606)

good idea

51–52 ↗(On Diff #30606)

good catch

74 ↗(On Diff #30606)

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 ↗(On Diff #30606)

Yeah, it's weird to use. I'd stick with the current approach

This revision was automatically updated to reflect the committed changes.