From 99228631856fc567625ffd1a19e30cecad3ca49b Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Tue, 16 Mar 2021 10:45:27 -0400 Subject: [PATCH] fix(tls): Update TLS strict cipher suite to actually work (#20921) --- .circleci/config.yml | 20 ++ .gitignore | 4 + CHANGELOG.md | 1 + Makefile | 5 +- cmd/influxd/launcher/_tlstests/tls_test.go | 97 ++++++++ cmd/influxd/launcher/cmd.go | 7 +- cmd/influxd/launcher/launcher.go | 266 +++++++++++---------- cmd/influxd/launcher/launcher_helpers.go | 11 +- etc/test-tls.sh | 39 +++ 9 files changed, 320 insertions(+), 130 deletions(-) create mode 100644 cmd/influxd/launcher/_tlstests/tls_test.go create mode 100755 etc/test-tls.sh diff --git a/.circleci/config.yml b/.circleci/config.yml index adf05f186c..f67ce16535 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,6 +15,9 @@ workflows: - fluxtest: requires: - godeps + - tlstest: + requires: + - godeps - influxql_validation: requires: - godeps @@ -83,6 +86,9 @@ workflows: - fluxtest: requires: - godeps + - tlstest: + requires: + - godeps - deploy_nightly: requires: - gotest @@ -91,6 +97,7 @@ workflows: - jslint - influxql_validation - influxql_integration + - tlstest filters: branches: only: @@ -478,6 +485,19 @@ jobs: - install_core_deps - run: make test-flux + tlstest: + docker: + - image: cimg/go:1.15.6 + working_directory: /home/circleci/go/src/github.com/influxdata/influxdb + steps: + - checkout + - restore_cache: + name: Restore GOPATH/pkg/mod + keys: + - influxdb-gomod-sum-{{ checksum "go.sum" }} + - install_core_deps + - run: make test-tls + influxql_validation: docker: - image: cimg/go:1.15.6 diff --git a/.gitignore b/.gitignore index f066bdaed6..a90fe4d486 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,10 @@ influxd.bolt # GPG private keys private.key +# TLS keys generated for testing +test.crt +test.key + # Project distribution /dist diff --git a/CHANGELOG.md b/CHANGELOG.md index af5c97b00f..7e53fbad39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ or `/query` HTTP endpoints. 1. [20875](https://github.com/influxdata/influxdb/pull/20875): Prevent "do not have an execution context" error when parsing Flux options in tasks. 1. [20932](https://github.com/influxdata/influxdb/pull/20932): Prevent time field names from being formatted in the Table visualization 1. [20929](https://github.com/influxdata/influxdb/pull/20929): Log error details when `influxd upgrade` fails to migrate databases. +1. [20921](https://github.com/influxdata/influxdb/pull/20921): Fix the cipher suite used when TLS strict ciphers are enabled in `influxd`. ## v2.0.4 [2021-02-08] diff --git a/Makefile b/Makefile index 9377f76b14..9d75984fee 100644 --- a/Makefile +++ b/Makefile @@ -154,6 +154,9 @@ test-go: test-flux: @./etc/test-flux.sh +test-tls: + @./etc/test-tls.sh + test-influxql-integration: $(GO_TEST) -mod=readonly ./influxql/_v1tests @@ -225,4 +228,4 @@ dshell: dshell-image @docker container run --rm -p 8086:8086 -p 8080:8080 -u $(shell id -u) -it -v $(shell pwd):/code -w /code influxdb:dshell # .PHONY targets represent actions that do not create an actual file. -.PHONY: all $(SUBDIRS) run fmt checkfmt tidy checktidy checkgenerate test test-go test-js test-go-race bench clean node_modules vet nightly chronogiraffe dist ping protoc e2e run-e2e influxd libflux flags dshell dclean docker-image-flux docker-image-influx pkg-config +.PHONY: all $(SUBDIRS) run fmt checkfmt tidy checktidy checkgenerate test test-go test-js test-go-race test-tls bench clean node_modules vet nightly chronogiraffe dist ping protoc e2e run-e2e influxd libflux flags dshell dclean docker-image-flux docker-image-influx pkg-config diff --git a/cmd/influxd/launcher/_tlstests/tls_test.go b/cmd/influxd/launcher/_tlstests/tls_test.go new file mode 100644 index 0000000000..22b831db1a --- /dev/null +++ b/cmd/influxd/launcher/_tlstests/tls_test.go @@ -0,0 +1,97 @@ +package tlstests + +import ( + "context" + "os" + "testing" + + "github.com/influxdata/influxdb/v2/cmd/influxd/launcher" + "github.com/influxdata/influxdb/v2/http" + "github.com/stretchr/testify/require" +) + +const ( + certPathVar = "INFLUXDB_TEST_SSL_CERT_PATH" + certKeyVar = "INFLUXDB_TEST_SSL_KEY_PATH" +) + +var ( + certPath string + keyPath string +) + +func init() { + certPath = os.Getenv(certPathVar) + keyPath = os.Getenv(certKeyVar) +} + +func TestTLS_NonStrict(t *testing.T) { + require.NotEmpty(t, certPath, "INFLUXDB_TEST_SSL_CERT_PATH must be set to run this test") + require.NotEmpty(t, keyPath, "INFLUXDB_TEST_SSL_KEY_PATH must be set to run this test") + ctx := context.Background() + + for _, tlsVersion := range []string{"1.0", "1.1", "1.2", "1.3"} { + tlsVersion := tlsVersion + t.Run(tlsVersion, func(t *testing.T) { + l := launcher.NewTestLauncher() + l.RunOrFail(t, ctx, func(o *launcher.InfluxdOpts) { + o.HttpTLSCert = certPath + o.HttpTLSKey = keyPath + o.HttpTLSMinVersion = tlsVersion + o.HttpTLSStrictCiphers = false + }) + defer l.ShutdownOrFail(t, ctx) + + req, err := l.NewHTTPRequest("GET", "/ping", "", "") + require.NoError(t, err) + require.Regexp(t, "https://.*", req.URL) + + client := http.NewClient("https", true) + _, err = client.Do(req) + require.NoError(t, err) + }) + } +} + +func TestTLS_Strict(t *testing.T) { + require.NotEmpty(t, certPath, "INFLUXDB_TEST_SSL_CERT_PATH must be set to run this test") + require.NotEmpty(t, keyPath, "INFLUXDB_TEST_SSL_KEY_PATH must be set to run this test") + ctx := context.Background() + + for _, tlsVersion := range []string{"1.0", "1.1", "1.2", "1.3"} { + tlsVersion := tlsVersion + t.Run(tlsVersion, func(t *testing.T) { + l := launcher.NewTestLauncher() + l.RunOrFail(t, ctx, func(o *launcher.InfluxdOpts) { + o.HttpTLSCert = certPath + o.HttpTLSKey = keyPath + o.HttpTLSMinVersion = tlsVersion + o.HttpTLSStrictCiphers = true + }) + defer l.ShutdownOrFail(t, ctx) + + req, err := l.NewHTTPRequest("GET", "/ping", "", "") + require.NoError(t, err) + require.Regexp(t, "https://.*", req.URL) + + client := http.NewClient("https", true) + _, err = client.Do(req) + require.NoError(t, err) + }) + } +} + +func TestTLS_UnsupportedVersion(t *testing.T) { + require.NotEmpty(t, certPath, "INFLUXDB_TEST_SSL_CERT_PATH must be set to run this test") + require.NotEmpty(t, keyPath, "INFLUXDB_TEST_SSL_KEY_PATH must be set to run this test") + ctx := context.Background() + + l := launcher.NewTestLauncher() + err := l.Run(t, ctx, func(o *launcher.InfluxdOpts) { + o.HttpTLSCert = certPath + o.HttpTLSKey = keyPath + o.HttpTLSMinVersion = "1.4" + o.HttpTLSStrictCiphers = true + }) + require.Error(t, err) +} diff --git a/cmd/influxd/launcher/cmd.go b/cmd/influxd/launcher/cmd.go index 95ee418f32..e979611806 100644 --- a/cmd/influxd/launcher/cmd.go +++ b/cmd/influxd/launcher/cmd.go @@ -103,11 +103,10 @@ func cmdRunE(ctx context.Context, o *InfluxdOpts) func() error { l.log = logger // Start the launcher and wait for it to exit on SIGINT or SIGTERM. - runCtx := signals.WithStandardSignals(ctx) - if err := l.run(runCtx, o); err != nil { + if err := l.run(signals.WithStandardSignals(ctx), o); err != nil { return err } - <-runCtx.Done() + <-l.Done() // Tear down the launcher, allowing it a few seconds to finish any // in-progress requests. @@ -361,7 +360,7 @@ func (o *InfluxdOpts) bindCliOpts() []cli.Opt { DestP: &o.HttpTLSStrictCiphers, Flag: "tls-strict-ciphers", Default: o.HttpTLSStrictCiphers, - Desc: "Restrict accept ciphers to: ECDHE_RSA_WITH_AES_256_GCM_SHA384, ECDHE_RSA_WITH_AES_256_CBC_SHA, RSA_WITH_AES_256_GCM_SHA384, RSA_WITH_AES_256_CBC_SHA", + Desc: "Restrict accept ciphers to: ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, ECDHE_RSA_WITH_AES_128_GCM_SHA256, ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, ECDHE_RSA_WITH_AES_256_GCM_SHA384, ECDHE_ECDSA_WITH_CHACHA20_POLY1305, ECDHE_RSA_WITH_CHACHA20_POLY1305", }, { DestP: &o.NoTasks, diff --git a/cmd/influxd/launcher/launcher.go b/cmd/influxd/launcher/launcher.go index 3d3c776419..5aedc4ef80 100644 --- a/cmd/influxd/launcher/launcher.go +++ b/cmd/influxd/launcher/launcher.go @@ -93,8 +93,9 @@ const ( // Launcher represents the main program execution. type Launcher struct { - wg sync.WaitGroup - cancel func() + wg sync.WaitGroup + cancel func() + doneChan <-chan struct{} flagger feature.Flagger @@ -110,6 +111,7 @@ type Launcher struct { httpPort int httpServer *nethttp.Server + tlsEnabled bool natsServer *nats.Server natsPort int @@ -142,11 +144,6 @@ func (m *Launcher) Registry() *prom.Registry { return m.reg } -// URL returns the URL to connect to the HTTP server. -func (m *Launcher) URL() string { - return fmt.Sprintf("http://127.0.0.1:%d", m.httpPort) -} - // NatsURL returns the URL to connection to the NATS server. func (m *Launcher) NatsURL() string { return fmt.Sprintf("http://127.0.0.1:%d", m.natsPort) @@ -214,14 +211,16 @@ func (m *Launcher) Shutdown(ctx context.Context) error { return nil } -// Cancel executes the context cancel on the program. Used for testing. -func (m *Launcher) Cancel() { m.cancel() } +func (m *Launcher) Done() <-chan struct{} { + return m.doneChan +} func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { span, ctx := tracing.StartSpanFromContext(ctx) defer span.Finish() ctx, m.cancel = context.WithCancel(ctx) + m.doneChan = ctx.Done() info := platform.GetBuildInfo() m.log.Info("Welcome to InfluxDB", @@ -616,10 +615,6 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { log.Info("Stopping") }(m.log) - m.httpServer = &nethttp.Server{ - Addr: opts.HttpBindAddress, - } - if m.flagger == nil { m.flagger = feature.DefaultFlagger() if len(opts.FeatureFlags) > 0 { @@ -897,125 +892,148 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { ) } - { - platformHandler := http.NewPlatformHandler(m.apibackend, - http.WithResourceHandler(stacksHTTPServer), - http.WithResourceHandler(templatesHTTPServer), - http.WithResourceHandler(onboardHTTPServer), - http.WithResourceHandler(authHTTPServer), - http.WithResourceHandler(labelHandler), - http.WithResourceHandler(sessionHTTPServer.SignInResourceHandler()), - http.WithResourceHandler(sessionHTTPServer.SignOutResourceHandler()), - http.WithResourceHandler(userHTTPServer.MeResourceHandler()), - http.WithResourceHandler(userHTTPServer.UserResourceHandler()), - http.WithResourceHandler(orgHTTPServer), - http.WithResourceHandler(bucketHTTPServer), - http.WithResourceHandler(v1AuthHTTPServer), - http.WithResourceHandler(dashboardServer), - ) + platformHandler := http.NewPlatformHandler( + m.apibackend, + http.WithResourceHandler(stacksHTTPServer), + http.WithResourceHandler(templatesHTTPServer), + http.WithResourceHandler(onboardHTTPServer), + http.WithResourceHandler(authHTTPServer), + http.WithResourceHandler(labelHandler), + http.WithResourceHandler(sessionHTTPServer.SignInResourceHandler()), + http.WithResourceHandler(sessionHTTPServer.SignOutResourceHandler()), + http.WithResourceHandler(userHTTPServer.MeResourceHandler()), + http.WithResourceHandler(userHTTPServer.UserResourceHandler()), + http.WithResourceHandler(orgHTTPServer), + http.WithResourceHandler(bucketHTTPServer), + http.WithResourceHandler(v1AuthHTTPServer), + http.WithResourceHandler(dashboardServer), + ) - httpLogger := m.log.With(zap.String("service", "http")) - m.httpServer.Handler = http.NewRootHandler( - "platform", - http.WithLog(httpLogger), - http.WithAPIHandler(platformHandler), - http.WithPprofEnabled(!opts.ProfilingDisabled), - http.WithMetrics(m.reg, !opts.MetricsDisabled), - ) + httpLogger := m.log.With(zap.String("service", "http")) + var httpHandler nethttp.Handler = http.NewRootHandler( + "platform", + http.WithLog(httpLogger), + http.WithAPIHandler(platformHandler), + http.WithPprofEnabled(!opts.ProfilingDisabled), + http.WithMetrics(m.reg, !opts.MetricsDisabled), + ) - if opts.LogLevel == zap.DebugLevel { - m.httpServer.Handler = http.LoggingMW(httpLogger)(m.httpServer.Handler) - } - // If we are in testing mode we allow all data to be flushed and removed. - if opts.Testing { - m.httpServer.Handler = http.DebugFlush(ctx, m.httpServer.Handler, flushers) - } + if opts.LogLevel == zap.DebugLevel { + httpHandler = http.LoggingMW(httpLogger)(httpHandler) } - - ln, err := net.Listen("tcp", opts.HttpBindAddress) - if err != nil { - m.log.Error("failed http listener", zap.Error(err)) - m.log.Info("Stopping") - return err + // If we are in testing mode we allow all data to be flushed and removed. + if opts.Testing { + httpHandler = http.DebugFlush(ctx, httpHandler, flushers) } - var cer tls.Certificate - transport := "http" - - if opts.HttpTLSCert != "" && opts.HttpTLSKey != "" { - var err error - cer, err = tls.LoadX509KeyPair(opts.HttpTLSCert, opts.HttpTLSKey) - - if err != nil { - m.log.Error("failed to load x509 key pair", zap.Error(err)) - m.log.Info("Stopping") - return err - } - transport = "https" - - // Sensible default - var tlsMinVersion uint16 = tls.VersionTLS12 - - switch opts.HttpTLSMinVersion { - case "1.0": - m.log.Warn("Setting the minimum version of TLS to 1.0 - this is discouraged. Please use 1.2 or 1.3") - tlsMinVersion = tls.VersionTLS10 - case "1.1": - m.log.Warn("Setting the minimum version of TLS to 1.1 - this is discouraged. Please use 1.2 or 1.3") - tlsMinVersion = tls.VersionTLS11 - case "1.2": - tlsMinVersion = tls.VersionTLS12 - case "1.3": - tlsMinVersion = tls.VersionTLS13 - } - - strictCiphers := []uint16{ - tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_GCM_SHA384, - tls.TLS_RSA_WITH_AES_256_CBC_SHA, - } - - // nil uses the default cipher suite - var cipherConfig []uint16 = nil - - // TLS 1.3 does not support configuring the Cipher suites - if tlsMinVersion != tls.VersionTLS13 && opts.HttpTLSStrictCiphers { - cipherConfig = strictCiphers - } - - m.httpServer.TLSConfig = &tls.Config{ - CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256}, - PreferServerCipherSuites: true, - MinVersion: tlsMinVersion, - CipherSuites: cipherConfig, - } - } - - if addr, ok := ln.Addr().(*net.TCPAddr); ok { - m.httpPort = addr.Port - } - - m.wg.Add(1) - go func(log *zap.Logger) { - defer m.wg.Done() - log.Info("Listening", zap.String("transport", transport), zap.String("addr", opts.HttpBindAddress), zap.Int("port", m.httpPort)) - - if cer.Certificate != nil { - if err := m.httpServer.ServeTLS(ln, opts.HttpTLSCert, opts.HttpTLSKey); err != nethttp.ErrServerClosed { - log.Error("Failed https service", zap.Error(err)) - } - } else { - if err := m.httpServer.Serve(ln); err != nethttp.ErrServerClosed { - log.Error("Failed http service", zap.Error(err)) - } - } - log.Info("Stopping") - }(m.log) - if !opts.ReportingDisabled { m.runReporter(ctx) } + if err := m.runHTTP(opts, httpHandler); err != nil { + return err + } + + return nil +} + +// runHTTP configures and launches a listener for incoming HTTP(S) requests. +// The listener is run in a separate goroutine. If it fails to start up, it +// will cancel the launcher. +func (m *Launcher) runHTTP(opts *InfluxdOpts, handler nethttp.Handler) error { + log := m.log.With(zap.String("service", "tcp-listener")) + + m.httpServer = &nethttp.Server{ + Addr: opts.HttpBindAddress, + Handler: handler, + } + + ln, err := net.Listen("tcp", opts.HttpBindAddress) + if err != nil { + log.Error("Failed to set up TCP listener", zap.String("addr", opts.HttpBindAddress), zap.Error(err)) + return err + } + if addr, ok := ln.Addr().(*net.TCPAddr); ok { + m.httpPort = addr.Port + } + m.wg.Add(1) + + m.tlsEnabled = opts.HttpTLSCert != "" && opts.HttpTLSKey != "" + if !m.tlsEnabled { + if opts.HttpTLSCert != "" || opts.HttpTLSKey != "" { + log.Warn("TLS requires specifying both cert and key, falling back to HTTP") + } + + go func(log *zap.Logger) { + defer m.wg.Done() + log.Info("Listening", zap.String("transport", "http"), zap.String("addr", opts.HttpBindAddress), zap.Int("port", m.httpPort)) + + if err := m.httpServer.Serve(ln); err != nethttp.ErrServerClosed { + log.Error("Failed to serve HTTP", zap.Error(err)) + m.cancel() + } + log.Info("Stopping") + }(log) + + return nil + } + + if _, err = tls.LoadX509KeyPair(opts.HttpTLSCert, opts.HttpTLSKey); err != nil { + log.Error("Failed to load x509 key pair", zap.String("cert-path", opts.HttpTLSCert), zap.String("key-path", opts.HttpTLSKey)) + return err + } + + var tlsMinVersion uint16 + var useStrictCiphers = opts.HttpTLSStrictCiphers + switch opts.HttpTLSMinVersion { + case "1.0": + log.Warn("Setting the minimum version of TLS to 1.0 - this is discouraged. Please use 1.2 or 1.3") + tlsMinVersion = tls.VersionTLS10 + case "1.1": + log.Warn("Setting the minimum version of TLS to 1.1 - this is discouraged. Please use 1.2 or 1.3") + tlsMinVersion = tls.VersionTLS11 + case "1.2": + tlsMinVersion = tls.VersionTLS12 + case "1.3": + if useStrictCiphers { + log.Warn("TLS version 1.3 does not support configuring strict ciphers") + useStrictCiphers = false + } + tlsMinVersion = tls.VersionTLS13 + default: + return fmt.Errorf("unsupported TLS version: %s", opts.HttpTLSMinVersion) + } + + // nil uses the default cipher suite + var cipherConfig []uint16 = nil + if useStrictCiphers { + // See https://ssl-config.mozilla.org/#server=go&version=1.14.4&config=intermediate&guideline=5.6 + cipherConfig = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + } + } + + m.httpServer.TLSConfig = &tls.Config{ + CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256}, + PreferServerCipherSuites: !useStrictCiphers, + MinVersion: tlsMinVersion, + CipherSuites: cipherConfig, + } + + go func(log *zap.Logger) { + defer m.wg.Done() + log.Info("Listening", zap.String("transport", "https"), zap.String("addr", opts.HttpBindAddress), zap.Int("port", m.httpPort)) + + if err := m.httpServer.ServeTLS(ln, opts.HttpTLSCert, opts.HttpTLSKey); err != nethttp.ErrServerClosed { + log.Error("Failed to serve HTTPS", zap.Error(err)) + m.cancel() + } + log.Info("Stopping") + }(log) return nil } diff --git a/cmd/influxd/launcher/launcher_helpers.go b/cmd/influxd/launcher/launcher_helpers.go index f6a32b7ef1..95460c2183 100644 --- a/cmd/influxd/launcher/launcher_helpers.go +++ b/cmd/influxd/launcher/launcher_helpers.go @@ -89,6 +89,15 @@ func NewTestLauncherServer() *TestLauncher { return l } +// URL returns the URL to connect to the HTTP server. +func (tl *TestLauncher) URL() string { + transport := "http" + if tl.Launcher.tlsEnabled { + transport = "https" + } + return fmt.Sprintf("%s://127.0.0.1:%d", transport, tl.Launcher.httpPort) +} + type OptSetter = func(o *InfluxdOpts) func (tl *TestLauncher) SetFlagger(flagger feature.Flagger) { @@ -129,7 +138,7 @@ func (tl *TestLauncher) Run(tb zaptest.TestingT, ctx context.Context, setters .. // Shutdown stops the program and cleans up temporary paths. func (tl *TestLauncher) Shutdown(ctx context.Context) error { defer os.RemoveAll(tl.Path) - tl.Cancel() + tl.cancel() return tl.Launcher.Shutdown(ctx) } diff --git a/etc/test-tls.sh b/etc/test-tls.sh new file mode 100755 index 0000000000..4040fa415c --- /dev/null +++ b/etc/test-tls.sh @@ -0,0 +1,39 @@ +#!/bin/bash +set -eu -o pipefail +readonly GO=${GO:-go} + +generate_keypair() { + local cert key + cert="$1" + key="$2" + + openssl req -batch -new \ + -newkey rsa:4096 \ + -x509 \ + -sha256 \ + -days 1 \ + -nodes \ + -subj "/C=US/ST=CA/L=./O=./OU=./CN=." \ + -out "${cert}" \ + -keyout "${key}" +} + +run_tls_tests() { + local cert key + cert="$1" + key="$2" + + 1>&2 echo Running go tests... + INFLUXDB_TEST_SSL_CERT_PATH="${cert}" INFLUXDB_TEST_SSL_KEY_PATH="${key}" "${GO}" test -mod=readonly ./cmd/influxd/launcher/_tlstests +} + +main() { + local cert key + cert=$(pwd)/test.crt + key=$(pwd)/test.key + + trap "rm -f '${cert}' '${key}'" EXIT + generate_keypair "${cert}" "${key}" + run_tls_tests "${cert}" "${key}" +} +main