Skip to content

Commit

Permalink
Merge branch 'revert-excluding-keychains-from-backups-ios-1007'
Browse files Browse the repository at this point in the history
  • Loading branch information
@buggmagnet
buggmagnet committed Jan 13, 2025
2 parents 78e9389 + 6248972 commit 525aaa0
Show file tree
Hide file tree
Showing 8 changed files with 10 additions and 158 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/git-commit-message-style.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
77 changes: 7 additions & 70 deletions ios/MullvadSettings/KeychainSettingsStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
63 changes: 0 additions & 63 deletions ios/MullvadSettings/KeychainSettingsStoreMigration.swift
View file

This file was deleted.

16 changes: 2 additions & 14 deletions ios/MullvadSettings/SettingsManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion ios/MullvadSettings/SettingsStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
4 changes: 0 additions & 4 deletions ios/MullvadVPN.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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>"; };
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -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 */,
Expand Down
3 changes: 0 additions & 3 deletions ios/MullvadVPN/AppDelegate.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 0 additions & 2 deletions ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {}
}

0 comments on commit 525aaa0

Please sign in to comment.