fix(tls): Update TLS strict cipher suite to actually work (#20921)

.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

.gitignore

@@ -17,6 +17,10 @@ influxd.bolt
 # GPG private keys
 private.key
 
+# TLS keys generated for testing
+test.crt
+test.key
+
 # Project distribution
 /dist
 

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]
 

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

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)
+}

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(signals.WithStandardSignals(ctx), o); err != nil {
-		if err := l.run(runCtx, 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,

cmd/influxd/launcher/launcher.go

@@ -95,6 +95,7 @@ const (
 type Launcher struct {
 	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) Done() <-chan struct{} {
-func (m *Launcher) Cancel() { m.cancel() }
+	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,8 +892,8 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) {
 		)
 	}
 
-	{
+	platformHandler := http.NewPlatformHandler(
-		platformHandler := http.NewPlatformHandler(m.apibackend,
+		m.apibackend,
 		http.WithResourceHandler(stacksHTTPServer),
 		http.WithResourceHandler(templatesHTTPServer),
 		http.WithResourceHandler(onboardHTTPServer),
@@ -915,7 +910,7 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) {
 	)
 
 	httpLogger := m.log.With(zap.String("service", "http"))
-		m.httpServer.Handler = http.NewRootHandler(
+	var httpHandler nethttp.Handler = http.NewRootHandler(
 		"platform",
 		http.WithLog(httpLogger),
 		http.WithAPIHandler(platformHandler),
@@ -924,98 +919,121 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) {
 	)
 
 	if opts.LogLevel == zap.DebugLevel {
-			m.httpServer.Handler = http.LoggingMW(httpLogger)(m.httpServer.Handler)
+		httpHandler = http.LoggingMW(httpLogger)(httpHandler)
 	}
 	// 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)
+		httpHandler = http.DebugFlush(ctx, httpHandler, flushers)
 	}
-	}
-
-	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
-	}
-
-	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
 }

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)
 }
 

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