From 828d7c04d4a264c3c067ee0ee2f0b4839c7f17c7 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 13 May 2025 12:41:08 +1000 Subject: [PATCH 1/3] fix: manually upgrade network extension --- .../Coder-Desktop/VPN/NetworkExtension.swift | 3 +- .../Coder-Desktop/VPN/VPNService.swift | 3 +- .../VPN/VPNSystemExtension.swift | 124 ++++++++++++------ .../Coder-Desktop/Views/VPN/VPNMenu.swift | 19 ++- .../Coder-Desktop/Views/VPN/VPNState.swift | 6 +- 5 files changed, 114 insertions(+), 41 deletions(-) diff --git a/Coder-Desktop/Coder-Desktop/VPN/NetworkExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/NetworkExtension.swift index 660ef37d..7c90bd5d 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/NetworkExtension.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/NetworkExtension.swift @@ -58,8 +58,9 @@ extension CoderVPNService { try await tm.saveToPreferences() neState = .disabled } catch { + // This typically fails when the user declines the permission dialog logger.error("save tunnel failed: \(error)") - neState = .failed(error.localizedDescription) + neState = .failed("Failed to save tunnel: \(error.localizedDescription). Try logging in and out again.") } } diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift index c3c17738..baa16804 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift @@ -81,12 +81,13 @@ final class CoderVPNService: NSObject, VPNService { // systemExtnDelegate holds a reference to the SystemExtensionDelegate so that it doesn't get // garbage collected while the OSSystemExtensionRequest is in flight, since the OS framework // only stores a weak reference to the delegate. - var systemExtnDelegate: SystemExtensionDelegate? + var systemExtnDelegate: SystemExtensionDelegate! var serverAddress: String? override init() { super.init() + systemExtnDelegate = SystemExtensionDelegate(asyncDelegate: self) } func start() async { diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift index aade55d9..a6ed431f 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift @@ -22,6 +22,35 @@ enum SystemExtensionState: Equatable, Sendable { } } +let extensionBundle: Bundle = { + let extensionsDirectoryURL = URL( + fileURLWithPath: "Contents/Library/SystemExtensions", + relativeTo: Bundle.main.bundleURL + ) + let extensionURLs: [URL] + do { + extensionURLs = try FileManager.default.contentsOfDirectory(at: extensionsDirectoryURL, + includingPropertiesForKeys: nil, + options: .skipsHiddenFiles) + } catch { + fatalError("Failed to get the contents of " + + "\(extensionsDirectoryURL.absoluteString): \(error.localizedDescription)") + } + + // here we're just going to assume that there is only ever going to be one SystemExtension + // packaged up in the application bundle. If we ever need to ship multiple versions or have + // multiple extensions, we'll need to revisit this assumption. + guard let extensionURL = extensionURLs.first else { + fatalError("Failed to find any system extensions") + } + + guard let extensionBundle = Bundle(url: extensionURL) else { + fatalError("Failed to create a bundle with URL \(extensionURL.absoluteString)") + } + + return extensionBundle +}() + protocol SystemExtensionAsyncRecorder: Sendable { func recordSystemExtensionState(_ state: SystemExtensionState) async } @@ -36,35 +65,6 @@ extension CoderVPNService: SystemExtensionAsyncRecorder { } } - var extensionBundle: Bundle { - let extensionsDirectoryURL = URL( - fileURLWithPath: "Contents/Library/SystemExtensions", - relativeTo: Bundle.main.bundleURL - ) - let extensionURLs: [URL] - do { - extensionURLs = try FileManager.default.contentsOfDirectory(at: extensionsDirectoryURL, - includingPropertiesForKeys: nil, - options: .skipsHiddenFiles) - } catch { - fatalError("Failed to get the contents of " + - "\(extensionsDirectoryURL.absoluteString): \(error.localizedDescription)") - } - - // here we're just going to assume that there is only ever going to be one SystemExtension - // packaged up in the application bundle. If we ever need to ship multiple versions or have - // multiple extensions, we'll need to revisit this assumption. - guard let extensionURL = extensionURLs.first else { - fatalError("Failed to find any system extensions") - } - - guard let extensionBundle = Bundle(url: extensionURL) else { - fatalError("Failed to create a bundle with URL \(extensionURL.absoluteString)") - } - - return extensionBundle - } - func installSystemExtension() { logger.info("activating SystemExtension") guard let bundleID = extensionBundle.bundleIdentifier else { @@ -75,9 +75,7 @@ extension CoderVPNService: SystemExtensionAsyncRecorder { forExtensionWithIdentifier: bundleID, queue: .main ) - let delegate = SystemExtensionDelegate(asyncDelegate: self) - systemExtnDelegate = delegate - request.delegate = delegate + request.delegate = systemExtnDelegate OSSystemExtensionManager.shared.submitRequest(request) logger.info("submitted SystemExtension request with bundleID: \(bundleID)") } @@ -90,6 +88,10 @@ class SystemExtensionDelegate: { private var logger = Logger(subsystem: Bundle.main.bundleIdentifier!, category: "vpn-installer") private var asyncDelegate: AsyncDelegate + // The `didFinishWithResult` function is called for both activation, + // deactivation, and replacement requests. The API provides no way to + // differentiate them. https://developer.apple.com/forums/thread/684021 + private var state: SystemExtensionDelegateState = .installing init(asyncDelegate: AsyncDelegate) { self.asyncDelegate = asyncDelegate @@ -109,9 +111,35 @@ class SystemExtensionDelegate: } return } - logger.info("SystemExtension activated") - Task { [asyncDelegate] in - await asyncDelegate.recordSystemExtensionState(SystemExtensionState.installed) + switch state { + case .installing: + logger.info("SystemExtension installed") + Task { [asyncDelegate] in + await asyncDelegate.recordSystemExtensionState(SystemExtensionState.installed) + } + case .deleting: + logger.info("SystemExtension deleted") + Task { [asyncDelegate] in + await asyncDelegate.recordSystemExtensionState(SystemExtensionState.uninstalled) + } + let request = OSSystemExtensionRequest.activationRequest( + forExtensionWithIdentifier: extensionBundle.bundleIdentifier!, + queue: .main + ) + request.delegate = self + state = .installing + OSSystemExtensionManager.shared.submitRequest(request) + case .replacing: + logger.info("SystemExtension replaced") + // The installed extension now has the same version strings as this + // bundle, so sending the deactivationRequest will work. + let request = OSSystemExtensionRequest.deactivationRequest( + forExtensionWithIdentifier: extensionBundle.bundleIdentifier!, + queue: .main + ) + request.delegate = self + state = .deleting + OSSystemExtensionManager.shared.submitRequest(request) } } @@ -135,8 +163,30 @@ class SystemExtensionDelegate: actionForReplacingExtension existing: OSSystemExtensionProperties, withExtension extension: OSSystemExtensionProperties ) -> OSSystemExtensionRequest.ReplacementAction { - // swiftlint:disable:next line_length - logger.info("Replacing \(request.identifier) v\(existing.bundleShortVersion) with v\(`extension`.bundleShortVersion)") + logger.info("Replacing \(request.identifier) v\(existing.bundleVersion) with v\(`extension`.bundleVersion)") + // This is counterintuitive, but this function is only called if the + // versions are the same in a dev environment. + // In a release build, this only gets called when the version string is + // different. We don't want to manually reinstall the extension in a dev + // environment, because the bug doesn't happen. + if existing.bundleVersion == `extension`.bundleVersion { + return .replace + } + // To work around the bug described in + // https://github.com/coder/coder-desktop-macos/issues/121, + // we're going to manually reinstall after the replacement is done. + // If we returned `.cancel` here the deactivation request will fail as + // it looks for an extension with the *current* version string. + // There's no way to modify the deactivate request to use a different + // version string (i.e. `existing.bundleVersion`). + logger.info("App upgrade detected, replacing and then reinstalling") + state = .replacing return .replace } } + +enum SystemExtensionDelegateState { + case installing + case replacing + case deleting +} diff --git a/Coder-Desktop/Coder-Desktop/Views/VPN/VPNMenu.swift b/Coder-Desktop/Coder-Desktop/Views/VPN/VPNMenu.swift index 83757efd..89365fd3 100644 --- a/Coder-Desktop/Coder-Desktop/Views/VPN/VPNMenu.swift +++ b/Coder-Desktop/Coder-Desktop/Views/VPN/VPNMenu.swift @@ -81,6 +81,21 @@ struct VPNMenu: View { }.buttonStyle(.plain) TrayDivider() } + // This shows when + // 1. The user is logged in + // 2. The network extension is installed + // 3. The VPN is unconfigured + // It's accompanied by a message in the VPNState view + // that the user needs to reconfigure. + if state.hasSession, vpn.state == .failed(.networkExtensionError(.unconfigured)) { + Button { + state.reconfigure() + } label: { + ButtonRowView { + Text("Reconfigure VPN") + } + }.buttonStyle(.plain) + } if vpn.state == .failed(.systemExtensionError(.needsUserApproval)) { Button { openSystemExtensionSettings() @@ -128,7 +143,9 @@ struct VPNMenu: View { vpn.state == .connecting || vpn.state == .disconnecting || // Prevent starting the VPN before the user has approved the system extension. - vpn.state == .failed(.systemExtensionError(.needsUserApproval)) + vpn.state == .failed(.systemExtensionError(.needsUserApproval)) || + // Prevent starting the VPN without a VPN configuration. + vpn.state == .failed(.networkExtensionError(.unconfigured)) } } diff --git a/Coder-Desktop/Coder-Desktop/Views/VPN/VPNState.swift b/Coder-Desktop/Coder-Desktop/Views/VPN/VPNState.swift index 64c08568..23319020 100644 --- a/Coder-Desktop/Coder-Desktop/Views/VPN/VPNState.swift +++ b/Coder-Desktop/Coder-Desktop/Views/VPN/VPNState.swift @@ -17,6 +17,10 @@ struct VPNState: View { Text("Sign in to use Coder Desktop") .font(.body) .foregroundColor(.secondary) + case (.failed(.networkExtensionError(.unconfigured)), _): + Text("The system VPN requires reconfiguration.") + .font(.body) + .foregroundStyle(.secondary) case (.disabled, _): Text("Enable Coder Connect to see workspaces") .font(.body) @@ -38,7 +42,7 @@ struct VPNState: View { .padding(.horizontal, Theme.Size.trayInset) .padding(.vertical, Theme.Size.trayPadding) .frame(maxWidth: .infinity) - default: + case (.connected, true): EmptyView() } } From 7e61eb926128a4b41952672d14b7ed4bc8109030 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 14 May 2025 13:38:45 +1000 Subject: [PATCH 2/3] review --- .../Coder-Desktop/VPN/VPNService.swift | 3 +- .../VPN/VPNSystemExtension.swift | 53 +++++++++++-------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift index baa16804..c3c17738 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNService.swift @@ -81,13 +81,12 @@ final class CoderVPNService: NSObject, VPNService { // systemExtnDelegate holds a reference to the SystemExtensionDelegate so that it doesn't get // garbage collected while the OSSystemExtensionRequest is in flight, since the OS framework // only stores a weak reference to the delegate. - var systemExtnDelegate: SystemExtensionDelegate! + var systemExtnDelegate: SystemExtensionDelegate? var serverAddress: String? override init() { super.init() - systemExtnDelegate = SystemExtensionDelegate(asyncDelegate: self) } func start() async { diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift index a6ed431f..7aae356b 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift @@ -66,18 +66,8 @@ extension CoderVPNService: SystemExtensionAsyncRecorder { } func installSystemExtension() { - logger.info("activating SystemExtension") - guard let bundleID = extensionBundle.bundleIdentifier else { - logger.error("Bundle has no identifier") - return - } - let request = OSSystemExtensionRequest.activationRequest( - forExtensionWithIdentifier: bundleID, - queue: .main - ) - request.delegate = systemExtnDelegate - OSSystemExtensionManager.shared.submitRequest(request) - logger.info("submitted SystemExtension request with bundleID: \(bundleID)") + systemExtnDelegate = SystemExtensionDelegate(asyncDelegate: self) + systemExtnDelegate!.installSystemExtension() } } @@ -91,7 +81,8 @@ class SystemExtensionDelegate: // The `didFinishWithResult` function is called for both activation, // deactivation, and replacement requests. The API provides no way to // differentiate them. https://developer.apple.com/forums/thread/684021 - private var state: SystemExtensionDelegateState = .installing + // This tracks the last request type made, to handle them accordingly. + private var action: SystemExtensionDelegateAction = .none init(asyncDelegate: AsyncDelegate) { self.asyncDelegate = asyncDelegate @@ -99,6 +90,19 @@ class SystemExtensionDelegate: logger.info("SystemExtensionDelegate initialized") } + func installSystemExtension() { + logger.info("activating SystemExtension") + let bundleID = extensionBundle.bundleIdentifier! + let request = OSSystemExtensionRequest.activationRequest( + forExtensionWithIdentifier: bundleID, + queue: .main + ) + request.delegate = self + action = .installing + OSSystemExtensionManager.shared.submitRequest(request) + logger.info("submitted SystemExtension request with bundleID: \(bundleID)") + } + func request( _: OSSystemExtensionRequest, didFinishWithResult result: OSSystemExtensionRequest.Result @@ -111,23 +115,24 @@ class SystemExtensionDelegate: } return } - switch state { + switch action { case .installing: logger.info("SystemExtension installed") Task { [asyncDelegate] in - await asyncDelegate.recordSystemExtensionState(SystemExtensionState.installed) + await asyncDelegate.recordSystemExtensionState(.installed) } + action = .none case .deleting: logger.info("SystemExtension deleted") Task { [asyncDelegate] in - await asyncDelegate.recordSystemExtensionState(SystemExtensionState.uninstalled) + await asyncDelegate.recordSystemExtensionState(.uninstalled) } let request = OSSystemExtensionRequest.activationRequest( forExtensionWithIdentifier: extensionBundle.bundleIdentifier!, queue: .main ) request.delegate = self - state = .installing + action = .installing OSSystemExtensionManager.shared.submitRequest(request) case .replacing: logger.info("SystemExtension replaced") @@ -138,8 +143,11 @@ class SystemExtensionDelegate: queue: .main ) request.delegate = self - state = .deleting + action = .deleting OSSystemExtensionManager.shared.submitRequest(request) + case .none: + logger.warning("Received an unexpected request result") + break } } @@ -147,14 +155,14 @@ class SystemExtensionDelegate: logger.error("System extension request failed: \(error.localizedDescription)") Task { [asyncDelegate] in await asyncDelegate.recordSystemExtensionState( - SystemExtensionState.failed(error.localizedDescription)) + .failed(error.localizedDescription)) } } func requestNeedsUserApproval(_ request: OSSystemExtensionRequest) { logger.error("Extension \(request.identifier) requires user approval") Task { [asyncDelegate] in - await asyncDelegate.recordSystemExtensionState(SystemExtensionState.needsUserApproval) + await asyncDelegate.recordSystemExtensionState(.needsUserApproval) } } @@ -180,12 +188,13 @@ class SystemExtensionDelegate: // There's no way to modify the deactivate request to use a different // version string (i.e. `existing.bundleVersion`). logger.info("App upgrade detected, replacing and then reinstalling") - state = .replacing + action = .replacing return .replace } } -enum SystemExtensionDelegateState { +enum SystemExtensionDelegateAction { + case none case installing case replacing case deleting From daa06e9521d087eb4a8f53092ba9be9f7bd231de Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 14 May 2025 15:11:15 +1000 Subject: [PATCH 3/3] fmt --- Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift index 7aae356b..39d625ca 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift @@ -147,7 +147,6 @@ class SystemExtensionDelegate: OSSystemExtensionManager.shared.submitRequest(request) case .none: logger.warning("Received an unexpected request result") - break } }