Skip to content

Commit 525aaa0

Browse files
committed
Merge branch 'revert-excluding-keychains-from-backups-ios-1007'
2 parents 78e9389 + 6248972 commit 525aaa0

File tree

8 files changed

+10
-158
lines changed

8 files changed

+10
-158
lines changed

.github/workflows/git-commit-message-style.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,4 @@ jobs:
3434
# This action defaults to 50 char subjects, but 72 is fine.
3535
max-subject-line-length: '72'
3636
# The action's wordlist is a bit short. Add more accepted verbs
37-
additional-verbs: 'tidy, wrap, obfuscate, bias, prohibit, forbid'
37+
additional-verbs: 'tidy, wrap, obfuscate, bias, prohibit, forbid, revert'

ios/MullvadSettings/KeychainSettingsStore.swift

+7-70
Original file line numberDiff line numberDiff line change
@@ -7,68 +7,33 @@
77
//
88

99
import Foundation
10-
import MullvadLogging
1110
import MullvadTypes
1211
import Security
1312

1413
public class KeychainSettingsStore: SettingsStore {
1514
public let serviceName: String
1615
public let accessGroup: String
17-
private let logger = Logger(label: "KeychainSettingsStore")
18-
private let cacheDirectory: URL
1916

20-
public init(serviceName: String, accessGroup: String, cacheDirectory: URL) {
17+
public init(serviceName: String, accessGroup: String) {
2118
self.serviceName = serviceName
2219
self.accessGroup = accessGroup
23-
self.cacheDirectory = cacheDirectory.appendingPathComponent("keychainLock.json")
2420
}
2521

2622
public func read(key: SettingsKey) throws -> Data {
27-
try coordinate(Data(), try readItemData(key))
23+
try readItemData(key)
2824
}
2925

3026
public func write(_ data: Data, for key: SettingsKey) throws {
31-
try coordinate((), try addOrUpdateItem(key, data: data))
27+
try addOrUpdateItem(key, data: data)
3228
}
3329

3430
public func delete(key: SettingsKey) throws {
35-
try coordinate((), try deleteItem(key))
36-
}
37-
38-
/// Prevents all items in `keys` from backup inclusion
39-
///
40-
/// This method uses the `coordinate` helper function to guarantee atomicity
41-
/// of the keychain exclusion process so that a pre-running VPN process cannot
42-
/// accidentally read or write to the keychain when the exclusion happens.
43-
/// It will be blocked temporarily and automatically resume when the migration is done.
44-
///
45-
/// Likewise, the exclusion process will also be forced to wait until it can access the keychain
46-
/// if the VPN process is operating on it.
47-
///
48-
/// - Important: Do not call `read`, `write`, or `delete` from this method,
49-
/// the coordinator is *not reentrant* and will deadlock if you do so.
50-
/// Only call methods that do not call `coordinate`.
51-
///
52-
/// - Parameter keys: The keys to exclude from backup
53-
public func excludeFromBackup(keys: [SettingsKey]) {
54-
let coordination = { [unowned self] in
55-
for key in keys {
56-
do {
57-
let data = try readItemData(key)
58-
try deleteItem(key)
59-
try addItem(key, data: data)
60-
} catch {
61-
logger.error("Could not exclude \(key) from backups. \(error)")
62-
}
63-
}
64-
}
65-
66-
try? coordinate((), coordination())
31+
try deleteItem(key)
6732
}
6833

6934
private func addItem(_ item: SettingsKey, data: Data) throws {
7035
var query = createDefaultAttributes(item: item)
71-
query.merge(createAccessAttributesThisDeviceOnly()) { current, _ in
36+
query.merge(createAccessAttributes()) { current, _ in
7237
current
7338
}
7439
query[kSecValueData] = data
@@ -131,38 +96,10 @@ public class KeychainSettingsStore: SettingsStore {
13196
]
13297
}
13398

134-
private func createAccessAttributesThisDeviceOnly() -> [CFString: Any] {
99+
private func createAccessAttributes() -> [CFString: Any] {
135100
[
136101
kSecAttrAccessGroup: accessGroup,
137-
kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,
102+
kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock,
138103
]
139104
}
140-
141-
/// Runs `action` in a cross process synchronized way
142-
///
143-
/// This enables doing CRUD operations on the keychain items in a cross process safe way.
144-
/// This does not prevent TOCTOU issues.
145-
/// - Parameters:
146-
/// - initial: Dummy value used for the returned value, if any.
147-
/// - action: The CRUD operation to run on the keychain.
148-
/// - Returns: The result of the keychain operation, if any.
149-
private func coordinate<T>(_ initial: T, _ action: @autoclosure () throws -> T) throws -> T {
150-
let fileCoordinator = NSFileCoordinator(filePresenter: nil)
151-
var error: NSError?
152-
var thrownError: Error?
153-
var returnedValue: T = initial
154-
fileCoordinator.coordinate(writingItemAt: cacheDirectory, error: &error) { _ in
155-
do {
156-
returnedValue = try action()
157-
} catch {
158-
thrownError = error
159-
}
160-
}
161-
162-
if let thrownError {
163-
throw thrownError
164-
}
165-
166-
return returnedValue
167-
}
168105
}

ios/MullvadSettings/KeychainSettingsStoreMigration.swift

-63
This file was deleted.

ios/MullvadSettings/SettingsManager.swift

+2-14
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@ public enum SettingsManager {
2020
#if DEBUG
2121
private static var _store = KeychainSettingsStore(
2222
serviceName: keychainServiceName,
23-
accessGroup: ApplicationConfiguration.securityGroupIdentifier,
24-
cacheDirectory: ApplicationConfiguration.containerURL
23+
accessGroup: ApplicationConfiguration.securityGroupIdentifier
2524
)
2625

2726
/// Alternative store used for tests.
@@ -35,8 +34,7 @@ public enum SettingsManager {
3534
#else
3635
public static let store: SettingsStore = KeychainSettingsStore(
3736
serviceName: keychainServiceName,
38-
accessGroup: ApplicationConfiguration.securityGroupIdentifier,
39-
cacheDirectory: ApplicationConfiguration.containerURL
37+
accessGroup: ApplicationConfiguration.securityGroupIdentifier
4038
)
4139

4240
#endif
@@ -165,16 +163,6 @@ public enum SettingsManager {
165163
}
166164
}
167165

168-
public static func excludeKeychainSettingsFromBackups() {
169-
let migration = KeychainSettingsStoreMigration(
170-
serviceName: keychainServiceName,
171-
accessGroup: ApplicationConfiguration.securityGroupIdentifier,
172-
store: store
173-
)
174-
175-
migration.excludeKeychainSettingsFromBackups()
176-
}
177-
178166
// MARK: - Private
179167

180168
private static func checkLatestSettingsVersion() throws {

ios/MullvadSettings/SettingsStore.swift

-1
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,4 @@ public protocol SettingsStore {
2222
func read(key: SettingsKey) throws -> Data
2323
func write(_ data: Data, for key: SettingsKey) throws
2424
func delete(key: SettingsKey) throws
25-
func excludeFromBackup(keys: [SettingsKey])
2625
}

ios/MullvadVPN.xcodeproj/project.pbxproj

-4
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,6 @@
751751
A93969812CE606190032A7A0 /* Maybenot.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9840BB32C69F78A0030F05E /* Maybenot.swift */; };
752752
A94D691A2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E22AA72AE9003D1918 /* WireGuardKitTypes */; };
753753
A94D691B2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E72AA7399D003D1918 /* WireGuardKitTypes */; };
754-
A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */ = {isa = PBXBuildFile; fileRef = A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */; };
755754
A95EEE362B722CD600A8A39B /* TunnelMonitorState.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */; };
756755
A95EEE382B722DFC00A8A39B /* PingStats.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE372B722DFC00A8A39B /* PingStats.swift */; };
757756
A970C89D2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift in Sources */ = {isa = PBXBuildFile; fileRef = A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */; };
@@ -2144,7 +2143,6 @@
21442143
A944F2712B8E02E800473F4C /* libmullvad_ios.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; name = libmullvad_ios.a; path = "../target/aarch64-apple-ios/debug/libmullvad_ios.a"; sourceTree = "<group>"; };
21452144
A9467E7E2A29DEFE000DC21F /* RelayCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayCacheTests.swift; sourceTree = "<group>"; };
21462145
A948809A2BC9308D0090A44C /* EphemeralPeerExchangeActor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EphemeralPeerExchangeActor.swift; sourceTree = "<group>"; };
2147-
A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeychainSettingsStoreMigration.swift; sourceTree = "<group>"; };
21482146
A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TunnelMonitorState.swift; sourceTree = "<group>"; };
21492147
A95EEE372B722DFC00A8A39B /* PingStats.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PingStats.swift; sourceTree = "<group>"; };
21502148
A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Socks5UsernamePasswordCommand.swift; sourceTree = "<group>"; };
@@ -3504,7 +3502,6 @@
35043502
7A5869B22B5697AC00640D27 /* IPOverride.swift */,
35053503
7A5869BA2B56EE9500640D27 /* IPOverrideRepository.swift */,
35063504
06410DFD292CE18F00AFC18C /* KeychainSettingsStore.swift */,
3507-
A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */,
35083505
068CE5732927B7A400A068BB /* Migration.swift */,
35093506
A9D96B192A8247C100A5C673 /* MigrationManager.swift */,
35103507
58B2FDD52AA71D2A003EB5C6 /* MullvadSettings.h */,
@@ -5773,7 +5770,6 @@
57735770
58B2FDE72AA71D5C003EB5C6 /* SettingsStore.swift in Sources */,
57745771
44DD7D2D2B74E44A0005F67F /* QuantumResistanceSettings.swift in Sources */,
57755772
F08827872B318C840020A383 /* ShadowsocksCipherOptions.swift in Sources */,
5776-
A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */,
57775773
58B2FDE92AA71D5C003EB5C6 /* SettingsParser.swift in Sources */,
57785774
F08827892B3192110020A383 /* AccessMethodRepositoryProtocol.swift in Sources */,
57795775
F050AE5A2B7376F4003F4EDB /* CustomList.swift in Sources */,

ios/MullvadVPN/AppDelegate.swift

-3
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD
163163

164164
startInitialization(application: application)
165165

166-
// Wait for `getWipeSettingsOperation` to have run before excluding keychain from backups
167-
SettingsManager.excludeKeychainSettingsFromBackups()
168-
169166
return true
170167
}
171168

ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift

-2
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,4 @@ class InMemorySettingsStore<ThrownError: Error>: SettingsStore where ThrownError
2828
func delete(key: SettingsKey) throws {
2929
settings.removeValue(forKey: key)
3030
}
31-
32-
func excludeFromBackup(keys: [MullvadSettings.SettingsKey]) {}
3331
}

0 commit comments

Comments
 (0)