From 7e3fe48b80083b41e9ff82a474a36484cabc701a Mon Sep 17 00:00:00 2001 From: mpl Date: Tue, 6 Dec 2022 18:28:05 +0100 Subject: [PATCH 1/4] Handle broken TLS conf better Co-authored-by: Jean-Baptiste Doumenjou <925513+jbdoumenjou@users.noreply.github.com> Co-authored-by: Romain --- .../https/https_invalid_tls_options.toml | 60 +++++++ .../fixtures/tcp/multi-tls-options.toml | 11 ++ integration/https_test.go | 50 ++++++ integration/tcp_test.go | 8 + pkg/server/router/router.go | 19 ++- pkg/server/router/router_test.go | 88 ++++++++++- pkg/server/router/tcp/manager.go | 146 ++++++++++++------ pkg/server/router/tcp/router.go | 98 ++++++------ pkg/server/routerfactory.go | 2 +- pkg/tls/tlsmanager.go | 32 ++-- pkg/tls/tlsmanager_test.go | 49 ++---- 11 files changed, 404 insertions(+), 159 deletions(-) create mode 100644 integration/fixtures/https/https_invalid_tls_options.toml diff --git a/integration/fixtures/https/https_invalid_tls_options.toml b/integration/fixtures/https/https_invalid_tls_options.toml new file mode 100644 index 000000000..cc915c4ca --- /dev/null +++ b/integration/fixtures/https/https_invalid_tls_options.toml @@ -0,0 +1,60 @@ +[global] + checkNewVersion = false + sendAnonymousUsage = false + +[log] + level = "DEBUG" + +[entryPoints.websecure] + address = ":4443" + +[api] + insecure = true + +[providers.file] + filename = "{{ .SelfFilename }}" + +## dynamic configuration ## + +[http.routers] + + [http.routers.router1] + entryPoints = ["websecure"] + service = "service1" + rule = "Host(`snitest.com`)" + [http.routers.router1.tls] + options = "invalidTLSOptions" + + [http.routers.router2] + entryPoints = ["websecure"] + service = "service1" + rule = "Host(`snitest.org`)" + [http.routers.router2.tls] + + # fallback router + [http.routers.router3] + entryPoints = ["websecure"] + service = "service1" + rule = "Path(`/`)" + [http.routers.router3.tls] + +[[http.services.service1.loadBalancer.servers]] + url = "http://127.0.0.1:9010" + +[[tls.certificates]] + certFile = "fixtures/https/snitest.com.cert" + keyFile = "fixtures/https/snitest.com.key" + +[[tls.certificates]] + certFile = "fixtures/https/snitest.org.cert" + keyFile = "fixtures/https/snitest.org.key" + +[tls.options] + + [tls.options.default.clientAuth] + # Missing caFile to have an invalid mTLS configuration. + clientAuthType = "RequireAndVerifyClientCert" + + [tls.options.invalidTLSOptions.clientAuth] + # Missing caFile to have an invalid mTLS configuration. + clientAuthType = "RequireAndVerifyClientCert" diff --git a/integration/fixtures/tcp/multi-tls-options.toml b/integration/fixtures/tcp/multi-tls-options.toml index f0b4a931c..480df2d60 100644 --- a/integration/fixtures/tcp/multi-tls-options.toml +++ b/integration/fixtures/tcp/multi-tls-options.toml @@ -33,6 +33,13 @@ [tcp.routers.to-whoami-sni-strict.tls] options = "bar" + [tcp.routers.to-whoami-invalid-tls] + rule = "HostSNI(`whoami-i.test`)" + service = "whoami-no-cert" + entryPoints = [ "tcp" ] + [tcp.routers.to-whoami-invalid-tls.tls] + options = "invalid" + [tcp.services.whoami-no-cert] [tcp.services.whoami-no-cert.loadBalancer] [[tcp.services.whoami-no-cert.loadBalancer.servers]] @@ -45,3 +52,7 @@ [tls.options.bar] minVersion = "VersionTLS13" + + [tls.options.invalid.clientAuth] + # Missing CA files to have an invalid mTLS configuration. + clientAuthType = "RequireAndVerifyClientCert" diff --git a/integration/https_test.go b/integration/https_test.go index 868d31aff..19983faeb 100644 --- a/integration/https_test.go +++ b/integration/https_test.go @@ -1226,3 +1226,53 @@ func (s *HTTPSSuite) TestWithDomainFronting(c *check.C) { c.Assert(err, checker.IsNil) } } + +// TestWithInvalidTLSOption verifies the behavior when using an invalid tlsOption configuration. +func (s *HTTPSSuite) TestWithInvalidTLSOption(c *check.C) { + backend := startTestServer("9010", http.StatusOK, "server1") + defer backend.Close() + + file := s.adaptFile(c, "fixtures/https/https_invalid_tls_options.toml", struct{}{}) + defer os.Remove(file) + cmd, display := s.traefikCmd(withConfigFile(file)) + defer display(c) + err := cmd.Start() + c.Assert(err, checker.IsNil) + defer s.killCmd(cmd) + + // wait for Traefik + err = try.GetRequest("http://127.0.0.1:8080/api/rawdata", 500*time.Millisecond, try.BodyContains("Host(`snitest.com`)")) + c.Assert(err, checker.IsNil) + + testCases := []struct { + desc string + serverName string + }{ + { + desc: "With invalid TLS Options specified", + serverName: "snitest.com", + }, + { + desc: "With invalid Default TLS Options", + serverName: "snitest.org", + }, + { + desc: "With TLS Options without servername (fallback to default)", + }, + } + + for _, test := range testCases { + test := test + + tlsConfig := &tls.Config{ + InsecureSkipVerify: true, + } + if test.serverName != "" { + tlsConfig.ServerName = test.serverName + } + + conn, err := tls.Dial("tcp", "127.0.0.1:4443", tlsConfig) + c.Assert(err, checker.NotNil, check.Commentf("connected to server successfully")) + c.Assert(conn, checker.IsNil) + } +} diff --git a/integration/tcp_test.go b/integration/tcp_test.go index 52a3813c9..0a60ae5b4 100644 --- a/integration/tcp_test.go +++ b/integration/tcp_test.go @@ -116,6 +116,14 @@ func (s *TCPSuite) TestTLSOptions(c *check.C) { _, err = guessWhoTLSMaxVersion("127.0.0.1:8093", "whoami-d.test", true, tls.VersionTLS12) c.Assert(err, checker.NotNil) c.Assert(err.Error(), checker.Contains, "protocol version not supported") + + // Check that we can't reach a route with an invalid mTLS configuration. + conn, err := tls.Dial("tcp", "127.0.0.1:8093", &tls.Config{ + ServerName: "whoami-i.test", + InsecureSkipVerify: true, + }) + c.Assert(conn, checker.IsNil) + c.Assert(err, checker.NotNil) } func (s *TCPSuite) TestNonTLSFallback(c *check.C) { diff --git a/pkg/server/router/router.go b/pkg/server/router/router.go index 6f1819dbe..6359102de 100644 --- a/pkg/server/router/router.go +++ b/pkg/server/router/router.go @@ -3,6 +3,7 @@ package router import ( "context" "errors" + "fmt" "net/http" "github.com/containous/alice" @@ -16,6 +17,7 @@ import ( httpmuxer "github.com/traefik/traefik/v2/pkg/muxer/http" "github.com/traefik/traefik/v2/pkg/server/middleware" "github.com/traefik/traefik/v2/pkg/server/provider" + "github.com/traefik/traefik/v2/pkg/tls" ) type middlewareBuilder interface { @@ -35,10 +37,11 @@ type Manager struct { middlewaresBuilder middlewareBuilder chainBuilder *middleware.ChainBuilder conf *runtime.Configuration + tlsManager *tls.Manager } -// NewManager Creates a new Manager. -func NewManager(conf *runtime.Configuration, serviceManager serviceManager, middlewaresBuilder middlewareBuilder, chainBuilder *middleware.ChainBuilder, metricsRegistry metrics.Registry) *Manager { +// NewManager creates a new Manager. +func NewManager(conf *runtime.Configuration, serviceManager serviceManager, middlewaresBuilder middlewareBuilder, chainBuilder *middleware.ChainBuilder, metricsRegistry metrics.Registry, tlsManager *tls.Manager) *Manager { return &Manager{ routerHandlers: make(map[string]http.Handler), serviceManager: serviceManager, @@ -46,6 +49,7 @@ func NewManager(conf *runtime.Configuration, serviceManager serviceManager, midd middlewaresBuilder: middlewaresBuilder, chainBuilder: chainBuilder, conf: conf, + tlsManager: tlsManager, } } @@ -141,6 +145,17 @@ func (m *Manager) buildRouterHandler(ctx context.Context, routerName string, rou return handler, nil } + if routerConfig.TLS != nil { + // Don't build the router if the TLSOptions configuration is invalid. + tlsOptionsName := tls.DefaultTLSConfigName + if len(routerConfig.TLS.Options) > 0 && routerConfig.TLS.Options != tls.DefaultTLSConfigName { + tlsOptionsName = provider.GetQualifiedName(ctx, routerConfig.TLS.Options) + } + if _, err := m.tlsManager.Get(tls.DefaultTLSStoreName, tlsOptionsName); err != nil { + return nil, fmt.Errorf("building router handler: %w", err) + } + } + handler, err := m.buildHTTPHandler(ctx, routerConfig, routerName) if err != nil { return nil, err diff --git a/pkg/server/router/router_test.go b/pkg/server/router/router_test.go index 26e8a68a3..0ba85b527 100644 --- a/pkg/server/router/router_test.go +++ b/pkg/server/router/router_test.go @@ -20,6 +20,7 @@ import ( "github.com/traefik/traefik/v2/pkg/server/middleware" "github.com/traefik/traefik/v2/pkg/server/service" "github.com/traefik/traefik/v2/pkg/testhelpers" + "github.com/traefik/traefik/v2/pkg/tls" "github.com/traefik/traefik/v2/pkg/types" ) @@ -317,8 +318,9 @@ func TestRouterManager_Get(t *testing.T) { serviceManager := service.NewManager(rtConf.Services, nil, nil, roundTripperManager) middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, nil) chainBuilder := middleware.NewChainBuilder(nil, nil, nil) + tlsManager := tls.NewManager() - routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry()) + routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry(), tlsManager) handlers := routerManager.BuildHandlers(context.Background(), test.entryPoints, false) @@ -423,8 +425,9 @@ func TestAccessLog(t *testing.T) { serviceManager := service.NewManager(rtConf.Services, nil, nil, roundTripperManager) middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, nil) chainBuilder := middleware.NewChainBuilder(nil, nil, nil) + tlsManager := tls.NewManager() - routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry()) + routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry(), tlsManager) handlers := routerManager.BuildHandlers(context.Background(), test.entryPoints, false) @@ -462,6 +465,7 @@ func TestRuntimeConfiguration(t *testing.T) { serviceConfig map[string]*dynamic.Service routerConfig map[string]*dynamic.Router middlewareConfig map[string]*dynamic.Middleware + tlsOptions map[string]tls.Options expectedError int }{ { @@ -665,7 +669,6 @@ func TestRuntimeConfiguration(t *testing.T) { }, expectedError: 1, }, - { desc: "Router with broken middleware", serviceConfig: map[string]*dynamic.Service{ @@ -696,8 +699,71 @@ func TestRuntimeConfiguration(t *testing.T) { }, expectedError: 2, }, + { + desc: "Router with broken tlsOption", + serviceConfig: map[string]*dynamic.Service{ + "foo-service": { + LoadBalancer: &dynamic.ServersLoadBalancer{ + Servers: []dynamic.Server{ + { + URL: "http://127.0.0.1", + }, + }, + }, + }, + }, + middlewareConfig: map[string]*dynamic.Middleware{}, + routerConfig: map[string]*dynamic.Router{ + "bar": { + EntryPoints: []string{"web"}, + Service: "foo-service", + Rule: "Host(`foo.bar`)", + TLS: &dynamic.RouterTLSConfig{ + Options: "broken-tlsOption", + }, + }, + }, + tlsOptions: map[string]tls.Options{ + "broken-tlsOption": { + ClientAuth: tls.ClientAuth{ + ClientAuthType: "foobar", + }, + }, + }, + expectedError: 1, + }, + { + desc: "Router with broken default tlsOption", + serviceConfig: map[string]*dynamic.Service{ + "foo-service": { + LoadBalancer: &dynamic.ServersLoadBalancer{ + Servers: []dynamic.Server{ + { + URL: "http://127.0.0.1", + }, + }, + }, + }, + }, + middlewareConfig: map[string]*dynamic.Middleware{}, + routerConfig: map[string]*dynamic.Router{ + "bar": { + EntryPoints: []string{"web"}, + Service: "foo-service", + Rule: "Host(`foo.bar`)", + TLS: &dynamic.RouterTLSConfig{}, + }, + }, + tlsOptions: map[string]tls.Options{ + "default": { + ClientAuth: tls.ClientAuth{ + ClientAuthType: "foobar", + }, + }, + }, + expectedError: 1, + }, } - for _, test := range testCases { test := test t.Run(test.desc, func(t *testing.T) { @@ -711,6 +777,9 @@ func TestRuntimeConfiguration(t *testing.T) { Routers: test.routerConfig, Middlewares: test.middlewareConfig, }, + TLS: &dynamic.TLSConfiguration{ + Options: test.tlsOptions, + }, }) roundTripperManager := service.NewRoundTripperManager() @@ -718,10 +787,13 @@ func TestRuntimeConfiguration(t *testing.T) { serviceManager := service.NewManager(rtConf.Services, nil, nil, roundTripperManager) middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, nil) chainBuilder := middleware.NewChainBuilder(nil, nil, nil) + tlsManager := tls.NewManager() + tlsManager.UpdateConfigs(context.Background(), nil, test.tlsOptions, nil) - routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry()) + routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry(), tlsManager) _ = routerManager.BuildHandlers(context.Background(), entryPoints, false) + _ = routerManager.BuildHandlers(context.Background(), entryPoints, true) // even though rtConf was passed by argument to the manager builders above, // it's ok to use it as the result we check, because everything worth checking @@ -793,8 +865,9 @@ func TestProviderOnMiddlewares(t *testing.T) { serviceManager := service.NewManager(rtConf.Services, nil, nil, roundTripperManager) middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, nil) chainBuilder := middleware.NewChainBuilder(nil, nil, nil) + tlsManager := tls.NewManager() - routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry()) + routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry(), tlsManager) _ = routerManager.BuildHandlers(context.Background(), entryPoints, false) @@ -861,8 +934,9 @@ func BenchmarkRouterServe(b *testing.B) { serviceManager := service.NewManager(rtConf.Services, nil, nil, staticRoundTripperGetter{res}) middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, nil) chainBuilder := middleware.NewChainBuilder(nil, nil, nil) + tlsManager := tls.NewManager() - routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry()) + routerManager := NewManager(rtConf, serviceManager, middlewaresBuilder, chainBuilder, metrics.NewVoidRegistry(), tlsManager) handlers := routerManager.BuildHandlers(context.Background(), entryPoints, false) diff --git a/pkg/server/router/tcp/manager.go b/pkg/server/router/tcp/manager.go index ff92f3112..b149f5f0b 100644 --- a/pkg/server/router/tcp/manager.go +++ b/pkg/server/router/tcp/manager.go @@ -103,18 +103,21 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string router.SetHTTPHandler(handlerHTTP) + // Even though the error is seemingly ignored (aside from logging it), + // we actually rely later on the fact that a tls config is nil (which happens when an error is returned) to take special steps + // when assigning a handler to a route. defaultTLSConf, err := m.tlsManager.Get(traefiktls.DefaultTLSStoreName, traefiktls.DefaultTLSConfigName) if err != nil { log.FromContext(ctx).Errorf("Error during the build of the default TLS configuration: %v", err) } - // Keyed by domain. The source of truth for doing SNI checking, and for what TLS - // options will actually be used for the connection. + // Keyed by domain. The source of truth for doing SNI checking (domain fronting). // As soon as there's (at least) two different tlsOptions found for the same domain, // we set the value to the default TLS conf. tlsOptionsForHost := map[string]string{} // Keyed by domain, then by options reference. + // The actual source of truth for what TLS options will actually be used for the connection. // As opposed to tlsOptionsForHost, it keeps track of all the (different) TLS // options that occur for a given host name, so that later on we can set relevant // errors and logging for all the routers concerned (i.e. wrongly configured). @@ -142,21 +145,20 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string } if len(domains) == 0 { - // Extra Host(*) rule, for HTTPS routers with no Host rule, and for requests for - // which the SNI does not match _any_ of the other existing routers Host. This is - // only about choosing the TLS configuration. The actual routing will be done - // further on by the HTTPS handler. See examples below. + // Extra Host(*) rule, for HTTPS routers with no Host rule, + // and for requests for which the SNI does not match _any_ of the other existing routers Host. + // This is only about choosing the TLS configuration. + // The actual routing will be done further on by the HTTPS handler. + // See examples below. router.AddHTTPTLSConfig("*", defaultTLSConf) - // The server name (from a Host(SNI) rule) is the only parameter (available in - // HTTP routing rules) on which we can map a TLS config, because it is the only one - // accessible before decryption (we obtain it during the ClientHello). Therefore, - // when a router has no Host rule, it does not make any sense to specify some TLS - // options. Consequently, when it comes to deciding what TLS config will be used, - // for a request that will match an HTTPS router with no Host rule, the result will - // depend on the _others_ existing routers (their Host rule, to be precise), and - // the TLS options associated with them, even though they don't match the incoming - // request. Consider the following examples: + // The server name (from a Host(SNI) rule) is the only parameter (available in HTTP routing rules) on which we can map a TLS config, + // because it is the only one accessible before decryption (we obtain it during the ClientHello). + // Therefore, when a router has no Host rule, it does not make any sense to specify some TLS options. + // Consequently, when it comes to deciding what TLS config will be used, + // for a request that will match an HTTPS router with no Host rule, + // the result will depend on the _others_ existing routers (their Host rule, to be precise), and the TLS options associated with them, + // even though they don't match the incoming request. Consider the following examples: // # conf1 // httpRouter1: @@ -170,17 +172,19 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string // httpRouter2: // rule: Host("foo.com") && PathPrefix("/bar") // tlsoptions: myTLSOptions - // # When a request for "/foo" comes, even though it won't be routed by - // httpRouter2, if its SNI is set to foo.com, myTLSOptions will be used for the TLS - // connection. Otherwise, it will fallback to the default TLS config. + // # When a request for "/foo" comes, even though it won't be routed by httpRouter2, + // # if its SNI is set to foo.com, myTLSOptions will be used for the TLS connection. + // # Otherwise, it will fallback to the default TLS config. logger.Warnf("No domain found in rule %v, the TLS options applied for this router will depend on the SNI of each request", routerHTTPConfig.Rule) } - tlsConf, err := m.tlsManager.Get(traefiktls.DefaultTLSStoreName, tlsOptionsName) - if err != nil { - routerHTTPConfig.AddError(err, true) - logger.Error(err) - continue + // Even though the error is seemingly ignored (aside from logging it), + // we actually rely later on the fact that a tls config is nil (which happens when an error is returned) to take special steps + // when assigning a handler to a route. + tlsConf, tlsConfErr := m.tlsManager.Get(traefiktls.DefaultTLSStoreName, tlsOptionsName) + if tlsConfErr != nil { + // Note: we do not call AddError here because we already did so when buildRouterHandler errored for the same reason. + logger.Error(tlsConfErr) } for _, domain := range domains { @@ -204,6 +208,7 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string sniCheck := snicheck.New(tlsOptionsForHost, handlerHTTPS) + // Keep in mind that defaultTLSConf might be nil here. router.SetHTTPSHandler(sniCheck, defaultTLSConf) logger := log.FromContext(ctx) @@ -217,22 +222,42 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string break } - logger.Debugf("Adding route for %s with TLS options %s", hostSNI, optionsName) - - router.AddHTTPTLSConfig(hostSNI, config) - } else { - routers := make([]string, 0, len(tlsConfigs)) - for _, v := range tlsConfigs { - configsHTTP[v.routerName].AddError(fmt.Errorf("found different TLS options for routers on the same host %v, so using the default TLS options instead", hostSNI), false) - routers = append(routers, v.routerName) + if config == nil { + // we use nil config as a signal to insert a handler + // that enforces that TLS connection attempts to the corresponding (broken) router should fail. + logger.Debugf("Adding special closing route for %s because broken TLS options %s", hostSNI, optionsName) + router.AddHTTPTLSConfig(hostSNI, nil) + continue } - logger.Warnf("Found different TLS options for routers on the same host %v, so using the default TLS options instead for these routers: %#v", hostSNI, routers) - - router.AddHTTPTLSConfig(hostSNI, defaultTLSConf) + logger.Debugf("Adding route for %s with TLS options %s", hostSNI, optionsName) + router.AddHTTPTLSConfig(hostSNI, config) + continue } + + // multiple tlsConfigs + + routers := make([]string, 0, len(tlsConfigs)) + for _, v := range tlsConfigs { + configsHTTP[v.routerName].AddError(fmt.Errorf("found different TLS options for routers on the same host %v, so using the default TLS options instead", hostSNI), false) + routers = append(routers, v.routerName) + } + + logger.Warnf("Found different TLS options for routers on the same host %v, so using the default TLS options instead for these routers: %#v", hostSNI, routers) + if defaultTLSConf == nil { + logger.Debugf("Adding special closing route for %s because broken default TLS options", hostSNI) + } + + router.AddHTTPTLSConfig(hostSNI, defaultTLSConf) } + m.addTCPHandlers(ctx, configs, router) + + return router, nil +} + +// addTCPHandlers creates the TCP handlers defined in configs, and adds them to router. +func (m *Manager) addTCPHandlers(ctx context.Context, configs map[string]*runtime.TCPRouterInfo, router *Router) { for routerName, routerConfig := range configs { ctxRouter := log.With(provider.AddInContext(ctx, routerName), log.Str(log.RouterName, routerName)) logger := log.FromContext(ctxRouter) @@ -251,13 +276,6 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string continue } - handler, err := m.buildTCPHandler(ctxRouter, routerConfig) - if err != nil { - routerConfig.AddError(err, true) - logger.Error(err) - continue - } - domains, err := tcpmuxer.ParseHostSNI(routerConfig.Rule) if err != nil { routerErr := fmt.Errorf("invalid rule: %q , %w", routerConfig.Rule, err) @@ -274,6 +292,16 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string logger.Error(routerErr) } + var handler tcp.Handler + if routerConfig.TLS == nil || routerConfig.TLS.Passthrough { + handler, err = m.buildTCPHandler(ctxRouter, routerConfig) + if err != nil { + routerConfig.AddError(err, true) + logger.Error(err) + continue + } + } + if routerConfig.TLS == nil { logger.Debugf("Adding route for %q", routerConfig.Rule) if err := router.AddRoute(routerConfig.Rule, routerConfig.Priority, handler); err != nil { @@ -285,7 +313,7 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string if routerConfig.TLS.Passthrough { logger.Debugf("Adding Passthrough route for %q", routerConfig.Rule) - if err := router.AddRouteTLS(routerConfig.Rule, routerConfig.Priority, handler, nil); err != nil { + if err := router.muxerTCPTLS.AddRoute(routerConfig.Rule, routerConfig.Priority, handler); err != nil { routerConfig.AddError(err, true) logger.Error(err) } @@ -315,7 +343,15 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string tlsConf, err := m.tlsManager.Get(traefiktls.DefaultTLSStoreName, tlsOptionsName) if err != nil { routerConfig.AddError(err, true) + logger.Error(err) + logger.Debugf("Adding special TLS closing route for %q because broken TLS options %s", routerConfig.Rule, tlsOptionsName) + + err = router.muxerTCPTLS.AddRoute(routerConfig.Rule, routerConfig.Priority, &brokenTLSRouter{}) + if err != nil { + routerConfig.AddError(err, true) + logger.Error(err) + } continue } @@ -327,20 +363,30 @@ func (m *Manager) buildEntryPointHandler(ctx context.Context, configs map[string // rule: HostSNI(foo.com) && ClientIP(IP2) // tlsOption: tlsTwo // i.e. same HostSNI but different tlsOptions - // This is only applicable if the muxer can decide about the routing _before_ - // telling the client about the tlsConf (i.e. before the TLS HandShake). This seems - // to be the case so far with the existing matchers (HostSNI, and ClientIP), so - // it's all good. Otherwise, we would have to do as for HTTPS, i.e. disallow - // different TLS configs for the same HostSNIs. + // This is only applicable if the muxer can decide about the routing _before_ telling the client about the tlsConf (i.e. before the TLS HandShake). + // This seems to be the case so far with the existing matchers (HostSNI, and ClientIP), so it's all good. + // Otherwise, we would have to do as for HTTPS, i.e. disallow different TLS configs for the same HostSNIs. + + handler, err = m.buildTCPHandler(ctxRouter, routerConfig) + if err != nil { + routerConfig.AddError(err, true) + logger.Error(err) + continue + } + + handler = &tcp.TLSHandler{ + Next: handler, + Config: tlsConf, + } logger.Debugf("Adding TLS route for %q", routerConfig.Rule) - if err := router.AddRouteTLS(routerConfig.Rule, routerConfig.Priority, handler, tlsConf); err != nil { + + err = router.muxerTCPTLS.AddRoute(routerConfig.Rule, routerConfig.Priority, handler) + if err != nil { routerConfig.AddError(err, true) logger.Error(err) } } - - return router, nil } func (m *Manager) buildTCPHandler(ctx context.Context, router *runtime.TCPRouterInfo) (tcp.Handler, error) { diff --git a/pkg/server/router/tcp/router.go b/pkg/server/router/tcp/router.go index 0a41e80e2..dca4e6fae 100644 --- a/pkg/server/router/tcp/router.go +++ b/pkg/server/router/tcp/router.go @@ -27,19 +27,20 @@ type Router struct { muxerHTTPS tcpmuxer.Muxer // Forwarder handlers. - // Handles all HTTP requests. + // httpForwarder handles all HTTP requests. httpForwarder tcp.Handler - // Handles (indirectly through muxerHTTPS, or directly) all HTTPS requests. + // httpsForwarder handles (indirectly through muxerHTTPS, or directly) all HTTPS requests. httpsForwarder tcp.Handler - // Neither is used directly, but they are held here, and recreated on config - // reload, so that they can be passed to the Switcher at the end of the config - // reload phase. + // Neither is used directly, but they are held here, and recreated on config reload, + // so that they can be passed to the Switcher at the end of the config reload phase. httpHandler http.Handler httpsHandler http.Handler // TLS configs. - httpsTLSConfig *tls.Config // default TLS config + httpsTLSConfig *tls.Config // default TLS config + // hostHTTPTLSConfig contains TLS configs keyed by SNI. + // A nil config is the hint to set up a brokenTLSRouter. hostHTTPTLSConfig map[string]*tls.Config // TLS configs keyed by SNI } @@ -80,11 +81,11 @@ func (r *Router) GetTLSGetClientInfo() func(info *tls.ClientHelloInfo) (*tls.Con // ServeTCP forwards the connection to the right TCP/HTTP handler. func (r *Router) ServeTCP(conn tcp.WriteCloser) { - // Handling Non-TLS TCP connection early if there is neither HTTP(S) nor TLS - // routers on the entryPoint, and if there is at least one non-TLS TCP router. - // In the case of a non-TLS TCP client (that does not "send" first), we would - // block forever on clientHelloInfo, which is why we want to detect and - // handle that case first and foremost. + // Handling Non-TLS TCP connection early if there is neither HTTP(S) nor TLS routers on the entryPoint, + // and if there is at least one non-TLS TCP router. + // In the case of a non-TLS TCP client (that does not "send" first), + // we would block forever on clientHelloInfo, + // which is why we want to detect and handle that case first and foremost. if r.muxerTCP.HasRoutes() && !r.muxerTCPTLS.HasRoutes() && !r.muxerHTTPS.HasRoutes() { connData, err := tcpmuxer.NewConnData("", conn, nil) if err != nil { @@ -152,9 +153,9 @@ func (r *Router) ServeTCP(conn tcp.WriteCloser) { // (wrapped inside the returned handler) requested for the given HostSNI. handlerHTTPS, catchAllHTTPS := r.muxerHTTPS.Match(connData) if handlerHTTPS != nil && !catchAllHTTPS { - // In order not to depart from the behavior in 2.6, we only allow an HTTPS router - // to take precedence over a TCP-TLS router if it is _not_ an HostSNI(*) router (so - // basically any router that has a specific HostSNI based rule). + // In order not to depart from the behavior in 2.6, + // we only allow an HTTPS router to take precedence over a TCP-TLS router if it is _not_ an HostSNI(*) router + // (so basically any router that has a specific HostSNI based rule). handlerHTTPS.ServeTCP(r.GetConn(conn, hello.peeked)) return } @@ -180,7 +181,7 @@ func (r *Router) ServeTCP(conn tcp.WriteCloser) { return } - // needed to handle 404s for HTTPS, as well as all non-Host (e.g. PathPrefix) matches. + // To handle 404s for HTTPS. if r.httpsForwarder != nil { r.httpsForwarder.ServeTCP(r.GetConn(conn, hello.peeked)) return @@ -194,19 +195,6 @@ func (r *Router) AddRoute(rule string, priority int, target tcp.Handler) error { return r.muxerTCP.AddRoute(rule, priority, target) } -// AddRouteTLS defines a handler for a given rule and sets the matching tlsConfig. -func (r *Router) AddRouteTLS(rule string, priority int, target tcp.Handler, config *tls.Config) error { - // TLS PassThrough - if config == nil { - return r.muxerTCPTLS.AddRoute(rule, priority, target) - } - - return r.muxerTCPTLS.AddRoute(rule, priority, &tcp.TLSHandler{ - Next: target, - Config: config, - }) -} - // AddHTTPTLSConfig defines a handler for a given sniHost and sets the matching tlsConfig. func (r *Router) AddHTTPTLSConfig(sniHost string, config *tls.Config) { if r.hostHTTPTLSConfig == nil { @@ -242,20 +230,44 @@ func (r *Router) SetHTTPForwarder(handler tcp.Handler) { r.httpForwarder = handler } -// SetHTTPSForwarder sets the tcp handler that will forward the TLS connections to an http handler. +// brokenTLSRouter is associated to a Host(SNI) rule for which we know the TLS conf is broken. +// It is used to make sure any attempt to connect to that hostname is closed, +// since we cannot proceed with the intended TLS conf. +type brokenTLSRouter struct{} + +// ServeTCP instantly closes the connection. +func (t *brokenTLSRouter) ServeTCP(conn tcp.WriteCloser) { + _ = conn.Close() +} + +// SetHTTPSForwarder sets the tcp handler that will forward the TLS connections to an HTTP handler. +// It also sets up each TLS handler (with its TLS config) for each Host(SNI) rule we previously kept track of. +// It sets up a special handler that closes the connection if a TLS config is nil. func (r *Router) SetHTTPSForwarder(handler tcp.Handler) { for sniHost, tlsConf := range r.hostHTTPTLSConfig { + var tcpHandler tcp.Handler + if tlsConf == nil { + tcpHandler = &brokenTLSRouter{} + } else { + tcpHandler = &tcp.TLSHandler{ + Next: handler, + Config: tlsConf, + } + } + // muxerHTTPS only contains single HostSNI rules (and no other kind of rules), // so there's no need for specifying a priority for them. - err := r.muxerHTTPS.AddRoute("HostSNI(`"+sniHost+"`)", 0, &tcp.TLSHandler{ - Next: handler, - Config: tlsConf, - }) + err := r.muxerHTTPS.AddRoute("HostSNI(`"+sniHost+"`)", 0, tcpHandler) if err != nil { log.WithoutContext().Errorf("Error while adding route for host: %v", err) } } + if r.httpsTLSConfig == nil { + r.httpsForwarder = &brokenTLSRouter{} + return + } + r.httpsForwarder = &tcp.TLSHandler{ Next: handler, Config: r.httpsTLSConfig, @@ -275,15 +287,14 @@ func (r *Router) SetHTTPSHandler(handler http.Handler, config *tls.Config) { // Conn is a connection proxy that handles Peeked bytes. type Conn struct { - // Peeked are the bytes that have been read from Conn for the - // purposes of route matching, but have not yet been consumed - // by Read calls. It set to nil by Read when fully consumed. + // Peeked are the bytes that have been read from Conn for the purposes of route matching, + // but have not yet been consumed by Read calls. + // It set to nil by Read when fully consumed. Peeked []byte // Conn is the underlying connection. - // It can be type asserted against *net.TCPConn or other types - // as needed. It should not be read from directly unless - // Peeked is nil. + // It can be type asserted against *net.TCPConn or other types as needed. + // It should not be read from directly unless Peeked is nil. tcp.WriteCloser } @@ -320,15 +331,14 @@ func clientHelloInfo(br *bufio.Reader) (*clientHello, error) { return nil, err } - // No valid TLS record has a type of 0x80, however SSLv2 handshakes - // start with a uint16 length where the MSB is set and the first record - // is always < 256 bytes long. Therefore typ == 0x80 strongly suggests - // an SSLv2 client. + // No valid TLS record has a type of 0x80, however SSLv2 handshakes start with an uint16 length + // where the MSB is set and the first record is always < 256 bytes long. + // Therefore, typ == 0x80 strongly suggests an SSLv2 client. const recordTypeSSLv2 = 0x80 const recordTypeHandshake = 0x16 if hdr[0] != recordTypeHandshake { if hdr[0] == recordTypeSSLv2 { - // we consider SSLv2 as TLS and it will be refused by real TLS handshake. + // we consider SSLv2 as TLS, and it will be refused by real TLS handshake. return &clientHello{ isTLS: true, peeked: getPeeked(br), diff --git a/pkg/server/routerfactory.go b/pkg/server/routerfactory.go index fff7e3eea..6b7b80ff4 100644 --- a/pkg/server/routerfactory.go +++ b/pkg/server/routerfactory.go @@ -72,7 +72,7 @@ func (f *RouterFactory) CreateRouters(rtConf *runtime.Configuration) (map[string middlewaresBuilder := middleware.NewBuilder(rtConf.Middlewares, serviceManager, f.pluginBuilder) - routerManager := router.NewManager(rtConf, serviceManager, middlewaresBuilder, f.chainBuilder, f.metricsRegistry) + routerManager := router.NewManager(rtConf, serviceManager, middlewaresBuilder, f.chainBuilder, f.metricsRegistry, f.tlsManager) handlersNonTLS := routerManager.BuildHandlers(ctx, f.entryPointsTCP, false) handlersTLS := routerManager.BuildHandlers(ctx, f.entryPointsTCP, true) diff --git a/pkg/tls/tlsmanager.go b/pkg/tls/tlsmanager.go index 0c7daa992..91fd6402f 100644 --- a/pkg/tls/tlsmanager.go +++ b/pkg/tls/tlsmanager.go @@ -157,19 +157,16 @@ func (m *Manager) Get(storeName, configName string) (*tls.Config, error) { m.lock.RLock() defer m.lock.RUnlock() - var tlsConfig *tls.Config - var err error - sniStrict := false config, ok := m.configs[configName] - if ok { - sniStrict = config.SniStrict - tlsConfig, err = buildTLSConfig(config) - } else { - err = fmt.Errorf("unknown TLS options: %s", configName) + if !ok { + return nil, fmt.Errorf("unknown TLS options: %s", configName) } + + sniStrict = config.SniStrict + tlsConfig, err := buildTLSConfig(config) if err != nil { - tlsConfig = &tls.Config{} + return nil, fmt.Errorf("building TLS config: %w", err) } store := m.getStore(storeName) @@ -177,7 +174,7 @@ func (m *Manager) Get(storeName, configName string) (*tls.Config, error) { err = fmt.Errorf("TLS store %s not found", storeName) } acmeTLSStore := m.getStore(tlsalpn01.ACMETLS1Protocol) - if acmeTLSStore == nil { + if acmeTLSStore == nil && err == nil { err = fmt.Errorf("ACME TLS store %s not found", tlsalpn01.ACMETLS1Protocol) } @@ -188,15 +185,12 @@ func (m *Manager) Get(storeName, configName string) (*tls.Config, error) { certificate := acmeTLSStore.GetBestCertificate(clientHello) if certificate == nil { log.WithoutContext().Debugf("TLS: no certificate for TLSALPN challenge: %s", domainToCheck) - // We want the user to eventually get the (alertUnrecognizedName) "unrecognized - // name" error. - // Unfortunately, if we returned an error here, since we can't use - // the unexported error (errNoCertificates) that our caller (config.getCertificate - // in crypto/tls) uses as a sentinel, it would report an (alertInternalError) - // "internal error" instead of an alertUnrecognizedName. - // Which is why we return no error, and we let the caller detect that there's - // actually no certificate, and fall back into the flow that will report - // the desired error. + // We want the user to eventually get the (alertUnrecognizedName) "unrecognized name" error. + // Unfortunately, if we returned an error here, + // since we can't use the unexported error (errNoCertificates) that our caller (config.getCertificate in crypto/tls) uses as a sentinel, + // it would report an (alertInternalError) "internal error" instead of an alertUnrecognizedName. + // Which is why we return no error, and we let the caller detect that there's actually no certificate, + // and fall back into the flow that will report the desired error. // https://cs.opensource.google/go/go/+/dev.boringcrypto.go1.17:src/crypto/tls/common.go;l=1058 return nil, nil } diff --git a/pkg/tls/tlsmanager_test.go b/pkg/tls/tlsmanager_test.go index 7f3853299..08acf04a2 100644 --- a/pkg/tls/tlsmanager_test.go +++ b/pkg/tls/tlsmanager_test.go @@ -119,8 +119,9 @@ func TestManager_Get(t *testing.T) { }} tlsConfigs := map[string]Options{ - "foo": {MinVersion: "VersionTLS12"}, - "bar": {MinVersion: "VersionTLS11"}, + "foo": {MinVersion: "VersionTLS12"}, + "bar": {MinVersion: "VersionTLS11"}, + "invalid": {CurvePreferences: []string{"42"}}, } testCases := []struct { @@ -140,15 +141,20 @@ func TestManager_Get(t *testing.T) { expectedMinVersion: uint16(tls.VersionTLS11), }, { - desc: "Get an tls config from an invalid name", + desc: "Get a tls config from an invalid name", tlsOptionsName: "unknown", expectedError: true, }, { - desc: "Get an tls config from unexisting 'default' name", + desc: "Get a tls config from unexisting 'default' name", tlsOptionsName: "default", expectedError: true, }, + { + desc: "Get an invalid tls config", + tlsOptionsName: "invalid", + expectedError: true, + }, } tlsManager := NewManager() @@ -161,42 +167,13 @@ func TestManager_Get(t *testing.T) { config, err := tlsManager.Get("default", test.tlsOptionsName) if test.expectedError { - assert.Error(t, err) + require.Nil(t, config) + require.Error(t, err) return } - assert.NoError(t, err) - assert.Equal(t, config.MinVersion, test.expectedMinVersion) - }) - } -} - -func TestManager_Get_GetCertificate(t *testing.T) { - testCases := []struct { - desc string - expectedGetConfigErr require.ErrorAssertionFunc - expectedCertificate assert.ValueAssertionFunc - }{ - { - desc: "Get a default certificate from non-existing store", - expectedGetConfigErr: require.Error, - expectedCertificate: assert.Nil, - }, - } - - tlsManager := NewManager() - - for _, test := range testCases { - test := test - t.Run(test.desc, func(t *testing.T) { - t.Parallel() - - config, err := tlsManager.Get("default", "foo") - test.expectedGetConfigErr(t, err) - - certificate, err := config.GetCertificate(&tls.ClientHelloInfo{}) require.NoError(t, err) - test.expectedCertificate(t, certificate) + assert.Equal(t, config.MinVersion, test.expectedMinVersion) }) } } From abd569701f1d10872e970ac3bd753b3f2957491f Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Wed, 7 Dec 2022 10:02:04 +0100 Subject: [PATCH 2/4] fix: update golang.org/x/net --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index f8e560207..a0d3c1fbf 100644 --- a/go.mod +++ b/go.mod @@ -71,8 +71,8 @@ require ( go.elastic.co/apm v1.13.1 go.elastic.co/apm/module/apmot v1.13.1 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 - golang.org/x/net v0.1.0 - golang.org/x/text v0.4.0 + golang.org/x/net v0.3.1-0.20221206200815-1e63c2f08a10 + golang.org/x/text v0.5.0 golang.org/x/time v0.0.0-20220224211638-0e9765cccd65 golang.org/x/tools v0.1.12 google.golang.org/grpc v1.41.0 @@ -325,8 +325,8 @@ require ( golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 // indirect golang.org/x/oauth2 v0.0.0-20220909003341-f21342109be1 // indirect golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4 // indirect - golang.org/x/sys v0.1.0 // indirect - golang.org/x/term v0.1.0 // indirect + golang.org/x/sys v0.3.0 // indirect + golang.org/x/term v0.3.0 // indirect golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect google.golang.org/api v0.44.0 // indirect google.golang.org/appengine v1.6.7 // indirect diff --git a/go.sum b/go.sum index 00253253e..0020ab332 100644 --- a/go.sum +++ b/go.sum @@ -2093,8 +2093,8 @@ golang.org/x/net v0.0.0-20211216030914-fe4d6282115f/go.mod h1:9nx3DQGgdP8bBQD5qx golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd/go.mod h1:CfG3xpIq0wQ8r1q4Su4UZFWDARRcnwPjda9FqA0JpMk= golang.org/x/net v0.0.0-20220225172249-27dd8689420f/go.mod h1:CfG3xpIq0wQ8r1q4Su4UZFWDARRcnwPjda9FqA0JpMk= golang.org/x/net v0.0.0-20220624214902-1bab6f366d9e/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= -golang.org/x/net v0.1.0 h1:hZ/3BUoy5aId7sCpA/Tc5lt8DkFgdVS2onTpJsZ/fl0= -golang.org/x/net v0.1.0/go.mod h1:Cx3nUiGt4eDBEyega/BKRp+/AlGL8hYe7U9odMt2Cco= +golang.org/x/net v0.3.1-0.20221206200815-1e63c2f08a10 h1:Frnccbp+ok2GkUS2tC84yAq/U9Vg+0sIO7aRL3T4Xnc= +golang.org/x/net v0.3.1-0.20221206200815-1e63c2f08a10/go.mod h1:MBQ8lrhLObU/6UmLb4fmbmk5OcyYmqtbGd/9yIeKjEE= golang.org/x/oauth2 v0.0.0-20180724155351-3d292e4d0cdc/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20181017192945-9dcd33a902f4/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= @@ -2254,14 +2254,14 @@ golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U= -golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.3.0 h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ= +golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= -golang.org/x/term v0.1.0 h1:g6Z6vPFA9dYBAF7DWcH6sCcOntplXsDKcliusYijMlw= -golang.org/x/term v0.1.0/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= +golang.org/x/term v0.3.0 h1:qoo4akIqOcDME5bhc/NgxUdovd6BSS2uMsVjB56q1xI= +golang.org/x/term v0.3.0/go.mod h1:q750SLmJuPmVoN1blW3UFBPREJfb1KmY3vwxfr+nFDA= golang.org/x/text v0.0.0-20160726164857-2910a502d2bf/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -2272,8 +2272,8 @@ golang.org/x/text v0.3.4/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.5/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= -golang.org/x/text v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg= -golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= +golang.org/x/text v0.5.0 h1:OLmvp0KP+FVG99Ct/qFiL/Fhk4zp4QQnZ7b2U+5piUM= +golang.org/x/text v0.5.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= From a8df674dcf77909e6e0f9c0452abf7a7c6857571 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Wed, 7 Dec 2022 10:56:05 +0100 Subject: [PATCH 3/4] fix: flaky tests --- pkg/server/configurationwatcher_test.go | 78 +++++++++++++----------- pkg/server/server_entrypoint_tcp_test.go | 22 +++---- pkg/server/server_entrypoint_udp_test.go | 32 +++++----- 3 files changed, 71 insertions(+), 61 deletions(-) diff --git a/pkg/server/configurationwatcher_test.go b/pkg/server/configurationwatcher_test.go index afd886e84..da8d39c4d 100644 --- a/pkg/server/configurationwatcher_test.go +++ b/pkg/server/configurationwatcher_test.go @@ -58,7 +58,7 @@ func (p *mockProvider) Init() error { func TestNewConfigurationWatcher(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() + t.Cleanup(routinesPool.Stop) pvd := &mockProvider{ messages: []dynamic.Message{{ @@ -115,7 +115,6 @@ func TestNewConfigurationWatcher(t *testing.T) { func TestWaitForRequiredProvider(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() pvdAggregator := &mockProvider{ wait: 5 * time.Millisecond, @@ -151,7 +150,9 @@ func TestWaitForRequiredProvider(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed time.Sleep(20 * time.Millisecond) @@ -162,7 +163,6 @@ func TestWaitForRequiredProvider(t *testing.T) { func TestIgnoreTransientConfiguration(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() config := &dynamic.Configuration{ HTTP: th.BuildConfiguration( @@ -190,7 +190,9 @@ func TestIgnoreTransientConfiguration(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) watcher.allProvidersConfigs <- dynamic.Message{ ProviderName: "mock", @@ -243,7 +245,6 @@ func TestIgnoreTransientConfiguration(t *testing.T) { func TestListenProvidersThrottleProviderConfigReload(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() pvd := &mockProvider{ wait: 10 * time.Millisecond, @@ -274,7 +275,9 @@ func TestListenProvidersThrottleProviderConfigReload(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // Give some time so that the configuration can be processed. time.Sleep(100 * time.Millisecond) @@ -287,7 +290,6 @@ func TestListenProvidersThrottleProviderConfigReload(t *testing.T) { func TestListenProvidersSkipsEmptyConfigs(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() pvd := &mockProvider{ messages: []dynamic.Message{{ProviderName: "mock"}}, @@ -299,7 +301,9 @@ func TestListenProvidersSkipsEmptyConfigs(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed time.Sleep(100 * time.Millisecond) @@ -307,7 +311,6 @@ func TestListenProvidersSkipsEmptyConfigs(t *testing.T) { func TestListenProvidersSkipsSameConfigurationForProvider(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() message := dynamic.Message{ ProviderName: "mock", @@ -331,7 +334,9 @@ func TestListenProvidersSkipsSameConfigurationForProvider(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed time.Sleep(100 * time.Millisecond) @@ -340,7 +345,6 @@ func TestListenProvidersSkipsSameConfigurationForProvider(t *testing.T) { func TestListenProvidersDoesNotSkipFlappingConfiguration(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() configuration := &dynamic.Configuration{ HTTP: th.BuildConfiguration( @@ -374,7 +378,9 @@ func TestListenProvidersDoesNotSkipFlappingConfiguration(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed time.Sleep(100 * time.Millisecond) @@ -407,7 +413,6 @@ func TestListenProvidersDoesNotSkipFlappingConfiguration(t *testing.T) { func TestListenProvidersIgnoreSameConfig(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() configuration := &dynamic.Configuration{ HTTP: th.BuildConfiguration( @@ -453,8 +458,7 @@ func TestListenProvidersIgnoreSameConfig(t *testing.T) { configurationReloads++ lastConfig = conf - // Allows next configurations to be sent by the mock provider - // as soon as the first configuration message is applied. + // Allows next configurations to be sent by the mock provider as soon as the first configuration message is applied. once.Do(func() { pvd.first <- struct{}{} // Wait for all configuration messages to pile in @@ -463,7 +467,9 @@ func TestListenProvidersIgnoreSameConfig(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // Wait long enough time.Sleep(50 * time.Millisecond) @@ -498,7 +504,6 @@ func TestListenProvidersIgnoreSameConfig(t *testing.T) { func TestApplyConfigUnderStress(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() watcher := NewConfigurationWatcher(routinesPool, &mockProvider{}, []string{"defaultEP"}, "") @@ -525,15 +530,16 @@ func TestApplyConfigUnderStress(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) time.Sleep(100 * time.Millisecond) // Ensure that at least two configurations have been applied - // if we simulate being spammed configuration changes by the - // provider(s). - // In theory, checking at least one would be sufficient, but - // checking for two also ensures that we're looping properly, + // if we simulate being spammed configuration changes by the provider(s). + // In theory, checking at least one would be sufficient, + // but checking for two also ensures that we're looping properly, // and that the whole algo holds, etc. t.Log(configurationReloads) assert.GreaterOrEqual(t, configurationReloads, 2) @@ -541,7 +547,6 @@ func TestApplyConfigUnderStress(t *testing.T) { func TestListenProvidersIgnoreIntermediateConfigs(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() configuration := &dynamic.Configuration{ HTTP: th.BuildConfiguration( @@ -596,7 +601,9 @@ func TestListenProvidersIgnoreIntermediateConfigs(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // Wait long enough time.Sleep(500 * time.Millisecond) @@ -631,7 +638,6 @@ func TestListenProvidersIgnoreIntermediateConfigs(t *testing.T) { func TestListenProvidersPublishesConfigForEachProvider(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() configuration := &dynamic.Configuration{ HTTP: th.BuildConfiguration( @@ -656,7 +662,9 @@ func TestListenProvidersPublishesConfigForEachProvider(t *testing.T) { }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed time.Sleep(100 * time.Millisecond) @@ -695,7 +703,6 @@ func TestListenProvidersPublishesConfigForEachProvider(t *testing.T) { func TestPublishConfigUpdatedByProvider(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() pvdConfiguration := dynamic.Configuration{ TCP: &dynamic.TCPConfiguration{ @@ -725,12 +732,14 @@ func TestPublishConfigUpdatedByProvider(t *testing.T) { watcher.AddListener(func(configuration dynamic.Configuration) { publishedConfigCount++ - // Update the provider configuration published in next dynamic Message which should trigger a new publish. + // Update the provider configuration published in next dynamic Message which should trigger a new publishing. pvdConfiguration.TCP.Routers["bar"] = &dynamic.TCPRouter{} }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed. time.Sleep(100 * time.Millisecond) @@ -740,7 +749,6 @@ func TestPublishConfigUpdatedByProvider(t *testing.T) { func TestPublishConfigUpdatedByConfigWatcherListener(t *testing.T) { routinesPool := safe.NewPool(context.Background()) - defer routinesPool.Stop() pvd := &mockProvider{ wait: 10 * time.Millisecond, @@ -774,13 +782,15 @@ func TestPublishConfigUpdatedByConfigWatcherListener(t *testing.T) { watcher.AddListener(func(configuration dynamic.Configuration) { publishedConfigCount++ - // Modify the provided configuration. This should not modify the configuration stored in the configuration - // watcher and cause a new publish. + // Modify the provided configuration. + // This should not modify the configuration stored in the configuration watcher and therefore there will be no new publishing. configuration.TCP.Routers["foo@mock"].Rule = "bar" }) watcher.Start() - defer watcher.Stop() + + t.Cleanup(watcher.Stop) + t.Cleanup(routinesPool.Stop) // give some time so that the configuration can be processed. time.Sleep(100 * time.Millisecond) diff --git a/pkg/server/server_entrypoint_tcp_test.go b/pkg/server/server_entrypoint_tcp_test.go index 73cc9c28b..342d7d9fa 100644 --- a/pkg/server/server_entrypoint_tcp_test.go +++ b/pkg/server/server_entrypoint_tcp_test.go @@ -48,18 +48,13 @@ func TestShutdownTCP(t *testing.T) { require.NoError(t, err) err = router.AddRoute("HostSNI(`*`)", 0, tcp.HandlerFunc(func(conn tcp.WriteCloser) { - for { - _, err := http.ReadRequest(bufio.NewReader(conn)) - - if errors.Is(err, io.EOF) || (err != nil && errors.Is(err, net.ErrClosed)) { - return - } - require.NoError(t, err) - - resp := http.Response{StatusCode: http.StatusOK} - err = resp.Write(conn) - require.NoError(t, err) + _, err := http.ReadRequest(bufio.NewReader(conn)) + if err != nil { + return } + + resp := http.Response{StatusCode: http.StatusOK} + _ = resp.Write(conn) })) require.NoError(t, err) @@ -89,6 +84,7 @@ func testShutdown(t *testing.T, router *tcprouter.Router) { conn, err := startEntrypoint(entryPoint, router) require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) epAddr := entryPoint.listener.Addr().String() @@ -97,14 +93,14 @@ func testShutdown(t *testing.T, router *tcprouter.Router) { time.Sleep(100 * time.Millisecond) - // We need to do a write on the conn before the shutdown to make it "exist". + // We need to do a write on conn before the shutdown to make it "exist". // Because the connection indeed exists as far as TCP is concerned, // but since we only pass it along to the HTTP server after at least one byte is peeked, // the HTTP server (and hence its shutdown) does not know about the connection until that first byte peeked. err = request.Write(conn) require.NoError(t, err) - reader := bufio.NewReader(conn) + reader := bufio.NewReaderSize(conn, 1) // Wait for first byte in response. _, err = reader.Peek(1) require.NoError(t, err) diff --git a/pkg/server/server_entrypoint_udp_test.go b/pkg/server/server_entrypoint_udp_test.go index f219cd98f..0396f434f 100644 --- a/pkg/server/server_entrypoint_udp_test.go +++ b/pkg/server/server_entrypoint_udp_test.go @@ -32,16 +32,19 @@ func TestShutdownUDPConn(t *testing.T) { for { b := make([]byte, 1024*1024) n, err := conn.Read(b) - require.NoError(t, err) - // We control the termination, otherwise we would block on the Read above, until - // conn is closed by a timeout. Which means we would get an error, and even though - // we are in a goroutine and the current test might be over, go test would still - // yell at us if this happens while other tests are still running. + if err != nil { + return + } + + // We control the termination, otherwise we would block on the Read above, + // until conn is closed by a timeout. + // Which means we would get an error, + // and even though we are in a goroutine and the current test might be over, + // go test would still yell at us if this happens while other tests are still running. if string(b[:n]) == "CLOSE" { return } - _, err = conn.Write(b[:n]) - require.NoError(t, err) + _, _ = conn.Write(b[:n]) } })) @@ -68,9 +71,9 @@ func TestShutdownUDPConn(t *testing.T) { // Packet is accepted, but dropped require.NoError(t, err) - // Make sure that our session is yet again still live. This is specifically to - // make sure we don't create a regression in listener's readLoop, i.e. that we only - // terminate the listener's readLoop goroutine by closing its pConn. + // Make sure that our session is yet again still live. + // This is specifically to make sure we don't create a regression in listener's readLoop, + // i.e. that we only terminate the listener's readLoop goroutine by closing its pConn. requireEcho(t, "TEST3", conn, time.Second) done := make(chan bool) @@ -101,10 +104,11 @@ func TestShutdownUDPConn(t *testing.T) { } } -// requireEcho tests that the conn session is live and functional, by writing -// data through it, and expecting the same data as a response when reading on it. -// It fatals if the read blocks longer than timeout, which is useful to detect -// regressions that would make a test wait forever. +// requireEcho tests that conn session is live and functional, +// by writing data through it, +// and expecting the same data as a response when reading on it. +// It fatals if the read blocks longer than timeout, +// which is useful to detect regressions that would make a test wait forever. func requireEcho(t *testing.T, data string, conn io.ReadWriter, timeout time.Duration) { t.Helper() From d97d3a6726ad6e50110dd2ff2b552a8b17eeed55 Mon Sep 17 00:00:00 2001 From: Tom Moulard Date: Wed, 7 Dec 2022 15:14:05 +0100 Subject: [PATCH 4/4] Prepare release v2.9.6 --- CHANGELOG.md | 19 +++++++++++++++++++ script/gcg/traefik-bugfix.toml | 6 +++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2660f4888..76675b738 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,22 @@ +## [v2.9.6](https://github.com/traefik/traefik/tree/v2.9.6) (2022-12-07) +[All Commits](https://github.com/traefik/traefik/compare/v2.9.5...v2.9.6) + +**Bug fixes:** +- **[acme]** Update go-acme/lego to v4.9.1 ([#9550](https://github.com/traefik/traefik/pull/9550) by [ldez](https://github.com/ldez)) +- **[k8s/crd]** Support of allowEmptyServices in TraefikService ([#9424](https://github.com/traefik/traefik/pull/9424) by [jeromeguiard](https://github.com/jeromeguiard)) +- **[logs]** Remove logs of the request ([#9574](https://github.com/traefik/traefik/pull/9574) by [ldez](https://github.com/ldez)) +- **[plugins]** Increase the timeout on plugin download ([#9529](https://github.com/traefik/traefik/pull/9529) by [ldez](https://github.com/ldez)) +- **[server]** Update golang.org/x/net ([#9582](https://github.com/traefik/traefik/pull/9582) by [ldez](https://github.com/ldez)) +- **[tls]** Handle broken TLS conf better ([#9572](https://github.com/traefik/traefik/pull/9572) by [mpl](https://github.com/mpl)) +- **[tracing]** Update DataDog tracing dependency to v1.43.1 ([#9526](https://github.com/traefik/traefik/pull/9526) by [rtribotte](https://github.com/rtribotte)) +- **[webui]** Add missing serialNumber passTLSClientCert option to middleware panel ([#9539](https://github.com/traefik/traefik/pull/9539) by [rtribotte](https://github.com/rtribotte)) + +**Documentation:** +- **[docker]** Add networking example ([#9542](https://github.com/traefik/traefik/pull/9542) by [Janik-Haag](https://github.com/Janik-Haag)) +- **[hub]** Add information about the Hub Agent ([#9560](https://github.com/traefik/traefik/pull/9560) by [nmengin](https://github.com/nmengin)) +- **[k8s/helm]** Update Helm installation section ([#9564](https://github.com/traefik/traefik/pull/9564) by [mloiseleur](https://github.com/mloiseleur)) +- **[middleware]** Clarify PathPrefix matcher greediness ([#9519](https://github.com/traefik/traefik/pull/9519) by [mpl](https://github.com/mpl)) + ## [v2.9.5](https://github.com/traefik/traefik/tree/v2.9.5) (2022-11-17) [All Commits](https://github.com/traefik/traefik/compare/v2.9.4...v2.9.5) diff --git a/script/gcg/traefik-bugfix.toml b/script/gcg/traefik-bugfix.toml index 95aa2b1ab..9db9d689c 100644 --- a/script/gcg/traefik-bugfix.toml +++ b/script/gcg/traefik-bugfix.toml @@ -4,11 +4,11 @@ RepositoryName = "traefik" OutputType = "file" FileName = "traefik_changelog.md" -# example new bugfix v2.9.5 +# example new bugfix v2.9.6 CurrentRef = "v2.9" -PreviousRef = "v2.9.4" +PreviousRef = "v2.9.5" BaseBranch = "v2.9" -FutureCurrentRefName = "v2.9.5" +FutureCurrentRefName = "v2.9.6" ThresholdPreviousRef = 10 ThresholdCurrentRef = 10