Merge branch 'tunnel-manager-verify-tunnel-configuration'

7 files changed, 262 insertions, 70 deletions

diff --git a/ios/CHANGELOG.md b/ios/CHANGELOG.md
index 40b9097073..a05b8fac1e 100644
--- a/ios/CHANGELOG.md
+++ b/ios/CHANGELOG.md
@@ -34,6 +34,8 @@ Line wrap the file at 100 chars.                                              Th
   disable on-demand when stopping the tunnel from within the app.
 - Fix bug that caused the app to skip tunnel settings migration from older versions of the app.
 - Localize some of well known StoreKit errors so that they look less cryptic when presented to user.
+- Improve tunnel settings verification to address issues with broken tunnel and missing Keychain
+  entries to tunnel settings in cases such as when setting up a new device from backup.
 
 
 ## [2021.1] - 2021-03-16
diff --git a/ios/MullvadVPN/Account.swift b/ios/MullvadVPN/Account.swift
index 6de4b7bc28..ccbb98f2c7 100644
--- a/ios/MullvadVPN/Account.swift
+++ b/ios/MullvadVPN/Account.swift
@@ -192,6 +192,23 @@ class Account {
         exclusivityController.addOperation(operation, categories: [.exclusive])
     }
 
+    /// Forget that user was logged in, but do not attempt to unset account in `TunnelManager`.
+    /// This function is used in cases where the tunnel or tunnel settings are corrupt.
+    func forget(completionHandler: @escaping () -> Void) {
+        let operation = AsyncBlockOperation { (finish) in
+            DispatchQueue.main.async {
+                self.removeFromPreferences()
+                finish()
+            }
+        }
+
+        operation.addDidFinishBlockObserver(queue: .main) { (operation) in
+            completionHandler()
+        }
+
+        exclusivityController.addOperation(operation, categories: [.exclusive])
+    }
+
     func updateAccountExpiry() {
         let makeRequest = ResultOperation { () -> TokenPayload<EmptyPayload>? in
             return self.token.flatMap { (token) in
diff --git a/ios/MullvadVPN/AppDelegate.swift b/ios/MullvadVPN/AppDelegate.swift
index 1bbcd2ca36..1fce9cc3c7 100644
--- a/ios/MullvadVPN/AppDelegate.swift
+++ b/ios/MullvadVPN/AppDelegate.swift
@@ -73,49 +73,67 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         RelayCache.shared.updateRelays()
 
         // Load initial relays
+        self.logger?.debug("Load relays")
         RelayCache.shared.read { (result) in
             DispatchQueue.main.async {
                 switch result {
                 case .success(let cachedRelays):
                     self.cachedRelays = cachedRelays
+                    self.logger?.debug("Loaded relays")
 
                 case .failure(let error):
-                    self.logger?.error(chainedError: error, message: "Failed to fetch initial relays")
+                    self.logger?.error(chainedError: error, message: "Failed to load initial relays")
                 }
             }
         }
 
         // Load tunnels
+        self.logger?.debug("Load tunnels")
         TunnelManager.shared.loadTunnel(accountToken: Account.shared.token) { (result) in
             DispatchQueue.main.async {
-                if case .failure(let error) = result {
-                    fatalError(error.displayChain(message: "Failed to load the tunnel for account"))
-                }
+                switch result {
+                case .success:
+                    self.logger?.debug("Loaded tunnels")
 
-                TunnelManager.shared.getRelayConstraints { (result) in
-                    DispatchQueue.main.async {
-                        switch result {
-                        case .success(let relayConstraints):
-                            self.relayConstraints = relayConstraints
+                    // Fetch relay constraints when logged in.
+                    if Account.shared.isLoggedIn {
+                        self.logger?.debug("Load relay constraints")
 
-                        case .failure(let error):
-                            self.logger?.error(chainedError: error, message: "Failed to load relay constraints")
-                        }
+                        TunnelManager.shared.getRelayConstraints { (result) in
+                            DispatchQueue.main.async {
+                                switch result {
+                                case .success(let relayConstraints):
+                                    self.relayConstraints = relayConstraints
+                                    self.logger?.debug("Loaded relay constraints: \(relayConstraints)")
 
-                        self.rootContainer = RootContainerViewController()
-                        self.rootContainer?.delegate = self
-                        self.window?.rootViewController = self.rootContainer
+                                case .failure(let error):
+                                    self.logger?.error(chainedError: error, message: "Failed to load relay constraints")
+                                }
 
-                        switch UIDevice.current.userInterfaceIdiom {
-                        case .pad:
-                            self.setupPadUI()
+                                self.didFinishInitialization()
+                            }
+                        }
+                    } else {
+                        self.didFinishInitialization()
+                    }
+
+                case .failure(let error):
+                    self.logger?.error(chainedError: error, message: "Failed to load tunnels")
 
-                        case .phone:
-                            self.setupPhoneUI()
+                    switch error {
+                    case .loadAllVPNConfigurations(_), .removeInconsistentVPNConfiguration(_):
+                        // TODO: avoid throwing fatal error and show the problem report UI instead.
+                        fatalError(error.displayChain(message: "Failed to load tunnels"))
 
-                        default:
-                            fatalError()
+                    case .migrateTunnelSettings(_), .readTunnelSettings(_):
+                        // Forget that user was logged in since tunnel settings are likely corrupt
+                        // or missing.
+                        Account.shared.forget {
+                            self.didFinishInitialization()
                         }
+
+                    default:
+                        fatalError("Unexpected error coming from loadTunnel()")
                     }
                 }
             }
@@ -124,8 +142,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         // Show the window
         self.window?.makeKeyAndVisible()
 
-        startPaymentQueueHandling()
-
         return true
     }
 
@@ -135,6 +151,35 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
 
     // MARK: - Private
 
+    private func didFinishInitialization() {
+        self.logger?.debug("Finished initialization. Show user interface.")
+
+        self.rootContainer = RootContainerViewController()
+        self.rootContainer?.delegate = self
+        self.window?.rootViewController = self.rootContainer
+
+        switch UIDevice.current.userInterfaceIdiom {
+        case .pad:
+            self.setupPadUI()
+
+        case .phone:
+            self.setupPhoneUI()
+
+        default:
+            fatalError()
+        }
+
+        startPaymentQueueHandling()
+    }
+
+    private func startPaymentQueueHandling() {
+        let paymentManager = AppStorePaymentManager.shared
+        paymentManager.delegate = self
+
+        Account.shared.startPaymentMonitoring(with: paymentManager)
+        paymentManager.startPaymentQueueMonitoring()
+    }
+
     private func setupPadUI() {
         let selectLocationController = makeSelectLocationController()
         let connectController = makeConnectViewController()
@@ -316,7 +361,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
             connectController?.setMainContentHidden(true, animated: animated)
         }
     }
-
 }
 
 // MARK: - RootContainerViewControllerDelegate
diff --git a/ios/MullvadVPN/AppStorePaymentManager.swift b/ios/MullvadVPN/AppStorePaymentManager.swift
index ac1bb386b7..4df32e7efb 100644
--- a/ios/MullvadVPN/AppStorePaymentManager.swift
+++ b/ios/MullvadVPN/AppStorePaymentManager.swift
@@ -160,6 +160,7 @@ class AppStorePaymentManager: NSObject, SKPaymentTransactionObserver {
     }
 
     func startPaymentQueueMonitoring() {
+        self.logger.debug("Start payment queue monitoring.")
         queue.add(self)
     }
 
diff --git a/ios/MullvadVPN/DisplayChainedError.swift b/ios/MullvadVPN/DisplayChainedError.swift
index d7cfc5a81c..c27658b6ce 100644
--- a/ios/MullvadVPN/DisplayChainedError.swift
+++ b/ios/MullvadVPN/DisplayChainedError.swift
@@ -76,6 +76,9 @@ extension TunnelManager.Error: DisplayChainedError {
         case .removeTunnelSettings(_):
             return NSLocalizedString("Failed to remove tunnel settings", comment: "")
 
+        case .migrateTunnelSettings(_):
+            return NSLocalizedString("Failed to migrate tunnel settings", comment: "")
+
         case .pushWireguardKey(let restError):
             let reason = restError.errorChainDescription ?? ""
             var message = String(format: NSLocalizedString("Failed to send the WireGuard key to server: %@", comment: ""), reason)
diff --git a/ios/MullvadVPN/TunnelManager.swift b/ios/MullvadVPN/TunnelManager.swift
index 6903856626..63ea678cb5 100644
--- a/ios/MullvadVPN/TunnelManager.swift
+++ b/ios/MullvadVPN/TunnelManager.swift
@@ -159,9 +159,8 @@ class TunnelManager {
         /// A failure to remove the system VPN configuration
         case removeVPNConfiguration(Swift.Error)
 
-        /// A failure to perform a recovery (by removing the VPN configuration) when the
-        /// inconsistency between the given account token and the username saved in the tunnel
-        /// provider configuration is detected.
+        /// A failure to perform a recovery (by removing the VPN configuration) when a corrupt
+        /// VPN configuration is detected.
         case removeInconsistentVPNConfiguration(Swift.Error)
 
         /// A failure to read tunnel settings
@@ -176,6 +175,9 @@ class TunnelManager {
         /// A failure to remove the tunnel settings from Keychain
         case removeTunnelSettings(TunnelSettingsManager.Error)
 
+        /// A failure to migrate tunnel settings
+        case migrateTunnelSettings(TunnelSettingsManager.Error)
+
         /// Unable to obtain the persistent keychain reference for the tunnel settings
         case obtainPersistentKeychainReference(TunnelSettingsManager.Error)
 
@@ -215,6 +217,8 @@ class TunnelManager {
                 return "Failed to update the tunnel settings"
             case .removeTunnelSettings:
                 return "Failed to remove the tunnel settings"
+            case .migrateTunnelSettings:
+                return "Failed to migrate the tunnel settings"
             case .obtainPersistentKeychainReference:
                 return "Failed to obtain the persistent keychain reference"
             case .pushWireguardKey:
@@ -304,7 +308,7 @@ class TunnelManager {
         }
     }
 
-    /// Initialize the TunnelManager with the tunnel from the system
+    /// Initialize the TunnelManager with the tunnel from the system.
     ///
     /// The given account token is used to ensure that the system tunnel was configured for the same
     /// account. The system tunnel is removed in case of inconsistency.
@@ -315,44 +319,7 @@ class TunnelManager {
                     if let error = error {
                         finish(.failure(.loadAllVPNConfigurations(error)))
                     } else {
-                        if let accountToken = accountToken {
-                            // Migrate the tunnel settings if needed
-                            self.migrateTunnelSettings(accountToken: accountToken)
-
-                            // Load last known public key
-                            self.loadPublicKey(accountToken: accountToken)
-                        }
-
-                        if let tunnelProvider = tunnels?.first {
-                            // Ensure the consistency between the given account token and the one
-                            // saved in the system tunnel configuration.
-                            if let username = tunnelProvider.protocolConfiguration?.username,
-                                let accountToken = accountToken, accountToken == username {
-                                self.accountToken = accountToken
-
-                                self.setTunnelProvider(tunnelProvider: tunnelProvider)
-
-                                finish(.success(()))
-                            } else {
-                                // In case of inconsistency, remove the tunnel
-                                tunnelProvider.removeFromPreferences { (error) in
-                                    self.dispatchQueue.async {
-                                        if let error = error {
-                                            finish(.failure(.removeInconsistentVPNConfiguration(error)))
-                                        } else {
-                                            self.accountToken = accountToken
-
-                                            finish(.success(()))
-                                        }
-                                    }
-                                }
-                            }
-                        } else {
-                            // No tunnels found. Save the account token.
-                            self.accountToken = accountToken
-
-                            finish(.success(()))
-                        }
+                        self.initializeManager(accountToken: accountToken, tunnels: tunnels, completionHandler: finish)
                     }
                 }
             }
@@ -767,6 +734,141 @@ class TunnelManager {
 
     // MARK: - Private methods
 
+    private func initializeManager(accountToken: String?, tunnels: [TunnelProviderManagerType]?, completionHandler: @escaping (Result<(), TunnelManager.Error>) -> Void) {
+        // Migrate the tunnel settings if needed
+        let migrationResult = accountToken.flatMap { self.migrateTunnelSettings(accountToken: $0) }
+        switch migrationResult {
+        case .success, .none:
+            break
+        case .failure(let migrationError):
+            completionHandler(.failure(migrationError))
+            return
+        }
+
+        switch (tunnels?.first, accountToken) {
+        // Case 1: tunnel exists and account token is set.
+        // Verify that tunnel can access the configuration via the persistent keychain reference
+        // stored in `passwordReference` field of VPN configuration.
+        case (.some(let tunnelProvider), .some(let accountToken)):
+            let verificationResult = self.verifyTunnel(tunnelProvider: tunnelProvider, expectedAccountToken: accountToken)
+            let tunnelSettingsResult = TunnelSettingsManager.load(searchTerm: .accountToken(accountToken)).mapError { (error) -> Error in
+                return .readTunnelSettings(error)
+            }
+
+            switch (verificationResult, tunnelSettingsResult) {
+            case (.success(true), .success(let keychainEntry)):
+                self.accountToken = accountToken
+                self.publicKeyWithMetadata = keychainEntry.tunnelSettings.interface.privateKey.publicKeyWithMetadata
+                self.setTunnelProvider(tunnelProvider: tunnelProvider)
+
+                completionHandler(.success(()))
+
+            // Remove the tunnel when failed to verify it but successfuly loaded the tunnel
+            // settings.
+            case (.failure(let verificationError), .success(let keychainEntry)):
+                self.logger.error(chainedError: verificationError, message: "Failed to verify the tunnel but successfully loaded the tunnel settings. Removing the tunnel.")
+
+                // Identical code path as the case below.
+                fallthrough
+
+            // Remove the tunnel with corrupt configuration.
+            // It will be re-created upon the first attempt to connect the tunnel.
+            case (.success(false), .success(let keychainEntry)):
+                tunnelProvider.removeFromPreferences { (error) in
+                    self.dispatchQueue.async {
+                        if let error = error {
+                            completionHandler(.failure(.removeInconsistentVPNConfiguration(error)))
+                        } else {
+                            self.accountToken = accountToken
+                            self.publicKeyWithMetadata = keychainEntry.tunnelSettings.interface.privateKey.publicKeyWithMetadata
+
+                            completionHandler(.success(()))
+                        }
+                    }
+                }
+
+            // Remove the tunnel when failed to verify the tunnel and load tunnel settings.
+            case (.failure(let verificationError), .failure(_)):
+                self.logger.error(chainedError: verificationError, message: "Failed to verify the tunnel and load tunnel settings. Removing the tunnel.")
+
+                tunnelProvider.removeFromPreferences { (error) in
+                    if let error = error {
+                        completionHandler(.failure(.removeInconsistentVPNConfiguration(error)))
+                    } else {
+                        completionHandler(.failure(verificationError))
+                    }
+                }
+
+            // Remove the tunnel when the app is not able to read tunnel settings
+            case (.success(_), .failure(let settingsReadError)):
+                self.logger.error(chainedError: settingsReadError, message: "Failed to load tunnel settings. Removing the tunnel.")
+
+                tunnelProvider.removeFromPreferences { (error) in
+                    if let error = error {
+                        completionHandler(.failure(.removeInconsistentVPNConfiguration(error)))
+                    } else {
+                        completionHandler(.failure(settingsReadError))
+                    }
+                }
+            }
+
+        // Case 2: tunnel exists but account token is unset.
+        // Remove the orphaned tunnel.
+        case (.some(let tunnelProvider), .none):
+            tunnelProvider.removeFromPreferences { (error) in
+                self.dispatchQueue.async {
+                    if let error = error {
+                        completionHandler(.failure(.removeInconsistentVPNConfiguration(error)))
+                    } else {
+                        completionHandler(.success(()))
+                    }
+                }
+            }
+
+        // Case 3: tunnel does not exist but the account token is set.
+        // Verify that tunnel settings exists in keychain.
+        case (.none, .some(let accountToken)):
+            switch TunnelSettingsManager.load(searchTerm: .accountToken(accountToken)) {
+            case .success(let keychainEntry):
+                self.accountToken = accountToken
+                self.publicKeyWithMetadata = keychainEntry.tunnelSettings.interface.privateKey.publicKeyWithMetadata
+
+                completionHandler(.success(()))
+
+            case .failure(let error):
+                completionHandler(.failure(.readTunnelSettings(error)))
+            }
+
+        // Case 4: no tunnels exist and account token is unset.
+        case (.none, .none):
+            completionHandler(.success(()))
+        }
+    }
+
+    private func verifyTunnel(tunnelProvider: TunnelProviderManagerType, expectedAccountToken accountToken: String) -> Result<Bool, Error> {
+        // Check that the VPN configuration points to the same account token
+        guard let username = tunnelProvider.protocolConfiguration?.username, username == accountToken else {
+            logger.warning("The token assigned to the VPN configuration does not match the logged in account.")
+            return .success(false)
+        }
+
+        // Check that the passwordReference, containing the keychain reference for tunnel
+        // configuration, is set.
+        guard let keychainReference = tunnelProvider.protocolConfiguration?.passwordReference else {
+            logger.warning("VPN configuration is missing the passwordReference.")
+            return .success(false)
+        }
+
+        // Verify that the keychain reference points to the existing entry in Keychain.
+        // Bad reference is possible when migrating the user data from one device to the other.
+        return TunnelSettingsManager.exists(searchTerm: .persistentReference(keychainReference))
+            .mapError { (error) -> Error in
+                logger.error(chainedError: error, message: "Failed to verify the persistent keychain reference for tunnel settings.")
+
+                return Error.readTunnelSettings(error)
+            }
+    }
+
     /// Set the instance of the active tunnel and add the tunnel status observer
     private func setTunnelProvider(tunnelProvider: TunnelProviderManagerType) {
         guard self.tunnelProvider != tunnelProvider else {
@@ -1082,21 +1184,26 @@ class TunnelManager {
         }
     }
 
-    private func migrateTunnelSettings(accountToken: String) {
+    private func migrateTunnelSettings(accountToken: String) -> Result<Bool, Error> {
         let result = TunnelSettingsManager
             .migrateKeychainEntry(searchTerm: .accountToken(accountToken))
+            .mapError { (error) -> Error in
+                return .migrateTunnelSettings(error)
+            }
 
         switch result {
         case .success(let migrated):
             if migrated {
-                self.logger.info("Migrated Keychain tunnel configuration")
+                self.logger.info("Migrated Keychain tunnel configuration.")
             } else {
                 self.logger.info("Tunnel settings are up to date. No migration needed.")
             }
 
         case .failure(let error):
-            self.logger.error(chainedError: error, message: "Failed to migrate tunnel settings")
+            self.logger.error(chainedError: error)
         }
+
+        return result
     }
 
 }
diff --git a/ios/MullvadVPN/TunnelSettingsManager.swift b/ios/MullvadVPN/TunnelSettingsManager.swift
index c30440324a..aaf5453f1b 100644
--- a/ios/MullvadVPN/TunnelSettingsManager.swift
+++ b/ios/MullvadVPN/TunnelSettingsManager.swift
@@ -245,6 +245,24 @@ extension TunnelSettingsManager {
         }
     }
 
+    /// Verify that the keychain entry exists.
+    /// Returns an error in case of failure to access Keychain.
+    static func exists(searchTerm: KeychainSearchTerm) -> Result<Bool> {
+        let query = searchTerm.makeKeychainAttributes()
+
+        return Keychain.findFirst(query: query)
+            .map({ (attributes) -> Bool in
+                return true
+            })
+            .flatMapError({ (error) -> Result<Bool> in
+                if case .itemNotFound = error {
+                    return .success(false)
+                } else {
+                    return .failure(.lookupEntry(error))
+                }
+            })
+    }
+
     private static func encode(tunnelConfig: TunnelSettings) -> Result<Data> {
         return Swift.Result { try JSONEncoder().encode(tunnelConfig) }
             .mapError { .encode($0) }