From 19e4a5489b86a783954718868b9f9d2ce9accd7c Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 23 May 2018 13:34:53 -0400 Subject: [PATCH 1/4] add support for tls PreferServerCipherSuites add further tests for tls configuration --- command/agent/config-test-fixtures/basic.hcl | 3 + command/agent/config_parse.go | 1 + command/agent/config_parse_test.go | 19 ++-- helper/tlsutil/config.go | 43 ++++---- helper/tlsutil/config_test.go | 99 +++++++++++++++++++ nomad/structs/config/tls.go | 11 +++ nomad/structs/config/tls_test.go | 27 ++--- .../docs/agent/configuration/tls.html.md | 3 + 8 files changed, 169 insertions(+), 37 deletions(-) diff --git a/command/agent/config-test-fixtures/basic.hcl b/command/agent/config-test-fixtures/basic.hcl index 0c83ae95f..7398dff43 100644 --- a/command/agent/config-test-fixtures/basic.hcl +++ b/command/agent/config-test-fixtures/basic.hcl @@ -156,6 +156,9 @@ tls { key_file = "pipe" rpc_upgrade_mode = true verify_https_client = true + tls_prefer_server_cipher_suites = true + tls_cipher_suites = "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256" + tls_min_version = "tls12" } sentinel { import "foo" { diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index b9577327a..9d82050cb 100644 --- a/command/agent/config_parse.go +++ b/command/agent/config_parse.go @@ -763,6 +763,7 @@ func parseTLSConfig(result **config.TLSConfig, list *ast.ObjectList) error { "verify_https_client", "tls_cipher_suites", "tls_min_version", + "tls_prefer_server_cipher_suites", } if err := helper.CheckHCLKeys(listVal, valid); err != nil { diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index b47fa572c..994ed1d2a 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -167,14 +167,17 @@ func TestConfig_Parse(t *testing.T) { Token: "12345", }, TLSConfig: &config.TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: "foo", - CertFile: "bar", - KeyFile: "pipe", - RPCUpgradeMode: true, - VerifyHTTPSClient: true, + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: "foo", + CertFile: "bar", + KeyFile: "pipe", + RPCUpgradeMode: true, + VerifyHTTPSClient: true, + TLSPreferServerCipherSuites: true, + TLSCipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + TLSMinVersion: "tls12", }, HTTPAPIResponseHeaders: map[string]string{ "Access-Control-Allow-Origin": "*", diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 8bd6ff2e4..dfd1ba6ab 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -107,6 +107,12 @@ type Config struct { // MinVersion contains the minimum SSL/TLS version that is accepted. MinVersion uint16 + + // PreferServerCipherSuites controls whether the server selects the + // client's most preferred ciphersuite, or the server's most preferred + // ciphersuite. If true then the server's preference, as expressed in + // the order of elements in CipherSuites, is used. + PreferServerCipherSuites bool } func NewTLSConfiguration(newConf *config.TLSConfig, verifyIncoming, verifyOutgoing bool) (*Config, error) { @@ -121,15 +127,16 @@ func NewTLSConfiguration(newConf *config.TLSConfig, verifyIncoming, verifyOutgoi } return &Config{ - VerifyIncoming: verifyIncoming, - VerifyOutgoing: verifyOutgoing, - VerifyServerHostname: newConf.VerifyServerHostname, - CAFile: newConf.CAFile, - CertFile: newConf.CertFile, - KeyFile: newConf.KeyFile, - KeyLoader: newConf.GetKeyLoader(), - CipherSuites: ciphers, - MinVersion: minVersion, + VerifyIncoming: verifyIncoming, + VerifyOutgoing: verifyOutgoing, + VerifyServerHostname: newConf.VerifyServerHostname, + CAFile: newConf.CAFile, + CertFile: newConf.CertFile, + KeyFile: newConf.KeyFile, + KeyLoader: newConf.GetKeyLoader(), + CipherSuites: ciphers, + MinVersion: minVersion, + PreferServerCipherSuites: newConf.TLSPreferServerCipherSuites, }, nil } @@ -184,10 +191,11 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) { } // Create the tlsConfig tlsConfig := &tls.Config{ - RootCAs: x509.NewCertPool(), - InsecureSkipVerify: true, - CipherSuites: c.CipherSuites, - MinVersion: c.MinVersion, + RootCAs: x509.NewCertPool(), + InsecureSkipVerify: true, + CipherSuites: c.CipherSuites, + MinVersion: c.MinVersion, + PreferServerCipherSuites: c.PreferServerCipherSuites, } if c.VerifyServerHostname { tlsConfig.InsecureSkipVerify = false @@ -304,10 +312,11 @@ func WrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) { func (c *Config) IncomingTLSConfig() (*tls.Config, error) { // Create the tlsConfig tlsConfig := &tls.Config{ - ClientCAs: x509.NewCertPool(), - ClientAuth: tls.NoClientCert, - CipherSuites: c.CipherSuites, - MinVersion: c.MinVersion, + ClientCAs: x509.NewCertPool(), + ClientAuth: tls.NoClientCert, + CipherSuites: c.CipherSuites, + MinVersion: c.MinVersion, + PreferServerCipherSuites: c.PreferServerCipherSuites, } // Parse the CA cert if any diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 2f38054e9..9d71272cb 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -167,6 +167,60 @@ func TestConfig_OutgoingTLS_WithKeyPair(t *testing.T) { assert.NotNil(cert) } +func TestConfig_OutgoingTLS_PreferServerCipherSuites(t *testing.T) { + require := require.New(t) + + { + conf := &Config{ + VerifyOutgoing: true, + CAFile: cacert, + } + tlsConfig, err := conf.OutgoingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.PreferServerCipherSuites, false) + } + { + conf := &Config{ + VerifyOutgoing: true, + CAFile: cacert, + PreferServerCipherSuites: true, + } + tlsConfig, err := conf.OutgoingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.PreferServerCipherSuites, true) + } +} + +func TestConfig_OutgoingTLS_TLSCipherSuites(t *testing.T) { + require := require.New(t) + + { + defaultCiphers := []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + } + conf := &Config{ + VerifyOutgoing: true, + CAFile: cacert, + CipherSuites: defaultCiphers, + } + tlsConfig, err := conf.OutgoingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.CipherSuites, defaultCiphers) + } + { + conf := &Config{ + VerifyOutgoing: true, + CAFile: cacert, + CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}, + } + tlsConfig, err := conf.OutgoingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.CipherSuites, []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}) + } +} + func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) @@ -416,6 +470,51 @@ func TestConfig_IncomingTLS_NoVerify(t *testing.T) { } } +func TestConfig_IncomingTLS_PreferServerCipherSuites(t *testing.T) { + require := require.New(t) + + { + conf := &Config{} + tlsConfig, err := conf.IncomingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.PreferServerCipherSuites, false) + } + { + conf := &Config{ + PreferServerCipherSuites: true, + } + tlsConfig, err := conf.IncomingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.PreferServerCipherSuites, true) + } +} + +func TestConfig_IncomingTLS_TLSCipherSuites(t *testing.T) { + require := require.New(t) + + { + defaultCiphers := []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + } + conf := &Config{ + CipherSuites: defaultCiphers, + } + tlsConfig, err := conf.IncomingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.CipherSuites, defaultCiphers) + } + { + conf := &Config{ + CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}, + } + tlsConfig, err := conf.IncomingTLSConfig() + require.Nil(err) + require.Equal(tlsConfig.CipherSuites, []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}) + } +} + func TestConfig_ParseCiphers_Valid(t *testing.T) { require := require.New(t) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index b0ebfda49..4476c78b6 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -63,6 +63,12 @@ type TLSConfig struct { // TLSMinVersion is used to set the minimum TLS version used for TLS // connections. Should be either "tls10", "tls11", or "tls12". TLSMinVersion string `mapstructure:"tls_min_version"` + + // TLSPreferServerCipherSuites controls whether the server selects the + // client's most preferred ciphersuite, or the server's most preferred + // ciphersuite. If true then the server's preference, as expressed in + // the order of elements in CipherSuites, is used. + TLSPreferServerCipherSuites bool `mapstructure:"tls_prefer_server_cipher_suites"` } type KeyLoader struct { @@ -158,6 +164,8 @@ func (t *TLSConfig) Copy() *TLSConfig { new.TLSCipherSuites = t.TLSCipherSuites new.TLSMinVersion = t.TLSMinVersion + new.TLSPreferServerCipherSuites = t.TLSPreferServerCipherSuites + new.SetChecksum() return new @@ -211,6 +219,9 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig { if b.TLSMinVersion != "" { result.TLSMinVersion = b.TLSMinVersion } + if b.TLSPreferServerCipherSuites { + result.TLSPreferServerCipherSuites = true + } return result } diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index bda9942fb..b57b4fa25 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -15,14 +15,15 @@ func TestTLSConfig_Merge(t *testing.T) { } b := &TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: "test-ca-file-2", - CertFile: "test-cert-file-2", - RPCUpgradeMode: true, - TLSCipherSuites: "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", - TLSMinVersion: "tls12", + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: "test-ca-file-2", + CertFile: "test-cert-file-2", + RPCUpgradeMode: true, + TLSCipherSuites: "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", + TLSMinVersion: "tls12", + TLSPreferServerCipherSuites: true, } new := a.Merge(b) @@ -173,10 +174,12 @@ func TestTLS_Copy(t *testing.T) { fookey = "../../../helper/tlsutil/testdata/nomad-foo-key.pem" ) a := &TLSConfig{ - CAFile: cafile, - CertFile: foocert, - KeyFile: fookey, - TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305", + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305", + TLSMinVersion: "tls12", + TLSPreferServerCipherSuites: true, } a.SetChecksum() diff --git a/website/source/docs/agent/configuration/tls.html.md b/website/source/docs/agent/configuration/tls.html.md index 8979610a7..8ec43a2ab 100644 --- a/website/source/docs/agent/configuration/tls.html.md +++ b/website/source/docs/agent/configuration/tls.html.md @@ -67,6 +67,9 @@ the [Agent's Gossip and RPC Encryption](/docs/agent/encryption.html). - `tls_min_version` - Specifies the minimum supported version of TLS. Accepted values are "tls10", "tls11", "tls12". Defaults to TLS 1.2. +- tls_prefer_server_cipher_suites - This option will cause Nomad to prefer the + server's ciphersuite over the client ciphersuites. + - `verify_https_client` `(bool: false)` - Specifies agents should require client certificates for all incoming HTTPS requests. The client certificates must be signed by the same CA as Nomad. From 3edf3090965ce3052d009816c608f282468bc441 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 21:30:49 -0400 Subject: [PATCH 2/4] fixup! clearify docs and group similar TLS fields --- helper/tlsutil/config.go | 6 +++--- website/source/docs/agent/configuration/tls.html.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index dfd1ba6ab..07c1e997b 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -105,14 +105,14 @@ type Config struct { // these values for acceptable safe alternatives. CipherSuites []uint16 - // MinVersion contains the minimum SSL/TLS version that is accepted. - MinVersion uint16 - // PreferServerCipherSuites controls whether the server selects the // client's most preferred ciphersuite, or the server's most preferred // ciphersuite. If true then the server's preference, as expressed in // the order of elements in CipherSuites, is used. PreferServerCipherSuites bool + + // MinVersion contains the minimum SSL/TLS version that is accepted. + MinVersion uint16 } func NewTLSConfiguration(newConf *config.TLSConfig, verifyIncoming, verifyOutgoing bool) (*Config, error) { diff --git a/website/source/docs/agent/configuration/tls.html.md b/website/source/docs/agent/configuration/tls.html.md index 8ec43a2ab..07deb14f3 100644 --- a/website/source/docs/agent/configuration/tls.html.md +++ b/website/source/docs/agent/configuration/tls.html.md @@ -67,8 +67,8 @@ the [Agent's Gossip and RPC Encryption](/docs/agent/encryption.html). - `tls_min_version` - Specifies the minimum supported version of TLS. Accepted values are "tls10", "tls11", "tls12". Defaults to TLS 1.2. -- tls_prefer_server_cipher_suites - This option will cause Nomad to prefer the - server's ciphersuite over the client ciphersuites. +- `tls_prefer_server_cipher_suites` - Specifies whether TLS connections should + prefer the server's ciphersuites over the client's. Defaults to false. - `verify_https_client` `(bool: false)` - Specifies agents should require client certificates for all incoming HTTPS requests. The client certificates From b7880933f59b7b8640fa379f9f5853632c041a9f Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 21:35:42 -0400 Subject: [PATCH 3/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 812e2bee3..8a06b68c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ IMPROVEMENTS: * core: Updated serf library to improve how leave intents are handled [[GH-4278](https://github.com/hashicorp/nomad/issues/4278)] + * core: Added TLS configuration option to prefer server's ciphersuites over clients[[GH-4338](https://github.com/hashicorp/nomad/issues/4338)] * core: Add the option for operators to configure TLS versions and allowed cipher suites. Default is a subset of safe ciphers and TLS 1.2 [[GH-4269](https://github.com/hashicorp/nomad/pull/4269)] * core: Add a new [progress_deadline](https://www.nomadproject.io/docs/job-specification/update.html#progress_deadline) parameter to From c7d3dc476e0ecb7bc83110f5240fbe6c632d1599 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 30 May 2018 10:23:41 -0700 Subject: [PATCH 4/4] Fix docs for defaults --- .../source/docs/agent/configuration/tls.html.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/website/source/docs/agent/configuration/tls.html.md b/website/source/docs/agent/configuration/tls.html.md index 07deb14f3..a5f049bea 100644 --- a/website/source/docs/agent/configuration/tls.html.md +++ b/website/source/docs/agent/configuration/tls.html.md @@ -58,17 +58,18 @@ the [Agent's Gossip and RPC Encryption](/docs/agent/encryption.html). cluster is being upgraded to TLS, and removed after the migration is complete. This allows the agent to accept both TLS and plaintext traffic. -- `tls_cipher_suites` - Specifies the TLS cipher suites that will be used by - the agent. Known insecure ciphers are disabled (3DES and RC4). By default, - an agent is configured to use TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, +- `tls_cipher_suites` `(array: [])` - Specifies the TLS cipher suites + that will be used by the agent. Known insecure ciphers are disabled (3DES and + RC4). By default, an agent is configured to use + TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, and TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384. -- `tls_min_version` - Specifies the minimum supported version of TLS. Accepted - values are "tls10", "tls11", "tls12". Defaults to TLS 1.2. +- `tls_min_version` `(string: "tls12")`- Specifies the minimum supported version + of TLS. Accepted values are "tls10", "tls11", "tls12". -- `tls_prefer_server_cipher_suites` - Specifies whether TLS connections should - prefer the server's ciphersuites over the client's. Defaults to false. +- `tls_prefer_server_cipher_suites` `(bool: false)` - Specifies whether + TLS connections should prefer the server's ciphersuites over the client's. - `verify_https_client` `(bool: false)` - Specifies agents should require client certificates for all incoming HTTPS requests. The client certificates