The downcasting abilities of `anyhow` are pretty powerful.
Unfortunately, they can also be a bit tricky to get right. Whilst `is`
and `downcast` work fine for any errors that are within the `anyhow`
error chain, they don't check the chain of errors prior to that. In
other words, if we already have a nested `std::error::Error` with
several causes, `anyhow` cannot downcast to these causes directly.
In order to avoid this footgun, we create a thin-layer on top of the
`anyhow` crate with some downcasting functions that always try to do the
right thing.
Building on top of a series of refactors and smaller features, this PR
enables connlib to send DNS queries over HTTPS to one or more configured
DoH providers.
A DoH server itself is addressed via a domain which first needs to be
resolved before it can be contacted. The RFC recommends to perform this
bootstrapping using the system DNS resolvers. For connlib, this is a bit
tricky because the system resolvers may already be set to connlib's
sentinel servers by the time we need to bootstrap the DoH clients.
Therefore, we maintain a dedicated UDP DNS client inside connlib's `Io`
component which is always configured with the latest system DNS
resolvers known to connlib.
The actual bootstrapping of a DoH client happens in the following cases:
1. Our TUN device configuration changes and the configured DNS servers
mapping contains DoH upstreams.
2. We need to make a DNS query to a DoH server but don't have a client
yet.
The first case ensures we bootstrap the DoH clients as early as
possible. The latter case ensures we have a self-healing behaviour in
case the TCP connection to the DoH server breaks (in which case the DoH
client will be de-allocated).
Once the DoH client is initialized, making queries with it is a trivial
act of sending an HTTP request and parsing the HTTP response. Within
connlib, this now requires almost no special handling apart from a new
`dns::Upstream` type that differentiates between Do53 servers (addressed
by a `SocketAddr`) and DoH servers (addressed by a `Url`).
Related: #10764
Related: #10788
Related: #10850
Related: #10851
Related: #10856
Related: #10857
Related: #10871
Related: #10872
Related: #10875
Related: #10881Resolves: #10790
In #10817, connlib gained the ability to re-resolve the portal's
hostname on WebSocket connection hiccups. The list of upstream servers
used for that may contain sentinel DNS server IPs on certain systems if
connlib's DNS control is currently active. Connlib filters these servers
internally before computing the effective list of upstream servers.
The DNS client used by the event-loop contacts all servers in the list
but waits for at most 2s before merging all received records together.
If there are upstream DNS servers defined in the portal and those are
also resources which we are currently not connected to, querying these
servers would trigger a message to the portal, forming a circular
dependency. This circular dependency is only broken by the 2s timeout.
Whilst not fatal for connlib's functionality, it means that in such a
situation, reconnecting to the portal always has to wait for this
timeout.
To fix this, we first apply the system DNS resolvers to connlib and only
pass the now returned sanitized list on to the DNS client.
Related: #10854
---------
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: thomaseizinger <5486389+thomaseizinger@users.noreply.github.com>
In order to bootstrap DoH servers, we need a way of reliably resolving
the domain of the DoH server to an IP address. Initially, I thought that
this would be tricky to do if we have to integrate this into the
Client's state machine.
Whilst implementing DoH however, I realised that we can instead put this
responsibility onto the IO layer of connlib. Similar to other cases, we
can reuse external triggers as our retry mechanism in case of failure.
In particular, we can simply issue UDP DNS queries for the DoH domain to
all system-defined DNS resolvers every time we are told to send a DNS
query over DoH but the corresponding client isn't initialized yet.
In other words, instead of building a retry mechanism ourselves, we
attempt to repair any kind of broken state once per DNS query that we
receive.
Performing this DNS resolution does require a bit of code. We already
started to do something similar in #10817. In order to reuse that code,
we extract it into a `l4-udp-dns-client` crate and slightly refactor its
semantics. In particular, we now wait for the response of all upstream
servers (but at most 2s) and combine the result.
The resulting `UdpDnsClient` can now be used inside the Client's
event-loop to re-resolve the portal URL and will also be used as part of
our DoH implementation to bootstrap the connection to the DoH server.
Related: #4668
In #10817, we landed a fix that allows Clients to re-resolve the portal
URL every time the WebSocket connection fails. Currently, we use the
active upstream resolvers for this.
This can lead to a kind of deadlock in case the upstream resolver is a
CIDR resource that we are not yet connected to. In that case, we'd need
a connection to the portal to establish a connection to the Gateway.
By always using the system resolvers for this, we avoid this circular
dependency.
Currently, the DNS records for the portal's hostname are only resolved
during startup. When the WebSocket connection fails, we try to reconnect
but only with the IPs that we have previously resolved. If the local IP
stack changed since then or the hostname now points to different IPs, we
will run into the reconnect-timeout configured in `phoenix-channel`.
To fix this, we re-resolve the portal's hostname every time the
WebSocket connection fails. For the Gateway, this is easy as we can
simply reuse the already existing `TokioResolver` provided by hickory.
For the Client, we need to write our own DNS client on top of our socket
factory abstraction to ensure we don't create a routing loop with the
resulting DNS queries. To simplify things, we only send DNS queries over
UDP. Those are not guaranteed to succeed but given that we do this on
every "hiccup", we already have a retry mechanism. We use the currently
configured upstream DNS servers for this.
Resolves: #10238
This PR eliminates JSON-based communication across the FFI boundary,
replacing it with proper
uniffi-generated types for improved type safety, performance, and
reliability. We replace JSON string parameters with native uniffi types
for:
- Resources (DNS, CIDR, Internet)
- Device information
- DNS server lists
- Network routes (CIDR representation)
Also, get rid of JSON serialisation in Swift client IPC in favour of
PropertyList based serialisation.
Fixes: https://github.com/firezone/firezone/issues/9548
---------
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Building on top of #10507, setting the initial Internet Resource state
is a piece of cake. All we need to do is thread a boolean variable
through to all call-sites of `Session::connect`. Without the need for
the Internet Resource's ID, we can simply pass in the boolean that is
saved in the configuration of each client.
Resolves: #10255
Instead of the generic "disable any kind of resource"-functionality that
connlib currently exposes, we now provide an API to only enable /
disable the Internet Resource. This is a lot simpler to deal with and
reason about than the previous system, especially when it comes to the
proptests. Those need to model connlib's behaviour correctly across its
entire API surface which makes them unnecessarily complex if we only
ever use the `set_disabled_resources` API with a single resource.
In preparation for #4789, I want to extend the proptests to cover
traffic filters (#7126). This will make them a fair bit more
complicated, so any prior removal of complexity is appreciated.
Simplifying the implementation here is also a good starting point to fix
#10255. Not implicitly enabling the Internet Resource when it gets added
should be quite simple after this change.
Finally, resolving #8885 should also be quite easy. We just need to
store the state of the Internet Resource once per API URL instead of
globally.
Resolves: #8404
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In #10347, we made sure that we always return all errors that happen
during a single tick of the event-loop. What we overlooked is that as
part of handling the errors, we need to use `continue` to jump to the
next one instead of returning directly from the function.
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
In #10076, connlib gained the ability to gracefully close connections
between peers. The Gateway already uses this when it is being gracefully
shutdown such as during an upgrade. This allows Clients to immediately
fail-over to a different Gateway instead of waiting for an ICE timeout.
When a Client signs out, we currently just drop all the state, resulting
in an ICE timeout on the Gateway ~15 seconds later. This makes it
difficult for us to analyze, whether an ICE timeout in the logs presents
an actual problem where a network connection got cut or whether the
Client simply signed out.
Whilst not water-tight, attempting to gracefully close our connections
when the Client signs out is better than nothing so we implement this
here.
All Clients use the `Session` abstraction from `client-shared` which
spawns the event-loop into a dedicated task.
- For the Linux and Windows GUI client, the already present tokio
runtime instance of the tunnel service is used for this.
- For Android and Apple, we create a dedicated, single-threaded runtime
instance for connlib.
- For the headless client, we also reuse the already existing tokio
runtime instance of the binary.
In case of Android, Apple and the headless client, this means we need to
ensure the tokio runtime instances stays alive long enough to actually
complete the graceful shutdown task. We achieve this by draining the
`EventStream` returned from `Session`. The `EventStream` is a wrapper
around a channel connected to the event-loop. This stream only finishes
once the event-loop is entirely dropped (and therefore completed the
graceful shutdown) as it holds the sender-end of the channel.
In case of the Linux and Windows GUI client, the runtime outlives the
`Session` because it is scoped to the entire tunnel process. Therefore,
no additional measures are necessary there to ensure the graceful
shutdown task completes.
A `Future` in Rust should not be polled once it has been completed as
that may lead to panics or otherwise undesirable behaviour. To avoid
this, a `Future` can be `fuse`d which will make it return
`Poll::Pending` indefinitely after it has returned `Ready`.
We have received several Sentry alerts of poll-after-completion panics
that I believe are all stemming from this particular code.
The event-loop inside `Tunnel` processes input according to a certain
priority. We only take input from lower priority sources when the higher
priority sources are not ready. The current priorities are:
- Flush all buffers
- Read from UDP sockets
- Read from TUN device
- Read from DNS servers
- Process recursive DNS queries
- Check timeout
The idea of this priority ordering is to keep all kinds of processing
bounded and "finish" any kind of work that is on-going before taking on
new work. Anything that sits in a buffer is basically done with
processing and just needs to be written out to the network / device.
Arriving UDP packets have already traversed the network and been
encrypted on the other end, meaning they are higher priority than
reading from the TUN device. Packets from the TUN device still need to
be encrypted and sent to the remote.
Whilst there is merit in this design, it also bears the potential of
starving input sources further down if the top ones are extremely busy.
To prevent this, we refactor `Io` to read from all input sources and
present it to the event-loop as a batch, allowing all sources to make
progress before looping around. Since this event-loop has first been
conceived, we have refactored `Io` to use background threads for the UDP
sockets and TUN device, meaning they will make progress by themselves
anyway until the channels to the main-thread fill up. As such, there
shouldn't be any latency increase in processing packets even though we
are performing slightly more work per event-loop tick.
This kind of batch-processing highlights a problem: Bailing out with an
error midway through processing a batch leaves the remainder of the
batch unprocessed, essentially dropping packets. To fix this, we
introduce a new `TunnelError` type that presents a collection of errors
that we encountered while processing the batch. This might actually also
be a problem with what is currently in `main` because we are already
batch-processing packets there but possibly are bailing out midway
through the batch.
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Mariusz Klochowicz <mariusz@klochowicz.com>
In earlier versions of Firezone, the WebSocket protocol with the portal
was using the request-response semantics built into Phoenix. This
however is quite cumbersome to work with to due to the polymorphic
nature of the protocol design.
We ended up moving away from it and instead only use one-way messages
where each event directly corresponds to a message type. However, we
have never removed the capability reply messages from the
`phoenix-channel` module, instead all usages just set it to `()`.
We can simplify the code here by always setting this to `()`.
Resolves: #7091
When we receive a DNS query for a DNS resource in Firezone, we take the
next available 4 IPs from the CG-NAT range and assign them to the domain
name. For example, if `example.com` is a DNS resource and it is the
first resource being queried in a Firezone session, we will assigned the
IPs `100.96.0.1` - `100.96.0.4` to it. If the user now restarts Firezone
or signs out and back in, this state is lost and we assign those same
IPs to the next DNS query coming in.
This creates a problem for applications that do not re-query DNS very
often or never. They expect these IPs to not change. Restarting software
or signing out and back in is a common approach to fixing software
problems, yet in this specific case, doing so may create even more
problems for the user.
To mitigate this, `ClientState` introduce a new event
`DnsRecordsChanged` that gets emitted to the event-loop every time we
assign new records. The event-loop then caches this in memory and reuses
it in case a new session is initiated. The records are only stored
in-memory and not on disk. Most likely, the tunnel process will be alive
for the entire OS session.
To verify this behaviour, we add a new `RestartClient` transition to our
proptests. In the proptests, we already keep a mapping of all DNS names
we ever resolved, including DNS resources. When generating IP traffic,
we sample from this list of IPs and then expect the packet to be routed.
By replacing the `ClientState` as part of this transition and re-seeding
it with the previously exported DNS records, we can verify that packets
to IPs resolved from a previous session still get successfully routed to
the resource.
Related: #5498
Right now, connections cannot be actively closed in Firezone. The
WireGuard tunnel and the ICE agent are coupled together, meaning only if
either one of them fails will we clean up the connection. One exception
here is when the Client roams. In that case, the Client simply clears
its local memory completely and then re-establishes all necessary
connections by re-requesting access.
There are three cases where gracefully closing a connection is useful:
1. If an access authorization is revoked or expires and this was the
last resource authorisation for that peer, we don't currently remove the
connection on the Gateway. Instead, the Client is still able to send
packets by they'll be dropped because we don't have a peer state
anymore.
1. If a Gateway gets restarted due to e.g. an upgrade or other
maintenance work, it loses all its connections and every Client needs to
wait for the ICE timeout (~15 seconds) before it can establish a new
one.
1. If a Client has its access revoked for all resources it has access to
in a particular site we also don't remove this connection, even though
it has become practically useless.
All of these cases are fixed with this PR. Here we introduce a way to
gracefully shutdown a connection without forcing the other side into an
ICE timeout. The graceful connection shutdown works by introducing a new
"goodbye" p2p control protocol message. Like all our p2p control
protocol messages, this is based on IP and therefore delivery is not
guaranteed. In other words, this "goodbye" message is sent on a
best-effort basis.
In the case of shutdown, the Gateway will wait for all UDP packets to be
flushed but will not resend them or wait for an ACK.
If either end receives such a "goodbye" message, they simply remove the
local peer and connection state just as if the connection would have
failed due to either ICE or WireGuard. For the Client, this means that
the next packet for a resource will trigger a new access authorization
request.
Right now, the Client event-loops have a channel with 1000 items for
sending new resource lists and updates to the TUN device to the host
app. This is kind of unnecessary as we always only care about the last
version of these. Intermediate updates that the host app doesn't process
are effectively irrelevant.
We've had an issue before where a bug in the portal caused us to receive
many updates to resources which ended up crashing Client apps because
this channel filled up.
To be more resilient on this front, we refactor the Client event loop to
use a `watch` channel for this. Watch channels only retain the last
value that got sent into them.
We cannot poll the `PhoenixChannel` after it has returned an error,
otherwise it will panic. Therefore, we exit the event-loop then. The
outer event-loop also exits as soon as it receives an error from this
channel so this is fine.
`PhoenixChannel` only returns an error when it has irrecoverably
disconnected, e.g. after the retries have been exhausted or we hit a 4xx
error on the WebSocket connection.
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Currently, `connlib`'s event-loop for clients uses manual polling to
advance the state of the tunnel and the phoenix-channel. Manual polling
is powerful but also easy to get wrong, resulting in task-wakeup bugs.
Additionally, if the tunnel is very busy with processing packets, the
phoenix-channel may not get enough CPU time, resulting in a loss of the
WebSocket connection.
To fix this, we move the phoenix-channel to a separate task and use
channels to connect it with `connlib`'s main event-loop. This one is now
primarily focused on advancing the tunnel state, effectively offloading
the problem of fair scheduling to the tokio runtime.
Related: #10003
When filtering through logs in Sentry, it is useful to narrow them down
by context of a client, gateway or resource. Currently, these fields are
sometimes called `client`, `cid`, `client_id` etc and the same for the
Gateway and Resources.
To make this filtering easier, name all of them `cid` for Client IDs,
`gid` for Gateway IDs and `rid` for Resource IDs.
These appear to happen on systems that e.g. don't have IPv6 support or
where the destination cannot be reached. It is a bit of a catch-all but
all the ones I am seeing in Sentry are false-positives. To reduce the
noise a bit, we log these on DEBUG now.
In Docker environments, applying iptables rules to filter
container-container traffic on the Docker bridged network is not
reliable, leading to direct connections being established in our relayed
tests. To fix this, we insert the rules directly from the client
container itself.
---------
Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>
Certain UNIX systems such as macOS also use the EHOSTDOWN error to
signal that a packet cannot be sent to a certain IP. There is nothing we
can do about this error so we downgrade it from a WARN to a DEBUG like
we do for other kinds of "unreachable" errors.
Customer hit what seems to be a rare race condition where we try to
connect whilst we already have a session. I don't know which state it is
in so I am replacing it with a WARN log to learn more about this in
Sentry in case it gets hit again.
To make our FFI layer between Android and Rust safer, we adopt the
UniFFI tool from Mozilla. UniFFI allows us to create a dedicated crate
(here `client-ffi`) that contains Rust structs annotated with various
attributes. These macros then generate code at compile time that is
built into the shared object. Using a dedicated CLI from the UniFFI
project, we can then generate Kotlin bindings from this shared object.
The primary motivation for this effort is memory safety across the FFI
boundary. Most importantly, we want to ensure that:
- The session pointer is not used after it has been free'd
- Disconnecting the session frees the pointer
- Freeing the session does not happen as part of a callback as that
triggers a cyclic dependency on the Rust side (callbacks are executed on
a runtime and that runtime is dropped as part of dropping the session)
To achieve all of these goals, we move away from callbacks altogether.
UniFFI has great support for async functions. We leverage this support
to expose a `suspend fn` to Android that returns `Event`s. These events
map to the current callback functions. Internally, these events are read
from a channel with a capacity of 1000 events. It is therefore not very
time-critical that the app reads from this channel. `connlib` will
happily continue even if the channel is full. 1000 events should be more
than sufficient though in case the host app cannot immediately process
them. We don't send events very often after all.
This event-based design has major advantages: It allows us to make use
of `AutoCloseable` on the Kotlin side, meaning the `session` pointer is
only ever accessed as part of a `use` block and automatically closed
(and therefore free'd) at the end of the block.
To communicate with the session, we introduce a `TunnelCommand` which
represents all actions that the host app can send to `connlib`. These
are passed through a channel to the `suspend fn` which continuously
listens for events and commands.
Resolves: #9499
Related: #3959
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>
These don't happen very often so are safe to log on INFO. That is the
default log level and it is useful to see, why we are re-connecting to
the portal.
As relict from very early designs of `connlib`, the `Callbacks` trait is
still present and defines how the host app receives events from a
running `Session`. Callbacks are not a great design pattern however
because they force the running code, i.e. `connlib`s event-loop to
execute unknown code. For example, if that code panics, all of `connlib`
is taken down. Additionally, not all consumers may want to receive
events via callbacks. The GUI and headless client for example already
have their own event-loop in which they process all kinds of things.
Having to deal with the `Callbacks` interface introduces an odd
indirection here.
To fix this, we instead return an `EventStream` when constructing a
`Session`. This essentially aligns the API of `Session` with that of a
channel. You receive two handles, one for sending in commands and one
for receiving events. A `Session` will automatically spawn itself onto
the given runtime so progress is made even if one does not poll on these
channel handles.
This greatly simplifies the code:
- We get to delete the `Callbacks` interface.
- We can delete the threaded callback adapter. This was only necessary
because we didn't want to block `connlib` with the handling of the
event. By using a channel for events, this is automatically guaranteed.
- The GUI and headless client can directly integrate the event handling
in their event-loop, without having to create an indirection with a
channel.
- It is now clear that only the Apple and Android FFI layers actually
use callbacks to communicate these events.
- We net-delete 100 LoC
The name IPC service is not very descriptive. By nature of being
separate processes, we need to use IPC to communicate between them. The
important thing is that the service process has control over the tunnel.
Therefore, we rename everything to "Tunnel service".
The only part that is not changed are historic changelog entries.
Resolves: #9048
The current `rust/` directory is a bit of a wild-west in terms of how
the crates are organised. Most of them are simply at the top-level when
in reality, they are all `connlib`-related. The Apple and Android FFI
crates - which are entrypoints in the Rust code are defined several
layers deep.
To improve the situation, we move around and rename several crates. The
end result is that all top-level crates / directories are:
- Either entrypoints into the Rust code, i.e. applications such as
Gateway, Relay or a Client
- Or crates shared across all those entrypoints, such as `telemetry` or
`logging`
