Merge branch 'remove-keychain-revision'

9 files changed, 43 insertions, 144 deletions

diff --git a/ios/MullvadVPN.xcodeproj/project.pbxproj b/ios/MullvadVPN.xcodeproj/project.pbxproj
index 5f81d89189..ce72f88bb4 100644
--- a/ios/MullvadVPN.xcodeproj/project.pbxproj
+++ b/ios/MullvadVPN.xcodeproj/project.pbxproj
@@ -731,13 +731,7 @@
 				5840250022B1124600E4CFEC /* IPAddress+Codable.swift */,
 				5850366725A47AC700A43E93 /* IPAddressRange+Codable.swift */,
 				58561C98239A5D1500BD6B5E /* IPEndpoint.swift */,
-				58FAEDF6245088E100CB0F5B /* Keychain.swift */,
-				58FAEDEB245059F000CB0F5B /* KeychainAttributes.swift */,
-				58FAEE0024533A9C00CB0F5B /* KeychainClass.swift */,
-				58AEEF642344A36000C9BBD5 /* KeychainError.swift */,
-				58F840AE2464382C0044E708 /* KeychainItemRevision.swift */,
-				58FAEDFC24533A5500CB0F5B /* KeychainMatchLimit.swift */,
-				58FAEDFE24533A7000CB0F5B /* KeychainReturn.swift */,
+				58FB865626E8C06800F188BC /* Keychain */,
 				58727282265D173C00F315B2 /* LaunchScreen.storyboard */,
 				58A1AA8623F43901009F7EA6 /* Location.swift */,
 				583DA21325FA4B5C00318683 /* LocationDataSource.swift */,
@@ -857,6 +851,19 @@
 			path = Assets;
 			sourceTree = "<group>";
 		};
+		58FB865626E8C06800F188BC /* Keychain */ = {
+			isa = PBXGroup;
+			children = (
+				58FAEDF6245088E100CB0F5B /* Keychain.swift */,
+				58FAEDEB245059F000CB0F5B /* KeychainAttributes.swift */,
+				58FAEE0024533A9C00CB0F5B /* KeychainClass.swift */,
+				58AEEF642344A36000C9BBD5 /* KeychainError.swift */,
+				58FAEDFC24533A5500CB0F5B /* KeychainMatchLimit.swift */,
+				58FAEDFE24533A7000CB0F5B /* KeychainReturn.swift */,
+			);
+			path = Keychain;
+			sourceTree = "<group>";
+		};
 /* End PBXGroup section */
 
 /* Begin PBXLegacyTarget section */
@@ -1298,6 +1305,7 @@
 				58B93A1826C54D7E00A55733 /* Locking.swift in Sources */,
 				58FAEE0224533ABB00CB0F5B /* KeychainMatchLimit.swift in Sources */,
 				5860392A26DCE7AB00554C79 /* PromiseCompletion.swift in Sources */,
+				58FAEE0224533ABB00CB0F5B /* KeychainMatchLimit.swift in Sources */,
 				58FAEE0324533ABE00CB0F5B /* KeychainReturn.swift in Sources */,
 				58BFA5CD22A7CE1F00A6173D /* ApplicationConfiguration.swift in Sources */,
 				5850368D25A49E2200A43E93 /* PrivateKeyWithMetadata.swift in Sources */,
diff --git a/ios/MullvadVPN/Keychain.swift b/ios/MullvadVPN/Keychain/Keychain.swift
index 383219990a..383219990a 100644
--- a/ios/MullvadVPN/Keychain.swift
+++ b/ios/MullvadVPN/Keychain/Keychain.swift
diff --git a/ios/MullvadVPN/KeychainAttributes.swift b/ios/MullvadVPN/Keychain/KeychainAttributes.swift
index 397620f37b..397620f37b 100644
--- a/ios/MullvadVPN/KeychainAttributes.swift
+++ b/ios/MullvadVPN/Keychain/KeychainAttributes.swift
diff --git a/ios/MullvadVPN/KeychainClass.swift b/ios/MullvadVPN/Keychain/KeychainClass.swift
index 86e13f2bd6..86e13f2bd6 100644
--- a/ios/MullvadVPN/KeychainClass.swift
+++ b/ios/MullvadVPN/Keychain/KeychainClass.swift
diff --git a/ios/MullvadVPN/KeychainError.swift b/ios/MullvadVPN/Keychain/KeychainError.swift
index 4758d08b66..4758d08b66 100644
--- a/ios/MullvadVPN/KeychainError.swift
+++ b/ios/MullvadVPN/Keychain/KeychainError.swift
diff --git a/ios/MullvadVPN/KeychainMatchLimit.swift b/ios/MullvadVPN/Keychain/KeychainMatchLimit.swift
index f86b010532..f86b010532 100644
--- a/ios/MullvadVPN/KeychainMatchLimit.swift
+++ b/ios/MullvadVPN/Keychain/KeychainMatchLimit.swift
diff --git a/ios/MullvadVPN/KeychainReturn.swift b/ios/MullvadVPN/Keychain/KeychainReturn.swift
index 7216ffbd80..7216ffbd80 100644
--- a/ios/MullvadVPN/KeychainReturn.swift
+++ b/ios/MullvadVPN/Keychain/KeychainReturn.swift
diff --git a/ios/MullvadVPN/KeychainItemRevision.swift b/ios/MullvadVPN/KeychainItemRevision.swift
deleted file mode 100644
index 6e73d1a6a8..0000000000
--- a/ios/MullvadVPN/KeychainItemRevision.swift
+++ /dev/null
@@ -1,70 +0,0 @@
-//
-//  KeychainItemRevision.swift
-//  MullvadVPN
-//
-//  Created by pronebird on 07/05/2020.
-//  Copyright © 2020 Mullvad VPN AB. All rights reserved.
-//
-
-import Foundation
-
-/// A struct that helps to organize revisions for Keychain items
-/// Uses `Keychain.Attributes.generic` field for storing `Int64` revision number.
-struct KeychainItemRevision {
-    let revision: Int64
-
-    /// Returns the `KeychainItemRevision` initialized with the next in sequence revision
-    var nextRevision: KeychainItemRevision {
-        return .init(revision: revision + 1)
-    }
-
-    /// Initialize a struct with the given revision number
-    init(revision: Int64) {
-        self.revision = revision
-    }
-
-    /// Initialize a struct from the given `Keychain.Attributes`
-    ///
-    /// Returns `nil` when `Keychain.Attributes.generic` is `nil` or when it's set to data that
-    /// cannot be interpreted as `Int64`
-    init?(attributes: Keychain.Attributes) {
-        let revision = attributes.generic.flatMap { Self.parseRevision(from: $0) }
-        if let revision = revision {
-            self.revision = revision
-        } else {
-            return nil
-        }
-    }
-
-    /// Store the revision number in the given `Keychain.Attributes`
-    func store(in attributes: inout Keychain.Attributes) {
-        attributes.generic = asData()
-    }
-
-    /// A convenience method to initialize the first revision in sequence
-    static func firstRevision() -> Self {
-        return .init(revision: 1)
-    }
-
-    /// Serialize the revision number as `Data`
-    private func asData() -> Data {
-        return withUnsafeBytes(of: revision) { Data($0) }
-    }
-
-    /// Private helper to parse `Data` into `Int64`
-    /// Returns `nil` if the given raw data does not match the size of `Int64`
-    private static func parseRevision(from rawData: Data) -> Int64? {
-        var value: Int64 = 0
-
-        // Ensure that the buffer has as many bytes as needed to fill the `Int64`
-        guard rawData.count == MemoryLayout.size(ofValue: value) else {
-            return nil
-        }
-
-        _ = withUnsafeMutableBytes(of: &value) { (valuePointer) in
-            rawData.copyBytes(to: valuePointer)
-        }
-
-        return value
-    }
-}
diff --git a/ios/MullvadVPN/TunnelSettingsManager.swift b/ios/MullvadVPN/TunnelSettingsManager.swift
index aaf5453f1b..25bf72c41f 100644
--- a/ios/MullvadVPN/TunnelSettingsManager.swift
+++ b/ios/MullvadVPN/TunnelSettingsManager.swift
@@ -12,9 +12,6 @@ import Security
 /// Service name used for keychain items
 private let kServiceName = "Mullvad VPN"
 
-/// Maximum number of attempts to perform when updating the Keychain entry "atomically"
-private let kMaxAtomicUpdateRetryLimit = 20
-
 enum TunnelSettingsManager {}
 
 extension TunnelSettingsManager {
@@ -32,9 +29,6 @@ extension TunnelSettingsManager {
         /// A failure to update the existing entry in Keychain
         case updateEntry(Keychain.Error)
 
-        /// A failure to atomically update a Keychain entry after multiple attempts
-        case updateEntryAtomicallyRetryLimitExceeded
-
         /// A failure to remove an entry in Keychain
         case removeEntry(Keychain.Error)
 
@@ -106,9 +100,6 @@ extension TunnelSettingsManager {
                 // Store value
                 attributes.valueData = data
 
-                // Add revision
-                KeychainItemRevision.firstRevision().store(in: &attributes)
-
                 return Keychain.add(attributes)
                     .mapError { .addEntry($0) }
                     .map { _ in () }
@@ -136,11 +127,6 @@ extension TunnelSettingsManager {
                 let searchAttributes = searchTerm.makeKeychainAttributes()
                 var updateAttributes = Keychain.Attributes()
 
-                // Add revision if it's missing
-                if KeychainItemRevision(attributes: itemAttributes) == nil {
-                    KeychainItemRevision.firstRevision().store(in: &updateAttributes)
-                }
-
                 // Fix the accessibility permission for the Keychain entry
                 if itemAttributes.accessible != Self.keychainAccessibleLevel {
                     updateAttributes.accessible = Self.keychainAccessibleLevel
@@ -163,68 +149,43 @@ extension TunnelSettingsManager {
     /// The given block may run multiple times if Keychain entry was changed between read and write
     /// operations.
     static func update(searchTerm: KeychainSearchTerm,
-                       using changeConfiguration: (inout TunnelSettings) -> Void)
-        -> Result<TunnelSettings>
+                       using changeConfiguration: (inout TunnelSettings) -> Void) -> Result<TunnelSettings>
     {
-        for _ in (0 ..< kMaxAtomicUpdateRetryLimit) {
-            var searchQuery = searchTerm.makeKeychainAttributes()
-            searchQuery.return = [.attributes, .data]
-
-            let result = Keychain.findFirst(query: searchQuery)
-                .mapError { .lookupEntry($0) }
-                .flatMap { (itemAttributes) -> Result<TunnelSettings> in
-                    let itemAttributes = itemAttributes!
-                    let serializedData = itemAttributes.valueData!
-                    let account = itemAttributes.account!
-
-                    // Parse the current revision from Keychain attributes
-                    let currentRevision = KeychainItemRevision(attributes: itemAttributes)
+        var searchQuery = searchTerm.makeKeychainAttributes()
+        searchQuery.return = [.attributes, .data]
 
-                    // Pick the next revision in sequence
-                    let nextRevision = currentRevision?.nextRevision
-                        ?? KeychainItemRevision.firstRevision()
-
-                    return Self.decode(data: serializedData)
-                        .flatMap { (tunnelConfig) -> Result<TunnelSettings> in
-                            var tunnelConfig = tunnelConfig
-                            changeConfiguration(&tunnelConfig)
-
-                            return Self.encode(tunnelConfig: tunnelConfig)
-                                .flatMap { (newData) -> Result<TunnelSettings> in
-                                    // `SecItemUpdate` does not accept query parameters when using
-                                    // persistent reference, so constraint the query to account
-                                    // token instead now when we know it
-                                    var updateQuery = KeychainSearchTerm
-                                        .accountToken(account)
-                                        .makeKeychainAttributes()
+        let result = Keychain.findFirst(query: searchQuery)
+            .mapError { .lookupEntry($0) }
+            .flatMap { (itemAttributes) -> Result<TunnelSettings> in
+                let itemAttributes = itemAttributes!
+                let serializedData = itemAttributes.valueData!
+                let account = itemAttributes.account!
 
-                                    // Provide the last known revision via generic field to prevent
-                                    // overwriting the item if it was modified in the meanwhile.
-                                    // This field can be missing for the existing apps on AppStore
-                                    currentRevision?.store(in: &updateQuery)
+                return Self.decode(data: serializedData)
+                    .flatMap { (tunnelConfig) -> Result<TunnelSettings> in
+                        var tunnelConfig = tunnelConfig
+                        changeConfiguration(&tunnelConfig)
 
-                                    var updateAttributes = Keychain.Attributes()
-                                    updateAttributes.valueData = newData
+                        return Self.encode(tunnelConfig: tunnelConfig)
+                            .flatMap { (newData) -> Result<TunnelSettings> in
+                                // `SecItemUpdate` does not accept query parameters when using
+                                // persistent reference, so constraint the query to account
+                                // token instead now when we know it
+                                let updateQuery = KeychainSearchTerm
+                                    .accountToken(account)
+                                    .makeKeychainAttributes()
 
-                                    // Add the next revision number
-                                    nextRevision.store(in: &updateAttributes)
+                                var updateAttributes = Keychain.Attributes()
+                                updateAttributes.valueData = newData
 
-                                    return Keychain.update(query: updateQuery, update: updateAttributes)
-                                        .mapError { .updateEntry($0) }
-                                        .map { tunnelConfig }
+                                return Keychain.update(query: updateQuery, update: updateAttributes)
+                                    .mapError { .updateEntry($0) }
+                                    .map { tunnelConfig }
                             }
-                        }
+                    }
             }
 
-            // Retry if Keychain reported that the item was not found when updating
-            if case .failure(.updateEntry(.itemNotFound)) = result  {
-                continue
-            } else {
-                return result
-            }
-        }
-
-        return .failure(.updateEntryAtomicallyRetryLimitExceeded)
+        return result
     }
 
     static func remove(searchTerm: KeychainSearchTerm) -> Result<()> {