Page MenuHomePhabricator

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

Authored by bartek on Mar 8 2023, 10:13 AM.
Tags
None
Referenced Files
F3373471: D7007.id23672.diff
Tue, Nov 26, 9:53 AM
F3373455: D7007.id23857.diff
Tue, Nov 26, 9:47 AM
F3373311: D7007.id23542.diff
Tue, Nov 26, 8:58 AM
Unknown Object (File)
Fri, Nov 22, 7:08 PM
Unknown Object (File)
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
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
No Lint Coverage
Unit
No Test Coverage

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

Should we be breaking this into three "pieces"?

  • IV
  • Ciphertext
  • GCM tag
64–66

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

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