refactor(apple/macos): Use System Extension packaging mode for macOS Network Extension - #7344
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Can we start this one in CI to do some smoke testing against staging for example? |
No because it's still a network extension. We would need a replacement glue wrapper that uses the UNIX utun api. |
04fd5f9 to
2276913
Compare
6d2fda9 to
aa5245c
Compare
aa5245c to
508700b
Compare
fe1def4 to
92d720a
Compare
92d720a to
ba09fe3
Compare
jamilbk
changed the title
ba09fe3 to
e146519
Compare
e146519 to
4038a46
Compare
4038a46 to
a73dcdb
Compare
| options = ["token": token as NSObject] | ||
| } | ||
|
|
||
| #if os(macOS) |
There was a problem hiding this comment.
This is worth understanding.
There was a problem hiding this comment.
This is the new entrypoint of the macOS app.
There was a problem hiding this comment.
This is the meat of it. This class provides a helper to install the SystemExtension which we use when starting the tunnel.
The remaining request functions are lifetime functions called by the system to update us on the status of the installation. Note the last one -- .replace tells the system that we'd like to replace the existing system extension if it's already installed. It will only be replaced if the marketing version of the replacement is newer than the existing extension. This will happen when a user upgrades his/her client, for example.
There was a problem hiding this comment.
It will only be replaced if the marketing version of the replacement is newer than the existing extension. This will happen when a user upgrades his/her client, for example.
So in order to downgrade, the user needs to completely uninstall Firezone first? Should we document this in the KB?
There was a problem hiding this comment.
I'm not exactly sure yet -- need to test how that works on a live system.
We'll be able to test that after the PR for the release builds is finalized. The behavior of the SystemExtension with developer mode enabled and not is quite different unfortunately.
a73dcdb to
895667e
Compare
895667e to
181de9f
Compare
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
It would be nice to split some of the unrelated stuff out into other PRs, just to make sure we have a minimal commit on main for the change.
What are the follow tasks from this?
- We need to update firezone.dev/changelog to offer the new downloads.
- We'll have to start making draft releases on GitHub.
- Makefile docs for releasing will need to be updated.
- Update KB on how to downgrade the client might be useful (as far as I understand, you can install an old version but need to remove the current one first?)
| if let error = error { | ||
| Log.app.error("\(#function): Installing system extension failed! \(error.localizedDescription)") | ||
| } else { | ||
| self.startTunnel(options: options) | ||
| } |
There was a problem hiding this comment.
| if let error = error { | |
| Log.app.error("\(#function): Installing system extension failed! \(error.localizedDescription)") | |
| } else { | |
| self.startTunnel(options: options) | |
| } | |
| if let error = error { | |
| Log.app.error("\(#function): Installing system extension failed! \(error.localizedDescription)") | |
| return | |
| } | |
| self.startTunnel(options: options) |
Personally, I find that avoiding else makes code more readable because it visually separates happy-path from error paths through differences in indentation.
There was a problem hiding this comment.
Unfortunately this just returns from the closure and not the outer function.
There was a problem hiding this comment.
Actually that shouldn't matter in this case the closure is the final statement anyhow.
There was a problem hiding this comment.
A closure should have a scoped control flow so you should be able to early return from that to avoid executing statements after the if.
| // | ||
| // main.swift | ||
| // FirezoneNetworkExtension | ||
| // | ||
| // Created by Jamil Bou Kheir on 11/14/24. | ||
| // |
There was a problem hiding this comment.
Do we need these headers? Seems pretty redundant with what we know from Git.
There was a problem hiding this comment.
They're pretty standard in Swift / Xcode (Android too). However they weren't following our convention, so I updated them to do so.
There was a problem hiding this comment.
It will only be replaced if the marketing version of the replacement is newer than the existing extension. This will happen when a user upgrades his/her client, for example.
So in order to downgrade, the user needs to completely uninstall Firezone first? Should we document this in the KB?
| public func requestNeedsUserApproval(_ request: OSSystemExtensionRequest) { | ||
| completionHandler?(SystemExtensionError.NeedsUserApproval) | ||
|
|
||
| // TODO: Inform the user to approve the system extension in System Preferences > Security & Privacy. |
There was a problem hiding this comment.
Does this need addressing?
There was a problem hiding this comment.
Not strictly required - the user is shown a macOS alert when the sysex is installed, so this might not be necessary.
We may need to handle the edge case where the user dismisses it, then tries to sign in anyway, so this TODO might not be in the relevant place here. Will circle back to it when I get the release / distributable build PR open.
|
Removed from queue in case you want to address any of the comments, feel free to re-queue. |
181de9f to
2949b47
Compare
2949b47 to
e39cbeb
Compare
e39cbeb to
cba9046
Compare
cba9046 to
cc89e47
Compare
jamilbk
left a comment
jamilbk
left a comment
There was a problem hiding this comment.
Addressed PR feedback!
There was a problem hiding this comment.
I'm not exactly sure yet -- need to test how that works on a live system.
We'll be able to test that after the PR for the release builds is finalized. The behavior of the SystemExtension with developer mode enabled and not is quite different unfortunately.
| public func requestNeedsUserApproval(_ request: OSSystemExtensionRequest) { | ||
| completionHandler?(SystemExtensionError.NeedsUserApproval) | ||
|
|
||
| // TODO: Inform the user to approve the system extension in System Preferences > Security & Privacy. |
There was a problem hiding this comment.
Not strictly required - the user is shown a macOS alert when the sysex is installed, so this might not be necessary.
We may need to handle the edge case where the user dismisses it, then tries to sign in anyway, so this TODO might not be in the relevant place here. Will circle back to it when I get the release / distributable build PR open.
| if let error = error { | ||
| Log.app.error("\(#function): Installing system extension failed! \(error.localizedDescription)") | ||
| } else { | ||
| self.startTunnel(options: options) | ||
| } |
There was a problem hiding this comment.
Unfortunately this just returns from the closure and not the outer function.
cc89e47 to
cf03b6b
Compare
Updated #7071 with a tasklist for tracking the work here. |
2 participants
To allow macOS users to rollback, it would be helpful to distribute a standalone macOS app, similar to how we distribute the GUI client.
The first step in this process is to refactor the macOS client to use a System Extension -based Network Extension rather than an App Extension based one. This offers us the flexibility to distribute the macOS client outside the Mac App Store in addition to via the store.
For this PR I focused on making the minimal set of changes necessary to support this change. This PR intentionally doesn't update the CI pipeline to notarize and attach a standalone bundle that will run ad-hoc on other Macs. That will come in a subsequent PR.
One thing to note about System Extensions is that they're slightly more finicky when it comes to getting the signing and packaging right. Thus, the README.md is updated to account for the gotchas involved in developing System Extensions locally.
Related: #7071.