review

Author: ethanndickson

Coder Desktop/Coder Desktop/Coder_DesktopApp.swift

@@ -47,7 +47,7 @@ class AppDelegate: NSObject, NSApplicationDelegate {
         }
     }
 
-    // MUST eventually call `NSApp.reply(toApplicationShouldTerminate: true)`
+    // This function MUST eventually call `NSApp.reply(toApplicationShouldTerminate: true)`
     // or return `.terminateNow`
     func applicationShouldTerminate(_: NSApplication) -> NSApplication.TerminateReply {
         if !settings.stopVPNOnQuit { return .terminateNow }

Coder Desktop/Coder Desktop/NetworkExtension.swift

@@ -24,6 +24,15 @@ enum NetworkExtensionState: Equatable {
 /// An actor that handles configuring, enabling, and disabling the VPN tunnel via the
 /// NetworkExtension APIs.
 extension CoderVPNService {
+    func hasNetworkExtensionConfig() async -> Bool {
+        do {
+            _ = try await getTunnelManager()
+            return true
+        } catch {
+            return false
+        }
+    }
+
     func configureNetworkExtension(proto: NETunnelProviderProtocol) async {
         // removing the old tunnels, rather than reconfiguring ensures that configuration changes
         // are picked up.
@@ -61,7 +70,7 @@ extension CoderVPNService {
         }
     }
 
-    func enableNetworkExtension() async {
+    func startTunnel() async {
         do {
             let tm = try await getTunnelManager()
             try tm.connection.startVPNTunnel()
@@ -74,7 +83,7 @@ extension CoderVPNService {
         neState = .enabled
     }
 
-    func disableNetworkExtension() async {
+    func stopTunnel() async {
         do {
             let tm = try await getTunnelManager()
             tm.connection.stopVPNTunnel()
@@ -88,7 +97,7 @@ extension CoderVPNService {
     }
 
     @discardableResult
-    func getTunnelManager() async throws(VPNServiceError) -> NETunnelProviderManager {
+    private func getTunnelManager() async throws(VPNServiceError) -> NETunnelProviderManager {
         var tunnels: [NETunnelProviderManager] = []
         do {
             tunnels = try await NETunnelProviderManager.loadAllFromPreferences()

Coder Desktop/Coder Desktop/SystemExtension.swift

@@ -29,16 +29,7 @@ protocol SystemExtensionAsyncRecorder: Sendable {
 extension CoderVPNService: SystemExtensionAsyncRecorder {
     func recordSystemExtensionState(_ state: SystemExtensionState) async {
         sysExtnState = state
-        if state == .uninstalled {
-            installSystemExtension()
-        }
         if state == .installed {
-            do {
-                try await getTunnelManager()
-                neState = .disabled
-            } catch {
-                neState = .unconfigured
-            }
             // system extension was successfully installed, so we don't need the delegate any more
             systemExtnDelegate = nil
         }
@@ -73,21 +64,7 @@ extension CoderVPNService: SystemExtensionAsyncRecorder {
         return extensionBundle
     }
 
-    func attemptSystemExtensionInstall() {
-        logger.info("checking SystemExtension status")
-        guard let bundleID = extensionBundle.bundleIdentifier else {
-            logger.error("Bundle has no identifier")
-            return
-        }
-        let request = OSSystemExtensionRequest.propertiesRequest(forExtensionWithIdentifier: bundleID, queue: .main)
-        let delegate = SystemExtensionDelegate(asyncDelegate: self)
-        request.delegate = delegate
-        systemExtnDelegate = delegate
-        OSSystemExtensionManager.shared.submitRequest(request)
-        logger.info("submitted SystemExtension properties request with bundleID: \(bundleID)")
-    }
-
-    private func installSystemExtension() {
+    func installSystemExtension() {
         logger.info("activating SystemExtension")
         guard let bundleID = extensionBundle.bundleIdentifier else {
             logger.error("Bundle has no identifier")
@@ -97,9 +74,11 @@ extension CoderVPNService: SystemExtensionAsyncRecorder {
             forExtensionWithIdentifier: bundleID,
             queue: .main
         )
-        request.delegate = systemExtnDelegate
+        let delegate = SystemExtensionDelegate(asyncDelegate: self)
+        systemExtnDelegate = delegate
+        request.delegate = delegate
         OSSystemExtensionManager.shared.submitRequest(request)
-        logger.info("submitted SystemExtension activate request with bundleID: \(bundleID)")
+        logger.info("submitted SystemExtension request with bundleID: \(bundleID)")
     }
 }
 
@@ -109,8 +88,6 @@ class SystemExtensionDelegate<AsyncDelegate: SystemExtensionAsyncRecorder>:
     NSObject, OSSystemExtensionRequestDelegate
 {
     private var logger = Logger(subsystem: Bundle.main.bundleIdentifier!, category: "vpn-installer")
-    // TODO: Refactor this to use a continuation, so the result of a request can be
-    // 'await'd for the determined state
     private var asyncDelegate: AsyncDelegate
 
     init(asyncDelegate: AsyncDelegate) {
@@ -161,38 +138,4 @@ class SystemExtensionDelegate<AsyncDelegate: SystemExtensionAsyncRecorder>:
         logger.info("Replacing \(request.identifier) v\(existing.bundleShortVersion) with v\(`extension`.bundleShortVersion)")
         return .replace
     }
-
-    public func request(
-        _: OSSystemExtensionRequest,
-        foundProperties properties: [OSSystemExtensionProperties]
-    ) {
-        // In debug builds we always replace the SE to test
-        // changes made without bumping the version
-        #if DEBUG
-            Task { [asyncDelegate] in
-                await asyncDelegate.recordSystemExtensionState(.uninstalled)
-            }
-            return
-        #else
-            let version = Bundle.main.object(forInfoDictionaryKey: "CFBundleVersion") as? String
-            let shortVersion = Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String
-
-            let versionMatches = properties.contains { sysex in
-                sysex.isEnabled
-                    && sysex.bundleVersion == version
-                    && sysex.bundleShortVersion == shortVersion
-            }
-            if versionMatches {
-                Task { [asyncDelegate] in
-                    await asyncDelegate.recordSystemExtensionState(.installed)
-                }
-                return
-            }
-
-            // Either uninstalled or needs replacing
-            Task { [asyncDelegate] in
-                await asyncDelegate.recordSystemExtensionState(.uninstalled)
-            }
-        #endif
-    }
 }

Coder Desktop/Coder Desktop/VPNService.swift

@@ -65,7 +65,15 @@ final class CoderVPNService: NSObject, VPNService {
 
     override init() {
         super.init()
-        attemptSystemExtensionInstall()
+        installSystemExtension()
+        Task {
+            neState = if await hasNetworkExtensionConfig() {
+                .disabled
+            } else {
+                .unconfigured
+            }
+        }
+        xpc.getPeerState()
         NotificationCenter.default.addObserver(
             self,
             selector: #selector(vpnDidUpdate(_:)),
@@ -91,15 +99,15 @@ final class CoderVPNService: NSObject, VPNService {
             return
         }
 
-        await enableNetworkExtension()
+        await startTunnel()
         // this ping is somewhat load bearing since it causes xpc to init
         xpc.ping()
         logger.debug("network extension enabled")
     }
 
     func stop() async {
         guard tunnelState == .connected else { return }
-        await disableNetworkExtension()
+        await stopTunnel()
         logger.info("network extension stopped")
     }
 
@@ -134,11 +142,11 @@ final class CoderVPNService: NSObject, VPNService {
     }
 
     func onExtensionPeerState(_ data: Data?) {
-        logger.info("network extension peer state")
         guard let data else {
-            logger.error("could not retrieve peer state from network extension")
+            logger.error("could not retrieve peer state from network extension, it may not be running")
             return
         }
+        logger.info("received network extension peer state")
         do {
             let msg = try Vpn_PeerUpdate(serializedBytes: data)
             debugPrint(msg)
@@ -218,11 +226,6 @@ extension CoderVPNService {
         case .connecting:
             tunnelState = .connecting
         case .connected:
-            // If we moved from disabled to connected, then the NE was already
-            // running, and we need to request the current peer state
-            if tunnelState == .disabled {
-                xpc.getPeerState()
-            }
             tunnelState = .connected
         case .reasserting:
             tunnelState = .connecting