From d4152c77eabaf5205f00b9021388cea97bf52b52 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 25 May 2018 14:47:19 -0400 Subject: [PATCH 1/5] parse CA certificate to catch more specific errors --- helper/tlsutil/config.go | 13 ++++++++- helper/tlsutil/config_test.go | 50 ++++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 8bd6ff2e41c..9fa0227368b 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -3,6 +3,7 @@ package tlsutil import ( "crypto/tls" "crypto/x509" + "encoding/pem" "fmt" "io/ioutil" "net" @@ -146,8 +147,18 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { return fmt.Errorf("Failed to read CA file: %v", err) } + block, _ := pem.Decode([]byte(data)) + if block == nil { + return fmt.Errorf("Failed to decode CA file from pem format") + } + + // Parse the certificate to ensure that it is properly formatted + if _, err := x509.ParseCertificates(block.Bytes); err != nil { + return fmt.Errorf("Failed to parse CA file: %v", err) + } + if !pool.AppendCertsFromPEM(data) { - return fmt.Errorf("Failed to parse any CA certificates") + return fmt.Errorf("Failed to add any CA certificates") } return nil diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 2f38054e995..f74d5bd6571 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net" + "os" "strings" "testing" @@ -26,14 +27,55 @@ const ( ) func TestConfig_AppendCA_None(t *testing.T) { + require := require.New(t) + conf := &Config{} pool := x509.NewCertPool() err := conf.AppendCA(pool) - if err != nil { - t.Fatalf("err: %v", err) + + require.Nil(err) +} + +func TestConfig_AppendCA_Valid(t *testing.T) { + require := require.New(t) + + conf := &Config{ + CAFile: cacert, + } + pool := x509.NewCertPool() + err := conf.AppendCA(pool) + + require.Nil(err) +} + +func TestConfig_AppendCA_Invalid(t *testing.T) { + require := require.New(t) + { + conf := &Config{ + CAFile: "invalidFile", + } + pool := x509.NewCertPool() + err := conf.AppendCA(pool) + require.NotNil(err) + require.Contains(err.Error(), "Failed to read CA file") + require.Equal(len(pool.Subjects()), 0) } - if len(pool.Subjects()) != 0 { - t.Fatalf("bad: %v", pool.Subjects()) + + { + tmpFile, err := ioutil.TempFile("/tmp", "test_ca_file") + require.Nil(err) + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.Write([]byte("Invalid CA Content!")) + require.Nil(err) + + conf := &Config{ + CAFile: tmpFile.Name(), + } + pool := x509.NewCertPool() + err = conf.AppendCA(pool) + require.NotNil(err) + require.Contains(err.Error(), "Failed to decode CA file from pem format") + require.Equal(len(pool.Subjects()), 0) } } From a8ff38f7900aee0016b9d2f822f49852c1753553 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 17:07:38 -0400 Subject: [PATCH 2/5] remove unnecessary type conversation --- 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 9fa0227368b..8db29e0889f 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -147,7 +147,7 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { return fmt.Errorf("Failed to read CA file: %v", err) } - block, _ := pem.Decode([]byte(data)) + block, _ := pem.Decode(data) if block == nil { return fmt.Errorf("Failed to decode CA file from pem format") } From 5ae88d9f0ce289aa2b7466e4b8c9742c34ba19cc Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 18:25:43 -0400 Subject: [PATCH 3/5] handle parsing multiple certificates in a pem file --- helper/tlsutil/config.go | 23 ++++++++- helper/tlsutil/config_test.go | 87 +++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 8db29e0889f..851316cd42e 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -147,7 +147,7 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { return fmt.Errorf("Failed to read CA file: %v", err) } - block, _ := pem.Decode(data) + block, rest := pem.Decode(data) if block == nil { return fmt.Errorf("Failed to decode CA file from pem format") } @@ -161,6 +161,27 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { return fmt.Errorf("Failed to add any CA certificates") } + for len(rest) > 0 { + block, rest = pem.Decode(rest) + + if block == nil { + return fmt.Errorf("Failed to decode CA file from pem format") + } + + // Parse the certificate to ensure that it is properly formatted + if _, err := x509.ParseCertificates(block.Bytes); err != nil { + return fmt.Errorf("Failed to parse CA file: %v", err) + } + + if !pool.AppendCertsFromPEM(data) { + return fmt.Errorf("Failed to add any CA certificates") + } + + if len(rest) == 0 { + break + } + } + return nil } diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index f74d5bd6571..f5eb1611fc8 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -48,6 +48,93 @@ func TestConfig_AppendCA_Valid(t *testing.T) { require.Nil(err) } +func TestConfig_AppendCA_Valid_MultipleCerts(t *testing.T) { + require := require.New(t) + + tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file") + require.Nil(err) + defer os.Remove(tmpCAFile.Name()) + + certs := ` +-----BEGIN CERTIFICATE----- +MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw +eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh +biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx +GDAWBgNVBAMTD25vbWFkLmhhc2hpY29ycDAeFw0xNjExMTAxOTQ4MDBaFw0yMTEx +MDkxOTQ4MDBaMHgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYw +FAYDVQQHEw1TYW4gRnJhbmNpc2NvMRIwEAYDVQQKEwlIYXNoaUNvcnAxDjAMBgNV +BAsTBU5vbWFkMRgwFgYDVQQDEw9ub21hZC5oYXNoaWNvcnAwWTATBgcqhkjOPQIB +BggqhkjOPQMBBwNCAARfJmTdHzYIMPD8SK+kj5Gc79fmpOcg6wnb4JNVwCqWw9O+ +uNdZJZWSi4Q/4HojM5FTSBqYxNgSrmY/o3oQrCPlo0IwQDAOBgNVHQ8BAf8EBAMC +AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm +/UAwCgYIKoZIzj0EAwIDRwAwRAIgTemDJGSGtcQPXLWKiQNw4SKO9wAPhn/WoKW4 +Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw +eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh +biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx +GDAWBgNVBAMTD25vbWFkLmhhc2hpY29ycDAeFw0xNjExMTAxOTQ4MDBaFw0yMTEx +MDkxOTQ4MDBaMHgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYw +FAYDVQQHEw1TYW4gRnJhbmNpc2NvMRIwEAYDVQQKEwlIYXNoaUNvcnAxDjAMBgNV +BAsTBU5vbWFkMRgwFgYDVQQDEw9ub21hZC5oYXNoaWNvcnAwWTATBgcqhkjOPQIB +BggqhkjOPQMBBwNCAARfJmTdHzYIMPD8SK+kj5Gc79fmpOcg6wnb4JNVwCqWw9O+ +uNdZJZWSi4Q/4HojM5FTSBqYxNgSrmY/o3oQrCPlo0IwQDAOBgNVHQ8BAf8EBAMC +AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm +/UAwCgYIKoZIzj0EAwIDRwAwRAIgTemDJGSGtcQPXLWKiQNw4SKO9wAPhn/WoKW4 +Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 +-----END CERTIFICATE-----` + + _, err = tmpCAFile.Write([]byte(certs)) + require.Nil(err) + + conf := &Config{ + CAFile: tmpCAFile.Name(), + } + pool := x509.NewCertPool() + err = conf.AppendCA(pool) + + require.Nil(err) +} + +func TestConfig_AppendCA_InValid_MultipleCerts(t *testing.T) { + require := require.New(t) + + tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file") + require.Nil(err) + defer os.Remove(tmpCAFile.Name()) + + certs := ` +-----BEGIN CERTIFICATE----- +MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw +eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh +biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx +GDAWBgNVBAMTD25vbWFkLmhhc2hpY29ycDAeFw0xNjExMTAxOTQ4MDBaFw0yMTEx +MDkxOTQ4MDBaMHgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYw +FAYDVQQHEw1TYW4gRnJhbmNpc2NvMRIwEAYDVQQKEwlIYXNoaUNvcnAxDjAMBgNV +BAsTBU5vbWFkMRgwFgYDVQQDEw9ub21hZC5oYXNoaWNvcnAwWTATBgcqhkjOPQIB +BggqhkjOPQMBBwNCAARfJmTdHzYIMPD8SK+kj5Gc79fmpOcg6wnb4JNVwCqWw9O+ +uNdZJZWSi4Q/4HojM5FTSBqYxNgSrmY/o3oQrCPlo0IwQDAOBgNVHQ8BAf8EBAMC +AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm +/UAwCgYIKoZIzj0EAwIDRwAwRAIgTemDJGSGtcQPXLWKiQNw4SKO9wAPhn/WoKW4 +Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +Invalid +-----END CERTIFICATE-----` + + _, err = tmpCAFile.Write([]byte(certs)) + require.Nil(err) + + conf := &Config{ + CAFile: tmpCAFile.Name(), + } + pool := x509.NewCertPool() + err = conf.AppendCA(pool) + + require.NotNil(err) +} + func TestConfig_AppendCA_Invalid(t *testing.T) { require := require.New(t) { From ac8afe0edb2da612466aad937693d6e915b0d045 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 18:32:13 -0400 Subject: [PATCH 4/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 812e2bee385..17ba5e06973 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: Add more descriptive errors when parsing agent TLS certificates [[GH-4340](https://github.com/hashicorp/nomad/issues/4340)] * 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 8e2da4e0488dce482baa599fa1a5629d9bd671c7 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 29 May 2018 18:44:30 -0400 Subject: [PATCH 5/5] refactor to remove duplication --- helper/tlsutil/config.go | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 851316cd42e..cb440f579ba 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -148,38 +148,34 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { } block, rest := pem.Decode(data) - if block == nil { - return fmt.Errorf("Failed to decode CA file from pem format") + if err := validateCertificate(block); err != nil { + return err } - // Parse the certificate to ensure that it is properly formatted - if _, err := x509.ParseCertificates(block.Bytes); err != nil { - return fmt.Errorf("Failed to parse CA file: %v", err) + for len(rest) > 0 { + block, rest = pem.Decode(rest) + if err := validateCertificate(block); err != nil { + return err + } } if !pool.AppendCertsFromPEM(data) { return fmt.Errorf("Failed to add any CA certificates") } - for len(rest) > 0 { - block, rest = pem.Decode(rest) - - if block == nil { - return fmt.Errorf("Failed to decode CA file from pem format") - } - - // Parse the certificate to ensure that it is properly formatted - if _, err := x509.ParseCertificates(block.Bytes); err != nil { - return fmt.Errorf("Failed to parse CA file: %v", err) - } + return nil +} - if !pool.AppendCertsFromPEM(data) { - return fmt.Errorf("Failed to add any CA certificates") - } +// validateCertificate checks to ensure a certificate is valid. If it is not, +// return a descriptive error of why the certificate is invalid. +func validateCertificate(block *pem.Block) error { + if block == nil { + return fmt.Errorf("Failed to decode CA file from pem format") + } - if len(rest) == 0 { - break - } + // Parse the certificate to ensure that it is properly formatted + if _, err := x509.ParseCertificates(block.Bytes); err != nil { + return fmt.Errorf("Failed to parse CA file: %v", err) } return nil