diff --git a/CHANGELOG.md b/CHANGELOG.md index addd73600a6..9dd17909559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ IMPROVEMENTS: BUG FIXES: + * core: Fix treating valid PEM files as invalid [[GH-4613](https://github.com/hashicorp/nomad/issues/4613)] * core: Reset queued allocation summary to zero when job stopped [[GH-4414](https://github.com/hashicorp/nomad/issues/4414)] * core: Fix panic due to missing synchronization in delayed evaluations heap [[GH-4632](https://github.com/hashicorp/nomad/issues/4632)] * client: Fix migrating ephemeral disks when TLS is enabled [[GH-4648](https://github.com/hashicorp/nomad/issues/4648)] diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 707ed59299c..820ac92f1ba 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -5,7 +5,6 @@ import ( "crypto/rsa" "crypto/tls" "crypto/x509" - "encoding/pem" "fmt" "io/ioutil" "net" @@ -194,35 +193,11 @@ 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 - } - } - + // Read certificates and return an error if no valid certificates were + // found. Unfortunately it is very difficult to return meaningful + // errors as PEM files are extremely permissive. if !pool.AppendCertsFromPEM(data) { - 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 fmt.Errorf("Failed to parse any valid certificates in CA file: %s", c.CAFile) } return nil diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 249b6470e6a..f1837db7bde 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -51,10 +51,6 @@ func TestConfig_AppendCA_Valid(t *testing.T) { 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 @@ -71,6 +67,61 @@ AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 -----END CERTIFICATE----- -----BEGIN CERTIFICATE----- +MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb +MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv +bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT +Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw +EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/ +CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2 +obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK +KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w +HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB +iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x +sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl +TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0= +-----END CERTIFICATE----- +` + + tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file") + require.NoError(err) + defer os.Remove(tmpCAFile.Name()) + + _, err = tmpCAFile.Write([]byte(certs)) + require.NoError(err) + tmpCAFile.Close() + + conf := &Config{ + CAFile: tmpCAFile.Name(), + } + pool := x509.NewCertPool() + require.NoError(conf.AppendCA(pool)) + + require.Len(pool.Subjects(), 2) +} + +// TestConfig_AppendCA_Valid_Whitespace asserts that a PEM file containing +// trailing whitespace is valid. +func TestConfig_AppendCA_Valid_Whitespace(t *testing.T) { + require := require.New(t) + + const cacertWhitespace = "./testdata/ca-whitespace.pem" + conf := &Config{ + CAFile: cacertWhitespace, + } + pool := x509.NewCertPool() + require.NoError(conf.AppendCA(pool)) + + require.Len(pool.Subjects(), 1) +} + +// TestConfig_AppendCA_Invalid_MultipleCerts_Whitespace asserts that a PEM file +// containing non-PEM data between certificate blocks is still valid. +func TestConfig_AppendCA_Valid_MultipleCerts_ExtraData(t *testing.T) { + require := require.New(t) + + certs := ` +Did you know... +-----BEGIN CERTIFICATE----- MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx @@ -83,10 +134,34 @@ uNdZJZWSi4Q/4HojM5FTSBqYxNgSrmY/o3oQrCPlo0IwQDAOBgNVHQ8BAf8EBAMC AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm /UAwCgYIKoZIzj0EAwIDRwAwRAIgTemDJGSGtcQPXLWKiQNw4SKO9wAPhn/WoKW4 Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 ------END CERTIFICATE-----` +-----END CERTIFICATE----- + +...PEM parsers don't care about data... + +-----BEGIN CERTIFICATE----- +MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb +MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv +bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT +Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw +EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/ +CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2 +obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK +KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w +HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB +iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x +sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl +TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0= +-----END CERTIFICATE----- + +...outside of -----XXX----- blocks? +` + tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file_extra") + require.NoError(err) + defer os.Remove(tmpCAFile.Name()) _, err = tmpCAFile.Write([]byte(certs)) - require.Nil(err) + require.NoError(err) + tmpCAFile.Close() conf := &Config{ CAFile: tmpCAFile.Name(), @@ -94,16 +169,15 @@ Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 pool := x509.NewCertPool() err = conf.AppendCA(pool) - require.Nil(err) + require.NoError(err) + require.Len(pool.Subjects(), 2) } -func TestConfig_AppendCA_InValid_MultipleCerts(t *testing.T) { +// TestConfig_AppendCA_Invalid_MultipleCerts asserts only the valid certificate +// is returned. +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 @@ -123,16 +197,20 @@ Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4 Invalid -----END CERTIFICATE-----` + tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file") + require.NoError(err) + defer os.Remove(tmpCAFile.Name()) _, err = tmpCAFile.Write([]byte(certs)) - require.Nil(err) + require.NoError(err) + tmpCAFile.Close() conf := &Config{ CAFile: tmpCAFile.Name(), } pool := x509.NewCertPool() - err = conf.AppendCA(pool) + require.NoError(conf.AppendCA(pool)) - require.NotNil(err) + require.Len(pool.Subjects(), 1) } func TestConfig_AppendCA_Invalid(t *testing.T) { @@ -160,8 +238,8 @@ func TestConfig_AppendCA_Invalid(t *testing.T) { } pool := x509.NewCertPool() err = conf.AppendCA(pool) - require.NotNil(err) - require.Contains(err.Error(), "Failed to decode CA file from pem format") + require.Error(err) + require.Contains(err.Error(), "Failed to parse any valid certificates in CA file:") require.Equal(len(pool.Subjects()), 0) } } diff --git a/helper/tlsutil/testdata/ca-whitespace.pem b/helper/tlsutil/testdata/ca-whitespace.pem new file mode 100644 index 00000000000..a8a7a189143 --- /dev/null +++ b/helper/tlsutil/testdata/ca-whitespace.pem @@ -0,0 +1,15 @@ +-----BEGIN CERTIFICATE----- +MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb +MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv +bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT +Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw +EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/ +CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2 +obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK +KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w +HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB +iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x +sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl +TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0= +-----END CERTIFICATE----- +