From e9898f2d0d726e6d59f2caee159551ec6f107ea5 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 23 Aug 2018 16:57:42 -0700 Subject: [PATCH] config: accept CA PEM files with extra whitespace Previously we did a validation pass over CA PEM files before calling Go's CertPool.AppendCertsFromPEM to provide more detailed error messages than the stdlib provides. Unfortunately our validation was overly strict and rejected valid CA files. This is actually the reason the stdlib PEM parser doesn't return meaningful errors: PEM files are extremely permissive and it's difficult to tell the difference between invalid data and valid metadata. This PR removes our custom validation as it would reject valid data and the extra error messages were not useful in diagnosing the error encountered. --- helper/tlsutil/config.go | 33 ++-------- helper/tlsutil/config_test.go | 112 ++++++++++++++++++++++++++++------ 2 files changed, 99 insertions(+), 46 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 707ed59299c5..820ac92f1ba9 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 992af302b9b9..428c19d93973 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) } }