feat: implement DNS over QUIC #18
Conversation
|
Hi @bassosimone! I made changes as per RFC 9250. Could you test once? One more change is pending. The edns-tcp-keepalive option (section 5.5.2). This option should not be set. If we receive this option as an argument in NewQuery function we need to handle it. |
|
Nice, it seems the code is now working as intended with |
|
More specifically, I think there are two criteria we should meet for merging an MVP of DNS over QUIC:
I will allocate some time to study the RFC and review the code over the weekend! Thank you for working on DoQ, @roopeshsn! 🙏 ✨ |
|
I tried to resolve the following domain, I got the following error, I'll look into this. |
Thanks to you! |
From response.go file, I came to know that if a domain is not valid, then we'll get an error with a suffix "no such host". This is expected, correct? But in this case, roopeshsn.com is valid. So dns0.eu is not able to resolve this domain correct? |
Let me try and run the same command you were running with other protocols. So, I tried using DNS-over-TLS first: It's curious I get an Then I tried using DNS-over-UDP: This result seems to suggest (See how the answer is
So, However, Anyway, back onto the DoQ topic, I think the DoQ code is working as intended, since it shows However, I am still a bit confused by the following: I would have expected |
| udpAddr, err := net.ResolveUDPAddr("udp", addr.Address) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| udpConn, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.IPv4zero, Port: 0}) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Would it be possible here to use net.ListenConfig.ListenPacket instead? My thinking is that, like we do for TCP, I would like (not in this PR) to abstract away the establishment of a connection, which is useful for (a) testing and (b) measurements (by wrapping the returned conn). In light of this, it seems to me net.ListenConfig.ListenPacket is a better API to aim for, given that it combines DNS lookup and connection creation in a single call.
| // 4.2.1. DNS Message IDs | ||
| // When sending queries over a QUIC connection, the DNS Message ID MUST | ||
| // be set to 0. | ||
| query.Id = 0 |
There was a problem hiding this comment.
We should extend this comment to mention that we're modifying the original query, which means that, after additional refactoring, this code could cause a data race if it is changed so that we rewrite the query.Id in a background goroutine. I see this as documenting and making it explicit what could go wrong with refactoring.
Likewise, I would like a corresponding comment to be present in the *Transport.Query method. The comment should read something like "This function MAY modify the provided query in place. This happens, e.g., with DoH and DoQ, where we set the query.Id to zero. However, we guarantee that this modification will always happen in the same goroutine that called this function, thus avoiding data races."
(The actual comment does not need to be exactly like this, but I wrote this ~quick explainer to clarify what I mean and why I think it's important to document expectations with respect to mutability.)
| if err != nil { | ||
| return | ||
| } | ||
| stream.Write(rawQuery) |
There was a problem hiding this comment.
I think what we are doing here is not fully compliant with the RFC and, in particular, with this requirement laid out in Sect. 4.2.:
In order for multiple responses to be parsed, a 2-octet length field is used in exactly the same way as the 2-octet length field defined for DNS over TCP [RFC1035]. The practical result of this is that the content of each QUIC stream is exactly the same as the content of a TCP connection that would manage exactly one query.
All DNS messages (queries and responses) sent over DoQ connections MUST be encoded as a 2-octet length field followed by the message content as specified in [RFC1035].
We already have code to implement this kind of exchange with the server, in dotcp.go and we're using it to implement DNS over TCP and DNS over TLS. In fact, DNS over TLS is just a tiny function that setups the TLS connection and then invokes the *Transport.queryStream function.
In light of this, I was wondering whether it could be sensible to attempt to use the same code for DNS over QUIC as well. I think doing this would reduce the amount of duplicate code, but there will be some required changes (I am mostly guessing here, since I did not try this out systematically, just skimmed through the code):
-
We may want to change the
queryStreamfunction signature to take a specific interface (maybe nameddnsStream?) rather than anet.Conn, and this interface should only specify the functions we need insidequeryStream(and using a different interface would also communicate thatqueryStreamaccepts data types beyond the classicalnet.Connand indeed could also accept a QUIC stream). -
The
queryStreamfunction should have a way to only close the stream/conn when we're doing DoQ, which means DoQ requires us to complicate the original algorithm by a singleif, which seems ~fine. -
It seems to me
quic.Streamdoes not have theLocalAddrandRemoteAddrmethod, so we should probably play composition by creating a new ad-hoc type that uses thequic.Connor thequic.Streamto implement the required methods ofdnsStream(or however we want to call this interface) to ensure the combination ofquic.Streamandquic.Connmeets the requirements of thequeryStreammethod.
|
Hi @bassosimone! Thanks for taking the time to review my PR! I'll go through your comments and make the changes this weekend. |
Implemented DNS over QUIC.
Part of rbmk-project/issues#3