From 2ee5508ec22420f921d67e74c8eb86dae61722ca Mon Sep 17 00:00:00 2001 From: Jamil Date: Wed, 27 Mar 2024 09:14:30 -0700 Subject: [PATCH] fix(apple): Use keychain from the tunnel process *only* (#4335) This fixes another long-standing bug with the Apple client: Keychain groups. Apple's Keychain docs are woefully unclear and lacking on the Keychain. These are the main takeaways: - Apple wants you to use the "[Data protection keychain](https://developer.apple.com/documentation/security/ksecusedataprotectionkeychain)" on macOS which allows it to behave like an iOS keychain. That opens up the door for possible to sync to iCloud (which we don't use). - Data protection keychain items, [it appears](https://forums.developer.apple.com/forums/thread/710758), cannot be created by Network Extensions. - However, we _can_ save to the regular keychain (by default the system keychain for root procs like us), which is file-based. - Keychain items can be shared (both read/write) between apps, but **not between users**. The tunnel process and gui process run as different users. The only way for this to happen is to use the old file-based Keychain and use [very deprecated](https://developer.apple.com/documentation/technotes/tn3137-on-mac-keychains) APIs to allow both "users" access, which is what we were doing before. - To fix this, we limit all keychain operations to the tunnel proc only. The GUI passes the auth token in during the `startTunnel` call, which the system then passes to our `PacketTunnelProvider` class. This uses the file-based Keychain, but since we need to use that keychain as the root tunnel proc, we don't have much choice. The "Allow access" dialog bug on macOS 12 is fixed by the fact that we are only accessing it from the same user (tunnel proc) that created it now. --- .../FirezoneKit/Keychain/Keychain.swift | 51 ++++--- .../Keychain/KeychainStorage.swift | 3 +- .../FirezoneKit/Stores/TunnelStore.swift | 115 ++++++--------- .../PacketTunnelProvider.swift | 135 +++++++++++++----- 4 files changed, 173 insertions(+), 131 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift index b184db952..16a2293ef 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift @@ -20,8 +20,11 @@ public enum KeychainError: Error { public actor Keychain { private let label = "Firezone token" private let description = "Firezone access token used to authenticate the client." - private let account = "Firezone" - private let service = Bundle.main.bundleIdentifier! + private let service = "dev.firezone.firezone" + + // Bump this for backwards-incompatible Keychain changes; this is effectively the + // upsert key. + private let account = "1" private let workQueue = DispatchQueue(label: "FirezoneKeychainWorkQueue") @@ -47,7 +50,7 @@ public actor Keychain { public init() {} - func add(token: Token) async throws -> PersistentRef { + public func add(token: Token) async throws { return try await withCheckedThrowingContinuation { [weak self] continuation in self?.workQueue.async { [weak self] in guard let self = self else { @@ -56,16 +59,12 @@ public actor Keychain { } let query: [CFString: Any] = [ - // Common for both iOS and macOS: kSecClass: kSecClassGenericPassword, - kSecAttrLabel: label, - kSecAttrDescription: description, - kSecAttrAccount: account, - kSecAttrService: service, + kSecAttrLabel: self.label, + kSecAttrAccount: self.account, + kSecAttrDescription: self.description, + kSecAttrService: self.service, kSecValueData: token.data(using: .utf8) as Any, - kSecReturnPersistentRef: true, - kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock, - kSecAttrAccessGroup: AppInfoPlistConstants.appGroupId as CFString as Any, ] var ref: CFTypeRef? @@ -76,18 +75,13 @@ public actor Keychain { return } - guard let savedPersistentRef = ref as? Data else { - continuation.resume(throwing: KeychainError.nilResultFromAppleSecCall(call: "SecItemAdd")) - return - } - continuation.resume(returning: savedPersistentRef) + continuation.resume() return } } } - func update(token: Token) async throws { - + public func update(token: Token) async throws { return try await withCheckedThrowingContinuation { [weak self] continuation in self?.workQueue.async { [weak self] in guard let self = self else { @@ -97,7 +91,9 @@ public actor Keychain { let query: [CFString: Any] = [ kSecClass: kSecClassGenericPassword, + kSecAttrLabel: self.label, kSecAttrAccount: self.account, + kSecAttrDescription: self.description, kSecAttrService: self.service, ] let attributesToUpdate = [ @@ -121,6 +117,7 @@ public actor Keychain { self?.workQueue.async { let query = [ + kSecClass: kSecClassGenericPassword, kSecValuePersistentRef: persistentRef, kSecReturnData: true, ] as [CFString: Any] @@ -138,13 +135,14 @@ public actor Keychain { } } - func search() async -> PersistentRef? { + public func search() async -> PersistentRef? { return await withCheckedContinuation { [weak self] continuation in guard let self = self else { return } self.workQueue.async { let query = [ kSecClass: kSecClassGenericPassword, + kSecAttrLabel: self.label, kSecAttrAccount: self.account, kSecAttrDescription: self.description, kSecAttrService: self.service, @@ -161,6 +159,21 @@ public actor Keychain { } } + public func delete(persistentRef: PersistentRef) async throws { + return try await withCheckedThrowingContinuation { [weak self] continuation in + self?.workQueue.async { + let query = [kSecValuePersistentRef: persistentRef] as [CFString: Any] + let ret = SecStatus(SecItemDelete(query as CFDictionary)) + guard ret.isSuccess || ret == .status(.itemNotFound) else { + continuation.resume( + throwing: KeychainError.appleSecError(call: "SecItemDelete", status: ret)) + return + } + continuation.resume(returning: ()) + } + } + } + private func securityError(_ status: OSStatus) -> Error { KeychainError.securityError(KeychainStatus(rawValue: status)!) } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift index 1df518508..f27700fb3 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift @@ -8,7 +8,7 @@ import Dependencies import Foundation struct KeychainStorage: Sendable { - var add: @Sendable (Keychain.Token) async throws -> Keychain.PersistentRef + var add: @Sendable (Keychain.Token) async throws -> Void var update: @Sendable (Keychain.Token) async throws -> Void var search: @Sendable () async -> Keychain.PersistentRef? } @@ -31,7 +31,6 @@ extension KeychainStorage: DependencyKey { storage.withValue { let uuid = UUID().uuidString.data(using: .utf8)! $0[uuid] = (token) - return uuid } }, update: { token in diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift index 1adf00896..e14f4a20e 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift @@ -97,10 +97,8 @@ public final class TunnelStore: ObservableObject { } } - // Connect on app launch unless we're already connected - if let _ = manager?.protocolConfiguration?.passwordReference, - self.status == .disconnected - { + // Try to connect on app launch + if self.status == .disconnected { try await start() } @@ -123,36 +121,27 @@ public final class TunnelStore: ObservableObject { // Initialize and save a new VPN profile in system Preferences func createManager() async throws { - if let manager = manager { - // Someone deleted the manager while Fireone is running! - // Let's assume that was an accident and recreate it from - // our current state. - try await manager.saveToPreferences() - try await manager.loadFromPreferences() - } else { - let protocolConfiguration = NETunnelProviderProtocol() - let manager = NETunnelProviderManager() - let providerConfiguration = - protocolConfiguration.providerConfiguration - as? [String: String] - ?? Settings.defaultValue.toProviderConfiguration() + let protocolConfiguration = NETunnelProviderProtocol() + let manager = NETunnelProviderManager() + let providerConfiguration = + protocolConfiguration.providerConfiguration + as? [String: String] + ?? Settings.defaultValue.toProviderConfiguration() - protocolConfiguration.providerConfiguration = providerConfiguration - protocolConfiguration.providerBundleIdentifier = bundleIdentifier - protocolConfiguration.serverAddress = providerConfiguration[TunnelStoreKeys.apiURL] - manager.localizedDescription = bundleDescription - manager.protocolConfiguration = protocolConfiguration + protocolConfiguration.providerConfiguration = providerConfiguration + protocolConfiguration.providerBundleIdentifier = bundleIdentifier + protocolConfiguration.serverAddress = providerConfiguration[TunnelStoreKeys.apiURL] + manager.localizedDescription = bundleDescription + manager.protocolConfiguration = protocolConfiguration - // Save the new VPN profile to System Preferences - try await manager.saveToPreferences() - try await manager.loadFromPreferences() + // Save the new VPN profile to System Preferences + try await manager.saveToPreferences() - self.manager = manager - self.status = .disconnected - } + self.manager = manager + self.status = .disconnected } - func start() async throws { + func start(token: String? = nil) async throws { logger.log("\(#function)") guard let manager = manager @@ -169,17 +158,22 @@ public final class TunnelStore: ObservableObject { manager.isEnabled = true try await manager.saveToPreferences() - try await manager.loadFromPreferences() let session = castToSession(manager.connection) do { - try session.startTunnel() + var options: [String: NSObject]? = nil + if let token = token { + options = ["token": token as NSObject] + } + + try session.startTunnel(options: options) } catch { + logger.error("Error starting tunnel: \(error)") throw TunnelStoreError.startTunnelErrored(error) } } - func stop() async throws { + func stop(clearToken: Bool = false) async throws { logger.log("\(#function)") guard let manager = manager else { @@ -193,7 +187,13 @@ public final class TunnelStore: ObservableObject { return } let session = castToSession(manager.connection) - session.stopTunnel() + if clearToken { + try session.sendProviderMessage("signOut".data(using: .utf8)!) { _ in + session.stopTunnel() + } + } else { + session.stopTunnel() + } } public func cancelSignIn() { @@ -213,44 +213,26 @@ public final class TunnelStore: ObservableObject { let authURL = URL(string: settings.authBaseURL)! let authResponse = try await auth.signIn(authURL) - // Apple recommends updating Keychain items in place if possible - var tokenRef: Keychain.PersistentRef - if let ref = await keychain.search() { - try await keychain.update(authResponse.token) - tokenRef = ref - } else { - tokenRef = try await keychain.add(authResponse.token) - } - - // Save token and actorName + // Save actorName providerConfiguration[TunnelStoreKeys.actorName] = authResponse.actorName protocolConfiguration.providerConfiguration = providerConfiguration - protocolConfiguration.passwordReference = tokenRef manager.protocolConfiguration = protocolConfiguration - try await manager.saveToPreferences() - try await manager.loadFromPreferences() - // Start tunnel - try await start() + try await manager.saveToPreferences() + + // Bring the tunnel up and send it a token to start + do { + try await start(token: authResponse.token) + } catch { + logger.error("Error signing in: \(error)") + } } func signOut() async throws { logger.log("\(#function)") - guard let manager = manager, - ![.disconnecting, .disconnected].contains(status), - let protocolConfiguration = manager.protocolConfiguration as? NETunnelProviderProtocol - else { - logger.error("\(#function): Tunnel seems to be already disconnected") - return - } - - // Clear token from VPN profile, but keep it in Keychain because the user - // may have customized the Keychain Item. - protocolConfiguration.passwordReference = nil - try await manager.saveToPreferences() // Stop tunnel - try await stop() + try await stop(clearToken: true) } func beginUpdatingResources() { @@ -418,17 +400,6 @@ public final class TunnelStore: ObservableObject { SessionNotificationHelper.showSignedOutAlertmacOS(logger: self.logger, tunnelStore: self) } #endif - - // Clear tokenRef - guard let manager = manager else { return } - let protocolConfiguration = manager.protocolConfiguration - protocolConfiguration?.passwordReference = nil - manager.protocolConfiguration = protocolConfiguration - do { - try await manager.saveToPreferences() - } catch { - logger.error("\(#function): Couldn't clear tokenRef") - } } } diff --git a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift index 4e34782da..26282697f 100644 --- a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift +++ b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift @@ -20,44 +20,74 @@ class PacketTunnelProvider: NEPacketTunnelProvider { private var adapter: Adapter? override func startTunnel( - options _: [String: NSObject]? = nil, + options: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void ) { + super.startTunnel(options: options, completionHandler: completionHandler) logger.log("\(#function)") - guard let apiURL = protocolConfiguration.serverAddress, - let tokenRef = protocolConfiguration.passwordReference, - let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)? - .providerConfiguration as? [String: String], - let logFilter = providerConfiguration[TunnelStoreKeys.logFilter] - else { - completionHandler( - PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress")) - return - } - Task { - let keychain = Keychain() - guard let token = await keychain.load(persistentRef: tokenRef) else { - logger.error("\(#function): No token found in Keychain") - completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain) - return - } - - let adapter = Adapter( - apiURL: apiURL, - token: token, - logFilter: logFilter, - packetTunnelProvider: self) - self.adapter = adapter do { - try adapter.start { error in - if let error { - self.logger.error("\(#function): \(error)") + var token = options?["token"] as? String + let keychain = Keychain() + var tokenRef = await keychain.search() + + if let token = token { + // 1. If we're passed a token, save it to keychain + + // Apple recommends updating Keychain items in place if possible + // In reality this won't happen unless there's some kind of race condition + // because we would have deleted the item upon sign out. + if let ref = tokenRef { + try await keychain.update(token: token) + } else { + try await keychain.add(token: token) } - completionHandler(error) + + } else { + + // 2. Otherwise, load it from the keychain + guard let tokenRef = tokenRef + else { + completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain) + return + } + + token = await keychain.load(persistentRef: tokenRef) } + + // 3. Now we should have a token, so connect + guard let apiURL = protocolConfiguration.serverAddress + else { + completionHandler( + PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress")) + return + } + + guard + let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)? + .providerConfiguration as? [String: String], + let logFilter = providerConfiguration[TunnelStoreKeys.logFilter] + else { + completionHandler( + PacketTunnelProviderError.savedProtocolConfigurationIsInvalid( + "providerConfiguration.logFilter")) + return + } + + guard let token = token + else { + completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain) + return + } + + let adapter = Adapter( + apiURL: apiURL, token: token, logFilter: logFilter, packetTunnelProvider: self) + self.adapter = adapter + + try adapter.start(completionHandler: completionHandler) } catch { + logger.error("\(#function): Error! \(error)") completionHandler(error) } } @@ -73,12 +103,17 @@ class PacketTunnelProvider: NEPacketTunnelProvider { if case .authenticationCanceled = reason { do { - // Remove the passwordReference from our configuration so that it's not used again - // if the app is re-launched. There's no good way to send data like this from the - // Network Extension to the GUI, so save it to a file for the GUI to read later. - try String(reason.rawValue).write(to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8) + // This was triggered from onDisconnect, so clear our token + Task { try await clearToken() } + + // There's no good way to send data like this from the + // Network Extension to the GUI, so save it to a file for the GUI to read upon + // either status change or the next launch. + try String(reason.rawValue).write( + to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8) } catch { - logger.error("\(#function): Couldn't write provider stop reason to file. Notification won't work.") + logger.error( + "\(#function): Couldn't write provider stop reason to file. Notification won't work.") } #if os(iOS) // iOS notifications should be shown from the tunnel process @@ -94,12 +129,36 @@ class PacketTunnelProvider: NEPacketTunnelProvider { // TODO: Use a message format to allow requesting different types of data. // This currently assumes we're requesting resources. - override func handleAppMessage(_ hash: Data, completionHandler: ((Data?) -> Void)? = nil) { - adapter?.getResourcesIfVersionDifferentFrom(hash: hash) { - resourceListJSON in - completionHandler?(resourceListJSON?.data(using: .utf8)) + override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) { + let string = String(data: message, encoding: .utf8) + + switch string { + case "signOut": + Task { + do { + try await clearToken() + } catch { + logger.error("\(#function): Error: \(error)") + } + } + default: + adapter?.getResourcesIfVersionDifferentFrom(hash: message) { + resourceListJSON in + completionHandler?(resourceListJSON?.data(using: .utf8)) + } } } + + private func clearToken() async throws { + let keychain = Keychain() + guard let ref = await keychain.search() + else { + logger.error("\(#function): Error: token not found!") + return + } + + try await keychain.delete(persistentRef: ref) + } } extension NEProviderStopReason: CustomStringConvertible {