core: Reject conflicting TLS policies on shared listeners. (#8565)

* core: Reject conflicting TLS policies on shared listeners.

This PR tries to fix the issue when multiple zones share a DoT listener, CoreDNS can apply one sibling block’s tls.Config to all zones, allowing weaker TLS or client-auth settings to override a stricter zone.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Fix ACME

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

---------

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

core/dnsserver/config.go

@@ -70,6 +70,10 @@ type Config struct {
 	// TLSConfig when listening for encrypted connections (gRPC, DNS-over-TLS).
 	TLSConfig *tls.Config
 
+	// tlsConfigIdentity identifies dynamic TLS configurations that are known to
+	// represent the same listener-wide policy even after tls.Config.Clone.
+	tlsConfigIdentity *TLSConfigIdentity
+
 	// MaxQUICStreams defines the maximum number of concurrent QUIC streams for a QUIC server.
 	// This is nil if not specified, allowing for a default to be used.
 	MaxQUICStreams *int
@@ -165,6 +169,22 @@ type Config struct {
 // FilterFunc is a function that filters requests from the Config
 type FilterFunc func(context.Context, *request.Request) bool
 
+// TLSConfigIdentity is an opaque identity for equivalent dynamic TLS policies.
+// Plugins should share one identity only when they can prove that independently
+// constructed TLS configs are interchangeable on the same listener.
+type TLSConfigIdentity struct {
+	_ byte
+}
+
+// NewTLSConfigIdentity returns a new opaque TLS policy identity.
+func NewTLSConfigIdentity() *TLSConfigIdentity { return &TLSConfigIdentity{} }
+
+// SetTLSConfigIdentity associates an opaque listener-wide policy identity with
+// this config.
+func (c *Config) SetTLSConfigIdentity(identity *TLSConfigIdentity) {
+	c.tlsConfigIdentity = identity
+}
+
 // keyForConfig builds a key for identifying the configs during setup time
 func keyForConfig(blocIndex int, blocKeyIndex int) string {
 	return fmt.Sprintf("%d:%d", blocIndex, blocKeyIndex)

core/dnsserver/register.go

@@ -298,8 +298,10 @@ func propagateConfigParams(configs []*Config) {
 		c.Stacktrace = c.firstConfigInBlock.Stacktrace
 		c.NumSockets = c.firstConfigInBlock.NumSockets
 
-		// Fork TLSConfig for each encrypted connection
+		// Fork TLSConfig for each encrypted connection while preserving the
+		// listener-wide policy identity used to compare dynamic callbacks.
 		c.TLSConfig = c.firstConfigInBlock.TLSConfig.Clone()
+		c.tlsConfigIdentity = c.firstConfigInBlock.tlsConfigIdentity
 		c.ReadTimeout = c.firstConfigInBlock.ReadTimeout
 		c.WriteTimeout = c.firstConfigInBlock.WriteTimeout
 		c.IdleTimeout = c.firstConfigInBlock.IdleTimeout

core/dnsserver/server_grpc.go

@@ -50,18 +50,13 @@ type ServergRPC struct {
 
 // NewServergRPC returns a new CoreDNS GRPC server and compiles all plugin in to it.
 func NewServergRPC(addr string, group []*Config) (*ServergRPC, error) {
-	s, err := NewServer(addr, group)
+	tlsConfig, err := sharedTLSConfig(addr, group)
 	if err != nil {
 		return nil, err
 	}
-	// The *tls* plugin must make sure that multiple conflicting
-	// TLS configuration returns an error: it can only be specified once.
-	var tlsConfig *tls.Config
-	for _, z := range s.zones {
-		for _, conf := range z {
-			// Should we error if some configs *don't* have TLS?
-			tlsConfig = conf.TLSConfig
-		}
+	s, err := NewServer(addr, group)
+	if err != nil {
+		return nil, err
 	}
 	// http/2 is required when using gRPC. We need to specify it in next protos
 	// or the upgrade won't happen.

core/dnsserver/server_https.go

@@ -58,18 +58,13 @@ type HTTPRequestKey struct{}
 
 // NewServerHTTPS returns a new CoreDNS HTTPS server and compiles all plugins in to it.
 func NewServerHTTPS(addr string, group []*Config) (*ServerHTTPS, error) {
-	s, err := NewServer(addr, group)
+	tlsConfig, err := sharedTLSConfig(addr, group)
 	if err != nil {
 		return nil, err
 	}
-	// The *tls* plugin must make sure that multiple conflicting
-	// TLS configuration returns an error: it can only be specified once.
-	var tlsConfig *tls.Config
-	for _, z := range s.zones {
-		for _, conf := range z {
-			// Should we error if some configs *don't* have TLS?
-			tlsConfig = conf.TLSConfig
-		}
+	s, err := NewServer(addr, group)
+	if err != nil {
+		return nil, err
 	}
 
 	// http/2 is recommended when using DoH. We need to specify it in next protos

core/dnsserver/server_https3.go

@@ -77,18 +77,15 @@ func (l *limitQUICListener) Accept(ctx context.Context) (*quic.Conn, error) {
 
 // NewServerHTTPS3 builds the HTTP/3 (DoH3) server.
 func NewServerHTTPS3(addr string, group []*Config) (*ServerHTTPS3, error) {
+	tlsConfig, err := sharedTLSConfig(addr, group)
+	if err != nil {
+		return nil, err
+	}
 	s, err := NewServer(addr, group)
 	if err != nil {
 		return nil, err
 	}
 
-	// Extract TLS config (CoreDNS guarantees it is consistent)
-	var tlsConfig *tls.Config
-	for _, z := range s.zones {
-		for _, conf := range z {
-			tlsConfig = conf.TLSConfig
-		}
-	}
 	if tlsConfig == nil {
 		return nil, fmt.Errorf("DoH3 requires TLS, no TLS config found")
 	}

core/dnsserver/server_quic.go

@@ -64,18 +64,13 @@ type ServerQUIC struct {
 
 // NewServerQUIC returns a new CoreDNS QUIC server and compiles all plugin in to it.
 func NewServerQUIC(addr string, group []*Config) (*ServerQUIC, error) {
-	s, err := NewServer(addr, group)
+	tlsConfig, err := sharedTLSConfig(addr, group)
 	if err != nil {
 		return nil, err
 	}
-	// The *tls* plugin must make sure that multiple conflicting
-	// TLS configuration returns an error: it can only be specified once.
-	var tlsConfig *tls.Config
-	for _, z := range s.zones {
-		for _, conf := range z {
-			// Should we error if some configs *don't* have TLS?
-			tlsConfig = conf.TLSConfig
-		}
+	s, err := NewServer(addr, group)
+	if err != nil {
+		return nil, err
 	}
 
 	if tlsConfig != nil {

core/dnsserver/server_tls.go

@@ -22,18 +22,13 @@ type ServerTLS struct {
 
 // NewServerTLS returns a new CoreDNS TLS server and compiles all plugin in to it.
 func NewServerTLS(addr string, group []*Config) (*ServerTLS, error) {
-	s, err := NewServer(addr, group)
+	tlsConfig, err := sharedTLSConfig(addr, group)
 	if err != nil {
 		return nil, err
 	}
-	// The *tls* plugin must make sure that multiple conflicting
-	// TLS configuration returns an error: it can only be specified once.
-	var tlsConfig *tls.Config
-	for _, z := range s.zones {
-		for _, conf := range z {
-			// Should we error if some configs *don't* have TLS?
-			tlsConfig = conf.TLSConfig
-		}
+	s, err := NewServer(addr, group)
+	if err != nil {
+		return nil, err
 	}
 
 	return &ServerTLS{Server: s, tlsConfig: tlsConfig}, nil

core/dnsserver/server_tls_test.go

@@ -4,6 +4,7 @@ import (
 	"crypto/tls"
 	"errors"
 	"net"
+	"strings"
 	"testing"
 	"time"
 
@@ -29,6 +30,101 @@ func (l *stubListener) Addr() net.Addr {
 	return &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0}
 }
 
+func TestNewServerTLSRejectsConflictingSharedTLSConfig(t *testing.T) {
+	certificateA := tls.Certificate{Certificate: [][]byte{[]byte("certificate-a")}}
+	certificateB := tls.Certificate{Certificate: [][]byte{[]byte("certificate-b")}}
+
+	config := func(zone string, tlsConfig *tls.Config) *Config {
+		c := testConfig("tls", testPlugin{})
+		c.Zone = zone
+		c.TLSConfig = tlsConfig
+		return c
+	}
+
+	tests := []struct {
+		name    string
+		first   *tls.Config
+		second  *tls.Config
+		wantErr string
+	}{
+		{
+			name: "client authentication",
+			first: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+				ClientAuth:   tls.RequireAndVerifyClientCert,
+			},
+			second: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+				ClientAuth:   tls.NoClientCert,
+			},
+			wantErr: "client authentication policies differ",
+		},
+		{
+			name: "TLS and plaintext",
+			first: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+			},
+			second:  nil,
+			wantErr: "TLS is configured for only one server block",
+		},
+		{
+			name: "server certificate",
+			first: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+			},
+			second: &tls.Config{
+				Certificates: []tls.Certificate{certificateB},
+			},
+			wantErr: "server certificates differ",
+		},
+		{
+			name: "dynamic TLS callbacks",
+			first: &tls.Config{
+				GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, nil },
+			},
+			second: &tls.Config{
+				GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, nil },
+			},
+			wantErr: "dynamic TLS callbacks differ",
+		},
+		{
+			name: "matching policies",
+			first: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+				ClientAuth:   tls.RequireAndVerifyClientCert,
+				MinVersion:   tls.VersionTLS12,
+			},
+			second: &tls.Config{
+				Certificates: []tls.Certificate{certificateA},
+				ClientAuth:   tls.RequireAndVerifyClientCert,
+				MinVersion:   tls.VersionTLS12,
+			},
+			wantErr: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := NewServerTLS("tls://127.0.0.1:0", []*Config{
+				config("a.example.test.", tt.first),
+				config("b.example.test.", tt.second),
+			})
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("NewServerTLS() failed: %v", err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("NewServerTLS() succeeded, want error containing %q", tt.wantErr)
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Fatalf("NewServerTLS() error = %q, want substring %q", err, tt.wantErr)
+			}
+		})
+	}
+}
+
 func TestServerTLSSetsTsigSecret(t *testing.T) {
 	server, err := NewServerTLS("tls://127.0.0.1:0", []*Config{testConfig("tls", testPlugin{})})
 	if err != nil {

core/dnsserver/tlsconfig.go

@@ -0,0 +1,132 @@
+package dnsserver
+
+import (
+	"bytes"
+	"crypto/tls"
+	"crypto/x509"
+	"fmt"
+	"slices"
+)
+
+// sharedTLSConfig returns the TLS configuration for a listener shared by group.
+// TLS is negotiated before CoreDNS knows which zone will handle the DNS request,
+// so listener-wide identity and client-authentication settings must agree.
+func sharedTLSConfig(addr string, group []*Config) (*tls.Config, error) {
+	if len(group) == 0 {
+		return nil, nil
+	}
+
+	first := group[0]
+	if first == nil {
+		return nil, fmt.Errorf("nil config for shared listener %s", addr)
+	}
+
+	for _, conf := range group[1:] {
+		if conf == nil {
+			return nil, fmt.Errorf("nil config for shared listener %s", addr)
+		}
+		if err := compatibleTLSConfig(first, conf); err != nil {
+			return nil, fmt.Errorf("conflicting TLS configuration for shared listener %s between zones %q and %q: %w", addr, first.Zone, conf.Zone, err)
+		}
+	}
+
+	if first.TLSConfig == nil {
+		return nil, nil
+	}
+	return first.TLSConfig.Clone(), nil
+}
+
+func sameServerBlock(a, b *Config) bool {
+	aFirst := a.firstConfigInBlock
+	if aFirst == nil {
+		aFirst = a
+	}
+	bFirst := b.firstConfigInBlock
+	if bFirst == nil {
+		bFirst = b
+	}
+	return aFirst == bFirst
+}
+
+func compatibleTLSConfig(aConfig, bConfig *Config) error {
+	a := aConfig.TLSConfig
+	b := bConfig.TLSConfig
+	if a == nil || b == nil {
+		if a == b {
+			return nil
+		}
+		return fmt.Errorf("TLS is configured for only one server block")
+	}
+
+	if a.ClientAuth != b.ClientAuth {
+		return fmt.Errorf("client authentication policies differ")
+	}
+	if !certPoolsEqual(a.ClientCAs, b.ClientCAs) {
+		return fmt.Errorf("client CA pools differ")
+	}
+	if !certificatesEqual(a.Certificates, b.Certificates) {
+		return fmt.Errorf("server certificates differ")
+	}
+
+	// Dynamic callbacks cannot be compared directly. Allow them only when the
+	// configs came from the same server block, are the same config object, or a
+	// plugin supplied a trusted identity proving the policies are equivalent.
+	trustedDynamicPolicy := sameServerBlock(aConfig, bConfig) || a == b ||
+		(aConfig.tlsConfigIdentity != nil && aConfig.tlsConfigIdentity == bConfig.tlsConfigIdentity)
+	if !trustedDynamicPolicy && (a.GetCertificate != nil || b.GetCertificate != nil ||
+		a.GetConfigForClient != nil || b.GetConfigForClient != nil ||
+		a.VerifyPeerCertificate != nil || b.VerifyPeerCertificate != nil ||
+		a.VerifyConnection != nil || b.VerifyConnection != nil) {
+		return fmt.Errorf("dynamic TLS callbacks differ")
+	}
+
+	if a.MinVersion != b.MinVersion || a.MaxVersion != b.MaxVersion {
+		return fmt.Errorf("TLS version policies differ")
+	}
+	if !slices.Equal(a.CipherSuites, b.CipherSuites) {
+		return fmt.Errorf("cipher suite policies differ")
+	}
+	if !slices.Equal(a.CurvePreferences, b.CurvePreferences) {
+		return fmt.Errorf("curve preference policies differ")
+	}
+	if a.SessionTicketsDisabled != b.SessionTicketsDisabled {
+		return fmt.Errorf("session ticket policies differ")
+	}
+
+	return nil
+}
+
+func certPoolsEqual(a, b *x509.CertPool) bool {
+	if a == nil || b == nil {
+		return a == b
+	}
+	return a.Equal(b)
+}
+
+func certificatesEqual(a, b []tls.Certificate) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if len(a[i].Certificate) != len(b[i].Certificate) {
+			return false
+		}
+		for j := range a[i].Certificate {
+			if !bytes.Equal(a[i].Certificate[j], b[i].Certificate[j]) {
+				return false
+			}
+		}
+		if !bytes.Equal(a[i].OCSPStaple, b[i].OCSPStaple) {
+			return false
+		}
+		if len(a[i].SignedCertificateTimestamps) != len(b[i].SignedCertificateTimestamps) {
+			return false
+		}
+		for j := range a[i].SignedCertificateTimestamps {
+			if !bytes.Equal(a[i].SignedCertificateTimestamps[j], b[i].SignedCertificateTimestamps[j]) {
+				return false
+			}
+		}
+	}
+	return true
+}