From e599eb2f09f9a67e102ba4239e8e36e9da880a7a Mon Sep 17 00:00:00 2001 From: Jamil Date: Fri, 16 May 2025 00:17:12 -0700 Subject: [PATCH] fix(apple): Ensure system extension loads after upgrade (#9166) On macOS, after upgrading the client, the new system extension fails to respond to IPC commands until it receives a `startTunnel` call. After that, subsequent IPC calls will succeed across relaunches of the app. To fix this, we introduce a dummy `startTunnel` call on macOS that attempts to bring life into the System extension whenever we receive `nil` configuration. We also tidy up a few other things to make this easier to follow. Fixes #9156 Fixes #8476 --------- Signed-off-by: Jamil Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../FirezoneKit/Helpers/IPCClient.swift | 10 + .../Managers/SystemExtensionManager.swift | 52 +++-- .../Sources/FirezoneKit/Stores/Store.swift | 210 +++++++++--------- .../Sources/FirezoneKit/Views/AppView.swift | 17 +- .../FirezoneKit/Views/GrantVPNView.swift | 2 +- .../Sources/FirezoneKit/Views/MenuBar.swift | 14 +- .../FirezoneKit/Views/SessionView.swift | 2 +- .../FirezoneKit/Views/SettingsView.swift | 6 +- .../FirezoneKit/Views/iOSNavigationView.swift | 4 +- .../PacketTunnelProvider.swift | 7 +- 10 files changed, 175 insertions(+), 149 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift index fd7e14727..13880d757 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift @@ -79,6 +79,16 @@ class IPCClient { try session([.connected, .connecting, .reasserting]).stopTunnel() } +#if os(macOS) + // On macOS, IPC calls to the system extension won't work after it's been upgraded, until the startTunnel call. + // Since we rely on IPC for the GUI to function, we need to send a dummy `startTunnel` that doesn't actually + // start the tunnel, but causes the system to start the extension. + func startSystemExtension() throws { + let options: [String: NSObject] = ["dryRun": true as NSObject] + try session().startTunnel(options: options) + } +#endif + func getConfiguration() async throws -> Configuration? { return try await withCheckedThrowingContinuation { continuation in do { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/SystemExtensionManager.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/SystemExtensionManager.swift index 13cee461b..15fc9c25e 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/SystemExtensionManager.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/SystemExtensionManager.swift @@ -7,7 +7,7 @@ #if os(macOS) import SystemExtensions -public enum SystemExtensionError: Error { +enum SystemExtensionError: Error { case unknownResult(OSSystemExtensionRequest.Result) var description: String { @@ -18,7 +18,7 @@ public enum SystemExtensionError: Error { } } -public enum SystemExtensionStatus { +enum SystemExtensionStatus { // Not installed or enabled at all case needsInstall @@ -30,43 +30,41 @@ public enum SystemExtensionStatus { case installed } -public class SystemExtensionManager: NSObject, OSSystemExtensionRequestDelegate, ObservableObject { +enum SystemExtensionRequestType { + case install + case check +} + +class SystemExtensionManager: NSObject, OSSystemExtensionRequestDelegate, ObservableObject { // Delegate methods complete with either a true or false outcome or an Error private var continuation: CheckedContinuation? - public func installSystemExtension( + func sendRequest( + requestType: SystemExtensionRequestType, identifier: String, continuation: CheckedContinuation ) { self.continuation = continuation - let request = OSSystemExtensionRequest.activationRequest(forExtensionWithIdentifier: identifier, queue: .main) + let request = switch requestType { + case .install: + OSSystemExtensionRequest.activationRequest(forExtensionWithIdentifier: identifier, queue: .main) + case .check: + OSSystemExtensionRequest.propertiesRequest( + forExtensionWithIdentifier: identifier, + queue: .main + ) + } + request.delegate = self - // Install extension - OSSystemExtensionManager.shared.submitRequest(request) - } - - public func checkStatus( - identifier: String, - continuation: CheckedContinuation - ) { - self.continuation = continuation - - let request = OSSystemExtensionRequest.propertiesRequest( - forExtensionWithIdentifier: identifier, - queue: .main - ) - request.delegate = self - - // Send request OSSystemExtensionManager.shared.submitRequest(request) } // MARK: - OSSystemExtensionRequestDelegate // Result of system extension installation - public func request( + func request( _ request: OSSystemExtensionRequest, didFinishWithResult result: OSSystemExtensionRequest.Result ) { @@ -81,7 +79,7 @@ public class SystemExtensionManager: NSObject, OSSystemExtensionRequestDelegate, } // Result of properties request - public func request( + func request( _ request: OSSystemExtensionRequest, foundProperties properties: [OSSystemExtensionProperties] ) { @@ -116,15 +114,15 @@ public class SystemExtensionManager: NSObject, OSSystemExtensionRequestDelegate, resume(returning: .needsInstall) } - public func request(_ request: OSSystemExtensionRequest, didFailWithError error: Error) { + func request(_ request: OSSystemExtensionRequest, didFailWithError error: Error) { resume(throwing: error) } - public func requestNeedsUserApproval(_ request: OSSystemExtensionRequest) { + func requestNeedsUserApproval(_ request: OSSystemExtensionRequest) { // We assume this state until we receive a success response. } - public func request( + func request( _ request: OSSystemExtensionRequest, actionForReplacingExtension existing: OSSystemExtensionProperties, withExtension ext: OSSystemExtensionProperties diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift index 07e49a0fe..df6e32628 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift @@ -15,6 +15,7 @@ import AppKit @MainActor // TODO: Move some state logic to view models +// swiftlint:disable:next type_body_length public final class Store: ObservableObject { @Published private(set) var actorName: String @Published private(set) var favorites = Favorites() @@ -27,7 +28,7 @@ public final class Store: ObservableObject { @Published private(set) var configuration: Configuration? // Enacapsulate Tunnel status here to make it easier for other components to observe - @Published private(set) var status: NEVPNStatus? + @Published private(set) var vpnStatus: NEVPNStatus? // User notifications @Published private(set) var decision: UNAuthorizationStatus? @@ -59,74 +60,48 @@ public final class Store: ObservableObject { } // Load our state from the system. Based on what's loaded, we may need to ask the user for permission for things. - initNotifications() - initSystemExtension() - initVPNConfiguration() - } - - func initNotifications() { + // When everything loads correctly, we attempt to start the tunnel if connectOnStart is enabled. Task { - self.decision = await self.sessionNotification.loadAuthorizationStatus() + do { + await initNotifications() + try await initSystemExtension() + try await initVPNConfiguration() + try await setupTunnelObservers() + try await initConfiguration() + try await maybeAutoConnect() + } catch { + Log.error(error) + } } } - func initSystemExtension() { #if os(macOS) - Task { - do { - self.systemExtensionStatus = try await self.checkSystemExtensionStatus() - } catch { - Log.error(error) - } + func systemExtensionRequest(_ requestType: SystemExtensionRequestType) async throws { + let manager = SystemExtensionManager() + + self.systemExtensionStatus = + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in + manager.sendRequest( + requestType: requestType, + identifier: VPNConfigurationManager.bundleIdentifier, + continuation: continuation + ) } + } #endif + + private func setupTunnelObservers() async throws { + let vpnStatusChangeHandler: (NEVPNStatus) async throws -> Void = { [weak self] status in + try await self?.handleVPNStatusChange(newVPNStatus: status) + } + try ipcClient().subscribeToVPNStatusUpdates(handler: vpnStatusChangeHandler) + self.vpnStatus = try ipcClient().sessionStatus() } - func initVPNConfiguration() { - Task { - do { - // Try to load existing configuration - if let manager = try await VPNConfigurationManager.load() { - try await manager.maybeMigrateConfiguration() - self.vpnConfigurationManager = manager - try await setupTunnelObservers() - self.configuration = try await ipcClient().getConfiguration() - Telemetry.firezoneId = configuration?.firezoneId + private func handleVPNStatusChange(newVPNStatus: NEVPNStatus) async throws { + self.vpnStatus = newVPNStatus - try await manager.enableConfiguration() - if configuration?.connectOnStart ?? true { - try ipcClient().start() - } - } else { - status = .invalid - } - } catch { - Log.error(error) - } - } - } - - func setupTunnelObservers() async throws { - let statusChangeHandler: (NEVPNStatus) async throws -> Void = { [weak self] status in - try await self?.handleStatusChange(newStatus: status) - } - - try ipcClient().subscribeToVPNStatusUpdates(handler: statusChangeHandler) - try await handleStatusChange(newStatus: ipcClient().sessionStatus()) - } - - func handleStatusChange(newStatus: NEVPNStatus) async throws { - status = newStatus - - if status == .invalid { - // VPN configuration was yanked from system settings - endConfigurationPolling() - } else { - // This is a no-op if the timer is already active - beginConfigurationPolling() - } - - if status == .connected { + if newVPNStatus == .connected { beginUpdatingResources() } else { endUpdatingResources() @@ -135,7 +110,7 @@ public final class Store: ObservableObject { #if os(macOS) // On macOS we must show notifications from the UI process. On iOS, we've already initiated the notification // from the tunnel process, because the UI process is not guaranteed to be alive. - if status == .disconnected { + if vpnStatus == .disconnected { do { let reason = try await ipcClient().consumeStopReason() if reason == .authenticationCanceled { @@ -148,54 +123,65 @@ public final class Store: ObservableObject { // When this happens, it's because either our VPN configuration or System Extension (or both) were removed. // So load the system extension status again to determine which view to load. - if status == .invalid { - self.systemExtensionStatus = try await checkSystemExtensionStatus() + if vpnStatus == .invalid { + try await systemExtensionRequest(.check) } #endif } + private func initNotifications() async { + self.decision = await self.sessionNotification.loadAuthorizationStatus() + } + + private func initSystemExtension() async throws { #if os(macOS) - func checkSystemExtensionStatus() async throws -> SystemExtensionStatus { - let checker = SystemExtensionManager() + try await systemExtensionRequest(.check) - let status = - try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in - checker.checkStatus( - identifier: VPNConfigurationManager.bundleIdentifier, - continuation: continuation - ) + // If already installed but the wrong version, go ahead and install. This shouldn't prompt the user. + if systemExtensionStatus == .needsReplacement { + try await systemExtensionRequest(.install) } - - // If already installed but the wrong version, go ahead and install. - // This shouldn't prompt the user. - if status == .needsReplacement { - try await installSystemExtension() - } - - return status - } - - func installSystemExtension() async throws { - let installer = SystemExtensionManager() - - // Apple recommends installing the system extension as early as possible after app launch. - // See https://developer.apple.com/documentation/systemextensions/installing-system-extensions-and-drivers - self.systemExtensionStatus = - try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in - installer.installSystemExtension( - identifier: VPNConfigurationManager.bundleIdentifier, - continuation: continuation - ) - } - } #endif + } + private func initVPNConfiguration() async throws { + // Try to load existing configuration + if let manager = try await VPNConfigurationManager.load() { + try await manager.maybeMigrateConfiguration() + self.vpnConfigurationManager = manager + } else { + self.vpnStatus = .invalid + } + } + + // On macOS, after upgrading Firezone, we need to issue a startTunnel to start the IPC service so that we + // can fetch configuration. We try a few times here to do that so that we can determine connectOnStart, before + // giving up and polling configuration anyway. + private func initConfiguration() async throws { + var configuration: Configuration? + let end = Date().addingTimeInterval(3) + + while configuration == nil && Date() < end { + configuration = try await getConfigurationStartingSystemExtension() + try await Task.sleep(nanoseconds: 100_000_000) + } + + self.configuration = configuration + + beginConfigurationPolling() + } + + private func maybeAutoConnect() async throws { + if configuration?.connectOnStart == true { + try await manager().enableConfiguration() + try ipcClient().start() + } + } func installVPNConfiguration() async throws { // Create a new VPN configuration in system settings. self.vpnConfigurationManager = try await VPNConfigurationManager() self.configuration = try await ipcClient().getConfiguration() - Telemetry.firezoneId = configuration?.firezoneId try await setupTunnelObservers() } @@ -281,7 +267,14 @@ public final class Store: ObservableObject { self.configurationUpdateTask = Task { if !Task.isCancelled { do { - self.configuration = try await self.ipcClient().getConfiguration() + self.configuration = try await self.getConfigurationStartingSystemExtension() + } catch let error as NSError { + // https://developer.apple.com/documentation/networkextension/nevpnerror-swift.struct/code + if error.domain == "NEVPNErrorDomain" && error.code == 1 { + // not initialized yet + } else { + Log.error(error) + } } catch { Log.error(error) } @@ -298,11 +291,21 @@ public final class Store: ObservableObject { self.configurationTimer = timer } - private func endConfigurationPolling() { - configurationUpdateTask?.cancel() - configurationTimer?.invalidate() - configurationTimer = nil - self.configuration = nil + private func getConfigurationStartingSystemExtension() async throws -> Configuration? { + var configuration = try await ipcClient().getConfiguration() + +#if os(macOS) + if configuration == nil { + try ipcClient().startSystemExtension() + configuration = try await ipcClient().getConfiguration() + } +#endif + + if Telemetry.firezoneId == nil { + Telemetry.firezoneId = configuration?.firezoneId + } + + return configuration } // Network Extensions don't have a 2-way binding up to the GUI process, @@ -323,6 +326,13 @@ public final class Store: ObservableObject { if !Task.isCancelled { do { self.resourceList = try await self.ipcClient().fetchResources() + } catch let error as NSError { + // https://developer.apple.com/documentation/networkextension/nevpnerror-swift.struct/code + if error.domain == "NEVPNErrorDomain" && error.code == 1 { + // not initialized yet + } else { + Log.error(error) + } } catch { Log.error(error) } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/AppView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/AppView.swift index ce61a3b11..f9bc964b3 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/AppView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/AppView.swift @@ -27,18 +27,18 @@ public struct AppView: View { // handle. private static var cancellables: Set = [] public static func subscribeToGlobalEvents(store: Store) { - store.$status + store.$vpnStatus .combineLatest(store.$systemExtensionStatus) .receive(on: DispatchQueue.main) .debounce(for: .seconds(0.3), scheduler: DispatchQueue.main) // Prevents flurry of windows from opening - .sink(receiveValue: { status, systemExtensionStatus in + .sink(receiveValue: { vpnStatus, systemExtensionStatus in // Open window in case permissions are revoked - if status == .invalid || systemExtensionStatus != .installed { + if vpnStatus == .invalid || systemExtensionStatus != .installed { WindowDefinition.main.openWindow() } // Close window for day to day use - if status != .invalid && systemExtensionStatus == .installed && launchedBefore() { + if vpnStatus != .invalid && systemExtensionStatus == .installed && launchedBefore() { WindowDefinition.main.window()?.close() } }) @@ -89,7 +89,7 @@ public struct AppView: View { @ViewBuilder public var body: some View { #if os(iOS) - switch (store.status, store.decision) { + switch (store.vpnStatus, store.decision) { case (nil, _), (_, nil): ProgressView() case (.invalid, _): @@ -106,9 +106,12 @@ public struct AppView: View { } } #elseif os(macOS) - switch (store.systemExtensionStatus, store.status) { + switch (store.systemExtensionStatus, store.vpnStatus) { case (nil, nil): - ProgressView() + VStack { + ProgressView() + Text("Getting things ready... this should only take a few seconds.") + } case (.needsInstall, _), (_, .invalid): GrantVPNView() default: diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/GrantVPNView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/GrantVPNView.swift index 202882eb1..0d91f6d91 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/GrantVPNView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/GrantVPNView.swift @@ -136,7 +136,7 @@ struct GrantVPNView: View { func installSystemExtension() { Task { do { - try await store.installSystemExtension() + try await store.systemExtensionRequest(.install) // The window has a tendency to go to the background after installing // the system extension diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index 75f18e84c..ddead0adf 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -203,7 +203,7 @@ public final class MenuBar: NSObject, ObservableObject { self.handleResourceListChanged() }).store(in: &cancellables) - store.$status + store.$vpnStatus .receive(on: DispatchQueue.main) .sink(receiveValue: { [weak self] _ in guard let self = self else { return } @@ -242,7 +242,7 @@ public final class MenuBar: NSObject, ObservableObject { updateStatusItemIcon() updateSignInMenuItems() quitMenuItem.title = { - switch store.status { + switch store.vpnStatus { case .connected, .connecting: return "Disconnect and Quit" default: @@ -324,13 +324,13 @@ public final class MenuBar: NSObject, ObservableObject { } func updateStatusItemIcon() { - updateAnimation(status: store.status) - statusItem.button?.image = getStatusIcon(status: store.status, notification: updateChecker.updateAvailable) + updateAnimation(status: store.vpnStatus) + statusItem.button?.image = getStatusIcon(status: store.vpnStatus, notification: updateChecker.updateAvailable) } func updateSignInMenuItems() { // Update "Sign In" / "Sign Out" menu items - switch store.status { + switch store.vpnStatus { case nil: signInMenuItem.title = "Loading VPN configurations from system settings…" signInMenuItem.action = nil @@ -374,7 +374,7 @@ public final class MenuBar: NSObject, ObservableObject { func updateResourcesMenuItems() { // Update resources "header" menu items - switch store.status { + switch store.vpnStatus { case .connecting: resourcesTitleMenuItem.isHidden = true resourcesUnavailableMenuItem.isHidden = false @@ -727,7 +727,7 @@ public final class MenuBar: NSObject, ObservableObject { // our VPN configuration got removed. Since we don't know which, reinstall // the system extension here too just in case. It's a no-op if already // installed. - try await store.installSystemExtension() + try await store.systemExtensionRequest(.install) try await store.installVPNConfiguration() } catch let error as NSError { if error.domain == "NEVPNErrorDomain" && error.code == 5 { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift index c91335c93..e4e99f33d 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift @@ -15,7 +15,7 @@ struct SessionView: View { @EnvironmentObject var store: Store var body: some View { - switch store.status { + switch store.vpnStatus { case .connected: switch store.resourceList { case .loaded(let resources): diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift index 7f4964f29..938aa3bcb 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift @@ -245,7 +245,7 @@ public struct SettingsView: View { ToolbarItem(placement: .navigationBarTrailing) { Button("Save") { let action = ConfirmationAlertContinueAction.saveAllSettingsAndDismiss - if case .connected = store.status { + if case .connected = store.vpnStatus { self.confirmationAlertContinueAction = action self.isShowingConfirmationAlert = true } else { @@ -313,7 +313,7 @@ public struct SettingsView: View { "Apply", action: { let action = ConfirmationAlertContinueAction.saveSettings - if [.connected, .connecting, .reasserting].contains(store.status) { + if [.connected, .connecting, .reasserting].contains(store.vpnStatus) { self.confirmationAlertContinueAction = action self.isShowingConfirmationAlert = true } else { @@ -758,7 +758,7 @@ public struct SettingsView: View { private func saveSettings() async throws { try await viewModel.save() - if [.connected, .connecting, .reasserting].contains(store.status) { + if [.connected, .connecting, .reasserting].contains(store.vpnStatus) { // TODO: Warn user instead of signing out try await self.store.signOut() } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift index f626a73cd..285cd6e25 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift @@ -54,12 +54,12 @@ struct iOSNavigationView: View { // swiftlint:disable:this type_n Label("Settings", systemImage: "gear") } ) - .disabled(store.status == .invalid) + .disabled(store.vpnStatus == .invalid) } private var authMenu: some View { Menu { - if store.status == .connected { + if store.vpnStatus == .connected { Text("Signed in as \(store.actorName)") Button( action: { diff --git a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift index 654cf7346..cd47665a2 100644 --- a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift +++ b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift @@ -42,7 +42,12 @@ class PacketTunnelProvider: NEPacketTunnelProvider { completionHandler: @escaping (Error?) -> Void ) { super.startTunnel(options: options, completionHandler: completionHandler) - Log.log("\(#function)") + + // Dummy start to get the extension running on macOS after upgrade + if options?["dryRun"] as? Bool == true { + completionHandler(nil) + return + } // If the tunnel starts up before the GUI after an upgrade crossing the 1.4.15 version boundary, // the old system settings-based config will still be present and the new configuration will be empty.