From f315601a9fd8311efd3353bf5c79faec401e12c9 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 13 Dec 2021 18:06:01 +0300 Subject: [PATCH] Pull request: all: simplify dnssec logic Closes #3904. Squashed commit of the following: commit 5948f0d3519299a1253e388f4bc83e2e55847f68 Author: Ainar Garipov Date: Mon Dec 13 17:53:40 2021 +0300 querylog: imp commit 852cc7dbdb495a17ff51b99ab12901b846f8be09 Author: Ainar Garipov Date: Mon Dec 13 17:44:41 2021 +0300 querylog: fix entry write commit 9d58046899f35162596bfc94fe88fa944309b2fd Author: Ainar Garipov Date: Mon Dec 13 16:45:56 2021 +0300 all: simplify dnssec logic --- CHANGELOG.md | 3 + internal/dnsforward/config.go | 2 +- internal/dnsforward/dns.go | 107 +++++++++++------------------- internal/dnsforward/stats.go | 25 +++---- internal/dnsforward/stats_test.go | 4 +- internal/querylog/decode.go | 10 +++ internal/querylog/decode_test.go | 6 +- internal/querylog/json.go | 105 +++++++++++++++-------------- internal/querylog/qlog.go | 40 ++++++----- internal/querylog/qlog_test.go | 5 +- internal/querylog/querylog.go | 22 ++++-- internal/querylog/search_test.go | 6 +- openapi/openapi.yaml | 2 + 13 files changed, 176 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e0a0ae7..f86cb6e6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index 0fdbf00b..3834f79d 100644 --- a/internal/dnsforward/config.go +++ b/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 diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index b6bafa97..20c86c9f 100644 --- a/internal/dnsforward/dns.go +++ b/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 + 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 diff --git a/internal/dnsforward/stats.go b/internal/dnsforward/stats.go index 67af8ff9..d113a5fd 100644 --- a/internal/dnsforward/stats.go +++ b/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 } diff --git a/internal/dnsforward/stats_test.go b/internal/dnsforward/stats_test.go index aa98a387..bdd4f4f5 100644 --- a/internal/dnsforward/stats_test.go +++ b/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 } diff --git a/internal/querylog/decode.go b/internal/querylog/decode.go index 65c8ada1..a9810629 100644 --- a/internal/querylog/decode.go +++ b/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 { diff --git a/internal/querylog/decode_test.go b/internal/querylog/decode_test.go index fe4e085e..3d651c81 100644 --- a/internal/querylog/decode_test.go +++ b/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{} diff --git a/internal/querylog/json.go b/internal/querylog/json.go index 6f59f1d5..e4bb63aa 100644 --- a/internal/querylog/json.go +++ b/internal/querylog/json.go @@ -40,29 +40,19 @@ 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 != "" { - question["unicode_name"] = qhost - } - } else { - log.Debug("translating %q into unicode: %s", hostname, err) + + 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 } eip := netutil.CloneIP(entry.IP) @@ -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 { diff --git a/internal/querylog/qlog.go b/internal/querylog/qlog.go index f04eecb5..1ca84455 100644 --- a/internal/querylog/qlog.go +++ b/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 diff --git a/internal/querylog/qlog_test.go b/internal/querylog/qlog_test.go index 1990d08f..4e6b76b6 100644 --- a/internal/querylog/qlog_test.go +++ b/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) } diff --git a/internal/querylog/querylog.go b/internal/querylog/querylog.go index 0b39c01e..f3de2aaf 100644 --- a/internal/querylog/querylog.go +++ b/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 + Elapsed time.Duration + ClientID string + ClientIP net.IP + // Upstream is the URL of the upstream DNS server. - Upstream string + 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. diff --git a/internal/querylog/search_test.go b/internal/querylog/search_test.go index 7d5444ba..dbea24a8 100644 --- a/internal/querylog/search_test.go +++ b/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}, diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index ea5aa05b..ef530663 100644 --- a/openapi/openapi.yaml +++ b/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': >