Fix TLS options fallback when domain and options are the same

Co-authored-by: Kevin Pollet <pollet.kevin@gmail.com>

pkg/server/router/tcp/router.go

@@ -109,13 +109,18 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string
 	tlsOptionsForHostSNI := map[string]map[string]nameAndConfig{}
 	tlsOptionsForHost := map[string]string{}
 	for routerHTTPName, routerHTTPConfig := range configsHTTP {
-		if len(routerHTTPConfig.TLS.Options) == 0 || routerHTTPConfig.TLS.Options == defaultTLSConfigName {
+		if routerHTTPConfig.TLS == nil {
 			continue
 		}
 
 		ctxRouter := log.With(provider.AddInContext(ctx, routerHTTPName), log.Str(log.RouterName, routerHTTPName))
 		logger := log.FromContext(ctxRouter)
 
+		tlsOptionsName := defaultTLSConfigName
+		if len(routerHTTPConfig.TLS.Options) > 0 && routerHTTPConfig.TLS.Options != defaultTLSConfigName {
+			tlsOptionsName = provider.GetQualifiedName(ctxRouter, routerHTTPConfig.TLS.Options)
+		}
+
 		domains, err := rules.ParseDomains(routerHTTPConfig.Rule)
 		if err != nil {
 			routerErr := fmt.Errorf("invalid rule %s, error: %w", routerHTTPConfig.Rule, err)
@@ -129,34 +134,27 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string
 		}
 
 		for _, domain := range domains {
-			if routerHTTPConfig.TLS != nil {
+			tlsConf, err := m.tlsManager.Get(defaultTLSStoreName, tlsOptionsName)
-				tlsOptionsName := routerHTTPConfig.TLS.Options
+			if err != nil {
-				if tlsOptionsName != defaultTLSConfigName {
+				routerHTTPConfig.AddError(err, true)
-					tlsOptionsName = provider.GetQualifiedName(ctxRouter, routerHTTPConfig.TLS.Options)
+				logger.Debug(err)
-				}
+				continue
+			}
 
-				tlsConf, err := m.tlsManager.Get(defaultTLSStoreName, tlsOptionsName)
+			// domain is already in lower case thanks to the domain parsing
-				if err != nil {
+			if tlsOptionsForHostSNI[domain] == nil {
-					routerHTTPConfig.AddError(err, true)
+				tlsOptionsForHostSNI[domain] = make(map[string]nameAndConfig)
-					logger.Debug(err)
+			}
-					continue
+			tlsOptionsForHostSNI[domain][tlsOptionsName] = nameAndConfig{
-				}
+				routerName: routerHTTPName,
+				TLSConfig:  tlsConf,
+			}
 
-				// domain is already in lower case thanks to the domain parsing
+			if name, ok := tlsOptionsForHost[domain]; ok && name != tlsOptionsName {
-				if tlsOptionsForHostSNI[domain] == nil {
+				// Different tlsOptions on the same domain fallback to default
-					tlsOptionsForHostSNI[domain] = make(map[string]nameAndConfig)
+				tlsOptionsForHost[domain] = defaultTLSConfigName
-				}
+			} else {
-				tlsOptionsForHostSNI[domain][routerHTTPConfig.TLS.Options] = nameAndConfig{
+				tlsOptionsForHost[domain] = tlsOptionsName
-					routerName: routerHTTPName,
-					TLSConfig:  tlsConf,
-				}
-
-				if _, ok := tlsOptionsForHost[domain]; ok {
-					// Multiple tlsOptions fallback to default
-					tlsOptionsForHost[domain] = "default"
-				} else {
-					tlsOptionsForHost[domain] = routerHTTPConfig.TLS.Options
-				}
 			}
 		}
 	}
@@ -304,5 +302,5 @@ func findTLSOptionName(tlsOptionsForHost map[string]string, host string) string
 		return tlsOptions
 	}
 
-	return "default"
+	return defaultTLSConfigName
 }

pkg/server/router/tcp/router_test.go

@@ -2,25 +2,31 @@ package tcp
 
 import (
 	"context"
+	"crypto/tls"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/traefik/traefik/v2/pkg/config/dynamic"
 	"github.com/traefik/traefik/v2/pkg/config/runtime"
 	"github.com/traefik/traefik/v2/pkg/server/service/tcp"
-	"github.com/traefik/traefik/v2/pkg/tls"
+	traefiktls "github.com/traefik/traefik/v2/pkg/tls"
 )
 
 func TestRuntimeConfiguration(t *testing.T) {
 	testCases := []struct {
-		desc          string
+		desc              string
-		serviceConfig map[string]*runtime.TCPServiceInfo
+		httpServiceConfig map[string]*runtime.ServiceInfo
-		routerConfig  map[string]*runtime.TCPRouterInfo
+		httpRouterConfig  map[string]*runtime.RouterInfo
-		expectedError int
+		tcpServiceConfig  map[string]*runtime.TCPServiceInfo
+		tcpRouterConfig   map[string]*runtime.TCPRouterInfo
+		expectedError     int
 	}{
 		{
 			desc: "No error",
-			serviceConfig: map[string]*runtime.TCPServiceInfo{
+			tcpServiceConfig: map[string]*runtime.TCPServiceInfo{
 				"foo-service": {
 					TCPService: &dynamic.TCPService{
 						LoadBalancer: &dynamic.TCPServersLoadBalancer{
@@ -38,7 +44,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 					},
 				},
 			},
-			routerConfig: map[string]*runtime.TCPRouterInfo{
+			tcpRouterConfig: map[string]*runtime.TCPRouterInfo{
 				"foo": {
 					TCPRouter: &dynamic.TCPRouter{
 						EntryPoints: []string{"web"},
@@ -65,9 +71,54 @@ func TestRuntimeConfiguration(t *testing.T) {
 			},
 			expectedError: 0,
 		},
+		{
+			desc: "HTTP routers with same domain but different TLS options",
+			httpServiceConfig: map[string]*runtime.ServiceInfo{
+				"foo-service": {
+					Service: &dynamic.Service{
+						LoadBalancer: &dynamic.ServersLoadBalancer{
+							Servers: []dynamic.Server{
+								{
+									Port: "8085",
+									URL:  "127.0.0.1:8085",
+								},
+								{
+									URL:  "127.0.0.1:8086",
+									Port: "8086",
+								},
+							},
+						},
+					},
+				},
+			},
+			httpRouterConfig: map[string]*runtime.RouterInfo{
+				"foo": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Service:     "foo-service",
+						Rule:        "Host(`bar.foo`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "foo",
+						},
+					},
+				},
+				"bar": {
+					Router: &dynamic.Router{
+
+						EntryPoints: []string{"web"},
+						Service:     "foo-service",
+						Rule:        "Host(`bar.foo`) && PathPrefix(`/path`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "bar",
+						},
+					},
+				},
+			},
+			expectedError: 2,
+		},
 		{
 			desc: "One router with wrong rule",
-			serviceConfig: map[string]*runtime.TCPServiceInfo{
+			tcpServiceConfig: map[string]*runtime.TCPServiceInfo{
 				"foo-service": {
 					TCPService: &dynamic.TCPService{
 						LoadBalancer: &dynamic.TCPServersLoadBalancer{
@@ -80,7 +131,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 					},
 				},
 			},
-			routerConfig: map[string]*runtime.TCPRouterInfo{
+			tcpRouterConfig: map[string]*runtime.TCPRouterInfo{
 				"foo": {
 					TCPRouter: &dynamic.TCPRouter{
 						EntryPoints: []string{"web"},
@@ -101,7 +152,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 		},
 		{
 			desc: "All router with wrong rule",
-			serviceConfig: map[string]*runtime.TCPServiceInfo{
+			tcpServiceConfig: map[string]*runtime.TCPServiceInfo{
 				"foo-service": {
 					TCPService: &dynamic.TCPService{
 						LoadBalancer: &dynamic.TCPServersLoadBalancer{
@@ -114,7 +165,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 					},
 				},
 			},
-			routerConfig: map[string]*runtime.TCPRouterInfo{
+			tcpRouterConfig: map[string]*runtime.TCPRouterInfo{
 				"foo": {
 					TCPRouter: &dynamic.TCPRouter{
 						EntryPoints: []string{"web"},
@@ -134,7 +185,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 		},
 		{
 			desc: "Router with unknown service",
-			serviceConfig: map[string]*runtime.TCPServiceInfo{
+			tcpServiceConfig: map[string]*runtime.TCPServiceInfo{
 				"foo-service": {
 					TCPService: &dynamic.TCPService{
 						LoadBalancer: &dynamic.TCPServersLoadBalancer{
@@ -147,7 +198,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 					},
 				},
 			},
-			routerConfig: map[string]*runtime.TCPRouterInfo{
+			tcpRouterConfig: map[string]*runtime.TCPRouterInfo{
 				"foo": {
 					TCPRouter: &dynamic.TCPRouter{
 						EntryPoints: []string{"web"},
@@ -168,14 +219,14 @@ func TestRuntimeConfiguration(t *testing.T) {
 		},
 		{
 			desc: "Router with broken service",
-			serviceConfig: map[string]*runtime.TCPServiceInfo{
+			tcpServiceConfig: map[string]*runtime.TCPServiceInfo{
 				"foo-service": {
 					TCPService: &dynamic.TCPService{
 						LoadBalancer: nil,
 					},
 				},
 			},
-			routerConfig: map[string]*runtime.TCPRouterInfo{
+			tcpRouterConfig: map[string]*runtime.TCPRouterInfo{
 				"bar": {
 					TCPRouter: &dynamic.TCPRouter{
 						EntryPoints: []string{"web"},
@@ -197,15 +248,17 @@ func TestRuntimeConfiguration(t *testing.T) {
 			entryPoints := []string{"web"}
 
 			conf := &runtime.Configuration{
-				TCPServices: test.serviceConfig,
+				Services:    test.httpServiceConfig,
-				TCPRouters:  test.routerConfig,
+				Routers:     test.httpRouterConfig,
+				TCPServices: test.tcpServiceConfig,
+				TCPRouters:  test.tcpRouterConfig,
 			}
 			serviceManager := tcp.NewManager(conf)
-			tlsManager := tls.NewManager()
+			tlsManager := traefiktls.NewManager()
 			tlsManager.UpdateConfigs(
 				context.Background(),
-				map[string]tls.Store{},
+				map[string]traefiktls.Store{},
-				map[string]tls.Options{
+				map[string]traefiktls.Options{
 					"default": {
 						MinVersion: "VersionTLS10",
 					},
@@ -216,7 +269,7 @@ func TestRuntimeConfiguration(t *testing.T) {
 						MinVersion: "VersionTLS11",
 					},
 				},
-				[]*tls.CertAndStores{})
+				[]*traefiktls.CertAndStores{})
 
 			routerManager := NewManager(conf, serviceManager,
 				nil, nil, tlsManager)
@@ -237,7 +290,235 @@ func TestRuntimeConfiguration(t *testing.T) {
 					allErrors++
 				}
 			}
+			for _, v := range conf.Services {
+				if v.Err != nil {
+					allErrors++
+				}
+			}
+			for _, v := range conf.Routers {
+				if len(v.Err) > 0 {
+					allErrors++
+				}
+			}
 			assert.Equal(t, test.expectedError, allErrors)
 		})
 	}
 }
+
+func TestDomainFronting(t *testing.T) {
+	tests := []struct {
+		desc           string
+		routers        map[string]*runtime.RouterInfo
+		expectedStatus int
+	}{
+		{
+			desc: "Request is misdirected when TLS options are different",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-2@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS:         &dynamic.RouterTLSConfig{},
+					},
+				},
+			},
+			expectedStatus: http.StatusMisdirectedRequest,
+		},
+		{
+			desc: "Request is OK when TLS options are the same",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-2@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+			},
+			expectedStatus: http.StatusOK,
+		},
+		{
+			desc: "Default TLS options is used when options are ambiguous for the same host",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-2@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`) && PathPrefix(`/foo`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "default",
+						},
+					},
+				},
+				"router-3@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+			},
+			expectedStatus: http.StatusMisdirectedRequest,
+		},
+		{
+			desc: "Default TLS options should not be used when options are the same for the same host",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-2@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`) && PathPrefix(`/bar`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-3@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+			},
+			expectedStatus: http.StatusOK,
+		},
+		{
+			desc: "Request is misdirected when TLS options have the same name but from different providers",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+				"router-2@crd": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1",
+						},
+					},
+				},
+			},
+			expectedStatus: http.StatusMisdirectedRequest,
+		},
+		{
+			desc: "Request is OK when TLS options reference from a different provider is the same",
+			routers: map[string]*runtime.RouterInfo{
+				"router-1@file": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host1.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1@crd",
+						},
+					},
+				},
+				"router-2@crd": {
+					Router: &dynamic.Router{
+						EntryPoints: []string{"web"},
+						Rule:        "Host(`host2.local`)",
+						TLS: &dynamic.RouterTLSConfig{
+							Options: "host1@crd",
+						},
+					},
+				},
+			},
+			expectedStatus: http.StatusOK,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			entryPoints := []string{"web"}
+			tlsOptions := map[string]traefiktls.Options{
+				"default": {
+					MinVersion: "VersionTLS10",
+				},
+				"host1@file": {
+					MinVersion: "VersionTLS12",
+				},
+				"host1@crd": {
+					MinVersion: "VersionTLS12",
+				},
+			}
+
+			conf := &runtime.Configuration{
+				Routers: test.routers,
+			}
+
+			serviceManager := tcp.NewManager(conf)
+
+			tlsManager := traefiktls.NewManager()
+			tlsManager.UpdateConfigs(context.Background(), map[string]traefiktls.Store{}, tlsOptions, []*traefiktls.CertAndStores{})
+
+			httpsHandler := map[string]http.Handler{
+				"web": http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {}),
+			}
+
+			routerManager := NewManager(conf, serviceManager, nil, httpsHandler, tlsManager)
+
+			routers := routerManager.BuildHandlers(context.Background(), entryPoints)
+
+			router, ok := routers["web"]
+			require.True(t, ok)
+
+			req := httptest.NewRequest(http.MethodGet, "/", nil)
+			req.Host = "host1.local"
+			req.TLS = &tls.ConnectionState{
+				ServerName: "host2.local",
+			}
+
+			rw := httptest.NewRecorder()
+
+			router.GetHTTPSHandler().ServeHTTP(rw, req)
+
+			assert.Equal(t, test.expectedStatus, rw.Code)
+		})
+	}
+}