From eee3c96e8ef057d6df496a6a5a451cebf822f59d Mon Sep 17 00:00:00 2001 From: Justin Purnell Date: Wed, 28 Oct 2026 16:42:58 -0400 Subject: [PATCH 1/2] refactor(keychain)!: keep the credential key with SwiftKeychain CredentialStoreKey now reads and creates its key through SwiftKeychain's KeychainKey.loadOrCreate, which adds a new key rather than storing over one, so two processes starting at once agree on a key. The hand-written SecItem code, KeychainAccess, SystemKeychain and CredentialKeyError are gone in favour of SwiftKeychain's types. The item is the one every earlier version wrote: same service and account, after first unlock on this device only, not synchronized, no access group, and on macOS the login keychain (SystemKeychain(dataProtection: false)). Tests pin each of those and read a key seeded at the old identity. SwiftKeychain is depended on for macOS, iOS and visionOS only; Linux, tvOS and watchOS compile the key out. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PtLUw634m85L9ucJTrftoD --- Package.resolved | 11 +- Package.swift | 14 ++ Sources/MCPClient/CredentialStoreKey.swift | 209 ++++------------ Sources/MCPClient/MCPOAuthSession.swift | 4 +- .../CredentialStoreKeyTests.swift | 230 ++++++++++-------- Tests/MCPClientTests/LiveApolloTests.swift | 2 +- 6 files changed, 201 insertions(+), 269 deletions(-) diff --git a/Package.resolved b/Package.resolved index 69616db..83af542 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "9b200c4bb58354a1395bedae996bffb7cc7f7f25f53c9d40c9f09d299a25cba4", + "originHash" : "9f325eab38ad27656a272b2b1c68919b7cecfd60ca01b13df8e94bd32cb281d7", "pins" : [ { "identity" : "async-http-client", @@ -244,6 +244,15 @@ "version" : "1.6.4" } }, + { + "identity" : "swiftkeychain", + "kind" : "remoteSourceControl", + "location" : "https://github.com/jpurnell/SwiftKeychain.git", + "state" : { + "revision" : "9f6215e622ce7825554302e16543340c7deae383", + "version" : "0.1.1" + } + }, { "identity" : "websocket-kit", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index 9d9e9d3..465c38d 100644 --- a/Package.swift +++ b/Package.swift @@ -19,6 +19,12 @@ var targets: [Target] = [ // does, so the dependency is declared rather than borrowed. .product(name: "NIOSSL", package: "swift-nio-ssl"), .product(name: "WebSocketKit", package: "websocket-kit"), + // The Keychain, for the one key that seals the credential files. Apple-only: + // there is no Keychain on Linux, and SwiftKeychain imports Security. tvOS and + // watchOS are left out because SwiftKeychain does not support them. + .product( + name: "SwiftKeychain", package: "SwiftKeychain", + condition: .when(platforms: [.macOS, .iOS, .visionOS])), ], // Declared rather than left implicit. SwiftPM does not claim this catalog on a // plain build and warns that it is unhandled; the obvious quietening — `exclude` — @@ -51,6 +57,10 @@ var targets: [Target] = [ .product(name: "X509", package: "swift-certificates"), .product(name: "SwiftASN1", package: "swift-asn1"), .product(name: "Crypto", package: "swift-crypto"), + // `InMemoryKeychain`, so the credential key's tests never touch the real Keychain. + .product( + name: "SwiftKeychain", package: "SwiftKeychain", + condition: .when(platforms: [.macOS, .iOS, .visionOS])), ] ), ] @@ -142,6 +152,10 @@ let package = Package( .package(url: "https://github.com/apple/swift-asn1.git", from: "1.6.0"), .package(url: "https://github.com/vapor/websocket-kit.git", from: "2.15.0"), .package(url: "https://github.com/swiftlang/swift-docc-plugin", from: "1.4.3"), + // Where the credential store's key lives, in place of SecItem code written here. No + // traits: the core store and `KeychainKey` are all this needs, and SwiftPM resolves + // nothing a disabled trait names. + .package(url: "https://github.com/jpurnell/SwiftKeychain.git", from: "0.1.1"), ], targets: targets ) diff --git a/Sources/MCPClient/CredentialStoreKey.swift b/Sources/MCPClient/CredentialStoreKey.swift index be3eb74..c09185b 100644 --- a/Sources/MCPClient/CredentialStoreKey.swift +++ b/Sources/MCPClient/CredentialStoreKey.swift @@ -1,136 +1,7 @@ -#if canImport(Security) +#if canImport(SwiftKeychain) import Foundation import Crypto -import Security - -/// Why the credential store's key could not be obtained. -public enum CredentialKeyError: Error, Equatable, Sendable { - - /// The Keychain refused the read, the write, or the delete. - /// - /// Carries the `OSStatus`, because "Keychain failed" is unactionable and the status is - /// the only thing distinguishing a locked keychain from a missing entitlement. - case keychain(OSStatus) - - /// The stored item was not a key of the expected size. - /// - /// Refused rather than replaced. Replacing it would mint a new key and quietly make every - /// stored credential undecryptable, which reaches the user as an unexplained request to - /// sign in again. - case malformedKey -} - -/// Somewhere small secrets are kept. -/// -/// A protocol so the logic above it can be tested. The real Keychain prompts, behaves -/// differently under an unsigned test bundle, and leaves items on the machine that ran the -/// tests — none of which exercises the behaviour that matters here, which is what happens on -/// the *second* launch and when a read is refused. -public protocol KeychainAccess: Sendable { - - /// The data stored for a service and account, if any. - func data(service: String, account: String) throws -> Data? - - /// Stores data, replacing anything already there. - func store(_ data: Data, service: String, account: String) throws - - /// Removes stored data. Removing what is not there is not an error. - func delete(service: String, account: String) throws -} - -/// The system Keychain. -public struct SystemKeychain: KeychainAccess { - - /// Creates an accessor. - public init() {} - - /// Reads an item. - /// - /// - Parameters: - /// - service: The service the item is filed under. - /// - account: The account name. - /// - Returns: The data, or `nil` if there is no such item. - /// - Throws: ``CredentialKeyError/keychain(_:)``. - public func data(service: String, account: String) throws -> Data? { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne - ] - - var item: CFTypeRef? - let status = SecItemCopyMatching(query as CFDictionary, &item) - - switch status { - case errSecSuccess: - return item as? Data - case errSecItemNotFound: - // First run, not a failure. - return nil - default: - throw CredentialKeyError.keychain(status) - } - } - - /// Stores an item, replacing any existing one. - /// - /// - Parameters: - /// - data: What to store. - /// - service: The service to file it under. - /// - account: The account name. - /// - Throws: ``CredentialKeyError/keychain(_:)``. - public func store(_ data: Data, service: String, account: String) throws { - let attributes: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecValueData as String: data, - // Never leaves this device, and is unavailable until the device has been unlocked - // once. A credential key that syncs is a credential key on every device the user - // owns, including the ones they stopped using. - kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly - ] - - var status = SecItemAdd(attributes as CFDictionary, nil) - - if status == errSecDuplicateItem { - // Replace rather than fail. An add that collides means something is already - // filed here, and the caller asked for this value to be the one stored. - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account - ] - status = SecItemUpdate( - query as CFDictionary, - [kSecValueData as String: data] as CFDictionary) - } - - guard status == errSecSuccess else { - throw CredentialKeyError.keychain(status) - } - } - - /// Removes an item. - /// - /// - Parameters: - /// - service: The service the item is filed under. - /// - account: The account name. - /// - Throws: ``CredentialKeyError/keychain(_:)``. - public func delete(service: String, account: String) throws { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account - ] - let status = SecItemDelete(query as CFDictionary) - guard status == errSecSuccess || status == errSecItemNotFound else { - throw CredentialKeyError.keychain(status) - } - } -} +import SwiftKeychain /// The key that `EncryptedFileClientStorage` seals credentials with, kept in the Keychain. /// @@ -142,23 +13,36 @@ public struct SystemKeychain: KeychainAccess { /// connection and a prompt surface that grows with them; putting the key beside the file /// protects nothing at all. One key in the Keychain, one encrypted file next to the app's /// other state. -public struct CredentialStoreKey { - - /// The service the key is filed under. - static let service = "com.swiftmcpclient.credential-store" - - /// The account name for the key item. - static let account = "credential-encryption-key" - - /// A 256-bit key, in bytes. - private static let keyByteCount = 32 +/// +/// The Keychain itself is SwiftKeychain's: pass any `KeychainAccess`, such as its +/// `InMemoryKeychain` in tests. Errors are SwiftKeychain's `KeychainError`. +public struct CredentialStoreKey: Sendable { - private let keychain: any KeychainAccess + /// The Keychain item the key is kept in. + /// + /// Fixed, and the same item every earlier version of this package wrote: a key filed + /// anywhere else is a key that is not found, and a key that is not found is replaced by a + /// new one that cannot open any credential sealed before it. Kept on this device, readable + /// after the first unlock, never synchronized, in the application's own access group. + public static let item = KeychainItem( + service: "com.swiftmcpclient.credential-store", + account: "credential-encryption-key", + accessibility: .afterFirstUnlockThisDeviceOnly, + synchronizable: false, + accessGroup: nil) + + /// Where the key is kept. + public let keychain: any KeychainAccess /// Creates an accessor for the credential key. /// - /// - Parameter keychain: Where the key is kept. Defaults to the system Keychain. - public init(keychain: any KeychainAccess = SystemKeychain()) { + /// The default is the system Keychain without the data-protection keychain, which on + /// macOS means the login keychain. That is where every earlier version stored the key, and + /// where an unsigned command-line build can store one at all. On iOS there is only one + /// keychain and the choice is ignored. + /// + /// - Parameter keychain: Where the key is kept. + public init(keychain: any KeychainAccess = SystemKeychain(dataProtection: false)) { self.keychain = keychain } @@ -169,37 +53,26 @@ public struct CredentialStoreKey { /// stored credential undecryptable and sends the user back to a sign-in screen with no /// explanation. /// + /// A stored item of the wrong size is refused rather than replaced, a refused read + /// (`KeychainError.locked`, for one) is thrown rather than read as "no key yet", and a new + /// key is only added, never stored over another: if a second process created one first, + /// that one is returned. + /// /// - Returns: The key. - /// - Throws: ``CredentialKeyError``. - public func loadOrCreate() throws -> SymmetricKey { - if let stored = try keychain.data(service: Self.service, account: Self.account) { - guard stored.count == Self.keyByteCount else { - // Not replaced. See `malformedKey`. - throw CredentialKeyError.malformedKey - } - return SymmetricKey(data: stored) - } - - let created = SymmetricKey(size: .bits256) - // `Data.init` passed directly rather than wrapped in a closure: a closure whose result - // mentions the borrowed buffer cannot be told apart from one that lets it escape, and - // the initialiser already takes a sequence of bytes. - let bytes = created.withUnsafeBytes(Data.init) - - // Stored before it is returned. Handing back a key that was never written would work - // for this launch and be gone by the next one, taking every credential with it. - try keychain.store(bytes, service: Self.service, account: Self.account) - return created + /// - Throws: `KeychainError`. + public func loadOrCreate() throws(KeychainError) -> SymmetricKey { + SymmetricKey(data: try KeychainKey.loadOrCreate(Self.item, using: keychain)) } /// Removes the stored key. /// /// The credential file becomes undecryptable, which is the intended effect: this is how a - /// caller discards every stored credential at once without reaching each one. + /// caller discards every stored credential at once without reaching each one. Removing a + /// key that is not there is not an error. /// - /// - Throws: ``CredentialKeyError/keychain(_:)``. - public func delete() throws { - try keychain.delete(service: Self.service, account: Self.account) + /// - Throws: `KeychainError`. + public func delete() throws(KeychainError) { + try keychain.delete(Self.item) } } #endif diff --git a/Sources/MCPClient/MCPOAuthSession.swift b/Sources/MCPClient/MCPOAuthSession.swift index 730f7bf..a9df1ed 100644 --- a/Sources/MCPClient/MCPOAuthSession.swift +++ b/Sources/MCPClient/MCPOAuthSession.swift @@ -63,7 +63,7 @@ public actor MCPOAuthSession { self.tokenTransport = tokenTransport } - #if canImport(Security) + #if canImport(SwiftKeychain) /// Creates a session that keeps its credential across launches. /// /// The credential file is sealed with a key from the Keychain — one small key there, and @@ -82,7 +82,7 @@ public actor MCPOAuthSession { /// deployment already has — which is the decision this method takes on an Apple platform. /// /// - Returns: A session backed by encrypted storage. - /// - Throws: ``CredentialKeyError`` if the Keychain refused, or a file error. + /// - Throws: SwiftKeychain's `KeychainError` if the Keychain refused, or a file error. public static func persistent( directory: URL? = nil, setup: MCPOAuthSetup = MCPOAuthSetup() diff --git a/Tests/MCPClientTests/CredentialStoreKeyTests.swift b/Tests/MCPClientTests/CredentialStoreKeyTests.swift index d2503f9..014fe39 100644 --- a/Tests/MCPClientTests/CredentialStoreKeyTests.swift +++ b/Tests/MCPClientTests/CredentialStoreKeyTests.swift @@ -1,135 +1,189 @@ -#if canImport(Security) +#if canImport(SwiftKeychain) import Foundation import Testing import Crypto -import Security -@testable import MCPClient +import SwiftKeychain +import os +import MCPClient /// The key that every stored credential is sealed with. /// /// Its failure modes are quiet ones. A key that changes between launches does not report an /// error — it makes the credential file undecryptable, and the user is asked to sign in again -/// with no explanation. So the tests here are mostly about *not* silently minting a new key. +/// with no explanation. So the tests here are mostly about *not* silently minting a new key, +/// and about finding the key that earlier versions of this package stored. @Suite("Credential store key") struct CredentialStoreKeyTests { + /// The service every version up to 0.14.0 filed the key under. Written out, not read from + /// the type under test, so a change to the type cannot quietly move this too. + static let legacyService = "com.swiftmcpclient.credential-store" + + /// The account every version up to 0.14.0 filed the key under. + static let legacyAccount = "credential-encryption-key" + + // MARK: - Compatibility with the key earlier versions stored + + /// The item's identity is what finds an existing user's key. Every field is pinned: + /// service and account name it; the accessibility class is what 0.14.0 wrote; an item + /// that synchronized, or sat in an access group, would be a different item, and the old + /// one — with every credential sealed by it — would be left where nothing reads it. + @Test("The key's Keychain item is the one earlier versions stored") + func itemIdentityIsPinned() { + let item = CredentialStoreKey.item + + #expect(item.service == Self.legacyService) + #expect(item.account == Self.legacyAccount) + #expect(item.accessibility == .afterFirstUnlockThisDeviceOnly) + #expect(item.synchronizable == false) + #expect(item.accessGroup == nil) + #expect(item == KeychainItem( + service: Self.legacyService, + account: Self.legacyAccount, + accessibility: .afterFirstUnlockThisDeviceOnly, + synchronizable: false, + accessGroup: nil)) + } + + /// Earlier versions wrote the key without `kSecUseDataProtectionKeychain`, which on macOS + /// is the login (file-based) keychain. The data-protection keychain is a different store: + /// reading there finds nothing, and the next step would mint a new key. On iOS the flag is + /// ignored, since there is only one keychain. + @Test("By default the key is read from the keychain earlier versions wrote it to") + func defaultKeychainIsTheLoginKeychain() throws { + let system = try #require( + CredentialStoreKey().keychain as? SystemKeychain, + "the default store is not the system Keychain") + #expect(system.dataProtection == false, "the data-protection keychain does not hold the existing key") + } + + /// The upgrade, end to end against a stand-in: a key stored at the old identity by an + /// earlier version is the key this version returns, byte for byte, and nothing is added. + @Test("A key stored by an earlier version is found and returned unchanged") + func findsKeyStoredByEarlierVersion() throws { + let keychain = InMemoryKeychain() + let existing = Data((0..<32).map { UInt8($0) }) + try keychain.store(existing, as: KeychainItem(service: Self.legacyService, account: Self.legacyAccount)) + + let key = try CredentialStoreKey(keychain: keychain).loadOrCreate() + + #expect(key == SymmetricKey(data: existing), "a different key was returned; every stored credential is now unreadable") + #expect(try keychain.accounts(service: Self.legacyService) == [Self.legacyAccount]) + } + + // MARK: - Behaviour + /// First run: there is no key, so one is created and kept. @Test("A key is created on first use and stored") func createsOnFirstUse() throws { - let keychain = FakeKeychain() - let store = CredentialStoreKey(keychain: keychain) + let keychain = InMemoryKeychain() - let key = try store.loadOrCreate() + let key = try CredentialStoreKey(keychain: keychain).loadOrCreate() #expect(key.bitCount == 256) - #expect(keychain.items.count == 1, "the key was not persisted") + #expect(try keychain.data(for: CredentialStoreKey.item)?.count == 32, "the key was not persisted") } /// The one that matters. A second launch must get the *same* key — a new one each time - /// makes every stored credential unreadable, and that surfaces as an unexplained request - /// to sign in again rather than as an error anyone can act on. + /// makes every stored credential unreadable. @Test("The same key comes back on a later launch") func sameKeyOnSecondLaunch() throws { - let keychain = FakeKeychain() + let keychain = InMemoryKeychain() let first = try CredentialStoreKey(keychain: keychain).loadOrCreate() // A separate instance, as though the process had restarted. let second = try CredentialStoreKey(keychain: keychain).loadOrCreate() #expect(first == second, "a different key was returned; every credential is now unreadable") - #expect(keychain.items.count == 1, "a second key was stored over the first") + #expect(try keychain.accounts(service: Self.legacyService) == [Self.legacyAccount]) } /// Two keys generated fresh must differ, or "generation" is not generating anything. @Test("Separate keychains get separate keys") func separateKeychainsDiffer() throws { - let first = try CredentialStoreKey(keychain: FakeKeychain()).loadOrCreate() - let second = try CredentialStoreKey(keychain: FakeKeychain()).loadOrCreate() + let first = try CredentialStoreKey(keychain: InMemoryKeychain()).loadOrCreate() + let second = try CredentialStoreKey(keychain: InMemoryKeychain()).loadOrCreate() #expect(first != second) } /// A stored item of the wrong size must be refused rather than replaced. Replacing it - /// would be the silent path to an unreadable credential file; throwing at least says - /// which layer failed. + /// would be the silent path to an unreadable credential file. @Test("A malformed stored key is refused, not replaced") - func malformedKeyRefused() { - let keychain = FakeKeychain() - keychain.items[FakeKeychain.Key(service: CredentialStoreKey.service, - account: CredentialStoreKey.account)] = Data([1, 2, 3]) + func malformedKeyRefused() throws { + let keychain = InMemoryKeychain() + let malformed = Data([1, 2, 3]) + try keychain.store(malformed, as: CredentialStoreKey.item) - let store = CredentialStoreKey(keychain: keychain) - #expect(throws: CredentialKeyError.malformedKey) { - try store.loadOrCreate() + let error = #expect(throws: KeychainError.self) { + try CredentialStoreKey(keychain: keychain).loadOrCreate() } - #expect(keychain.items.count == 1, "the malformed item was overwritten") + guard case .malformed = error else { + Issue.record("expected .malformed, got \(String(describing: error))") + return + } + #expect(try keychain.data(for: CredentialStoreKey.item) == malformed, "the malformed item was overwritten") } - /// A keychain that refuses must surface the status. "Keychain failed" is unactionable; - /// the status is what distinguishes a locked keychain from a missing entitlement. - @Test("A refusing keychain surfaces its status") - func refusalSurfacesStatus() { - let keychain = FakeKeychain() - keychain.readStatus = errSecInteractionNotAllowed + /// A locked keychain must surface as locked, never as "no key yet" — that reading would + /// mint a new key and replace the real one. + @Test("A locked keychain is reported, not read as empty") + func lockedKeychainReported() { + let keychain = RefusingKeychain(refuseReads: .locked) - #expect(throws: CredentialKeyError.keychain(errSecInteractionNotAllowed)) { + #expect(throws: KeychainError.locked) { try CredentialStoreKey(keychain: keychain).loadOrCreate() } + #expect(keychain.attemptedWrite == false, "a key was written while the keychain was locked") } - /// A write that fails must throw rather than handing back a key that was never stored — + /// A write that fails must throw rather than hand back a key that was never stored — /// that key would work for this launch and be gone by the next one. @Test("A failed write is reported rather than returning an unstored key") func failedWriteReported() { - let keychain = FakeKeychain() - keychain.writeStatus = errSecDuplicateItem + let keychain = RefusingKeychain(refuseWrites: .missingEntitlement) - #expect(throws: CredentialKeyError.keychain(errSecDuplicateItem)) { + #expect(throws: KeychainError.missingEntitlement) { try CredentialStoreKey(keychain: keychain).loadOrCreate() } } - /// Deleting the key is how a caller discards every credential at once, without having to - /// reach each one. + /// Deleting the key is how a caller discards every credential at once. @Test("Deleting the key removes it") func deleteRemovesKey() throws { - let keychain = FakeKeychain() + let keychain = InMemoryKeychain() let store = CredentialStoreKey(keychain: keychain) - _ = try store.loadOrCreate() - #expect(keychain.items.count == 1) - + let first = try store.loadOrCreate() try store.delete() - #expect(keychain.items.isEmpty) + #expect(try keychain.data(for: CredentialStoreKey.item) == nil) // And the next call mints a fresh one rather than failing. - _ = try store.loadOrCreate() - #expect(keychain.items.count == 1) + let second = try store.loadOrCreate() + #expect(first != second) + #expect(try keychain.data(for: CredentialStoreKey.item)?.count == 32, "the fresh key was not persisted") } - /// Deleting a key that is not there is not an error — a user pressing "sign out" twice - /// should not see a failure. + /// Deleting a key that is not there is not an error — "sign out" pressed twice should not + /// fail. @Test("Deleting an absent key is harmless") func deleteAbsentKeyIsHarmless() throws { - let keychain = FakeKeychain() + let keychain = InMemoryKeychain() let store = CredentialStoreKey(keychain: keychain) try store.delete() - #expect(keychain.items.isEmpty) - // And the store is still usable afterwards, rather than left in some half-state. let key = try store.loadOrCreate() #expect(key.bitCount == 256) - #expect(keychain.items.count == 1) } /// The key round-trips through storage byte for byte, or the credential file sealed with /// it on one launch cannot be opened on the next. @Test("The stored bytes reconstruct the same key") func storedBytesReconstructKey() throws { - let keychain = FakeKeychain() + let keychain = InMemoryKeychain() let created = try CredentialStoreKey(keychain: keychain).loadOrCreate() - let stored = try #require(keychain.items.values.first) + let stored = try #require(try keychain.data(for: CredentialStoreKey.item)) #expect(stored.count == 32) #expect(SymmetricKey(data: stored) == created) } @@ -137,65 +191,47 @@ struct CredentialStoreKeyTests { // MARK: - Helpers -/// A keychain that lives in memory. +/// A keychain that refuses reads, writes, or both, with a chosen error, and holds nothing. /// -/// The real one prompts, behaves differently under an unsigned test bundle, and leaves items -/// on the developer's machine. None of that exercises the logic being tested here, which is -/// entirely about what happens on the second launch and on refusal. -// Justification: every stored property is private and reached only under `lock`. -private final class FakeKeychain: KeychainAccess, @unchecked Sendable { +/// `InMemoryKeychain` never refuses, and the cases worth testing are the refusals: a locked +/// device and an unsigned process. +private final class RefusingKeychain: KeychainAccess { - struct Key: Hashable { - let service: String - let account: String - } - - private let lock = NSLock() - private var storage: [Key: Data] = [:] - private var injectedReadStatus: OSStatus? - private var injectedWriteStatus: OSStatus? + private let readError: KeychainError? + private let writeError: KeychainError? + private let wrote = OSAllocatedUnfairLock(initialState: false) - /// A status to return from reads instead of succeeding. - var readStatus: OSStatus? { - get { withLock { injectedReadStatus } } - set { withLock { injectedReadStatus = newValue } } + init(refuseReads readError: KeychainError? = nil, refuseWrites writeError: KeychainError? = nil) { + self.readError = readError + self.writeError = writeError } - /// A status to return from writes instead of succeeding. - var writeStatus: OSStatus? { - get { withLock { injectedWriteStatus } } - set { withLock { injectedWriteStatus = newValue } } - } + /// Whether anything tried to store or add an item. + var attemptedWrite: Bool { wrote.withLock { $0 } } - var items: [Key: Data] { - get { withLock { storage } } - set { withLock { storage = newValue } } + func data(for item: KeychainItem) throws(KeychainError) -> Data? { + if let readError { throw readError } + return nil } - private func withLock(_ body: () throws -> T) rethrows -> T { - lock.lock() - defer { lock.unlock() } - return try body() + func store(_ data: Data, as item: KeychainItem) throws(KeychainError) { + wrote.withLock { $0 = true } + if let writeError { throw writeError } } - func data(service: String, account: String) throws -> Data? { - try withLock { - if let injectedReadStatus { throw CredentialKeyError.keychain(injectedReadStatus) } - return storage[Key(service: service, account: account)] - } + func add(_ data: Data, as item: KeychainItem) throws(KeychainError) -> Bool { + wrote.withLock { $0 = true } + if let writeError { throw writeError } + return true } - func store(_ data: Data, service: String, account: String) throws { - try withLock { - if let injectedWriteStatus { throw CredentialKeyError.keychain(injectedWriteStatus) } - storage[Key(service: service, account: account)] = data - } + func delete(_ item: KeychainItem) throws(KeychainError) { + if let writeError { throw writeError } } - func delete(service: String, account: String) throws { - withLock { - _ = storage.removeValue(forKey: Key(service: service, account: account)) - } + func accounts(service: String) throws(KeychainError) -> [String] { + if let readError { throw readError } + return [] } } #endif diff --git a/Tests/MCPClientTests/LiveApolloTests.swift b/Tests/MCPClientTests/LiveApolloTests.swift index 35d7667..13a9334 100644 --- a/Tests/MCPClientTests/LiveApolloTests.swift +++ b/Tests/MCPClientTests/LiveApolloTests.swift @@ -1,4 +1,4 @@ -#if canImport(Security) +#if canImport(SwiftKeychain) import Foundation import Testing import SwiftOAuthClient From 2aba97e8e1def8c3893d97652d90fed06567ee13 Mon Sep 17 00:00:00 2001 From: Justin Purnell Date: Wed, 28 Oct 2026 16:52:02 -0400 Subject: [PATCH 2/2] docs: the Keychain types are SwiftKeychain's (0.15.0, breaking) CHANGELOG Unreleased gains a Breaking section; the migration guide maps each removed name to SwiftKeychain's and says stored credentials are untouched; README and persistent()'s documentation say which platforms keep the key in the Keychain and where it lives on macOS. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PtLUw634m85L9ucJTrftoD --- CHANGELOG.md | 33 ++++++++ README.md | 10 ++- .../MCPClient.docc/MigrationGuide.md | 79 +++++++++++++++++++ Sources/MCPClient/MCPOAuthSession.swift | 3 +- 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18a65bd..a64ec36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,28 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Breaking — the Keychain types are SwiftKeychain's (0.15.0) +- **`KeychainAccess`, `SystemKeychain` and `CredentialKeyError` are removed.** + `CredentialStoreKey` now keeps its key through + [SwiftKeychain](https://github.com/jpurnell/SwiftKeychain) 0.1.1, and takes SwiftKeychain's + `KeychainAccess` (`SystemKeychain`, `InMemoryKeychain`, or your own). MCPClient does not + re-export them: code that names them depends on SwiftKeychain and imports it. See + "Migrating to 0.15.0" in the migration guide for each old name's replacement. +- **`CredentialStoreKey.loadOrCreate()` and `delete()` throw `KeychainError`**, typed. + `CredentialKeyError.keychain(OSStatus)` becomes `.locked` (`errSecInteractionNotAllowed`), + `.missingEntitlement`, or `.status(Int32)`; `.malformedKey` becomes `.malformed(String)`. + `MCPOAuthSession.persistent()` throws them too. +- **`CredentialStoreKey` and `MCPOAuthSession.persistent()` are macOS, iOS and visionOS + only.** They were compiled wherever `Security` could be imported, which included tvOS and + watchOS; SwiftKeychain does not support those, so there they are now compiled out as on + Linux. +- **Stored credentials are not affected.** The key is read from the item every earlier version + wrote: service `com.swiftmcpclient.credential-store`, account `credential-encryption-key`, + after first unlock on this device only, not synchronized, no access group, and on macOS the + login keychain (`SystemKeychain(dataProtection: false)`), not the data-protection keychain. + `CredentialStoreKey.item` and the default `keychain` are public, and tests pin every field + of both. + ### Security - **A redirect could send a request — custom headers, session id and body — to any origin, and from `https` to `http` (CWE-200, CWE-522, CWE-319).** `StreamableHTTPTransport` and @@ -125,6 +147,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). a couple of short words, and a key in the host name. ### Changed +- **Two processes starting at once now agree on the key.** A new key is added, never stored + over another; the process that loses the race reads the winner's key + (`KeychainKey.loadOrCreate`). Before, each stored its own and the second replaced the first, + stranding anything the first had already sealed. +- The hand-written `SecItem` code is gone. `Package.resolved` gains one package, + `swiftkeychain`, with no traits enabled; it is linked on macOS, iOS and visionOS only. - **Cross-origin redirects are no longer followed.** A deployment that answered the configured URL with a redirect to another host or port — or from `http://` to `https://` — worked before and fails now with `MCPError.redirectRejected`. Configure the transport with the URL @@ -235,6 +263,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). server adds. ### Tests +- `CredentialStoreKeyTests` run against SwiftKeychain's `InMemoryKeychain` instead of a fake + of their own, through the public API rather than `@testable`. New: the item's identity is + pinned field by field against literal strings; the default store is + `SystemKeychain(dataProtection: false)`; a key seeded at the old identity is returned + unchanged; a locked keychain throws `.locked` and writes nothing. - `TransportRedirectWireTests` — two loopback servers, every request each HTTP transport makes × `301`/`302`/`303`/`307`/`308`, asserting on what the *second* server received; the same with an `authorization:` provider; four spellings of "elsewhere"; an HTTPS first server diff --git a/README.md b/README.md index 3a45332..897e5e0 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,13 @@ A Swift 6 client library for the [Model Context Protocol (MCP)](https://modelcon - macOS 14+ / iOS 17+ / tvOS 17+ / watchOS 10+ - Linux (Ubuntu 22.04+) — built and tested in CI +`MCPOAuthSession.persistent()` and `CredentialStoreKey`, which keep the key that seals stored +credentials in the Keychain, are available on macOS, iOS and visionOS. They use +[SwiftKeychain](https://github.com/jpurnell/SwiftKeychain) and take its `KeychainAccess`, so +code that passes its own store (an `InMemoryKeychain` in tests, say) imports `SwiftKeychain`. +On Linux, tvOS and watchOS, build the session from `EncryptedFileClientStorage` and +`EncryptedFileRegistrationStore` with a key from that platform's own secret store. + ## Installation Add to your `Package.swift`: @@ -303,7 +310,8 @@ version, and a Developer ID signature. The signature is not decoration. `MCPOAuthSession` seals its credentials with a key from the Keychain, and the Keychain decides what may read that key by the program's code identity. An unsigned build has a new identity every time it is compiled, so every rebuild would lose the -saved OAuth tokens. A signed one keeps them. +saved OAuth tokens. A signed one keeps them. The key is in the login keychain, not the +data-protection keychain, which an unsigned build cannot write to at all. The first Developer ID signing on a machine raises a system dialog asking to use the key, so run that command from a terminal you are sitting at and choose **Always Allow**. It is diff --git a/Sources/MCPClient/MCPClient.docc/MigrationGuide.md b/Sources/MCPClient/MCPClient.docc/MigrationGuide.md index 3c8b74a..ff2c2f7 100644 --- a/Sources/MCPClient/MCPClient.docc/MigrationGuide.md +++ b/Sources/MCPClient/MCPClient.docc/MigrationGuide.md @@ -396,3 +396,82 @@ needs one more arm. - **A provider that returns `nil`.** With an `authorization:` provider that returns `nil`, a static `Authorization` in `headers` is now left off the Streamable HTTP server stream's `GET`, as it already was off every `POST`. + +## Migrating to 0.15.0: the Keychain types are SwiftKeychain's + +``CredentialStoreKey`` keeps its key through +[SwiftKeychain](https://github.com/jpurnell/SwiftKeychain) instead of Keychain code +written here. **Stored credentials are unaffected:** the key is read from the same Keychain +item every earlier version wrote — the same service and account, readable after the first +unlock on this device only, not synchronized, in the application's own access group, and on +macOS in the login keychain rather than the data-protection keychain. Nothing is migrated, +and nobody is asked to sign in again. + +What changes is the API around it. MCPClient no longer declares `KeychainAccess`, +`SystemKeychain` or `CredentialKeyError`; SwiftKeychain's types take their place, and +MCPClient does not re-export them. Code that names any of them adds the dependency and +imports `SwiftKeychain` itself. In `Package.swift`, beside the dependency on this package: + + +```swift +.package(url: "https://github.com/jpurnell/SwiftKeychain.git", from: "0.1.1"), +``` + +Code that only calls ``CredentialStoreKey/loadOrCreate()`` or +`MCPOAuthSession.persistent()` and catches errors generically needs no change. + +| Before (0.14.0) | After (0.15.0) | +|---|---| +| `MCPClient.KeychainAccess` | `SwiftKeychain.KeychainAccess` | +| `MCPClient.SystemKeychain()` | `SwiftKeychain.SystemKeychain(dataProtection: false)` — the default ``CredentialStoreKey`` uses | +| a test double conforming to `KeychainAccess` | `SwiftKeychain.InMemoryKeychain()` | +| `CredentialKeyError.keychain(OSStatus)` | `KeychainError.locked`, `.missingEntitlement` or `.status(Int32)` | +| `CredentialKeyError.malformedKey` | `KeychainError.malformed(String)` | +| `throws` (untyped) | `throws(KeychainError)` | + +### Before (0.14.0) + + +```swift +import MCPClient + +do { + let key = try CredentialStoreKey(keychain: SystemKeychain()).loadOrCreate() + use(key) +} catch CredentialKeyError.keychain(let status) where status == errSecInteractionNotAllowed { + showLockedBanner() +} catch CredentialKeyError.malformedKey { + showCorruptKeyBanner() +} +``` + +### After (0.15.0) + + +```swift +import MCPClient +import SwiftKeychain + +do { + let key = try CredentialStoreKey().loadOrCreate() + use(key) +} catch .locked { + showLockedBanner() +} catch .malformed { + showCorruptKeyBanner() +} catch { + showKeychainFailure(error) +} +``` + +In tests, `CredentialStoreKey(keychain: InMemoryKeychain())` replaces a hand-written fake. +A fake of your own now conforms to SwiftKeychain's `KeychainAccess`, whose methods take a +`KeychainItem` rather than a service and an account; ``CredentialStoreKey/item`` is the item +the key lives in. + +### Platforms + +``CredentialStoreKey`` and `MCPOAuthSession.persistent()` exist on macOS, iOS and visionOS, +the platforms SwiftKeychain supports. On tvOS and watchOS they are no longer compiled, as on +Linux: construct the session with `EncryptedFileClientStorage` and +``EncryptedFileRegistrationStore`` and a key from the platform's own secret store. diff --git a/Sources/MCPClient/MCPOAuthSession.swift b/Sources/MCPClient/MCPOAuthSession.swift index a9df1ed..c7bd743 100644 --- a/Sources/MCPClient/MCPOAuthSession.swift +++ b/Sources/MCPClient/MCPOAuthSession.swift @@ -76,7 +76,8 @@ public actor MCPOAuthSession { /// support directory. /// - setup: How discovery is performed. Injected for tests. /// Available only where a Keychain is: the encrypted stores themselves are portable, but - /// *where the key lives* is a platform decision and this convenience makes it. On Linux, + /// *where the key lives* is a platform decision and this convenience makes it. That means + /// macOS, iOS and visionOS, where SwiftKeychain runs. On Linux, tvOS and watchOS, /// construct the session directly with `EncryptedFileClientStorage` and /// ``EncryptedFileRegistrationStore``, supplying a key from whatever secret store that /// deployment already has — which is the decision this method takes on an Apple platform.