From a059ecba54c528961bad7dda42011cb8d0d6802f Mon Sep 17 00:00:00 2001 From: Kess Plasmeier Date: Tue, 18 Aug 2026 16:01:32 -0700 Subject: [PATCH] fix(kms-keyring-node): copy branch key material on read --- .../src/kms_hkeyring_node_helpers.ts | 17 +- .../kms_hkeyring_node.concurrency.test.ts | 145 ++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts diff --git a/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts b/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts index d142c9341..0c182f4a7 100644 --- a/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts +++ b/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts @@ -252,7 +252,22 @@ export async function getBranchKeyMaterials( branchKeyMaterials = cacheEntry.response } - return branchKeyMaterials + /* Hand back a copy the cache can never touch. The CMC zeroes a material's + * buffer in place on eviction; callers read the branch key AFTER this await, + * so a concurrent eviction (overwrite, TTL, or tail) could otherwise zero the + * buffer mid-derivation. */ + return deepCopyBranchKeyMaterial(branchKeyMaterials) +} + +function deepCopyBranchKeyMaterial( + material: NodeBranchKeyMaterial +): NodeBranchKeyMaterial { + return new NodeBranchKeyMaterial( + Buffer.from(material.branchKey()), + material.branchKeyIdentifier, + material.branchKeyVersion.toString('utf-8'), + { ...material.encryptionContext } + ) } //= aws-encryption-sdk-specification/framework/aws-kms/aws-kms-hierarchical-keyring.md#onencrypt diff --git a/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts b/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts new file mode 100644 index 000000000..2369b8dd7 --- /dev/null +++ b/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts @@ -0,0 +1,145 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { + EncryptedDataKey, + NodeBranchKeyMaterial, + NodeDecryptionMaterial, + NodeEncryptionMaterial, + unwrapDataKey, +} from '@aws-crypto/material-management' +import { + BRANCH_KEY_ID_A, + BRANCH_KEY_ID_B, + EC_A, + EC_B, + KEYSTORE, + TEST_ESDK_ALG_SUITE, + TTL, +} from './fixtures' +import { KmsHierarchicalKeyRingNode } from '../src/kms_hkeyring_node' +import { + BRANCH_KEY_ID_SUPPLIER, + deepCopyBranchKeyMaterial, +} from './kms_hkeyring_node.test' +import { expect } from 'chai' +import Sinon from 'sinon' +import { + BranchKeyStoreNode, + KeyStoreInfoOutput, +} from '@aws-crypto/branch-keystore-node' + +const CONCURRENCY = 25 + +let activeMaterialA: NodeBranchKeyMaterial +let activeMaterialB: NodeBranchKeyMaterial +before(async function () { + activeMaterialA = await KEYSTORE.getActiveBranchKey(BRANCH_KEY_ID_A) + activeMaterialB = await KEYSTORE.getActiveBranchKey(BRANCH_KEY_ID_B) +}) + +// A keystore stub that returns fresh material (its own buffer, as a real +// keystore would) and resolves on the microtask queue. +function stubKeyStore(): BranchKeyStoreNode { + const keyStore = Sinon.createStubInstance(BranchKeyStoreNode) + const forId = async (branchKeyId: string) => + deepCopyBranchKeyMaterial( + branchKeyId === BRANCH_KEY_ID_A ? activeMaterialA : activeMaterialB + ) + keyStore.getActiveBranchKey.callsFake(forId) + // The two active versions are the only versions used here, so map by id. + keyStore.getBranchKeyVersion.callsFake(forId) + keyStore.getKeyStoreInfo.callsFake( + (): KeyStoreInfoOutput => ({ + keystoreId: 'keyStoreId', + keystoreTableName: 'keystoreTableName', + logicalKeyStoreName: 'logicalKeyStoreName', + grantTokens: [], + kmsConfiguration: null as any, + }) + ) + return keyStore +} + +function newKeyring(maxCacheSize?: number): KmsHierarchicalKeyRingNode { + return new KmsHierarchicalKeyRingNode({ + branchKeyIdSupplier: BRANCH_KEY_ID_SUPPLIER, + keyStore: stubKeyStore(), + cacheLimitTtl: TTL, + maxCacheSize, + }) +} + +describe('KmsHierarchicalKeyRingNode: concurrent cold-cache operations (#1691)', () => { + it('concurrent onEncrypt does not wrap data keys under an evicted (zeroed) branch key', async () => { + // maxCacheSize=1 + two branch keys => every alternating encrypt evicts the + // other entry, zeroing its buffer. + const hkr = newKeyring(1) + const materials = Array.from( + { length: CONCURRENCY }, + (_, i) => + new NodeEncryptionMaterial( + TEST_ESDK_ALG_SUITE, + i % 2 === 0 ? EC_A : EC_B + ) + ) + + await Promise.all( + materials.map(async (m) => { + await hkr.onEncrypt(m) + }) + ) + + const verifier = newKeyring() + for (const m of materials) { + const expectedPdk = unwrapDataKey(m.getUnencryptedDataKey()) + const decryptionMaterial = new NodeDecryptionMaterial( + TEST_ESDK_ALG_SUITE, + m.encryptionContext + ) + await verifier.onDecrypt(decryptionMaterial, m.encryptedDataKeys) + expect( + unwrapDataKey(decryptionMaterial.getUnencryptedDataKey()) + ).to.deep.equal(expectedPdk) + } + }) + + it('concurrent onDecrypt does not derive from an evicted (zeroed) branch key', async () => { + // Build valid ciphertexts for both branch keys (sequential = uncorrupted). + const setup = newKeyring() + const encA = new NodeEncryptionMaterial(TEST_ESDK_ALG_SUITE, EC_A) + const encB = new NodeEncryptionMaterial(TEST_ESDK_ALG_SUITE, EC_B) + await setup.onEncrypt(encA) + await setup.onEncrypt(encB) + + const cases = [ + { + edks: encA.encryptedDataKeys, + ec: EC_A, + pdk: unwrapDataKey(encA.getUnencryptedDataKey()), + }, + { + edks: encB.encryptedDataKeys, + ec: EC_B, + pdk: unwrapDataKey(encB.getUnencryptedDataKey()), + }, + ] + + const hkr = newKeyring(1) + const recovered = await Promise.all( + Array.from({ length: CONCURRENCY }, async (_, i) => { + const { edks, ec } = cases[i % 2] + const decryptionMaterial = new NodeDecryptionMaterial( + TEST_ESDK_ALG_SUITE, + ec + ) + await hkr.onDecrypt(decryptionMaterial, edks as EncryptedDataKey[]) + return unwrapDataKey(decryptionMaterial.getUnencryptedDataKey()) + }) + ) + + for (let i = 0; i < recovered.length; i++) { + expect(recovered[i]).to.deep.equal(cases[i % 2].pdk) + } + }) +})