Skip to content

Commit 6eee230

Browse files
beautifulentropybeautifulentropy
authored
BDNS: Ensure DNS server addresses are dialable (letsencrypt#5520)
- Add function `validateServerAddress()` to `bdns/servers.go` which ensures that DNS server addresses are TCP/ UDP dial-able per: https://golang.org/src/net/dial.go?#L281 - Add unit test for `validateServerAddress()` in `bdns/servers_test.go` - Update `cmd/boulder-va/main.go` to handle `bdns.NewStaticProvider()` potentially returning an error. - Update unit tests in `bdns/dns_test.go`: - Handle `bdns.NewStaticProvider()` potentially returning an error - Add an IPv6 address to `TestRotateServerOnErr` - Ensure DNS server addresses are validated by `validateServerAddress` whenever: - `dynamicProvider.update() is called` - `staticProvider` is constructed - Construct server addresses using `net.JoinHostPost()` when `dynamicProvider.Addrs()` is called Fixes letsencrypt#5463
1 parent b59f438 commit 6eee230

File tree

5 files changed

+198
-37
lines changed

5 files changed

+198
-37
lines changed

‎bdns/dns_test.go‎

Lines changed: 73 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -243,9 +243,12 @@ func TestMain(m *testing.M) {
243243
}
244244

245245
func TestDNSNoServers(t *testing.T) {
246-
obj := NewTest(time.Hour, NewStaticProvider([]string{}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
246+
staticProvider, err := NewStaticProvider([]string{})
247+
test.AssertNotError(t, err, "Got error creating StaticProvider")
247248

248-
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
249+
obj := NewTest(time.Hour, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
250+
251+
_, err = obj.LookupHost(context.Background(), "letsencrypt.org")
249252
test.AssertError(t, err, "No servers")
250253

251254
_, err = obj.LookupTXT(context.Background(), "letsencrypt.org")
@@ -256,26 +259,35 @@ func TestDNSNoServers(t *testing.T) {
256259
}
257260

258261
func TestDNSOneServer(t *testing.T) {
259-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
262+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
263+
test.AssertNotError(t, err, "Got error creating StaticProvider")
264+
265+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
260266

261-
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
267+
_, err = obj.LookupHost(context.Background(), "letsencrypt.org")
262268

263269
test.AssertNotError(t, err, "No message")
264270
}
265271

266272
func TestDNSDuplicateServers(t *testing.T) {
267-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr, dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
273+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr, dnsLoopbackAddr})
274+
test.AssertNotError(t, err, "Got error creating StaticProvider")
268275

269-
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
276+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
277+
278+
_, err = obj.LookupHost(context.Background(), "letsencrypt.org")
270279

271280
test.AssertNotError(t, err, "No message")
272281
}
273282

274283
func TestDNSServFail(t *testing.T) {
275-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
284+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
285+
test.AssertNotError(t, err, "Got error creating StaticProvider")
286+
287+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
276288
bad := "servfail.com"
277289

278-
_, err := obj.LookupTXT(context.Background(), bad)
290+
_, err = obj.LookupTXT(context.Background(), bad)
279291
test.AssertError(t, err, "LookupTXT didn't return an error")
280292

281293
_, err = obj.LookupHost(context.Background(), bad)
@@ -287,7 +299,10 @@ func TestDNSServFail(t *testing.T) {
287299
}
288300

289301
func TestDNSLookupTXT(t *testing.T) {
290-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
302+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
303+
test.AssertNotError(t, err, "Got error creating StaticProvider")
304+
305+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
291306

292307
a, err := obj.LookupTXT(context.Background(), "letsencrypt.org")
293308
t.Logf("A: %v", a)
@@ -301,7 +316,10 @@ func TestDNSLookupTXT(t *testing.T) {
301316
}
302317

303318
func TestDNSLookupHost(t *testing.T) {
304-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
319+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
320+
test.AssertNotError(t, err, "Got error creating StaticProvider")
321+
322+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
305323

306324
ip, err := obj.LookupHost(context.Background(), "servfail.com")
307325
t.Logf("servfail.com - IP: %s, Err: %s", ip, err)
@@ -366,10 +384,13 @@ func TestDNSLookupHost(t *testing.T) {
366384
}
367385

368386
func TestDNSNXDOMAIN(t *testing.T) {
369-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
387+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
388+
test.AssertNotError(t, err, "Got error creating StaticProvider")
389+
390+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
370391

371392
hostname := "nxdomain.letsencrypt.org"
372-
_, err := obj.LookupHost(context.Background(), hostname)
393+
_, err = obj.LookupHost(context.Background(), hostname)
373394
expected := &Error{dns.TypeA, hostname, nil, dns.RcodeNameError}
374395
test.AssertDeepEquals(t, err, expected)
375396

@@ -379,7 +400,10 @@ func TestDNSNXDOMAIN(t *testing.T) {
379400
}
380401

381402
func TestDNSLookupCAA(t *testing.T) {
382-
obj := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
403+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
404+
test.AssertNotError(t, err, "Got error creating StaticProvider")
405+
406+
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
383407
removeIDExp := regexp.MustCompile(" id: [[:digit:]]+")
384408

385409
caas, resp, err := obj.LookupCAA(context.Background(), "bracewel.net")
@@ -600,10 +624,13 @@ func TestRetry(t *testing.T) {
600624

601625
for i, tc := range tests {
602626
t.Run(tc.name, func(t *testing.T) {
603-
testClient := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, blog.UseMock())
627+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
628+
test.AssertNotError(t, err, "Got error creating StaticProvider")
629+
630+
testClient := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, blog.UseMock())
604631
dr := testClient.(*impl)
605632
dr.dnsClient = tc.te
606-
_, err := dr.LookupTXT(context.Background(), "example.com")
633+
_, err = dr.LookupTXT(context.Background(), "example.com")
607634
if err == errTooManyRequests {
608635
t.Errorf("#%d, sent more requests than the test case handles", i)
609636
}
@@ -627,12 +654,15 @@ func TestRetry(t *testing.T) {
627654
})
628655
}
629656

630-
testClient := NewTest(time.Second*10, NewStaticProvider([]string{dnsLoopbackAddr}), metrics.NoopRegisterer, clock.NewFake(), 3, blog.UseMock())
657+
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
658+
test.AssertNotError(t, err, "Got error creating StaticProvider")
659+
660+
testClient := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, blog.UseMock())
631661
dr := testClient.(*impl)
632662
dr.dnsClient = &testExchanger{errs: []error{isTempErr, isTempErr, nil}}
633663
ctx, cancel := context.WithCancel(context.Background())
634664
cancel()
635-
_, err := dr.LookupTXT(ctx, "example.com")
665+
_, err = dr.LookupTXT(ctx, "example.com")
636666
if err == nil ||
637667
err.Error() != "DNS problem: query timed out (and was canceled) looking up TXT for example.com" {
638668
t.Errorf("expected %s, got %s", context.Canceled, err)
@@ -710,38 +740,51 @@ func (e *rotateFailureExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.
710740
func TestRotateServerOnErr(t *testing.T) {
711741
// Configure three DNS servers
712742
dnsServers := []string{
713-
"a", "b", "c",
743+
"a:53", "b:53", "[2606:4700:4700::1111]:53",
714744
}
745+
715746
// Set up a DNS client using these servers that will retry queries up to
716747
// a maximum of 5 times. It's important to choose a maxTries value >= the
717748
// number of dnsServers to ensure we always get around to trying the one
718749
// working server
750+
staticProvider, err := NewStaticProvider(dnsServers)
751+
test.AssertNotError(t, err, "Got error creating StaticProvider")
752+
fmt.Println(staticProvider.servers)
753+
719754
maxTries := 5
720-
client := NewTest(time.Second*10, NewStaticProvider(dnsServers), metrics.NoopRegisterer, clock.NewFake(), maxTries, blog.UseMock())
755+
client := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), maxTries, blog.UseMock())
721756

722757
// Configure a mock exchanger that will always return a retryable error for
723-
// the A and B servers. This will force the C server to do all the work once
724-
// retries reach it.
758+
// servers A and B. This will force server "[2606:4700:4700::1111]:53" to do
759+
// all the work once retries reach it.
725760
mock := &rotateFailureExchanger{
726-
brokenAddresses: map[string]bool{"a": true, "b": true},
727-
lookups: make(map[string]int),
761+
brokenAddresses: map[string]bool{
762+
"a:53": true,
763+
"b:53": true,
764+
},
765+
lookups: make(map[string]int),
728766
}
729767
client.(*impl).dnsClient = mock
730768

731769
// Perform a bunch of lookups. We choose the initial server randomly. Any time
732770
// A or B is chosen there should be an error and a retry using the next server
733771
// in the list. Since we configured maxTries to be larger than the number of
734772
// servers *all* queries should eventually succeed by being retried against
735-
// the C server.
773+
// server "[2606:4700:4700::1111]:53".
736774
for i := 0; i < maxTries*2; i++ {
737775
_, err := client.LookupTXT(context.Background(), "example.com")
738-
// Any errors are unexpected - the C server should have responded without error.
776+
// Any errors are unexpected - server "[2606:4700:4700::1111]:53" should
777+
// have responded without error.
739778
test.AssertNotError(t, err, "Expected no error from eventual retry with functional server")
740779
}
741780

742-
// We expect that the A and B servers had a non-zero number of lookups attempted
743-
test.Assert(t, mock.lookups["a"] > 0, "Expected A server to have non-zero lookup attempts")
744-
test.Assert(t, mock.lookups["b"] > 0, "Expected B server to have non-zero lookup attempts")
745-
// We expect that the C server eventually served all of the lookups attempted
746-
test.AssertEquals(t, mock.lookups["c"], maxTries*2)
781+
// We expect that the A and B servers had a non-zero number of lookups
782+
// attempted.
783+
test.Assert(t, mock.lookups["a:53"] > 0, "Expected A server to have non-zero lookup attempts")
784+
test.Assert(t, mock.lookups["b:53"] > 0, "Expected B server to have non-zero lookup attempts")
785+
786+
// We expect that the server "[2606:4700:4700::1111]:53" eventually served
787+
// all of the lookups attempted.
788+
test.AssertEquals(t, mock.lookups["[2606:4700:4700::1111]:53"], maxTries*2)
789+
747790
}

‎bdns/servers.go‎

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@ package bdns
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67
"math/rand"
78
"net"
9+
"strconv"
810
"sync"
911
"time"
1012

13+
"github.com/miekg/dns"
1114
"github.com/prometheus/client_golang/prometheus"
1215
)
1316

@@ -29,8 +32,52 @@ type staticProvider struct {
2932

3033
var _ ServerProvider = &staticProvider{}
3134

32-
func NewStaticProvider(servers []string) *staticProvider {
33-
return &staticProvider{servers: servers}
35+
// validateServerAddress ensures that a given server address is formatted in
36+
// such a way that it can be dialed. The provided server address must include a
37+
// host/IP and port separated by colon. Additionally, if the host is a literal
38+
// IPv6 address, it must be enclosed in square brackets.
39+
// (https://golang.org/src/net/dial.go?s=9833:9881#L281)
40+
func validateServerAddress(address string) error {
41+
// Ensure the host and port portions of `address` can be split.
42+
host, port, err := net.SplitHostPort(address)
43+
if err != nil {
44+
return err
45+
}
46+
47+
// Ensure `address` contains both a `host` and `port` portion.
48+
if host == "" || port == "" {
49+
return errors.New("port cannot be missing")
50+
}
51+
52+
// Ensure the `port` portion of `address` is a valid port.
53+
portNum, err := strconv.Atoi(port)
54+
if err != nil {
55+
return errors.New("port must be an integer: %s")
56+
}
57+
if portNum <= 0 || portNum > 65535 {
58+
return errors.New("port must be an integer between 0 - 65535")
59+
}
60+
61+
// Ensure the `host` portion of `address` is a valid FQDN or IP address.
62+
IPv6 := net.ParseIP(host).To16()
63+
IPv4 := net.ParseIP(host).To4()
64+
FQDN := dns.IsFqdn(dns.Fqdn(host))
65+
if IPv6 == nil && IPv4 == nil && !FQDN {
66+
return errors.New("host is not an FQDN or IP address")
67+
}
68+
return nil
69+
}
70+
71+
func NewStaticProvider(servers []string) (*staticProvider, error) {
72+
var serverAddrs []string
73+
for _, server := range servers {
74+
err := validateServerAddress(server)
75+
if err != nil {
76+
return nil, fmt.Errorf("server address %q invalid: %s", server, err)
77+
}
78+
serverAddrs = append(serverAddrs, server)
79+
}
80+
return &staticProvider{servers: serverAddrs}, nil
3481
}
3582

3683
func (sp *staticProvider) Addrs() ([]string, error) {
@@ -137,7 +184,7 @@ func (dp *dynamicProvider) update() error {
137184
if err != nil {
138185
return fmt.Errorf("failed to lookup SRV records for %q: %w", dp.name, err)
139186
}
140-
if srvs == nil || len(srvs) == 0 {
187+
if len(srvs) == 0 {
141188
return fmt.Errorf("no SRV records found for %q", dp.name)
142189
}
143190

@@ -148,6 +195,11 @@ func (dp *dynamicProvider) update() error {
148195
return fmt.Errorf("failed to resolve SRV Target %q: %w", srv.Target, err)
149196
}
150197
for _, addr := range addrs {
198+
joinedHostPort := net.JoinHostPort(addr, fmt.Sprint(srv.Port))
199+
err := validateServerAddress(joinedHostPort)
200+
if err != nil {
201+
return fmt.Errorf("invalid SRV addr %q: %w", joinedHostPort, err)
202+
}
151203
addrPorts[addr] = append(addrPorts[addr], srv.Port)
152204
}
153205
}
@@ -164,8 +216,8 @@ func (dp *dynamicProvider) Addrs() ([]string, error) {
164216
var r []string
165217
dp.mu.RLock()
166218
for ip, ports := range dp.addrs {
167-
port := ports[rand.Intn(len(ports))]
168-
addr := fmt.Sprintf("%s:%d", ip, port)
219+
port := fmt.Sprint(ports[rand.Intn(len(ports))])
220+
addr := net.JoinHostPort(ip, port)
169221
r = append(r, addr)
170222
}
171223
dp.mu.RUnlock()

‎bdns/servers_test.go‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package bdns
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func Test_validateServerAddress(t *testing.T) {
8+
type args struct {
9+
server string
10+
}
11+
tests := []struct {
12+
name string
13+
args args
14+
wantErr bool
15+
}{
16+
// ipv4 cases
17+
{"ipv4 with port", args{"1.1.1.1:53"}, false},
18+
// sad path
19+
{"ipv4 without port", args{"1.1.1.1"}, true},
20+
{"ipv4 port num missing", args{"1.1.1.1:"}, true},
21+
{"ipv4 string for port", args{"1.1.1.1:foo"}, true},
22+
{"ipv4 port out of range high", args{"1.1.1.1:65536"}, true},
23+
{"ipv4 port out of range low", args{"1.1.1.1:0"}, true},
24+
25+
// ipv6 cases
26+
{"ipv6 with port", args{"[2606:4700:4700::1111]:53"}, false},
27+
// sad path
28+
{"ipv6 sans brackets", args{"2606:4700:4700::1111:53"}, true},
29+
{"ipv6 without port", args{"[2606:4700:4700::1111]"}, true},
30+
{"ipv6 port num missing", args{"[2606:4700:4700::1111]:"}, true},
31+
{"ipv6 string for port", args{"[2606:4700:4700::1111]:foo"}, true},
32+
{"ipv6 port out of range high", args{"[2606:4700:4700::1111]:65536"}, true},
33+
{"ipv6 port out of range low", args{"[2606:4700:4700::1111]:0"}, true},
34+
35+
// hostname cases
36+
{"hostname with port", args{"foo:53"}, false},
37+
// sad path
38+
{"hostname without port", args{"foo"}, true},
39+
{"hostname port num missing", args{"foo:"}, true},
40+
{"hostname string for port", args{"foo:bar"}, true},
41+
{"hostname port out of range high", args{"foo:65536"}, true},
42+
{"hostname port out of range low", args{"foo:0"}, true},
43+
44+
// fqdn cases
45+
{"fqdn with port", args{"bar.foo.baz:53"}, false},
46+
// sad path
47+
{"fqdn without port", args{"bar.foo.baz"}, true},
48+
{"fqdn port num missing", args{"bar.foo.baz:"}, true},
49+
{"fqdn string for port", args{"bar.foo.baz:bar"}, true},
50+
{"fqdn port out of range high", args{"bar.foo.baz:65536"}, true},
51+
{"fqdn port out of range low", args{"bar.foo.baz:0"}, true},
52+
}
53+
for _, tt := range tests {
54+
t.Run(tt.name, func(t *testing.T) {
55+
err := validateServerAddress(tt.args.server)
56+
if (err != nil) != tt.wantErr {
57+
t.Errorf("formatServer() error = %v, wantErr %v", err, tt.wantErr)
58+
return
59+
}
60+
})
61+
}
62+
}

‎cmd/boulder-va/main.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ func main() {
128128
servers, err = bdns.StartDynamicProvider(c.VA.DNSResolver, 60*time.Second)
129129
cmd.FailOnError(err, "Couldn't start dynamic DNS server resolver")
130130
} else {
131-
servers = bdns.NewStaticProvider(c.VA.DNSResolvers)
131+
servers, err = bdns.NewStaticProvider(c.VA.DNSResolvers)
132+
cmd.FailOnError(err, "Couldn't parse static DNS server(s)")
132133
}
133134

134135
var resolver bdns.Client

‎va/dns_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,12 @@ func TestDNSValidationServFail(t *testing.T) {
136136

137137
func TestDNSValidationNoServer(t *testing.T) {
138138
va, log := setup(nil, 0, "", nil)
139+
staticProvider, err := bdns.NewStaticProvider([]string{})
140+
test.AssertNotError(t, err, "Couldn't make new static provider")
141+
139142
va.dnsClient = bdns.NewTest(
140143
time.Second*5,
141-
bdns.NewStaticProvider([]string{}),
144+
staticProvider,
142145
metrics.NoopRegisterer,
143146
clock.New(),
144147
1,

0 commit comments

Comments
 (0)