fix(windows): lock down access to firezone ID file#13153
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
thomaseizinger
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds Windows-specific ACL hardening for the on-disk Firezone device ID (and its config directory) and refactors GUI telemetry initialization so it’s owned by the GUI Controller, with the Firezone ID provided via the tunnel-service IPC Hello message (avoiding direct device-id file access from the GUI on Windows).
Changes:
- Add Windows security descriptor support in
bin-sharedto apply restrictive DACLs to the device-id file and its parent directory, plus Windows-only unit tests. - Extend the tunnel-service → GUI IPC handshake (
ServerMsg::Hello) to includefirezone_id. - Move GUI telemetry lifecycle management from
firezone-gui-clientintoControllerand passtelemetry_allowedviaRunConfig.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/libs/bin-shared/src/device_id.rs | Adds Windows ACL application (SDDL → security descriptor → DACL set) and Windows-only tests for descriptor parsing/application. |
| rust/libs/bin-shared/Cargo.toml | Enables the Windows API surface needed for security descriptor authorization functions. |
| rust/gui-client/src-tauri/src/service.rs | Updates IPC protocol: Hello now carries firezone_id and the tunnel service sends it on connect. |
| rust/gui-client/src-tauri/src/gui.rs | Adds telemetry_allowed to RunConfig and forwards it into Controller::start. |
| rust/gui-client/src-tauri/src/controller.rs | Introduces MaybeTelemetry, starts/stops GUI telemetry in the controller, and consumes firezone_id from Hello. |
| rust/gui-client/src-tauri/src/bin/firezone-gui-client.rs | Removes main-process telemetry initialization and wires telemetry_allowed into GUI startup config. |
thomaseizinger
force-pushed
the
claude/safe-unsafe-abstractions-H74Hi
branch
from
55295d5 to
92a5856
Compare
- Embed `firezone_id` inside `Hello` so the GUI no longer waits for a separate `FirezoneId` follow-up message. - Combine `Option<Telemetry> + telemetry_allowed: bool` into a single `MaybeTelemetry` enum that owns the Firezone ID for the `Initialized` variant. This drops the awkward `let _ = &self.firezone_id;` workaround that only existed to silence unused-field warnings under `cfg(test)`. - Remove a stale comment in `firezone-gui-client.rs` that no longer reflects how the Controller starts up.
Move the unsafe Windows security descriptor calls into a private `windows_security` module that exposes a `SecurityDescriptor` type with a single safe constructor (`from_sddl`). Because `from_sddl` is the only way to obtain one, the `Drop` impl can soundly call `LocalFree` on the buffer returned by `ConvertStringSecurityDescriptorToSecurityDescriptorW`. - Pull both SDDL strings into named constants with rustdoc that decodes the meaning of each ACE. - Add unit tests that exercise the parsing, `Drop`, and `apply_to_path` paths so we have at least basic coverage of the unsafe FFI.
Mirror Swift's `Telemetry.start()` + `setUser` split: callers now start
a Sentry session in `entrypoint` mode before the Firezone ID is known
and bind the user later via `Telemetry::set_firezone_id`, instead of
waiting for the IPC `Hello { firezone_id }` round-trip. Crashes during
settings load, Tauri setup, IPC connect, and the Hello-wait window
are now captured.
Single `Telemetry::start(env_or_api_url, ..)` API: pass `"entrypoint"`
or an api URL. The new `Env::parse` tries env names first, then falls
back to `ApiUrl::new`, so the same code path handles the entrypoint
session and the eventual env-change reinit.
https://claude.ai/code/session_01RhwNJKLHxrzkXNNzCchVZe
`ConvertStringSecurityDescriptorToSecurityDescriptorW("")` succeeds on
Windows (it returns a descriptor with no DACL/SACL), so the assertion
that an empty string is rejected was wrong and tripped CI.
https://claude.ai/code/session_01RhwNJKLHxrzkXNNzCchVZe
thomaseizinger
force-pushed
the
claude/safe-unsafe-abstractions-H74Hi
branch
from
a77f5c3 to
e05a5a8
Compare
Member
Author
|
@jamilbk Verified on my Windows machine, the entire Downgrading from this to an earlier version is not going to work, I am pretty sure the client will fail to start because I can no longer read the ID file. Do we need to mention this somewhere? |
thomaseizinger
force-pushed
the
claude/safe-unsafe-abstractions-H74Hi
branch
from
0cd7143 to
b23eef9
Compare
thomaseizinger
pushed a commit
that referenced
this pull request
May 8, 2026
Pull the unsafe Windows security-descriptor wrappers from #13153 into a new `windows-security` workspace crate, then use it to replace the NULL DACL on every Firezone named pipe with one that grants only `LocalSystem`, `BUILTIN\Administrators`, and `BUILTIN\Users` (read/write/sync). Until now the Tunnel and GUI pipes accepted connections from any local principal — including Low-IL sandboxed processes, `NETWORK SERVICE`, and anonymous logons — because `SetSecurityDescriptorDacl(psd, true, None, false)` installs a NULL DACL. The new SDDL keeps the non-admin GUI working (the user is in `BU`) while excluding service identities and other unintended callers. The new crate is `cfg(windows)`-only at the source level so it builds to an empty rlib on Linux/macOS, leaving cross-platform CI green.
Tarang11
pushed a commit
to Tarang11/firezone
that referenced
this pull request
May 28, 2026
Currently, the named pipes used for IPC in the Windows GUI client do not have any DACLs set. This was originally done so we can run the GUI as a regular user and still connect to the privileged tunnel service. We can preserve this requirement and still harden access to the pipes by applying DACLs that prevent access from other service users like `NETWORK SERVICE` or `ANONYMOUS LOGON`. To implement this, we extract the safe abstraction around security descriptors introduced in firezone#13153 into a dedicated crate. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The firezone ID file on Windows is currently unprotected and can be read by all users on the system. This was initially necessary because we also needed to read the ID from the GUI process to initialise things like telemetry.
We lock the ID file to just be readable by the system user and local administrators. This however means we need to rework how our
Telemetrysetup works.Similarly to how we do this on macOS, we now initialize telemetry with an "entrypoint" environment when neither user ID nor environment is known. Once we learn the API URL from the settings and the user ID of IPC from the tunnel binary, we then update the telemetry context.