[native][AES] Implement decrypt function on iOS
ClosedPublic
Actions

Authored by bartek on Mar 8 2023, 10:09 AM.

Details

Reviewers

marcin
atul

Commits

rCOMM868e5fda92f6: [native][AES] Implement decrypt function on iOS

Summary

This diff implements the decrypt() function on iOS.

Its desired behavior is described here: https://linear.app/comm/issue/ENG-388#comment-76a38890

Docs: https://developer.apple.com/documentation/cryptokit/aes/gcm/open(_:using:)
Inspired by: https://dev.to/craftzdog/how-to-encrypt-decrypt-with-aes-gcm-using-cryptokit-in-swift-24h1

Depends on D7005

Test Plan

Placed this e.g. in root component render function

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

const key = AES.generateKey();
console.log('Key', key);
const plaintextBytes = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
const ciphertext = AES.encrypt(key, plaintextBytes);
console.log('Ciphertext', ciphertext);
const decrypted = AES.decrypt(key, ciphertext);
console.log('Decrypted', decrypted); // should be [1,2,3...9,10]

should display sth like:

LOG  Key [82, 226, 211, 141, 62, 105, 93, 161, 238, 193, 163, 46, 151, 205, 133, 163, 175, 15, 110, 16, 139, 86, 150, 10, 26, 30, 179, 245, 221, 215, 19, 13]
LOG  Ciphertext [74, 52, 31, 132, 80, 213, 246, 255, 188, 231, 18, 27, 222, 224, 91, 53, 204, 198, 109, 53, 40, 194, 198, 149, 181, 34, 131, 102, 228, 246, 171, 190, 89, 232, 36, 74, 173, 211]
LOG  Decrypted [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Mar 8 2023, 10:09 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 8 2023, 10:09 AM

bartek added a child revision: D7007: [native][AES] Implement encrypt function on Android.Mar 8 2023, 10:13 AM

bartek mentioned this in D7008: [native][AES] Implement decrypt function on Android.Mar 8 2023, 10:15 AM

bartek edited the test plan for this revision. (Show Details)Mar 8 2023, 10:16 AM

Harbormaster completed remote builds in B17190: Diff 23541.Mar 8 2023, 10:25 AM

bartek published this revision for review.Mar 8 2023, 10:41 AM

Some questions inline. Definitely feel free to re-request review if there's something I'm missing.

native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
53	Already mentioned in D7005, but it's kind of confusing how `ciphertext` can mean multiple things. I'm guessing here `ciphertext` is `iv \|\| ciphertext \|\| tag` concatenation vs. "just the ciphertext"? Think some sort of `seal(...)`/`sealedData` (where `sealedData` = `iv \|\| ciphertext \|\| tag` concatenation) naming convention would make the distinction clearer?
55	It looks like this check handles one specific possible invalid `ciphertext` case. Not sure we need this, I'm fairly confident we can trust `AES.GCM.SealedBox(...)` to `throw` if `ciphertext` is invalid (this case + others)? Let me know if there's something I'm missing
56	Is there a reason we're doing a `>=` check instead of `==`? Seems like elsewhere we expect the sizes to be exact?
62–68	This looks correct, but I'm hesitant about deconstructing `cipherText` "manually" based on offsets. This case is pretty simple, but still seems like a brittle approach. Is there a reason we can't use the init<D>(combined: D) throws where D : DataProtocol initializer instead? According to https://developer.apple.com/documentation/cryptokit/aes/gcm/sealedbox/init(combined:): Creates a sealed box from the `combined` bytes of an authentication tag, nonce, and encrypted data. That sounds like exactly what we need? Is there something preventing us from using that initializer?
70	Personal style preference would be to pull `nonce` out to a separate line. Totally arbitrary preference, feel free to go with whatever you prefer

This revision now requires changes to proceed.Mar 9 2023, 2:47 PM

bartek added inline comments.Mar 13 2023, 4:12 AM

native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
53	Done
55	If this check fails, we don't even need to create `SealedBox` because it's evident that the given input is too short. Anyway, I'll move this check to JS because we cannot even create a buffer with a negative length
62–68	Yes, this looks like what we want. I must have missed this initializer. I'll try and check if it works