Internal DNS handling overhaul + TCP + truncated reply support #18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements DNS name compression (see the decription added in
dns/encode.cpp `encode_name()` for details).
This reduces the sizes of nearly all query responses
(because every response answer includes the question, and then uses the
name again for the response), and in some cases significantly so.
This is rather important for Session Router all of our names are 52 byte
pubkeys (plus the 4 or 5 byte tld), and so we are potentially running up
against the DNS 512-byte max message size.
(We should also enable EDNS to allow longer messages, but that is left
here as a FIXME and not yet implemented).
An example shows how the compression helps:
For example, an AAAA query for `localhost.sesh` responds with an answer
of:
;; QUESTION SECTION:
;localhost.sesh. IN AAAA
;; ANSWER SECTION:
localhost.sesh. 10 IN CNAME sh6tnpf84s885m8ygsjw7g8qjuo1jk7ydufiog8sjdtgkhb3w8iy.sesh.
sh6tnpf84s885m8ygsjw7g8qjuo1jk7ydufiog8sjdtgkhb3w8iy.sesh. 10 IN AAAA fd2e:7365:7368::1
The repeated question doesn't compress anything, of course, but once you
hit the answer, you start getting savings:
The repeated `localhost.sesh` in the first answer gets compressed from
16 bytes (without compression) to a 2 byte pointer (back to the same
address in the question).
The first PUBKEY.sesh (in the CNAME target) gets slightly reduced by
being able to encode the trailing `sesh` from 6 bytes uncompressed
(4+"sesh"+\0) to a 2-byte pointer (again back
into the question, pointing just at the sesh tld rather than the entire
lokinet.sesh value).
The pubkey.sesh. in the second answers gets hugely reduced: an
uncompressed 59 bytes (52+"pubkey"+4+"sesh"+0) becomes a simple 2-byte
pointer to the same name in the previous answer line.
For some queries like SRV records the savings are even potentially even
larger, especially when there are multiple SRV entries for a .sesh
address.
An empty name was getting encoded as two \0's instead of just one.
This adds support for EDNS requests, allowing for larger responses. As part of that, it adds support for handling DNS cookies, which are a sort of pseudo-mac over DNS requests to prevent out-of-path attackers from being able to forge responses.
- Greatly simplify the DNS code into a dns::Handler + dns::Listener
instead of 14 (approximately, it's hard to tell) intermixed classes
doing various things in needlessly complicated ways.
- Move all of the actual DNS record logic from tun.cpp (which was more
than half of tun.cpp!) into dns/handler.cpp.
- Remove DNSInterceptor (and l3-intercept config option), as it doesn't
work reliably even in current stable Lokinet. The only platform that
might actually need it is Android (eventually) but it can either be
resurrected or (more likely) an Android-specific interceptor built.
Other platforms have better alternatives than attempting IP packet
interception.
- Implement TCP DNS requests, so that we can successful return queries
that might not fit in a UDP packet (even with EDNS).
- Implement dns truncation bit, which is meant to tell an application
that the response is too large and it needs to retry via TCP (hence
the above addition):
- Catch the exception and set the truncation flag when we overrun the
reply buffer. Previously we were just not responding.
- Make sure we can't return sliced RR records in a truncated reply by
throwing away all the answer section if we run out of space.
jagerman
force-pushed
the
dns-tcp-listener
branch
from
6a17841 to
9a2df97
Compare
- A TXT lookup for path.PUBKEY.{sesh,snode} now returns info about your
path in a TXT record.
- Replace the stdev calculation for paths with a jitter calculation as
that is probably slightly more relevant. (jitter = mean absolute
difference between sequential pings)
- Refactor the current_path methods to return more raw info rather that
stringifying early, deferring stringification to later parts where
needed (session_router, and now DNS). Also adds ping and expiry info
into it.
Example TXT info:
;; QUESTION SECTION:
;path.55fxrybf3jtausbnmxpgwcsz9t8qkf5pr8t5f4xyto4omjrkorpy.snode. IN TXT
;; ANSWER SECTION:
path.55fxrybf3jtausbnmxpgwcsz9t8qkf5pr8t5f4xyto4omjrkorpy.snode. 0 IN TXT "d=out;
path=55fxro86pzfka36f19qagzsuogk6qwek1g8hdgkx5menjn1s6iso@199.127.61.170
55fxne4863o4r4ohntbnx1baqupfgmghdw4sas5zbnxso4a9d1oy@172.93.103.156
55fxr5s76pmfaqfrhxbyfgnkcueok8j6a8c6fci9ts8axmwgmaoo@206.221.176.9
55fxrybf3jtausbnmxpgwcsz9t8qkf5pr8t5f4xy" "to4omjrkorpy@23.88.6.250;
ttl=1160; p=116; pj=0.714; pr=8; pt=0; pT=0"
where:
- `d` is `in`/`out`/`none` showing the session direction.
When `d` is not `none` this is followed by:
- `path=...` space-separated PUBKEY@IP values along the path
- `ttl=...` seconds remaining before this path expires
- `p=...` average path ping time (in milliseconds)
- `pj=...` average path jitter (milliseconds, with three decimal points)
- `pr=...` number of ping responses
- `pt=...` number of ping timeouts
- `pT=...` number of recent ping timeouts (i.e. 3 means the last 3 pings
have timed out).
This solves issues during destruction where we could get a segfault or a deadlock because of race conditions between connection close and link endpoint destruction. Basically what happened was: - during Router stop, link_endpoint gets destroyed. - that destruction closes all the connections, firing the on_conn_closed callbacks during destruction. - those callbacks were capturing a shared_ptr to the closed connection in a lambda job pushed onto the router loop - that router loop job can't run yet because we're already inside a router job doing the shutdown that triggered all of this - we get "router is stopped" - the queued cleanup code fires. There were multiple things wrong here: - trying to call `selected_alpn` (or `remote()`) would crash or deadlock because the network loop was gone, but those accessors go through a call_get. - the cleanup job had an invalid `this` pointer because link_endpoint was gone. - the cleanup job was also capturing a shared_ptr<Connection> to a Connection tied to the link_endpoint's loop, and so attempting to destroy that lambda (even if we early return) could crash or deadlock because the loop that handles the Connection destructor isn't around anymore. This reworks it to fix it: - adds a shared_ptr canary we capture to make sure we are still alive, to early-return from the lambda if `this` isn't valid anymore. - captures all the conn details we need in the lambda, rather than capturing a shared_ptr to get it out of inside the lambda. - changes all the "is the conn pointer the same" logic to use the reference id rather than pointer (since we can't keep the conn pointer anymore).
jagerman
force-pushed
the
dns-tcp-listener
branch
from
9a2df97 to
92ce7c7
Compare
Member
Author
|
Closing (included in #20) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(On top of #17)
Greatly simplify the DNS code into a dns::Handler + dns::Listener instead of 14 (approximately, it's hard to tell) intermixed classes doing various things in needlessly complicated ways.
Move all of the actual DNS record logic from tun.cpp (which was more than half of tun.cpp!) into dns/handler.cpp.
Remove DNSInterceptor (and l3-intercept config option), as it doesn't work reliably even in current stable Lokinet. The only platform that might actually need it is Android (eventually) but it can either be resurrected or (more likely) an Android-specific interceptor built. Other platforms have better alternatives than attempting IP packet interception.
Implement TCP DNS requests, so that we can successful return queries that might not fit in a UDP packet (even with EDNS).
Implement dns truncation bit, which is meant to tell an application that the response is too large and it needs to retry via TCP (hence the above addition):
Add a DNS TXT record for
path.PUBKEY.{sesh,snode}that returns path info and statisticsReplace path stdev tracking/calculations with a jitter calculation instead.
Still TODO: