From c72769e2ea077332f982c4b1ef38c6b111e9998b Mon Sep 17 00:00:00 2001 From: Harold Ozouf Date: Wed, 9 Dec 2020 14:16:03 +0100 Subject: [PATCH] Fix TLS options fallback when domain and options are the same Co-authored-by: Kevin Pollet --- pkg/server/router/tcp/router.go | 54 +++-- pkg/server/router/tcp/router_test.go | 323 +++++++++++++++++++++++++-- 2 files changed, 328 insertions(+), 49 deletions(-) diff --git a/pkg/server/router/tcp/router.go b/pkg/server/router/tcp/router.go index ed8bc8ff9..69a04fc4f 100644 --- a/pkg/server/router/tcp/router.go +++ b/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 { - tlsOptionsName := routerHTTPConfig.TLS.Options - if tlsOptionsName != defaultTLSConfigName { - tlsOptionsName = provider.GetQualifiedName(ctxRouter, routerHTTPConfig.TLS.Options) - } + tlsConf, err := m.tlsManager.Get(defaultTLSStoreName, tlsOptionsName) + if err != nil { + routerHTTPConfig.AddError(err, true) + logger.Debug(err) + continue + } - tlsConf, err := m.tlsManager.Get(defaultTLSStoreName, tlsOptionsName) - if err != nil { - routerHTTPConfig.AddError(err, true) - logger.Debug(err) - continue - } + // domain is already in lower case thanks to the domain parsing + if tlsOptionsForHostSNI[domain] == nil { + tlsOptionsForHostSNI[domain] = make(map[string]nameAndConfig) + } + tlsOptionsForHostSNI[domain][tlsOptionsName] = nameAndConfig{ + routerName: routerHTTPName, + TLSConfig: tlsConf, + } - // domain is already in lower case thanks to the domain parsing - if tlsOptionsForHostSNI[domain] == nil { - tlsOptionsForHostSNI[domain] = make(map[string]nameAndConfig) - } - tlsOptionsForHostSNI[domain][routerHTTPConfig.TLS.Options] = nameAndConfig{ - routerName: routerHTTPName, - TLSConfig: tlsConf, - } - - if _, ok := tlsOptionsForHost[domain]; ok { - // Multiple tlsOptions fallback to default - tlsOptionsForHost[domain] = "default" - } else { - tlsOptionsForHost[domain] = routerHTTPConfig.TLS.Options - } + if name, ok := tlsOptionsForHost[domain]; ok && name != tlsOptionsName { + // Different tlsOptions on the same domain fallback to default + tlsOptionsForHost[domain] = defaultTLSConfigName + } else { + tlsOptionsForHost[domain] = tlsOptionsName } } } @@ -304,5 +302,5 @@ func findTLSOptionName(tlsOptionsForHost map[string]string, host string) string return tlsOptions } - return "default" + return defaultTLSConfigName } diff --git a/pkg/server/router/tcp/router_test.go b/pkg/server/router/tcp/router_test.go index 6a966b79d..a00465a86 100644 --- a/pkg/server/router/tcp/router_test.go +++ b/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 - serviceConfig map[string]*runtime.TCPServiceInfo - routerConfig map[string]*runtime.TCPRouterInfo - expectedError int + desc string + httpServiceConfig map[string]*runtime.ServiceInfo + httpRouterConfig map[string]*runtime.RouterInfo + 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, - TCPRouters: test.routerConfig, + Services: test.httpServiceConfig, + 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]tls.Options{ + map[string]traefiktls.Store{}, + 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) + }) + } +}