Pull request: all: simplify dnssec logic

Closes AdguardTeam#3904.

Squashed commit of the following:

commit 5948f0d
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Dec 13 17:53:40 2021 +0300

    querylog: imp

commit 852cc7d
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Dec 13 17:44:41 2021 +0300

    querylog: fix entry write

commit 9d58046
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Dec 13 16:45:56 2021 +0300

    all: simplify dnssec logic

CHANGELOG.md

@@ -49,6 +49,8 @@ and this project adheres to
 
 ### Changed
 
+- The DNSSEC check now simply checks against the AD flag in the response
+  ([#3904]).
 - Client objects in the configuration file are now sorted ([#3933]).
 - Responses from cache are now labeled ([#3772]).
 - Better error message for ED25519 private keys, which are not widely supported
@@ -233,6 +235,7 @@ In this release, the schema version has changed from 10 to 12.
 [#3772]: https://github.com/AdguardTeam/AdGuardHome/issues/3772
 [#3815]: https://github.com/AdguardTeam/AdGuardHome/issues/3815
 [#3890]: https://github.com/AdguardTeam/AdGuardHome/issues/3890
+[#3904]: https://github.com/AdguardTeam/AdGuardHome/issues/3904
 [#3933]: https://github.com/AdguardTeam/AdGuardHome/pull/3933
 
 

internal/dnsforward/config.go

@@ -118,7 +118,7 @@ type FilteringConfig struct {
 
 	BogusNXDomain          []string `yaml:"bogus_nxdomain"`     // transform responses with these IP addresses to NXDOMAIN
 	AAAADisabled           bool     `yaml:"aaaa_disabled"`      // Respond with an empty answer to all AAAA requests
-	EnableDNSSEC           bool     `yaml:"enable_dnssec"`      // Set DNSSEC flag in outcoming DNS request
+	EnableDNSSEC           bool     `yaml:"enable_dnssec"`      // Set AD flag in outcoming DNS request
 	EnableEDNSClientSubnet bool     `yaml:"edns_client_subnet"` // Enable EDNS Client Subnet option
 	MaxGoroutines          uint32   `yaml:"max_goroutines"`     // Max. number of parallel goroutines for processing incoming requests
 

internal/dnsforward/dns.go

@@ -18,33 +18,44 @@ import (
 // To transfer information between modules
 type dnsContext struct {
 	proxyCtx *proxy.DNSContext
+
 	// setts are the filtering settings for the client.
-	setts     *filtering.Settings
-	startTime time.Time
-	result    *filtering.Result
+	setts *filtering.Settings
+
+	result *filtering.Result
 	// origResp is the response received from upstream.  It is set when the
 	// response is modified by filters.
 	origResp *dns.Msg
+
 	// unreversedReqIP stores an IP address obtained from PTR request if it
 	// parsed successfully and belongs to one of locally-served IP ranges as per
 	// RFC 6303.
 	unreversedReqIP net.IP
+
 	// err is the error returned from a processing function.
 	err error
+
 	// clientID is the clientID from DoH, DoQ, or DoT, if provided.
 	clientID string
+
 	// origQuestion is the question received from the client.  It is set
 	// when the request is modified by rewrites.
 	origQuestion dns.Question
+
+	// startTime is the time at which the processing of the request has started.
+	startTime time.Time
+
 	// protectionEnabled shows if the filtering is enabled, and if the
 	// server's DNS filter is ready.
 	protectionEnabled bool
+
 	// responseFromUpstream shows if the response is received from the
 	// upstream servers.
 	responseFromUpstream bool
-	// origReqDNSSEC shows if the DNSSEC flag in the original request from
-	// the client is set.
-	origReqDNSSEC bool
+
+	// responseAD shows if the response had the AD bit set.
+	responseAD bool
+
 	// isLocalClient shows if client's IP address is from locally-served
 	// network.
 	isLocalClient bool
@@ -90,7 +101,6 @@ func (s *Server) handleDNSRequest(_ *proxy.Proxy, d *proxy.DNSContext) error {
 		s.processFilteringBeforeRequest,
 		s.processLocalPTR,
 		s.processUpstream,
-		s.processDNSSECAfterResponse,
 		s.processFilteringAfterResponse,
 		s.ipset.process,
 		s.processQueryLogsAndStats,
@@ -514,96 +524,53 @@ func ipStringFromAddr(addr net.Addr) (ipStr string) {
 }
 
 // processUpstream passes request to upstream servers and handles the response.
-func (s *Server) processUpstream(ctx *dnsContext) (rc resultCode) {
-	d := ctx.proxyCtx
-	if d.Res != nil {
-		return resultCodeSuccess // response is already set - nothing to do
+func (s *Server) processUpstream(dctx *dnsContext) (rc resultCode) {
+	pctx := dctx.proxyCtx
+	if pctx.Res != nil {
+		// The response has already been set.
+		return resultCodeSuccess
 	}
 
-	if d.Addr != nil && s.conf.GetCustomUpstreamByClient != nil {
+	if pctx.Addr != nil && s.conf.GetCustomUpstreamByClient != nil {
 		// Use the clientID first, since it has a higher priority.
-		id := stringutil.Coalesce(ctx.clientID, ipStringFromAddr(d.Addr))
+		id := stringutil.Coalesce(dctx.clientID, ipStringFromAddr(pctx.Addr))
 		upsConf, err := s.conf.GetCustomUpstreamByClient(id)
 		if err != nil {
 			log.Error("dns: getting custom upstreams for client %s: %s", id, err)
 		} else if upsConf != nil {
 			log.Debug("dns: using custom upstreams for client %s", id)
-			d.CustomUpstreamConfig = upsConf
+			pctx.CustomUpstreamConfig = upsConf
 		}
 	}
 
-	req := d.Req
+	req := pctx.Req
+	origReqAD := false
 	if s.conf.EnableDNSSEC {
-		opt := req.IsEdns0()
-		if opt == nil {
-			log.Debug("dns: adding OPT record with DNSSEC flag")
-			req.SetEdns0(4096, true)
-		} else if !opt.Do() {
-			opt.SetDo(true)
+		if req.AuthenticatedData {
+			origReqAD = true
 		} else {
-			ctx.origReqDNSSEC = true
+			req.AuthenticatedData = true
 		}
 	}
 
 	// Process the request further since it wasn't filtered.
-
 	prx := s.proxy()
 	if prx == nil {
-		ctx.err = srvClosedErr
+		dctx.err = srvClosedErr
 
 		return resultCodeError
 	}
 
-	if ctx.err = prx.Resolve(d); ctx.err != nil {
+	if dctx.err = prx.Resolve(pctx); dctx.err != nil {
 		return resultCodeError
 	}
 
-	ctx.responseFromUpstream = true
-
-	return resultCodeSuccess
-}
-
-// Process DNSSEC after response from upstream server
-func (s *Server) processDNSSECAfterResponse(ctx *dnsContext) (rc resultCode) {
-	d := ctx.proxyCtx
-
-	// Don't process response if it's not from upstream servers.
-	if !ctx.responseFromUpstream || !s.conf.EnableDNSSEC {
-		return resultCodeSuccess
-	}
-
-	if !ctx.origReqDNSSEC {
-		optResp := d.Res.IsEdns0()
-		if optResp != nil && !optResp.Do() {
-			return resultCodeSuccess
-		}
+	dctx.responseFromUpstream = true
+	dctx.responseAD = pctx.Res.AuthenticatedData
 
-		// Remove RRSIG records from response
-		// because there is no DO flag in the original request from client,
-		// but we have EnableDNSSEC set, so we have set DO flag ourselves,
-		// and now we have to clean up the DNS records our client didn't ask for.
-
-		answers := []dns.RR{}
-		for _, a := range d.Res.Answer {
-			switch a.(type) {
-			case *dns.RRSIG:
-				log.Debug("Removing RRSIG record from response: %v", a)
-			default:
-				answers = append(answers, a)
-			}
-		}
-		d.Res.Answer = answers
-
-		answers = []dns.RR{}
-		for _, a := range d.Res.Ns {
-			switch a.(type) {
-			case *dns.RRSIG:
-				log.Debug("Removing RRSIG record from response: %v", a)
-			default:
-				answers = append(answers, a)
-			}
-		}
-		d.Res.Ns = answers
+	if s.conf.EnableDNSSEC && !origReqAD {
+		pctx.Req.AuthenticatedData = false
+		pctx.Res.AuthenticatedData = false
 	}
 
 	return resultCodeSuccess

internal/dnsforward/stats.go

@@ -15,9 +15,9 @@ import (
 )
 
 // Write Stats data and logs
-func (s *Server) processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) {
-	elapsed := time.Since(ctx.startTime)
-	pctx := ctx.proxyCtx
+func (s *Server) processQueryLogsAndStats(dctx *dnsContext) (rc resultCode) {
+	elapsed := time.Since(dctx.startTime)
+	pctx := dctx.proxyCtx
 
 	shouldLog := true
 	msg := pctx.Req
@@ -41,14 +41,15 @@ func (s *Server) processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) {
 	// uninitialized while in use.  This can happen after proxy server has been
 	// stopped, but its workers haven't yet exited.
 	if shouldLog && s.queryLog != nil {
-		p := querylog.AddParams{
-			Question:   msg,
-			Answer:     pctx.Res,
-			OrigAnswer: ctx.origResp,
-			Result:     ctx.result,
-			Elapsed:    elapsed,
-			ClientIP:   ip,
-			ClientID:   ctx.clientID,
+		p := &querylog.AddParams{
+			Question:          msg,
+			Answer:            pctx.Res,
+			OrigAnswer:        dctx.origResp,
+			Result:            dctx.result,
+			Elapsed:           elapsed,
+			ClientID:          dctx.clientID,
+			ClientIP:          ip,
+			AuthenticatedData: dctx.responseAD,
 		}
 
 		switch pctx.Proto {
@@ -74,7 +75,7 @@ func (s *Server) processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) {
 		s.queryLog.Add(p)
 	}
 
-	s.updateStats(ctx, elapsed, *ctx.result, ip)
+	s.updateStats(dctx, elapsed, *dctx.result, ip)
 
 	return resultCodeSuccess
 }

internal/dnsforward/stats_test.go

@@ -22,11 +22,11 @@ type testQueryLog struct {
 	// a querylog.QueryLog without actually implementing all methods.
 	querylog.QueryLog
 
-	lastParams querylog.AddParams
+	lastParams *querylog.AddParams
 }
 
 // Add implements the querylog.QueryLog interface for *testQueryLog.
-func (l *testQueryLog) Add(p querylog.AddParams) {
+func (l *testQueryLog) Add(p *querylog.AddParams) {
 	l.lastParams = p
 }
 

internal/querylog/decode.go

@@ -119,6 +119,16 @@ var logEntryHandlers = map[string]logEntryHandler{
 
 		return nil
 	},
+	"AD": func(t json.Token, ent *logEntry) error {
+		v, ok := t.(bool)
+		if !ok {
+			return nil
+		}
+
+		ent.AuthenticatedData = v
+
+		return nil
+	},
 	"Upstream": func(t json.Token, ent *logEntry) error {
 		v, ok := t.(string)
 		if !ok {

internal/querylog/decode_test.go

@@ -34,6 +34,7 @@ func TestDecodeLogEntry(t *testing.T) {
 			`"CP":"",` +
 			`"Answer":"` + ansStr + `",` +
 			`"Cached":true,` +
+			`"AD":true,` +
 			`"Result":{` +
 			`"IsFiltered":true,` +
 			`"Reason":3,` +
@@ -81,8 +82,9 @@ func TestDecodeLogEntry(t *testing.T) {
 					},
 				},
 			},
-			Upstream: "https://some.upstream",
-			Elapsed:  837429,
+			Upstream:          "https://some.upstream",
+			Elapsed:           837429,
+			AuthenticatedData: true,
 		}
 
 		got := &logEntry{}