From bfaf4dcb2b289e342ab0b5650d9dc959f7f67c38 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 6 Aug 2018 13:54:57 -0400 Subject: [PATCH 01/13] change function signature to take entire tls config object --- command/agent/config_parse.go | 2 +- helper/tlsutil/config.go | 8 +++--- helper/tlsutil/config_test.go | 50 ++++++++++++++++++++--------------- 3 files changed, 33 insertions(+), 27 deletions(-) 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/helper/tlsutil/config.go b/helper/tlsutil/config.go index 016ecbb1c..1d5813546 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -125,7 +125,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,17 +385,17 @@ 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 if cipherStr == "" { ciphers = defaultTLSCiphers } else { - ciphers = strings.Split(cipherStr, ",") + ciphers = strings.Split(tlsConfig.TLSCipherSuites, ",") } for _, cipher := range ciphers { c, ok := supportedTLSCiphers[cipher] diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 8b408d763..61585acdf 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -647,25 +647,27 @@ func TestConfig_IncomingTLS_TLSCipherSuites(t *testing.T) { 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{ + 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,7 +689,7 @@ 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) } @@ -708,7 +710,8 @@ func TestConfig_ParseCiphers_Default(t *testing.T) { tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, } - parsedCiphers, err := ParseCiphers("") + empty := &config.TLSConfig{} + parsedCiphers, err := ParseCiphers(empty) require.Nil(err) require.Equal(parsedCiphers, expectedCiphers) } @@ -722,7 +725,10 @@ func TestConfig_ParseCiphers_Invalid(t *testing.T) { } for _, cipher := range invalidCiphers { - parsedCiphers, err := ParseCiphers(cipher) + tlsConfig := &config.TLSConfig{ + TLSCipherSuites: cipher, + } + parsedCiphers, err := ParseCiphers(tlsConfig) require.NotNil(err) require.Equal(fmt.Sprintf("unsupported TLS cipher %q", cipher), err.Error()) require.Equal(0, len(parsedCiphers)) From bc01b401fc4e09a4c04ec801ca7c044acf526e4a Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 7 Aug 2018 14:14:40 -0400 Subject: [PATCH 02/13] add functionality to check if signature algorithm is supported in cipher suites --- helper/tlsutil/config.go | 55 +++++++++++++++++++++++++++++++++++ helper/tlsutil/config_test.go | 53 +++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 1d5813546..913738e9e 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -41,6 +41,61 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } +var rsa string = "RSA" +var ecdsa string = "ECDSA" + +var supportedCipherSignatures = map[string]string{ + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": rsa, + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": ecdsa, + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256": rsa, + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256": ecdsa, + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384": rsa, + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384": ecdsa, + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256": rsa, + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA": rsa, + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256": ecdsa, + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA": ecdsa, + "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA": rsa, + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA": ecdsa, + "TLS_RSA_WITH_AES_128_GCM_SHA256": rsa, + "TLS_RSA_WITH_AES_256_GCM_SHA384": rsa, + "TLS_RSA_WITH_AES_128_CBC_SHA256": rsa, + "TLS_RSA_WITH_AES_128_CBC_SHA": rsa, + "TLS_RSA_WITH_AES_256_CBC_SHA": rsa, +} + +var signatureAlgorithmMapping = map[x509.SignatureAlgorithm]string{ + x509.MD2WithRSA: rsa, + x509.MD5WithRSA: rsa, + x509.SHA1WithRSA: rsa, + x509.SHA256WithRSA: rsa, + x509.SHA384WithRSA: rsa, + x509.SHA512WithRSA: rsa, + x509.ECDSAWithSHA1: ecdsa, + x509.ECDSAWithSHA256: ecdsa, + x509.ECDSAWithSHA384: ecdsa, + x509.ECDSAWithSHA512: ecdsa, + x509.SHA256WithRSAPSS: rsa, + x509.SHA384WithRSAPSS: rsa, + x509.SHA512WithRSAPSS: rsa, +} + +func cipherSuitesSupportSignatureAlgorithm(supportedSignature x509.SignatureAlgorithm, supportedCipherSuites []string) (bool, error) { + supportedSignatureAlgorithm, ok := signatureAlgorithmMapping[supportedSignature] + if !ok { + return false, fmt.Errorf("Unsupported signature scheme: %s", supportedSignature.String()) + } + + for _, cipher := range supportedCipherSuites { + cipherSupportedAlg := supportedCipherSignatures[cipher] + if supportedSignatureAlgorithm == cipherSupportedAlg { + return true, nil + } + } + + return false, fmt.Errorf("Specified cipher suites don't support %s, consider adding more cipher suites with this signature algorithm.", supportedSignatureAlgorithm) +} + // defaultTLSCiphers are the TLS Ciphers that are supported by default var defaultTLSCiphers = []string{ "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 61585acdf..5f5dde16b 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -875,3 +875,56 @@ func TestConfig_ShouldReloadRPCConnections(t *testing.T) { require.Equal(shouldReload, testCase.shouldReload, testCase.errorStr) } } + +func TestConfig_ShouldDetectUnsupportedSignatureAlgorithms(t *testing.T) { + require := require.New(t) + + type individualTestCase struct { + supportedSignature x509.SignatureAlgorithm + supportedCiphers []string + isSupported bool + description string + } + + testCases := []*individualTestCase{ + { + supportedSignature: x509.SHA256WithRSA, + supportedCiphers: []string{ + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + }, + isSupported: true, + description: "Should be supported", + }, + { + supportedSignature: x509.SHA256WithRSA, + supportedCiphers: []string{ + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + }, + isSupported: false, + description: "Should not be supported", + }, + { + supportedSignature: x509.SHA256WithRSA, + supportedCiphers: []string{ + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + }, + isSupported: false, + description: "Multiple options without a match should not be supported", + }, + { + supportedSignature: x509.MD2WithRSA, + supportedCiphers: []string{ + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + }, + isSupported: false, + description: "Unsupported signature should not find a match", + }, + } + + for _, testCase := range testCases { + isSupported, _ := cipherSuitesSupportSignatureAlgorithm(testCase.supportedSignature, testCase.supportedCiphers) + require.Equal(testCase.isSupported, isSupported, testCase.description) + } +} From 92fc1ce4708c845cf2b599ef2691181921aac6ac Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 08:58:32 -0400 Subject: [PATCH 03/13] refactor to use golang built in api for certs --- helper/tlsutil/config.go | 121 ++++++++++++++++++---------------- helper/tlsutil/config_test.go | 110 +++++++++++++++---------------- 2 files changed, 119 insertions(+), 112 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 913738e9e..f8abd895a 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,59 +43,27 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } -var rsa string = "RSA" -var ecdsa string = "ECDSA" +var rsaStringRepr string = "RSA" +var ecdsaStringRepr string = "ECDSA" var supportedCipherSignatures = map[string]string{ - "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": rsa, - "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": ecdsa, - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256": rsa, - "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256": ecdsa, - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384": rsa, - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384": ecdsa, - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256": rsa, - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA": rsa, - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256": ecdsa, - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA": ecdsa, - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA": rsa, - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA": ecdsa, - "TLS_RSA_WITH_AES_128_GCM_SHA256": rsa, - "TLS_RSA_WITH_AES_256_GCM_SHA384": rsa, - "TLS_RSA_WITH_AES_128_CBC_SHA256": rsa, - "TLS_RSA_WITH_AES_128_CBC_SHA": rsa, - "TLS_RSA_WITH_AES_256_CBC_SHA": rsa, -} - -var signatureAlgorithmMapping = map[x509.SignatureAlgorithm]string{ - x509.MD2WithRSA: rsa, - x509.MD5WithRSA: rsa, - x509.SHA1WithRSA: rsa, - x509.SHA256WithRSA: rsa, - x509.SHA384WithRSA: rsa, - x509.SHA512WithRSA: rsa, - x509.ECDSAWithSHA1: ecdsa, - x509.ECDSAWithSHA256: ecdsa, - x509.ECDSAWithSHA384: ecdsa, - x509.ECDSAWithSHA512: ecdsa, - x509.SHA256WithRSAPSS: rsa, - x509.SHA384WithRSAPSS: rsa, - x509.SHA512WithRSAPSS: rsa, -} - -func cipherSuitesSupportSignatureAlgorithm(supportedSignature x509.SignatureAlgorithm, supportedCipherSuites []string) (bool, error) { - supportedSignatureAlgorithm, ok := signatureAlgorithmMapping[supportedSignature] - if !ok { - return false, fmt.Errorf("Unsupported signature scheme: %s", supportedSignature.String()) - } - - for _, cipher := range supportedCipherSuites { - cipherSupportedAlg := supportedCipherSignatures[cipher] - if supportedSignatureAlgorithm == cipherSupportedAlg { - return true, nil - } - } - - return false, fmt.Errorf("Specified cipher suites don't support %s, consider adding more cipher suites with this signature algorithm.", supportedSignatureAlgorithm) + "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 @@ -445,14 +415,14 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { cipherStr := strings.TrimSpace(tlsConfig.TLSCipherSuites) - var ciphers []string + var parsedCiphers []string if cipherStr == "" { - ciphers = defaultTLSCiphers + parsedCiphers = defaultTLSCiphers } else { - ciphers = strings.Split(tlsConfig.TLSCipherSuites, ",") + 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) @@ -460,7 +430,46 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { suites = append(suites, c) } - return suites, nil + var supportedSignatureAlgorithm string + + // 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. + if tlsConfig.KeyLoader != nil { + // Ensure that the keypair has been loaded before continuing + tlsConfig.KeyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) + + if tlsConfig.KeyLoader.GetCertificate() != nil { + tlsCert := tlsConfig.KeyLoader.GetCertificate() + if tlsCert != nil { + // Determine what type of signature algorithm is being used by typecasting + // the certificate's private key + privKey := tlsCert.PrivateKey + switch privKey.(type) { + case *rsa.PrivateKey: + supportedSignatureAlgorithm = rsaStringRepr + case *ecdsa.PrivateKey: + supportedSignatureAlgorithm = ecdsaStringRepr + default: + return []uint16{}, fmt.Errorf("Unsupported Signature Algorithm; RSA and ECDSA only are supported.") + } + } + } + } + + 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 %s, consider adding more cipher suites with this signature algorithm.", supportedSignatureAlgorithm) } // 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 5f5dde16b..992af302b 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -644,10 +644,15 @@ 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) 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", @@ -694,6 +699,8 @@ func TestConfig_ParseCiphers_Valid(t *testing.T) { 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) @@ -710,12 +717,18 @@ func TestConfig_ParseCiphers_Default(t *testing.T) { tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, } - empty := &config.TLSConfig{} + 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) @@ -727,6 +740,9 @@ func TestConfig_ParseCiphers_Invalid(t *testing.T) { for _, cipher := range invalidCiphers { tlsConfig := &config.TLSConfig{ TLSCipherSuites: cipher, + CertFile: foocert, + KeyFile: fookey, + KeyLoader: &config.KeyLoader{}, } parsedCiphers, err := ParseCiphers(tlsConfig) require.NotNil(err) @@ -735,6 +751,38 @@ func TestConfig_ParseCiphers_Invalid(t *testing.T) { } } +// 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) @@ -775,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) @@ -784,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) @@ -875,56 +926,3 @@ func TestConfig_ShouldReloadRPCConnections(t *testing.T) { require.Equal(shouldReload, testCase.shouldReload, testCase.errorStr) } } - -func TestConfig_ShouldDetectUnsupportedSignatureAlgorithms(t *testing.T) { - require := require.New(t) - - type individualTestCase struct { - supportedSignature x509.SignatureAlgorithm - supportedCiphers []string - isSupported bool - description string - } - - testCases := []*individualTestCase{ - { - supportedSignature: x509.SHA256WithRSA, - supportedCiphers: []string{ - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - }, - isSupported: true, - description: "Should be supported", - }, - { - supportedSignature: x509.SHA256WithRSA, - supportedCiphers: []string{ - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - }, - isSupported: false, - description: "Should not be supported", - }, - { - supportedSignature: x509.SHA256WithRSA, - supportedCiphers: []string{ - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - }, - isSupported: false, - description: "Multiple options without a match should not be supported", - }, - { - supportedSignature: x509.MD2WithRSA, - supportedCiphers: []string{ - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - }, - isSupported: false, - description: "Unsupported signature should not find a match", - }, - } - - for _, testCase := range testCases { - isSupported, _ := cipherSuitesSupportSignatureAlgorithm(testCase.supportedSignature, testCase.supportedCiphers) - require.Equal(testCase.isSupported, isSupported, testCase.description) - } -} From 781b9c640d2760e241d216b530b48a2f5ccd94b8 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 09:55:11 -0400 Subject: [PATCH 04/13] add simple getter for certificate --- helper/tlsutil/config.go | 54 ++++++++++++++++++------------------- nomad/structs/config/tls.go | 6 +++++ 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index f8abd895a..f89fb41c2 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -430,46 +430,46 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { suites = append(suites, c) } - var supportedSignatureAlgorithm string - // 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. - if tlsConfig.KeyLoader != nil { - // Ensure that the keypair has been loaded before continuing - tlsConfig.KeyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) + keyLoader := tlsConfig.GetKeyLoader() - if tlsConfig.KeyLoader.GetCertificate() != nil { - tlsCert := tlsConfig.KeyLoader.GetCertificate() - if tlsCert != nil { - // Determine what type of signature algorithm is being used by typecasting - // the certificate's private key - privKey := tlsCert.PrivateKey - switch privKey.(type) { - case *rsa.PrivateKey: - supportedSignatureAlgorithm = rsaStringRepr - case *ecdsa.PrivateKey: - supportedSignatureAlgorithm = ecdsaStringRepr - default: - return []uint16{}, fmt.Errorf("Unsupported Signature Algorithm; RSA and ECDSA only are supported.") + // Ensure that the keypair has been loaded before continuing + keyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) + + if keyLoader.GetCertificate() != nil { + var supportedSignatureAlgorithm string + + tlsCert := keyLoader.GetCertificate() + if tlsCert != nil { + // Determine what type of signature algorithm is being used by typecasting + // the certificate's private key + privKey := tlsCert.PrivateKey + switch privKey.(type) { + case *rsa.PrivateKey: + supportedSignatureAlgorithm = rsaStringRepr + case *ecdsa.PrivateKey: + supportedSignatureAlgorithm = ecdsaStringRepr + default: + return []uint16{}, fmt.Errorf("Unsupported Signature Algorithm; RSA and ECDSA only are supported.") + } + + 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 } } } } - 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 %s, consider adding more cipher suites with this signature algorithm.", supportedSignatureAlgorithm) + return []uint16{}, fmt.Errorf("Specified cipher suites don't support the certificate signature algorithm, consider adding more cipher suites to match this signature algorithm.") } // ParseMinVersion parses the specified minimum TLS version for the Nomad agent 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. From febf24e71f9cc1d90cbebd1066b761f8240a6645 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 13:50:46 -0400 Subject: [PATCH 05/13] type safety for string keys --- helper/tlsutil/config.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index f89fb41c2..8d24bc0f3 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -43,10 +43,12 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } -var rsaStringRepr string = "RSA" -var ecdsaStringRepr string = "ECDSA" +type algorithmStringRepr string -var supportedCipherSignatures = map[string]string{ +var rsaStringRepr algorithmStringRepr = "RSA" +var ecdsaStringRepr algorithmStringRepr = "ECDSA" + +var supportedCipherSignatures = map[string]algorithmStringRepr{ "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": rsaStringRepr, "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": ecdsaStringRepr, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256": rsaStringRepr, @@ -440,7 +442,7 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { keyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) if keyLoader.GetCertificate() != nil { - var supportedSignatureAlgorithm string + var supportedSignatureAlgorithm algorithmStringRepr tlsCert := keyLoader.GetCertificate() if tlsCert != nil { From 1a1effd2aa3cedf1cd8d989cd69cc95dd165893a Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 13:58:28 -0400 Subject: [PATCH 06/13] add comments --- helper/tlsutil/config.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 8d24bc0f3..b8e043cbd 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -43,11 +43,14 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } +// algorithmStringRepr is the string representation of a signing algorithm type algorithmStringRepr string var rsaStringRepr algorithmStringRepr = "RSA" var ecdsaStringRepr algorithmStringRepr = "ECDSA" +// supportedCipherSignatures is the supported cipher suites with their +// corresponding signature algorithm var supportedCipherSignatures = map[string]algorithmStringRepr{ "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": rsaStringRepr, "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": ecdsaStringRepr, From 5bb7d9d5705f28d45c6b1697a516cb7523e14e16 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 16:59:15 -0400 Subject: [PATCH 07/13] add default case for empty TLS structs --- helper/tlsutil/config.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index b8e043cbd..ce783656a 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -458,7 +458,7 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { case *ecdsa.PrivateKey: supportedSignatureAlgorithm = ecdsaStringRepr default: - return []uint16{}, fmt.Errorf("Unsupported Signature Algorithm; RSA and ECDSA only are supported.") + return []uint16{}, fmt.Errorf("Unsupported signature algorithm %T; RSA and ECDSA only are supported.", privKey) } for _, cipher := range parsedCiphers { @@ -469,12 +469,16 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { } } } + + // 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, consider adding more cipher suites to match this signature algorithm.") } - // 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, consider adding more cipher suites to match this signature algorithm.") + // 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 } // ParseMinVersion parses the specified minimum TLS version for the Nomad agent From 4fe562ca8c7071848abbfc9fe9774c5c0da243a2 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 8 Aug 2018 18:31:37 -0400 Subject: [PATCH 08/13] remove redundant nil check --- helper/tlsutil/config.go | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index ce783656a..ece665fe7 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -448,25 +448,24 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { var supportedSignatureAlgorithm algorithmStringRepr tlsCert := keyLoader.GetCertificate() - if tlsCert != nil { - // Determine what type of signature algorithm is being used by typecasting - // the certificate's private key - privKey := tlsCert.PrivateKey - switch privKey.(type) { - case *rsa.PrivateKey: - supportedSignatureAlgorithm = rsaStringRepr - case *ecdsa.PrivateKey: - supportedSignatureAlgorithm = ecdsaStringRepr - default: - return []uint16{}, fmt.Errorf("Unsupported signature algorithm %T; RSA and ECDSA only are supported.", privKey) - } - 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 - } + // Determine what type of signature algorithm is being used by typecasting + // the certificate's private key + privKey := tlsCert.PrivateKey + switch privKey.(type) { + case *rsa.PrivateKey: + supportedSignatureAlgorithm = rsaStringRepr + case *ecdsa.PrivateKey: + supportedSignatureAlgorithm = ecdsaStringRepr + default: + return []uint16{}, fmt.Errorf("Unsupported signature algorithm %T; RSA and ECDSA only are supported.", privKey) + } + + 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 } } From 4f1d40926e0bc1c0cbb09009cb33382b38678aa7 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 10 Aug 2018 12:20:04 -0400 Subject: [PATCH 09/13] change string repr of signature algorithms to constants --- helper/tlsutil/config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index ece665fe7..4aad22db5 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -46,8 +46,10 @@ var supportedTLSCiphers = map[string]uint16{ // algorithmStringRepr is the string representation of a signing algorithm type algorithmStringRepr string -var rsaStringRepr algorithmStringRepr = "RSA" -var ecdsaStringRepr algorithmStringRepr = "ECDSA" +const ( + rsaStringRepr algorithmStringRepr = "RSA" + ecdsaStringRepr algorithmStringRepr = "ECDSA" +) // supportedCipherSignatures is the supported cipher suites with their // corresponding signature algorithm From 011eced69d7404f721cd79ef416b2bdb96e2ddfc Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 13 Aug 2018 16:08:23 -0400 Subject: [PATCH 10/13] extract functionality for determining signature algorithm per code review feedback --- helper/tlsutil/config.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 4aad22db5..9d6eb8153 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -447,20 +447,9 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { keyLoader.LoadKeyPair(tlsConfig.CertFile, tlsConfig.KeyFile) if keyLoader.GetCertificate() != nil { - var supportedSignatureAlgorithm algorithmStringRepr - - tlsCert := keyLoader.GetCertificate() - - // Determine what type of signature algorithm is being used by typecasting - // the certificate's private key - privKey := tlsCert.PrivateKey - switch privKey.(type) { - case *rsa.PrivateKey: - supportedSignatureAlgorithm = rsaStringRepr - case *ecdsa.PrivateKey: - supportedSignatureAlgorithm = ecdsaStringRepr - default: - return []uint16{}, fmt.Errorf("Unsupported signature algorithm %T; RSA and ECDSA only are supported.", privKey) + supportedSignatureAlgorithm, err := getSignatureAlgorithm(keyLoader.GetCertificate()) + if err != nil { + return []uint16{}, err } for _, cipher := range parsedCiphers { @@ -482,6 +471,22 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { 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) (algorithmStringRepr, 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 func ParseMinVersion(version string) (uint16, error) { if version == "" { From eb3cead2bcdfc8716c233b68e75a8b4b16bccd84 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 13 Aug 2018 16:11:49 -0400 Subject: [PATCH 11/13] rename signature algorithm type per code review feedback --- helper/tlsutil/config.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 9d6eb8153..5d9b6da89 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -43,17 +43,17 @@ var supportedTLSCiphers = map[string]uint16{ "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, } -// algorithmStringRepr is the string representation of a signing algorithm -type algorithmStringRepr string +// signatureAlgorithm is the string representation of a signing algorithm +type signatureAlgorithm string const ( - rsaStringRepr algorithmStringRepr = "RSA" - ecdsaStringRepr algorithmStringRepr = "ECDSA" + rsaStringRepr signatureAlgorithm = "RSA" + ecdsaStringRepr signatureAlgorithm = "ECDSA" ) // supportedCipherSignatures is the supported cipher suites with their // corresponding signature algorithm -var supportedCipherSignatures = map[string]algorithmStringRepr{ +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, @@ -475,7 +475,7 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { // 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) (algorithmStringRepr, error) { +func getSignatureAlgorithm(tlsCert *tls.Certificate) (signatureAlgorithm, error) { privKey := tlsCert.PrivateKey switch privKey.(type) { case *rsa.PrivateKey: From 067eef565a9ab3a24991e39c75479a6c8600abd2 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 13 Aug 2018 16:21:18 -0400 Subject: [PATCH 12/13] add signature algorithm to error message --- helper/tlsutil/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 5d9b6da89..707ed5929 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -463,7 +463,7 @@ func ParseCiphers(tlsConfig *config.TLSConfig) ([]uint16, error) { // 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, consider adding more cipher suites to match this signature algorithm.") + 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 From 0bb13c0f040516e62158979e7c6665e8b678eafb Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 15 Aug 2018 00:59:29 -0400 Subject: [PATCH 13/13] fix up test failure due to keyloader instantiated on tls config during parsing --- command/agent/config_parse_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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 +}