Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for tls PreferServerCipherSuites #4338

Merged
merged 4 commits into from
May 30, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions command/agent/config-test-fixtures/basic.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down
1 change: 1 addition & 0 deletions command/agent/config_parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
19 changes: 11 additions & 8 deletions command/agent/config_parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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": "*",
Expand Down
43 changes: 26 additions & 17 deletions helper/tlsutil/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to be next to cipher suites

// 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) {
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
99 changes: 99 additions & 0 deletions helper/tlsutil/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down
11 changes: 11 additions & 0 deletions nomad/structs/config/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
27 changes: 15 additions & 12 deletions nomad/structs/config/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()

Expand Down
3 changes: 3 additions & 0 deletions website/source/docs/agent/configuration/tls.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put it in code blocks and always start the docs with Specifies.... Here it would be Specifies whether TLS connections should prefer the server's ciphersuite over the client's.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the default value to this, tls_min_version and tls_cipher_suites. For the cipher_suites use: https://www.nomadproject.io/docs/agent/configuration/client.html#servers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tls_min_version and tls_cipher_suites both specify defaults, but if there is a clearer way to indicate this let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See how all other fields use the common format of specifying defaults after the parameter name: https://github.com/hashicorp/nomad/pull/4338/files#diff-3b31a52d9f28cef5fa96fb6f8b149907R57

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.
Expand Down