[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 ↗	(On Diff #23541)	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 ↗	(On Diff #23541)	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 ↗	(On Diff #23541)	Is there a reason we're doing a `>=` check instead of `==`? Seems like elsewhere we expect the sizes to be exact?
62–68 ↗	(On Diff #23541)	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 ↗	(On Diff #23541)	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 ↗	(On Diff #23541)	Done
55 ↗	(On Diff #23541)	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 ↗	(On Diff #23541)	Yes, this looks like what we want. I must have missed this initializer. I'll try and check if it works