Page MenuHomePhabricator

[native][AES] Implement encrypt function on Android
ClosedPublic

Authored by bartek on Mar 8 2023, 10:13 AM.
Tags
None
Referenced Files
F3336524: D7007.id23672.diff
Thu, Nov 21, 2:42 PM
Unknown Object (File)
Thu, Nov 14, 7:37 PM
Unknown Object (File)
Sun, Nov 10, 5:54 PM
Unknown Object (File)
Fri, Nov 8, 5:35 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Subscribers

Details

Summary
Test Plan

The same as in D7005:

import * as AES from './utils/aes-crypto-module.js';

const key = AES.generateKey();
const plaintextBytes = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
const ciphertext = AES.encrypt(key, plaintextBytes);
console.log('Ciphertext', ciphertext);
// ensured that ciphertext is 12+10+16=38 bytes long (ciphertext.length==38)

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.Mar 8 2023, 10:43 AM

Accepting since a lot of the feedback was captured in the iOS diffs.

Just a quick inline question about iOS "compatibility" and changing condition from < to == and this should be good to land.

native/expo-modules/aes-crypto/android/src/main/java/app/comm/android/aescrypto/AESCryptoModule.kt
52–53 ↗(On Diff #23542)

Should we be breaking this into three "pieces"?

  • IV
  • Ciphertext
  • GCM tag
64–66 ↗(On Diff #23542)

Mentioned this in the corresponding iOS diff, but I think it'd be good to use == to be more precise if we're assuming that they should always be exactly the same length.

74–75 ↗(On Diff #23542)

Are these "bytes" going to be directly compatible with the .combined bytes from iOS? It seems like it's a standard format (iv || ciphertext || tag), but wasn't sure if there were any subtle differences or it's truly 1:1.

This revision is now accepted and ready to land.Mar 10 2023, 2:22 PM

Address code review feedback