Skip to content

Commit

Permalink
tls: remove deprecated prefer_server_cipher_suites field
Browse files Browse the repository at this point in the history
The TLS configuration object includes a deprecated `prefer_server_cipher_suites`
field. In version of Go prior to 1.17, this property controlled whether a TLS
connection would use the cipher suites preferred by the server or by the
client. This field is ignored as of 1.17 and, according to the `crypto/tls`
docs: "Servers now select the best mutually supported cipher suite based on
logic that takes into account inferred client hardware, server hardware, and
security."

This property has been long-deprecated and leaving it in place may lead to false
assumptions about how cipher suites are negotiated in connection to a server. So
we want to remove it in Nomad 1.9.0.

Fixes: hashicorp/nomad-enterprise#999
Ref: https://hashicorp.atlassian.net/browse/NET-10531
  • Loading branch information
tgross committed Jul 31, 2024
1 parent b1212ba commit 931dd81
Show file tree
Hide file tree
Showing 9 changed files with 53 additions and 125 deletions.
3 changes: 3 additions & 0 deletions .changelog/23712.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:breaking-change
tls: Removed deprecated `tls.prefer_server_cipher_suites` field from agent configuration
```
21 changes: 10 additions & 11 deletions command/agent/config_parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,17 +282,16 @@ var basicConfig = &Config{
},
}},
TLSConfig: &config.TLSConfig{
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",
EnableHTTP: true,
EnableRPC: true,
VerifyServerHostname: true,
CAFile: "foo",
CertFile: "bar",
KeyFile: "pipe",
RPCUpgradeMode: true,
VerifyHTTPSClient: true,
TLSCipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
TLSMinVersion: "tls12",
},
HTTPAPIResponseHeaders: map[string]string{
"Access-Control-Allow-Origin": "*",
Expand Down
21 changes: 10 additions & 11 deletions command/agent/testdata/basic.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -290,17 +290,16 @@ vault {
}

tls {
http = true
rpc = true
verify_server_hostname = true
ca_file = "foo"
cert_file = "bar"
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"
http = true
rpc = true
verify_server_hostname = true
ca_file = "foo"
cert_file = "bar"
key_file = "pipe"
rpc_upgrade_mode = true
verify_https_client = true
tls_cipher_suites = "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"
tls_min_version = "tls12"
}

sentinel {
Expand Down
1 change: 0 additions & 1 deletion command/agent/testdata/basic.json
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,6 @@
"rpc_upgrade_mode": true,
"tls_cipher_suites": "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
"tls_min_version": "tls12",
"tls_prefer_server_cipher_suites": true,
"verify_https_client": true,
"verify_server_hostname": true
}
Expand Down
43 changes: 17 additions & 26 deletions helper/tlsutil/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,6 @@ type Config struct {
// these values for acceptable safe alternatives.
CipherSuites []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
}
Expand All @@ -164,16 +158,15 @@ 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,
PreferServerCipherSuites: newConf.TLSPreferServerCipherSuites,
VerifyIncoming: verifyIncoming,
VerifyOutgoing: verifyOutgoing,
VerifyServerHostname: newConf.VerifyServerHostname,
CAFile: newConf.CAFile,
CertFile: newConf.CertFile,
KeyFile: newConf.KeyFile,
KeyLoader: newConf.GetKeyLoader(),
CipherSuites: ciphers,
MinVersion: minVersion,
}, nil
}

Expand Down Expand Up @@ -231,11 +224,10 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) {
}
// Create the tlsConfig
tlsConfig := &tls.Config{
RootCAs: x509.NewCertPool(),
InsecureSkipVerify: true,
CipherSuites: c.CipherSuites,
MinVersion: c.MinVersion,
PreferServerCipherSuites: c.PreferServerCipherSuites,
RootCAs: x509.NewCertPool(),
InsecureSkipVerify: true,
CipherSuites: c.CipherSuites,
MinVersion: c.MinVersion,
}
if c.VerifyServerHostname {
tlsConfig.InsecureSkipVerify = false
Expand Down Expand Up @@ -349,11 +341,10 @@ 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,
PreferServerCipherSuites: c.PreferServerCipherSuites,
ClientCAs: x509.NewCertPool(),
ClientAuth: tls.NoClientCert,
CipherSuites: c.CipherSuites,
MinVersion: c.MinVersion,
}

// Parse the CA cert if any
Expand Down
47 changes: 0 additions & 47 deletions helper/tlsutil/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,32 +409,6 @@ func TestConfig_OutgoingTLS_WithKeyPair(t *testing.T) {
assert.NotNil(cert)
}

func TestConfig_OutgoingTLS_PreferServerCipherSuites(t *testing.T) {
ci.Parallel(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) {
ci.Parallel(t)

Expand Down Expand Up @@ -731,27 +705,6 @@ func TestConfig_IncomingTLS_NoVerify(t *testing.T) {
}
}

func TestConfig_IncomingTLS_PreferServerCipherSuites(t *testing.T) {
ci.Parallel(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) {
ci.Parallel(t)

Expand Down
11 changes: 0 additions & 11 deletions nomad/structs/config/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,6 @@ type TLSConfig struct {
// connections. Should be either "tls10", "tls11", or "tls12".
TLSMinVersion string `hcl:"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 `hcl:"tls_prefer_server_cipher_suites"`

// ExtraKeysHCL is used by hcl to surface unexpected keys
ExtraKeysHCL []string `hcl:",unusedKeys" json:"-"`
}
Expand Down Expand Up @@ -170,8 +164,6 @@ func (t *TLSConfig) Copy() *TLSConfig {
new.TLSCipherSuites = t.TLSCipherSuites
new.TLSMinVersion = t.TLSMinVersion

new.TLSPreferServerCipherSuites = t.TLSPreferServerCipherSuites

new.SetChecksum()

return new
Expand Down Expand Up @@ -225,9 +217,6 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig {
if b.TLSMinVersion != "" {
result.TLSMinVersion = b.TLSMinVersion
}
if b.TLSPreferServerCipherSuites {
result.TLSPreferServerCipherSuites = true
}
return result
}

Expand Down
28 changes: 13 additions & 15 deletions nomad/structs/config/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,14 @@ 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",
TLSPreferServerCipherSuites: true,
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",
}

new := a.Merge(b)
Expand Down Expand Up @@ -188,12 +187,11 @@ func TestTLS_Copy(t *testing.T) {
fookey = "../../../helper/tlsutil/testdata/regionFoo-client-nomad-key.pem"
)
a := &TLSConfig{
CAFile: cafile,
CertFile: foocert,
KeyFile: fookey,
TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305",
TLSMinVersion: "tls12",
TLSPreferServerCipherSuites: true,
CAFile: cafile,
CertFile: foocert,
KeyFile: fookey,
TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305",
TLSMinVersion: "tls12",
}
a.SetChecksum()

Expand Down
3 changes: 0 additions & 3 deletions website/content/docs/configuration/tls.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@ the [Enable TLS Encryption for Nomad Tutorial](/nomad/tutorials/transport-securi
- `tls_min_version` `(string: "tls12")`- Specifies the minimum supported version
of TLS. Accepted values are "tls10", "tls11", "tls12".

- `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, effectively upgrading
[`tls.http=true`](#http) to mTLS. The client certificates must be signed by
Expand Down

0 comments on commit 931dd81

Please sign in to comment.