fix(apple): sync tunnel configuration after saving #4338

jamilbk · 2024-03-27T06:21:58Z

vercel · 2024-03-27T06:22:04Z

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 4:51pm

jamilbk · 2024-03-27T06:22:38Z

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

@@ -123,10 +123,7 @@ public final class TunnelStore: ObservableObject {
  func createManager() async throws {
    let protocolConfiguration = NETunnelProviderProtocol()
    let manager = NETunnelProviderManager()
-    let providerConfiguration =
-      protocolConfiguration.providerConfiguration
-      as? [String: String]


This was ~~the~~ a bug.

Was it trying to read something that wasn't there?

I think by default it's an empty dict [] which can be cast to [String: String], causing the conditional to evaluate true.

github-actions · 2024-03-27T06:24:00Z

Terraform Cloud Plan Output

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

Terraform Cloud Plan

github-actions · 2024-03-27T06:31:30Z

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	221.8 MiB (-2%)	223.3 MiB (-2%)	229 (+51%)
direct-tcp-server2client	224.6 MiB (+1%)	226.2 MiB (+1%)	84 (-69%)
relayed-tcp-client2server	144.6 MiB (-2%)	145.3 MiB (-3%)	198 (+43%)
relayed-tcp-server2client	153.5 MiB (-1%)	153.9 MiB (-1%)	162 (-4%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	50.0 MiB (-0%)	0.06ms (+72%)	0.00% (NaN%)
direct-udp-server2client	50.0 MiB (+0%)	0.01ms (-47%)	0.00% (NaN%)
relayed-udp-client2server	50.0 MiB (-0%)	0.08ms (-8%)	0.00% (NaN%)
relayed-udp-server2client	50.0 MiB (+0%)	0.04ms (-25%)	0.00% (NaN%)

jamilbk

This TunnelStore class is full of code smells still, but at least it's commented. Baby steps.

jamilbk · 2024-03-27T14:02:58Z

swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift

@@ -26,12 +26,12 @@ public final class SettingsViewModel: ObservableObject {
  public init(tunnelStore: TunnelStore, logger: AppLogger) {
    self.tunnelStore = tunnelStore
    self.logger = logger
-    settings = Settings.defaultValue
+    self.settings = tunnelStore.settings


Fixes another exposed race condition that we hit more often now because we connect faster and don't switch between states unless we need to.

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

swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift

  }

-  func loadSettings() {
+  func setupObservers() {


Renamed to match the convention elsewhere

jamilbk · 2024-03-27T14:04:06Z

swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift

-  func loadSettings() {
-    model.loadSettings()
+  func reloadSettings() {
+    model.settings = model.tunnelStore.settings


Not sure why this was rebinding settings each time... maybe a memory leak

jamilbk · 2024-03-27T14:05:18Z

swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/DeviceMetadata.swift

 import Foundation

 #if os(iOS)
  import UIKit.UIDevice
 #endif

 public class DeviceMetadata {
+  // If firezone-id hasn't ever been written, the app is considered
+  // to be launched for the first time.
+  public static func firstTime() -> Bool {


Moved to DeviceMetadata since it doesn't really belong in TunnelStore

jamilbk · 2024-03-27T14:05:43Z

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

@@ -20,7 +20,7 @@ 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 service = "dev.firezone.firezone"
+  private let service = Bundle.main.bundleIdentifier!


This is updated just so that debug testing and release testing don't clobber each other's Keychain items.

jamilbk · 2024-03-27T14:06:05Z

swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift

+  static func fromProviderConfiguration(providerConfiguration: [String: Any]?) -> Settings {
+    if let providerConfiguration = providerConfiguration as? [String: String] {
      return Settings(
        authBaseURL: providerConfiguration[TunnelStoreKeys.authBaseURL]
          ?? Settings.defaultValue.authBaseURL,
-        apiURL: providerConfiguration[TunnelStoreKeys.apiURL] ?? Settings.defaultValue.apiURL,
+        apiURL: providerConfiguration[TunnelStoreKeys.apiURL]
+          ?? Settings.defaultValue.apiURL,


ReactorScram

I don't understand most of it and I just skimmed through it

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

swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift

@@ -27,12 +27,13 @@ struct Settings: Equatable {
  }

  // Convert provider configuration (which may have empty fields if it was tampered with) to Settings
-  static func fromProviderConfiguration(providerConfiguration: [String: String]?) -> Settings {
-    if let providerConfiguration = providerConfiguration {
+  static func fromProviderConfiguration(providerConfiguration: [String: Any]?) -> Settings {


Why did this change to Any?

We take on the responsibility of casting in here instead of making the caller do it

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

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

-      // loadAllFromPreferences() returns list of tunnel configurations we created. Since our bundle ID
-      // can change (by us), find the one that's current and ignore the others.
+      // loadAllFromPreferences() returns list of tunnel configurations created by our bundle ID.
+      // Since our bundle ID can change (by us), find the one that's current and ignore the others.


It can't change at runtime, right? This is talking about the ID changing after an update is installed?

The App Store doesn't use the bundle as a package ID? Is such a thing possible?

Nah, this can't change at runtime. It's inexorably linked to other things in the App Store listing, so it wouldn't be easy to change.

One scenario we may want to change it though is if we build a different Network Extension for some reason (different transport, DNS proxy, content filter, etc) and want to include both or update this one's name.

ReactorScram · 2024-03-27T14:41:44Z

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

@@ -123,10 +123,7 @@ public final class TunnelStore: ObservableObject {
  func createManager() async throws {
    let protocolConfiguration = NETunnelProviderProtocol()
    let manager = NETunnelProviderManager()
-    let providerConfiguration =
-      protocolConfiguration.providerConfiguration
-      as? [String: String]


Was it trying to read something that wasn't there?

ReactorScram · 2024-03-27T14:42:39Z

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

+  // Network Extensions don't have a 2-way binding up to the GUI process,
+  // so we need to periodically ask the tunnel process for them.


Right, you mentioned that, now I remember.

jamilbk requested a review from ReactorScram March 27, 2024 06:22

jamilbk commented Mar 27, 2024

View reviewed changes

jamilbk force-pushed the fix/tunnel-config branch 4 times, most recently from d70aeb0 to d68d311 Compare March 27, 2024 14:25

jamilbk mentioned this pull request Mar 27, 2024

bug(macos Client): At startup the Welcome window flashes for a couple frames before closing #4339

Closed

jamilbk commented Mar 27, 2024

View reviewed changes

ReactorScram approved these changes Mar 27, 2024

View reviewed changes

Base automatically changed from fix/macos-12-keychain-group to main March 27, 2024 16:30

Sync tunnel config before using

b3a019b

jamilbk force-pushed the fix/tunnel-config branch from ff503d2 to b3a019b Compare March 27, 2024 16:50

jamilbk enabled auto-merge March 27, 2024 16:51

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

Merged via the queue into main with commit 2e35fe8 Mar 27, 2024
155 checks passed

jamilbk deleted the fix/tunnel-config branch March 27, 2024 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apple): sync tunnel configuration after saving #4338

fix(apple): sync tunnel configuration after saving #4338

jamilbk commented Mar 27, 2024 •

edited

vercel bot commented Mar 27, 2024 •

edited

jamilbk Mar 27, 2024 •

edited

ReactorScram Mar 27, 2024

jamilbk Mar 27, 2024

github-actions bot commented Mar 27, 2024 •

edited

github-actions bot commented Mar 27, 2024 •

edited

jamilbk left a comment

jamilbk Mar 27, 2024

jamilbk Mar 27, 2024

jamilbk Mar 27, 2024

jamilbk Mar 27, 2024

jamilbk Mar 27, 2024

jamilbk 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

ReactorScram Mar 27, 2024

		// Network Extensions don't have a 2-way binding up to the GUI process,
		// so we need to periodically ask the tunnel process for them.

fix(apple): sync tunnel configuration after saving #4338

fix(apple): sync tunnel configuration after saving #4338

Conversation

jamilbk commented Mar 27, 2024 • edited

vercel bot commented Mar 27, 2024 • edited

jamilbk Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2024 • edited

Terraform Cloud Plan Output

github-actions bot commented Mar 27, 2024 • edited

Performance Test Results

TCP

UDP

jamilbk 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

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

jamilbk commented Mar 27, 2024 •

edited

vercel bot commented Mar 27, 2024 •

edited

jamilbk Mar 27, 2024 •

edited

github-actions bot commented Mar 27, 2024 •

edited

github-actions bot commented Mar 27, 2024 •

edited