diff --git a/README.md b/README.md index 9631793..f3add6f 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,13 @@ tang auth status --diagnostic # expiry/scope/audience; token value stays hid tang auth logout # confirms before deleting the session ``` +On macOS, `tang` accesses its Keychain entries through Apple's stable +`/usr/bin/security` helper. This prevents Homebrew or other Node upgrades from +changing the executable identity attached to the credentials and causing a new +Keychain prompt on every command. Existing credentials created by an older +version may ask once whether `security` can access the item; enter your macOS +login password and choose **Always Allow** to complete the migration. + Legacy PDS app passwords remain available: ```bash diff --git a/src/lib/keychain.ts b/src/lib/keychain.ts new file mode 100644 index 0000000..41fca47 --- /dev/null +++ b/src/lib/keychain.ts @@ -0,0 +1,156 @@ +import { spawn } from 'node:child_process'; +import { AsyncEntry } from '@napi-rs/keyring'; + +const MACOS_SECURITY_PATH = '/usr/bin/security'; +const ITEM_NOT_FOUND_STATUS = 44; +const DUPLICATE_ITEM_STATUS = 45; + +interface CommandResult { + code: number; + stdout: string; + stderr: string; +} + +type SecurityRunner = (args: string[], input?: string) => Promise; + +export interface KeychainStore { + setPassword(service: string, account: string, password: string): Promise; + getPassword(service: string, account: string): Promise; + deletePassword(service: string, account: string): Promise; +} + +function runSecurityCommand(args: string[], input?: string): Promise { + return new Promise((resolve, reject) => { + const child = spawn(MACOS_SECURITY_PATH, args, { + stdio: ['pipe', 'pipe', 'pipe'], + }); + let stdout = ''; + let stderr = ''; + + child.stdout.setEncoding('utf8'); + child.stderr.setEncoding('utf8'); + child.stdout.on('data', (chunk: string) => { + stdout += chunk; + }); + child.stderr.on('data', (chunk: string) => { + stderr += chunk; + }); + child.once('error', reject); + child.once('close', (code) => { + resolve({ code: code ?? 1, stdout, stderr }); + }); + + child.stdin.end(input); + }); +} + +function commandError(action: string, result: CommandResult): Error { + const detail = result.stderr.trim() || result.stdout.trim() || `exit status ${result.code}`; + return new Error(`macOS Keychain ${action} failed: ${detail}`); +} + +function passwordInput(password: string): string { + // `security ... -w` without a value reads and confirms the secret on stdin. + // This keeps session tokens out of argv and process listings. + return `${password}\n${password}\n`; +} + +function withoutTrailingNewline(value: string): string { + return value.endsWith('\n') ? value.slice(0, -1) : value; +} + +/** + * Build a macOS Keychain store whose stable accessor is Apple's + * `/usr/bin/security`, rather than the current Node executable. + * + * Homebrew Node builds are ad-hoc signed, so their code identity changes after + * upgrades. Keychain entries created directly by Node then prompt again after + * every upgrade. Trusting the stable system helper avoids coupling credentials + * to a particular Node build while keeping secrets encrypted in the login + * keychain and out of command-line arguments. + */ +export function createMacOsKeychainStore( + runSecurity: SecurityRunner = runSecurityCommand +): KeychainStore { + return { + async setPassword(service, account, password) { + const createArgs = [ + 'add-generic-password', + '-a', + account, + '-s', + service, + '-T', + MACOS_SECURITY_PATH, + '-w', + ]; + const created = await runSecurity(createArgs, passwordInput(password)); + if (created.code === 0) return; + + const duplicate = + created.code === DUPLICATE_ITEM_STATUS || created.stderr.includes('already exists'); + if (!duplicate) throw commandError('write', created); + + // Do not pass -T while updating. Existing items retain their access list, + // including the stable helper after the one-time legacy-item authorization. + const updateArgs = ['add-generic-password', '-U', '-a', account, '-s', service, '-w']; + const updated = await runSecurity(updateArgs, passwordInput(password)); + if (updated.code !== 0) throw commandError('update', updated); + }, + + async getPassword(service, account) { + const result = await runSecurity([ + 'find-generic-password', + '-a', + account, + '-s', + service, + '-w', + ]); + if (result.code === ITEM_NOT_FOUND_STATUS) return null; + if (result.code !== 0) throw commandError('read', result); + return withoutTrailingNewline(result.stdout); + }, + + async deletePassword(service, account) { + const result = await runSecurity(['delete-generic-password', '-a', account, '-s', service]); + if (result.code === ITEM_NOT_FOUND_STATUS) return false; + if (result.code !== 0) throw commandError('delete', result); + return true; + }, + }; +} + +const macOsKeychainStore = createMacOsKeychainStore(); + +const nativeKeyringStore: KeychainStore = { + async setPassword(service, account, password) { + await new AsyncEntry(service, account).setPassword(password); + }, + async getPassword(service, account) { + return (await new AsyncEntry(service, account).getPassword()) ?? null; + }, + async deletePassword(service, account) { + return await new AsyncEntry(service, account).deleteCredential(); + }, +}; + +function keychainStore(): KeychainStore { + return process.platform === 'darwin' ? macOsKeychainStore : nativeKeyringStore; +} + +export async function saveKeychainSecret( + service: string, + account: string, + password: string +): Promise { + await keychainStore().setPassword(service, account, password); +} + +export async function loadKeychainSecret(service: string, account: string): Promise { + return await keychainStore().getPassword(service, account); +} + +export async function deleteKeychainSecret(service: string, account: string): Promise { + return await keychainStore().deletePassword(service, account); +} diff --git a/src/lib/session.ts b/src/lib/session.ts index e1dc691..2504549 100644 --- a/src/lib/session.ts +++ b/src/lib/session.ts @@ -3,7 +3,7 @@ import { homedir } from 'node:os'; import { join } from 'node:path'; import type { AtpSessionData } from '@atproto/api'; import type { NodeSavedSession, NodeSavedState } from '@atproto/oauth-client-node'; -import { AsyncEntry } from '@napi-rs/keyring'; +import { deleteKeychainSecret, loadKeychainSecret, saveKeychainSecret } from './keychain.js'; const SERVICE_NAME = 'tangled-cli'; const OAUTH_SESSION_SERVICE_NAME = 'tangled-cli-oauth-session'; @@ -27,13 +27,11 @@ export interface SessionMetadata { async function saveKeychainJson(service: string, accountId: string, value: unknown): Promise { const serialized = JSON.stringify(value); - const entry = new AsyncEntry(service, accountId); - await entry.setPassword(serialized); + await saveKeychainSecret(service, accountId, serialized); } async function loadKeychainJson(service: string, accountId: string): Promise { - const entry = new AsyncEntry(service, accountId); - const serialized = await entry.getPassword(); + const serialized = await loadKeychainSecret(service, accountId); if (!serialized) return null; return JSON.parse(serialized) as T; } @@ -93,8 +91,7 @@ export async function loadOAuthSession(sub: string): Promise { try { - const entry = new AsyncEntry(OAUTH_SESSION_SERVICE_NAME, sub); - return await entry.deleteCredential(); + return await deleteKeychainSecret(OAUTH_SESSION_SERVICE_NAME, sub); } catch (error) { throw new Error( `Failed to delete OAuth session from keychain: ${error instanceof Error ? error.message : 'Unknown error'}` @@ -111,8 +108,7 @@ export async function loadOAuthState(key: string): Promise { - const entry = new AsyncEntry(OAUTH_STATE_SERVICE_NAME, key); - return await entry.deleteCredential(); + return await deleteKeychainSecret(OAUTH_STATE_SERVICE_NAME, key); } /** @@ -121,8 +117,7 @@ export async function deleteOAuthState(key: string): Promise { */ export async function deleteSession(accountId: string): Promise { try { - const entry = new AsyncEntry(SERVICE_NAME, accountId); - return await entry.deleteCredential(); + return await deleteKeychainSecret(SERVICE_NAME, accountId); } catch (error) { throw new Error( `Failed to delete session from keychain: ${error instanceof Error ? error.message : 'Unknown error'}` diff --git a/tests/lib/keychain.test.ts b/tests/lib/keychain.test.ts new file mode 100644 index 0000000..ee9e3e7 --- /dev/null +++ b/tests/lib/keychain.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createMacOsKeychainStore } from '../../src/lib/keychain.js'; + +describe('macOS Keychain store', () => { + it('creates credentials through the stable security helper without putting secrets in argv', async () => { + const run = vi.fn().mockResolvedValue({ code: 0, stdout: '', stderr: '' }); + const store = createMacOsKeychainStore(run); + + await store.setPassword('tangled-cli-oauth-session', 'did:plc:test', 'secret-value'); + + expect(run).toHaveBeenCalledOnce(); + const [args, input] = run.mock.calls[0]; + expect(args).toEqual([ + 'add-generic-password', + '-a', + 'did:plc:test', + '-s', + 'tangled-cli-oauth-session', + '-T', + '/usr/bin/security', + '-w', + ]); + expect(args).not.toContain('secret-value'); + expect(input).toBe('secret-value\nsecret-value\n'); + }); + + it('updates existing credentials without replacing their stable access list', async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ code: 45, stdout: '', stderr: 'item already exists' }) + .mockResolvedValueOnce({ code: 0, stdout: '', stderr: '' }); + const store = createMacOsKeychainStore(run); + + await store.setPassword('service', 'account', 'updated-secret'); + + expect(run).toHaveBeenCalledTimes(2); + expect(run.mock.calls[1][0]).toEqual([ + 'add-generic-password', + '-U', + '-a', + 'account', + '-s', + 'service', + '-w', + ]); + expect(run.mock.calls[1][0]).not.toContain('-T'); + expect(run.mock.calls[1][1]).toBe('updated-secret\nupdated-secret\n'); + }); + + it('reads a credential and removes the security command newline', async () => { + const run = vi.fn().mockResolvedValue({ code: 0, stdout: 'stored-secret\n', stderr: '' }); + const store = createMacOsKeychainStore(run); + + await expect(store.getPassword('service', 'account')).resolves.toBe('stored-secret'); + expect(run).toHaveBeenCalledWith([ + 'find-generic-password', + '-a', + 'account', + '-s', + 'service', + '-w', + ]); + }); + + it('returns null when a credential does not exist', async () => { + const run = vi.fn().mockResolvedValue({ code: 44, stdout: '', stderr: 'not found' }); + const store = createMacOsKeychainStore(run); + + await expect(store.getPassword('service', 'account')).resolves.toBeNull(); + }); + + it('reports whether a credential was deleted', async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ code: 0, stdout: '', stderr: '' }) + .mockResolvedValueOnce({ code: 44, stdout: '', stderr: 'not found' }); + const store = createMacOsKeychainStore(run); + + await expect(store.deletePassword('service', 'account')).resolves.toBe(true); + await expect(store.deletePassword('service', 'account')).resolves.toBe(false); + }); + + it('surfaces Keychain command failures without exposing the secret', async () => { + const run = vi.fn().mockResolvedValue({ code: 1, stdout: '', stderr: 'keychain locked' }); + const store = createMacOsKeychainStore(run); + + await expect(store.setPassword('service', 'account', 'do-not-print')).rejects.toThrow( + 'macOS Keychain write failed: keychain locked' + ); + }); +}); diff --git a/tests/lib/session.test.ts b/tests/lib/session.test.ts index f1c3b65..6cf5ca8 100644 --- a/tests/lib/session.test.ts +++ b/tests/lib/session.test.ts @@ -10,28 +10,23 @@ import { } from '../../src/lib/session.js'; import { mockSessionData, mockSessionData2, mockSessionMetadata } from '../helpers/mock-data.js'; -// Mock @napi-rs/keyring const mockKeyringStorage = new Map(); -vi.mock('@napi-rs/keyring', () => { - return { - AsyncEntry: vi.fn().mockImplementation((service: string, account: string) => { - const key = `${service}:${account}`; - - return { - setPassword: vi.fn().mockImplementation(async (password: string) => { - mockKeyringStorage.set(key, password); - }), - getPassword: vi.fn().mockImplementation(async () => { - return mockKeyringStorage.get(key) || null; - }), - deleteCredential: vi.fn().mockImplementation(async () => { - return mockKeyringStorage.delete(key); - }), - }; - }), - }; -}); +vi.mock('../../src/lib/keychain.js', () => ({ + saveKeychainSecret: vi.fn( + async (service: string, account: string, password: string): Promise => { + mockKeyringStorage.set(`${service}:${account}`, password); + } + ), + loadKeychainSecret: vi.fn( + async (service: string, account: string): Promise => + mockKeyringStorage.get(`${service}:${account}`) ?? null + ), + deleteKeychainSecret: vi.fn( + async (service: string, account: string): Promise => + mockKeyringStorage.delete(`${service}:${account}`) + ), +})); // Mock node:fs/promises for metadata file storage const mockFileStorage = new Map();