Merge pull request #504 from xuezhaojun/fix-ciphersuites-option

Fix: ciphersuites length equals to 1(expecting 0) when using the default value.

Author: k8s-ci-robot

cmd/server/app/options/options.go

@@ -101,7 +101,7 @@ type ProxyRunOptions struct {
 	// also checks if given comma separated list contains cipher from tls.InsecureCipherSuites().
 	// NOTE that cipher suites are not configurable for TLS1.3,
 	// see: https://pkg.go.dev/crypto/tls#Config, so in that case, this option won't have any effect.
-	CipherSuites string
+	CipherSuites []string
 }
 
 func (o *ProxyRunOptions) Flags() *pflag.FlagSet {
@@ -136,7 +136,7 @@ func (o *ProxyRunOptions) Flags() *pflag.FlagSet {
 	flags.IntVar(&o.KubeconfigBurst, "kubeconfig-burst", o.KubeconfigBurst, "Maximum client burst (proxy server uses this client to authenticate agent tokens).")
 	flags.StringVar(&o.AuthenticationAudience, "authentication-audience", o.AuthenticationAudience, "Expected agent's token authentication audience (used with agent-namespace, agent-service-account, kubeconfig).")
 	flags.StringVar(&o.ProxyStrategies, "proxy-strategies", o.ProxyStrategies, "The list of proxy strategies used by the server to pick a backend/tunnel, available strategies are: default, destHost.")
-	flags.StringVar(&o.CipherSuites, "cipher-suites", o.CipherSuites, "The comma separated list of allowed cipher suites. Has no effect on TLS1.3. Empty means allow default list.")
+	flags.StringSliceVar(&o.CipherSuites, "cipher-suites", o.CipherSuites, "The comma separated list of allowed cipher suites. Has no effect on TLS1.3. Empty means allow default list.")
 
 	flags.Bool("warn-on-channel-limit", true, "This behavior is now thread safe and always on. This flag will be removed in a future release.")
 	flags.MarkDeprecated("warn-on-channel-limit", "This behavior is now thread safe and always on. This flag will be removed in a future release.")
@@ -306,10 +306,9 @@ func (o *ProxyRunOptions) Validate() error {
 	}
 
 	// validate the cipher suites
-	if o.CipherSuites != "" {
+	if len(o.CipherSuites) != 0 {
 		acceptedCiphers := util.GetAcceptedCiphers()
-		css := strings.Split(o.CipherSuites, ",")
-		for _, cipher := range css {
+		for _, cipher := range o.CipherSuites {
 			_, ok := acceptedCiphers[cipher]
 			if !ok {
 				return fmt.Errorf("cipher suite %s not supported, doesn't exist or considered as insecure", cipher)
@@ -352,7 +351,7 @@ func NewProxyRunOptions() *ProxyRunOptions {
 		KubeconfigBurst:           0,
 		AuthenticationAudience:    "",
 		ProxyStrategies:           "default",
-		CipherSuites:              "",
+		CipherSuites:              make([]string, 0),
 	}
 	return &o
 }

cmd/server/app/options/options_test.go

@@ -18,10 +18,11 @@ package options
 
 import (
 	"fmt"
-	"github.com/stretchr/testify/assert"
 	"reflect"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 /*
@@ -59,7 +60,7 @@ func TestDefaultServerOptions(t *testing.T) {
 	assertDefaultValue(t, "KubeconfigBurst", defaultServerOptions.KubeconfigBurst, 0)
 	assertDefaultValue(t, "AuthenticationAudience", defaultServerOptions.AuthenticationAudience, "")
 	assertDefaultValue(t, "ProxyStrategies", defaultServerOptions.ProxyStrategies, "default")
-	assertDefaultValue(t, "CipherSuites", defaultServerOptions.CipherSuites, "")
+	assertDefaultValue(t, "CipherSuites", defaultServerOptions.CipherSuites, make([]string, 0))
 }
 
 func assertDefaultValue(t *testing.T, fieldName string, actual, expected interface{}) {
@@ -139,10 +140,17 @@ func TestValidate(t *testing.T) {
 			value:    49152,
 			expected: fmt.Errorf("please do not try to use ephemeral port 49152 for the health port"),
 		},
+		"CommaSparatedCipherSuites": {
+			field:    "CipherSuites",
+			value:    "TLS_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+			expected: nil,
+		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			testServerOptions := NewProxyRunOptions()
-			if tc.field != "" {
+			if tc.field == "CipherSuites" {
+				testServerOptions.Flags().Set("cipher-suites", tc.value.(string))
+			} else if tc.field != "" {
 				rv := reflect.ValueOf(testServerOptions)
 				rv = rv.Elem()
 				fv := rv.FieldByName(tc.field)

cmd/server/app/server.go

@@ -30,7 +30,6 @@ import (
 	"runtime"
 	runpprof "runtime/pprof"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -260,13 +259,13 @@ func (p *Proxy) runUDSFrontendServer(ctx context.Context, o *options.ProxyRunOpt
 	return stop, nil
 }
 
-func (p *Proxy) getTLSConfig(caFile, certFile, keyFile, cipherSuites string) (*tls.Config, error) {
+func (p *Proxy) getTLSConfig(caFile, certFile, keyFile string, cipherSuites []string) (*tls.Config, error) {
 	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load X509 key pair %s and %s: %v", certFile, keyFile, err)
 	}
 
-	cipherSuiteIDs := tlsCipherSuites(strings.Split(cipherSuites, ","))
+	cipherSuiteIDs := tlsCipherSuites(cipherSuites)
 
 	if caFile == "" {
 		return &tls.Config{Certificates: []tls.Certificate{cert}, MinVersion: tls.VersionTLS12, CipherSuites: cipherSuiteIDs}, nil