Refs #3713
With this, the deb package for the Linux GUI Client contains a build of
the Linux CLI Client, at `/usr/bin/firezone-client-tunnel`. Future PRs
can add IPC to the code.
There is also a Windows stub, since Windows will eventually need a
tunnel process and a CLI Client.
In the future we might need to move or rename things, since the CLI
Clients and tunnel binaries for both Linux and Windows may all share
code or at least architecture. For now there is a slight duplication
with this being built as both "Firezone Client Tunnnel" and "Firezone
Linux Client"
Motivated by: #4340.
I also activated
[`clippy::unnnecessary_wraps`](https://rust-lang.github.io/rust-clippy/master/#/unnecessary_wraps)
which does create some false-positives for the platform-specific code
but is IMO overall a net-positive. With the amount of Rust code and
crates increasing, it is good to have tools point out simplifications
like these as they are otherwise hard to spot, especially across crate
boundaries.
* Move the resource changes to `ClientState` to unit test easier
* Add unit tests
* Set new config on update from portal
* Set parameters as told by portal on re-init
Fixes: #2728
Supersedes #4320, closes#4318
Updates the interface if effective dns have changed.
Fixes a bug where we could set upstream_dns to have sentinel dns
Adds corresponding unit tests.
---------
Signed-off-by: Gabi <gabrielalejandro7@gmail.com>
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com>
- [x] Updated log level string for client and gateways to info or higher
- [x] Update logs to hide DNS information
I also removed `hickory_resolve` errors which could contain sensitive
info from our general error and hide the logs that specifically relates
to them.
@bmanifold double checking that the log levels in the gateway's `*.tf`
files are just used for our own gateways.
Also, the relays still have `debug`, since only we see that I think that
makes sense but double checking with @jamilbk
Fixes: #3618.
---------
Signed-off-by: Gabi <gabrielalejandro7@gmail.com>
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com>
This updates connlib to follow the new guidelines described in #4262. I
only made the bare-minimum changes to the clients. With these changes
`reconnect` should only be called when the network interface actually
changed, meaning clients have to be updated to reflect that.
Currently, a failure during DNS resolution results in the client hanging
during the connection setup. Instead, we fall back to an empty list
which results in an empty DNS query result for the client.
That in turn will make most application consider the DNS request failed.
As far as I know, we don't currently retry these DNS requests, meaning a
user would have to sign-in and out again to fix this state.
Whilst not ideal, I think this is a better behaviour and what we
currently have where the initial connection just hangs.
Running as sudo / root causes a lot of problems for GUI programs, so
we're unwinding that. In this case we can go back to using Tauri's "open
URL" function, which is great.
Closes#4103
Refs #3713
Affects #3972 - I was finally able to debug it because it came up
constantly during this PR
Currently, an error returned by `Tunnel::poll_next_event` is only
logged. In other words, they are never fatal. This creates a tricky to
understand relationship on what kind of errors should be returned from
callbacks. Because connlib is used on multiple operating systems, it has
no idea how fatal a particular error is.
This PR removes all of these `Result` return values with the following
consequences:
- For Android, we now panic when a callback fails. This is a slight
change in behaviour. I believe that previously, any exception thrown by
a callback into Android was caught and returned as an error. Now, we
panic because in the FFI layer, we don't have any information on how
fatal the error is. For non-fatal errors, the Android app should simply
not throw an exception. The panics will cause the connlib task to be
shut down which triggers an `on_disconnect`.
- For Swift, there is no behaviour change. The FFI layer already did not
support `Result`s for those callbacks. I don't know how exceptions from
Swift are translated across the FFI layer but there is no change to what
we had before.
- For the Tauri client:
- I chose to log errors on ERROR level and continue gracefully for the
DNS resolvers.
- We panic in case the controller channel is full / closed. That should
really never happen in practice though unless we are currently shutting
down the app.
Resolves: #4064.
In the future we will want to refactor this to a builder pattern to
prevent the number of parameters from growing and have them clearer but
this works simply for now.
Found while discussing #4174
---------
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Bumps [anyhow](https://github.com/dtolnay/anyhow) from 1.0.80 to 1.0.81.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/dtolnay/anyhow/releases">anyhow's
releases</a>.</em></p>
<blockquote>
<h2>1.0.81</h2>
<ul>
<li>Make backtrace support available when using -Dwarnings (<a
href="https://redirect.github.com/dtolnay/anyhow/issues/354">#354</a>)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="4aad4edebd"><code>4aad4ed</code></a>
Release 1.0.81</li>
<li><a
href="8be90917c6"><code>8be9091</code></a>
Merge pull request <a
href="https://redirect.github.com/dtolnay/anyhow/issues/354">#354</a>
from dtolnay/deadcode</li>
<li><a
href="a2eb7dd5e1"><code>a2eb7dd</code></a>
Make compatible with -Dwarnings</li>
<li>See full diff in <a
href="https://github.com/dtolnay/anyhow/compare/1.0.80...1.0.81">compare
view</a></li>
</ul>
</details>
<br />
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
</details>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This isn't really user-facing, so I marked it down from `feat` to
`chore`. Closes#3817
- If we exit gracefully, `/etc/resolv.conf` is reverted
- We always keep the `.before-firezone` backup in case we lose power and
the revert transaction is corrupted or rolled back
- We use a magic header to detect whether the last run was a crash or
not. If Firezone crashes and the user wants to modify their default DNS,
they need to delete that header so that Firezone won't accidentally
revert its backup and trash their change.
- All error variants for this module replaced with `anyhow::Error` since
they were never matched by callers.
I ran `cargo mutants` locally and it helped me validate the unit tests
and it picked up a `match` branch that I forgot to delete.
```[tasklist]
- [x] (Failed: Integration tests didn't like it) ~~Add the system default resolvers below Firezone's sentinels~~
- [x] `tracing::info` "Last run crashed" if we have to revert the file at startup
```
---------
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
This refactors `Session` to allow for commands to be sent to the
`Eventloop`. Currently, we only send a `Stop` command. With #3429, we
will add more commands like refreshing and updating the DNS servers.
Currently, we are passing a lot of data into `Session::connect`. Half of
this data is only needed to construct the URL we will use to connect to
the portal. We can simplify this by extracting a dedicated `LoginUrl`
component that captures and validates this data early.
Not only does this reduce the number of parameters we pass to
`Session::connect`, it also reduces the number of failure cases we have
to deal with in `Session::connect`. Any time the session fails, we have
to call `onDisconnected` to inform the client. Thus, we should perform
as much validation as we can early on. In other words, once
`Session::connect` returns, the client should be able to expect that the
tunnel is starting.
Closes#3815
Changes that are breaking (but these aren't in production so it should
be okay)
- Windows, renaming `device_id.json` to `firezone-id.json` to match the
rest of the code
- Linux GUI, storing the firezone-id under `/var/lib` instead of under
`$HOME`
- Linux GUI, bails out if not run with `sudo --preserve-env` by
detecting `$HOME == root` or `$USER != root`
---------
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Previously, we called `onDisconnect` in two kinds of situations:
- With an error when we wanted the clients to clear the token
- Without an error when the token was still valid (i.e. after a call to
`disconnect` from the clients)
This is unnecessarily redundant. Firezone is designed to **not** have a
state of "signed in but disconnected". Thus, every time connlib calls
`disconnect`, we should clear the token and sign the user out.
At present, we only do this for errors with the control plane. Errors in
the actual tunnel are only logged and we continue trying to use the
tunnel. There are errors in the tunnel where we should also give up
(i.e. TUN device gone, fatal IO error, etc). At present, those are not
yet bubbled up but we will at some point. Once we have
https://github.com/firezone/firezone/pull/3682, it will be much easier
to create a type-safe contract that ensures we only disconnect on fatal
errors.
---------
Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>
Co-authored-by: ReactorScram <ReactorScram@users.noreply.github.com>
Bumps [base64](https://github.com/marshallpierce/rust-base64) from
0.21.7 to 0.22.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md">base64's
changelog</a>.</em></p>
<blockquote>
<h1>0.22.0</h1>
<ul>
<li><code>DecodeSliceError::OutputSliceTooSmall</code> is now
conservative rather than precise. That is, the error will only occur if
the decoded output <em>cannot</em> fit, meaning that
<code>Engine::decode_slice</code> can now be used with exactly-sized
output slices. As part of this, <code>Engine::internal_decode</code> now
returns <code>DecodeSliceError</code> instead of
<code>DecodeError</code>, but that is not expected to affect any
external callers.</li>
<li><code>DecodeError::InvalidLength</code> now refers specifically to
the <em>number of valid symbols</em> being invalid (i.e. <code>len % 4
== 1</code>), rather than just the number of input bytes. This avoids
confusing scenarios when based on interpretation you could make a case
for either <code>InvalidLength</code> or <code>InvalidByte</code> being
appropriate.</li>
<li>Decoding is somewhat faster (5-10%)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="5d70ba7576"><code>5d70ba7</code></a>
Merge pull request <a
href="https://redirect.github.com/marshallpierce/rust-base64/issues/269">#269</a>
from marshallpierce/mp/decode-precisely</li>
<li><a
href="efb6c006c7"><code>efb6c00</code></a>
Release notes</li>
<li><a
href="2b91084a31"><code>2b91084</code></a>
Add some tests to boost coverage</li>
<li><a
href="9e9c7abe65"><code>9e9c7ab</code></a>
Engine::internal_decode now returns DecodeSliceError</li>
<li><a
href="a8a60f43c5"><code>a8a60f4</code></a>
Decode main loop improvements</li>
<li><a
href="a25be0667c"><code>a25be06</code></a>
Simplify leftover output writes</li>
<li><a
href="9979cc33bb"><code>9979cc3</code></a>
Keep morsels as separate bytes</li>
<li><a
href="37670c5ec2"><code>37670c5</code></a>
Bump dev toolchain version (<a
href="https://redirect.github.com/marshallpierce/rust-base64/issues/268">#268</a>)</li>
<li>See full diff in <a
href="https://github.com/marshallpierce/rust-base64/compare/v0.21.7...v0.22.0">compare
view</a></li>
</ul>
</details>
<br />
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
</details>
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Closes#3879 and #3902
I re-created Cargo.lock, so it incidentally updated a bunch of other
stuff. I can revert that file if it's a problem.
Had to search a bit for the breaking changes. Found here that they
renamed `ComInterface`:
https://github.com/microsoft/windows-rs/issues/2875#issuecomment-1962332067
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Part of a small yak shave for #3817
The `atomicwrites` lib uses the atomic rename trick and does correct
fsyncs for us, so if we lose power while rewriting or reverting
`/etc/resolv.conf`, it should always be in a recoverable state. (Unless
the hard drive lies about fsync, but then it's beyond our control.)
---------
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Bumps [tempfile](https://github.com/Stebalien/tempfile) from 3.10.0 to
3.10.1.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/Stebalien/tempfile/blob/master/CHANGELOG.md">tempfile's
changelog</a>.</em></p>
<blockquote>
<h2>3.10.1</h2>
<ul>
<li>Handle potential integer overflows in 32-bit systems when
seeking/truncating "spooled" temporary files past 4GiB
(2³²).</li>
<li>Handle a theoretical 32-bit overflow when generating a temporary
file name larger than 4GiB. Now it'll panic (on allocation failure)
rather than silently succeeding due to wraparound.</li>
</ul>
<p>Thanks to <a
href="https://github.com/stoeckmann"><code>@stoeckmann</code></a> for
finding and fixing both of these issues.</p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="094c115110"><code>094c115</code></a>
chore: release 3.10.1</li>
<li><a
href="56c593477f"><code>56c5934</code></a>
Fix integer overflows and truncation (<a
href="https://redirect.github.com/Stebalien/tempfile/issues/278">#278</a>)</li>
<li><a
href="5a949d6e75"><code>5a949d6</code></a>
chore: 2021 edition (<a
href="https://redirect.github.com/Stebalien/tempfile/issues/276">#276</a>)</li>
<li>See full diff in <a
href="https://github.com/Stebalien/tempfile/compare/v3.10.0...v3.10.1">compare
view</a></li>
</ul>
</details>
<br />
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
</details>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The reference that is specified as part of the connection intent
fulfills one particular purpose: To avoid accepting connection details
for a "stale" intent, i.e. a previous one that we sent for the same
resource.
With the move to `phoenix-channel` in #3682, we can no longer specify
the reference explicitly. Instead, sending a message to the portal gives
us an `OutboundRequestId`.
To make the transition in #3682 easier, we emulate this behaviour here
temporarily in the `ControlPlane` of the clients.
This PR doesn't yet provide support for the update of upstream DNS but
it does provide support for all the other resources update messages.
Should comply with the description of issue #2022 but it doesn't respond
to DNS upstream updates which is imply it should on the issue title
---------
Signed-off-by: Gabi <gabrielalejandro7@gmail.com>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
It is still client-specific, but this was the closest place I could find
in connlib to put it.
A hypothetical GUI / .deb / systemd-involved gateway would need to be
"dev.firezone.gateway"
Extracted out of #3391.
We don't actually need this for #3391 though because we've added a
compatibility layer during deserialization. But, it will be good to
remove that compat layer at some point which means we have to return the
addresses as plain socket addresses. Because that is a breaking change,
I decided to extract this into a different PR.
Co-authored-by: conectado <gabrielalejandro7@gmail.com>
---------
Co-authored-by: conectado <gabrielalejandro7@gmail.com>
If `FIREZONE_DNS_CONTROL` is set to `systemd-resolved`, then shell out
to `resolvectl` to request all system DNS queries to go to Firezone's
sentinel DNS server(s).
```[tasklist]
- [ ] Figure out how to stop the runner from using the Docker bridge iface
```
---------
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Reverts #3622 I don't know why, but that change seemed to cause the
`/etc/resolv.conf` test to fail in CI and I was thinking of the "roll
back first" principle
https://cloud.google.com/blog/products/gcp/reliable-releases-and-rollbacks-cre-life-lessons
~~I also change one `ping` in CI to `until ping`. This was an earlier
attempt before I did the revert, and it seems safe to leave it in.~~
With #3391, constructing a new tunnel will no longer be `async` which
makes DNS resolution the only `async` component of
`set_peer_connection_request`. In general, adding resources as part of
setting up a connection is a duplicated of the logic within
`allow_access`.
We solve both of these problems at once by moving the DNS resolution out
of `connlib` into the `gateway` binary and perform it as part of the
eventloop during a connection setup.
Only user-facing if users are using the Docker image for the Linux
client.
I split off a module for `/etc/resolv.conf` since the code and unit
tests are about 300 lines and aren't related to the rest of the
`tun_linux.rs` code.
---------
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
The Docker image for the client is opted in to this new feature. The
bare `linux-client-x64` exe is not. I don't know if users are using the
Docker images?
I wanted to use CLI args, but the DNS control code ("config" or
"control"? Or "SplitDNS"?) has to run at the end of `set_iface_config`,
which on Linux runs in a worker, so I couldn't figure out how to move it
into `on_set_interface_config` in the callbacks. Maybe there is a way,
but the env var results in a small diff.
This splits off the easy parts from #3605.
- Add quotes around `PHOENIX_SECURE_COOKIES` because my local
`docker-compose` considers unquoted 'false' to be a schema error - Env
vars are strings or numbers, not bools, it says
- Create `test.httpbin.docker.local` container in a new subnet so it can
be used as a DNS resource without the existing CIDR resource picking it
up
- Add resources and policies to `seeds.exs` per #3342
- Fix warning about `CONNLIB_LOG_UPLOAD_INTERVAL_SECS` not being set
- Add `resolv-conf` dep and unit tests to `firezone-tunnel` and
`firezone-linux-client`
- Impl `on_disconnect` in the Linux client with `tracing::error!`
- Add comments
```[tasklist]
- [x] (failed) Confirm that the client container actually does stop faster this way
- [x] Wait for tests to pass
- [x] Mark as ready for review
```
Currently, we log messages from the portal several times via different
ways. For one, the message is included in the span via
`tracing::instrument`. Then it is also logged on `trace!` level twice
(this PR removes one of them). Plus, the fields of the
`request_connection` span are not printed in a very human-readable way
(bytes arrays). This makes the logs super noisy, see here for the latest
run on `main`:
https://github.com/firezone/firezone/actions/runs/7808301643/job/21298585685#step:13:13
Compare this with the logs from the run in this PR:
https://github.com/firezone/firezone/actions/runs/7813774334/job/21313812863?pr=3596#step:13:13
Some of these improvements were made as part of debugging #3391.
Extracting them here in a separate PR to reduce the diff of #3391.
