bdns: deprecate DOH feature flag (letsencrypt#8234)

Since the bdns unittests used a local DNS server via TCP, modify that
server to instead speak DoH.

Fixes letsencrypt#8120

Author: jsha

bdns/dns.go

@@ -21,7 +21,6 @@ import (
 	"github.com/miekg/dns"
 	"github.com/prometheus/client_golang/prometheus"
 
-	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/policy"
@@ -77,30 +76,23 @@ func New(
 	tlsConfig *tls.Config,
 ) Client {
 	var client exchanger
-	if features.Get().DOH {
-		// Clone the default transport because it comes with various settings
-		// that we like, which are different from the zero value of an
-		// `http.Transport`.
-		transport := http.DefaultTransport.(*http.Transport).Clone()
-		transport.TLSClientConfig = tlsConfig
-		// The default transport already sets this field, but it isn't
-		// documented that it will always be set. Set it again to be sure,
-		// because Unbound will reject non-HTTP/2 DoH requests.
-		transport.ForceAttemptHTTP2 = true
-		client = &dohExchanger{
-			clk: clk,
-			hc: http.Client{
-				Timeout:   readTimeout,
-				Transport: transport,
-			},
-			userAgent: userAgent,
-		}
-	} else {
-		client = &dns.Client{
-			// Set timeout for underlying net.Conn
-			ReadTimeout: readTimeout,
-			Net:         "udp",
-		}
+
+	// Clone the default transport because it comes with various settings
+	// that we like, which are different from the zero value of an
+	// `http.Transport`.
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+	transport.TLSClientConfig = tlsConfig
+	// The default transport already sets this field, but it isn't
+	// documented that it will always be set. Set it again to be sure,
+	// because Unbound will reject non-HTTP/2 DoH requests.
+	transport.ForceAttemptHTTP2 = true
+	client = &dohExchanger{
+		clk: clk,
+		hc: http.Client{
+			Timeout:   readTimeout,
+			Transport: transport,
+		},
+		userAgent: userAgent,
 	}
 
 	queryTime := prometheus.NewHistogramVec(
@@ -281,17 +273,10 @@ func (dnsClient *impl) exchangeOne(ctx context.Context, hostname string, qtype u
 		case r := <-ch:
 			if r.err != nil {
 				var isRetryable bool
-				if features.Get().DOH {
-					// According to the http package documentation, retryable
-					// errors emitted by the http package are of type *url.Error.
-					var urlErr *url.Error
-					isRetryable = errors.As(r.err, &urlErr) && urlErr.Temporary()
-				} else {
-					// According to the net package documentation, retryable
-					// errors emitted by the net package are of type *net.OpError.
-					var opErr *net.OpError
-					isRetryable = errors.As(r.err, &opErr) && opErr.Temporary()
-				}
+				// According to the http package documentation, retryable
+				// errors emitted by the http package are of type *url.Error.
+				var urlErr *url.Error
+				isRetryable = errors.As(r.err, &urlErr) && urlErr.Temporary()
 				hasRetriesLeft := tries < dnsClient.maxTries
 				if isRetryable && hasRetriesLeft {
 					tries++

bdns/dns_test.go

@@ -2,10 +2,14 @@ package bdns
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
 	"errors"
 	"fmt"
+	"io"
 	"log"
 	"net"
+	"net/http"
 	"net/netip"
 	"net/url"
 	"os"
@@ -20,15 +24,37 @@ import (
 	"github.com/miekg/dns"
 	"github.com/prometheus/client_golang/prometheus"
 
-	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/test"
 )
 
 const dnsLoopbackAddr = "127.0.0.1:4053"
 
-func mockDNSQuery(w dns.ResponseWriter, r *dns.Msg) {
+func mockDNSQuery(w http.ResponseWriter, httpReq *http.Request) {
+	if httpReq.Header.Get("Content-Type") != "application/dns-message" {
+		w.WriteHeader(http.StatusBadRequest)
+		fmt.Fprintf(w, "client didn't send Content-Type: application/dns-message")
+	}
+	if httpReq.Header.Get("Accept") != "application/dns-message" {
+		w.WriteHeader(http.StatusBadRequest)
+		fmt.Fprintf(w, "client didn't accept Content-Type: application/dns-message")
+	}
+
+	requestBody, err := io.ReadAll(httpReq.Body)
+	if err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		fmt.Fprintf(w, "reading body: %s", err)
+	}
+	httpReq.Body.Close()
+
+	r := new(dns.Msg)
+	err = r.Unpack(requestBody)
+	if err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		fmt.Fprintf(w, "unpacking request: %s", err)
+	}
+
 	m := new(dns.Msg)
 	m.SetReply(r)
 	m.Compress = false
@@ -174,45 +200,37 @@ func mockDNSQuery(w dns.ResponseWriter, r *dns.Msg) {
 		}
 	}
 
-	err := w.WriteMsg(m)
+	body, err := m.Pack()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "packing reply: %s\n", err)
+	}
+	w.Header().Set("Content-Type", "application/dns-message")
+	_, err = w.Write(body)
 	if err != nil {
 		panic(err) // running tests, so panic is OK
 	}
 }
 
 func serveLoopResolver(stopChan chan bool) {
-	dns.HandleFunc(".", mockDNSQuery)
-	tcpServer := &dns.Server{
-		Addr:         dnsLoopbackAddr,
-		Net:          "tcp",
-		ReadTimeout:  time.Second,
-		WriteTimeout: time.Second,
-	}
-	udpServer := &dns.Server{
+	m := http.NewServeMux()
+	m.HandleFunc("/dns-query", mockDNSQuery)
+	httpServer := &http.Server{
 		Addr:         dnsLoopbackAddr,
-		Net:          "udp",
+		Handler:      m,
 		ReadTimeout:  time.Second,
 		WriteTimeout: time.Second,
 	}
 	go func() {
-		err := tcpServer.ListenAndServe()
-		if err != nil {
-			fmt.Println(err)
-		}
-	}()
-	go func() {
-		err := udpServer.ListenAndServe()
+		cert := "../test/certs/ipki/localhost/cert.pem"
+		key := "../test/certs/ipki/localhost/key.pem"
+		err := httpServer.ListenAndServeTLS(cert, key)
 		if err != nil {
 			fmt.Println(err)
 		}
 	}()
 	go func() {
 		<-stopChan
-		err := tcpServer.Shutdown()
-		if err != nil {
-			log.Fatal(err)
-		}
-		err = udpServer.Shutdown()
+		err := httpServer.Shutdown(context.Background())
 		if err != nil {
 			log.Fatal(err)
 		}
@@ -240,7 +258,21 @@ func pollServer() {
 	}
 }
 
+// tlsConfig is used for the TLS config of client instances that talk to the
+// DoH server set up in TestMain.
+var tlsConfig *tls.Config
+
 func TestMain(m *testing.M) {
+	root, err := os.ReadFile("../test/certs/ipki/minica.pem")
+	if err != nil {
+		log.Fatal(err)
+	}
+	pool := x509.NewCertPool()
+	pool.AppendCertsFromPEM(root)
+	tlsConfig = &tls.Config{
+		RootCAs: pool,
+	}
+
 	stop := make(chan bool, 1)
 	serveLoopResolver(stop)
 	pollServer()
@@ -253,7 +285,7 @@ func TestDNSNoServers(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Hour, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Hour, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	_, resolvers, err := obj.LookupHost(context.Background(), "letsencrypt.org")
 	test.AssertEquals(t, len(resolvers), 0)
@@ -270,7 +302,7 @@ func TestDNSOneServer(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	_, resolvers, err := obj.LookupHost(context.Background(), "cps.letsencrypt.org")
 	test.AssertEquals(t, len(resolvers), 2)
@@ -283,7 +315,7 @@ func TestDNSDuplicateServers(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr, dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	_, resolvers, err := obj.LookupHost(context.Background(), "cps.letsencrypt.org")
 	test.AssertEquals(t, len(resolvers), 2)
@@ -296,7 +328,7 @@ func TestDNSServFail(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 	bad := "servfail.com"
 
 	_, _, err = obj.LookupTXT(context.Background(), bad)
@@ -314,7 +346,7 @@ func TestDNSLookupTXT(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	a, _, err := obj.LookupTXT(context.Background(), "letsencrypt.org")
 	t.Logf("A: %v", a)
@@ -332,7 +364,7 @@ func TestDNSLookupHost(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	ip, resolvers, err := obj.LookupHost(context.Background(), "servfail.com")
 	t.Logf("servfail.com - IP: %s, Err: %s", ip, err)
@@ -418,7 +450,7 @@ func TestDNSNXDOMAIN(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 
 	hostname := "nxdomain.letsencrypt.org"
 	_, _, err = obj.LookupHost(context.Background(), hostname)
@@ -434,7 +466,7 @@ func TestDNSLookupCAA(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), nil)
+	obj := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig)
 	removeIDExp := regexp.MustCompile(" id: [[:digit:]]+")
 
 	caas, resp, resolvers, err := obj.LookupCAA(context.Background(), "bracewel.net")
@@ -513,10 +545,9 @@ func (te *testExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration
 }
 
 func TestRetry(t *testing.T) {
-	isTempErr := &net.OpError{Op: "read", Err: tempError(true)}
-	nonTempErr := &net.OpError{Op: "read", Err: tempError(false)}
+	isTempErr := &url.Error{Op: "read", Err: tempError(true)}
+	nonTempErr := &url.Error{Op: "read", Err: tempError(false)}
 	servFailError := errors.New("DNS problem: server failure at resolver looking up TXT for example.com")
-	netError := errors.New("DNS problem: networking error looking up TXT for example.com")
 	type testCase struct {
 		name              string
 		maxTries          int
@@ -567,7 +598,7 @@ func TestRetry(t *testing.T) {
 					isTempErr,
 				},
 			},
-			expected:          netError,
+			expected:          servFailError,
 			expectedCount:     3,
 			metricsAllRetries: 1,
 		},
@@ -620,7 +651,7 @@ func TestRetry(t *testing.T) {
 					isTempErr,
 				},
 			},
-			expected:          netError,
+			expected:          servFailError,
 			expectedCount:     3,
 			metricsAllRetries: 1,
 		},
@@ -634,7 +665,7 @@ func TestRetry(t *testing.T) {
 					nonTempErr,
 				},
 			},
-			expected:      netError,
+			expected:      servFailError,
 			expectedCount: 2,
 		},
 	}
@@ -644,7 +675,7 @@ func TestRetry(t *testing.T) {
 			staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 			test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-			testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, "", blog.UseMock(), nil)
+			testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, "", blog.UseMock(), tlsConfig)
 			dr := testClient.(*impl)
 			dr.dnsClient = tc.te
 			_, _, err = dr.LookupTXT(context.Background(), "example.com")
@@ -675,7 +706,7 @@ func TestRetry(t *testing.T) {
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), nil)
+	testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), tlsConfig)
 	dr := testClient.(*impl)
 	dr.dnsClient = &testExchanger{errs: []error{isTempErr, isTempErr, nil}}
 	ctx, cancel := context.WithCancel(context.Background())
@@ -754,7 +785,7 @@ func (e *rotateFailureExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.
 
 	// If its a broken server, return a retryable error
 	if e.brokenAddresses[a] {
-		isTempErr := &net.OpError{Op: "read", Err: tempError(true)}
+		isTempErr := &url.Error{Op: "read", Err: tempError(true)}
 		return nil, 2 * time.Millisecond, isTempErr
 	}
 
@@ -776,10 +807,9 @@ func TestRotateServerOnErr(t *testing.T) {
 	// working server
 	staticProvider, err := NewStaticProvider(dnsServers)
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
-	fmt.Println(staticProvider.servers)
 
 	maxTries := 5
-	client := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), maxTries, "", blog.UseMock(), nil)
+	client := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), maxTries, "", blog.UseMock(), tlsConfig)
 
 	// Configure a mock exchanger that will always return a retryable error for
 	// servers A and B. This will force server "[2606:4700:4700::1111]:53" to do
@@ -843,13 +873,10 @@ func (dohE *dohAlwaysRetryExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, t
 }
 
 func TestDOHMetric(t *testing.T) {
-	features.Set(features.Config{DOH: true})
-	defer features.Reset()
-
 	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
 	test.AssertNotError(t, err, "Got error creating StaticProvider")
 
-	testClient := New(time.Second*11, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 0, "", blog.UseMock(), nil)
+	testClient := New(time.Second*11, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 0, "", blog.UseMock(), tlsConfig)
 	resolver := testClient.(*impl)
 	resolver.dnsClient = &dohAlwaysRetryExchanger{err: &url.Error{Op: "read", Err: tempError(true)}}
 

cmd/boulder-va/main.go

@@ -82,16 +82,12 @@ func main() {
 	clk := cmd.Clock()
 
 	var servers bdns.ServerProvider
-	proto := "udp"
-	if features.Get().DOH {
-		proto = "tcp"
-	}
 
 	if len(c.VA.DNSStaticResolvers) != 0 {
 		servers, err = bdns.NewStaticProvider(c.VA.DNSStaticResolvers)
 		cmd.FailOnError(err, "Couldn't start static DNS server resolver")
 	} else {
-		servers, err = bdns.StartDynamicProvider(c.VA.DNSProvider, 60*time.Second, proto)
+		servers, err = bdns.StartDynamicProvider(c.VA.DNSProvider, 60*time.Second, "tcp")
 		cmd.FailOnError(err, "Couldn't start dynamic DNS server resolver")
 	}
 	defer servers.Stop()