From 99c81490e36ef931d7e15f07f019a010f03e0108 Mon Sep 17 00:00:00 2001 From: YairEtzion Date: Sat, 4 Apr 2026 13:14:57 +0300 Subject: [PATCH] fix: macOS Keychain stale key accumulation on reinit SecItemDelete only removes one Keychain item per call. Multiple amesh init --force runs accumulated stale keys under the same tag, causing sign() to use an old private key while identity.json stored the new public key. selfSig verification failed on the remote peer. Fix: loop SecItemDelete until all matching items are gone before generating a new key. Applied to both generate and delete actions. Added regression tests for both encrypted-file and macOS Keychain drivers verifying that generateAndStore twice with the same ID produces signatures that verify against the second (new) public key. --- .../src/__tests__/encrypted-file.test.ts | 28 +++++ .../src/__tests__/macos-keychain.test.ts | 105 ++++++++++++++++++ packages/keystore/swift/main.swift | 12 +- 3 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 packages/keystore/src/__tests__/macos-keychain.test.ts diff --git a/packages/keystore/src/__tests__/encrypted-file.test.ts b/packages/keystore/src/__tests__/encrypted-file.test.ts index a2196b6..41ec5f9 100644 --- a/packages/keystore/src/__tests__/encrypted-file.test.ts +++ b/packages/keystore/src/__tests__/encrypted-file.test.ts @@ -157,6 +157,34 @@ describe('EncryptedFileKeyStore', () => { }); }); + describe('key overwrite (re-init)', () => { + it('generateAndStore twice with same ID uses the new key', async () => { + const store = new EncryptedFileKeyStore(tempDir, PASSPHRASE); + const { publicKey: pk1 } = await store.generateAndStore(DEVICE_ID); + const { publicKey: pk2 } = await store.generateAndStore(DEVICE_ID); + + // Keys should differ + expect(pk1).not.toEqual(pk2); + + // Sign with the store — must use the NEW key + const msg = new TextEncoder().encode('test overwrite'); + const sig = await store.sign(DEVICE_ID, msg); + + // Signature must verify against the NEW public key + expect(verifyMessage(sig, msg, pk2)).toBe(true); + // Signature must NOT verify against the OLD public key + expect(verifyMessage(sig, msg, pk1)).toBe(false); + }); + + it('getPublicKey returns the new key after overwrite', async () => { + const store = new EncryptedFileKeyStore(tempDir, PASSPHRASE); + await store.generateAndStore(DEVICE_ID); + const { publicKey: pk2 } = await store.generateAndStore(DEVICE_ID); + + expect(await store.getPublicKey(DEVICE_ID)).toEqual(pk2); + }); + }); + describe('adversarial: tampered key file', () => { it('fails to decrypt if ciphertext is modified', async () => { const store = new EncryptedFileKeyStore(tempDir, PASSPHRASE); diff --git a/packages/keystore/src/__tests__/macos-keychain.test.ts b/packages/keystore/src/__tests__/macos-keychain.test.ts new file mode 100644 index 0000000..15fe87b --- /dev/null +++ b/packages/keystore/src/__tests__/macos-keychain.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect, afterEach, setDefaultTimeout } from 'bun:test'; +import { verifyMessage } from '@authmesh/core'; + +setDefaultTimeout(30_000); + +// These tests require the macOS Keychain Swift helper and only run on macOS. +// They use a unique tag prefix to avoid colliding with real amesh keys. +const TEST_DEVICE_ID = 'am_test_keychain_ci'; + +const isMacOS = process.platform === 'darwin'; + +// Skip all tests on non-macOS platforms +const describeOnMac = isMacOS ? describe : describe.skip; + +// Dynamic import to avoid errors on non-macOS +async function getKeychainStore() { + const { MacOSKeychainKeyStore, isMacOSKeychainAvailable } = + await import('../drivers/macos-keychain.js'); + const { available } = await isMacOSKeychainAvailable(); + return { MacOSKeychainKeyStore, available }; +} + +describeOnMac('MacOSKeychainKeyStore', () => { + afterEach(async () => { + // Clean up test keys from keychain + try { + const { MacOSKeychainKeyStore } = await getKeychainStore(); + const store = new MacOSKeychainKeyStore('/tmp'); + await store.delete(TEST_DEVICE_ID); + } catch { + // ignore cleanup errors + } + }); + + it('generates a 33-byte compressed P-256 public key', async () => { + const { MacOSKeychainKeyStore, available } = await getKeychainStore(); + if (!available) return; + const store = new MacOSKeychainKeyStore('/tmp'); + const { publicKey } = await store.generateAndStore(TEST_DEVICE_ID); + + expect(publicKey).toBeInstanceOf(Uint8Array); + expect(publicKey.length).toBe(33); + expect([0x02, 0x03]).toContain(publicKey[0]); + }); + + it('sign produces a verifiable 64-byte signature', async () => { + const { MacOSKeychainKeyStore, available } = await getKeychainStore(); + if (!available) return; + const store = new MacOSKeychainKeyStore('/tmp'); + const { publicKey } = await store.generateAndStore(TEST_DEVICE_ID); + + const msg = new TextEncoder().encode('test message'); + const sig = await store.sign(TEST_DEVICE_ID, msg); + + expect(sig.length).toBe(64); + expect(verifyMessage(sig, msg, publicKey)).toBe(true); + }); + + it('generateAndStore twice with same ID uses the new key (stale key regression)', async () => { + const { MacOSKeychainKeyStore, available } = await getKeychainStore(); + if (!available) return; + const store = new MacOSKeychainKeyStore('/tmp'); + + // First generate + const { publicKey: pk1 } = await store.generateAndStore(TEST_DEVICE_ID); + + // Second generate — same device ID (simulates `amesh init --force`) + const { publicKey: pk2 } = await store.generateAndStore(TEST_DEVICE_ID); + + // Keys should differ + expect(pk1).not.toEqual(pk2); + + // Sign must use the NEW key + const msg = new TextEncoder().encode('stale key regression test'); + const sig = await store.sign(TEST_DEVICE_ID, msg); + + // Must verify against the NEW public key + expect(verifyMessage(sig, msg, pk2)).toBe(true); + // Must NOT verify against the OLD public key + expect(verifyMessage(sig, msg, pk1)).toBe(false); + }); + + it('getPublicKey returns the new key after overwrite', async () => { + const { MacOSKeychainKeyStore, available } = await getKeychainStore(); + if (!available) return; + const store = new MacOSKeychainKeyStore('/tmp'); + + await store.generateAndStore(TEST_DEVICE_ID); + const { publicKey: pk2 } = await store.generateAndStore(TEST_DEVICE_ID); + + const retrieved = await store.getPublicKey(TEST_DEVICE_ID); + expect(retrieved).toEqual(pk2); + }); + + it('delete removes the key', async () => { + const { MacOSKeychainKeyStore, available } = await getKeychainStore(); + if (!available) return; + const store = new MacOSKeychainKeyStore('/tmp'); + + await store.generateAndStore(TEST_DEVICE_ID); + await store.delete(TEST_DEVICE_ID); + + await expect(store.sign(TEST_DEVICE_ID, new TextEncoder().encode('test'))).rejects.toThrow(); + }); +}); diff --git a/packages/keystore/swift/main.swift b/packages/keystore/swift/main.swift index 06f8783..c405226 100644 --- a/packages/keystore/swift/main.swift +++ b/packages/keystore/swift/main.swift @@ -105,6 +105,13 @@ case "check": case "generate": guard let id = cmd.deviceId else { fail("missing_device_id") } + // Delete ALL existing keys with this tag to avoid stale key shadowing. + // SecItemDelete may only remove one item per call, so loop until clean. + let delQuery: [String: Any] = [ + kSecClass as String: kSecClassKey, + kSecAttrApplicationTag as String: tagFor(id), + ] + while SecItemDelete(delQuery as CFDictionary) == errSecSuccess {} // Try Secure Enclave first, fall back to software keychain if let key = trySecureEnclaveGenerate(id) { guard let pub = exportPubKey(key) else { fail("export_failed") } @@ -134,10 +141,11 @@ case "get-public-key": case "delete": guard let id = cmd.deviceId else { fail("missing_device_id") } - SecItemDelete([ + let delQ: [String: Any] = [ kSecClass as String: kSecClassKey, kSecAttrApplicationTag as String: tagFor(id), - ] as CFDictionary) + ] + while SecItemDelete(delQ as CFDictionary) == errSecSuccess {} ok() default: