Merge branch 'revert-excluding-keychains-from-backups-ios-1007'

8 files changed, 10 insertions, 158 deletions

diff --git a/.github/workflows/git-commit-message-style.yml b/.github/workflows/git-commit-message-style.yml
index 850a563c81..622c2c093f 100644
--- a/.github/workflows/git-commit-message-style.yml
+++ b/.github/workflows/git-commit-message-style.yml
@@ -34,4 +34,4 @@ jobs:
           # This action defaults to 50 char subjects, but 72 is fine.
           max-subject-line-length: '72'
           # The action's wordlist is a bit short. Add more accepted verbs
-          additional-verbs: 'tidy, wrap, obfuscate, bias, prohibit, forbid'
+          additional-verbs: 'tidy, wrap, obfuscate, bias, prohibit, forbid, revert'
diff --git a/ios/MullvadSettings/KeychainSettingsStore.swift b/ios/MullvadSettings/KeychainSettingsStore.swift
index d9a058cbcd..ba0612de0d 100644
--- a/ios/MullvadSettings/KeychainSettingsStore.swift
+++ b/ios/MullvadSettings/KeychainSettingsStore.swift
@@ -7,68 +7,33 @@
 //
 
 import Foundation
-import MullvadLogging
 import MullvadTypes
 import Security
 
 public class KeychainSettingsStore: SettingsStore {
     public let serviceName: String
     public let accessGroup: String
-    private let logger = Logger(label: "KeychainSettingsStore")
-    private let cacheDirectory: URL
 
-    public init(serviceName: String, accessGroup: String, cacheDirectory: URL) {
+    public init(serviceName: String, accessGroup: String) {
         self.serviceName = serviceName
         self.accessGroup = accessGroup
-        self.cacheDirectory = cacheDirectory.appendingPathComponent("keychainLock.json")
     }
 
     public func read(key: SettingsKey) throws -> Data {
-        try coordinate(Data(), try readItemData(key))
+        try readItemData(key)
     }
 
     public func write(_ data: Data, for key: SettingsKey) throws {
-        try coordinate((), try addOrUpdateItem(key, data: data))
+        try addOrUpdateItem(key, data: data)
     }
 
     public func delete(key: SettingsKey) throws {
-        try coordinate((), try deleteItem(key))
-    }
-
-    /// Prevents all items in `keys` from backup inclusion
-    ///
-    /// This method uses the `coordinate` helper function to guarantee atomicity
-    /// of the keychain exclusion process so that a pre-running VPN process cannot
-    /// accidentally read or write to the keychain when the exclusion happens.
-    /// It will be blocked temporarily and automatically resume when the migration is done.
-    ///
-    /// Likewise, the exclusion process will also be forced to wait until it can access the keychain
-    /// if the VPN process is operating on it.
-    ///
-    /// - Important: Do not call `read`, `write`, or `delete` from this method,
-    /// the coordinator is *not reentrant* and will deadlock if you do so.
-    /// Only call methods that do not call `coordinate`.
-    ///
-    /// - Parameter keys: The keys to exclude from backup
-    public func excludeFromBackup(keys: [SettingsKey]) {
-        let coordination = { [unowned self] in
-            for key in keys {
-                do {
-                    let data = try readItemData(key)
-                    try deleteItem(key)
-                    try addItem(key, data: data)
-                } catch {
-                    logger.error("Could not exclude \(key) from backups. \(error)")
-                }
-            }
-        }
-
-        try? coordinate((), coordination())
+        try deleteItem(key)
     }
 
     private func addItem(_ item: SettingsKey, data: Data) throws {
         var query = createDefaultAttributes(item: item)
-        query.merge(createAccessAttributesThisDeviceOnly()) { current, _ in
+        query.merge(createAccessAttributes()) { current, _ in
             current
         }
         query[kSecValueData] = data
@@ -131,38 +96,10 @@ public class KeychainSettingsStore: SettingsStore {
         ]
     }
 
-    private func createAccessAttributesThisDeviceOnly() -> [CFString: Any] {
+    private func createAccessAttributes() -> [CFString: Any] {
         [
             kSecAttrAccessGroup: accessGroup,
-            kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,
+            kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock,
         ]
     }
-
-    /// Runs `action` in a cross process synchronized way
-    ///
-    /// This enables doing CRUD operations on the keychain items in a cross process safe way.
-    /// This does not prevent TOCTOU issues.
-    /// - Parameters:
-    ///   - initial: Dummy value used for the returned value, if any.
-    ///   - action: The CRUD operation to run on the keychain.
-    /// - Returns: The result of the keychain operation, if any.
-    private func coordinate<T>(_ initial: T, _ action: @autoclosure () throws -> T) throws -> T {
-        let fileCoordinator = NSFileCoordinator(filePresenter: nil)
-        var error: NSError?
-        var thrownError: Error?
-        var returnedValue: T = initial
-        fileCoordinator.coordinate(writingItemAt: cacheDirectory, error: &error) { _ in
-            do {
-                returnedValue = try action()
-            } catch {
-                thrownError = error
-            }
-        }
-
-        if let thrownError {
-            throw thrownError
-        }
-
-        return returnedValue
-    }
 }
diff --git a/ios/MullvadSettings/KeychainSettingsStoreMigration.swift b/ios/MullvadSettings/KeychainSettingsStoreMigration.swift
deleted file mode 100644
index e6dc38f33b..0000000000
--- a/ios/MullvadSettings/KeychainSettingsStoreMigration.swift
+++ /dev/null
@@ -1,63 +0,0 @@
-//
-//  KeychainSettingsStoreMigration.swift
-//  MullvadSettings
-//
-//  Created by Marco Nikic on 2025-01-07.
-//  Copyright © 2025 Mullvad VPN AB. All rights reserved.
-//
-
-import Foundation
-
-public struct KeychainSettingsStoreMigration {
-    private let serviceName: String
-    private let accessGroup: String
-    private let store: SettingsStore
-
-    init(serviceName: String, accessGroup: String, store: SettingsStore) {
-        self.serviceName = serviceName
-        self.accessGroup = accessGroup
-        self.store = store
-    }
-
-    /// Creates a keychain query matching old keychain settings that would be included in backups.
-    private func createQueryAttributes(item: SettingsKey) -> [CFString: Any] {
-        [
-            kSecClass: kSecClassGenericPassword,
-            kSecAttrService: serviceName,
-            kSecAttrAccount: item.rawValue,
-            kSecAttrAccessGroup: accessGroup,
-            kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock,
-        ]
-    }
-
-    /// Whether items saved in the keychain should be migrated to be excluded from backups.
-    ///
-    /// Create a keychain item query using the old value `kSecAttrAccessibleAfterFirstUnlock`
-    /// for keychain items accessibility.
-    ///
-    /// If there is a match, probe whether it's a first time launch by querying whether the `SettingKey`
-    /// `shouldWipeSettings` has already been set.
-    ///
-    /// If both the query is successful, and `shouldWipeSettings` has already been set,
-    /// this means the keychain saved settings accessibility need to be upgraded to `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly`.
-    ///
-    /// This will be done automatically by deleting and re-adding entries in the keychain.
-    /// - Returns: Whether keychain settings should be deleted and added again
-    private func shouldExcludeSettingsFromBackup() -> Bool {
-        var query = createQueryAttributes(item: .shouldWipeSettings)
-        query[kSecReturnData] = true
-
-        var result: CFTypeRef?
-        let status = SecItemCopyMatching(query as CFDictionary, &result)
-        let shouldWipeSettingsWasSet = (try? store.read(key: .shouldWipeSettings)) != nil
-
-        return status == errSecSuccess && shouldWipeSettingsWasSet
-    }
-
-    public func excludeKeychainSettingsFromBackups() {
-        guard shouldExcludeSettingsFromBackup() == true else { return }
-        store.excludeFromBackup(keys: SettingsKey.allCases)
-
-        precondition(shouldExcludeSettingsFromBackup() == false)
-    }
-}
diff --git a/ios/MullvadSettings/SettingsManager.swift b/ios/MullvadSettings/SettingsManager.swift
index cf3a9a8a0e..d414d7ed0a 100644
--- a/ios/MullvadSettings/SettingsManager.swift
+++ b/ios/MullvadSettings/SettingsManager.swift
@@ -20,8 +20,7 @@ public enum SettingsManager {
     #if DEBUG
     private static var _store = KeychainSettingsStore(
         serviceName: keychainServiceName,
-        accessGroup: ApplicationConfiguration.securityGroupIdentifier,
-        cacheDirectory: ApplicationConfiguration.containerURL
+        accessGroup: ApplicationConfiguration.securityGroupIdentifier
     )
 
     /// Alternative store used for tests.
@@ -35,8 +34,7 @@ public enum SettingsManager {
     #else
     public static let store: SettingsStore = KeychainSettingsStore(
         serviceName: keychainServiceName,
-        accessGroup: ApplicationConfiguration.securityGroupIdentifier,
-        cacheDirectory: ApplicationConfiguration.containerURL
+        accessGroup: ApplicationConfiguration.securityGroupIdentifier
     )
 
     #endif
@@ -165,16 +163,6 @@ public enum SettingsManager {
         }
     }
 
-    public static func excludeKeychainSettingsFromBackups() {
-        let migration = KeychainSettingsStoreMigration(
-            serviceName: keychainServiceName,
-            accessGroup: ApplicationConfiguration.securityGroupIdentifier,
-            store: store
-        )
-
-        migration.excludeKeychainSettingsFromBackups()
-    }
-
     // MARK: - Private
 
     private static func checkLatestSettingsVersion() throws {
diff --git a/ios/MullvadSettings/SettingsStore.swift b/ios/MullvadSettings/SettingsStore.swift
index 2e908d008b..2901ae2bb3 100644
--- a/ios/MullvadSettings/SettingsStore.swift
+++ b/ios/MullvadSettings/SettingsStore.swift
@@ -22,5 +22,4 @@ public protocol SettingsStore {
     func read(key: SettingsKey) throws -> Data
     func write(_ data: Data, for key: SettingsKey) throws
     func delete(key: SettingsKey) throws
-    func excludeFromBackup(keys: [SettingsKey])
 }
diff --git a/ios/MullvadVPN.xcodeproj/project.pbxproj b/ios/MullvadVPN.xcodeproj/project.pbxproj
index e1cda675d7..e15a6de7d5 100644
--- a/ios/MullvadVPN.xcodeproj/project.pbxproj
+++ b/ios/MullvadVPN.xcodeproj/project.pbxproj
@@ -751,7 +751,6 @@
 		A93969812CE606190032A7A0 /* Maybenot.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9840BB32C69F78A0030F05E /* Maybenot.swift */; };
 		A94D691A2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E22AA72AE9003D1918 /* WireGuardKitTypes */; };
 		A94D691B2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E72AA7399D003D1918 /* WireGuardKitTypes */; };
-		A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */ = {isa = PBXBuildFile; fileRef = A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */; };
 		A95EEE362B722CD600A8A39B /* TunnelMonitorState.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */; };
 		A95EEE382B722DFC00A8A39B /* PingStats.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE372B722DFC00A8A39B /* PingStats.swift */; };
 		A970C89D2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift in Sources */ = {isa = PBXBuildFile; fileRef = A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */; };
@@ -2144,7 +2143,6 @@
 		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>"; };
-		A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeychainSettingsStoreMigration.swift; sourceTree = "<group>"; };
 		A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TunnelMonitorState.swift; sourceTree = "<group>"; };
 		A95EEE372B722DFC00A8A39B /* PingStats.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PingStats.swift; sourceTree = "<group>"; };
 		A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Socks5UsernamePasswordCommand.swift; sourceTree = "<group>"; };
@@ -3504,7 +3502,6 @@
 				7A5869B22B5697AC00640D27 /* IPOverride.swift */,
 				7A5869BA2B56EE9500640D27 /* IPOverrideRepository.swift */,
 				06410DFD292CE18F00AFC18C /* KeychainSettingsStore.swift */,
-				A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */,
 				068CE5732927B7A400A068BB /* Migration.swift */,
 				A9D96B192A8247C100A5C673 /* MigrationManager.swift */,
 				58B2FDD52AA71D2A003EB5C6 /* MullvadSettings.h */,
@@ -5773,7 +5770,6 @@
 				58B2FDE72AA71D5C003EB5C6 /* SettingsStore.swift in Sources */,
 				44DD7D2D2B74E44A0005F67F /* QuantumResistanceSettings.swift in Sources */,
 				F08827872B318C840020A383 /* ShadowsocksCipherOptions.swift in Sources */,
-				A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */,
 				58B2FDE92AA71D5C003EB5C6 /* SettingsParser.swift in Sources */,
 				F08827892B3192110020A383 /* AccessMethodRepositoryProtocol.swift in Sources */,
 				F050AE5A2B7376F4003F4EDB /* CustomList.swift in Sources */,
diff --git a/ios/MullvadVPN/AppDelegate.swift b/ios/MullvadVPN/AppDelegate.swift
index 92fac4ab5f..2f78e90476 100644
--- a/ios/MullvadVPN/AppDelegate.swift
+++ b/ios/MullvadVPN/AppDelegate.swift
@@ -163,9 +163,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
 
         startInitialization(application: application)
 
-        // Wait for `getWipeSettingsOperation` to have run before excluding keychain from backups
-        SettingsManager.excludeKeychainSettingsFromBackups()
-
         return true
     }
 
diff --git a/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift b/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift
index b551689390..1f19b6429c 100644
--- a/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift
+++ b/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift
@@ -28,6 +28,4 @@ class InMemorySettingsStore<ThrownError: Error>: SettingsStore where ThrownError
     func delete(key: SettingsKey) throws {
         settings.removeValue(forKey: key)
     }
-
-    func excludeFromBackup(keys: [MullvadSettings.SettingsKey]) {}
 }