go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/ameshkov/dnsstamps v1.0.3
 	github.com/beefsack/go-rate v0.0.0-20200827232406-6cde80facd47
 	github.com/jessevdk/go-flags v1.5.0
-	github.com/lucas-clemente/quic-go v0.25.0
+	github.com/lucas-clemente/quic-go v0.27.1
 	github.com/miekg/dns v1.1.44
 	github.com/patrickmn/go-cache v2.1.0+incompatible
 	github.com/stretchr/testify v1.7.0
@@ -24,9 +24,9 @@ require (
 	github.com/fsnotify/fsnotify v1.5.1 // indirect
 	github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0 // indirect
 	github.com/kr/text v0.2.0 // indirect
-	github.com/marten-seemann/qtls-go1-16 v0.1.4 // indirect
-	github.com/marten-seemann/qtls-go1-17 v0.1.0 // indirect
-	github.com/marten-seemann/qtls-go1-18 v0.1.0 // indirect
+	github.com/marten-seemann/qtls-go1-16 v0.1.5 // indirect
+	github.com/marten-seemann/qtls-go1-17 v0.1.1 // indirect
+	github.com/marten-seemann/qtls-go1-18 v0.1.1 // indirect
 	github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
 	github.com/nxadm/tail v1.4.8 // indirect
 	github.com/onsi/ginkgo v1.16.5 // indirect

go.sum

@@ -94,19 +94,17 @@ github.com/kr/pty v1.1.3/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
-github.com/lucas-clemente/quic-go v0.25.0 h1:K+X9Gvd7JXsOHtU0N2icZ2Nw3rx82uBej3mP4CLgibc=
-github.com/lucas-clemente/quic-go v0.25.0/go.mod h1:YtzP8bxRVCBlO77yRanE264+fY/T2U9ZlW1AaHOsMOg=
+github.com/lucas-clemente/quic-go v0.27.1 h1:sOw+4kFSVrdWOYmUjufQ9GBVPqZ+tu+jMtXxXNmRJyk=
+github.com/lucas-clemente/quic-go v0.27.1/go.mod h1:AzgQoPda7N+3IqMMMkywBKggIFo2KT6pfnlrQ2QieeI=
 github.com/lunixbochs/vtclean v1.0.0/go.mod h1:pHhQNgMf3btfWnGBVipUOjRYhoOsdGqdm/+2c2E2WMI=
 github.com/mailru/easyjson v0.0.0-20190312143242-1de009706dbe/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc=
 github.com/marten-seemann/qpack v0.2.1/go.mod h1:F7Gl5L1jIgN1D11ucXefiuJS9UMVP2opoCp2jDKb7wc=
-github.com/marten-seemann/qtls-go1-15 v0.1.4/go.mod h1:GyFwywLKkRt+6mfU99csTEY1joMZz5vmB1WNZH3P81I=
-github.com/marten-seemann/qtls-go1-16 v0.1.4 h1:xbHbOGGhrenVtII6Co8akhLEdrawwB2iHl5yhJRpnco=
-github.com/marten-seemann/qtls-go1-16 v0.1.4/go.mod h1:gNpI2Ol+lRS3WwSOtIUUtRwZEQMXjYK+dQSBFbethAk=
-github.com/marten-seemann/qtls-go1-17 v0.1.0 h1:P9ggrs5xtwiqXv/FHNwntmuLMNq3KaSIG93AtAZ48xk=
-github.com/marten-seemann/qtls-go1-17 v0.1.0/go.mod h1:fz4HIxByo+LlWcreM4CZOYNuz3taBQ8rN2X6FqvaWo8=
-github.com/marten-seemann/qtls-go1-18 v0.1.0-beta.1/go.mod h1:PUhIQk19LoFt2174H4+an8TYvWOGjb/hHwphBeaDHwI=
-github.com/marten-seemann/qtls-go1-18 v0.1.0 h1:gCiNAyl7K4yBBjKkI4LeJjMwIEyCweFoEQFOnRn2MuA=
-github.com/marten-seemann/qtls-go1-18 v0.1.0/go.mod h1:PUhIQk19LoFt2174H4+an8TYvWOGjb/hHwphBeaDHwI=
+github.com/marten-seemann/qtls-go1-16 v0.1.5 h1:o9JrYPPco/Nukd/HpOHMHZoBDXQqoNtUCmny98/1uqQ=
+github.com/marten-seemann/qtls-go1-16 v0.1.5/go.mod h1:gNpI2Ol+lRS3WwSOtIUUtRwZEQMXjYK+dQSBFbethAk=
+github.com/marten-seemann/qtls-go1-17 v0.1.1 h1:DQjHPq+aOzUeh9/lixAGunn6rIOQyWChPSI4+hgW7jc=
+github.com/marten-seemann/qtls-go1-17 v0.1.1/go.mod h1:C2ekUKcDdz9SDWxec1N/MvcXBpaX9l3Nx67XaR84L5s=
+github.com/marten-seemann/qtls-go1-18 v0.1.1 h1:qp7p7XXUFL7fpBvSS1sWD+uSqPvzNQK43DH+/qEkj0Y=
+github.com/marten-seemann/qtls-go1-18 v0.1.1/go.mod h1:mJttiymBAByA49mhlNZZGrH5u1uXYZJ+RW28Py7f4m4=
 github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
 github.com/microcosm-cc/bluemonday v1.0.1/go.mod h1:hsXNsILzKxV+sX77C5b8FSuKF00vh2OMYv+xgHpAMF4=
 github.com/miekg/dns v1.1.40/go.mod h1:KNUDUusw/aVsxyTYZM1oqvCicbwhgbNgztCETuNZ7xM=

proxy/dns_context.go

@@ -41,9 +41,9 @@ type DNSContext struct {
 	// localIP - local IP address (for UDP socket to call udpMakeOOBWithSrc)
 	localIP net.IP
 
-	// HTTPRequest - HTTP request (for DOH only)
+	// HTTPRequest - HTTP request (for DoH only)
 	HTTPRequest *http.Request
-	// HTTPResponseWriter - HTTP response writer (for DOH only)
+	// HTTPResponseWriter - HTTP response writer (for DoH only)
 	HTTPResponseWriter http.ResponseWriter
 
 	// DNSCryptResponseWriter - necessary to respond to a DNSCrypt query
@@ -53,9 +53,14 @@ type DNSContext struct {
 	// ProtoQUIC only.
 	QUICStream quic.Stream
 
-	// QUICSession is the QUIC session from which we got the query.  For
+	// QUICConnection is the QUIC session from which we got the query.  For
 	// ProtoQUIC only.
-	QUICSession quic.Session
+	QUICConnection quic.Connection
+
+	// DoQVersion is the DoQ protocol version. It can (and should) be read from
+	// ALPN, but in the current version we also use the way DNS messages are
+	// encoded as a signal.
+	DoQVersion DoQVersion
 
 	// RequestID is an opaque numerical identifier of this request that is
 	// guaranteed to be unique across requests processed by a single Proxy
@@ -113,3 +118,17 @@ func (dctx *DNSContext) scrub() {
 	// Some devices require DNS message compression.
 	dctx.Res.Compress = true
 }
+
+// DoQVersion is an enumeration with supported DoQ versions.
+type DoQVersion int
+
+const (
+	// DoQv1Draft represents old DoQ draft versions that do not send a 2-octet
+	// prefix with the DNS message length.
+	//
+	// TODO(ameshkov): remove in the end of 2024.
+	DoQv1Draft DoQVersion = 0x00
+
+	// DoQv1 represents DoQ v1.0: https://www.rfc-editor.org/rfc/rfc9250.html.
+	DoQv1 DoQVersion = 0x01
+)

proxy/proxy_test.go

@@ -590,7 +590,7 @@ func TestFallbackFromInvalidBootstrap(t *testing.T) {
 		dnsProxy.Fallbacks = append(dnsProxy.Fallbacks, f)
 	}
 
-	// using a DOT server with invalid bootstrap
+	// Using a DoT server with invalid bootstrap.
 	u, _ := upstream.AddressToUpstream(
 		"tls://dns.adguard.com",
 		&upstream.Options{

proxy/server.go

@@ -140,7 +140,7 @@ func (p *Proxy) handleDNSRequest(d *DNSContext) error {
 
 // respond writes the specified response to the client (or does nothing if d.Res is empty)
 func (p *Proxy) respond(d *DNSContext) {
-	// d.Conn can be nil in the case of a DOH request
+	// d.Conn can be nil in the case of a DoH request.
 	if d.Conn != nil {
 		d.Conn.SetWriteDeadline(time.Now().Add(defaultTimeout)) //nolint
 	}

proxy/server_https.go

@@ -53,11 +53,14 @@ func (p *Proxy) listenHTTPS(srv *http.Server, l net.Listener) {
 	}
 }
 
-// ServeHTTP is the http.RequestHandler implementation that handles DOH queries
+// ServeHTTP is the http.RequestHandler implementation that handles DoH queries
 // Here is what it returns:
-// http.StatusBadRequest - if there is no DNS request data
-// http.StatusUnsupportedMediaType - if request content type is not application/dns-message
-// http.StatusMethodNotAllowed - if request method is not GET or POST
+//
+//  - http.StatusBadRequest if there is no DNS request data;
+//  - http.StatusUnsupportedMediaType if request content type is not
+//    "application/dns-message";
+//  - http.StatusMethodNotAllowed if request method is not GET or POST.
+//
 func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	log.Tracef("Incoming HTTPS request on %s", r.URL)
 
@@ -126,7 +129,7 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-// Writes a response to the DOH client
+// Writes a response to the DoH client.
 func (p *Proxy) respondHTTPS(d *DNSContext) error {
 	resp := d.Res
 	w := d.HTTPResponseWriter

proxy/server_quic.go

@@ -2,10 +2,13 @@ package proxy
 
 import (
 	"context"
+	"encoding/binary"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
 
+	"github.com/AdguardTeam/dnsproxy/proxyutil"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/lucas-clemente/quic-go"
@@ -15,20 +18,32 @@ import (
 // NextProtoDQ is the ALPN token for DoQ. During connection establishment,
 // DNS/QUIC support is indicated by selecting the ALPN token "dq" in the
 // crypto handshake.
-// Current draft version:
-// https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-02
-const NextProtoDQ = "doq-i02"
+// DoQ RFC: https://www.rfc-editor.org/rfc/rfc9250.html
+const NextProtoDQ = "doq"
 
 // compatProtoDQ is a list of ALPN tokens used by a QUIC connection.
 // NextProtoDQ is the latest draft version supported by dnsproxy, but it also
 // includes previous drafts.
-var compatProtoDQ = []string{NextProtoDQ, "doq-i00", "dq", "doq"}
+var compatProtoDQ = []string{NextProtoDQ, "doq-i02", "doq-i00", "dq"}
 
 // maxQUICIdleTimeout is maximum QUIC idle timeout.  The default value in
 // quic-go is 30 seconds, but our internal tests show that a higher value works
 // better for clients written with ngtcp2.
 const maxQUICIdleTimeout = 5 * time.Minute
 
+const (
+	// DoQCodeNoError is used when the connection or stream needs to be closed,
+	// but there is no error to signal.
+	DoQCodeNoError quic.ApplicationErrorCode = 0
+	// DoQCodeInternalError signals that the DoQ implementation encountered
+	// an internal error and is incapable of pursuing the transaction or the
+	// connection.
+	DoQCodeInternalError quic.ApplicationErrorCode = 1
+	// DoQCodeProtocolError signals that the DoQ implementation encountered
+	// a protocol error and is forcibly aborting the connection.
+	DoQCodeProtocolError quic.ApplicationErrorCode = 2
+)
+
 func (p *Proxy) createQUICListeners() error {
 	for _, a := range p.QUICListenAddr {
 		log.Info("Creating a QUIC listener")
@@ -51,7 +66,7 @@ func (p *Proxy) createQUICListeners() error {
 func (p *Proxy) quicPacketLoop(l quic.Listener, requestGoroutinesSema semaphore) {
 	log.Info("Entering the DNS-over-QUIC listener loop on %s", l.Addr())
 	for {
-		session, err := l.Accept(context.Background())
+		conn, err := l.Accept(context.Background())
 		if err != nil {
 			if isQUICNonCrit(err) {
 				log.Tracef("quic connection closed or timeout: %s", err)
@@ -64,24 +79,24 @@ func (p *Proxy) quicPacketLoop(l quic.Listener, requestGoroutinesSema semaphore)
 
 		requestGoroutinesSema.acquire()
 		go func() {
-			p.handleQUICSession(session, requestGoroutinesSema)
+			p.handleQUICConnection(conn, requestGoroutinesSema)
 			requestGoroutinesSema.release()
 		}()
 	}
 }
 
-// handleQUICSession handles a new QUIC session.  It waits for new streams and
-// passes them to handleQUICStream.
+// handleQUICConnection handles a new QUIC connection.  It waits for new streams
+// and passes them to handleQUICStream.
 //
 // See also the comment on Proxy.requestGoroutinesSema.
-func (p *Proxy) handleQUICSession(session quic.Session, requestGoroutinesSema semaphore) {
+func (p *Proxy) handleQUICConnection(conn quic.Connection, requestGoroutinesSema semaphore) {
 	for {
 		// The stub to resolver DNS traffic follows a simple pattern in which
 		// the client sends a query, and the server provides a response.  This
 		// design specifies that for each subsequent query on a QUIC connection
 		// the client MUST select the next available client-initiated
-		// bidirectional stream
-		stream, err := session.AcceptStream(context.Background())
+		// bidirectional stream.
+		stream, err := conn.AcceptStream(context.Background())
 		if err != nil {
 			if isQUICNonCrit(err) {
 				log.Tracef("quic connection closed or timeout: %s", err)
@@ -89,77 +104,87 @@ func (p *Proxy) handleQUICSession(session quic.Session, requestGoroutinesSema se
 				log.Info("got error when accepting a new QUIC stream: %s", err)
 			}
 
-			// Close the session to make sure resources are freed
-			_ = session.CloseWithError(0, "")
+			// Close the connection to make sure resources are freed.
+			closeQUICConn(conn, DoQCodeNoError)
 
 			return
 		}
 
 		requestGoroutinesSema.acquire()
 		go func() {
-			p.handleQUICStream(stream, session)
+			p.handleQUICStream(stream, conn)
+
+			// The server MUST send the response(s) on the same stream and MUST
+			// indicate, after the last response, through the STREAM FIN
+			// mechanism that no further data will be sent on that stream.
 			_ = stream.Close()
+
 			requestGoroutinesSema.release()
 		}()
 	}
 }
 
 // handleQUICStream reads DNS queries from the stream, processes them,
-// and writes back the responses
-func (p *Proxy) handleQUICStream(stream quic.Stream, session quic.Session) {
+// and writes back the response.
+func (p *Proxy) handleQUICStream(stream quic.Stream, conn quic.Connection) {
 	bufPtr := p.bytesPool.Get().(*[]byte)
 	defer p.bytesPool.Put(bufPtr)
 
-	// One query -- one stream
-	// The client MUST send the DNS query over the selected stream, and MUST
-	// indicate through the STREAM FIN mechanism that no further data will
-	// be sent on that stream.
+	// One query - one stream.
+	// The client MUST select the next available client-initiated bidirectional
+	// stream for each subsequent query on a QUIC connection.
 
-	// err is not checked here because STREAM FIN sent by the client is indicated as error here.
-	// instead, we should check the number of bytes received.
+	// err is not checked here because STREAM FIN sent by the client is
+	// indicated as error here.  Instead, we should check the number of bytes
+	// received.
 	buf := *bufPtr
 	n, err := stream.Read(buf)
 
-	// The server MUST send the response on the same stream, and MUST indicate through
-	// the STREAM FIN mechanism that no further data will be sent on that stream.
-	defer stream.Close()
-
 	if n < minDNSPacketSize {
 		logShortQUICRead(err)
 
 		return
 	}
 
+	// In theory, we should use ALPN to get the DoQ version properly. However,
+	// since there are not too many versions now, we only check how the DNS
+	// query is encoded. If it's sent with a 2-byte prefix, we consider this a
+	// DoQ v1. Otherwise, a draft version.
+	doqVersion := DoQv1
 	req := &dns.Msg{}
-	err = req.Unpack(buf)
+
+	// Note that we support both the old drafts and the new RFC. In the old
+	// draft DNS messages were not prefixed with the message length.
+	packetLen := binary.BigEndian.Uint16(buf[:2])
+	if packetLen == uint16(n-2) {
+		err = req.Unpack(buf[2:])
+	} else {
+		err = req.Unpack(buf)
+		doqVersion = DoQv1Draft
+	}
+
 	if err != nil {
 		log.Error("unpacking quic packet: %s", err)
+		closeQUICConn(conn, DoQCodeProtocolError)
 
 		return
 	}
 
-	// If any message sent on a DoQ connection contains an edns-tcp-keepalive EDNS(0) Option,
-	// this is a fatal error and the recipient of the defective message MUST forcibly abort
-	// the connection immediately.
-	// https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-02#section-6.6.2
-	if opt := req.IsEdns0(); opt != nil {
-		for _, option := range opt.Option {
-			// Check for EDNS TCP keepalive option
-			if option.Option() == dns.EDNS0TCPKEEPALIVE {
-				log.Debug("client sent EDNS0 TCP keepalive option")
-				errorCode := quic.ApplicationErrorCode(quic.ConnectionRefused)
+	if !validQUICMsg(req) {
+		// If a peer encounters such an error condition, it is considered a
+		// fatal error. It SHOULD forcibly abort the connection using QUIC's
+		// CONNECTION_CLOSE mechanism and SHOULD use the DoQ error code
+		// DOQ_PROTOCOL_ERROR.
+		closeQUICConn(conn, DoQCodeProtocolError)
 
-				// Already closing the connection so we don't care about the error.
-				_ = session.CloseWithError(errorCode, "")
-				return
-			}
-		}
+		return
 	}
 
 	d := p.newDNSContext(ProtoQUIC, req)
-	d.Addr = session.RemoteAddr()
+	d.Addr = conn.RemoteAddr()
 	d.QUICStream = stream
-	d.QUICSession = session
+	d.QUICConnection = conn
+	d.DoQVersion = doqVersion
 
 	err = p.handleDNSRequest(d)
 	if err != nil {
@@ -167,29 +192,15 @@ func (p *Proxy) handleQUICStream(stream quic.Stream, session quic.Session) {
 	}
 }
 
-// logShortQUICRead is a logging helper for short reads from a QUIC stream.
-func logShortQUICRead(err error) {
-	if err == nil {
-		log.Info("quic packet too short for dns query")
-
-		return
-	}
-
-	if isQUICNonCrit(err) {
-		log.Tracef("quic connection closed or timeout: %s", err)
-	} else {
-		log.Error("reading from quic stream: %s", err)
-	}
-}
-
-// Writes a response to the QUIC stream
+// respondQUIC writes a response to the QUIC stream.
 func (p *Proxy) respondQUIC(d *DNSContext) error {
 	resp := d.Res
 
 	if resp == nil {
-		// If no response has been written, close the QUIC session right away.
-		errorCode := quic.ApplicationErrorCode(quic.InternalError)
-		return d.QUICSession.CloseWithError(errorCode, "")
+		// If no response has been written, close the QUIC connection now.
+		closeQUICConn(d.QUICConnection, DoQCodeInternalError)
+
+		return errors.New("no response to write")
 	}
 
 	bytes, err := resp.Pack()
@@ -197,17 +208,92 @@ func (p *Proxy) respondQUIC(d *DNSContext) error {
 		return fmt.Errorf("couldn't convert message into wire format: %w", err)
 	}
 
-	n, err := d.QUICStream.Write(bytes)
+	// Depending on the DoQ version with either write a 2-bytes prefixed message
+	// or just write the message (for old draft versions).
+	var buf []byte
+	switch d.DoQVersion {
+	case DoQv1:
+		buf = proxyutil.AddPrefix(bytes)
+	case DoQv1Draft:
+		buf = bytes
+	default:
+		return fmt.Errorf("invalid protocol version: %d", d.DoQVersion)
+	}
+
+	n, err := d.QUICStream.Write(buf)
 	if err != nil {
 		return fmt.Errorf("conn.Write(): %w", err)
 	}
-	if n != len(bytes) {
-		return fmt.Errorf("conn.Write() returned with %d != %d", n, len(bytes))
+	if n != len(buf) {
+		return fmt.Errorf("conn.Write() returned with %d != %d", n, len(buf))
 	}
 
 	return nil
 }
 
+// validQUICMsg validates the incoming DNS message and returns false if
+// something is wrong with the message.
+func validQUICMsg(req *dns.Msg) (ok bool) {
+	// See https://www.rfc-editor.org/rfc/rfc9250.html#name-protocol-errors
+
+	// 1. a client or server receives a message with a non-zero Message ID.
+	//
+	// We do consciously not validate this case since there are stub proxies
+	// that are sending a non-zero Message IDs.
+
+	// 2. a client or server receives a STREAM FIN before receiving all the
+	// bytes for a message indicated in the 2-octet length field.
+	// 3. a server receives more than one query on a stream
+	//
+	// These cases are covered earlier when unpacking the DNS message.
+
+	// 4. the client or server does not indicate the expected STREAM FIN after
+	// sending requests or responses (see Section 4.2).
+	//
+	// This is quite problematic to validate this case since this would imply
+	// we have to wait until STREAM FIN is arrived before we start processing
+	// the message. So we're consciously ignoring this case in this
+	// implementation.
+
+	// 5. an implementation receives a message containing the edns-tcp-keepalive
+	// EDNS(0) Option [RFC7828] (see Section 5.5.2).
+	if opt := req.IsEdns0(); opt != nil {
+		for _, option := range opt.Option {
+			// Check for EDNS TCP keepalive option
+			if option.Option() == dns.EDNS0TCPKEEPALIVE {
+				log.Debug("client sent EDNS0 TCP keepalive option")
+
+				return false
+			}
+		}
+	}
+
+	// 6. a client or a server attempts to open a unidirectional QUIC stream.
+	//
+	// This case can only be handled when writing a response.
+
+	// 7. a server receives a "replayable" transaction in 0-RTT data
+	//
+	// The information necessary to validate this is not exposed by quic-go.
+
+	return true
+}
+
+// logShortQUICRead is a logging helper for short reads from a QUIC stream.
+func logShortQUICRead(err error) {
+	if err == nil {
+		log.Info("quic packet too short for dns query")
+
+		return
+	}
+
+	if isQUICNonCrit(err) {
+		log.Tracef("quic connection closed or timeout: %s", err)
+	} else {
+		log.Error("reading from quic stream: %s", err)
+	}
+}
+
 // isQUICNonCrit returns true if err is a non-critical error, most probably
 // a timeout or a closed connection.
 //
@@ -224,3 +310,11 @@ func isQUICNonCrit(err error) (ok bool) {
 		strings.HasSuffix(errStr, "Application error 0x0") ||
 		errStr == "EOF"
 }
+
+// closeQUICConn quietly closes the QUIC connection.
+func closeQUICConn(conn quic.Connection, code quic.ApplicationErrorCode) {
+	err := conn.CloseWithError(code, "")
+	if err != nil {
+		log.Debug("failed to close QUIC connection: %v", err)
+	}
+}

proxy/server_quic_test.go

@@ -4,22 +4,23 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"io"
 	"testing"
 
-	"github.com/miekg/dns"
-
+	"github.com/AdguardTeam/dnsproxy/proxyutil"
 	"github.com/lucas-clemente/quic-go"
-	"github.com/stretchr/testify/assert"
+	"github.com/miekg/dns"
+	"github.com/stretchr/testify/require"
 )
 
 func TestQuicProxy(t *testing.T) {
-	// Prepare the proxy server
+	// Prepare the proxy server.
 	serverConfig, caPem := createServerTLSConfig(t)
 	dnsProxy := createTestProxy(t, serverConfig)
 
-	// Start listening
+	// Start listening.
 	err := dnsProxy.Start()
-	assert.Nil(t, err)
+	require.NoError(t, err)
 
 	roots := x509.NewCertPool()
 	roots.AppendCertsFromPEM(caPem)
@@ -29,55 +30,70 @@ func TestQuicProxy(t *testing.T) {
 		NextProtos: append([]string{NextProtoDQ}, compatProtoDQ...),
 	}
 
-	// Create a DNS-over-QUIC client connection
+	// Create a DNS-over-QUIC client connection.
 	addr := dnsProxy.Addr(ProtoQUIC)
 
-	// Open QUIC session
-	sess, err := quic.DialAddr(addr.String(), tlsConfig, nil)
-	assert.Nil(t, err)
-	defer sess.CloseWithError(0, "")
+	// Open QUIC connection.
+	conn, err := quic.DialAddr(addr.String(), tlsConfig, nil)
+	require.NoError(t, err)
+	defer conn.CloseWithError(DoQCodeNoError, "")
 
-	// Send several test messages
+	// Send several test messages.
 	for i := 0; i < 10; i++ {
-		sendTestQUICMessage(t, sess)
+		sendTestQUICMessage(t, conn, DoQv1)
+
+		// Send a message encoded for a draft version as well.
+		sendTestQUICMessage(t, conn, DoQv1Draft)
 	}
 
-	// Stop the proxy
+	// Stop the proxy.
 	err = dnsProxy.Stop()
 	if err != nil {
 		t.Fatalf("cannot stop the DNS proxy: %s", err)
 	}
 }
 
-func sendTestQUICMessage(t *testing.T, sess quic.Session) {
-	// Open stream
-	stream, err := sess.OpenStreamSync(context.Background())
-	assert.Nil(t, err)
+// sendTestQUICMessage send a test message to the specified QUIC connection.
+func sendTestQUICMessage(t *testing.T, conn quic.Connection, doqVersion DoQVersion) {
+	// Open a new stream.
+	stream, err := conn.OpenStreamSync(context.Background())
+	require.NoError(t, err)
 	defer stream.Close()
 
-	// Write
+	// Prepare a test message.
 	msg := createTestMessage()
-	buf, err := msg.Pack()
-	assert.Nil(t, err)
+	packedMsg, err := msg.Pack()
+	require.NoError(t, err)
 
-	// Send the DNS query
+	buf := packedMsg
+	if doqVersion == DoQv1 {
+		buf = proxyutil.AddPrefix(packedMsg)
+	}
+
+	// Send the DNS query to the stream.
 	_, err = stream.Write(buf)
-	assert.Nil(t, err)
+	require.NoError(t, err)
 
-	// Close closes the write-direction of the stream
-	// and sends a STREAM FIN packet.
-	stream.Close()
+	// Close closes the write-direction of the stream and sends
+	// a STREAM FIN packet.
+	_ = stream.Close()
 
-	// Now read the response
+	// Now read the response from the stream.
 	respBytes := make([]byte, 64*1024)
 	n, err := stream.Read(respBytes)
-	assert.True(t, err == nil || err.Error() == "EOF")
-	assert.True(t, n > minDNSPacketSize)
+	if err != nil {
+		require.ErrorIs(t, err, io.EOF)
+	}
+	require.Greater(t, n, minDNSPacketSize)
 
-	// Unpack the response
+	// Unpack the DNS response.
 	reply := new(dns.Msg)
-	err = reply.Unpack(respBytes)
-	assert.Nil(t, err)
+	if doqVersion == DoQv1 {
+		err = reply.Unpack(respBytes[2:])
+	} else {
+		err = reply.Unpack(respBytes)
+	}
+	require.NoError(t, err)
 
 	// Check the response
 	assertResponse(t, reply)

proxyutil/dns.go

@@ -56,11 +56,21 @@ func ReadPrefixed(conn net.Conn) ([]byte, error) {
 	return buf, nil
 }
 
-// WritePrefixed -- write a DNS message to a TCP connection
-// it first writes a 2-byte prefix followed by the message itself
+// WritePrefixed writes a DNS message to a TCP connection it first writes
+// a 2-byte prefix followed by the message itself.
 func WritePrefixed(b []byte, conn net.Conn) error {
 	l := make([]byte, 2)
 	binary.BigEndian.PutUint16(l, uint16(len(b)))
 	_, err := (&net.Buffers{l, b}).WriteTo(conn)
+
 	return err
 }
+
+// AddPrefix adds a 2-byte prefix with the DNS message length.
+func AddPrefix(b []byte) (m []byte) {
+	m = make([]byte, 2+len(b))
+	binary.BigEndian.PutUint16(m, uint16(len(b)))
+	copy(m[2:], b)
+
+	return m
+}

upstream/bootstrap_resolver.go

@@ -50,7 +50,7 @@ func NewResolver(resolverAddress string, options *Options) (*Resolver, error) {
 	}
 
 	// Validate the bootstrap resolver. It must be either a plain DNS resolver.
-	// Or a DOT/DOH resolver with an IP address (not a hostname).
+	// Or a DoT/DoH resolver with an IP address (not a hostname).
 	if !isResolverValidBootstrap(r.upstream) {
 		r.upstream = nil
 		log.Error("Resolver %s is not eligible to be a bootstrap DNS server", resolverAddress)
@@ -60,9 +60,9 @@ func NewResolver(resolverAddress string, options *Options) (*Resolver, error) {
 	return r, nil
 }
 
-// isResolverValidBootstrap checks if the upstream is eligible to be a bootstrap DNS server
-// DNSCrypt and plain DNS resolvers are okay
-// DOH and DOT are okay only in the case if an IP address is used in the IP address
+// isResolverValidBootstrap checks if the upstream is eligible to be a bootstrap
+// DNS server DNSCrypt and plain DNS resolvers are okay DoH and DoT are okay
+// only in the case if an IP address is used in the IP address.
 func isResolverValidBootstrap(upstream Upstream) bool {
 	if u, ok := upstream.(*dnsOverTLS); ok {
 		urlAddr, err := url.Parse(u.Address())

upstream/upstream.go

@@ -66,7 +66,7 @@ const (
 	// defaultPortDoQ is the default port for DNS-over-QUIC.  Prior to version
 	// -10 of the draft experiments were directed to use ports 8853, 784.
 	//
-	// See https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-10#section-10.2.
+	// See https://www.rfc-editor.org/rfc/rfc9250.html#name-port-selection.
 	defaultPortDoQ = 853
 )
 

upstream/upstream_doh.go

@@ -76,7 +76,7 @@ func (p *dnsOverHTTPS) Exchange(m *dns.Msg) (*dns.Msg, error) {
 	return r, err
 }
 
-// exchangeHTTPSClient sends the DNS query to a DOH resolver using the specified
+// exchangeHTTPSClient sends the DNS query to a DoH resolver using the specified
 // http.Client instance.
 func (p *dnsOverHTTPS) exchangeHTTPSClient(m *dns.Msg, client *http.Client) (*dns.Msg, error) {
 	buf, err := m.Pack()
@@ -136,7 +136,7 @@ func (p *dnsOverHTTPS) exchangeHTTPSClient(m *dns.Msg, client *http.Client) (*dn
 }
 
 // getClient gets or lazily initializes an HTTP client (and transport) that will
-// be used for this DOH resolver.
+// be used for this DoH resolver.
 func (p *dnsOverHTTPS) getClient() (c *http.Client, err error) {
 	startTime := time.Now()
 
@@ -175,7 +175,7 @@ func (p *dnsOverHTTPS) createClient() (*http.Client, error) {
 }
 
 // createTransport initializes an HTTP transport that will be used specifically
-// for this DOH resolver. This HTTP transport ensures that the HTTP requests
+// for this DoH resolver. This HTTP transport ensures that the HTTP requests
 // will be sent exactly to the IP address got from the bootstrap resolver.
 func (p *dnsOverHTTPS) createTransport() (*http.Transport, error) {
 	tlsConfig, dialContext, err := p.boot.get()

upstream/upstream_quic.go

@@ -8,22 +8,41 @@ import (
 	"sync"
 	"time"
 
-	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/dnsproxy/proxyutil"
+	"github.com/AdguardTeam/golibs/log"
 	"github.com/lucas-clemente/quic-go"
 	"github.com/miekg/dns"
 )
 
-const handshakeTimeout = time.Second
+const (
+	// DoQCodeNoError is used when the connection or stream needs to be closed,
+	// but there is no error to signal.
+	DoQCodeNoError = quic.ApplicationErrorCode(0)
+	// DoQCodeInternalError signals that the DoQ implementation encountered
+	// an internal error and is incapable of pursuing the transaction or the
+	// connection.
+	DoQCodeInternalError = quic.ApplicationErrorCode(1)
+	// DoQCodeProtocolError signals that the DoQ implementation encountered
+	// a protocol error and is forcibly aborting the connection.
+	DoQCodeProtocolError = quic.ApplicationErrorCode(2)
+)
 
 //
-// DNS-over-QUIC
+// dnsOverQUIC is a DNS-over-QUIC implementation according to the spec:
+// https://www.rfc-editor.org/rfc/rfc9250.html
 //
 type dnsOverQUIC struct {
-	boot    *bootstrapper
-	session quic.Session
-
-	bytesPool    *sync.Pool // byte packets pool
-	sync.RWMutex            // protects session and bytesPool
+	// boot is a bootstrap DNS abstraction that is used to resolve the upstream
+	// server's address and open a network connection to it.
+	boot *bootstrapper
+	// conn is the current active QUIC connection.  It can be closed and
+	// re-opened when needed.
+	conn quic.Connection
+	// bytesPool is a *sync.Pool we use to store byte buffers in.  These byte
+	// buffers are used to read responses from the upstream.
+	bytesPool *sync.Pool
+	// sync.RWMutex protects conn and bytesPool.
+	sync.RWMutex
 }
 
 // type check
@@ -44,28 +63,15 @@ func newDoQ(uu *url.URL, opts *Options) (u Upstream, err error) {
 
 func (p *dnsOverQUIC) Address() string { return p.boot.URL.String() }
 
-func (p *dnsOverQUIC) Exchange(m *dns.Msg) (*dns.Msg, error) {
-	session, err := p.getSession(true)
+func (p *dnsOverQUIC) Exchange(m *dns.Msg) (res *dns.Msg, err error) {
+	var conn quic.Connection
+	conn, err = p.getConnection(true)
 	if err != nil {
 		return nil, err
 	}
 
-	// If any message sent on a DoQ connection contains an edns-tcp-keepalive EDNS(0) Option,
-	// this is a fatal error and the recipient of the defective message MUST forcibly abort
-	// the connection immediately.
-	// https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-02#section-6.6.2
-	if opt := m.IsEdns0(); opt != nil {
-		for _, option := range opt.Option {
-			// Check for EDNS TCP keepalive option
-			if option.Option() == dns.EDNS0TCPKEEPALIVE {
-				_ = session.CloseWithError(0, "") // Already closing the connection so we don't care about the error
-				return nil, errors.Error("EDNS0 TCP keepalive option is set")
-			}
-		}
-	}
-
-	// https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-02#section-6.4
-	// When sending queries over a QUIC connection, the DNS Message ID MUST be set to zero.
+	// When sending queries over a QUIC connection, the DNS Message ID MUST be
+	// set to zero.
 	id := m.Id
 	var reply *dns.Msg
 	m.Id = 0
@@ -77,19 +83,23 @@ func (p *dnsOverQUIC) Exchange(m *dns.Msg) (*dns.Msg, error) {
 		}
 	}()
 
-	stream, err := p.openStream(session)
+	var buf []byte
+	buf, err = m.Pack()
 	if err != nil {
-		return nil, fmt.Errorf("open new stream to %s: %w", p.Address(), err)
+		return nil, fmt.Errorf("failed to pack DNS message for DoQ: %w", err)
 	}
 
-	buf, err := m.Pack()
+	var stream quic.Stream
+	stream, err = p.openStream(conn)
 	if err != nil {
-		return nil, err
+		p.closeConnWithError(DoQCodeInternalError)
+		return nil, fmt.Errorf("open new stream to %s: %w", p.Address(), err)
 	}
 
-	_, err = stream.Write(buf)
+	_, err = stream.Write(proxyutil.AddPrefix(buf))
 	if err != nil {
-		return nil, err
+		p.closeConnWithError(DoQCodeInternalError)
+		return nil, fmt.Errorf("failed to write to a QUIC stream: %w", err)
 	}
 
 	// The client MUST send the DNS query over the selected stream, and MUST
@@ -98,27 +108,19 @@ func (p *dnsOverQUIC) Exchange(m *dns.Msg) (*dns.Msg, error) {
 	// stream.Close() -- closes the write-direction of the stream.
 	_ = stream.Close()
 
-	pool := p.getBytesPool()
-	bufPtr := pool.Get().(*[]byte)
-
-	defer pool.Put(bufPtr)
-
-	respBuf := *bufPtr
-	n, err := stream.Read(respBuf)
-	if err != nil && n == 0 {
-		return nil, fmt.Errorf("reading response from %s: %w", p.Address(), err)
-	}
-
-	reply = new(dns.Msg)
-	err = reply.Unpack(respBuf)
+	res, err = p.readMsg(stream)
 	if err != nil {
-		return nil, fmt.Errorf("unpacking response from %s: %w", p.Address(), err)
+		// If a peer encounters such an error condition, it is considered a
+		// fatal error.  It SHOULD forcibly abort the connection using QUIC's
+		// CONNECTION_CLOSE mechanism and SHOULD use the DoQ error code
+		// DOQ_PROTOCOL_ERROR.
+		p.closeConnWithError(DoQCodeProtocolError)
 	}
-
-	return reply, nil
+	return res, err
 }
 
-func (p *dnsOverQUIC) getBytesPool() *sync.Pool {
+// getBytesPool returns (creates if needed) a pool we store byte buffers in.
+func (p *dnsOverQUIC) getBytesPool() (pool *sync.Pool) {
 	p.Lock()
 	if p.bytesPool == nil {
 		p.bytesPool = &sync.Pool{
@@ -133,20 +135,21 @@ func (p *dnsOverQUIC) getBytesPool() *sync.Pool {
 	return p.bytesPool
 }
 
-// getSession - opens or returns an existing quic.Session
-// useCached - if true and cached session exists, return it right away
-// otherwise - forcibly creates a new session
-func (p *dnsOverQUIC) getSession(useCached bool) (quic.Session, error) {
-	var session quic.Session
+// getConnection opens or returns an existing quic.Connection. useCached
+// argument controls whether we should try to use the existing cached
+// connection.  If it is false, we will forcibly create a new connection and
+// close the existing one if needed.
+func (p *dnsOverQUIC) getConnection(useCached bool) (quic.Connection, error) {
+	var conn quic.Connection
 	p.RLock()
-	session = p.session
-	if session != nil && useCached {
+	conn = p.conn
+	if conn != nil && useCached {
 		p.RUnlock()
-		return session, nil
+		return conn, nil
 	}
-	if session != nil {
-		// we're recreating the session, let's create a new one
-		_ = session.CloseWithError(0, "")
+	if conn != nil {
+		// we're recreating the connection, let's create a new one.
+		_ = conn.CloseWithError(DoQCodeNoError, "")
 	}
 	p.RUnlock()
 
@@ -154,23 +157,24 @@ func (p *dnsOverQUIC) getSession(useCached bool) (quic.Session, error) {
 	defer p.Unlock()
 
 	var err error
-	session, err = p.openSession()
+	conn, err = p.openConnection()
 	if err != nil {
 		// This does not look too nice, but QUIC (or maybe quic-go)
 		// doesn't seem stable enough.
 		// Maybe retransmissions aren't fully implemented in quic-go?
 		// Anyways, the simple solution is to make a second try when
-		// it fails to open the QUIC session.
-		session, err = p.openSession()
+		// it fails to open the QUIC conn.
+		conn, err = p.openConnection()
 		if err != nil {
 			return nil, err
 		}
 	}
-	p.session = session
-	return session, nil
+	p.conn = conn
+	return conn, nil
 }
 
-func (p *dnsOverQUIC) openStream(session quic.Session) (quic.Stream, error) {
+// openStream opens a new QUIC stream for the specified connection.
+func (p *dnsOverQUIC) openStream(conn quic.Connection) (quic.Stream, error) {
 	ctx := context.Background()
 
 	if p.boot.options.Timeout > 0 {
@@ -180,32 +184,33 @@ func (p *dnsOverQUIC) openStream(session quic.Session) (quic.Stream, error) {
 		defer cancel() // avoid resource leak
 	}
 
-	stream, err := session.OpenStreamSync(ctx)
+	stream, err := conn.OpenStreamSync(ctx)
 	if err == nil {
 		return stream, nil
 	}
 
-	// try to recreate the session
-	newSession, err := p.getSession(false)
+	// try to recreate the connection.
+	newConn, err := p.getConnection(false)
 	if err != nil {
 		return nil, err
 	}
-	// open a new stream
-	return newSession.OpenStreamSync(ctx)
+	// open a new stream.
+	return newConn.OpenStreamSync(ctx)
 }
 
-func (p *dnsOverQUIC) openSession() (quic.Session, error) {
+// openConnection opens a new QUIC connection.
+func (p *dnsOverQUIC) openConnection() (conn quic.Connection, err error) {
 	tlsConfig, dialContext, err := p.boot.get()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to bootstrap QUIC connection: %w", err)
 	}
 
 	// we're using bootstrapped address instead of what's passed to the function
 	// it does not create an actual connection, but it helps us determine
-	// what IP is actually reachable (when there're v4/v6 addresses)
+	// what IP is actually reachable (when there're v4/v6 addresses).
 	rawConn, err := dialContext(context.Background(), "udp", "")
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to open a QUIC connection: %w", err)
 	}
 	// It's never actually used
 	_ = rawConn.Close()
@@ -216,13 +221,57 @@ func (p *dnsOverQUIC) openSession() (quic.Session, error) {
 	}
 
 	addr := udpConn.RemoteAddr().String()
-	quicConfig := &quic.Config{
-		HandshakeIdleTimeout: handshakeTimeout,
+	quicConfig := &quic.Config{}
+	conn, err = quic.DialAddrContext(context.Background(), addr, tlsConfig, quicConfig)
+	if err != nil {
+		return nil, fmt.Errorf("opening quic connection to %s: %w", p.Address(), err)
 	}
-	session, err := quic.DialAddrContext(context.Background(), addr, tlsConfig, quicConfig)
+
+	return conn, nil
+}
+
+// closeConnWithError closes the active connection with error to make sure that
+// new queries were processed in another connection. We can do that in the case
+// of a fatal error.
+func (p *dnsOverQUIC) closeConnWithError(code quic.ApplicationErrorCode) {
+	p.Lock()
+	defer p.Unlock()
+
+	if p.conn == nil {
+		// Do nothing, there's no active conn anyways.
+		return
+	}
+
+	err := p.conn.CloseWithError(code, "")
+	if err != nil {
+		log.Error("failed to close the conn: %v", err)
+	}
+	p.conn = nil
+}
+
+// readMsg reads the incoming DNS message from the QUIC stream.
+func (p *dnsOverQUIC) readMsg(stream quic.Stream) (m *dns.Msg, err error) {
+	pool := p.getBytesPool()
+	bufPtr := pool.Get().(*[]byte)
+
+	defer pool.Put(bufPtr)
+
+	respBuf := *bufPtr
+	n, err := stream.Read(respBuf)
+	if err != nil && n == 0 {
+		return nil, fmt.Errorf("reading response from %s: %w", p.Address(), err)
+	}
+
+	// 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].
+	// IMPORTANT: Note, that we ignore this prefix here as this implementation
+	// does not support receiving multiple messages over a single connection.
+	m = new(dns.Msg)
+	err = m.Unpack(respBuf[2:])
 	if err != nil {
-		return nil, fmt.Errorf("opening quic session to %s: %w", p.Address(), err)
+		return nil, fmt.Errorf("unpacking response from %s: %w", p.Address(), err)
 	}
 
-	return session, nil
+	return m, nil
 }

upstream/upstream_quic_test.go

@@ -4,27 +4,27 @@ import (
 	"testing"
 
 	"github.com/lucas-clemente/quic-go"
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func TestUpstreamDOQ(t *testing.T) {
+func TestUpstreamDoQ(t *testing.T) {
 	// Create a DNS-over-QUIC upstream
-	address := "quic://dns.adguard.com:784"
+	address := "quic://dns.adguard.com"
 	u, err := AddressToUpstream(address, &Options{InsecureSkipVerify: true})
-	assert.Nil(t, err)
+	require.NoError(t, err)
 
 	uq := u.(*dnsOverQUIC)
-	var sess quic.Session
+	var conn quic.Connection
 
 	// Test that it responds properly
 	for i := 0; i < 10; i++ {
 		checkUpstream(t, u, address)
 
-		if sess == nil {
-			sess = uq.session
+		if conn == nil {
+			conn = uq.conn
 		} else {
-			// This way we test that the session is properly reused
-			assert.True(t, sess == uq.session)
+			// This way we test that the conn is properly reused
+			require.Equal(t, conn, uq.conn)
 		}
 	}
 }

upstream/upstream_test.go

@@ -275,7 +275,7 @@ func TestAddressToUpstream_bads(t *testing.T) {
 	}
 }
 
-func TestUpstreamDOTBootstrap(t *testing.T) {
+func TestUpstreamDoTBootstrap(t *testing.T) {
 	upstreams := []struct {
 		address   string
 		bootstrap []string
@@ -377,12 +377,12 @@ func TestUpstreamsWithServerIP(t *testing.T) {
 		serverIP:  net.IP{94, 140, 14, 14},
 		bootstrap: invalidBootstrap,
 	}, {
-		// AdGuard DNS DOH with the IP address specified
+		// AdGuard DNS DoH with the IP address specified.
 		address:   "sdns://AgcAAAAAAAAADzE3Ni4xMDMuMTMwLjEzMAAPZG5zLmFkZ3VhcmQuY29tCi9kbnMtcXVlcnk",
 		serverIP:  nil,
 		bootstrap: invalidBootstrap,
 	}, {
-		// AdGuard DNS DOT with the IP address specified
+		// AdGuard DNS DoT with the IP address specified.
 		address:   "sdns://AwAAAAAAAAAAEzE3Ni4xMDMuMTMwLjEzMDo4NTMAD2Rucy5hZGd1YXJkLmNvbQ",
 		serverIP:  nil,
 		bootstrap: invalidBootstrap,