Skip to content
Permalink
Browse files Browse the repository at this point in the history
Handle broken TLS conf better
Co-authored-by: Jean-Baptiste Doumenjou <925513+jbdoumenjou@users.noreply.github.com>
Co-authored-by: Romain <rtribotte@users.noreply.github.com>
  • Loading branch information
3 people committed Dec 6, 2022
1 parent 778188e commit 7e3fe48
Show file tree
Hide file tree
Showing 11 changed files with 402 additions and 157 deletions.
60 changes: 60 additions & 0 deletions 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"
11 changes: 11 additions & 0 deletions integration/fixtures/tcp/multi-tls-options.toml
Expand Up @@ -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]]
Expand All @@ -45,3 +52,7 @@

[tls.options.bar]
minVersion = "VersionTLS13"

[tls.options.invalid.clientAuth]
# Missing CA files to have an invalid mTLS configuration.
clientAuthType = "RequireAndVerifyClientCert"
50 changes: 50 additions & 0 deletions integration/https_test.go
Expand Up @@ -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)
}
}
8 changes: 8 additions & 0 deletions integration/tcp_test.go
Expand Up @@ -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) {
Expand Down
19 changes: 17 additions & 2 deletions pkg/server/router/router.go
Expand Up @@ -3,6 +3,7 @@ package router
import (
"context"
"errors"
"fmt"
"net/http"

"github.com/containous/alice"
Expand All @@ -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 {
Expand All @@ -35,17 +37,19 @@ 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,
metricsRegistry: metricsRegistry,
middlewaresBuilder: middlewaresBuilder,
chainBuilder: chainBuilder,
conf: conf,
tlsManager: tlsManager,
}
}

Expand Down Expand Up @@ -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
Expand Down
88 changes: 81 additions & 7 deletions pkg/server/router/router_test.go
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
}{
{
Expand Down Expand Up @@ -665,7 +669,6 @@ func TestRuntimeConfiguration(t *testing.T) {
},
expectedError: 1,
},

{
desc: "Router with broken middleware",
serviceConfig: map[string]*dynamic.Service{
Expand Down Expand Up @@ -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) {
Expand All @@ -711,17 +777,23 @@ func TestRuntimeConfiguration(t *testing.T) {
Routers: test.routerConfig,
Middlewares: test.middlewareConfig,
},
TLS: &dynamic.TLSConfiguration{
Options: test.tlsOptions,
},
})

roundTripperManager := service.NewRoundTripperManager()
roundTripperManager.Update(map[string]*dynamic.ServersTransport{"default@internal": {}})
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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 7e3fe48

Please sign in to comment.