fix: DoH SHOULD and DoQ MUST use a zero query ID (#19)

RFC9250 Sect. 4.2.1 says:

```
When sending queries over a QUIC connection, the DNS Message ID MUST
be set to 0.  The stream mapping for DoQ allows for unambiguous
correlation of queries and responses, so the Message ID field is not
required.

This has implications for proxying DoQ messages to and from other
transports.  For example, proxies may have to manage the fact that
DoQ can support a larger number of outstanding queries on a single
connection than, for example, DNS over TCP, because DoQ is not
limited by the Message ID space.  This issue already exists for DoH,
where a Message ID of 0 is recommended.
```

RFC 8484 Sect. 4.1 says:

```
In order to maximize HTTP cache friendliness, DoH clients using media
formats that include the ID field from the DNS message header, such
as "application/dns-message", SHOULD use a DNS ID of 0 in every DNS
request.  HTTP correlates the request and response, thus eliminating
the need for the ID in a media type such as "application/dns-
message".  The use of a varying DNS ID can cause semantically
equivalent DNS queries to be cached separately.
```

We noticed this issue in
#18,
where DoQ queries consistently failed with `dns.alidns.com` when
not using a zero DNS query ID.

This diff aims at addressing the issue by adding support for generating
a protocol-specific query by default.

We do this by adding a new constructor: NewQueryWithServerAddr.

From the provided ServerAddr, we obtain the protocol, which, in turn
determines whether we should use a zero query ID.

The existing NewQuery protocol is deprecated and becomes a wrapper
around the new NewQueryWithServerAddr function.

Because we recognise the value of customising the actual query ID beyond
what the RFC says, we also introduce a new QueryOption called
QueryOptionID that allows setting an arbitrary ID.

We also update tests to ensure full coverage.

We also update the `internal` testing commands accordingly.

While there, add convenience aliases for DNS protocol names (I found
myself wanting this three times, so...)

example_https_test.go

@@ -28,7 +28,7 @@ func ExampleTransport_dnsOverHTTPS() {
 			dnscore.EDNS0FlagDO|dnscore.EDNS0FlagBlockLengthPadding,
 		),
 	}
-	query, err := dnscore.NewQuery("dns.google", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "dns.google", dns.TypeA, options...)
 	if err != nil {
 		log.Fatal(err)
 	}

example_tcp_test.go

@@ -28,7 +28,7 @@ func ExampleTransport_dnsOverTCP() {
 			0,
 		),
 	}
-	query, err := dnscore.NewQuery("dns.google", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "dns.google", dns.TypeA, options...)
 	if err != nil {
 		log.Fatal(err)
 	}

example_tls_test.go

@@ -28,7 +28,7 @@ func ExampleTransport_dnsOverTLS() {
 			dnscore.EDNS0FlagDO|dnscore.EDNS0FlagBlockLengthPadding,
 		),
 	}
-	query, err := dnscore.NewQuery("dns.google", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "dns.google", dns.TypeA, options...)
 	if err != nil {
 		log.Fatal(err)
 	}

example_udp_test.go

@@ -28,7 +28,7 @@ func ExampleTransport_dnsOverUDP() {
 			0,
 		),
 	}
-	query, err := dnscore.NewQuery("dns.google", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "dns.google", dns.TypeA, options...)
 	if err != nil {
 		log.Fatal(err)
 	}

integration_test.go

@@ -46,7 +46,7 @@ func TestTransport_RoundTrip_UDP(t *testing.T) {
 			0,
 		),
 	}
-	query, err := dnscore.NewQuery("example.com", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "example.com", dns.TypeA, options...)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -79,7 +79,7 @@ func TestTransport_RoundTrip_TCP(t *testing.T) {
 			0,
 		),
 	}
-	query, err := dnscore.NewQuery("example.com", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "example.com", dns.TypeA, options...)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -112,7 +112,7 @@ func TestTransport_RoundTrip_TLS(t *testing.T) {
 			dnscore.EDNS0FlagDO|dnscore.EDNS0FlagBlockLengthPadding,
 		),
 	}
-	query, err := dnscore.NewQuery("example.com", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "example.com", dns.TypeA, options...)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -153,7 +153,7 @@ func TestTransport_RoundTrip_HTTPS(t *testing.T) {
 			dnscore.EDNS0FlagDO|dnscore.EDNS0FlagBlockLengthPadding,
 		),
 	}
-	query, err := dnscore.NewQuery("example.com", dns.TypeA, options...)
+	query, err := dnscore.NewQueryWithServerAddr(serverAddr, "example.com", dns.TypeA, options...)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/cmd/transport/main.go

@@ -60,7 +60,7 @@ func main() {
 
 	// Create the DNS query
 	optEDNS0 := dnscore.QueryOptionEDNS0(maxlength, flags)
-	query := runtimex.Try1(dnscore.NewQuery(*domain, dnsType, optEDNS0))
+	query := runtimex.Try1(dnscore.NewQueryWithServerAddr(server, *domain, dnsType, optEDNS0))
 	fmt.Printf(";; Query:\n%s\n", query.String())
 
 	// Perform the DNS query

lookup.go

@@ -46,7 +46,7 @@ func (r *Resolver) exchange(ctx context.Context,
 	}
 
 	// Encode the query
-	query, err := NewQuery(name, qtype, server.queryOptions...)
+	query, err := NewQueryWithServerAddr(server.address, name, qtype, server.queryOptions...)
 	if err != nil {
 		return nil, err
 	}

query.go

@@ -72,7 +72,22 @@ func QueryOptionEDNS0(maxResponseSize uint16, flags int) QueryOption {
 	}
 }
 
-// NewQuery constructs a [*dns.Message] containing a query.
+// QueryOptionID allows setting an arbitrary query ID.
+//
+// Otherwise, the default is using [dns.Id] for all protocols
+// except DNS-over-HTTPS and DNS-over-QUIC, where we use
+// zero, thus following RFC 9250 Sect 4.2.1.
+func QueryOptionID(id uint16) QueryOption {
+	return func(q *dns.Msg) error {
+		q.Id = id
+		return nil
+	}
+}
+
+// NewQueryWithServerAddr constructs a [*dns.Message] containing a
+// query for the given domain, query type and [*ServerAddr]. We use
+// the [*ServerAddr] to enforce protocol-specific query settings,
+// such as, that DoH SHOULD use a zero query ID.
 //
 // This function takes care of IDNA encoding the domain name and
 // fails if the domain name is invalid.
@@ -82,7 +97,8 @@ func QueryOptionEDNS0(maxResponseSize uint16, flags int) QueryOption {
 // Use constants such as [dns.TypeAAAA] to specify the query type.
 //
 // The [QueryOption] functions can be used to set additional options.
-func NewQuery(name string, qtype uint16, options ...QueryOption) (*dns.Msg, error) {
+func NewQueryWithServerAddr(serverAddr *ServerAddr, name string, qtype uint16,
+	options ...QueryOption) (*dns.Msg, error) {
 	// IDNA encode the domain name.
 	punyName, err := idna.Lookup.ToASCII(name)
 	if err != nil {
@@ -101,11 +117,21 @@ func NewQuery(name string, qtype uint16, options ...QueryOption) (*dns.Msg, erro
 		Qclass: dns.ClassINET,
 	}
 	query := new(dns.Msg)
-	query.Id = dns.Id()
 	query.RecursionDesired = true
 	query.Question = make([]dns.Question, 1)
 	query.Question[0] = question
 
+	// Only set the queryID for protocols that actually
+	// require a nonzero queryID to be set.
+	// TODO(bassosimone,roopeshsn): update for DoQ
+	switch serverAddr.Protocol {
+	case ProtocolDoH:
+		// for DoH/DoQ, by default we leave the query ID to
+		// zero, which is what the RFCs suggest/require.
+	default:
+		query.Id = dns.Id()
+	}
+
 	// Apply the query options.
 	for _, option := range options {
 		if err := option(query); err != nil {
@@ -114,3 +140,15 @@ func NewQuery(name string, qtype uint16, options ...QueryOption) (*dns.Msg, erro
 	}
 	return query, nil
 }
+
+// NewQuery is equivalent to calling [NewQueryWithServerAddr] with
+// a zero-initialized [*ServerAddr]. We retain this function for backward
+// compatibility with the previous API. Existing code that is using this
+// function SHOULD use [NewQueryWithServerAddr] with DoH (and MUST with
+// DoQ) such that we correctly set the query ID to zero. Other protocols
+// are not impacted by this issue and may continue using [NewQuery].
+//
+// Deprecated: use [NewQueryWithServerAddr] instead.
+func NewQuery(name string, qtype uint16, options ...QueryOption) (*dns.Msg, error) {
+	return NewQueryWithServerAddr(&ServerAddr{}, name, qtype, options...)
+}

query_test.go

@@ -9,7 +9,100 @@ import (
 	"github.com/miekg/dns"
 )
 
+func TestNewQueryWithServerAddr(t *testing.T) {
+	// Override the `dns.Id` factory for testing purposes
+	const expectedNonZeroQueryID = 4
+	savedId := dns.Id
+	dns.Id = func() uint16 { return expectedNonZeroQueryID }
+	defer func() { dns.Id = savedId }()
+
+	// TODO(bassosimone,roopeshsn): ensure we also test for DoQ
+	// once we merge the corresponding PR.
+
+	tests := []struct {
+		name       string
+		serverAddr *ServerAddr
+		qname      string
+		qtype      uint16
+		options    []QueryOption
+		wantName   string
+		wantId     uint16
+		wantErr    bool
+	}{
+		{
+			name:       "standard UDP query",
+			serverAddr: NewServerAddr(ProtocolUDP, "8.8.8.8:53"),
+			qname:      "www.example.com",
+			qtype:      dns.TypeA,
+			wantName:   "www.example.com.",
+			wantId:     expectedNonZeroQueryID,
+		},
+		{
+			name:       "standard TCP query",
+			serverAddr: NewServerAddr(ProtocolTCP, "8.8.8.8:53"),
+			qname:      "www.example.com",
+			qtype:      dns.TypeA,
+			wantName:   "www.example.com.",
+			wantId:     expectedNonZeroQueryID,
+		},
+		{
+			name:       "standard TLS query",
+			serverAddr: NewServerAddr(ProtocolTLS, "8.8.8.8:53"),
+			qname:      "www.example.com",
+			qtype:      dns.TypeA,
+			wantName:   "www.example.com.",
+			wantId:     expectedNonZeroQueryID,
+		},
+		{
+			name:       "DoH query should have zero ID",
+			serverAddr: NewServerAddr(ProtocolHTTPS, "https://dns.google/dns-query"),
+			qname:      "example.com",
+			qtype:      dns.TypeAAAA,
+			wantName:   "example.com.",
+			wantId:     0,
+		},
+		{
+			name:       "invalid domain",
+			serverAddr: NewServerAddr(ProtocolUDP, "8.8.8.8:53"),
+			qname:      "invalid domain",
+			qtype:      dns.TypeA,
+			wantErr:    true,
+		},
+		{
+			name:       "with failing option",
+			serverAddr: NewServerAddr(ProtocolUDP, "8.8.8.8:53"),
+			qname:      "www.example.com",
+			qtype:      dns.TypeA,
+			options:    []QueryOption{mockedFailingOption},
+			wantErr:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := NewQueryWithServerAddr(tt.serverAddr, tt.qname, tt.qtype, tt.options...)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("NewQueryWithServerAddr() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if err != nil {
+				return
+			}
+			if got.Question[0].Name != tt.wantName {
+				t.Errorf("NewQueryWithServerAddr() name = %v, want %v", got.Question[0].Name, tt.wantName)
+			}
+			if tt.wantId == 0 && got.Id != 0 {
+				t.Errorf("NewQueryWithServerAddr() id = %v, want 0", got.Id)
+			}
+			if tt.wantId != 0 && got.Id == 0 {
+				t.Errorf("NewQueryWithServerAddr() id = 0, want non-zero")
+			}
+		})
+	}
+}
+
 func TestNewQuery(t *testing.T) {
+	// Note: NewQuery has been deprecated on 2025-02-20
 	tests := []struct {
 		name     string
 		qtype    uint16
@@ -54,3 +147,14 @@ func TestQueryOptionEDNS0(t *testing.T) {
 		t.Errorf("QueryOptionEDNS0() did not set padding option")
 	}
 }
+
+func TestQueryOptionID(t *testing.T) {
+	query := new(dns.Msg)
+	option := QueryOptionID(42)
+	if err := option(query); err != nil {
+		t.Errorf("QueryOptionID() error = %v", err)
+	}
+	if query.Id != 42 {
+		t.Errorf("QueryOptionID() did not set ID")
+	}
+}

serveraddr.go

@@ -5,6 +5,7 @@ package dnscore
 // Protocol is a transport protocol.
 type Protocol string
 
+// All the implemented DNS protocols.
 const (
 	// ProtocolUDP is DNS over UDP.
 	ProtocolUDP = Protocol("udp")
@@ -19,6 +20,15 @@ const (
 	ProtocolDoH = Protocol("doh")
 )
 
+// Name aliases for DNS protocols.
+const (
+	// ProtocolTLS is an alias for ProtocolDoT.
+	ProtocolTLS = ProtocolDoT
+
+	// ProtocolHTTPS is an alias for ProtocolDoH.
+	ProtocolHTTPS = ProtocolDoH
+)
+
 // ServerAddr is a DNS server address.
 //
 // While currently minimal, ServerAddr is designed as a pointer type to