refactor(connlib): tidy-up tech-debt around mangling of forwarded DNS queries #5391
Labels
area/connlib
Firezone's core connectivity library
complexity/medium
Something that should not take more than a day or two.
kind/refactor
Code refactoring
With #5049, we are more explicitly modelling connlib's DNS handling using a
StubResolver
. TheStubResolver
looks at every incoming packet and, in case it is a DNS query for one of our resources, send a DNS response.In case it isn't for one of our DNS resources, it doesn't touch the packet but instead emits a
ResolveStrategy::ForwardQuery
. Forwarding a query means we need to contact another DNS server to resolve it.hickory-dns
library to contact the original DNS server (i.e. the one that is configured either on the system or in the portal).For historical reasons, connlib models this forwarding a bit awkwardly:
In the first case, the query packet needs to be mangled to point to the actual CIDR resource. However, this isn't done by
StubResolver
but instead happens only once we've already selected the gateway: https://github.com/firezone/firezone/blob/cba64355a42b7a2ae74313d30c9fc4741b33dbd0/rust/connlib/tunnel/src/client.rs#L426-L431In the second case, we don't use the original
IpPacket
for much other than selecting, which upstream DNS server to contact: https://github.com/firezone/firezone/blob/cba64355a42b7a2ae74313d30c9fc4741b33dbd0/rust/connlib/tunnel/src/io.rs#L130-L135This is the same logic as mangling the packet for case 1! If
StubResolver
would mangle theIpPacket
right away,Io
would not need to know about the mapping of proxy IPs -> upstream servers, improving the encapsulation ofClientState
.Additionally, this would allow moving the state tracking of
mangled_dns_queries
intoStubResolver
, further simplifyingClientState
. IfStubResolver
does the mangling, it should also own the state created as part of it. As a consequence,StubResolver
will also need to know about responses coming back either via the tunnel (from DNS servers that are CIDR resources) or from hickory (for DNS servers that are not). This makes sense because a stub resolver sends and receives its own queries (to upstream DNS servers). To include this in connlib's model,StubResolver
could adopt a similar API assnownet::Node
andClientOnGateway
where a packets are inbound and outbound packets are transformed. Those are currently calledencapsulate
anddecapsulate
although that naming isn't quite as good forStubResolver
.As a final, very intertwined piece,
dns_mapping
should be moved intoStubResolver
. Conceptually, these are the DNS servers that theStubResolver
will contact in case it cannot answer the query itself (because the query isn't for one of our DNS resources). This state should be owned byStubResolver
and notClientState
.The text was updated successfully, but these errors were encountered: