Merge pull request #5003 from FlorianLiebhart/DNS-over-https-check

Implement the DNS-over-HTTPS check

cmd/controller/app/options/options.go

@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"net/url"
 	"strings"
 	"time"
 
@@ -97,6 +98,11 @@ type ControllerOptions struct {
 	DefaultAutoCertificateAnnotations []string
 
 	// Allows specifying a list of custom nameservers to perform DNS checks on.
+	// Each nameserver can be either the IP address and port of a standard
+	// recursive DNS server, or the endpoint to an RFC 8484 DNS over HTTPS
+	// endpoint. For example, the following values are valid:
+	//  - "8.8.8.8:53" (Standard DNS)
+	//  - "https://1.1.1.1/dns-query" (DNS over HTTPS)
 	DNS01RecursiveNameservers []string
 	// Allows controlling if recursive nameservers are only used for all checks.
 	// Normally authoritative nameservers are used for checking propagation.
@@ -362,10 +368,13 @@ func (s *ControllerOptions) AddFlags(fs *pflag.FlagSet) {
 		"Kind of the Issuer to use when the tls is requested but issuer kind is not specified on the ingress resource.")
 	fs.StringVar(&s.DefaultIssuerGroup, "default-issuer-group", defaultTLSACMEIssuerGroup, ""+
 		"Group of the Issuer to use when the tls is requested but issuer group is not specified on the ingress resource.")
+
 	fs.StringSliceVar(&s.DNS01RecursiveNameservers, "dns01-recursive-nameservers",
-		[]string{}, "A list of comma separated dns server endpoints used for "+
-			"DNS01 check requests. This should be a list containing host and "+
-			"port, for example 8.8.8.8:53,8.8.4.4:53")
+		[]string{}, "A list of comma separated dns server endpoints used for DNS01 and DNS-over-HTTPS (DoH) check requests. "+
+			"This should be a list containing entries of the following formats: `<ip address>:<port>` or `https://<DoH RFC 8484 server address>`. "+
+			"For example: `8.8.8.8:53,8.8.4.4:53` or `https://1.1.1.1/dns-query,https://8.8.8.8/dns-query`. "+
+			"To make sure ALL DNS requests happen through DoH, `dns01-recursive-nameservers-only` should also be set to true.")
+
 	fs.BoolVar(&s.DNS01RecursiveNameserversOnly, "dns01-recursive-nameservers-only",
 		defaultDNS01RecursiveNameserversOnly,
 		"When true, cert-manager will only ever query the configured DNS resolvers "+
@@ -434,14 +443,32 @@ func (o *ControllerOptions) Validate() error {
 		return fmt.Errorf("invalid value for kube-api-burst: %v must be higher or equal to kube-api-qps: %v", o.KubernetesAPIQPS, o.KubernetesAPIQPS)
 	}
 
-	for _, server := range append(o.DNS01RecursiveNameservers, o.ACMEHTTP01SolverNameservers...) {
+	for _, server := range o.ACMEHTTP01SolverNameservers {
 		// ensure all servers have a port number
 		_, _, err := net.SplitHostPort(server)
 		if err != nil {
 			return fmt.Errorf("invalid DNS server (%v): %v", err, server)
 		}
 	}
 
+	for _, server := range o.DNS01RecursiveNameservers {
+		// ensure all servers follow one of the following formats:
+		// - <ip address>:<port>
+		// - https://<DoH RFC 8484 server address>
+
+		if strings.HasPrefix(server, "https://") {
+			_, err := url.ParseRequestURI(server)
+			if err != nil {
+				return fmt.Errorf("invalid DNS server (%v): %v", err, server)
+			}
+		} else {
+			_, _, err := net.SplitHostPort(server)
+			if err != nil {
+				return fmt.Errorf("invalid DNS server (%v): %v", err, server)
+			}
+		}
+	}
+
 	errs := []error{}
 	allControllersSet := sets.NewString(allControllers...)
 	for _, controller := range o.controllers {

cmd/controller/app/options/options_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package options
 
 import (
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/component-base/logs"
 )
 
 func TestEnabledControllers(t *testing.T) {
@@ -63,3 +65,48 @@ func TestEnabledControllers(t *testing.T) {
 		})
 	}
 }
+
+func TestValidate(t *testing.T) {
+	tests := map[string]struct {
+		DNS01RecursiveServers []string
+		expError              string
+	}{
+		"if valid dns servers with ip address and port, return no errors": {
+			DNS01RecursiveServers: []string{"192.168.0.1:53", "10.0.0.1:5353"},
+			expError:              "",
+		},
+		"if valid DNS servers with DoH server addresses including https prefix, return no errors": {
+			DNS01RecursiveServers: []string{"https://dns.example.com", "https://doh.server"},
+			expError:              "",
+		},
+		"if invalid DNS server format due to missing https prefix, return 'invalid DNS server' error": {
+			DNS01RecursiveServers: []string{"dns.example.com"},
+			expError:              "invalid DNS server",
+		},
+		"if invalid DNS server format due to invalid IP address length and no port, return 'invalid DNS server' error": {
+			DNS01RecursiveServers: []string{"192.168.0.1.53"},
+			expError:              "invalid DNS server",
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			o := ControllerOptions{
+				DNS01RecursiveNameservers: test.DNS01RecursiveServers,
+				DefaultIssuerKind:         defaultTLSACMEIssuerKind,
+				KubernetesAPIBurst:        defaultKubernetesAPIBurst,
+				KubernetesAPIQPS:          defaultKubernetesAPIQPS,
+				Logging:                   logs.NewOptions(),
+			}
+
+			err := o.Validate()
+			if test.expError != "" {
+				if err == nil || !strings.Contains(err.Error(), test.expError) {
+					t.Errorf("expected error containing '%s', but got: %v", test.expError, err)
+				}
+			} else if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+		})
+	}
+}

pkg/issuer/acme/dns/util/wait.go

@@ -9,8 +9,12 @@ this directory.
 package util
 
 import (
+	"bytes"
+	"context"
 	"fmt"
+	"io/ioutil"
 	"net"
+	"net/http"
 	"strings"
 	"sync"
 	"time"
@@ -153,13 +157,20 @@ func checkAuthoritativeNss(fqdn, value string, nameservers []string) (bool, erro
 			return false, nil
 		}
 	}
-
+	logf.V(logf.DebugLevel).Infof("Selfchecking using the DNS Lookup method was successful")
 	return true, nil
 }
 
 // DNSQuery will query a nameserver, iterating through the supplied servers as it retries
 // The nameserver should include a port, to facilitate testing where we talk to a mock dns server.
 func DNSQuery(fqdn string, rtype uint16, nameservers []string, recursive bool) (in *dns.Msg, err error) {
+	switch rtype {
+	case dns.TypeCAA, dns.TypeCNAME, dns.TypeNS, dns.TypeSOA, dns.TypeTXT:
+	default:
+		// We explicitly specified here what types are supported, so we can more confidently create tests for this function.
+		return nil, fmt.Errorf("unsupported DNS record type %d", rtype)
+	}
+
 	m := new(dns.Msg)
 	m.SetQuestion(fqdn, rtype)
 	m.SetEdns0(4096, false)
@@ -168,18 +179,30 @@ func DNSQuery(fqdn string, rtype uint16, nameservers []string, recursive bool) (
 		m.RecursionDesired = false
 	}
 
+	udp := &dns.Client{Net: "udp", Timeout: DNSTimeout}
+	tcp := &dns.Client{Net: "tcp", Timeout: DNSTimeout}
+	httpClient := *http.DefaultClient
+	httpClient.Timeout = DNSTimeout
+	http := httpDNSClient{
+		HTTPClient: &httpClient,
+	}
+
 	// Will retry the request based on the number of servers (n+1)
-	for i := 1; i <= len(nameservers)+1; i++ {
-		ns := nameservers[i%len(nameservers)]
-		udp := &dns.Client{Net: "udp", Timeout: DNSTimeout}
-		in, _, err = udp.Exchange(m, ns)
-
-		if (in != nil && in.Truncated) ||
-			(err != nil && strings.HasPrefix(err.Error(), "read udp") && strings.HasSuffix(err.Error(), "i/o timeout")) {
-			logf.V(logf.DebugLevel).Infof("UDP dns lookup failed, retrying with TCP: %v", err)
-			tcp := &dns.Client{Net: "tcp", Timeout: DNSTimeout}
-			// If the TCP request succeeds, the err will reset to nil
-			in, _, err = tcp.Exchange(m, ns)
+	for _, ns := range nameservers {
+		// If the TCP request succeeds, the err will reset to nil
+		if strings.HasPrefix(ns, "https://") {
+			in, _, err = http.Exchange(context.TODO(), m, ns)
+
+		} else {
+			in, _, err = udp.Exchange(m, ns)
+
+			// Try TCP if UDP fails
+			if (in != nil && in.Truncated) ||
+				(err != nil && strings.HasPrefix(err.Error(), "read udp") && strings.HasSuffix(err.Error(), "i/o timeout")) {
+				logf.V(logf.DebugLevel).Infof("UDP dns lookup failed, retrying with TCP: %v", err)
+				// If the TCP request succeeds, the err will reset to nil
+				in, _, err = tcp.Exchange(m, ns)
+			}
 		}
 
 		if err == nil {
@@ -189,6 +212,64 @@ func DNSQuery(fqdn string, rtype uint16, nameservers []string, recursive bool) (
 	return
 }
 
+type httpDNSClient struct {
+	HTTPClient *http.Client
+}
+
+const dohMimeType = "application/dns-message"
+
+func (c *httpDNSClient) Exchange(ctx context.Context, m *dns.Msg, a string) (r *dns.Msg, rtt time.Duration, err error) {
+	p, err := m.Pack()
+	if err != nil {
+		return nil, 0, err
+	}
+
+	req, err := http.NewRequest(http.MethodPost, a, bytes.NewReader(p))
+	if err != nil {
+		return nil, 0, err
+	}
+
+	req.Header.Set("Content-Type", dohMimeType)
+	req.Header.Set("Accept", dohMimeType)
+
+	hc := http.DefaultClient
+	if c.HTTPClient != nil {
+		hc = c.HTTPClient
+	}
+
+	req = req.WithContext(ctx)
+
+	t := time.Now()
+
+	resp, err := hc.Do(req)
+	if err != nil {
+		return nil, 0, err
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, 0, fmt.Errorf("dns: server returned HTTP %d error: %q", resp.StatusCode, resp.Status)
+	}
+
+	if ct := resp.Header.Get("Content-Type"); ct != dohMimeType {
+		return nil, 0, fmt.Errorf("dns: unexpected Content-Type %q; expected %q", ct, dohMimeType)
+	}
+
+	p, err = ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, 0, err
+	}
+
+	rtt = time.Since(t)
+
+	r = new(dns.Msg)
+	if err := r.Unpack(p); err != nil {
+		return r, 0, err
+	}
+
+	return r, rtt, nil
+}
+
 func ValidateCAA(domain string, issuerID []string, iswildcard bool, nameservers []string) error {
 	// see https://tools.ietf.org/html/rfc6844#section-4
 	// for more information about how CAA lookup is performed

pkg/issuer/acme/dns/util/wait_test.go

@@ -165,6 +165,20 @@ func TestMatchCAA(t *testing.T) {
 	}
 }
 
+func TestPreCheckDNSOverHTTPSNoAuthoritative(t *testing.T) {
+	ok, err := PreCheckDNS("google.com.", "v=spf1 include:_spf.google.com ~all", []string{"https://1.1.1.1/dns-query"}, false)
+	if err != nil || !ok {
+		t.Errorf("preCheckDNS failed for acme-staging.api.letsencrypt.org: %s", err.Error())
+	}
+}
+
+func TestPreCheckDNSOverHTTPS(t *testing.T) {
+	ok, err := PreCheckDNS("google.com.", "v=spf1 include:_spf.google.com ~all", []string{"https://8.8.8.8/dns-query"}, true)
+	if err != nil || !ok {
+		t.Errorf("preCheckDNS failed for acme-staging.api.letsencrypt.org: %s", err.Error())
+	}
+}
+
 func TestPreCheckDNS(t *testing.T) {
 	// TODO: find a better TXT record to use in tests
 	ok, err := PreCheckDNS("google.com.", "v=spf1 include:_spf.google.com ~all", []string{"8.8.8.8:53"}, true)
@@ -259,30 +273,34 @@ func TestResolveConfServers(t *testing.T) {
 
 // TODO: find a website which uses issuewild?
 func TestValidateCAA(t *testing.T) {
-	// google installs a CAA record at google.com
-	// ask for the www.google.com record to test that
-	// we recurse up the labels
-	err := ValidateCAA("www.google.com", []string{"letsencrypt", "pki.goog"}, false, RecursiveNameservers)
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	// now ask, expecting a CA that won't match
-	err = ValidateCAA("www.google.com", []string{"daniel.homebrew.ca"}, false, RecursiveNameservers)
-	if err == nil {
-		t.Fatalf("expected err, got success")
-	}
-	// if the CAA record allows non-wildcards then it has an `issue` tag,
-	// and it is known that it has no issuewild tags, then wildcard certificates
-	// will also be allowed
-	err = ValidateCAA("www.google.com", []string{"pki.goog"}, true, RecursiveNameservers)
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	// ask for a domain you know does not have CAA records.
-	// it should succeed
-	err = ValidateCAA("www.example.org", []string{"daniel.homebrew.ca"}, false, RecursiveNameservers)
-	if err != nil {
-		t.Fatalf("expected err, got %s", err)
+
+	for _, nameservers := range [][]string{RecursiveNameservers, []string{"https://1.1.1.1/dns-query"}, []string{"https://8.8.8.8/dns-query"}} {
+
+		// google installs a CAA record at google.com
+		// ask for the www.google.com record to test that
+		// we recurse up the labels
+		err := ValidateCAA("www.google.com", []string{"letsencrypt", "pki.goog"}, false, nameservers)
+		if err != nil {
+			t.Fatalf("unexpected error: %s", err)
+		}
+		// now ask, expecting a CA that won't match
+		err = ValidateCAA("www.google.com", []string{"daniel.homebrew.ca"}, false, nameservers)
+		if err == nil {
+			t.Fatalf("expected err, got success")
+		}
+		// if the CAA record allows non-wildcards then it has an `issue` tag,
+		// and it is known that it has no issuewild tags, then wildcard certificates
+		// will also be allowed
+		err = ValidateCAA("www.google.com", []string{"pki.goog"}, true, nameservers)
+		if err != nil {
+			t.Fatalf("unexpected error: %s", err)
+		}
+		// ask for a domain you know does not have CAA records.
+		// it should succeed
+		err = ValidateCAA("www.example.org", []string{"daniel.homebrew.ca"}, false, nameservers)
+		if err != nil {
+			t.Fatalf("expected err, got %s", err)
+		}
 	}
 }