diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a06b68c3..d6054829e 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: Added TLS configuration option to prefer server's ciphersuites over clients[[GH-4338](https://github.com/hashicorp/nomad/issues/4338)] * 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)] diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 07c1e997b..91d2cfb66 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" @@ -153,8 +154,35 @@ func (c *Config) AppendCA(pool *x509.CertPool) error { return fmt.Errorf("Failed to read CA file: %v", err) } + block, rest := pem.Decode(data) + if err := validateCertificate(block); err != nil { + return 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 parse any CA certificates") + return fmt.Errorf("Failed to add any CA certificates") + } + + return nil +} + +// 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") + } + + // 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 diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 9d71272cb..f8eca41aa 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,142 @@ 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, } - if len(pool.Subjects()) != 0 { - t.Fatalf("bad: %v", pool.Subjects()) + pool := x509.NewCertPool() + err := conf.AppendCA(pool) + + 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) + { + 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) + } + + { + 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) } }