From cac078280ce1b117c54ddeb6e0021a5f1c7fee9a Mon Sep 17 00:00:00 2001 From: Muqsit Date: Sun, 14 Jun 2026 22:17:59 -0700 Subject: [PATCH] fix(secrets): pin keychain access group + explicit data-protection routing (#279) DRAFT / NEEDS-VERIFICATION on an affected machine (reporter: Darwin 25.5.0). The -25293 (errSecAuthFailed) regression does NOT reproduce on this build (macOS 26.4, 25E246) so the fix could not be end-to-end verified here. Issue #279: `agents secrets create/add` fails immediately on macOS with OSStatus -25293 (errSecAuthFailed). The original `set` attached only a biometry SecAccessControl with no kSecUseDataProtectionKeychain and no kSecAttrAccessGroup. Empirical correction to the original hypothesis (probed on macOS 26.4): adding a biometry SecAccessControl already routes the item to the data-protection keychain (the file-based keychain cannot store biometry-gated items) -- a plain add returns 0 here, a biometry-ACL add returns -34018, a DP-keychain-only error. So the write was NOT on the file-based keychain; moving it there explicitly does not, by itself, change which keychain is used. What this change actually does: - Pin kSecAttrAccessGroup to the concrete application-identifier group (2HTP252L87.com.phnx-labs.agents-keychain). The embedded profile grants the entitlement as a wildcard (2HTP252L87.*) with no concrete default group, so an add that omits the access group relies on implicit default-group resolution. Pinning removes that dependency -- the most likely real fix for -25293. - State kSecUseDataProtectionKeychain explicitly on every SecItem call (set, get, get-batch, delete, has, list, migrate-acl, migrateInline) so routing is deterministic and consistent across read and write. - Mark items kSecAttrSynchronizable=false (device-local), matching the existing ThisDeviceOnly intent. - Forward migration: readItem queries the data-protection keychain first, then the legacy file-based keychain ONCE on a clean miss; get/get-batch then rewrite the value into the data-protection keychain (delete legacy + re-add). Per-item and self-retiring -- idempotent without a separate sentinel. delete/set clear both keychains so no stale legacy copy shadows the new item. Signing/entitlements unchanged (already correct). Helper compiles clean for both arch slices. The biometry-SecAccessControl-evaluated-headlessly path (the reporter's own hypothesis) remains the alternative suspect if the access-group pin does not resolve it on the affected machine. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../secrets/__tests__/keychain-helper.test.ts | 163 +++++++++-- src/lib/secrets/keychain-helper.swift | 277 ++++++++++-------- 2 files changed, 298 insertions(+), 142 deletions(-) diff --git a/src/lib/secrets/__tests__/keychain-helper.test.ts b/src/lib/secrets/__tests__/keychain-helper.test.ts index 738b654e..393f8531 100644 --- a/src/lib/secrets/__tests__/keychain-helper.test.ts +++ b/src/lib/secrets/__tests__/keychain-helper.test.ts @@ -4,56 +4,165 @@ import * as path from 'path'; const helperSource = () => fs.readFileSync(path.join(process.cwd(), 'src/lib/secrets/keychain-helper.swift'), 'utf8'); +// Slice the source for a single `case "":` block so an assertion can't be +// satisfied by an unrelated command elsewhere in the file. `order` lists the +// switch cases in source order; the block runs from the named case up to the +// next case in that list (or the `default:` for the last one). +const order = ['list', 'has', 'get', 'get-batch', 'set', 'delete', 'migrate-acl']; +function caseBlock(source: string, cmd: string): string { + const idx = order.indexOf(cmd); + const start = source.indexOf(`case "${cmd}":`); + const nextMarker = idx + 1 < order.length ? `case "${order[idx + 1]}":` : 'default:'; + const end = source.indexOf(nextMarker, start); + expect(start, `case "${cmd}" present`).toBeGreaterThanOrEqual(0); + expect(end, `marker after case "${cmd}" present`).toBeGreaterThan(start); + return source.slice(start, end); +} + describe('keychain-helper Touch ID policy', () => { it('writes secrets with SecAccessControl user-presence protection', () => { const source = helperSource(); - expect(source).toContain('SecAccessControlCreateWithFlags'); expect(source).toContain('kSecAttrAccessibleWhenUnlockedThisDeviceOnly'); expect(source).toContain('.biometryCurrentSet'); expect(source).toContain('.devicePasscode'); - expect(source).toContain('kSecAttrAccessControl: buildBiometryAccessControl()'); + expect(source).toContain('addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl()'); }); it('reads secrets through a shared LocalAuthentication context', () => { const source = helperSource(); - expect(source).toContain('import LocalAuthentication'); expect(source).toContain('let authContext: LAContext'); - expect(source).toContain('kSecUseAuthenticationContext: authContext'); + expect(source).toContain('kSecUseAuthenticationContext] = authContext'); expect(source).toContain('case "get":'); expect(source).toContain('case "get-batch":'); }); }); -describe('keychain-helper data-protection keychain coverage', () => { - // Items written by `set` carry a biometry ACL, which forces them into the - // data-protection keychain. A query with kSecUseAuthenticationUIFail and - // no kSecUseDataProtectionKeychain key never sees those items, so has/list - // reported every stored secret as missing (RUSH-1083). Each command needs - // a DP pass alongside the file-based one. - it('has probes both the file-based and data-protection keychains', () => { +describe('keychain-helper data-protection keychain routing', () => { + // Issue #279: the biometry SecAccessControl already routed our items to the + // data-protection keychain implicitly, but the routing and the access group + // were never stated. dpBase() makes all three explicit (DP key + pinned access + // group + device-local) so every write/read/delete is deterministic and never + // relies on default-access-group resolution under a wildcard-only entitlement. + it('dpBase pins the DP keychain, a concrete access group, and device-local', () => { const source = helperSource(); - const hasBlock = source.slice(source.indexOf('case "has":'), source.indexOf('case "get":')); + const block = source.slice(source.indexOf('func dpBase('), source.indexOf('func fileBase(')); + expect(block).toContain('kSecUseDataProtectionKeychain: kCFBooleanTrue!'); + expect(block).toContain('kSecAttrAccessGroup: kAccessGroup as CFString'); + expect(block).toContain('kSecAttrSynchronizable: kCFBooleanFalse!'); + // The access group is the stable application-identifier from the profile. + expect(source).toContain('let kAccessGroup = "2HTP252L87.com.phnx-labs.agents-keychain"'); + }); - expect(hasBlock).toContain('for dp in [false, true]'); - expect(hasBlock).toContain('kSecUseDataProtectionKeychain'); - // UIFail is the no-prompt guarantee; a present DP item then surfaces as - // errSecInteractionNotAllowed, which must count as "exists". - expect(hasBlock).toContain('kSecUseAuthenticationUIFail'); - expect(hasBlock).toContain('errSecInteractionNotAllowed'); + it('fileBase carries no DP key and no access group (legacy reads only)', () => { + const source = helperSource(); + const block = source.slice(source.indexOf('func fileBase('), source.indexOf('// Read one item')); + expect(block).not.toContain('kSecUseDataProtectionKeychain'); + expect(block).not.toContain('kSecAttrAccessGroup'); }); - it('list enumerates both keychains and tolerates a locked DP keybag', () => { + it('every write/delete attrs dictionary is built from dpBase or fileBase', () => { + const source = helperSource(); + // No SecItemAdd/Delete/CopyMatching may hand-roll a kSecClass dictionary that + // bypasses the dpBase/fileBase helpers — that is how the old set/delete paths + // silently targeted only one keychain. Allow kSecClass only inside the + // helpers themselves and the two prefix-enumeration queries in `list`. + const listBlock = caseBlock(source, 'list'); + const occurrences = source.split('kSecClass: kSecClassGenericPassword').length - 1; + const allowed = + 2 + // dpBase + fileBase + (listBlock.split('kSecClass: kSecClassGenericPassword').length - 1); // fileQuery + dpQuery + expect(occurrences, 'kSecClass only appears in dpBase, fileBase, and list queries').toBe(allowed); + }); +}); + +describe('keychain-helper set / delete cover both keychains', () => { + it('set writes to DP and clears any stale copy from both keychains', () => { + const block = caseBlock(helperSource(), 'set'); + expect(block).toContain('SecItemDelete(dpBase(service: service, account: account) as CFDictionary)'); + expect(block).toContain('SecItemDelete(fileBase(service: service, account: account) as CFDictionary)'); + expect(block).toContain('var addAttrs = dpBase(service: service, account: account)'); + expect(block).toContain('addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl()'); + expect(block).toContain('SecItemAdd(addAttrs as CFDictionary, nil)'); + }); + + it('delete removes from both keychains and reports existence in either', () => { + const block = caseBlock(helperSource(), 'delete'); + expect(block).toContain('let dpStatus = SecItemDelete(dpBase('); + expect(block).toContain('let fileStatus = SecItemDelete(fileBase('); + expect(block).toContain('(dpStatus == errSecSuccess || fileStatus == errSecSuccess)'); + }); +}); + +describe('keychain-helper forward migration (file-based -> data-protection)', () => { + // Items written by a pre-migration helper live in whichever keychain the old + // attrs selected. readItem now resolves DP first, then the legacy file-based + // keychain ONCE on a clean miss; get/get-batch then migrate the value forward. + // This is the per-item, self-retiring analogue of a one-shot bulk migration: + // once an item is rewritten to DP, the DP lookup hits and the fallback never + // fires again — so it is idempotent without a separate sentinel. + it('readItem queries the data-protection keychain before the file-based one', () => { + const source = helperSource(); + const block = source.slice(source.indexOf('func readItem('), source.indexOf('func migrateInline(')); + const dpIdx = block.indexOf('var dpQuery = dpBase('); + const fileIdx = block.indexOf('var fileQuery = fileBase('); + expect(dpIdx).toBeGreaterThanOrEqual(0); + expect(fileIdx).toBeGreaterThan(dpIdx); // DP first, file fallback second + // The file fallback only runs on a clean not-found, never on auth errors. + expect(block).toContain('guard dpStatus == errSecItemNotFound else { return (nil, dpStatus, false) }'); + // A value found only in the file-based keychain is flagged for migration. + expect(block).toContain('return (value, fileStatus, true)'); + }); + + it('migrateInline deletes the legacy copy and re-adds into the DP keychain', () => { + const source = helperSource(); + const block = source.slice(source.indexOf('func migrateInline('), source.indexOf('func dieIfCancelled(')); + // Remove the legacy file-based copy so the next read resolves from DP. + expect(block).toContain('SecItemDelete(fileBase(service: service, account: account) as CFDictionary)'); + // Re-add carries the DP base (incl. access group) plus the biometry ACL. + expect(block).toContain('var addAttrs = dpBase(service: service, account: account)'); + expect(block).toContain('addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl()'); + expect(block).toContain('SecItemAdd(addAttrs as CFDictionary, nil)'); + }); + + it('get and get-batch only migrate items in our own namespace', () => { const source = helperSource(); - const listBlock = source.slice(source.indexOf('case "list":'), source.indexOf('case "has":')); + // Both callers guard migration on the agents-cli prefix so we never rewrite + // another app's items. + const getBlock = caseBlock(source, 'get'); + const batchBlock = caseBlock(source, 'get-batch'); + for (const block of [getBlock, batchBlock]) { + expect(block).toContain('needsMigration && service.hasPrefix("agents-cli.")'); + expect(block).toContain('migrateInline(service: service, account: account, value: value)'); + } + }); + + it('migrate-acl reads the legacy file copy and rewrites it into the DP keychain', () => { + const block = caseBlock(helperSource(), 'migrate-acl'); + expect(block).toContain('var readQuery = fileBase(service: service, account: account)'); + expect(block).toContain('SecItemDelete(fileBase(service: service, account: account) as CFDictionary)'); + expect(block).toContain('var addAttrs = dpBase(service: service, account: account)'); + expect(block).toContain('SecItemAdd(addAttrs as CFDictionary, nil)'); + }); +}); - expect(listBlock).toContain('kSecUseDataProtectionKeychain'); - expect(listBlock).toContain('for query in [fileQuery, dpQuery]'); - // The DP keybag locks with the screen; enumeration must skip, not die. - expect(listBlock).toContain('if status == errSecInteractionNotAllowed { continue }'); - // The DP pass must keep an explicit kSecReturn* key: with none at all, - // SecItemCopyMatching evaluates the biometry ACL and blocks on Touch ID. - expect(listBlock).toContain('kSecReturnAttributes'); +describe('keychain-helper has / list still probe both keychains', () => { + it('has probes the file-based and data-protection keychains without prompting', () => { + const block = caseBlock(helperSource(), 'has'); + expect(block).toContain('var fileQuery = fileBase(service: service, account: account)'); + expect(block).toContain('var dpQuery = dpBase(service: service, account: account)'); + expect(block).toContain('for query in [fileQuery, dpQuery]'); + expect(block).toContain('kSecUseAuthenticationUIFail'); + expect(block).toContain('errSecInteractionNotAllowed'); + }); + + it('list enumerates both keychains and tolerates a locked DP keybag', () => { + const block = caseBlock(helperSource(), 'list'); + expect(block).toContain('kSecUseDataProtectionKeychain: kCFBooleanTrue!'); + expect(block).toContain('kSecAttrAccessGroup: kAccessGroup as CFString'); + expect(block).toContain('for query in [fileQuery, dpQuery]'); + expect(block).toContain('if status == errSecInteractionNotAllowed { continue }'); + expect(block).toContain('kSecReturnAttributes'); }); }); diff --git a/src/lib/secrets/keychain-helper.swift b/src/lib/secrets/keychain-helper.swift index f5049124..c994aca6 100644 --- a/src/lib/secrets/keychain-helper.swift +++ b/src/lib/secrets/keychain-helper.swift @@ -43,73 +43,135 @@ func buildBiometryAccessControl() -> SecAccessControl { return access } -// Read one item's value, decrypting through the shared auth context. +// The data-protection keychain access group. This is the +// com.apple.application-identifier granted by the embedded provisioning +// profile (2HTP252L87.com.phnx-labs.agents-keychain) and is covered by the +// keychain-access-groups entitlement (2HTP252L87.*). Pinning it explicitly +// makes every item land in one deterministic group no matter how the helper is +// spawned, instead of relying on the implicit "first entitled group" default. +let kAccessGroup = "2HTP252L87.com.phnx-labs.agents-keychain" + +// Base attributes for every DATA-PROTECTION keychain operation (issue #279). // -// For modern biometry-protected items, the first read pops Touch ID and -// later reads reuse the assertion. For legacy items with a trusted-app -// ACL (kSecAttrAccess) that doesn't list this binary, macOS shows the -// "enter password" sheet — kSecUseAuthenticationUIAllow makes that -// fallback explicit and intentional. We use that one password sheet to -// drive the JIT upgrade: the caller (get / get-batch) re-writes the -// item with biometry ACL immediately afterward, so the next read is -// Touch ID forever. +// Three properties, each made explicit on purpose: // -// Returns the decoded value, the raw OSStatus (so callers can -// distinguish missing/cancelled), and a needsMigration flag — true if -// the item has no kSecAttrAccessControl (legacy item). -func readItem(service: String, account: String) -> (value: String?, status: OSStatus, needsMigration: Bool) { - let query: [CFString: Any] = [ +// - kSecUseDataProtectionKeychain: routes the SecItem call to the +// data-protection keychain. The biometry SecAccessControl already forced our +// items onto this keychain implicitly (the file-based keychain cannot store +// biometry-gated items); stating it removes any ambiguity and is Apple's +// documented direction — the file-based keychain is on the road to +// deprecation. See TN3137 "On Mac keychain APIs and implementations". +// +// - kSecAttrAccessGroup: pins items to ONE concrete access group. The embedded +// provisioning profile grants the entitlement as a wildcard (2HTP252L87.*) +// with no concrete default group, so an add that omits the access group +// relies on the system's implicit default-group resolution. Pinning the +// application-identifier group removes that dependency and makes every item +// deterministic regardless of how the helper is spawned. +// +// - kSecAttrSynchronizable false: keeps items device-local (matching the +// kSecAttrAccessibleWhenUnlockedThisDeviceOnly intent of the biometry ACL and +// never letting them reach iCloud Keychain). +func dpBase(service: String, account: String) -> [CFString: Any] { + return [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: service as CFString, + kSecAttrAccount: account as CFString, + kSecUseDataProtectionKeychain: kCFBooleanTrue!, + kSecAttrAccessGroup: kAccessGroup as CFString, + kSecAttrSynchronizable: kCFBooleanFalse!, + ] +} + +// Base attributes for the LEGACY file-based login keychain. Used ONLY to read +// or remove items written by helper versions before the data-protection +// migration. The file-based keychain has no access-group concept, so we add +// neither kSecAttrAccessGroup nor kSecUseDataProtectionKeychain here. +func fileBase(service: String, account: String) -> [CFString: Any] { + return [ kSecClass: kSecClassGenericPassword, kSecAttrService: service as CFString, kSecAttrAccount: account as CFString, - kSecReturnData: kCFBooleanTrue!, - kSecReturnAttributes: kCFBooleanTrue!, - kSecMatchLimit: kSecMatchLimitOne, - kSecUseAuthenticationContext: authContext, - kSecUseAuthenticationUI: kSecUseAuthenticationUIAllow, - kSecUseOperationPrompt: "Unlock agents-cli secrets" as CFString, ] - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - guard status == errSecSuccess, - let dict = result as? [String: Any], - let data = dict[kSecValueData as String] as? Data, - let value = String(data: data, encoding: .utf8) else { return (nil, status, false) } - // Legacy items (kSecAttrAccess, "any-app", or no ACL at all) lack the - // kSecAttrAccessControl attribute. Items written by the new `set` - // path have it. Use that as the migration signal. - let needsMigration = dict[kSecAttrAccessControl as String] == nil - return (value, status, needsMigration) } -// Re-write a legacy item with the modern biometry access control. Called -// inline by get / get-batch right after the legacy password sheet has -// produced the plaintext — every future read of this item will then -// require Touch ID via the LAContext flow. Delete + re-add is required -// because SecItemUpdate cannot change an item's ACL. If anything goes -// wrong, log to stderr but don't fail the parent read: the caller -// already has the value and the user just typed their password for it. +// Read one item's value, decrypting through the shared auth context. +// +// Lookup order: +// 1. The DATA-PROTECTION keychain, where `set` now writes. For modern +// biometry-protected items the first read pops Touch ID and later reads +// reuse the assertion via the shared LAContext. +// 2. On a clean miss, the LEGACY file-based login keychain ONCE. Items written +// by a pre-migration helper still live there; reading one may pop the +// legacy "enter password" sheet for a trusted-app ACL, which +// kSecUseAuthenticationUIAllow makes explicit and intentional. The caller +// (get / get-batch) then re-writes the value into the data-protection +// keychain via migrateInline, so the next read resolves at step 1 and this +// fallback never fires again for that item. +// +// Returns the decoded value, the raw OSStatus (so callers can distinguish +// missing/cancelled), and a needsMigration flag — true when the value was found +// only in the legacy file-based keychain and must be migrated forward. +func readItem(service: String, account: String) -> (value: String?, status: OSStatus, needsMigration: Bool) { + var dpQuery = dpBase(service: service, account: account) + dpQuery[kSecReturnData] = kCFBooleanTrue! + dpQuery[kSecMatchLimit] = kSecMatchLimitOne + dpQuery[kSecUseAuthenticationContext] = authContext + dpQuery[kSecUseAuthenticationUI] = kSecUseAuthenticationUIAllow + dpQuery[kSecUseOperationPrompt] = "Unlock agents-cli secrets" as CFString + var dpResult: AnyObject? + let dpStatus = SecItemCopyMatching(dpQuery as CFDictionary, &dpResult) + if dpStatus == errSecSuccess, + let data = dpResult as? Data, + let value = String(data: data, encoding: .utf8) { + return (value, dpStatus, false) + } + // Only a clean "not found" justifies the legacy fallback. errSecAuthFailed, + // user-cancel, interaction-not-allowed, etc. are surfaced to the caller as-is. + guard dpStatus == errSecItemNotFound else { return (nil, dpStatus, false) } + + var fileQuery = fileBase(service: service, account: account) + fileQuery[kSecReturnData] = kCFBooleanTrue! + fileQuery[kSecMatchLimit] = kSecMatchLimitOne + fileQuery[kSecUseAuthenticationContext] = authContext + fileQuery[kSecUseAuthenticationUI] = kSecUseAuthenticationUIAllow + fileQuery[kSecUseOperationPrompt] = "Unlock agents-cli secrets" as CFString + var fileResult: AnyObject? + let fileStatus = SecItemCopyMatching(fileQuery as CFDictionary, &fileResult) + guard fileStatus == errSecSuccess, + let data = fileResult as? Data, + let value = String(data: data, encoding: .utf8) else { + // Nothing in either keychain — report the data-protection miss so the + // caller treats it as "not found" rather than a legacy read error. + return (nil, dpStatus, false) + } + return (value, fileStatus, true) +} + +// Migrate a value found in the legacy file-based keychain forward into the +// data-protection keychain with the modern biometry access control. Called +// inline by get / get-batch right after the legacy read produced the plaintext +// — every future read then resolves from the data-protection keychain and +// requires Touch ID via the LAContext flow. If anything goes wrong, log to +// stderr but don't fail the parent read: the caller already has the value. func migrateInline(service: String, account: String, value: String) { guard let valueData = value.data(using: .utf8) else { writeStderr("migrate-inline: could not encode value for \(service)") return } - let delStatus = SecItemDelete([ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - ] as CFDictionary) + // Drop the legacy file-based copy so the next read resolves cleanly from the + // data-protection keychain instead of re-triggering this fallback. + let delStatus = SecItemDelete(fileBase(service: service, account: account) as CFDictionary) if delStatus != errSecSuccess && delStatus != errSecItemNotFound { - writeStderr("migrate-inline: delete failed for \(service) (OSStatus \(delStatus))") + writeStderr("migrate-inline: legacy delete failed for \(service) (OSStatus \(delStatus))") return } - let addAttrs: [CFString: Any] = [ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - kSecAttrAccessControl: buildBiometryAccessControl(), - kSecValueData: valueData, - ] + // Defensive: clear any partial data-protection copy so SecItemAdd cannot + // fail with errSecDuplicateItem. + SecItemDelete(dpBase(service: service, account: account) as CFDictionary) + var addAttrs = dpBase(service: service, account: account) + addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl() + addAttrs[kSecValueData] = valueData let addStatus = SecItemAdd(addAttrs as CFDictionary, nil) if addStatus != errSecSuccess { writeStderr("migrate-inline: re-add failed for \(service) (OSStatus \(addStatus))") @@ -162,6 +224,8 @@ case "list": kSecMatchLimit: kSecMatchLimitAll, kSecReturnAttributes: kCFBooleanTrue!, kSecUseDataProtectionKeychain: kCFBooleanTrue!, + kSecAttrAccessGroup: kAccessGroup as CFString, + kSecAttrSynchronizable: kCFBooleanFalse!, ] var items: [[String: Any]] = [] for query in [fileQuery, dpQuery] { @@ -191,23 +255,20 @@ case "has": // reports that interaction would be required, the item still exists. // // Two passes, because no single prompt-free query covers both keychains: - // items written by `set` carry a biometry ACL, which forces them into the - // data-protection keychain, and a UIFail query without the DP key skips - // the DP keychain entirely — it reports errSecItemNotFound for items that - // exist. The DP pass keeps UIFail (guaranteed no prompt); a present - // DP item then surfaces as errSecInteractionNotAllowed, which already - // counts as "exists". + // new items live in the data-protection keychain (DP pass) while items + // written by a pre-migration helper still live in the file-based one (file + // pass). Both passes use UIFail to guarantee no prompt; a present DP item + // then surfaces as errSecInteractionNotAllowed, which already counts as + // "exists". guard args.count == 4 else { die(2, "Usage: agents-keychain has ") } let (service, account) = (args[2], args[3]) - for dp in [false, true] { - var query: [CFString: Any] = [ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - kSecMatchLimit: kSecMatchLimitOne, - kSecUseAuthenticationUI: kSecUseAuthenticationUIFail, - ] - if dp { query[kSecUseDataProtectionKeychain] = kCFBooleanTrue! } + var fileQuery = fileBase(service: service, account: account) + fileQuery[kSecMatchLimit] = kSecMatchLimitOne + fileQuery[kSecUseAuthenticationUI] = kSecUseAuthenticationUIFail + var dpQuery = dpBase(service: service, account: account) + dpQuery[kSecMatchLimit] = kSecMatchLimitOne + dpQuery[kSecUseAuthenticationUI] = kSecUseAuthenticationUIFail + for query in [fileQuery, dpQuery] { var ignored: AnyObject? let status = SecItemCopyMatching(query as CFDictionary, &ignored) if status == errSecSuccess || status == errSecInteractionNotAllowed { exit(0) } @@ -267,10 +328,12 @@ case "get-batch": } case "set": - // set — value on stdin. Always written device-local - // with the biometry access control. SecItemUpdate cannot change an - // item's ACL, so we delete any existing copy and re-add it; deleting a - // protected item needs no authentication, so set never prompts. + // set — value on stdin. Always written to the + // data-protection keychain, device-local, with the biometry access control. + // SecItemUpdate cannot change an item's ACL, so we delete any existing copy + // (in both keychains, so no stale legacy item shadows the new one) and + // re-add it; deleting a protected item needs no authentication, so set never + // prompts. guard args.count == 4 else { die(2, "Usage: agents-keychain set ") } let (service, account) = (args[2], args[3]) let stdinData = FileHandle.standardInput.readDataToEndOfFile() @@ -280,50 +343,41 @@ case "set": while value.hasSuffix("\n") || value.hasSuffix("\r") { value = String(value.dropLast()) } guard let valueData = value.data(using: .utf8) else { die(2, "Failed to encode value") } - SecItemDelete([ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - ] as CFDictionary) + SecItemDelete(dpBase(service: service, account: account) as CFDictionary) + SecItemDelete(fileBase(service: service, account: account) as CFDictionary) - let addAttrs: [CFString: Any] = [ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - kSecAttrAccessControl: buildBiometryAccessControl(), - kSecValueData: valueData, - ] + var addAttrs = dpBase(service: service, account: account) + addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl() + addAttrs[kSecValueData] = valueData let status = SecItemAdd(addAttrs as CFDictionary, nil) guard status == errSecSuccess else { die(2, "Failed to write keychain item (OSStatus \(status))") } case "delete": - // delete — remove the item. Deletion never decrypts, - // so it never prompts. Exit 0 if something was removed, else 1. + // delete — remove the item from BOTH keychains. New + // items live in the data-protection keychain; an un-migrated legacy copy may + // still sit in the file-based one, so we delete from both to avoid orphans. + // Deletion never decrypts, so it never prompts. Exit 0 if either keychain + // held a copy, else 1. guard args.count == 4 else { die(2, "Usage: agents-keychain delete ") } let (service, account) = (args[2], args[3]) - let status = SecItemDelete([ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - ] as CFDictionary) - exit(status == errSecSuccess ? 0 : 1) + let dpStatus = SecItemDelete(dpBase(service: service, account: account) as CFDictionary) + let fileStatus = SecItemDelete(fileBase(service: service, account: account) as CFDictionary) + exit((dpStatus == errSecSuccess || fileStatus == errSecSuccess) ? 0 : 1) case "migrate-acl": - // migrate-acl — one-time upgrade for items written by - // an older helper that used a trusted-app ACL. Reading such an item may - // pop the legacy password sheet ONCE (the only place a password prompt is - // acceptable); we allow that UI explicitly. We then delete and re-add the - // item with the biometry access control so every future read is Touch ID. + // migrate-acl — one-time upgrade for items written by an + // older helper into the legacy file-based keychain (trusted-app ACL or + // pre-migration layout). Reading such an item may pop the legacy password + // sheet ONCE (the only place a password prompt is acceptable); we allow that + // UI explicitly. We then delete the legacy copy and re-add the item into the + // data-protection keychain with the biometry access control and pinned access + // group so every future read is Touch ID and resolves deterministically. guard args.count == 4 else { die(2, "Usage: agents-keychain migrate-acl ") } let (service, account) = (args[2], args[3]) - let readQuery: [CFString: Any] = [ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - kSecReturnData: kCFBooleanTrue!, - kSecMatchLimit: kSecMatchLimitOne, - kSecUseAuthenticationUI: kSecUseAuthenticationUIAllow, - ] + var readQuery = fileBase(service: service, account: account) + readQuery[kSecReturnData] = kCFBooleanTrue! + readQuery[kSecMatchLimit] = kSecMatchLimitOne + readQuery[kSecUseAuthenticationUI] = kSecUseAuthenticationUIAllow var result: AnyObject? let readStatus = SecItemCopyMatching(readQuery as CFDictionary, &result) if readStatus == errSecItemNotFound { exit(1) } @@ -331,19 +385,12 @@ case "migrate-acl": die(2, "Failed to read legacy keychain item (OSStatus \(readStatus))") } - SecItemDelete([ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - ] as CFDictionary) + SecItemDelete(fileBase(service: service, account: account) as CFDictionary) + SecItemDelete(dpBase(service: service, account: account) as CFDictionary) - let addAttrs: [CFString: Any] = [ - kSecClass: kSecClassGenericPassword, - kSecAttrService: service as CFString, - kSecAttrAccount: account as CFString, - kSecAttrAccessControl: buildBiometryAccessControl(), - kSecValueData: valueData, - ] + var addAttrs = dpBase(service: service, account: account) + addAttrs[kSecAttrAccessControl] = buildBiometryAccessControl() + addAttrs[kSecValueData] = valueData let addStatus = SecItemAdd(addAttrs as CFDictionary, nil) guard addStatus == errSecSuccess else { die(2, "Failed to rewrite item with biometry ACL (OSStatus \(addStatus))") }