From f4edb28ef32065242e64ecbf332196cc33dfa56d Mon Sep 17 00:00:00 2001 From: Igor Dubinskiy Date: Wed, 17 Feb 2016 16:54:28 -0800 Subject: [PATCH 1/2] Make sure UDP DNS responses aren't larger than allowed --- command/agent/dns.go | 44 +++++++++++---- command/agent/dns_test.go | 56 +++++++++++++++++++ .../source/docs/agent/options.html.markdown | 5 +- 3 files changed, 91 insertions(+), 14 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index a50486173..c0ee3f773 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -487,6 +487,27 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy return records } +// trimAnswers makes sure a UDP response is not longer than allowed by RFC 1035 +func trimAnswers(resp *dns.Msg) (trimmed bool) { + numAnswers := len(resp.Answer) + + if numAnswers > maxServiceResponses { + resp.Answer = resp.Answer[:maxServiceResponses] + } + + // Check that the response isn't more than 512 bytes + for respBytes := resp.Len(); respBytes > 512; respBytes = resp.Len() { + resp.Answer = resp.Answer[:len(resp.Answer)-1] + + if len(resp.Answer) == 0 { + // We've done all we can + break + } + } + + return len(resp.Answer) < numAnswers +} + // serviceLookup is used to handle a service query func (d *DNSServer) serviceLookup(network, datacenter, service, tag string, req, resp *dns.Msg) { // Make an RPC request @@ -547,17 +568,17 @@ RPC: } // If the network is not TCP, restrict the number of responses - if network != "tcp" && len(resp.Answer) > maxServiceResponses { - resp.Answer = resp.Answer[:maxServiceResponses] + if network != "tcp" { + wasTrimmed := trimAnswers(resp) // Flag that there are more records to return in the UDP response - if d.config.EnableTruncate { + if wasTrimmed && d.config.EnableTruncate { resp.Truncated = true } } - // If the answer is empty, return not found - if len(resp.Answer) == 0 { + // If the answer is empty and the response isn't truncated, return not found + if len(resp.Answer) == 0 && !resp.Truncated { d.addSOA(d.domain, resp) return } @@ -642,18 +663,17 @@ RPC: } // If the network is not TCP, restrict the number of responses. - if network != "tcp" && len(resp.Answer) > maxServiceResponses { - resp.Answer = resp.Answer[:maxServiceResponses] + if network != "tcp" { + wasTrimmed := trimAnswers(resp) - // Flag that there are more records to return in the UDP - // response. - if d.config.EnableTruncate { + // Flag that there are more records to return in the UDP response + if wasTrimmed && d.config.EnableTruncate { resp.Truncated = true } } - // If the answer is empty, return not found. - if len(resp.Answer) == 0 { + // If the answer is empty and the response isn't truncated, return not found + if len(resp.Answer) == 0 && !resp.Truncated { d.addSOA(d.domain, resp) return } diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index 32f4c1f14..afbd6ebbc 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -1961,6 +1961,62 @@ func TestDNS_ServiceLookup_Truncate(t *testing.T) { } } +func TestDNS_ServiceLookup_LargeResponses(t *testing.T) { + dir, srv := makeDNSServerConfig(t, nil, func(c *DNSConfig) { + c.EnableTruncate = true + }) + defer os.RemoveAll(dir) + defer srv.agent.Shutdown() + + testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + + longServiceName := "this-is-a-very-very-very-very-very-long-name-for-a-service" + + // Register 3 nodes + for i := 0; i < 3; i++ { + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: fmt.Sprintf("foo%d", i), + Address: fmt.Sprintf("127.0.0.%d", i+1), + Service: &structs.NodeService{ + Service: longServiceName, + Tags: []string{"master"}, + Port: 12345, + }, + } + + var out struct{} + if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil { + t.Fatalf("err: %v", err) + } + } + + m := new(dns.Msg) + m.SetQuestion("_"+longServiceName+"._master.service.consul.", dns.TypeSRV) + + c := new(dns.Client) + addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS) + in, _, err := c.Exchange(m, addr.String()) + if err != nil && err != dns.ErrTruncated { + t.Fatalf("err: %v", err) + } + + // Make sure the response size is RFC 1035-compliant for UDP messages + if in.Len() > 512 { + t.Fatalf("Bad: %#v", in.Len()) + } + + // We should only have two answers now + if len(in.Answer) != 2 { + t.Fatalf("Bad: %#v", len(in.Answer)) + } + + // Check for the truncate bit + if !in.Truncated { + t.Fatalf("should have truncate bit") + } +} + func TestDNS_ServiceLookup_MaxResponses(t *testing.T) { dir, srv := makeDNSServer(t) defer os.RemoveAll(dir) diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index e227028ea..23c29c6ac 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -493,8 +493,9 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass setting this value. * `enable_truncate` If set to - true, a UDP DNS query that would return more than 3 records will set the truncated flag, - indicating to clients that they should re-query using TCP to get the full set of records. + true, a UDP DNS query that would return more 3 records, or more than would fit into a valid + UDP response, will set the truncated flag, indicating to clients that they should re-query + using TCP to get the full set of records. * `only_passing` If set to true, any nodes whose healthchecks are not passing will be excluded from DNS results. By default (or From b132e8f21bfaf9f3d651f8ece219947808c69de7 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 7 Mar 2016 10:37:54 -0800 Subject: [PATCH 2/2] Tweaks algorithm so it's safe with an empty list and adds a PQ test. --- command/agent/dns.go | 14 ++++----- command/agent/dns_test.go | 62 +++++++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index c0ee3f773..409fca710 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -487,22 +487,20 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy return records } -// trimAnswers makes sure a UDP response is not longer than allowed by RFC 1035 +// trimAnswers makes sure a UDP response is not longer than allowed by RFC 1035. +// We first enforce an arbitrary limit, and then make sure the response doesn't +// exceed 512 bytes. func trimAnswers(resp *dns.Msg) (trimmed bool) { numAnswers := len(resp.Answer) + // This cuts UDP responses to a useful but limited number of responses. if numAnswers > maxServiceResponses { resp.Answer = resp.Answer[:maxServiceResponses] } - // Check that the response isn't more than 512 bytes - for respBytes := resp.Len(); respBytes > 512; respBytes = resp.Len() { + // This enforces the hard limit of 512 bytes per the RFC. + for len(resp.Answer) > 0 && resp.Len() > 512 { resp.Answer = resp.Answer[:len(resp.Answer)-1] - - if len(resp.Answer) == 0 { - // We've done all we can - break - } } return len(resp.Answer) < numAnswers diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index afbd6ebbc..94cc1e14d 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -1991,29 +1991,55 @@ func TestDNS_ServiceLookup_LargeResponses(t *testing.T) { } } - m := new(dns.Msg) - m.SetQuestion("_"+longServiceName+"._master.service.consul.", dns.TypeSRV) - - c := new(dns.Client) - addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS) - in, _, err := c.Exchange(m, addr.String()) - if err != nil && err != dns.ErrTruncated { - t.Fatalf("err: %v", err) + // Register an equivalent prepared query. + { + args := &structs.PreparedQueryRequest{ + Datacenter: "dc1", + Op: structs.PreparedQueryCreate, + Query: &structs.PreparedQuery{ + Name: longServiceName, + Service: structs.ServiceQuery{ + Service: longServiceName, + Tags: []string{"master"}, + }, + }, + } + var id string + if err := srv.agent.RPC("PreparedQuery.Apply", args, &id); err != nil { + t.Fatalf("err: %v", err) + } } - // Make sure the response size is RFC 1035-compliant for UDP messages - if in.Len() > 512 { - t.Fatalf("Bad: %#v", in.Len()) + // Look up the service directly and via prepared query. + questions := []string{ + "_" + longServiceName + "._master.service.consul.", + longServiceName + ".query.consul.", } + for _, question := range questions { + m := new(dns.Msg) + m.SetQuestion(question, dns.TypeSRV) - // We should only have two answers now - if len(in.Answer) != 2 { - t.Fatalf("Bad: %#v", len(in.Answer)) - } + c := new(dns.Client) + addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS) + in, _, err := c.Exchange(m, addr.String()) + if err != nil && err != dns.ErrTruncated { + t.Fatalf("err: %v", err) + } - // Check for the truncate bit - if !in.Truncated { - t.Fatalf("should have truncate bit") + // Make sure the response size is RFC 1035-compliant for UDP messages + if in.Len() > 512 { + t.Fatalf("Bad: %#v", in.Len()) + } + + // We should only have two answers now + if len(in.Answer) != 2 { + t.Fatalf("Bad: %#v", len(in.Answer)) + } + + // Check for the truncate bit + if !in.Truncated { + t.Fatalf("should have truncate bit") + } } }