feat: Introduce dns.RawResolver interface#594
Conversation
Extracts the RawResolver interface, FuncRawResolver, RawToResolver, and their tests out of PR #592 to decouple DNS querying from response parsing.
fortuna
left a comment
There was a problem hiding this comment.
Some API questions to consider
| // allowing callers to parse the response with any library — including those that support | ||
| // record types not yet recognized by golang.org/x/net/dns/dnsmessage. | ||
| type RawResolver interface { | ||
| QueryRaw(ctx context.Context, name string, qtype uint16) ([]byte, error) |
There was a problem hiding this comment.
We need to decide on the qname format. Could be the native []byte format with length-prefixed labels, or [][]byte (slice of labels), or a escaped string ("foo.bar.com").
While this doesn't affect regular usage, it can help use cases like DNS tunneling.
This commit modifies RawResolver's QueryRaw to take an arbitrary buf []byte to enable zero-allocation DNS wire-format pipelines logic within queryDatagram and queryStream.
c48ea1c to
918b795
Compare
…dAll Extract body read loop to readAllInto, taking ContentLength into consideration to pre-allocate capacity.
| // this adapter only unpacks the result. | ||
| func RawToResolver(r RawResolver) Resolver { | ||
| return FuncResolver(func(ctx context.Context, q dnsmessage.Question) (*dnsmessage.Message, error) { | ||
| buf := make([]byte, 0, maxUDPMessageSize) |
There was a problem hiding this comment.
What should the size be? I'm not convinced this is the best approach.
There was a problem hiding this comment.
Perhaps we should set the max message size in the resolver object. Each implementation will have different parameters.
We may want to specify OPT options.
Also see https://datatracker.ietf.org/doc/html/rfc6891#section-6.2.5
There was a problem hiding this comment.
Should we read it into buf instead to avoid allocating another buffer?
| 1. Zero-dependency footprint: Bypassing rigid DNS parsing libraries allows Outline to pack esoteric labels natively natively (e.g. obscure DNS tunneling headers). | ||
| 2. Future-proof compatibility: Callers can inject or parse the raw responses using any external library, including those supporting newer record types. | ||
| 3. Zero-allocation cycles: Providing a reusable `buf []byte` hands memory management to the caller, removing slice allocations in the hot path. |
There was a problem hiding this comment.
Let's reorder:
- Future proof
- Zero Allocation
- BYO parsing
| // allowing callers to parse the response with any library — including those that support | ||
| // record types not yet recognized by golang.org/x/net/dns/dnsmessage. | ||
| type RawResolver interface { | ||
| QueryRaw(ctx context.Context, name string, qtype uint16, buf []byte) ([]byte, error) |
There was a problem hiding this comment.
We should explain that the returned buffer may or may not be a slice of the input. It depends on the implementation.
Or should we change that to guarantee it?
The main challenge is when we have dynamic lengths, as in TCP and HTTP
|
|
||
| // RawToResolver wraps a [RawResolver] in a [Resolver] that parses the wire-format | ||
| // response bytes using golang.org/x/net/dns/dnsmessage. | ||
| // The underlying [RawResolver] is responsible for ID matching and returning valid bytes; |
There was a problem hiding this comment.
We can remove this line.
| respQName := rawResp[12 : 12+len(expectedName)] | ||
|
|
||
| // Case-insensitive comparison, as the server may echo back randomized caps (e.g. 0x20 encoding) | ||
| for i := 0; i < len(expectedName); i++ { |
There was a problem hiding this comment.
Move to a function. Also, need to compare the lengths!
| } | ||
| } | ||
|
|
||
| respQType := binary.BigEndian.Uint16(rawResp[12+len(expectedName):]) |
There was a problem hiding this comment.
We should check type and class first.
| func checkResponse(reqID uint16, reqQues dnsmessage.Question, respHdr dnsmessage.Header, respQs []dnsmessage.Question) error { | ||
| if !respHdr.Response { | ||
| // checkResponseRaw verifies the DNS response matches the request parameters directly on the wire bytes. | ||
| func checkResponseRaw(reqID uint16, name string, qtype uint16, rawResp []byte) error { |
There was a problem hiding this comment.
It seems like we may need another abstraction layer. This kind of check is standard, and independent on how we exchange messages (though the message exchanges depend on the message id)
There's also things like padding, extended errors, client subnet, DNSSEC, and so on.... Where should that live?
Oh, and caching!
| // [DNS-over-UDP]: https://datatracker.ietf.org/doc/html/rfc1035#section-4.2.1 | ||
| func NewUDPResolver(pd transport.PacketDialer, resolverAddr string) Resolver { | ||
| func NewUDPRawResolver(pd transport.PacketDialer, resolverAddr string) RawResolver { |
There was a problem hiding this comment.
I don't think these factory functions work well. We should consider a way to specify configs, like the max packet length for UDP, and layer other functionality that are common across implementations.
| } | ||
| if err := checkResponse(0, q, msg.Header, msg.Questions); err != nil { | ||
| // Ignore invalid packets that fail to parse. It could be injected. | ||
| if err := checkResponseRaw(0, name, qtype, buf); err != nil { |
There was a problem hiding this comment.
I think we should match on ID first, then validate.
Motivation
The main goal here is to decouple this foundational piece from the DNS parsing library. I've had a few issues with
dns.Resolverwhile working on ECH research:I ended up forced to use miekg's library instead of the SDK for that work.
This decoupling will allow the user to use whatever parsing library they prefer.
It also streamlines the implementation of the system resolver, removing unnecessary transformations when we fetch a
[]byteresponse from the system APIs. See #592 for where I'm going.The new interface takes the output buffer as input, which lets the caller control the lifecycle of the buffer and avoid unnecessary allocations.
I also want this to be a building block for the dnstt implementation.
Changes
The new API includes:
RawResolver(takes QNAME + QTYPE + output buffer)FuncRawResolver- conveniencefunc->RawResolveradaptorRawToResolver- convenience adaptor to map to the current interface.