From 64876fffa338d5d5d772bef17c9efd0111c5fe3d Mon Sep 17 00:00:00 2001 From: Jamil Date: Tue, 14 Jan 2025 06:14:51 -0800 Subject: [PATCH] fix(apple): Don't rely on Keychain for critical functions (#7752) The Keychain on Apple platforms, while secure, is not always available. It can be unavailable if the user has changed its permissions accidentally, the keychain database is corrupt, there is an issue with the secure enclave, or any number of other system-related or Apple account-related reasons. There are only two things we use the Keychain for: - Storing the `firezone-id`. This is actually not a secret. - Persisting the `token` upon sign in so that: - the iOS system can keep the tunnel alive without the GUI running - the macOS app can relaunch after `Disconnect & Quit` without having to sign in again For the first case, we move back to persisting this to a file (see #7464). For the second case, we simply don't care too much if the Keychain can't be saved to. We simply move on with activating the tunnel and logging the error so we know how often these edge cases occur. --- .../Firezone/Application/FirezoneApp.swift | 17 -- .../Managers/VPNProfileManager.swift | 11 +- .../FirezoneKit/Models/FirezoneId.swift | 148 +++++----------- .../FirezoneNetworkExtension/Adapter.swift | 5 +- .../PacketTunnelProvider.swift | 163 ++++++++++-------- 5 files changed, 147 insertions(+), 197 deletions(-) diff --git a/swift/apple/Firezone/Application/FirezoneApp.swift b/swift/apple/Firezone/Application/FirezoneApp.swift index f53d5bcf2..397e9af77 100644 --- a/swift/apple/Firezone/Application/FirezoneApp.swift +++ b/swift/apple/Firezone/Application/FirezoneApp.swift @@ -74,23 +74,6 @@ struct FirezoneApp: App { public var store: Store? func applicationDidFinishLaunching(_: Notification) { - - Task { - // In 1.4.0 and higher, the macOS client uses a system extension as its - // Network Extension packaging type. It runs as root and can't read the - // existing firezone-id file. So read it here from the app process instead - // and save it to the Keychain, where we should store shared persistent - // data going forward. - // - // Can be removed once all clients >= 1.4.0 - try FirezoneId.migrate() - - let id = try FirezoneId.createIfMissing() - - // Hydrate telemetry userId with our firezone id - Telemetry.firezoneId = id.uuid.uuidString - } - if let store = store { menuBar = MenuBar(model: SessionViewModel(favorites: favorites, store: store)) } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNProfileManager.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNProfileManager.swift index c75694447..e7c32c366 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNProfileManager.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNProfileManager.swift @@ -269,10 +269,17 @@ public class VPNProfileManager { } func start(token: String? = nil) { - var options: [String: NSObject]? + var options: [String: NSObject] = [:] + // Pass token if provided if let token = token { - options = ["token": token as NSObject] + options.merge(["token": token as NSObject]) { _, n in n } + } + + // Pass pre-1.4.0 Firezone ID if it exists. Pre 1.4.0 clients will have this + // persisted to the app side container URL. + if let id = FirezoneId.load(.Pre_1_4_0) { + options.merge(["id": id as NSObject]) { _, n in n } } do { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneId.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneId.swift index f512f65c2..ee9b7bf11 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneId.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneId.swift @@ -3,130 +3,62 @@ // (c) 2024 Firezone, Inc. // LICENSE: Apache-2.0 // -// Convenience wrapper for working with our firezone-id stored in the Keychain. +// Convenience wrapper for working with our firezone-id file stored by the +// tunnel process. import Foundation +/// Prior to 1.4.0, our firezone-id was saved in a file accessible to both the +/// app and tunnel process. Starting with 1.4.0, +/// the macOS client uses a system extension, which makes sharing folders with +/// the app cumbersome, so we move to persisting the firezone-id only from the +/// tunnel process since that is the only place it's used. +/// +/// Can be refactored to remove the Version enum all clients >= 1.4.0 public struct FirezoneId { - private static let query: [CFString: Any] = [ - kSecAttrLabel: "Firezone id", - kSecAttrAccount: "2", - kSecAttrService: BundleHelper.appGroupId, - kSecAttrDescription: "Firezone device id", - ] - - public var uuid: UUID - - public init(_ uuid: UUID? = nil) { - self.uuid = uuid ?? UUID() + public enum Version { + case Pre_1_4_0 + case Post_1_4_0 } - // Upsert the firezone-id to the Keychain - public func save() throws { - guard Keychain.search(query: FirezoneId.query) == nil + public static func save(_ id: String) { + guard let fileURL = FileManager.default.containerURL( + forSecurityApplicationGroupIdentifier: BundleHelper.appGroupId)? + .appendingPathComponent("firezone-id") else { - let query = FirezoneId.query.merging([ - kSecClass: kSecClassGenericPassword - ]) { (_, new) in new } - return try Keychain.update( - query: query, - attributesToUpdate: [kSecValueData: uuid.toData()] - ) + // Nothing we can do about disk errors + return } - let query = FirezoneId.query.merging([ - kSecClass: kSecClassGenericPassword, - kSecValueData: uuid.toData() - ]) { (_, new) in new } - - try Keychain.add(query: query) + try? id.write( + to: fileURL, + atomically: true, + encoding: .utf8 + ) } - // Attempt to load the firezone-id from the Keychain - public static func load() throws -> FirezoneId? { - guard let idRef = Keychain.search(query: query) - else { return nil } - - guard let data = Keychain.load(persistentRef: idRef) - else { return nil } - - guard data.count == UUID.sizeInBytes - else { - fatalError("Firezone ID loaded from keychain must be exactly \(UUID.sizeInBytes) bytes") - } - - let uuid = UUID(fromData: data) - return FirezoneId(uuid) - } - - // Prior to 1.4.0, our firezone-id was saved in a file. Starting with 1.4.0, - // the macOS client uses a system extension, which makes sharing folders with - // the app cumbersome, so we moved to using the keychain for this due to its - // better ergonomics. If the old firezone-id doesn't exist, this function - // is a no-op. - // - // Can be refactored to remove the file check once all clients >= 1.4.0 - public static func migrate() throws { - guard try load() == nil - else { return } // New firezone-id already saved in Keychain - + public static func load(_ version: Version) -> String? { + let appGroupId = switch version { + case .Post_1_4_0: + BundleHelper.appGroupId + case .Pre_1_4_0: #if os(macOS) - let appGroupIdPre_1_4_0 = "47R2M6779T.group.dev.firezone.firezone" + "47R2M6779T.group.dev.firezone.firezone" #elseif os(iOS) - let appGroupIdPre_1_4_0 = "group.dev.firezone.firezone" + "group.dev.firezone.firezone" #endif - - guard let containerURL = FileManager.default.containerURL( - forSecurityApplicationGroupIdentifier: appGroupIdPre_1_4_0) - else { fatalError("Couldn't find app group container") } - - let idFileURL = containerURL.appendingPathComponent("firezone-id") - - // If the file isn't there or can't be read, bail - guard FileManager.default.fileExists(atPath: idFileURL.path), - let uuidString = try? String(contentsOf: idFileURL) - else { return } - - let firezoneId = FirezoneId(UUID(uuidString: uuidString)) - try firezoneId.save() - } - - public static func createIfMissing() throws -> FirezoneId { - guard let id = try load() - else { - let id = FirezoneId(UUID()) - try id.save() - - return id } - // New firezone-id already saved in Keychain + guard let containerURL = + FileManager.default.containerURL( + forSecurityApplicationGroupIdentifier: appGroupId), + let id = + try? String( + contentsOf: containerURL.appendingPathComponent("firezone-id")) + else { + return nil + } + return id } } - -// Convenience extension to convert to/from Data for storing in Keychain -extension UUID { - // We need the size of a UUID to (1) know how big to make the Data buffer, - // and (2) to make sure the UUID we read from the keychain is a valid length. - public static let sizeInBytes = MemoryLayout.size(ofValue: UUID()) - - init(fromData: Data) { - self = fromData.withUnsafeBytes { rawBufferPointer in - guard let baseAddress = rawBufferPointer.baseAddress - else { - fatalError("Buffer should point to a valid memory address") - } - - return UUID(uuid: baseAddress.assumingMemoryBound(to: uuid_t.self).pointee) - } - } - - func toData() -> Data { - let data = withUnsafePointer(to: self) { rawBufferPinter in - Data(bytes: rawBufferPinter, count: UUID.sizeInBytes) - } - - return data - } -} diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index 4d0baa3d7..e6575ca2c 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -88,18 +88,21 @@ class Adapter { /// Starting parameters private var apiURL: String private var token: Token + private let id: String private let logFilter: String private let connlibLogFolderPath: String init( apiURL: String, token: Token, + id: String, logFilter: String, internetResourceEnabled: Bool, packetTunnelProvider: PacketTunnelProvider ) { self.apiURL = apiURL self.token = token + self.id = id self.packetTunnelProvider = packetTunnelProvider self.callbackHandler = CallbackHandler() self.state = .tunnelStopped @@ -142,7 +145,7 @@ class Adapter { try WrappedSession.connect( apiURL, "\(token)", - "\(Telemetry.firezoneId!)", + "\(id)", "\(Telemetry.accountSlug!)", DeviceMetadata.getDeviceName(), DeviceMetadata.getOSVersion(), diff --git a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift index 6f9989e6a..662f23040 100644 --- a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift +++ b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift @@ -39,90 +39,87 @@ class PacketTunnelProvider: NEPacketTunnelProvider { Log.log("\(#function)") Task { - do { - // Can be removed after all clients >= 1.4.0 - try FirezoneId.migrate() + // If we don't have a token, we can't continue. + guard let token = loadAndSaveToken(from: options) + else { + completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain) - // The tunnel can come up without the app having been launched first, so - // initialize the id here too. - let id = try FirezoneId.createIfMissing() + return + } - // Hydrate the telemetry userId with our firezone id - Telemetry.firezoneId = id.uuid.uuidString + // Try to save the token back to the Keychain but continue if we can't + do { try token.save() } catch { Log.error(error) } - let passedToken = options?["token"] as? String - let keychainToken = try Token.load() + // Use and persist the provided ID or try loading it from disk, + // generating a new one if both of those are nil. + let id = loadAndSaveFirezoneId(from: options) - // Use the provided token or try loading one from the Keychain - guard let token = Token(passedToken) ?? keychainToken - else { - completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain) + // Now we should have a token, so continue connecting + guard let apiURL = protocolConfiguration.serverAddress + else { + completionHandler( + PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress")) + return + } - return - } + // Reconfigure our Telemetry environment now that we know the API URL + Telemetry.setEnvironmentOrClose(apiURL) - // Save the token back to the Keychain - try token.save() + guard + let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)? + .providerConfiguration as? [String: String], + let logFilter = providerConfiguration[VPNProfileManagerKeys.logFilter] + else { + completionHandler( + PacketTunnelProviderError.savedProtocolConfigurationIsInvalid( + "providerConfiguration.logFilter")) + return + } - // Now we should have a token, so continue connecting - guard let apiURL = protocolConfiguration.serverAddress - else { - completionHandler( - PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress")) - return - } - - // Reconfigure our Telemetry environment now that we know the API URL - Telemetry.setEnvironmentOrClose(apiURL) - - guard - let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)? - .providerConfiguration as? [String: String], - let logFilter = providerConfiguration[VPNProfileManagerKeys.logFilter] - else { - completionHandler( - PacketTunnelProviderError.savedProtocolConfigurationIsInvalid( - "providerConfiguration.logFilter")) - return - } - - // Hydrate telemetry account slug - guard let accountSlug = providerConfiguration[VPNProfileManagerKeys.accountSlug] - else { - // This can happen if the user deletes the VPN profile while it's - // connected. The system will try to restart us with a fresh config - // once the user fixes the problem, but we'd rather not connect - // without a slug. - completionHandler( - PacketTunnelProviderError.savedProtocolConfigurationIsInvalid( - "providerConfiguration.accountSlug" - ) + // Hydrate telemetry account slug + guard let accountSlug = providerConfiguration[VPNProfileManagerKeys.accountSlug] + else { + // This can happen if the user deletes the VPN profile while it's + // connected. The system will try to restart us with a fresh config + // once the user fixes the problem, but we'd rather not connect + // without a slug. + completionHandler( + PacketTunnelProviderError.savedProtocolConfigurationIsInvalid( + "providerConfiguration.accountSlug" ) - return - } + ) + return + } - Telemetry.accountSlug = accountSlug + Telemetry.accountSlug = accountSlug - let internetResourceEnabled: Bool = if let internetResourceEnabledJSON = providerConfiguration[VPNProfileManagerKeys.internetResourceEnabled]?.data(using: .utf8) { - (try? JSONDecoder().decode(Bool.self, from: internetResourceEnabledJSON )) ?? false - } else { - false - } + let internetResourceEnabled: Bool = if let internetResourceEnabledJSON = providerConfiguration[VPNProfileManagerKeys.internetResourceEnabled]?.data(using: .utf8) { + (try? JSONDecoder().decode(Bool.self, from: internetResourceEnabledJSON )) ?? false + } else { + false + } - let adapter = Adapter( - apiURL: apiURL, token: token, logFilter: logFilter, internetResourceEnabled: internetResourceEnabled, packetTunnelProvider: self) - self.adapter = adapter + let adapter = Adapter( + apiURL: apiURL, + token: token, + id: id, + logFilter: logFilter, + internetResourceEnabled: internetResourceEnabled, + packetTunnelProvider: self + ) + self.adapter = adapter - - try await adapter.start() - - // Tell the system the tunnel is up, moving the tunnel manager status to - // `connected`. - completionHandler(nil) - } catch { + do { try await adapter.start() } + catch { Log.error(error) completionHandler(error) + + return } + + // Tell the system the tunnel is up, moving the tunnel manager status to + // `connected`. + completionHandler(nil) } } @@ -199,6 +196,34 @@ class PacketTunnelProvider: NEPacketTunnelProvider { } } + func loadAndSaveToken(from options: [String: NSObject]?) -> Token? { + let passedToken = options?["token"] as? String + + // Try to load saved token from Keychain, continuing if Keychain is + // unavailable. + let keychainToken = { + do { return try Token.load() } catch { Log.error(error) } + + return nil + }() + + return Token(passedToken) ?? keychainToken + } + + func loadAndSaveFirezoneId(from options: [String: NSObject]?) -> String { + let passedId = options?["id"] as? String + let persistedId = FirezoneId.load(.Post_1_4_0) + + let id = passedId ?? persistedId ?? UUID().uuidString + + FirezoneId.save(id) + + // Hydrate the telemetry userId with our firezone id + Telemetry.firezoneId = id + + return id + } + func clearLogs(_ completionHandler: ((Data?) -> Void)? = nil) { do { try Log.clear(in: SharedAccess.logFolderURL)