tls: add cipher flag #9216

ThatsMrTalbot · 2018-01-24T12:39:01Z

WIP implementation of TLS Cyphers flags (#8320)

This adds a new flag to the client and server specifying what tls cypher suites to enable, if not set then the full set will be available

It currently only seems to work when the TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite is provided. If it isn't in the list then I get an error in the etcd logs (see below) and openssl s_client -connect 127.0.0.1:2379 -tls1_2 refuses to connect

Error from etcd logs when TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 is not provided:

Failed to dial 0.0.0.0:2379: connection error: desc = "transport: authentication handshake failed: remote error: tls: handshake failure"; please retry.

Any insight would be appreciated

gyuho · 2018-01-24T21:25:18Z

@dvonthenen Could you help review this? Thanks!

dvonthenen · 2018-01-24T22:33:56Z

@gyuho sure thing!

dvonthenen · 2018-01-30T17:55:37Z

etcdctl/ctlv2/command/util.go

@@ -167,16 +170,24 @@ func getTransport(c *cli.Context) (*http.Transport, error) {
 	if keyfile == "" {
 		keyfile = os.Getenv("ETCDCTL_KEY_FILE")
 	}
+	if insecureSkipVerify == false {
+		insecureSkipVerify = os.Getenv("ETCDCTL_INSECURE_SKIP_TLS_VERIFY") != ""


I think this should be something to the effect of insecureSkipVerify = strings.EqualFold(os.Getenv("ETCDCTL_INSECURE_SKIP_TLS_VERIFY"), "TRUE"). Anything that doesn't match "true" from the ENV VAR is considered implicitly false.

dvonthenen · 2018-01-30T18:04:48Z

@ThatsMrTalbot In reference to only working with TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, I guess that would depend on the version of OpenSSL you are using and the supported cipher suites that are mutually shared between the server and client.

I did test setting a single arbitrary shared cipher between etcd and etcdctl (in my test case TLS_RSA_WITH_AES_256_GCM_SHA384) and it seemed to work just fine.

Just a quick look at OpenSSL latest using the -tls1_2 option, the only mutual ones are (the one with the [ ] is the one you noted works) again this depends on the version you are using:

TLS_RSA_WITH_AES_128_CBC_SHA256 AES128-SHA256
TLS_RSA_WITH_AES_128_GCM_SHA256 AES128-GCM-SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384 AES256-GCM-SHA384
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 ECDHE-RSA-AES128-SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 ECDHE-RSA-AES128-GCM-SHA256
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 ECDHE-RSA-AES256-GCM-SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 ECDHE-ECDSA-AES128-SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 ECDHE-ECDSA-AES128-GCM-SHA256
[TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 ECDHE-ECDSA-AES256-GCM-SHA384]

liggitt · 2018-02-06T17:17:47Z

etcdctl/ctlv2/ctl.go

+		cli.GenericFlag{Name: "cipher-suites",
+			Value: flags.NewStringSliceFlag(tlsutil.AvailableCipherSuites()...),
+			Usage: "Comma-separated list of cipher suites for the server. " +
+				"Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). " +


suggest enumerating the values in AvailableCipherSuites() rather than referencing godoc

liggitt · 2018-02-06T17:18:34Z

etcdctl/ctlv2/ctl.go

@@ -54,6 +56,12 @@ func Start(apiv string) {
 		cli.StringFlag{Name: "cert-file", Value: "", Usage: "identify HTTPS client using this SSL certificate file"},
 		cli.StringFlag{Name: "key-file", Value: "", Usage: "identify HTTPS client using this SSL key file"},
 		cli.StringFlag{Name: "ca-file", Value: "", Usage: "verify certificates of HTTPS-enabled servers using this CA bundle"},
+		cli.BoolFlag{Name: "insecure-skip-tls-verify", Usage: "skip server certificate verification"},
+		cli.GenericFlag{Name: "cipher-suites",
+			Value: flags.NewStringSliceFlag(tlsutil.AvailableCipherSuites()...),


does this mean AvailableCipherSuites() is the default value? I didn't think all available ciphers were included in the default set in go

It is used to validate the provided list, but the default value is still a nil slice (which is what is currently used)

xiang90 · 2018-02-12T19:47:17Z

etcdctl/ctlv2/command/util.go

@@ -155,6 +156,8 @@ func getTransport(c *cli.Context) (*http.Transport, error) {
 	cafile := c.GlobalString("ca-file")
 	certfile := c.GlobalString("cert-file")
 	keyfile := c.GlobalString("key-file")
+	insecureSkipVerify := c.GlobalBool("insecure-skip-tls-verify")


why do we need this in this PR?

agree, would like this removed

xiang90 · 2018-02-12T19:47:38Z

etcdctl/ctlv2/ctl.go

@@ -55,6 +58,12 @@ func Start(apiv string) {
 		cli.StringFlag{Name: "cert-file", Value: "", Usage: "identify HTTPS client using this SSL certificate file"},
 		cli.StringFlag{Name: "key-file", Value: "", Usage: "identify HTTPS client using this SSL key file"},
 		cli.StringFlag{Name: "ca-file", Value: "", Usage: "verify certificates of HTTPS-enabled servers using this CA bundle"},
+		cli.BoolFlag{Name: "insecure-skip-tls-verify", Usage: "skip server certificate verification"},


do we need this in this PR? or it can be separated to another one?

I added it to make local testing easier, can break it out if you want

yea. please break it out.

liggitt · 2018-04-10T18:50:23Z

etcdctl/ctlv3/ctl.go

@@ -63,6 +65,9 @@ func init() {
 	rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.CertFile, "cert", "", "identify secure client using this TLS certificate file")
 	rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.KeyFile, "key", "", "identify secure client using this TLS key file")
 	rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.CAFile, "cacert", "", "verify certificates of TLS-enabled secure servers using this CA bundle")
+	rootCmd.PersistentFlags().StringSliceVar(&globalFlags.TLS.CipherSuites, "cipher-suites", nil, "comma-separated list of cipher suites for the server. "+
+		"Available cipher suites include "+strings.Join(tlsutil.AvailableCipherSuites(), ",")+". "+


nit: join with ", " so text can wrap

liggitt · 2018-04-10T18:50:46Z

etcdmain/help.go

@@ -158,6 +160,8 @@ security flags:
 		peer TLS using self-generated certificates if --peer-key-file and --peer-cert-file are not provided.
 	--peer-crl-file ''
 		path to the peer certificate revocation list file.
+	--cipher-suites ''
+		comma-separated list of cipher suites for the server. Available cipher suites include ` + strings.Join(tlsutil.AvailableCipherSuites(), ",") + `. If omitted, the default Go cipher suites will be used.


liggitt · 2018-04-10T18:52:03Z

pkg/flags/strings.go

+
+// StringSliceFlag implements the flag.Value and pflag.Value interfaces.
+type StringSliceFlag struct {
+	Values []string


this is a confusing name, maybe ValidValues instead?

liggitt · 2018-04-10T18:52:59Z

pkg/flags/strings.go

+
+// NewStringSliceFlag creates a new string slice flag for which any one of the given
+// strings is a valid value, and any other value is an error.
+func NewStringSliceFlag(valids ...string) *StringSliceFlag {


suggest giving separate control over valid values and default value(s), instead of assuming []string{}

liggitt · 2018-04-10T18:56:31Z

pkg/tlsutil/tlscipher.go

+	return c
+}
+
+func TLSCipherSuites(cipherNames []string) ([]uint16, error) {


suggest a test like https://github.com/kubernetes/kubernetes/pull/48859/files#diff-4deda4b18dd1fb07896598924e440970R77 to make sure this doesn't drift or lag golang supported ciphers

gyuho · 2018-06-04T17:07:53Z

Default should be empty, and let Go populate the list with prioritized ordering.
Closing in favor of #9801.

tls: add cipher flag

6e14334

dvonthenen reviewed Jan 30, 2018

View reviewed changes

liggitt reviewed Feb 6, 2018

View reviewed changes

Adam Talbot added 3 commits February 6, 2018 21:05

Merge remote-tracking branch 'origin/master'

9e0ab48

tls: handle ETCDCTL_INSECURE_SKIP_TLS_VERIFY flag in v2 client

85bdac1

tls: cipher flag help message

f50271c

ThatsMrTalbot force-pushed the master branch from ecb3016 to f50271c Compare February 6, 2018 21:33

lint: gosimple util.go

a935f75

xiang90 reviewed Feb 12, 2018

View reviewed changes

gyuho added type/feature area/tls labels Feb 26, 2018

gyuho self-assigned this Mar 2, 2018

gyuho added the WIP - DO NOT MERGE label Mar 2, 2018

gyuho mentioned this pull request Mar 19, 2018

pkg/flags: clean up, add "SelectiveStringsValue" #9457

Merged

liggitt reviewed Apr 10, 2018

View reviewed changes

gyuho assigned joelegasse May 7, 2018

gyuho closed this Jun 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: add cipher flag #9216

tls: add cipher flag #9216

ThatsMrTalbot commented Jan 24, 2018

gyuho commented Jan 24, 2018

dvonthenen commented Jan 24, 2018

dvonthenen Jan 30, 2018

dvonthenen commented Jan 30, 2018 •

edited

Loading

liggitt Feb 6, 2018

liggitt Feb 6, 2018

ThatsMrTalbot Feb 6, 2018 •

edited

Loading

xiang90 Feb 12, 2018

liggitt Apr 10, 2018

xiang90 Feb 12, 2018

ThatsMrTalbot Feb 13, 2018

xiang90 Feb 13, 2018

liggitt Apr 10, 2018

liggitt Apr 10, 2018

liggitt Apr 10, 2018

liggitt Apr 10, 2018 •

edited

Loading

liggitt Apr 10, 2018

gyuho commented Jun 4, 2018

tls: add cipher flag #9216

tls: add cipher flag #9216

Conversation

ThatsMrTalbot commented Jan 24, 2018

gyuho commented Jan 24, 2018

dvonthenen commented Jan 24, 2018

Choose a reason for hiding this comment

dvonthenen commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatsMrTalbot Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Jun 4, 2018

dvonthenen commented Jan 30, 2018 •

edited

Loading

ThatsMrTalbot Feb 6, 2018 •

edited

Loading

liggitt Apr 10, 2018 •

edited

Loading