Merge branch 'do-settings-migration-from-the-packet-tunnel-ios-573'

Author: buggmagnet

Diff for: ios/MullvadREST/RetryStrategy/RetryStrategy.swift

@@ -78,6 +78,16 @@ extension REST {
             ),
             applyJitter: true
         )
+
+        public static var failedMigrationRecovery = RetryStrategy(
+            maxRetryCount: .max,
+            delay: .exponentialBackoff(
+                initial: .seconds(5),
+                multiplier: UInt64(1),
+                maxDelay: .minutes(1)
+            ),
+            applyJitter: true
+        )
     }
 
     public enum RetryDelay: Equatable {

Diff for: ios/MullvadSettings/MigrationManager.swift

@@ -23,38 +23,54 @@ public enum SettingsMigrationResult {
 
 public struct MigrationManager {
     private let logger = Logger(label: "MigrationManager")
+    private let cacheDirectory: URL
 
-    public init() {}
+    public init(cacheDirectory: URL) {
+        self.cacheDirectory = cacheDirectory.appendingPathComponent("migrationState.json")
+    }
 
     /// Migrate settings store if needed.
     ///
     /// Reads the current settings, upgrades them to the latest version if needed
     /// and writes back to `store` when settings are updated.
+    ///
+    /// In order to avoid migration happening from both the VPN and the host processes at the same time,
+    /// a non existant file path is used as a lock to synchronize access between the processes.
+    /// This file is accessed by `NSFileCoordinator` in order to prevent multiple processes accessing at the same time.
     /// - Parameters:
     ///   - store: The store to from which settings are read and written to.
-    ///   - proxyFactory: Factory used for migrations that involve API calls.
     ///   - migrationCompleted: Completion handler called with a migration result.
     public func migrateSettings(
         store: SettingsStore,
         migrationCompleted: @escaping (SettingsMigrationResult) -> Void
     ) {
-        let resetStoreHandler = { (result: SettingsMigrationResult) in
-            // Reset store upon failure to migrate settings.
-            if case .failure = result {
-                SettingsManager.resetStore()
+        let fileCoordinator = NSFileCoordinator(filePresenter: nil)
+        var error: NSError?
+
+        // This will block the calling thread if another process is currently running the same code.
+        // This is intentional to avoid TOCTOU issues, and guaranteeing settings cannot be read
+        // in a half written state.
+        // The resulting effect is that only one process at a time can do settings migrations.
+        // The other process will be blocked, and will have nothing to do as long as settings were successfully upgraded.
+        fileCoordinator.coordinate(writingItemAt: cacheDirectory, error: &error) { _ in
+            let resetStoreHandler = { (result: SettingsMigrationResult) in
+                // Reset store upon failure to migrate settings.
+                if case .failure = result {
+                    SettingsManager.resetStore()
+                }
+                migrationCompleted(result)
             }
-            migrationCompleted(result)
-        }
 
-        do {
-            try upgradeSettingsToLatestVersion(
-                store: store,
-                migrationCompleted: migrationCompleted
-            )
-        } catch .itemNotFound as KeychainError {
-            migrationCompleted(.nothing)
-        } catch {
-            resetStoreHandler(.failure(error))
+            do {
+                try upgradeSettingsToLatestVersion(
+                    store: store,
+                    migrationCompleted: migrationCompleted
+                )
+            } catch .itemNotFound as KeychainError {
+                migrationCompleted(.nothing)
+            } catch {
+                resetStoreHandler(.failure(error))
+            }
         }
     }
 

Diff for: ios/MullvadVPN.xcodeproj/project.pbxproj

@@ -696,6 +696,7 @@
 		A932D9F32B5EB61100999395 /* HeadRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A932D9F22B5EB61100999395 /* HeadRequestTests.swift */; };
 		A932D9F52B5EBB9D00999395 /* RESTTransportStub.swift in Sources */ = {isa = PBXBuildFile; fileRef = A932D9F42B5EBB9D00999395 /* RESTTransportStub.swift */; };
 		A935594C2B4C2DA900D5D524 /* APIAvailabilityTestRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = A935594B2B4C2DA900D5D524 /* APIAvailabilityTestRequest.swift */; };
+		A939661B2CAE6CE1008128CA /* MigrationManagerMultiProcessUpgradeTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A939661A2CAE6CE1008128CA /* MigrationManagerMultiProcessUpgradeTests.swift */; };
 		A94D691A2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E22AA72AE9003D1918 /* WireGuardKitTypes */; };
 		A94D691B2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E72AA7399D003D1918 /* WireGuardKitTypes */; };
 		A95EEE362B722CD600A8A39B /* TunnelMonitorState.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */; };
@@ -2020,6 +2021,7 @@
 		A932D9F42B5EBB9D00999395 /* RESTTransportStub.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RESTTransportStub.swift; sourceTree = "<group>"; };
 		A935594B2B4C2DA900D5D524 /* APIAvailabilityTestRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APIAvailabilityTestRequest.swift; sourceTree = "<group>"; };
 		A935594D2B4E919F00D5D524 /* Api.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = Api.xcconfig; sourceTree = "<group>"; };
+		A939661A2CAE6CE1008128CA /* MigrationManagerMultiProcessUpgradeTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MigrationManagerMultiProcessUpgradeTests.swift; sourceTree = "<group>"; };
 		A944F2712B8E02E800473F4C /* libmullvad_ios.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; name = libmullvad_ios.a; path = "../target/aarch64-apple-ios/debug/libmullvad_ios.a"; sourceTree = "<group>"; };
 		A9467E7E2A29DEFE000DC21F /* RelayCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayCacheTests.swift; sourceTree = "<group>"; };
 		A948809A2BC9308D0090A44C /* EphemeralPeerExchangeActor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EphemeralPeerExchangeActor.swift; sourceTree = "<group>"; };
@@ -2574,6 +2576,7 @@
 				7A5869C22B5820CE00640D27 /* IPOverrideRepositoryTests.swift */,
 				7AB4CCB82B69097E006037F5 /* IPOverrideTests.swift */,
 				7A516C3B2B712F0B00BBD33D /* IPOverrideWrapperTests.swift */,
+				A939661A2CAE6CE1008128CA /* MigrationManagerMultiProcessUpgradeTests.swift */,
 				A9B6AC172ADE8F4300F7802A /* MigrationManagerTests.swift */,
 				F072D3CE2C07122400906F64 /* SettingsUpdaterTests.swift */,
 				449872E32B7CB96300094DDC /* TunnelSettingsUpdateTests.swift */,
@@ -5377,6 +5380,7 @@
 				7A9BE5A52B90760C00E2A7D0 /* CustomListsDataSourceTests.swift in Sources */,
 				A9A5FA1B2ACB05160083449F /* Tunnel.swift in Sources */,
 				A9A5FA1C2ACB05160083449F /* Tunnel+Messaging.swift in Sources */,
+				A939661B2CAE6CE1008128CA /* MigrationManagerMultiProcessUpgradeTests.swift in Sources */,
 				7A9BE5A92B90806800E2A7D0 /* CustomListsRepositoryStub.swift in Sources */,
 				F09D04BB2AE95396003D4F89 /* URLSessionStub.swift in Sources */,
 				A9A5FA1D2ACB05160083449F /* TunnelBlockObserver.swift in Sources */,

Diff for: ios/MullvadVPN/AppDelegate.swift

@@ -42,7 +42,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
     private(set) var storePaymentManager: StorePaymentManager!
     private var transportMonitor: TransportMonitor!
     private var settingsObserver: TunnelBlockObserver!
-    private let migrationManager = MigrationManager()
+    private var migrationManager: MigrationManager!
 
     private(set) var accessMethodRepository = AccessMethodRepository()
     private(set) var shadowsocksLoader: ShadowsocksLoaderProtocol!
@@ -67,7 +67,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
         }
 
         let containerURL = ApplicationConfiguration.containerURL
-
+        migrationManager = MigrationManager(cacheDirectory: containerURL)
         configureLogging()
 
         addressCache = REST.AddressCache(canWriteToCache: true, cacheDirectory: containerURL)
@@ -478,14 +478,16 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
                     switch migrationResult {
                     case .success:
                         // Tell the tunnel to re-read tunnel configuration after migration.
-                        logger.debug("Reconnect the tunnel after settings migration.")
+                        logger.debug("Successful migration from UI Process")
                         tunnelManager.reconnectTunnel(selectNewRelay: true)
                         fallthrough
 
                     case .nothing:
+                        logger.debug("Attempted migration from UI Process, but found nothing to do")
                         finish(nil)
 
                     case let .failure(error):
+                        logger.error("Failed migration from UI Process: \(error)")
                         let migrationUIHandler = application.connectedScenes
                             .first { $0 is SettingsMigrationUIHandler } as? SettingsMigrationUIHandler
 

Diff for: ios/MullvadVPNTests/MullvadSettings/MigrationManagerMultiProcessUpgradeTests.swift

@@ -0,0 +1,58 @@
+//
+//  MigrationManagerMultiProcessUpgradeTests.swift
+//  MullvadVPNTests
+//
+//  Created by Marco Nikic on 2024-10-03.
+//  Copyright © 2024 Mullvad VPN AB. All rights reserved.
+//
+
+@testable import MullvadMockData
+@testable import MullvadREST
+@testable import MullvadSettings
+@testable import MullvadTypes
+import XCTest
+
+extension MigrationManagerTests {
+    func testMigrationDoesNothingIfAnotherProcessIsRunningUpdates() throws {
+        let hostProcess = DispatchQueue(label: "net.tests.HostMigration")
+        let packetTunnelProcess = DispatchQueue(label: "net.tests.PacketTunnelMigration")
+        let osakaRelayConstraints = RelayConstraints(
+            exitLocations: .only(UserSelectedRelays(locations: [.city("jp", "osa")]))
+        )
+        var settingsV1 = TunnelSettingsV1()
+        settingsV1.relayConstraints = osakaRelayConstraints
+
+        try write(settings: settingsV1, version: SchemaVersion.v1.rawValue, in: Self.store)
+
+        let backgroundMigrationExpectation = expectation(description: "Migration from packet tunnel")
+        let foregroundMigrationExpectation = expectation(description: "Migration from host")
+        var migrationHappenedInPacketTunnel = false
+        var migrationHappenedInHost = false
+
+        packetTunnelProcess.async { [unowned self] in
+            manager.migrateSettings(store: MigrationManagerTests.store) { backgroundMigrationResult in
+                if case .success = backgroundMigrationResult {
+                    migrationHappenedInPacketTunnel = true
+                }
+                backgroundMigrationExpectation.fulfill()
+            }
+        }
+
+        hostProcess.async { [unowned self] in
+            manager.migrateSettings(store: MigrationManagerTests.store) { foregroundMigrationResult in
+                if case .success = foregroundMigrationResult {
+                    migrationHappenedInHost = true
+                }
+                foregroundMigrationExpectation.fulfill()
+            }
+        }
+
+        wait(for: [backgroundMigrationExpectation, foregroundMigrationExpectation], timeout: .UnitTest.invertedTimeout)
+
+        // Migration happens either in one process, or the other.
+        // This check guarantees it didn't happen in both simultaneously.
+        XCTAssertNotEqual(migrationHappenedInPacketTunnel, migrationHappenedInHost)
+        let latestSettings = try SettingsManager.readSettings()
+        XCTAssertEqual(osakaRelayConstraints, latestSettings.relayConstraints)
+    }
+}

Diff for: ios/MullvadVPNTests/MullvadSettings/MigrationManagerTests.swift

@@ -16,6 +16,7 @@ final class MigrationManagerTests: XCTestCase {
     static let store = InMemorySettingsStore<SettingNotFound>()
 
     var manager: MigrationManager!
+    var testFileURL: URL!
     override static func setUp() {
         SettingsManager.unitTestStore = store
     }
@@ -25,7 +26,14 @@ final class MigrationManagerTests: XCTestCase {
     }
 
     override func setUpWithError() throws {
-        manager = MigrationManager()
+        testFileURL = FileManager.default.temporaryDirectory
+            .appendingPathComponent("MigrationManagerTests-\(UUID().uuidString)", isDirectory: true)
+        try FileManager.default.createDirectory(at: testFileURL, withIntermediateDirectories: true)
+        manager = MigrationManager(cacheDirectory: testFileURL)
+    }
+
+    override func tearDownWithError() throws {
+        try FileManager.default.removeItem(at: testFileURL)
     }
 
     func testNothingToMigrate() throws {
@@ -224,7 +232,7 @@ final class MigrationManagerTests: XCTestCase {
         wait(for: [successfulMigrationExpectation], timeout: .UnitTest.timeout)
     }
 
-    private func write(settings: any TunnelSettings, version: Int, in store: SettingsStore) throws {
+    func write(settings: any TunnelSettings, version: Int, in store: SettingsStore) throws {
         let parser = SettingsParser(decoder: JSONDecoder(), encoder: JSONEncoder())
         let payload = try parser.producePayload(settings, version: version)
         try store.write(payload, for: .settings)

Diff for: ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift

@@ -15,7 +15,7 @@ class FileCacheTests: XCTestCase {
 
     override func setUp() {
         testFileURL = FileManager.default.temporaryDirectory
-            .appendingPathComponent("FileCacheTest-\(UUID().uuidString)", isDirectory: true)
+            .appendingPathComponent("FileCacheTest-\(UUID().uuidString)", isDirectory: false)
     }
 
     override func tearDown() {

Diff for: ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift

@@ -29,6 +29,8 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
     private var ephemeralPeerExchangingPipeline: EphemeralPeerExchangingPipeline!
     private let tunnelSettingsUpdater: SettingsUpdater!
     private var encryptedDNSTransport: EncryptedDNSTransport!
+    private var migrationManager: MigrationManager!
+    let migrationFailureIterator = REST.RetryStrategy.failedMigrationRecovery.makeDelayIterator()
 
     private let tunnelSettingsListener = TunnelSettingsListener()
     private lazy var ephemeralPeerReceiver = {
@@ -49,9 +51,12 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
             ipOverrideRepository: IPOverrideRepository()
         )
         tunnelSettingsUpdater = SettingsUpdater(listener: tunnelSettingsListener)
+        migrationManager = MigrationManager(cacheDirectory: containerURL)
 
         super.init()
 
+        performSettingsMigration()
+
         let transportProvider = setUpTransportProvider(
             appContainerURL: containerURL,
             ipOverrideWrapper: ipOverrideWrapper,
@@ -168,6 +173,31 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
         actor.onWake()
     }
 
+    private func performSettingsMigration() {
+        migrationManager.migrateSettings(
+            store: SettingsManager.store,
+            migrationCompleted: { [unowned self] migrationResult in
+                switch migrationResult {
+                case .success:
+                    providerLogger.debug("Successful migration from PacketTunnel")
+                case .nothing:
+                    providerLogger.debug("Attempted migration from PacketTunnel, but found nothing to do")
+                case let .failure(error):
+                    // `next` returns an Optional value, but this iterator is guaranteed to always have a next value
+                    guard let delay = migrationFailureIterator.next() else { return }
+                    providerLogger
+                        .error(
+                            "Failed migration from PacketTunnel: \(error), retrying in \(delay.timeInterval) seconds"
+                        )
+                    // Block the launch of the Packet Tunnel for as long as the settings migration fail.
+                    // The process watchdog introduced by iOS 17 will kill this process after 60 seconds.
+                    Thread.sleep(forTimeInterval: delay.timeInterval)
+                    performSettingsMigration()
+                }
+            }
+        )
+    }
+
     private func setUpTransportProvider(
         appContainerURL: URL,
         ipOverrideWrapper: IPOverrideWrapper,