Prevent VPN server list persistence failures (#603)

Task/Issue URL: https://app.asana.com/0/0/1206201299599597/f iOS PR: duckduckgo/iOS#2275 macOS PR: duckduckgo/macos-browser#1985 What kind of version bump will this require?: Major Description: This PR makes two changes: * The WireGuard invalid state error has been given a reason field, to allow insight into what state is invalid exactly * The server list store no longer prevents the file from being stored if the old one can't be removed; this appears to be a major source of unhandled errors
duckduckgo · Dec 19, 2023 · ae9e918 · ae9e918
1 parent d07e18b
commit ae9e918
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 16 deletions.
diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift
@@ -61,7 +61,7 @@ public enum NetworkProtectionError: LocalizedError {
 
     // Wireguard errors
     case wireGuardCannotLocateTunnelFileDescriptor
-    case wireGuardInvalidState
+    case wireGuardInvalidState(reason: String)
     case wireGuardDnsResolution
     case wireGuardSetNetworkSettings(Error)
     case startWireGuardBackend(Int32)

diff --git a/...tworkProtection/Diagnostics/WireGuardAdapterError+NetworkProtectionErrorConvertible.swift b/...tworkProtection/Diagnostics/WireGuardAdapterError+NetworkProtectionErrorConvertible.swift
@@ -23,8 +23,8 @@ extension WireGuardAdapterError: NetworkProtectionErrorConvertible {
         switch self {
         case .cannotLocateTunnelFileDescriptor:
             return .wireGuardCannotLocateTunnelFileDescriptor
-        case .invalidState:
-            return .wireGuardInvalidState
+        case .invalidState(let reason):
+            return .wireGuardInvalidState(reason: reason.rawValue)
         case .dnsResolution:
             return .wireGuardDnsResolution
         case .setNetworkSettings(let error):

diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift
@@ -891,7 +891,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
         resetRegistrationKey()
 
         let serverCache = NetworkProtectionServerListFileSystemStore(errorEvents: nil)
-        try? serverCache.removeServerList()
+        serverCache.removeServerList()
 
         try? tokenStore.deleteToken()
 

diff --git a/Sources/NetworkProtection/Storage/NetworkProtectionServerListStore.swift b/Sources/NetworkProtection/Storage/NetworkProtectionServerListStore.swift
@@ -163,26 +163,26 @@ public class NetworkProtectionServerListFileSystemStore: NetworkProtectionServer
         do {
             data = try Data(contentsOf: fileURL)
         } catch {
-            try removeServerList()
+            removeServerList()
             throw NetworkProtectionServerListStoreError.failedToReadServerList(error)
         }
 
         do {
             return try JSONDecoder().decode([NetworkProtectionServer].self, from: data)
         } catch {
-            try removeServerList()
+            removeServerList()
             throw NetworkProtectionServerListStoreError.failedToDecodeServerList(error)
         }
     }
 
-    public func removeServerList() throws {
-        if FileManager.default.fileExists(atPath: fileURL.relativePath) {
-            try FileManager.default.removeItem(at: fileURL)
+    public func removeServerList() {
+        if FileManager.default.fileExists(atPath: fileURL.path) {
+            try? FileManager.default.removeItem(at: fileURL)
         }
     }
 
     private func replaceServerList(with newList: [NetworkProtectionServer]) throws {
-        try removeServerList()
+        removeServerList()
 
         let serializedJSONData: Data
 

diff --git a/Sources/NetworkProtection/WireGuardKit/WireGuardAdapter.swift b/Sources/NetworkProtection/WireGuardKit/WireGuardAdapter.swift
@@ -7,12 +7,18 @@ import NetworkExtension
 import WireGuard
 import Common
 
+public enum WireGuardAdapterErrorInvalidStateReason: String {
+    case alreadyStarted
+    case alreadyStopped
+    case updatedTunnelWhileStopped
+}
+
 public enum WireGuardAdapterError: Error {
     /// Failure to locate tunnel file descriptor.
     case cannotLocateTunnelFileDescriptor
 
-    /// Failure to perform an operation in such state.
-    case invalidState
+    /// Failure to perform an operation in such state. Includes a reason why the error was returned.
+    case invalidState(WireGuardAdapterErrorInvalidStateReason)
 
     /// Failure to resolve endpoints.
     case dnsResolution([DNSResolutionError])
@@ -263,7 +269,7 @@ public class WireGuardAdapter {
     public func start(tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (WireGuardAdapterError?) -> Void) {
         workQueue.async {
             guard case .stopped = self.state else {
-                completionHandler(.invalidState)
+                completionHandler(.invalidState(.alreadyStarted))
                 return
             }
 
@@ -307,7 +313,7 @@ public class WireGuardAdapter {
                 break
 
             case .stopped:
-                completionHandler(.invalidState)
+                completionHandler(.invalidState(.alreadyStopped))
                 return
             }
 
@@ -323,14 +329,14 @@ public class WireGuardAdapter {
     /// Update runtime configuration.
     /// - Parameters:
     ///   - tunnelConfiguration: tunnel configuration.
-    ///   - reassert: wether the connection should reassert or not.
+    ///   - reassert: whether the connection should reassert or not.
     ///   - completionHandler: completion handler.
     public func update(tunnelConfiguration: TunnelConfiguration,
                        reassert: Bool = true,
                        completionHandler: @escaping (WireGuardAdapterError?) -> Void) {
         workQueue.async {
             if case .stopped = self.state {
-                completionHandler(.invalidState)
+                completionHandler(.invalidState(.updatedTunnelWhileStopped))
                 return
             }