Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DNS proxying with large/truncated packets #239

Merged
merged 4 commits into from
Aug 13, 2022

Conversation

DasSkelett
Copy link
Member

Problems

The DNS proxy only forwards to upstreams over UDP.
It also passes the EDNS0 max message size option on unchanged, as received from the client.

This means that if you query a domain and record type with a big response (e.g. TXT cloudflare.com, especially with +dnssec) which doesn't fit in your advertised bufsize or exceeds the PMTU, the proxy receives a truncated response from the upstream. It returns it to the client, which then retries the query using TCP. However the proxy will either return a cached, truncated response, or ask the upstreams over UDP again and receive it truncated again.

Note that in some instances I could only reproduce this by querying the proxy over IPv6, for IPv4 I assume fragmentation along the path helps to hide the issues.

Changes

  • Set outgoing EDNS0 UDP buffer size to 1232 byte
    Instead of passing on what clients send us, we set the recommended value of 1232 bytes explicitly. This ensures upstreams don't try to send us bigger responses which may get dropped (silently), but instead return a reply with tc=1 so we can try again over TCP.
    For modifying the bufsize value the query message needs to be copied, so the original client message isn't manipulated.
    Additionally when storing and loading messages from the cache they are copied now as well to prevent accidental changes to the cached message. This means two or three allocations of a few hundred bytes per query now; hopefully this does not regress performance for bigger deployments. If it does we may have to use sync.Pool or similar.

  • Retry over TCP for truncated packets from upstreams
    Now the proxy has its own retry logic for truncated replies and immediately retries over TCP, using a separate, dedicated TCP client. This happens transparently.

  • Truncate response to client if required
    Since the proxy may now receive a response over TCP that is too big to send to the client, the proxy truncates the message if required and sets tc=1. If the transport between client<->proxy is TCP, the limit is 65535 bytes, if it's UDP w/ EDNS0 it's whatever the client told us, if it's UDP w/o EDNS0 it's 512 bytes.

  • And I've added a workflow to run go test, for each "supported" Go version (we should keep the matrix up to date to include all versions upwards from what we say in go.mod).

@DasSkelett DasSkelett added bug Something isn't working go Pull requests that update Go code labels Aug 5, 2022
@mergeable
Copy link

mergeable bot commented Aug 5, 2022

Thanks for creating a pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

@DasSkelett
Copy link
Member Author

read udp [::1]:35245->[::1]:8053: read: connection refused

Is IPv6 completely disabled on GitHub Action runners? Not even localhost works? 🤔

@DasSkelett
Copy link
Member Author

DasSkelett commented Aug 5, 2022

Ah, this might be a race condition, the listeners are started in goroutines but we don't wait for them. Hm...

Edit: yup, that was it. Now maybe we should also add a caching step for the Go dependencies.

* Set outgoing EDNS0 UDP buffer size to 1232 bytes
* Retry over TCP for truncated packets from upstreams
* Truncate response to client if required
@DasSkelett DasSkelett force-pushed the fix/dns-proxy-big-packets branch 2 times, most recently from 6155da0 to 4b07f99 Compare August 13, 2022 12:20
@DasSkelett DasSkelett merged commit c8da2fc into freifunkMUC:master Aug 13, 2022
@DasSkelett DasSkelett deleted the fix/dns-proxy-big-packets branch August 13, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants