Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

config: accept CA PEM files with extra whitespace #4613

Merged
merged 1 commit into from
Sep 7, 2018
Merged

config: accept CA PEM files with extra whitespace #4613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
33 changes: 4 additions & 29 deletions helper/tlsutil/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"crypto/rsa"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"io/ioutil"
"net"
Expand Down Expand Up @@ -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
Expand Down
112 changes: 95 additions & 17 deletions helper/tlsutil/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -83,27 +134,50 @@ 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(),
}
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
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}
}
Expand Down
15 changes: 15 additions & 0 deletions helper/tlsutil/testdata/ca-whitespace.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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-----