feat(encryption): support providing multiple decryption keys for key rotation #1942
Conversation
ymc9
commented
Jan 7, 2025
ymc9
commented
- Persist metadata alongside the encrypted data
- Use key digest to determine the proper key for decryption
…rotation - Persist metadata alongside with the encrypted data - Use key digest to determine the proper key for decryption
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the encryption handling capabilities in the runtime package. The changes focus on the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/runtime/src/enhancements/node/encryption.ts (2)
101-103: Remove Redundant Non-Null AssertionsThe non-null assertion operator
!is used onthis.options.encryption!after already checking for its existence. Sinceoptions.encryptionis verified earlier, the non-null assertions here are redundant and can be removed to improve code readability.Apply this diff to remove redundant non-null assertions:
- if (this.isCustomEncryption(this.options.encryption!)) { + if (this.isCustomEncryption(this.options.encryption)) { ... - if (this.isCustomEncryption(this.options.encryption!)) { + if (this.isCustomEncryption(this.options.encryption)) { ... - if (this.isCustomEncryption(this.options.encryption!)) { + if (this.isCustomEncryption(this.options.encryption)) {Also applies to: 111-113, 121-123
269-269: Consider Encrypting Empty Strings for ConsistencyCurrently, the code skips encryption for null, undefined, or empty string values. While it's reasonable to skip null or undefined, excluding empty strings from encryption may lead to inconsistent data handling and potential security concerns. Encrypting empty strings ensures uniformity and prevents patterns that could be exploited.
Adjust the condition to include empty strings for encryption:
- if (!data) return; + if (data == null) return;packages/runtime/src/types.ts (1)
176-195: Ensure Consistent Documentation FormattingThe comments for
SimpleEncryptionfields have inconsistent indentation and alignment, which can hinder readability. Align the asterisks and adjust the formatting to follow standard JSDoc conventions.Apply this diff to correct the documentation formatting:
/** - * Simple encryption settings for processing fields marked with `@encrypted`. - */ + * Simple encryption settings for processing fields marked with `@encrypted`. + */ export type SimpleEncryption = { /** - * The encryption key. - */ + * The encryption key. + */ encryptionKey: Uint8Array; /** - * Optional list of all decryption keys that were previously used to encrypt the data - * , for supporting key rotation. The `encryptionKey` field value is automatically - * included for decryption. - * - * When the encrypted data is persisted, a metadata object containing the digest of the - * encryption key is stored alongside the data. This digest is used to quickly determine - * the correct decryption key to use when reading the data. - */ + * Optional list of all decryption keys that were previously used to encrypt the data, + * for supporting key rotation. The `encryptionKey` field value is automatically + * included for decryption. + * + * When the encrypted data is persisted, a metadata object containing the digest of the + * encryption key is stored alongside the data. This digest is used to quickly determine + * the correct decryption key to use when reading the data. + */ decryptionKeys?: Uint8Array[]; };tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts (3)
56-56: Remove Debuggingconsole.logStatements from Test CodeThere are multiple
console.logstatements used for debugging purposes in the test code. It's best practice to remove these statements before committing to keep the test output clean and avoid cluttering logs during test runs.Apply this diff to remove the
console.logstatements:- console.log('Raw data:', JSON.stringify(rawRead)); ... - console.log('Raw data:', JSON.stringify(rawRead)); ... - console.log('Raw data:', JSON.stringify(rawRead)); ... - console.log('User1:', user1); - console.log('User2:', user2); - console.log('All users:', allUsers); ... - console.log('Mixed decrypted and raw:', found); ... - console.log('All raw:', found1);Also applies to: 107-107, 190-190, 214-214, 221-221, 228-228, 256-256, 265-265
198-204: Clarify Destructuring ofenhanceRawIn the test setup,
enhanceRawis aliased toenhancethrough destructuring, which might confuse readers accustomed toenhancebeing the default enhancer. To improve clarity, consider keeping the original naming or adding a comment explaining the aliasing.- const { enhanceRaw: enhance, prisma } = await loadSchema( + // Using enhanceRaw to get raw Prisma client without default enhancements + const { enhanceRaw, prisma } = await loadSchema( ... - const db1 = enhance(prisma, undefined, { + const db1 = enhanceRaw(prisma, undefined, {
214-215: Validate Test Assertions Instead of Logging OutputInstead of logging user data with
console.log, it's better to validate the test outcomes using assertions. This ensures the tests automatically verify the expected behavior without manual inspection of logs.Replace the
console.logstatements with appropriate assertions.- console.log('User1:', user1); + expect(user1).toBeDefined(); + expect(user1.secret).toBe('user1'); - console.log('User2:', user2); + expect(user2).toBeDefined(); + expect(user2.secret).toBe('user2'); - console.log('All users:', allUsers); + expect(allUsers).toHaveLength(2); + expect(allUsers).toEqual(expect.arrayContaining([user1, user2]));Also applies to: 221-222, 228-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime/src/enhancements/node/encryption.ts(4 hunks)packages/runtime/src/types.ts(1 hunks)tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts (1)
194-259: Great comprehensive test coverage for key rotation scenarios!The test thoroughly covers multiple scenarios including:
- Multiple decryption keys
- Key rotation
- Edge cases (duplicated keys, invalid keys)
- Security implications (missing keys)
However, consider making the test more maintainable with more descriptive variable names.
Consider these naming improvements:
-const dbAll = enhance(prisma, undefined, { +const dbWithAllKeys = enhance(prisma, undefined, { -const found = await dbWithMissingKeys.user.findMany(); +const usersWithMissingKey = await dbWithMissingKeys.user.findMany(); -const found1 = await dbWithAllWrongKeys.user.findMany(); +const usersWithWrongKeys = await dbWithAllWrongKeys.user.findMany();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dependency-review
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
🔇 Additional comments (2)
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts (2)
Line range hint
146-192: LGTM! Good test coverage for custom encryption.The test effectively verifies both the encrypted and raw data states, ensuring the custom encryption/decryption logic works as expected.
203-204: Security implementation looks solid!Good security practices observed:
- Using crypto.getRandomValues for secure key generation
- Proper key length (32 bytes)
- Safe handling of invalid keys
Also applies to: 220-220, 241-241, 255-255
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/runtime/src/enhancements/node/encryption.ts (1)
155-161:⚠️ Potential issueUse Web APIs for binary data handling.
The current implementation uses
btoaandString.fromCharCodewhich can cause issues with binary data. Since Buffer is not available in edge runtimes, use Web APIs instead.- return `${btoa(JSON.stringify(meta))}.${btoa(String.fromCharCode(...cipherBytes))}`; + const metaBase64 = btoa(JSON.stringify(meta)); + const cipherBase64 = btoa(String.fromCharCode.apply(null, Array.from(cipherBytes))); + return `${metaBase64}.${cipherBase64}`;
🧹 Nitpick comments (3)
packages/runtime/src/enhancements/node/encryption.ts (3)
66-80: Enhance error handling with more specific error types.Instead of using generic
unknownError, consider creating specific error types for encryption-related errors. This would make error handling more maintainable and help with debugging.- throw this.queryUtils.unknownError('Encryption options must be provided'); + throw new EncryptionConfigError('Encryption options must be provided'); - throw this.queryUtils.unknownError('Custom encryption must provide encrypt and decrypt functions'); + throw new EncryptionConfigError('Custom encryption must provide both encrypt and decrypt functions'); - throw this.queryUtils.unknownError('Encryption key must be provided'); + throw new EncryptionConfigError('Encryption key must be provided'); - throw this.queryUtils.unknownError(`Encryption key must be ${this.ENCRYPTION_KEY_BYTES} bytes`); + throw new EncryptionConfigError(`Invalid encryption key length: expected ${this.ENCRYPTION_KEY_BYTES} bytes`);
120-136: Optimize decryption key lookup with a Map-based cache.The current implementation rebuilds the decryption keys array on every lookup. Consider using a Map for O(1) lookup performance.
- private decryptionKeys: Array<{ key: CryptoKey; digest: string }> = []; + private decryptionKeysMap: Map<string, CryptoKey[]> = new Map(); private async findDecryptionKeys(keyDigest: string): Promise<CryptoKey[]> { if (this.isCustomEncryption(this.options.encryption!)) { throw new Error('Unexpected custom encryption settings'); } - if (this.decryptionKeys.length === 0) { + if (this.decryptionKeysMap.size === 0) { const keys = [this.options.encryption!.encryptionKey, ...(this.options.encryption!.decryptionKeys || [])]; - this.decryptionKeys = await Promise.all( - keys.map(async (key) => ({ - key: await this.loadKey(key, ['decrypt']), - digest: await this.computeKeyDigest(key), - })) - ); + await Promise.all( + keys.map(async (key) => { + const cryptoKey = await this.loadKey(key, ['decrypt']); + const digest = await this.computeKeyDigest(key); + const existing = this.decryptionKeysMap.get(digest) || []; + this.decryptionKeysMap.set(digest, [...existing, cryptoKey]); + }) + ); } - return this.decryptionKeys.filter((entry) => entry.digest === keyDigest).map((entry) => entry.key); + return this.decryptionKeysMap.get(keyDigest) || []; }
Line range hint
238-280: Enhance logging for better debugging.Add more detailed logging to help diagnose encryption/decryption issues in production.
if (shouldDecrypt) { try { + this.logger.debug(`Attempting to decrypt field "${field}" in model "${realModel}"`); entityData[field] = await this.decrypt(fieldInfo, entityData[field]); } catch (error) { - this.logger.warn(`Decryption failed, keeping original value: ${error}`); + this.logger.warn( + `Decryption failed for field "${field}" in model "${realModel}", keeping original value. Error: ${error}` + ); } } const encAttr = field.attributes?.find((attr) => attr.name === '@encrypted'); if (encAttr && field.type === 'String') { try { + this.logger.debug(`Attempting to encrypt field "${field.name}" in model "${model}"`); context.parent[field.name] = await this.encrypt(field, data); } catch (error) { - this.queryUtils.unknownError(`Encryption failed for field ${field.name}: ${error}`); + throw new Error(`Encryption failed for field "${field.name}" in model "${model}". Error: ${error}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/enhancements/node/encryption.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/runtime/src/enhancements/node/encryption.ts (1)
58-58: Consider increasing KEY_DIGEST_BYTES for better collision resistance.Using only 8 bytes (64 bits) for the key digest might not provide sufficient collision resistance in systems with multiple encryption keys.
