diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index 66f704f6c..28a189df5 100644 --- a/command/agent/config_parse.go +++ b/command/agent/config_parse.go @@ -844,7 +844,7 @@ func parseTLSConfig(result **config.TLSConfig, list *ast.ObjectList) error { return err } - if _, err := tlsutil.ParseCiphers(tlsConfig.TLSCipherSuites); err != nil { + if _, err := tlsutil.ParseCiphers(&tlsConfig); err != nil { return err } diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index 7bf4bfa95..43638befd 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -286,7 +286,19 @@ func TestConfig_Parse(t *testing.T) { if (err != nil) != tc.Err { t.Fatalf("file: %s\n\n%s", tc.File, err) } - require.EqualValues(actual, tc.Result) + + //panic(fmt.Sprintf("first: %+v \n second: %+v", actual.TLSConfig, tc.Result.TLSConfig)) + require.EqualValues(removeHelperAttributes(actual), tc.Result) }) } } + +// In order to compare the Config struct after parsing, and from generating what +// is expected in the test, we need to remove helper attributes that are +// instantiated in the process of parsing the configuration +func removeHelperAttributes(c *Config) *Config { + if c.TLSConfig != nil { + c.TLSConfig.KeyLoader = nil + } + return c +} diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 016ecbb1c..707ed5929 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -1,6 +1,8 @@ package tlsutil import ( + "crypto/ecdsa" + "crypto/rsa" "crypto/tls" "crypto/x509" "encoding/pem" @@ -41,6 +43,36 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } +// signatureAlgorithm is the string representation of a signing algorithm +type signatureAlgorithm string + +const ( + rsaStringRepr signatureAlgorithm = "RSA" + ecdsaStringRepr signatureAlgorithm = "ECDSA" +) + +// supportedCipherSignatures is the supported cipher suites with their +// corresponding signature algorithm +var supportedCipherSignatures = map[string]signatureAlgorithm{ + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": rsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": ecdsaStringRepr, + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256": rsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256": ecdsaStringRepr, + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384": rsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384": ecdsaStringRepr, + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256": rsaStringRepr, + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA": rsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256": ecdsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA": ecdsaStringRepr, + "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA": rsaStringRepr, + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA": ecdsaStringRepr, + "TLS_RSA_WITH_AES_128_GCM_SHA256": rsaStringRepr, + "TLS_RSA_WITH_AES_256_GCM_SHA384": rsaStringRepr, + "TLS_RSA_WITH_AES_128_CBC_SHA256": rsaStringRepr, + "TLS_RSA_WITH_AES_128_CBC_SHA": rsaStringRepr, + "TLS_RSA_WITH_AES_256_CBC_SHA": rsaStringRepr, +} + // defaultTLSCiphers are the TLS Ciphers that are supported by default var defaultTLSCiphers = []string{ "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", @@ -125,7 +157,7 @@ type Config struct { } func NewTLSConfiguration(newConf *config.TLSConfig, verifyIncoming, verifyOutgoing bool) (*Config, error) { - ciphers, err := ParseCiphers(newConf.TLSCipherSuites) + ciphers, err := ParseCiphers(newConf) if err != nil { return nil, err } @@ -385,19 +417,19 @@ func (c *Config) IncomingTLSConfig() (*tls.Config, error) { // ParseCiphers parses ciphersuites from the comma-separated string into // recognized slice -func ParseCiphers(cipherStr string) ([]uint16, error) { +func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { suites := []uint16{} - cipherStr = strings.TrimSpace(cipherStr) + cipherStr := strings.TrimSpace(tlsConfig.TLSCipherSuites) - var ciphers []string + var parsedCiphers []string if cipherStr == "" { - ciphers = defaultTLSCiphers + parsedCiphers = defaultTLSCiphers } else { - ciphers = strings.Split(cipherStr, ",") + parsedCiphers = strings.Split(tlsConfig.TLSCipherSuites, ",") } - for _, cipher := range ciphers { + for _, cipher := range parsedCiphers { c, ok := supportedTLSCiphers[cipher] if !ok { return suites, fmt.Errorf("unsupported TLS cipher %q", cipher) @@ -405,7 +437,54 @@ func ParseCiphers(cipherStr string) ([]uint16, error) { suites = append(suites, c) } - return suites, nil + // Ensure that the specified cipher suite list is supported by the TLS + // Certificate signature algorithm. This is a check for user error, where a + // TLS certificate could support RSA but a user has configured a cipher suite + // list of ciphers where only ECDSA is supported. + keyLoader := tlsConfig.GetKeyLoader() + + // Ensure that the keypair has been loaded before continuing + keyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) + + if keyLoader.GetCertificate() != nil { + supportedSignatureAlgorithm, err := getSignatureAlgorithm(keyLoader.GetCertificate()) + if err != nil { + return []uint16{}, err + } + + for _, cipher := range parsedCiphers { + if supportedCipherSignatures[cipher] == supportedSignatureAlgorithm { + // Positive case, return the matched cipher suites as the signature + // algorithm is also supported + return suites, nil + } + } + + // Negative case, if this is reached it means that none of the specified + // cipher suites signature algorithms match the signature algorithm + // for the certificate. + return []uint16{}, fmt.Errorf("Specified cipher suites don't support the certificate signature algorithm %s, consider adding more cipher suites to match this signature algorithm.", supportedSignatureAlgorithm) + } + + // Default in case this function is called but TLS is not actually configured + // This is only reached if the TLS certificate is nil + return []uint16{}, nil +} + +// getSignatureAlgorithm returns the signature algorithm for a TLS certificate +// This is determined by examining the type of the certificate's public key, +// as Golang doesn't expose a more straightforward API which returns this +// type +func getSignatureAlgorithm(tlsCert *tls.Certificate) (signatureAlgorithm, error) { + privKey := tlsCert.PrivateKey + switch privKey.(type) { + case *rsa.PrivateKey: + return rsaStringRepr, nil + case *ecdsa.PrivateKey: + return ecdsaStringRepr, nil + default: + return "", fmt.Errorf("Unsupported signature algorithm %T; RSA and ECDSA only are supported.", privKey) + } } // ParseMinVersion parses the specified minimum TLS version for the Nomad agent diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 8b408d763..992af302b 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -644,28 +644,35 @@ func TestConfig_IncomingTLS_TLSCipherSuites(t *testing.T) { } } +// This test relies on the fact that the specified certificate has an ECDSA +// signature algorithm func TestConfig_ParseCiphers_Valid(t *testing.T) { require := require.New(t) - validCiphers := strings.Join([]string{ - "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305", - "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305", - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", - "TLS_RSA_WITH_AES_128_GCM_SHA256", - "TLS_RSA_WITH_AES_256_GCM_SHA384", - "TLS_RSA_WITH_AES_128_CBC_SHA256", - "TLS_RSA_WITH_AES_128_CBC_SHA", - "TLS_RSA_WITH_AES_256_CBC_SHA", - }, ",") + tlsConfig := &config.TLSConfig{ + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, + TLSCipherSuites: strings.Join([]string{ + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305", + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305", + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", + "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", + "TLS_RSA_WITH_AES_128_GCM_SHA256", + "TLS_RSA_WITH_AES_256_GCM_SHA384", + "TLS_RSA_WITH_AES_128_CBC_SHA256", + "TLS_RSA_WITH_AES_128_CBC_SHA", + "TLS_RSA_WITH_AES_256_CBC_SHA", + }, ","), + } expectedCiphers := []uint16{ tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, @@ -687,11 +694,13 @@ func TestConfig_ParseCiphers_Valid(t *testing.T) { tls.TLS_RSA_WITH_AES_256_CBC_SHA, } - parsedCiphers, err := ParseCiphers(validCiphers) + parsedCiphers, err := ParseCiphers(tlsConfig) require.Nil(err) require.Equal(parsedCiphers, expectedCiphers) } +// This test relies on the fact that the specified certificate has an ECDSA +// signature algorithm func TestConfig_ParseCiphers_Default(t *testing.T) { require := require.New(t) @@ -708,11 +717,18 @@ func TestConfig_ParseCiphers_Default(t *testing.T) { tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, } - parsedCiphers, err := ParseCiphers("") + empty := &config.TLSConfig{ + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, + } + parsedCiphers, err := ParseCiphers(empty) require.Nil(err) require.Equal(parsedCiphers, expectedCiphers) } +// This test relies on the fact that the specified certificate has an ECDSA +// signature algorithm func TestConfig_ParseCiphers_Invalid(t *testing.T) { require := require.New(t) @@ -722,13 +738,51 @@ func TestConfig_ParseCiphers_Invalid(t *testing.T) { } for _, cipher := range invalidCiphers { - parsedCiphers, err := ParseCiphers(cipher) + tlsConfig := &config.TLSConfig{ + TLSCipherSuites: cipher, + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, + } + parsedCiphers, err := ParseCiphers(tlsConfig) require.NotNil(err) require.Equal(fmt.Sprintf("unsupported TLS cipher %q", cipher), err.Error()) require.Equal(0, len(parsedCiphers)) } } +// This test relies on the fact that the specified certificate has an ECDSA +// signature algorithm +func TestConfig_ParseCiphers_SupportedSignature(t *testing.T) { + require := require.New(t) + + // Supported signature + { + tlsConfig := &config.TLSConfig{ + TLSCipherSuites: "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305", + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, + } + parsedCiphers, err := ParseCiphers(tlsConfig) + require.Nil(err) + require.Equal(1, len(parsedCiphers)) + } + + // Unsupported signature + { + tlsConfig := &config.TLSConfig{ + TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305", + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, + } + parsedCiphers, err := ParseCiphers(tlsConfig) + require.NotNil(err) + require.Equal(0, len(parsedCiphers)) + } +} + func TestConfig_ParseMinVersion_Valid(t *testing.T) { require := require.New(t) @@ -769,7 +823,10 @@ func TestConfig_NewTLSConfiguration(t *testing.T) { require := require.New(t) conf := &config.TLSConfig{ - TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + TLSCipherSuites: "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, } tlsConf, err := NewTLSConfiguration(conf, true, true) @@ -778,7 +835,7 @@ func TestConfig_NewTLSConfiguration(t *testing.T) { require.True(tlsConf.VerifyOutgoing) expectedCiphers := []uint16{ - tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, } require.Equal(tlsConf.CipherSuites, expectedCiphers) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index 4476c78b6..875a6e20b 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -97,6 +97,12 @@ func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, err return k.certificate, nil } +func (k *KeyLoader) GetCertificate() *tls.Certificate { + k.cacheLock.Lock() + defer k.cacheLock.Unlock() + return k.certificate +} + // GetOutgoingCertificate fetches the currently-loaded certificate when // accepting a TLS connection. This currently does not consider information in // the ClientHello and only returns the certificate that was last loaded.