Pull request: all: simplify dnssec logic

Closes #3904.

Squashed commit of the following:

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

    querylog: imp

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

    querylog: fix entry write

commit 9d58046899f35162596bfc94fe88fa944309b2fd
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
 	// 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
+	dctx.responseFromUpstream = true
+	dctx.responseAD = pctx.Res.AuthenticatedData
 
-	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
-		}
-
-		// 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{
+		p := &querylog.AddParams{
 			Question:          msg,
 			Answer:            pctx.Res,
-			OrigAnswer: ctx.origResp,
-			Result:     ctx.result,
+			OrigAnswer:        dctx.origResp,
+			Result:            dctx.result,
 			Elapsed:           elapsed,
+			ClientID:          dctx.clientID,
 			ClientIP:          ip,
-			ClientID:   ctx.clientID,
+			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,` +
@@ -83,6 +84,7 @@ func TestDecodeLogEntry(t *testing.T) {
 			},
 			Upstream:          "https://some.upstream",
 			Elapsed:           837429,
+			AuthenticatedData: true,
 		}
 
 		got := &logEntry{}

internal/querylog/json.go

@@ -40,30 +40,20 @@ func (l *queryLog) entriesToJSON(entries []*logEntry, oldest time.Time) (res job
 	return res
 }
 
+// entryToJSON converts a log entry's data into an entry for the JSON API.
 func (l *queryLog) entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (jsonEntry jobject) {
-	var msg *dns.Msg
-
-	if len(entry.Answer) > 0 {
-		msg = new(dns.Msg)
-		if err := msg.Unpack(entry.Answer); err != nil {
-			log.Debug("Failed to unpack dns message answer: %s: %s", err, string(entry.Answer))
-			msg = nil
-		}
-	}
-
 	hostname := entry.QHost
 	question := jobject{
 		"type":  entry.QType,
 		"class": entry.QClass,
 		"name":  hostname,
 	}
-	if qhost, err := idna.ToUnicode(hostname); err == nil {
-		if qhost != hostname && qhost != "" {
+
+	if qhost, err := idna.ToUnicode(hostname); err != nil {
+		log.Debug("querylog: translating %q into unicode: %s", hostname, err)
+	} else if qhost != hostname && qhost != "" {
 		question["unicode_name"] = qhost
 	}
-	} else {
-		log.Debug("translating %q into unicode: %s", hostname, err)
-	}
 
 	eip := netutil.CloneIP(entry.IP)
 	anonFunc(eip)
@@ -77,7 +67,9 @@ func (l *queryLog) entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (json
 		"cached":       entry.Cached,
 		"upstream":     entry.Upstream,
 		"question":     question,
+		"rules":        resultRulesToJSONRules(entry.Result.Rules),
 	}
+
 	if eip.Equal(entry.IP) {
 		jsonEntry["client_info"] = entry.client
 	}
@@ -86,50 +78,65 @@ func (l *queryLog) entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (json
 		jsonEntry["client_id"] = entry.ClientID
 	}
 
-	if msg != nil {
-		jsonEntry["status"] = dns.RcodeToString[msg.Rcode]
-
-		opt := msg.IsEdns0()
-		dnssecOk := false
-		if opt != nil {
-			dnssecOk = opt.Do()
+	if len(entry.Result.Rules) > 0 {
+		if r := entry.Result.Rules[0]; len(r.Text) > 0 {
+			jsonEntry["rule"] = r.Text
+			jsonEntry["filterId"] = r.FilterListID
 		}
-
-		jsonEntry["answer_dnssec"] = dnssecOk
-	}
-
-	jsonEntry["rules"] = resultRulesToJSONRules(entry.Result.Rules)
-
-	if len(entry.Result.Rules) > 0 && len(entry.Result.Rules[0].Text) > 0 {
-		jsonEntry["rule"] = entry.Result.Rules[0].Text
-		jsonEntry["filterId"] = entry.Result.Rules[0].FilterListID
 	}
 
 	if len(entry.Result.ServiceName) != 0 {
 		jsonEntry["service_name"] = entry.Result.ServiceName
 	}
 
-	answers := answerToMap(msg)
-	if answers != nil {
-		jsonEntry["answer"] = answers
-	}
-
-	if len(entry.OrigAnswer) != 0 {
-		a := new(dns.Msg)
-		err := a.Unpack(entry.OrigAnswer)
-		if err == nil {
-			answers = answerToMap(a)
-			if answers != nil {
-				jsonEntry["original_answer"] = answers
-			}
-		} else {
-			log.Debug("Querylog: msg.Unpack(entry.OrigAnswer): %s: %s", err, string(entry.OrigAnswer))
-		}
-	}
+	l.setMsgData(entry, jsonEntry)
+	l.setOrigAns(entry, jsonEntry)
 
 	return jsonEntry
 }
 
+// setMsgData sets the message data in jsonEntry.
+func (l *queryLog) setMsgData(entry *logEntry, jsonEntry jobject) {
+	if len(entry.Answer) == 0 {
+		return
+	}
+
+	msg := &dns.Msg{}
+	if err := msg.Unpack(entry.Answer); err != nil {
+		log.Debug("querylog: failed to unpack dns msg answer: %v: %s", entry.Answer, err)
+
+		return
+	}
+
+	jsonEntry["status"] = dns.RcodeToString[msg.Rcode]
+	// Old query logs may still keep AD flag value in the message.  Try to get
+	// it from there as well.
+	jsonEntry["answer_dnssec"] = entry.AuthenticatedData || msg.AuthenticatedData
+
+	if a := answerToMap(msg); a != nil {
+		jsonEntry["answer"] = a
+	}
+}
+
+// setOrigAns sets the original answer data in jsonEntry.
+func (l *queryLog) setOrigAns(entry *logEntry, jsonEntry jobject) {
+	if len(entry.OrigAnswer) == 0 {
+		return
+	}
+
+	orig := &dns.Msg{}
+	err := orig.Unpack(entry.OrigAnswer)
+	if err != nil {
+		log.Debug("querylog: orig.Unpack(entry.OrigAnswer): %v: %s", entry.OrigAnswer, err)
+
+		return
+	}
+
+	if a := answerToMap(orig); a != nil {
+		jsonEntry["original_answer"] = a
+	}
+}
+
 func resultRulesToJSONRules(rules []*filtering.ResultRule) (jsonRules []jobject) {
 	jsonRules = make([]jobject, len(rules))
 	for i, r := range rules {

internal/querylog/qlog.go

@@ -75,7 +75,6 @@ type logEntry struct {
 	// client is the found client information, if any.
 	client *Client
 
-	IP   net.IP    `json:"IP"` // Client IP
 	Time time.Time `json:"T"`
 
 	QHost  string `json:"QH"`
@@ -87,11 +86,16 @@ type logEntry struct {
 
 	Answer     []byte `json:",omitempty"` // sometimes empty answers happen like binerdunt.top or rev2.globalrootservers.net
 	OrigAnswer []byte `json:",omitempty"`
-	Cached     bool   `json:",omitempty"`
 
 	Result   filtering.Result
-	Elapsed  time.Duration
 	Upstream string `json:",omitempty"`
+
+	IP net.IP `json:"IP"`
+
+	Elapsed time.Duration
+
+	Cached            bool `json:",omitempty"`
+	AuthenticatedData bool `json:"AD,omitempty"`
 }
 
 func (l *queryLog) Start() {
@@ -146,14 +150,12 @@ func (l *queryLog) clear() {
 	log.Debug("Query log: cleared")
 }
 
-func (l *queryLog) Add(params AddParams) {
-	var err error
-
+func (l *queryLog) Add(params *AddParams) {
 	if !l.conf.Enabled {
 		return
 	}
 
-	err = params.validate()
+	err := params.validate()
 	if err != nil {
 		log.Error("querylog: adding record: %s, skipping", err)
 
@@ -165,21 +167,27 @@ func (l *queryLog) Add(params AddParams) {
 	}
 
 	now := time.Now()
+	q := params.Question.Question[0]
 	entry := logEntry{
-		IP:   params.ClientIP,
 		Time: now,
 
-		Result:      *params.Result,
-		Elapsed:     params.Elapsed,
-		Upstream:    params.Upstream,
-		Cached:      params.Cached,
+		QHost:  strings.ToLower(q.Name[:len(q.Name)-1]),
+		QType:  dns.Type(q.Qtype).String(),
+		QClass: dns.Class(q.Qclass).String(),
+
 		ClientID:    params.ClientID,
 		ClientProto: params.ClientProto,
+
+		Result:   *params.Result,
+		Upstream: params.Upstream,
+
+		IP: params.ClientIP,
+
+		Elapsed: params.Elapsed,
+
+		Cached:            params.Cached,
+		AuthenticatedData: params.AuthenticatedData,
 	}
-	q := params.Question.Question[0]
-	entry.QHost = strings.ToLower(q.Name[:len(q.Name)-1]) // remove the last dot
-	entry.QType = dns.Type(q.Qtype).String()
-	entry.QClass = dns.Class(q.Qclass).String()
 
 	if params.Answer != nil {
 		var a []byte

internal/querylog/qlog_test.go

@@ -269,6 +269,7 @@ func addEntry(l *queryLog, host string, answerStr, client net.IP) {
 			A: answerStr,
 		}},
 	}
+
 	res := filtering.Result{
 		IsFiltered:  true,
 		Reason:      filtering.Rewritten,
@@ -278,7 +279,8 @@ func addEntry(l *queryLog, host string, answerStr, client net.IP) {
 			Text:         "SomeRule",
 		}},
 	}
-	params := AddParams{
+
+	params := &AddParams{
 		Question:   &q,
 		Answer:     &a,
 		OrigAnswer: &a,
@@ -286,6 +288,7 @@ func addEntry(l *queryLog, host string, answerStr, client net.IP) {
 		ClientIP:   client,
 		Upstream:   "upstream",
 	}
+
 	l.Add(params)
 }
 

internal/querylog/querylog.go

@@ -22,7 +22,7 @@ type QueryLog interface {
 	Close()
 
 	// Add a log entry
-	Add(params AddParams)
+	Add(params *AddParams)
 
 	// WriteDiskConfig - write configuration
 	WriteDiskConfig(c *Config)
@@ -76,22 +76,34 @@ type Config struct {
 // AddParams is the parameters for adding an entry.
 type AddParams struct {
 	Question *dns.Msg
+
 	// Answer is the response which is sent to the client, if any.
 	Answer *dns.Msg
+
 	// OrigAnswer is the response from an upstream server.  It's only set if the
 	// answer has been modified by filtering.
 	OrigAnswer *dns.Msg
-	// Cached indicates if the response is served from cache.
-	Cached bool
+
 	// Result is the filtering result (optional).
 	Result *filtering.Result
+
 	// Elapsed is the time spent for processing the request.
 	Elapsed time.Duration
+
 	ClientID string
+
 	ClientIP net.IP
+
 	// Upstream is the URL of the upstream DNS server.
 	Upstream string
+
 	ClientProto ClientProto
+
+	// Cached indicates if the response is served from cache.
+	Cached bool
+
+	// AuthenticatedData shows if the response had the AD bit set.
+	AuthenticatedData bool
 }
 
 // validate returns an error if the parameters aren't valid.

internal/querylog/search_test.go

@@ -52,20 +52,20 @@ func TestQueryLog_Search_findClient(t *testing.T) {
 		}},
 	}
 
-	l.Add(AddParams{
+	l.Add(&AddParams{
 		Question: q,
 		ClientID: knownClientID,
 		ClientIP: net.IP{1, 2, 3, 4},
 	})
 
 	// Add the same thing again to test the cache.
-	l.Add(AddParams{
+	l.Add(&AddParams{
 		Question: q,
 		ClientID: knownClientID,
 		ClientIP: net.IP{1, 2, 3, 4},
 	})
 
-	l.Add(AddParams{
+	l.Add(&AddParams{
 		Question: q,
 		ClientID: unknownClientID,
 		ClientIP: net.IP{1, 2, 3, 5},

openapi/openapi.yaml

@@ -1878,6 +1878,8 @@
             Upstream URL starting with tcp://, tls://, https://, or with an IP
             address.
         'answer_dnssec':
+          'description': >
+            If true, the response had the Authenticated Data (AD) flag set.
           'type': 'boolean'
         'client':
           'description': >