Merge pull request #4340 from hashicorp/f-tls-parse-certs

Parse CA certificate to catch more specific errors

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)]

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

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,
+	}
+	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(),
 	}
-	if len(pool.Subjects()) != 0 {
-		t.Fatalf("bad: %v", pool.Subjects())
+	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)
 	}
 }