From 6fcca56c1909c8df9a95024abe7350e72b57e763 Mon Sep 17 00:00:00 2001 From: mreiger Date: Wed, 31 Jul 2024 20:37:16 +0200 Subject: [PATCH 1/6] Detect client bufsize and truncate/compress the reply accordingly --- pkg/dns/dns_proxy_handler.go | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index f4dc6a77..27840592 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -58,7 +58,11 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { }() scopedLog.Info("started processing request") - response, err := h.getDataFromDNS(w.LocalAddr(), request) + serverAddress := w.LocalAddr() + bufsize := getBufSize(serverAddress.Network(), request) + scopedLog.Info("Request info", "server", serverAddress, "bufsize", bufsize, "request", request) + + response, err := h.getDataFromDNS(serverAddress, request) if err != nil { scopedLog.Error(err, "failed to get DNS response") err = w.WriteMsg(refusedMsg(request)) @@ -66,7 +70,29 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { } go h.updateCache(time.Now(), response) - err = w.WriteMsg(response) + + scopedLog.Info("Processing and truncating response before sending reply to client", "bufsize", bufsize) + reply := dnsgo.Msg{} + reply = *response + reply.Truncate(bufsize) // response is not (necessarily) compressed even if the original reply of the upstream DNS was. Therefore we must make sure the reply will not exceed downstream's buffer size. + scopedLog.Info("Original response and processed reply", "response", response, "reply", reply) + err = w.WriteMsg(&reply) +} + +func getBufSize(protocol string, request *dnsgo.Msg) int { + if request.Extra != nil { + for _, rr := range request.Extra { + switch r := rr.(type) { + case *dnsgo.OPT: + return int(r.UDPSize()) + } + } + } + + if protocol == "tcp" { + return dnsgo.MaxMsgSize + } + return dnsgo.MinMsgSize } // UpdateDNSServerAddr validates and if successful updates DNS server address From e789dd63756a5f9360d7a61a81560b90de8304a9 Mon Sep 17 00:00:00 2001 From: mreiger Date: Wed, 31 Jul 2024 20:55:43 +0200 Subject: [PATCH 2/6] Declaration instead of assignment to make linter happy --- pkg/dns/dns_proxy_handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index 27840592..f1ed7b7b 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -72,7 +72,7 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { go h.updateCache(time.Now(), response) scopedLog.Info("Processing and truncating response before sending reply to client", "bufsize", bufsize) - reply := dnsgo.Msg{} + var reply dnsgo.Msg reply = *response reply.Truncate(bufsize) // response is not (necessarily) compressed even if the original reply of the upstream DNS was. Therefore we must make sure the reply will not exceed downstream's buffer size. scopedLog.Info("Original response and processed reply", "response", response, "reply", reply) From 68d0b00b875d88350207b2ec94c7969882331182 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Tue, 6 Aug 2024 14:34:26 +0200 Subject: [PATCH 3/6] Review. --- pkg/dns/dns_proxy_handler.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index f1ed7b7b..50237f21 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -60,7 +60,7 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { scopedLog.Info("started processing request") serverAddress := w.LocalAddr() bufsize := getBufSize(serverAddress.Network(), request) - scopedLog.Info("Request info", "server", serverAddress, "bufsize", bufsize, "request", request) + scopedLog.Info("request info", "server", serverAddress, "bufsize", bufsize, "request", request) response, err := h.getDataFromDNS(serverAddress, request) if err != nil { @@ -71,12 +71,12 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { go h.updateCache(time.Now(), response) - scopedLog.Info("Processing and truncating response before sending reply to client", "bufsize", bufsize) - var reply dnsgo.Msg - reply = *response - reply.Truncate(bufsize) // response is not (necessarily) compressed even if the original reply of the upstream DNS was. Therefore we must make sure the reply will not exceed downstream's buffer size. - scopedLog.Info("Original response and processed reply", "response", response, "reply", reply) - err = w.WriteMsg(&reply) + scopedLog.Info("truncating response before sending reply to client", "bufsize", bufsize) + reply := response.Copy() + reply.Truncate(bufsize) // response is not (necessarily) compressed even if the original reply of the upstream DNS was. therefore we must make sure the reply will not exceed downstream's buffer size. + + scopedLog.Info("original response and processed reply", "response", response, "reply", reply) + err = w.WriteMsg(reply) } func getBufSize(protocol string, request *dnsgo.Msg) int { From 3cc7cc23c6818b8cbaa60c91cd3986d1f59ad69d Mon Sep 17 00:00:00 2001 From: mreiger Date: Tue, 6 Aug 2024 17:53:48 +0200 Subject: [PATCH 4/6] Explain the reason for the buffer gymnastics in dns proxy --- pkg/dns/dns_proxy_handler.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index 50237f21..89b19348 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -73,7 +73,13 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { scopedLog.Info("truncating response before sending reply to client", "bufsize", bufsize) reply := response.Copy() - reply.Truncate(bufsize) // response is not (necessarily) compressed even if the original reply of the upstream DNS was. therefore we must make sure the reply will not exceed downstream's buffer size. + /* + Why are we truncating the answer? + DNS has a feature where a DNS server can "compress" the names in a message, and will usually do this if the reply will not fit in the maximum payload length for a DNS packet; for more explanation see here: https://datatracker.ietf.org/doc/html/rfc1035#autoid-44 + Unfortunately, in dnsgo the compression status of a message is apparently not retained, so if you take a compressed message and write it out again your wind up with a bigger message, without any checks whether the resulting message is too big for the receiver. If this happens, it breaks name resolution. + Therefore we find out the buffer size of the client from the request (with UDP it's 512 bytes by default) and limit the reply to this buffer size before we send it out. The Truncate method will try compression first to fit the message into the buffer size and will truncate the message if necessary. + */ + reply.Truncate(bufsize) scopedLog.Info("original response and processed reply", "response", response, "reply", reply) err = w.WriteMsg(reply) From ec6cdfc19f4553aed761e8c8f97101c7d7a4dcd6 Mon Sep 17 00:00:00 2001 From: mreiger Date: Wed, 7 Aug 2024 10:51:15 +0200 Subject: [PATCH 5/6] Truncate before updating the cache so we don't update the cache on information the client will not get --- pkg/dns/dns_proxy_handler.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index 89b19348..1a29d365 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -69,20 +69,20 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { return } - go h.updateCache(time.Now(), response) - scopedLog.Info("truncating response before sending reply to client", "bufsize", bufsize) - reply := response.Copy() + originalResponse := response.Copy() /* Why are we truncating the answer? DNS has a feature where a DNS server can "compress" the names in a message, and will usually do this if the reply will not fit in the maximum payload length for a DNS packet; for more explanation see here: https://datatracker.ietf.org/doc/html/rfc1035#autoid-44 Unfortunately, in dnsgo the compression status of a message is apparently not retained, so if you take a compressed message and write it out again your wind up with a bigger message, without any checks whether the resulting message is too big for the receiver. If this happens, it breaks name resolution. Therefore we find out the buffer size of the client from the request (with UDP it's 512 bytes by default) and limit the reply to this buffer size before we send it out. The Truncate method will try compression first to fit the message into the buffer size and will truncate the message if necessary. */ - reply.Truncate(bufsize) + response.Truncate(bufsize) + scopedLog.Info("original response and processed reply", "original response", originalResponse, "response to client", response) + + go h.updateCache(time.Now(), response) - scopedLog.Info("original response and processed reply", "response", response, "reply", reply) - err = w.WriteMsg(reply) + err = w.WriteMsg(response) } func getBufSize(protocol string, request *dnsgo.Msg) int { From 4fb4d3d8f976e1cd0325daf165e4fd870898fe75 Mon Sep 17 00:00:00 2001 From: mreiger Date: Wed, 7 Aug 2024 11:28:45 +0200 Subject: [PATCH 6/6] Streamlined debug output a bit --- pkg/dns/dns_proxy_handler.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/dns/dns_proxy_handler.go b/pkg/dns/dns_proxy_handler.go index 1a29d365..62fdfb01 100644 --- a/pkg/dns/dns_proxy_handler.go +++ b/pkg/dns/dns_proxy_handler.go @@ -57,10 +57,9 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { } }() - scopedLog.Info("started processing request") serverAddress := w.LocalAddr() bufsize := getBufSize(serverAddress.Network(), request) - scopedLog.Info("request info", "server", serverAddress, "bufsize", bufsize, "request", request) + scopedLog.Info("started processing request", "server", serverAddress, "bufsize", bufsize, "request", request) response, err := h.getDataFromDNS(serverAddress, request) if err != nil { @@ -69,7 +68,6 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { return } - scopedLog.Info("truncating response before sending reply to client", "bufsize", bufsize) originalResponse := response.Copy() /* Why are we truncating the answer? @@ -78,7 +76,7 @@ func (h *DNSProxyHandler) ServeDNS(w dnsgo.ResponseWriter, request *dnsgo.Msg) { Therefore we find out the buffer size of the client from the request (with UDP it's 512 bytes by default) and limit the reply to this buffer size before we send it out. The Truncate method will try compression first to fit the message into the buffer size and will truncate the message if necessary. */ response.Truncate(bufsize) - scopedLog.Info("original response and processed reply", "original response", originalResponse, "response to client", response) + scopedLog.Info("processing response", "buffer size", bufsize, "original response", originalResponse, "truncated response", response) go h.updateCache(time.Now(), response)