Skip to content

Commit be957c2

Browse files
aarongableaarongable
authored
Refactor DNS to pass full messages up to the VA (#8513)
Change the exported methods of the bdns package to have consistent arguments and return values. Specifically: - Split LookupHost into two more specific methods, LookupA and LookupAAAA. - Since each method now returns the results of a single DNS query, update all four Lookup methods to return only a single resolver, rather than a slice of resolver addresses. - Create a new generic "Result" type, which embeds the upstream miekg/dns.Msg type, and also helpfully extracts all the answer records of a specific type. This embedding is important for the future, since we want to extract this result's Authenticated Data bit and all CNAME hops for logging in the VA. - Use this new type as the return type for all four Lookup methods, removing the custom pre-processing (e.g. producing a dig-like string for CAA lookups, or filtering out Restricted IP addresses) from each method. This unification and simplification allows us to streamline the body of bdns.exchangeOne, to reduce code duplication around incrementing metrics and clarify the retry loop's exit condition. Importantly, bdns.Exchange (which actually performs the DoH network calls) remains untouched. As a result of the changes to LookupA and LookupAAAA, move most of LookupHost's old logic into the VA's getAddrs function. We're still doing the exact same merging and filtering of IP addresses, just within the VA rather than within the DNS abstraction layer. This also moves usage of the `allowRestrictedAddrs` boolean from bdns.Client to va.impl, and allows us to remove bdns.NewTest. These interface changes require extensive test changes to match. Strip the bdns.MockClient down to almost nothing, because its guts had far too much knowledge of the desires of specific tests in a wholly different package (the VA). Instead, break the MockClient down into three much smaller fakes, and move them into the VA test files that actually need them. Have each test case explicitly specify which fake it is using, encouraging the use of smaller and more test-specific fakes in the future. Finally, use this opportunity to turn multiple linear tests into table-driven tests. Part of #2700
1 parent 3922886 commit be957c2

File tree

16 files changed

+949
-1002
lines changed

16 files changed

+949
-1002
lines changed

‎bdns/dns.go‎

Lines changed: 187 additions & 306 deletions
Large diffs are not rendered by default.

‎bdns/dns_test.go‎

Lines changed: 244 additions & 172 deletions
Large diffs are not rendered by default.

‎bdns/mocks.go‎

Lines changed: 12 additions & 142 deletions
Original file line numberDiff line numberDiff line change
@@ -3,159 +3,29 @@ package bdns
33
import (
44
"context"
55
"errors"
6-
"fmt"
7-
"net"
8-
"net/netip"
9-
"os"
106

117
"github.com/miekg/dns"
12-
13-
blog "github.com/letsencrypt/boulder/log"
148
)
159

1610
// MockClient is a mock
17-
type MockClient struct {
18-
Log blog.Logger
19-
}
11+
type MockClient struct{}
2012

2113
// LookupTXT is a mock
22-
func (mock *MockClient) LookupTXT(_ context.Context, hostname string) ([]string, ResolverAddrs, error) {
23-
// Use the example account-specific label prefix derived from
24-
// "https://example.com/acme/acct/ExampleAccount"
25-
const accountLabelPrefix = "_ujmmovf2vn55tgye._acme-challenge"
26-
27-
if hostname == accountLabelPrefix+".servfail.com" {
28-
// Mirror dns-01 servfail behaviour
29-
return nil, ResolverAddrs{"MockClient"}, fmt.Errorf("SERVFAIL")
30-
}
31-
if hostname == accountLabelPrefix+".good-dns01.com" {
32-
// Mirror dns-01 good record
33-
// base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
34-
// + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"))
35-
return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil
36-
}
37-
if hostname == accountLabelPrefix+".wrong-dns01.com" {
38-
// Mirror dns-01 wrong record
39-
return []string{"a"}, ResolverAddrs{"MockClient"}, nil
40-
}
41-
if hostname == accountLabelPrefix+".wrong-many-dns01.com" {
42-
// Mirror dns-01 wrong-many record
43-
return []string{"a", "b", "c", "d", "e"}, ResolverAddrs{"MockClient"}, nil
44-
}
45-
if hostname == accountLabelPrefix+".long-dns01.com" {
46-
// Mirror dns-01 long record
47-
return []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, ResolverAddrs{"MockClient"}, nil
48-
}
49-
if hostname == accountLabelPrefix+".no-authority-dns01.com" {
50-
// Mirror dns-01 no-authority good record
51-
// base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
52-
// + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"))
53-
return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil
54-
}
55-
if hostname == accountLabelPrefix+".empty-txts.com" {
56-
// Mirror dns-01 zero TXT records
57-
return []string{}, ResolverAddrs{"MockClient"}, nil
58-
}
59-
60-
if hostname == "_acme-challenge.servfail.com" {
61-
return nil, ResolverAddrs{"MockClient"}, fmt.Errorf("SERVFAIL")
62-
}
63-
if hostname == "_acme-challenge.good-dns01.com" {
64-
// base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
65-
// + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"))
66-
// expected token + test account jwk thumbprint
67-
return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil
68-
}
69-
if hostname == "_acme-challenge.wrong-dns01.com" {
70-
return []string{"a"}, ResolverAddrs{"MockClient"}, nil
71-
}
72-
if hostname == "_acme-challenge.wrong-many-dns01.com" {
73-
return []string{"a", "b", "c", "d", "e"}, ResolverAddrs{"MockClient"}, nil
74-
}
75-
if hostname == "_acme-challenge.long-dns01.com" {
76-
return []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, ResolverAddrs{"MockClient"}, nil
77-
}
78-
if hostname == "_acme-challenge.no-authority-dns01.com" {
79-
// base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
80-
// + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"))
81-
// expected token + test account jwk thumbprint
82-
return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil
83-
}
84-
// empty-txts.com always returns zero TXT records
85-
if hostname == "_acme-challenge.empty-txts.com" {
86-
return []string{}, ResolverAddrs{"MockClient"}, nil
87-
}
88-
89-
// Default fallback
90-
return []string{"hostname"}, ResolverAddrs{"MockClient"}, nil
14+
func (mock *MockClient) LookupTXT(_ context.Context, hostname string) (*Result[*dns.TXT], string, error) {
15+
return nil, "MockClient", errors.New("unexpected LookupTXT call on test fake")
9116
}
9217

93-
// makeTimeoutError returns a a net.OpError for which Timeout() returns true.
94-
func makeTimeoutError() *net.OpError {
95-
return &net.OpError{
96-
Err: os.NewSyscallError("ugh timeout", timeoutError{}),
97-
}
98-
}
99-
100-
type timeoutError struct{}
101-
102-
func (t timeoutError) Error() string {
103-
return "so sloooow"
104-
}
105-
func (t timeoutError) Timeout() bool {
106-
return true
18+
// LookupA is a fake
19+
func (mock *MockClient) LookupA(_ context.Context, hostname string) (*Result[*dns.A], string, error) {
20+
return nil, "MockClient", errors.New("unexpected LookupA call on test fake")
10721
}
10822

109-
// LookupHost is a mock
110-
func (mock *MockClient) LookupHost(_ context.Context, hostname string) ([]netip.Addr, ResolverAddrs, error) {
111-
if hostname == "always.invalid" ||
112-
hostname == "invalid.invalid" {
113-
return []netip.Addr{}, ResolverAddrs{"MockClient"}, nil
114-
}
115-
if hostname == "always.timeout" {
116-
return []netip.Addr{}, ResolverAddrs{"MockClient"}, &Error{dns.TypeA, "always.timeout", makeTimeoutError(), -1, nil}
117-
}
118-
if hostname == "always.error" {
119-
err := &net.OpError{
120-
Op: "read",
121-
Net: "udp",
122-
Err: errors.New("some net error"),
123-
}
124-
m := new(dns.Msg)
125-
m.SetQuestion(dns.Fqdn(hostname), dns.TypeA)
126-
m.AuthenticatedData = true
127-
m.SetEdns0(4096, false)
128-
return []netip.Addr{}, ResolverAddrs{"MockClient"}, &Error{dns.TypeA, hostname, err, -1, nil}
129-
}
130-
if hostname == "id.mismatch" {
131-
err := dns.ErrId
132-
m := new(dns.Msg)
133-
m.SetQuestion(dns.Fqdn(hostname), dns.TypeA)
134-
m.AuthenticatedData = true
135-
m.SetEdns0(4096, false)
136-
r := new(dns.Msg)
137-
record := new(dns.A)
138-
record.Hdr = dns.RR_Header{Name: dns.Fqdn(hostname), Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 0}
139-
record.A = net.ParseIP("127.0.0.1")
140-
r.Answer = append(r.Answer, record)
141-
return []netip.Addr{}, ResolverAddrs{"MockClient"}, &Error{dns.TypeA, hostname, err, -1, nil}
142-
}
143-
// dual-homed host with an IPv6 and an IPv4 address
144-
if hostname == "ipv4.and.ipv6.localhost" {
145-
return []netip.Addr{
146-
netip.MustParseAddr("::1"),
147-
netip.MustParseAddr("127.0.0.1"),
148-
}, ResolverAddrs{"MockClient"}, nil
149-
}
150-
if hostname == "ipv6.localhost" {
151-
return []netip.Addr{
152-
netip.MustParseAddr("::1"),
153-
}, ResolverAddrs{"MockClient"}, nil
154-
}
155-
return []netip.Addr{netip.MustParseAddr("127.0.0.1")}, ResolverAddrs{"MockClient"}, nil
23+
// LookupAAAA is a fake
24+
func (mock *MockClient) LookupAAAA(_ context.Context, hostname string) (*Result[*dns.AAAA], string, error) {
25+
return nil, "MockClient", errors.New("unexpected LookupAAAA call on test fake")
15626
}
15727

158-
// LookupCAA returns mock records for use in tests.
159-
func (mock *MockClient) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, string, ResolverAddrs, error) {
160-
return nil, "", ResolverAddrs{"MockClient"}, nil
28+
// LookupCAA is a fake
29+
func (mock *MockClient) LookupCAA(_ context.Context, domain string) (*Result[*dns.CAA], string, error) {
30+
return nil, "MockClient", errors.New("unexpected LookupCAA call on test fake")
16131
}

‎bdns/problem_test.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ import (
77
"net/url"
88
"testing"
99

10-
"github.com/letsencrypt/boulder/test"
1110
"github.com/miekg/dns"
11+
12+
"github.com/letsencrypt/boulder/test"
1213
)
1314

1415
func TestError(t *testing.T) {
@@ -17,9 +18,6 @@ func TestError(t *testing.T) {
1718
expected string
1819
}{
1920
{
20-
&Error{dns.TypeA, "hostname", makeTimeoutError(), -1, nil},
21-
"DNS problem: query timed out looking up A for hostname",
22-
}, {
2321
&Error{dns.TypeMX, "hostname", &net.OpError{Err: errors.New("some net error")}, -1, nil},
2422
"DNS problem: networking error looking up MX for hostname",
2523
}, {

‎cmd/boulder-va/main.go‎

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -100,28 +100,16 @@ func main() {
100100
tlsConfig, err := c.VA.TLS.Load(scope)
101101
cmd.FailOnError(err, "tlsConfig config")
102102

103-
var resolver bdns.Client
104-
if !c.VA.DNSAllowLoopbackAddresses {
105-
resolver = bdns.New(
106-
c.VA.DNSTimeout.Duration,
107-
servers,
108-
scope,
109-
clk,
110-
c.VA.DNSTries,
111-
c.VA.UserAgent,
112-
logger,
113-
tlsConfig)
114-
} else {
115-
resolver = bdns.NewTest(
116-
c.VA.DNSTimeout.Duration,
117-
servers,
118-
scope,
119-
clk,
120-
c.VA.DNSTries,
121-
c.VA.UserAgent,
122-
logger,
123-
tlsConfig)
124-
}
103+
resolver := bdns.New(
104+
c.VA.DNSTimeout.Duration,
105+
servers,
106+
scope,
107+
clk,
108+
c.VA.DNSTries,
109+
c.VA.UserAgent,
110+
logger,
111+
tlsConfig)
112+
125113
var remotes []va.RemoteVA
126114
if len(c.VA.RemoteVAs) > 0 {
127115
for _, rva := range c.VA.RemoteVAs {
@@ -155,6 +143,7 @@ func main() {
155143
"",
156144
iana.IsReservedAddr,
157145
c.VA.SlowRemoteTimeout.Duration,
146+
c.VA.DNSAllowLoopbackAddresses,
158147
)
159148
cmd.FailOnError(err, "Unable to create VA server")
160149

‎cmd/remoteva/main.go‎

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -106,28 +106,15 @@ func main() {
106106
tlsConfig.ClientAuth = tls.VerifyClientCertIfGiven
107107
}
108108

109-
var resolver bdns.Client
110-
if !c.RVA.DNSAllowLoopbackAddresses {
111-
resolver = bdns.New(
112-
c.RVA.DNSTimeout.Duration,
113-
servers,
114-
scope,
115-
clk,
116-
c.RVA.DNSTries,
117-
c.RVA.UserAgent,
118-
logger,
119-
tlsConfig)
120-
} else {
121-
resolver = bdns.NewTest(
122-
c.RVA.DNSTimeout.Duration,
123-
servers,
124-
scope,
125-
clk,
126-
c.RVA.DNSTries,
127-
c.RVA.UserAgent,
128-
logger,
129-
tlsConfig)
130-
}
109+
resolver := bdns.New(
110+
c.RVA.DNSTimeout.Duration,
111+
servers,
112+
scope,
113+
clk,
114+
c.RVA.DNSTries,
115+
c.RVA.UserAgent,
116+
logger,
117+
tlsConfig)
131118

132119
vai, err := va.NewValidationAuthorityImpl(
133120
resolver,
@@ -142,6 +129,7 @@ func main() {
142129
c.RVA.RIR,
143130
iana.IsReservedAddr,
144131
0,
132+
c.RVA.DNSAllowLoopbackAddresses,
145133
)
146134
cmd.FailOnError(err, "Unable to create Remote-VA server")
147135

‎va/caa.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ type caaResult struct {
154154
issuewild []*dns.CAA
155155
criticalUnknown bool
156156
dig string
157-
resolvers bdns.ResolverAddrs
157+
resolver string
158158
err error
159159
}
160160

@@ -213,14 +213,18 @@ func (va *ValidationAuthorityImpl) parallelCAALookup(ctx context.Context, name s
213213
// Start the concurrent DNS lookup.
214214
wg.Add(1)
215215
go func(name string, r *caaResult) {
216+
defer wg.Done()
216217
r.name = name
217-
var records []*dns.CAA
218-
records, r.dig, r.resolvers, r.err = va.dnsClient.LookupCAA(ctx, name)
219-
if len(records) > 0 {
218+
var records *bdns.Result[*dns.CAA]
219+
records, r.resolver, r.err = va.dnsClient.LookupCAA(ctx, name)
220+
if r.err != nil {
221+
return
222+
}
223+
r.dig = records.String()
224+
if len(records.Final) > 0 {
220225
r.present = true
221226
}
222-
r.issue, r.issuewild, r.criticalUnknown = filterCAA(records)
223-
wg.Done()
227+
r.issue, r.issuewild, r.criticalUnknown = filterCAA(records.Final)
224228
}(strings.Join(labels[i:], "."), &results[i])
225229
}
226230

0 commit comments

Comments
 (0)