Skip to content

Conversation

@thomaseizinger
Copy link
Member

When encountering a PTR query, connlib checks if the query is for a Firezone-managed resource and resolve it to the correct IP. If it isn't for a DNS resource, we should forward the query to the upstream resolver.

This isn't what is currently happening though. Instead of forwarding the query, we bail early from StubResolver::handle and thus attempt to route the packet through the tunnel. This however fails because the DNS query was targeted at connlib's stub resolver address which never corresponds to a resource IP.

When TRACE logs where activated, this resulted in several entries such as

Unknown resource dst=100.100.111.1

To ensure this doesn't regress, we now generate PTR and MX record queries in tunnel_test. We don't assert the response of those but we do assert that we always get a response. The inclusion of MX records asserts that unknown query types get correctly forwarded.

Resolves: #6749.

@vercel
Copy link

vercel bot commented Sep 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 10:34pm

Copy link
Contributor

@conectado conectado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This could be causing issues with DNS in a lot of cases

@thomaseizinger
Copy link
Member Author

This isn't what is currently happening though. Instead of forwarding the query, we bail early from StubResolver::handle and thus attempt to route the packet through the tunnel. This however fails because the DNS query was targeted at connlib's stub resolver address which never corresponds to a resource IP.

I'll send a follow-up where we I want to refactor the function signature to explicitly track the cases of "packet for our DNS resolver but failed otherwise".

@thomaseizinger
Copy link
Member Author

I ran 10000 proptest cases locally. Appears to be stable :)

@thomaseizinger thomaseizinger added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit 8bac75b Sep 18, 2024
@thomaseizinger thomaseizinger deleted the fix/connlib-forward-non-resource-ptr branch September 18, 2024 23:01
prop_oneof![Just(Rtype::A), Just(Rtype::AAAA)]
fn ptr_query_ip() -> impl Strategy<Value = IpAddr> {
prop_oneof![
host_v4(IPV4_RESOURCES).prop_map_into(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@conectado It just occurred to me that the chances of hitting one of these with a resource that we actually resolved are incredibly low. We should choose a much smaller netmask here (like 30 maybe?) to ensure we also generate some for the first IPs that we assign for DNS resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unknown resource dst=100.100.111.1 in trace logs

3 participants