Improved tunnel session access (#655)

Task/Issue URL: https://app.asana.com/0/1203137811378537/1206513608690551/f iOS PR: duckduckgo/iOS#2448 macOS PR: duckduckgo/macos-browser#2174 What kind of version bump will this require?: Patch ## Description Adds support for improved `NETunnelProviderSession` access in our macOS app (and potentially in our iOS app too, in the future).
duckduckgo · Feb 15, 2024 · 93a9a41 · 93a9a41
1 parent 328ce45
commit 93a9a41
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 36 deletions.
diff --git a/Sources/NetworkProtection/Controllers/TunnelController.swift b/Sources/NetworkProtection/Controllers/TunnelController.swift
@@ -17,6 +17,7 @@
 //
 
 import Foundation
+import NetworkExtension
 
 /// This protocol offers an interface to control the tunnel.
 ///
@@ -36,3 +37,12 @@ public protocol TunnelController {
     ///
     var isConnected: Bool { get async }
 }
+
+/// A convenience tunnel session provider protocol.
+///
+/// This should eventually be unified onto `TunnelController`, so that all these requests can be made
+/// directly or through IPC, but this protocol is added to avoid having to tackle that right now..
+///
+public protocol TunnelSessionProvider {
+    func activeSession() async -> NETunnelProviderSession?
+}
diff --git a/Sources/NetworkProtection/Session/ConnectionSessionUtilities.swift b/Sources/NetworkProtection/Session/ConnectionSessionUtilities.swift
@@ -22,6 +22,17 @@ import NetworkExtension
 /// These are only usable from the App that owns the tunnel.
 ///
 public class ConnectionSessionUtilities {
+
+    /// Ideally we should remove these for iOS too.
+    ///
+    /// This has been deprecated in macOS to avoid making multiple calls to
+    /// `NEPacketTunnelProviderManager.loadAllFromPreferences` since it causes notification
+    /// degradation issues over time.  Also, I tried removing this for both platforms, but iOS doesn't have
+    /// good ways to pass the tunnel controller where we need the session.
+    ///
+    /// Ref: https://app.asana.com/0/1203137811378537/1206513608690551/f
+    ///
+    @available(macOS, deprecated: 10.0, message: "Use NetworkProtectionTunnelController.activeSession instead.")
     public static func activeSession(networkExtensionBundleID: String) async throws -> NETunnelProviderSession? {
         let managers = try await NETunnelProviderManager.loadAllFromPreferences()
 
@@ -40,6 +51,16 @@ public class ConnectionSessionUtilities {
         return session
     }
 
+    /// Ideally we should remove these for iOS too.
+    ///
+    /// This has been deprecated in macOS to avoid making multiple calls to
+    /// `NEPacketTunnelProviderManager.loadAllFromPreferences` since it causes notification
+    /// degradation issues over time.  Also, I tried removing this for both platforms, but iOS doesn't have
+    /// good ways to pass the tunnel controller where we need the session.
+    ///
+    /// Ref: https://app.asana.com/0/1203137811378537/1206513608690551/f
+    ///
+    @available(macOS, deprecated: 10.0, message: "Use NetworkProtectionTunnelController.activeSession instead.")
     public static func activeSession() async throws -> NETunnelProviderSession? {
         let managers = try await NETunnelProviderManager.loadAllFromPreferences()
 

diff --git a/...workProtection/Status/ConnectionErrorObserver/ConnectionErrorObserverThroughSession.swift b/...workProtection/Status/ConnectionErrorObserver/ConnectionErrorObserverThroughSession.swift
@@ -33,6 +33,7 @@ public class ConnectionErrorObserverThroughSession: ConnectionErrorObserver {
 
     // MARK: - Notifications
 
+    private let tunnelSessionProvider: TunnelSessionProvider
     private let notificationCenter: NotificationCenter
     private let platformNotificationCenter: NotificationCenter
     private let platformDidWakeNotification: Notification.Name
@@ -44,14 +45,16 @@ public class ConnectionErrorObserverThroughSession: ConnectionErrorObserver {
 
     // MARK: - Initialization
 
-    public init(notificationCenter: NotificationCenter = .default,
+    public init(tunnelSessionProvider: TunnelSessionProvider,
+                notificationCenter: NotificationCenter = .default,
                 platformNotificationCenter: NotificationCenter,
                 platformDidWakeNotification: Notification.Name,
                 log: OSLog = .networkProtection) {
 
         self.notificationCenter = notificationCenter
         self.platformNotificationCenter = platformNotificationCenter
         self.platformDidWakeNotification = platformDidWakeNotification
+        self.tunnelSessionProvider = tunnelSessionProvider
         self.log = log
 
         start()
@@ -72,7 +75,7 @@ public class ConnectionErrorObserverThroughSession: ConnectionErrorObserver {
     private func handleDidWake(_ notification: Notification) {
         Task {
             do {
-                guard let session = try await ConnectionSessionUtilities.activeSession() else {
+                guard let session = await tunnelSessionProvider.activeSession() else {
                     return
                 }
 

diff --git a/...tion/Status/ConnectionServerInfoObserver/ConnectionServerInfoObserverThroughSession.swift b/...tion/Status/ConnectionServerInfoObserver/ConnectionServerInfoObserverThroughSession.swift
@@ -33,6 +33,8 @@ public class ConnectionServerInfoObserverThroughSession: ConnectionServerInfoObs
 
     private let subject = CurrentValueSubject<NetworkProtectionStatusServerInfo, Never>(.unknown)
 
+    private let tunnelSessionProvider: TunnelSessionProvider
+
     // MARK: - Notifications
 
     private let notificationCenter: NotificationCenter
@@ -46,14 +48,16 @@ public class ConnectionServerInfoObserverThroughSession: ConnectionServerInfoObs
 
     // MARK: - Initialization
 
-    public init(notificationCenter: NotificationCenter = .default,
+    public init(tunnelSessionProvider: TunnelSessionProvider,
+                notificationCenter: NotificationCenter = .default,
                 platformNotificationCenter: NotificationCenter,
                 platformDidWakeNotification: Notification.Name,
                 log: OSLog = .networkProtection) {
 
         self.notificationCenter = notificationCenter
         self.platformNotificationCenter = platformNotificationCenter
         self.platformDidWakeNotification = platformDidWakeNotification
+        self.tunnelSessionProvider = tunnelSessionProvider
         self.log = log
 
         start()
@@ -73,15 +77,11 @@ public class ConnectionServerInfoObserverThroughSession: ConnectionServerInfoObs
 
     private func handleDidWake(_ notification: Notification) {
         Task {
-            do {
-                guard let session = try await ConnectionSessionUtilities.activeSession() else {
-                    return
-                }
-
-                await updateServerInfo(session: session)
-            } catch {
-                os_log("Failed to handle wake %{public}@", log: log, type: .error, error.localizedDescription)
+            guard let session = await tunnelSessionProvider.activeSession() else {
+                return
             }
+
+            await updateServerInfo(session: session)
         }
     }
 

diff --git a/...rkProtection/Status/ConnectionStatusObserver/ConnectionStatusObserverThroughSession.swift b/...rkProtection/Status/ConnectionStatusObserver/ConnectionStatusObserverThroughSession.swift
@@ -33,6 +33,8 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
 
     private let subject = CurrentValueSubject<ConnectionStatus, Never>(.disconnected)
 
+    private let tunnelSessionProvider: TunnelSessionProvider
+
     // MARK: - Notifications
     private let notificationCenter: NotificationCenter
     private let platformNotificationCenter: NotificationCenter
@@ -45,14 +47,16 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
 
     // MARK: - Initialization
 
-    public init(notificationCenter: NotificationCenter = .default,
+    public init(tunnelSessionProvider: TunnelSessionProvider,
+                notificationCenter: NotificationCenter = .default,
                 platformNotificationCenter: NotificationCenter,
                 platformDidWakeNotification: Notification.Name,
                 log: OSLog = .networkProtection) {
 
         self.notificationCenter = notificationCenter
         self.platformNotificationCenter = platformNotificationCenter
         self.platformDidWakeNotification = platformDidWakeNotification
+        self.tunnelSessionProvider = tunnelSessionProvider
         self.log = log
 
         start()
@@ -66,20 +70,6 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
     }
 
     private func startObservers() {
-        notificationCenter.publisher(for: .NEVPNConfigurationChange).sink { _ in
-            Task {
-                // As crazy as it seems, this calls fixes an issue with tunnel session
-                // having a nil manager, when in theory it should never be `nil`.  I don't know
-                // why this happens, but I believe it may be because we run multiple instances
-                // of our App controlling the session, and if any modification is made to the
-                // session, other instances should reload it from preferences.
-                //
-                // For better or worse, this line ensures the session's manager is not nil.
-                //
-                try? await NETunnelProviderManager.loadAllFromPreferences()
-            }
-        }.store(in: &cancellables)
-
         notificationCenter.publisher(for: .NEVPNStatusDidChange).sink { [weak self] notification in
             self?.handleStatusChangeNotification(notification)
         }.store(in: &cancellables)
@@ -90,7 +80,7 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
     }
 
     private func loadInitialStatus() async {
-        guard let session = try? await ConnectionSessionUtilities.activeSession() else {
+        guard let session = await tunnelSessionProvider.activeSession() else {
             return
         }
 
@@ -101,15 +91,11 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
 
     private func handleDidWake(_ notification: Notification) {
         Task {
-            do {
-                guard let session = try await ConnectionSessionUtilities.activeSession() else {
-                    return
-                }
-
-                handleStatusChange(in: session)
-            } catch {
-                os_log("%{public}@: failed to handle wake %{public}@", log: log, type: .error, String(describing: self), error.localizedDescription)
+            guard let session = await tunnelSessionProvider.activeSession() else {
+                return
             }
+
+            handleStatusChange(in: session)
         }
     }
 
@@ -162,6 +148,8 @@ public class ConnectionStatusObserverThroughSession: ConnectionStatusObserver {
     // MARK: - Logging
 
     private func logStatusChanged(status: ConnectionStatus) {
-        os_log("%{public}@: connection status is now %{public}@", log: log, type: .debug, String(describing: self), String(describing: status))
+        let unmanagedObject = Unmanaged.passUnretained(self)
+        let address = unmanagedObject.toOpaque()
+        os_log("%{public}@<%{public}@>: connection status is now %{public}@", log: log, type: .debug, String(describing: self), String(describing: address), String(describing: status))
     }
 }