fix(apple): Use keychain from the tunnel process *only* (#4335)

This fixes another long-standing bug with the Apple client: Keychain
groups.

Apple's Keychain docs are woefully unclear and lacking on the Keychain.

These are the main takeaways:

- Apple wants you to use the "[Data protection
keychain](https://developer.apple.com/documentation/security/ksecusedataprotectionkeychain)"
on macOS which allows it to behave like an iOS keychain. That opens up
the door for possible to sync to iCloud (which we don't use).
- Data protection keychain items, [it
appears](https://forums.developer.apple.com/forums/thread/710758),
cannot be created by Network Extensions.
- However, we _can_ save to the regular keychain (by default the system
keychain for root procs like us), which is file-based.
- Keychain items can be shared (both read/write) between apps, but **not
between users**. The tunnel process and gui process run as different
users. The only way for this to happen is to use the old file-based
Keychain and use [very
deprecated](https://developer.apple.com/documentation/technotes/tn3137-on-mac-keychains)
APIs to allow both "users" access, which is what we were doing before.
- To fix this, we limit all keychain operations to the tunnel proc only.
The GUI passes the auth token in during the `startTunnel` call, which
the system then passes to our `PacketTunnelProvider` class.

This uses the file-based Keychain, but since we need to use that
keychain as the root tunnel proc, we don't have much choice. The "Allow
access" dialog bug on macOS 12 is fixed by the fact that we are only
accessing it from the same user (tunnel proc) that created it now.

swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift

@@ -20,8 +20,11 @@ public enum KeychainError: Error {
 public actor Keychain {
   private let label = "Firezone token"
   private let description = "Firezone access token used to authenticate the client."
-  private let account = "Firezone"
-  private let service = Bundle.main.bundleIdentifier!
+  private let service = "dev.firezone.firezone"
+
+  // Bump this for backwards-incompatible Keychain changes; this is effectively the
+  // upsert key.
+  private let account = "1"
 
   private let workQueue = DispatchQueue(label: "FirezoneKeychainWorkQueue")
 
@@ -47,7 +50,7 @@ public actor Keychain {
 
   public init() {}
 
-  func add(token: Token) async throws -> PersistentRef {
+  public func add(token: Token) async throws {
     return try await withCheckedThrowingContinuation { [weak self] continuation in
       self?.workQueue.async { [weak self] in
         guard let self = self else {
@@ -56,16 +59,12 @@ public actor Keychain {
         }
 
         let query: [CFString: Any] = [
-          // Common for both iOS and macOS:
           kSecClass: kSecClassGenericPassword,
-          kSecAttrLabel: label,
-          kSecAttrDescription: description,
-          kSecAttrAccount: account,
-          kSecAttrService: service,
+          kSecAttrLabel: self.label,
+          kSecAttrAccount: self.account,
+          kSecAttrDescription: self.description,
+          kSecAttrService: self.service,
           kSecValueData: token.data(using: .utf8) as Any,
-          kSecReturnPersistentRef: true,
-          kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock,
-          kSecAttrAccessGroup: AppInfoPlistConstants.appGroupId as CFString as Any,
         ]
 
         var ref: CFTypeRef?
@@ -76,18 +75,13 @@ public actor Keychain {
           return
         }
 
-        guard let savedPersistentRef = ref as? Data else {
-          continuation.resume(throwing: KeychainError.nilResultFromAppleSecCall(call: "SecItemAdd"))
-          return
-        }
-        continuation.resume(returning: savedPersistentRef)
+        continuation.resume()
         return
       }
     }
   }
 
-  func update(token: Token) async throws {
-
+  public func update(token: Token) async throws {
     return try await withCheckedThrowingContinuation { [weak self] continuation in
       self?.workQueue.async { [weak self] in
         guard let self = self else {
@@ -97,7 +91,9 @@ public actor Keychain {
 
         let query: [CFString: Any] = [
           kSecClass: kSecClassGenericPassword,
+          kSecAttrLabel: self.label,
           kSecAttrAccount: self.account,
+          kSecAttrDescription: self.description,
           kSecAttrService: self.service,
         ]
         let attributesToUpdate = [
@@ -121,6 +117,7 @@ public actor Keychain {
       self?.workQueue.async {
         let query =
           [
+            kSecClass: kSecClassGenericPassword,
             kSecValuePersistentRef: persistentRef,
             kSecReturnData: true,
           ] as [CFString: Any]
@@ -138,13 +135,14 @@ public actor Keychain {
     }
   }
 
-  func search() async -> PersistentRef? {
+  public func search() async -> PersistentRef? {
     return await withCheckedContinuation { [weak self] continuation in
       guard let self = self else { return }
       self.workQueue.async {
         let query =
           [
             kSecClass: kSecClassGenericPassword,
+            kSecAttrLabel: self.label,
             kSecAttrAccount: self.account,
             kSecAttrDescription: self.description,
             kSecAttrService: self.service,
@@ -161,6 +159,21 @@ public actor Keychain {
     }
   }
 
+  public func delete(persistentRef: PersistentRef) async throws {
+    return try await withCheckedThrowingContinuation { [weak self] continuation in
+      self?.workQueue.async {
+        let query = [kSecValuePersistentRef: persistentRef] as [CFString: Any]
+        let ret = SecStatus(SecItemDelete(query as CFDictionary))
+        guard ret.isSuccess || ret == .status(.itemNotFound) else {
+          continuation.resume(
+            throwing: KeychainError.appleSecError(call: "SecItemDelete", status: ret))
+          return
+        }
+        continuation.resume(returning: ())
+      }
+    }
+  }
+
   private func securityError(_ status: OSStatus) -> Error {
     KeychainError.securityError(KeychainStatus(rawValue: status)!)
   }

swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift

@@ -8,7 +8,7 @@ import Dependencies
 import Foundation
 
 struct KeychainStorage: Sendable {
-  var add: @Sendable (Keychain.Token) async throws -> Keychain.PersistentRef
+  var add: @Sendable (Keychain.Token) async throws -> Void
   var update: @Sendable (Keychain.Token) async throws -> Void
   var search: @Sendable () async -> Keychain.PersistentRef?
 }
@@ -31,7 +31,6 @@ extension KeychainStorage: DependencyKey {
         storage.withValue {
           let uuid = UUID().uuidString.data(using: .utf8)!
           $0[uuid] = (token)
-          return uuid
         }
       },
       update: { token in

swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift

@@ -97,10 +97,8 @@ public final class TunnelStore: ObservableObject {
         }
       }
 
-      // Connect on app launch unless we're already connected
-      if let _ = manager?.protocolConfiguration?.passwordReference,
-        self.status == .disconnected
-      {
+      // Try to connect on app launch
+      if self.status == .disconnected {
         try await start()
       }
 
@@ -123,36 +121,27 @@ public final class TunnelStore: ObservableObject {
 
   // Initialize and save a new VPN profile in system Preferences
   func createManager() async throws {
-    if let manager = manager {
-      // Someone deleted the manager while Fireone is running!
-      // Let's assume that was an accident and recreate it from
-      // our current state.
-      try await manager.saveToPreferences()
-      try await manager.loadFromPreferences()
-    } else {
-      let protocolConfiguration = NETunnelProviderProtocol()
-      let manager = NETunnelProviderManager()
-      let providerConfiguration =
-        protocolConfiguration.providerConfiguration
-        as? [String: String]
-        ?? Settings.defaultValue.toProviderConfiguration()
+    let protocolConfiguration = NETunnelProviderProtocol()
+    let manager = NETunnelProviderManager()
+    let providerConfiguration =
+      protocolConfiguration.providerConfiguration
+      as? [String: String]
+      ?? Settings.defaultValue.toProviderConfiguration()
 
-      protocolConfiguration.providerConfiguration = providerConfiguration
-      protocolConfiguration.providerBundleIdentifier = bundleIdentifier
-      protocolConfiguration.serverAddress = providerConfiguration[TunnelStoreKeys.apiURL]
-      manager.localizedDescription = bundleDescription
-      manager.protocolConfiguration = protocolConfiguration
+    protocolConfiguration.providerConfiguration = providerConfiguration
+    protocolConfiguration.providerBundleIdentifier = bundleIdentifier
+    protocolConfiguration.serverAddress = providerConfiguration[TunnelStoreKeys.apiURL]
+    manager.localizedDescription = bundleDescription
+    manager.protocolConfiguration = protocolConfiguration
 
-      // Save the new VPN profile to System Preferences
-      try await manager.saveToPreferences()
-      try await manager.loadFromPreferences()
+    // Save the new VPN profile to System Preferences
+    try await manager.saveToPreferences()
 
-      self.manager = manager
-      self.status = .disconnected
-    }
+    self.manager = manager
+    self.status = .disconnected
   }
 
-  func start() async throws {
+  func start(token: String? = nil) async throws {
     logger.log("\(#function)")
 
     guard let manager = manager
@@ -169,17 +158,22 @@ public final class TunnelStore: ObservableObject {
 
     manager.isEnabled = true
     try await manager.saveToPreferences()
-    try await manager.loadFromPreferences()
 
     let session = castToSession(manager.connection)
     do {
-      try session.startTunnel()
+      var options: [String: NSObject]? = nil
+      if let token = token {
+        options = ["token": token as NSObject]
+      }
+
+      try session.startTunnel(options: options)
     } catch {
+      logger.error("Error starting tunnel: \(error)")
       throw TunnelStoreError.startTunnelErrored(error)
     }
   }
 
-  func stop() async throws {
+  func stop(clearToken: Bool = false) async throws {
     logger.log("\(#function)")
 
     guard let manager = manager else {
@@ -193,7 +187,13 @@ public final class TunnelStore: ObservableObject {
       return
     }
     let session = castToSession(manager.connection)
-    session.stopTunnel()
+    if clearToken {
+      try session.sendProviderMessage("signOut".data(using: .utf8)!) { _ in
+        session.stopTunnel()
+      }
+    } else {
+      session.stopTunnel()
+    }
   }
 
   public func cancelSignIn() {
@@ -213,44 +213,26 @@ public final class TunnelStore: ObservableObject {
     let authURL = URL(string: settings.authBaseURL)!
     let authResponse = try await auth.signIn(authURL)
 
-    // Apple recommends updating Keychain items in place if possible
-    var tokenRef: Keychain.PersistentRef
-    if let ref = await keychain.search() {
-      try await keychain.update(authResponse.token)
-      tokenRef = ref
-    } else {
-      tokenRef = try await keychain.add(authResponse.token)
-    }
-
-    // Save token and actorName
+    // Save actorName
     providerConfiguration[TunnelStoreKeys.actorName] = authResponse.actorName
     protocolConfiguration.providerConfiguration = providerConfiguration
-    protocolConfiguration.passwordReference = tokenRef
     manager.protocolConfiguration = protocolConfiguration
-    try await manager.saveToPreferences()
-    try await manager.loadFromPreferences()
 
-    // Start tunnel
-    try await start()
+    try await manager.saveToPreferences()
+
+    // Bring the tunnel up and send it a token to start
+    do {
+      try await start(token: authResponse.token)
+    } catch {
+      logger.error("Error signing in: \(error)")
+    }
   }
 
   func signOut() async throws {
     logger.log("\(#function)")
-    guard let manager = manager,
-      ![.disconnecting, .disconnected].contains(status),
-      let protocolConfiguration = manager.protocolConfiguration as? NETunnelProviderProtocol
-    else {
-      logger.error("\(#function): Tunnel seems to be already disconnected")
-      return
-    }
-
-    // Clear token from VPN profile, but keep it in Keychain because the user
-    // may have customized the Keychain Item.
-    protocolConfiguration.passwordReference = nil
-    try await manager.saveToPreferences()
 
     // Stop tunnel
-    try await stop()
+    try await stop(clearToken: true)
   }
 
   func beginUpdatingResources() {
@@ -418,17 +400,6 @@ public final class TunnelStore: ObservableObject {
         SessionNotificationHelper.showSignedOutAlertmacOS(logger: self.logger, tunnelStore: self)
       }
     #endif
-
-    // Clear tokenRef
-    guard let manager = manager else { return }
-    let protocolConfiguration = manager.protocolConfiguration
-    protocolConfiguration?.passwordReference = nil
-    manager.protocolConfiguration = protocolConfiguration
-    do {
-      try await manager.saveToPreferences()
-    } catch {
-      logger.error("\(#function): Couldn't clear tokenRef")
-    }
   }
 }
 

swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift

@@ -20,44 +20,74 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
   private var adapter: Adapter?
 
   override func startTunnel(
-    options _: [String: NSObject]? = nil,
+    options: [String: NSObject]?,
     completionHandler: @escaping (Error?) -> Void
   ) {
+    super.startTunnel(options: options, completionHandler: completionHandler)
     logger.log("\(#function)")
 
-    guard let apiURL = protocolConfiguration.serverAddress,
-      let tokenRef = protocolConfiguration.passwordReference,
-      let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)?
-        .providerConfiguration as? [String: String],
-      let logFilter = providerConfiguration[TunnelStoreKeys.logFilter]
-    else {
-      completionHandler(
-        PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress"))
-      return
-    }
-
     Task {
-      let keychain = Keychain()
-      guard let token = await keychain.load(persistentRef: tokenRef) else {
-        logger.error("\(#function): No token found in Keychain")
-        completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain)
-        return
-      }
-
-      let adapter = Adapter(
-        apiURL: apiURL,
-        token: token,
-        logFilter: logFilter,
-        packetTunnelProvider: self)
-      self.adapter = adapter
       do {
-        try adapter.start { error in
-          if let error {
-            self.logger.error("\(#function): \(error)")
+        var token = options?["token"] as? String
+        let keychain = Keychain()
+        var tokenRef = await keychain.search()
+
+        if let token = token {
+          // 1. If we're passed a token, save it to keychain
+
+          // Apple recommends updating Keychain items in place if possible
+          // In reality this won't happen unless there's some kind of race condition
+          // because we would have deleted the item upon sign out.
+          if let ref = tokenRef {
+            try await keychain.update(token: token)
+          } else {
+            try await keychain.add(token: token)
           }
-          completionHandler(error)
+
+        } else {
+
+          // 2. Otherwise, load it from the keychain
+          guard let tokenRef = tokenRef
+          else {
+            completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain)
+            return
+          }
+
+          token = await keychain.load(persistentRef: tokenRef)
         }
+
+        // 3. Now we should have a token, so connect
+        guard let apiURL = protocolConfiguration.serverAddress
+        else {
+          completionHandler(
+            PacketTunnelProviderError.savedProtocolConfigurationIsInvalid("serverAddress"))
+          return
+        }
+
+        guard
+          let providerConfiguration = (protocolConfiguration as? NETunnelProviderProtocol)?
+            .providerConfiguration as? [String: String],
+          let logFilter = providerConfiguration[TunnelStoreKeys.logFilter]
+        else {
+          completionHandler(
+            PacketTunnelProviderError.savedProtocolConfigurationIsInvalid(
+              "providerConfiguration.logFilter"))
+          return
+        }
+
+        guard let token = token
+        else {
+          completionHandler(PacketTunnelProviderError.tokenNotFoundInKeychain)
+          return
+        }
+
+        let adapter = Adapter(
+          apiURL: apiURL, token: token, logFilter: logFilter, packetTunnelProvider: self)
+        self.adapter = adapter
+
+        try adapter.start(completionHandler: completionHandler)
       } catch {
+        logger.error("\(#function): Error! \(error)")
         completionHandler(error)
       }
     }
@@ -73,12 +103,17 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
 
     if case .authenticationCanceled = reason {
       do {
-        // Remove the passwordReference from our configuration so that it's not used again
-        // if the app is re-launched. There's no good way to send data like this from the
-        // Network Extension to the GUI, so save it to a file for the GUI to read later.
-        try String(reason.rawValue).write(to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8)
+        // This was triggered from onDisconnect, so clear our token
+        Task { try await clearToken() }
+
+        // There's no good way to send data like this from the
+        // Network Extension to the GUI, so save it to a file for the GUI to read upon
+        // either status change or the next launch.
+        try String(reason.rawValue).write(
+          to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8)
       } catch {
-        logger.error("\(#function): Couldn't write provider stop reason to file. Notification won't work.")
+        logger.error(
+          "\(#function): Couldn't write provider stop reason to file. Notification won't work.")
       }
       #if os(iOS)
         // iOS notifications should be shown from the tunnel process
@@ -94,12 +129,36 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
 
   // TODO: Use a message format to allow requesting different types of data.
   // This currently assumes we're requesting resources.
-  override func handleAppMessage(_ hash: Data, completionHandler: ((Data?) -> Void)? = nil) {
-    adapter?.getResourcesIfVersionDifferentFrom(hash: hash) {
-      resourceListJSON in
-      completionHandler?(resourceListJSON?.data(using: .utf8))
+  override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) {
+    let string = String(data: message, encoding: .utf8)
+
+    switch string {
+    case "signOut":
+      Task {
+        do {
+          try await clearToken()
+        } catch {
+          logger.error("\(#function): Error: \(error)")
+        }
+      }
+    default:
+      adapter?.getResourcesIfVersionDifferentFrom(hash: message) {
+        resourceListJSON in
+        completionHandler?(resourceListJSON?.data(using: .utf8))
+      }
     }
   }
+
+  private func clearToken() async throws {
+    let keychain = Keychain()
+    guard let ref = await keychain.search()
+    else {
+      logger.error("\(#function): Error: token not found!")
+      return
+    }
+
+    try await keychain.delete(persistentRef: ref)
+  }
 }
 
 extension NEProviderStopReason: CustomStringConvertible {