From 37d7e35008445a0b3917095b23035db5deac5c9b Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Wed, 14 May 2025 17:22:04 +1000 Subject: [PATCH 1/3] chore: set 'stop VPN on quit' setting to true by default (#155) It's fair to assume if you're closing Coder Desktop, you want Coder Connect to stop, so this is the default behaviour. Tailscale also stops their VPN when quitting the app, even though it works fine with the app closed. --- Coder-Desktop/Coder-Desktop/State.swift | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Coder-Desktop/Coder-Desktop/State.swift b/Coder-Desktop/Coder-Desktop/State.swift index e9a02488..902d5a12 100644 --- a/Coder-Desktop/Coder-Desktop/State.swift +++ b/Coder-Desktop/Coder-Desktop/State.swift @@ -55,7 +55,8 @@ class AppState: ObservableObject { } } - @Published var stopVPNOnQuit: Bool = UserDefaults.standard.bool(forKey: Keys.stopVPNOnQuit) { + // Defaults to `true` + @Published var stopVPNOnQuit: Bool = UserDefaults.standard.optionalBool(forKey: Keys.stopVPNOnQuit) ?? true { didSet { guard persistent else { return } UserDefaults.standard.set(stopVPNOnQuit, forKey: Keys.stopVPNOnQuit) @@ -239,3 +240,14 @@ extension LiteralHeader { .init(name: name, value: value) } } + +extension UserDefaults { + // Unlike the exisitng `bool(forKey:)` method which returns `false` for both + // missing values this method can return `nil`. + func optionalBool(forKey key: String) -> Bool? { + guard object(forKey: key) != nil else { + return nil + } + return bool(forKey: key) + } +} From 05e41b7beb583038d7d012dc99017d28fa40acd4 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Fri, 16 May 2025 13:52:53 +1000 Subject: [PATCH 2/3] fix: manually upgrade system extension (#158) This PR addresses #121. The underlying bug is still present in macOS, this is just a workaround, so I'm leaving the issue open. macOS calls `actionForReplacingExtension` whenever the version string(s) of the system extension living inside the Coder Desktop app bundle change. i.e. When a new version of the app is installed: 1. App sends `activationRequest` (it does this on every launch, to see if the NE is installed) 2. Eventually, `actionForReplacingExtension` is called, which can either return `.cancel` or `.replace`. 3. Eventually,`didFinishWithResult` is called with whether the replacement was successful. (`actionForReplacingExtension` is *always* called when developing the app locally, even if the version string(s) don't differ) However, in the linked issue, we note that this replacement process is bug-prone. This bug can be worked around by deleting the system extension (in settings), and reactivating it (such as by relaunching the app). Therefore, in this PR, when `didFinishWithResult` is called following a replacement request, we instead will: 1. Send a `deactivationRequest`, and wait for it to be successful. 2. Send another `activationRequest`, and wait for that to be successful. Of note is that we *cannot* return `.cancel` from `actionForReplacingExtension` and then later send a `deactivationRequest`. `deactivationRequest` *always* searches for a system extension with version string(s) that match the system extension living inside the currently installed app bundle. Therefore, we have to let the replacement take place before attempting to delete it. Also of note is that a successful `deactivationRequest` of the system extension deletes the corresponding VPN configuration. This configuration is normally created by logging in, but if the user is already logged in, we'll update the UI to include a `Reconfigure VPN` button. image I've tested this PR in a fresh macOS 15.4 VM, upgrading from the latest release. I also forced the bug in the linked issue to occur by toggling on the VPN in System Settings before opening the new version of the app for the first time, and going through all the additional prompts did indeed prevent the issue from happening. --- .../Coder-Desktop/VPN/NetworkExtension.swift | 3 +- .../VPN/VPNSystemExtension.swift | 158 ++++++++++++------ .../Coder-Desktop/Views/VPN/VPNMenu.swift | 19 ++- .../Coder-Desktop/Views/VPN/VPNState.swift | 6 +- 4 files changed, 133 insertions(+), 53 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/VPNSystemExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift index aade55d9..39d625ca 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,50 +65,9 @@ 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 { - logger.error("Bundle has no identifier") - return - } - let request = OSSystemExtensionRequest.activationRequest( - forExtensionWithIdentifier: bundleID, - queue: .main - ) - let delegate = SystemExtensionDelegate(asyncDelegate: self) - systemExtnDelegate = delegate - request.delegate = delegate - OSSystemExtensionManager.shared.submitRequest(request) - logger.info("submitted SystemExtension request with bundleID: \(bundleID)") + systemExtnDelegate = SystemExtensionDelegate(asyncDelegate: self) + systemExtnDelegate!.installSystemExtension() } } @@ -90,6 +78,11 @@ 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 + // This tracks the last request type made, to handle them accordingly. + private var action: SystemExtensionDelegateAction = .none init(asyncDelegate: AsyncDelegate) { self.asyncDelegate = asyncDelegate @@ -97,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 @@ -109,9 +115,38 @@ class SystemExtensionDelegate: } return } - logger.info("SystemExtension activated") - Task { [asyncDelegate] in - await asyncDelegate.recordSystemExtensionState(SystemExtensionState.installed) + switch action { + case .installing: + logger.info("SystemExtension installed") + Task { [asyncDelegate] in + await asyncDelegate.recordSystemExtensionState(.installed) + } + action = .none + case .deleting: + logger.info("SystemExtension deleted") + Task { [asyncDelegate] in + await asyncDelegate.recordSystemExtensionState(.uninstalled) + } + let request = OSSystemExtensionRequest.activationRequest( + forExtensionWithIdentifier: extensionBundle.bundleIdentifier!, + queue: .main + ) + request.delegate = self + action = .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 + action = .deleting + OSSystemExtensionManager.shared.submitRequest(request) + case .none: + logger.warning("Received an unexpected request result") } } @@ -119,14 +154,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) } } @@ -135,8 +170,31 @@ 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") + action = .replacing return .replace } } + +enum SystemExtensionDelegateAction { + case none + 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 9f356e55dab84d783959b039d5621b5a130e1737 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Fri, 16 May 2025 15:38:43 +1000 Subject: [PATCH 3/3] fix: set network extension as unconfigured when system extension is deleted (#162) This fixes a bug when `Launch Coder Connect at startup` is enabled when updating the app with #161, where the app attempts to start too early, as it thinks the VPN is configured, but it was unconfigured with the deletion of the system extension. --- Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift index 39d625ca..6b242020 100644 --- a/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift +++ b/Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift @@ -63,6 +63,10 @@ extension CoderVPNService: SystemExtensionAsyncRecorder { // system extension was successfully installed, so we don't need the delegate any more systemExtnDelegate = nil } + if state == .uninstalled { + // System extension was deleted, and the VPN configurations go with it + neState = .unconfigured + } } func installSystemExtension() {