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

jamilbk · 2024-03-27T03:49:11Z

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" 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, 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 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.

vercel · 2024-03-27T03:49:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
firezone	⬜️ Ignored (Inspect)	Visit Preview		Mar 27, 2024 3:17pm

github-actions · 2024-03-27T03:51:09Z

Terraform Cloud Plan Output

Plan: 9 to add, 8 to change, 9 to destroy.

Terraform Cloud Plan

github-actions · 2024-03-27T03:58:07Z

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	218.0 MiB (-3%)	219.4 MiB (-3%)	264 (+8%)
direct-tcp-server2client	225.0 MiB (+1%)	226.7 MiB (+1%)	360 (+107%)
relayed-tcp-client2server	147.4 MiB (+1%)	148.2 MiB (+1%)	201 (+15%)
relayed-tcp-server2client	151.1 MiB (-4%)	151.6 MiB (-4%)	163 (-19%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	50.0 MiB (+0%)	0.04ms (-81%)	0.00% (NaN%)
direct-udp-server2client	50.0 MiB (+0%)	0.02ms (+144%)	0.00% (NaN%)
relayed-udp-client2server	50.0 MiB (+0%)	0.08ms (-41%)	0.00% (NaN%)
relayed-udp-server2client	50.0 MiB (+0%)	0.05ms (+48%)	0.00% (NaN%)

jamilbk · 2024-03-27T04:02:48Z

swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift

-    adapter?.getResourcesIfVersionDifferentFrom(hash: hash) {
-      resourceListJSON in
-      completionHandler?(resourceListJSON?.data(using: .utf8))
+  override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) {


This is the primary IPC callback from GUI -> Tunnel.

We overload the message parameter to determine what the GUI is asking the tunnel to do. This could be expanded in the future using JSON and more structured format, but I kept it simple for now as it only handle 2 commands.

Data is Vec<u8>, right?

I believe so, it's a byte array

jamilbk · 2024-03-27T05:19:12Z

@AndrewDryga This was tested on macOS 12.7.4 and the keychain issue is fixed.

jamilbk · 2024-03-27T14:57:57Z

Fixes #2324 (finally!)

ReactorScram

I don't understand all the code changes but the reasoning makes sense.

On Windows and Linux I'm planning to go the other way and have the GUI process store the token in the keyring, since it should be more secure if the OS protects it with the user's password. So the GUI process will do almost everything with the token, except for sending it to the portal.

ReactorScram · 2024-03-27T14:53:53Z

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"


I saw this change the other direction in another PR. Is it because the bundle ID is not available in the NetworkExtension?

The other PR is based on this one, so it changes it back (sorry -- kinda a pita to switch branches when working on the Apple/Android clients)

ReactorScram · 2024-03-27T14:55:31Z

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

@@ -138,13 +135,14 @@ public actor Keychain {
    }
  }

-  func search() async -> PersistentRef? {
+  public func search() async -> PersistentRef? {


Is search what it's called when you get a token from the keychain?

search gives you a Persistent Ref which is just a 20-byte pointer to the item

ReactorScram · 2024-03-27T15:55:00Z

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

+          kSecAttrLabel: self.label,
+          kSecAttrAccount: self.account,
+          kSecAttrDescription: self.description,
+          kSecAttrService: self.service,


Where were these values coming from before if they weren't members of self?

Still from the class -- the scope changes when inside Task

ReactorScram · 2024-03-27T15:57:20Z

swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift

-    adapter?.getResourcesIfVersionDifferentFrom(hash: hash) {
-      resourceListJSON in
-      completionHandler?(resourceListJSON?.data(using: .utf8))
+  override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) {


Data is Vec<u8>, right?

jamilbk · 2024-03-27T16:14:20Z

On Windows and Linux I'm planning to go the other way and have the GUI process store the token in the keyring

Yeah, I considered that as well, but we need headless on the Apple clients because on iOS our GUI process can be reaped by the system at any time.

ReactorScram · 2024-03-27T16:18:50Z

So if the GUI and tunnel process are both reaped, the tunnel must be able to bring itself back without the GUI helping? Makes sense

jamilbk changed the title fix(apple): Use keychain from the tunnel process **only** fix(apple): Use keychain from the tunnel process *only* Mar 27, 2024

jamilbk requested review from ReactorScram and AndrewDryga March 27, 2024 03:49

Remove UI process access to Keychain; don't use shared Keychain

a81c963

jamilbk force-pushed the fix/macos-12-keychain-group branch from a540116 to a81c963 Compare March 27, 2024 04:01

jamilbk commented Mar 27, 2024

View reviewed changes

Merge branch 'main' into fix/macos-12-keychain-group

1c67f9f

ReactorScram approved these changes Mar 27, 2024

View reviewed changes

jamilbk added this pull request to the merge queue Mar 27, 2024

Merged via the queue into main with commit 2ee5508 Mar 27, 2024
152 checks passed

jamilbk deleted the fix/macos-12-keychain-group branch March 27, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jamilbk commented Mar 27, 2024 •

edited

Loading

vercel bot commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading

jamilbk Mar 27, 2024

ReactorScram Mar 27, 2024

jamilbk Mar 27, 2024

jamilbk commented Mar 27, 2024

jamilbk commented Mar 27, 2024

ReactorScram left a comment

ReactorScram Mar 27, 2024

jamilbk Mar 27, 2024

ReactorScram Mar 27, 2024

jamilbk Mar 27, 2024

ReactorScram Mar 27, 2024

jamilbk Mar 27, 2024

ReactorScram Mar 27, 2024

jamilbk commented Mar 27, 2024

ReactorScram commented Mar 27, 2024

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

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

Conversation

jamilbk commented Mar 27, 2024 • edited Loading

vercel bot commented Mar 27, 2024 • edited Loading

github-actions bot commented Mar 27, 2024 • edited Loading

Terraform Cloud Plan Output

github-actions bot commented Mar 27, 2024 • edited Loading

Performance Test Results

TCP

UDP

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamilbk commented Mar 27, 2024

jamilbk commented Mar 27, 2024

ReactorScram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamilbk commented Mar 27, 2024

ReactorScram commented Mar 27, 2024

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

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

jamilbk commented Mar 27, 2024 •

edited

Loading

vercel bot commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading