Improve VPN auth token storage (#639)

Required:

Task/Issue URL: https://app.asana.com/0/1199333091098016/1206460327917918/f
iOS PR: duckduckgo/iOS#2398
macOS PR: duckduckgo/macos-browser#2129
What kind of version bump will this require?: Major

Description:

This PR improves the way the auth token is handled for the VPN:

The token is no longer deleted and then re-added so frequently; instead, it gets updated only when required
We no longer emit error events when an invalid invite code is entered manually as this case is expected

Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift

@@ -58,6 +58,7 @@ public enum NetworkProtectionError: LocalizedError {
     case failedToCastKeychainValueToData(field: String)
     case keychainReadError(field: String, status: Int32)
     case keychainWriteError(field: String, status: Int32)
+    case keychainUpdateError(field: String, status: Int32)
     case keychainDeleteError(status: Int32)
 
     // Wireguard errors

Sources/NetworkProtection/FeatureActivation/NetworkProtectionCodeRedemptionCoordinator.swift

@@ -34,25 +34,30 @@ public final class NetworkProtectionCodeRedemptionCoordinator: NetworkProtection
     private let networkClient: NetworkProtectionClient
     private let tokenStore: NetworkProtectionTokenStore
     private let versionStore: NetworkProtectionLastVersionRunStore
+    private let isManualCodeRedemptionFlow: Bool
     private let errorEvents: EventMapping<NetworkProtectionError>
 
     convenience public init(environment: VPNSettings.SelectedEnvironment,
                             tokenStore: NetworkProtectionTokenStore,
                             versionStore: NetworkProtectionLastVersionRunStore = .init(),
+                            isManualCodeRedemptionFlow: Bool = false,
                             errorEvents: EventMapping<NetworkProtectionError>) {
         self.init(networkClient: NetworkProtectionBackendClient(environment: environment),
                   tokenStore: tokenStore,
                   versionStore: versionStore,
+                  isManualCodeRedemptionFlow: isManualCodeRedemptionFlow,
                   errorEvents: errorEvents)
     }
 
     init(networkClient: NetworkProtectionClient,
          tokenStore: NetworkProtectionTokenStore,
          versionStore: NetworkProtectionLastVersionRunStore = .init(),
+         isManualCodeRedemptionFlow: Bool = false,
          errorEvents: EventMapping<NetworkProtectionError>) {
         self.networkClient = networkClient
         self.tokenStore = tokenStore
         self.versionStore = versionStore
+        self.isManualCodeRedemptionFlow = isManualCodeRedemptionFlow
         self.errorEvents = errorEvents
     }
 
@@ -65,8 +70,13 @@ public final class NetworkProtectionCodeRedemptionCoordinator: NetworkProtection
             versionStore.lastVersionRun = AppVersion.shared.versionNumber
 
         case .failure(let error):
-            errorEvents.fire(error.networkProtectionError)
-            throw error
+            if case .invalidInviteCode = error, isManualCodeRedemptionFlow {
+                // Deliberately ignore cases where invalid invite codes are entered into the redemption form
+                throw error
+            } else {
+                errorEvents.fire(error.networkProtectionError)
+                throw error
+            }
         }
     }
 

Sources/NetworkProtection/KeyManagement/NetworkProtectionKeychainStore.swift

@@ -23,13 +23,15 @@ enum NetworkProtectionKeychainStoreError: Error, NetworkProtectionErrorConvertib
     case failedToCastKeychainValueToData(field: String)
     case keychainReadError(field: String, status: Int32)
     case keychainWriteError(field: String, status: Int32)
+    case keychainUpdateError(field: String, status: Int32)
     case keychainDeleteError(status: Int32)
 
     var networkProtectionError: NetworkProtectionError {
         switch self {
         case .failedToCastKeychainValueToData(let field): return .failedToCastKeychainValueToData(field: field)
         case .keychainReadError(let field, let status): return .keychainReadError(field: field, status: status)
         case .keychainWriteError(let field, let status): return .keychainWriteError(field: field, status: status)
+        case .keychainUpdateError(let field, let status): return .keychainUpdateError(field: field, status: status)
         case .keychainDeleteError(let status): return .keychainDeleteError(status: status)
         }
     }
@@ -83,11 +85,32 @@ final class NetworkProtectionKeychainStore {
 
         let status = SecItemAdd(query as CFDictionary, nil)
 
-        guard status == errSecSuccess else {
+        switch status {
+        case errSecSuccess:
+            return
+        case errSecDuplicateItem:
+            let updateStatus = updateData(data, named: name)
+
+            if updateStatus != errSecSuccess {
+                throw NetworkProtectionKeychainStoreError.keychainUpdateError(field: name, status: status)
+            }
+        default:
             throw NetworkProtectionKeychainStoreError.keychainWriteError(field: name, status: status)
         }
     }
 
+    private func updateData(_ data: Data, named name: String) -> OSStatus {
+        var query = defaultAttributes()
+        query[kSecAttrAccount] = name
+
+        let newAttributes = [
+          kSecValueData: data,
+          kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock
+        ] as [CFString: Any]
+
+        return SecItemUpdate(query as CFDictionary, newAttributes as CFDictionary)
+    }
+
     func deleteAll() throws {
         var query = defaultAttributes()
 #if os(macOS)

Sources/NetworkProtection/KeyManagement/NetworkProtectionTokenStore.swift

@@ -58,10 +58,6 @@ public final class NetworkProtectionKeychainTokenStore: NetworkProtectionTokenSt
     public func store(_ token: String) throws {
         let data = token.data(using: .utf8)!
         do {
-            try (try? keychainStore.deleteAll()) ?? {
-                // sometimes it fails from the first try: retry once
-                try keychainStore.deleteAll()
-            }()
             try keychainStore.writeData(data, named: Defaults.tokenStoreName)
         } catch {
             handle(error)

Sources/NetworkProtection/PacketTunnelProvider.swift

@@ -408,8 +408,12 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
 
     private func loadAuthToken(from options: StartupOptions) throws {
         switch options.authToken {
-        case .set(let authToken):
-            try tokenStore.store(authToken)
+        case .set(let newAuthToken):
+            if let currentAuthToken = try? tokenStore.fetchToken(), currentAuthToken == newAuthToken {
+                return
+            }
+
+            try tokenStore.store(newAuthToken)
         case .useExisting:
             break
         case .reset: