Addressing review comments

CHANGELOG.md

@@ -1,6 +1,5 @@
 ## Changelog
 
-* [ENHANCEMENT] Added `-.tls-min-version` and `.tls-cipher-suites` flag. #217
 * [CHANGE] Added new `-consul.cas-retry-delay` flag. It has a default value of `1s`, while previously there was no delay between retries. #178
 * [CHANGE] Flagext: `DayValue` now always uses UTC when parsing or displaying dates. #71
 * [CHANGE] Closer: remove the closer package since it's trivial to just copy/paste. #70
@@ -61,6 +60,7 @@
 * [ENHANCEMENT] ring: optimize shuffle-shard computation when lookback is used, and all instances have registered timestamp within the lookback window. In that case we can immediately return origial ring, because we would select all instances anyway. #181
 * [ENHANCEMENT] Runtimeconfig: Allow providing multiple runtime config yaml files as comma separated list file paths. #183
 * [ENHANCEMENT] Memberlist: Add cluster label support to memberlist client. #187
+* [ENHANCEMENT] Added `-.tls-min-version` and `.tls-cipher-suites` flags. #217
 * [BUGFIX] spanlogger: Support multiple tenant IDs. #59
 * [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
 * [BUGFIX] Ring: `ring_member_ownership_percent` and `ring_tokens_owned` metrics are not updated on scale down. #109

crypto/tls/tls.go

@@ -4,13 +4,13 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"flag"
+	"fmt"
 	"os"
 	"strings"
 
 	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
-	cliFlags "k8s.io/component-base/cli/flag"
 )
 
 // ClientConfig is the config for client TLS.
@@ -27,6 +27,13 @@ type ClientConfig struct {
 var (
 	errKeyMissing  = errors.New("certificate given but no key configured")
 	errCertMissing = errors.New("key given but no certificate configured")
+
+	tlsVersions = map[string]uint16{
+		"VersionTLS10": tls.VersionTLS10,
+		"VersionTLS11": tls.VersionTLS11,
+		"VersionTLS12": tls.VersionTLS12,
+		"VersionTLS13": tls.VersionTLS13,
+	}
 )
 
 // RegisterFlagsWithPrefix registers flags with prefix.
@@ -36,8 +43,8 @@ func (cfg *ClientConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet)
 	f.StringVar(&cfg.CAPath, prefix+".tls-ca-path", "", "Path to the CA certificates file to validate server certificate against. If not set, the host's root CA certificates are used.")
 	f.StringVar(&cfg.ServerName, prefix+".tls-server-name", "", "Override the expected name on the server certificate.")
 	f.BoolVar(&cfg.InsecureSkipVerify, prefix+".tls-insecure-skip-verify", false, "Skip validating server certificate.")
-	f.StringVar(&cfg.CipherSuites, prefix+".tls-cipher-suites", "", "Override the default cipher suite list (separated by commas).")
-	f.StringVar(&cfg.MinVersion, prefix+".tls-min-version", "", "Override the default minimum TLS version.")
+	f.StringVar(&cfg.CipherSuites, prefix+".tls-cipher-suites", "", "Override the default cipher suite list (separated by commas). Allowed values are listed in the crypto/tls package.")
+	f.StringVar(&cfg.MinVersion, prefix+".tls-min-version", "", "Override the default minimum TLS version. Allowed values are listed in the crypto/tls package.")
 }
 
 // GetTLSConfig initialises tls.Config from config options
@@ -76,16 +83,16 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {
 	}
 
 	if cfg.MinVersion != "" {
-		minVersion, err := cliFlags.TLSVersion(cfg.MinVersion)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to set minimum TLS version %s", cfg.MinVersion)
+		minVersion, ok := tlsVersions[cfg.MinVersion]
+		if !ok {
+			return nil, fmt.Errorf("failed to set minimum TLS version: %q", cfg.MinVersion)
 		}
 		config.MinVersion = minVersion
 	}
 
 	if cfg.CipherSuites != "" {
-		rawCipherSuites := strings.Split(cfg.CipherSuites, ",")
-		cipherSuites, err := cliFlags.TLSCipherSuites(rawCipherSuites)
+		cipherSuitesNames := strings.Split(cfg.CipherSuites, ",")
+		cipherSuites, err := mapCipherNamesToIDs(cipherSuitesNames)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to set cipher suites %s", cfg.CipherSuites)
 		}
@@ -108,3 +115,31 @@ func (cfg *ClientConfig) GetGRPCDialOptions(enabled bool) ([]grpc.DialOption, er
 
 	return []grpc.DialOption{grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))}, nil
 }
+
+func mapCipherNamesToIDs(cipherSuiteNames []string) ([]uint16, error) {
+	cipherSuites := []uint16{}
+	allCipherSuites := tlsCipherSuites()
+
+	for _, name := range cipherSuiteNames {
+		id, ok := allCipherSuites[name]
+		if !ok {
+			return nil, fmt.Errorf("unsupported cipher suite: %q", name)
+		}
+		cipherSuites = append(cipherSuites, id)
+	}
+
+	return cipherSuites, nil
+}
+
+func tlsCipherSuites() map[string]uint16 {
+	cipherSuites := map[string]uint16{}
+
+	for _, suite := range tls.CipherSuites() {
+		cipherSuites[suite.Name] = suite.ID
+	}
+	for _, suite := range tls.InsecureCipherSuites() {
+		cipherSuites[suite.Name] = suite.ID
+	}
+
+	return cipherSuites
+}

crypto/tls/tls_test.go

@@ -185,15 +185,176 @@ func TestGetTLSConfig_ServerName(t *testing.T) {
 }
 
 func TestGetTLSConfig_MinVersion(t *testing.T) {
-	c := &ClientConfig{
-		MinVersion: "VersionTLS13",
+	type test struct {
+		desc            string
+		MinVersion      string
+		ExpectedVersion uint16
+		RequireError    bool
+	}
+
+	table := []test{
+		{
+			desc:            "no version set",
+			MinVersion:      "",
+			ExpectedVersion: 0,
+			RequireError:    false,
+		},
+		{
+			desc:            "TLS v1.0 set",
+			MinVersion:      "VersionTLS10",
+			ExpectedVersion: tls.VersionTLS10,
+			RequireError:    false,
+		},
+		{
+			desc:            "TLS v1.1 set",
+			MinVersion:      "VersionTLS11",
+			ExpectedVersion: tls.VersionTLS11,
+			RequireError:    false,
+		},
+		{
+			desc:            "TLS v1.2 set",
+			MinVersion:      "VersionTLS12",
+			ExpectedVersion: tls.VersionTLS12,
+			RequireError:    false,
+		},
+		{
+			desc:            "TLS v1.3 set",
+			MinVersion:      "VersionTLS13",
+			ExpectedVersion: tls.VersionTLS13,
+			RequireError:    false,
+		},
+		{
+			desc:            "bad TLS version set",
+			MinVersion:      "VersionTLS14",
+			ExpectedVersion: 0,
+			RequireError:    true,
+		},
+	}
+
+	for _, tst := range table {
+		tst := tst
+		t.Run(tst.desc, func(t *testing.T) {
+			t.Parallel()
+
+			c := &ClientConfig{
+				MinVersion: tst.MinVersion,
+			}
+
+			tlsConfig, err := c.GetTLSConfig()
+
+			if tst.RequireError {
+				assert.Error(t, err)
+				assert.Nil(t, tlsConfig)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tst.ExpectedVersion, tlsConfig.MinVersion)
+			}
+		})
 	}
-	tlsConfig, err := c.GetTLSConfig()
-	assert.NoError(t, err)
-	assert.Equal(t, uint16(tls.VersionTLS13), tlsConfig.MinVersion)
 }
 
 func TestGetTLSConfig_CipherSuites(t *testing.T) {
+	type test struct {
+		desc                 string
+		CipherSuites         string
+		ExpectedCipherSuites []uint16
+		RequireError         bool
+	}
+
+	cipherSuiteNames := "TLS_RSA_WITH_RC4_128_SHA," +
+		"TLS_RSA_WITH_3DES_EDE_CBC_SHA," +
+		"TLS_RSA_WITH_AES_128_CBC_SHA," +
+		"TLS_RSA_WITH_AES_256_CBC_SHA," +
+		"TLS_RSA_WITH_AES_128_CBC_SHA256," +
+		"TLS_RSA_WITH_AES_128_GCM_SHA256," +
+		"TLS_RSA_WITH_AES_256_GCM_SHA384," +
+		"TLS_ECDHE_ECDSA_WITH_RC4_128_SHA," +
+		"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA," +
+		"TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA," +
+		"TLS_ECDHE_RSA_WITH_RC4_128_SHA," +
+		"TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA," +
+		"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA," +
+		"TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA," +
+		"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256," +
+		"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256," +
+		"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256," +
+		"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256," +
+		"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384," +
+		"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384," +
+		"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256," +
+		"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256," +
+		"TLS_AES_128_GCM_SHA256," +
+		"TLS_AES_256_GCM_SHA384," +
+		"TLS_CHACHA20_POLY1305_SHA256"
+
+	table := []test{
+		{
+			desc:                 "no cipher suites set",
+			CipherSuites:         "",
+			ExpectedCipherSuites: nil,
+			RequireError:         false,
+		},
+		{
+			desc:         "all cipher suites set",
+			CipherSuites: cipherSuiteNames,
+			ExpectedCipherSuites: []uint16{
+				tls.TLS_RSA_WITH_RC4_128_SHA,
+				tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
+				tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+				tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+				tls.TLS_RSA_WITH_AES_128_CBC_SHA256,
+				tls.TLS_RSA_WITH_AES_128_GCM_SHA256,
+				tls.TLS_RSA_WITH_AES_256_GCM_SHA384,
+				tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
+				tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+				tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+				tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA,
+				tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+				tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+				tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+				tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
+				tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
+				tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+				tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+				tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+				tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+				tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
+				tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,
+				tls.TLS_AES_128_GCM_SHA256,
+				tls.TLS_AES_256_GCM_SHA384,
+				tls.TLS_CHACHA20_POLY1305_SHA256,
+			},
+			RequireError: false,
+		},
+		{
+			desc:                 "bad cipher suites set",
+			CipherSuites:         "TLS_NO_SUITE",
+			ExpectedCipherSuites: nil,
+			RequireError:         true,
+		},
+	}
+
+	for _, tst := range table {
+		tst := tst
+		t.Run(tst.desc, func(t *testing.T) {
+			t.Parallel()
+
+			c := &ClientConfig{
+				CipherSuites: tst.CipherSuites,
+			}
+
+			tlsConfig, err := c.GetTLSConfig()
+
+			if tst.RequireError {
+				assert.Error(t, err)
+				assert.Nil(t, tlsConfig)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tst.ExpectedCipherSuites, tlsConfig.CipherSuites)
+			}
+		})
+	}
+
 	c := &ClientConfig{
 		CipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
 	}

go.mod

@@ -30,7 +30,6 @@ require (
 	google.golang.org/grpc v1.38.0
 	gopkg.in/yaml.v2 v2.4.0
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
-	k8s.io/component-base v0.21.4
 )
 
 require (
@@ -43,7 +42,6 @@ require (
 	github.com/felixge/httpsnoop v1.0.1 // indirect
 	github.com/go-kit/kit v0.10.0 // indirect
 	github.com/go-logfmt/logfmt v0.5.1 // indirect
-	github.com/go-logr/logr v0.4.0 // indirect
 	github.com/gogo/googleapis v1.1.0 // indirect
 	github.com/golang/protobuf v1.5.2 // indirect
 	github.com/google/btree v1.0.0 // indirect
@@ -59,7 +57,7 @@ require (
 	github.com/jpillora/backoff v1.0.0 // indirect
 	github.com/mattn/go-colorable v0.1.6 // indirect
 	github.com/mattn/go-isatty v0.0.12 // indirect
-	github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
+	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
 	github.com/miekg/dns v1.1.26 // indirect
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
 	github.com/mitchellh/mapstructure v1.1.2 // indirect
@@ -72,8 +70,7 @@ require (
 	github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 // indirect
 	github.com/sercand/kuberesolver v2.4.0+incompatible // indirect
 	github.com/sirupsen/logrus v1.8.1 // indirect
-	github.com/spf13/pflag v1.0.5 // indirect
-	github.com/stretchr/objx v0.2.0 // indirect
+	github.com/stretchr/objx v0.1.1 // indirect
 	github.com/uber/jaeger-client-go v2.28.0+incompatible // indirect
 	github.com/uber/jaeger-lib v2.2.0+incompatible // indirect
 	github.com/weaveworks/promrus v1.2.0 // indirect
@@ -87,8 +84,6 @@ require (
 	google.golang.org/appengine v1.6.6 // indirect
 	google.golang.org/genproto v0.0.0-20210602131652-f16073e35f0c // indirect
 	google.golang.org/protobuf v1.28.1 // indirect
-	k8s.io/apimachinery v0.21.4 // indirect
-	k8s.io/klog/v2 v2.8.0 // indirect
 )
 
 replace k8s.io/client-go v12.0.0+incompatible => k8s.io/client-go v0.21.4