Skip to content

Commit 556adad

Browse files
schmichaelschmichael
committed
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.
1 parent 6bd5852 commit 556adad

File tree

4 files changed

+115
-46
lines changed

4 files changed

+115
-46
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ IMPROVEMENTS:
1515

1616

1717
BUG FIXES:
18+
* core: Fix treating valid PEM files as invalid [[GH-4613](https://github.com/hashicorp/nomad/issues/4613)]
1819
* core: Reset queued allocation summary to zero when job stopped [[GH-4414](https://github.com/hashicorp/nomad/issues/4414)]
1920
* core: Fix panic due to missing synchronization in delayed evaluations heap [[GH-4632](https://github.com/hashicorp/nomad/issues/4632)]
2021
* client: Fix migrating ephemeral disks when TLS is enabled [[GH-4648](https://github.com/hashicorp/nomad/issues/4648)]

‎helper/tlsutil/config.go

+4-29
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"crypto/rsa"
66
"crypto/tls"
77
"crypto/x509"
8-
"encoding/pem"
98
"fmt"
109
"io/ioutil"
1110
"net"
@@ -194,35 +193,11 @@ func (c *Config) AppendCA(pool *x509.CertPool) error {
194193
return fmt.Errorf("Failed to read CA file: %v", err)
195194
}
196195

197-
block, rest := pem.Decode(data)
198-
if err := validateCertificate(block); err != nil {
199-
return err
200-
}
201-
202-
for len(rest) > 0 {
203-
block, rest = pem.Decode(rest)
204-
if err := validateCertificate(block); err != nil {
205-
return err
206-
}
207-
}
208-
196+
// Read certificates and return an error if no valid certificates were
197+
// found. Unfortunately it is very difficult to return meaningful
198+
// errors as PEM files are extremely permissive.
209199
if !pool.AppendCertsFromPEM(data) {
210-
return fmt.Errorf("Failed to add any CA certificates")
211-
}
212-
213-
return nil
214-
}
215-
216-
// validateCertificate checks to ensure a certificate is valid. If it is not,
217-
// return a descriptive error of why the certificate is invalid.
218-
func validateCertificate(block *pem.Block) error {
219-
if block == nil {
220-
return fmt.Errorf("Failed to decode CA file from pem format")
221-
}
222-
223-
// Parse the certificate to ensure that it is properly formatted
224-
if _, err := x509.ParseCertificates(block.Bytes); err != nil {
225-
return fmt.Errorf("Failed to parse CA file: %v", err)
200+
return fmt.Errorf("Failed to parse any valid certificates in CA file: %s", c.CAFile)
226201
}
227202

228203
return nil

‎helper/tlsutil/config_test.go

+95-17
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,6 @@ func TestConfig_AppendCA_Valid(t *testing.T) {
5151
func TestConfig_AppendCA_Valid_MultipleCerts(t *testing.T) {
5252
require := require.New(t)
5353

54-
tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file")
55-
require.Nil(err)
56-
defer os.Remove(tmpCAFile.Name())
57-
5854
certs := `
5955
-----BEGIN CERTIFICATE-----
6056
MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw
@@ -71,6 +67,61 @@ AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm
7167
Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4
7268
-----END CERTIFICATE-----
7369
-----BEGIN CERTIFICATE-----
70+
MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb
71+
MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv
72+
bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT
73+
Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw
74+
EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/
75+
CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2
76+
obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK
77+
KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w
78+
HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB
79+
iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x
80+
sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl
81+
TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0=
82+
-----END CERTIFICATE-----
83+
`
84+
85+
tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file")
86+
require.NoError(err)
87+
defer os.Remove(tmpCAFile.Name())
88+
89+
_, err = tmpCAFile.Write([]byte(certs))
90+
require.NoError(err)
91+
tmpCAFile.Close()
92+
93+
conf := &Config{
94+
CAFile: tmpCAFile.Name(),
95+
}
96+
pool := x509.NewCertPool()
97+
require.NoError(conf.AppendCA(pool))
98+
99+
require.Len(pool.Subjects(), 2)
100+
}
101+
102+
// TestConfig_AppendCA_Valid_Whitespace asserts that a PEM file containing
103+
// trailing whitespace is valid.
104+
func TestConfig_AppendCA_Valid_Whitespace(t *testing.T) {
105+
require := require.New(t)
106+
107+
const cacertWhitespace = "./testdata/ca-whitespace.pem"
108+
conf := &Config{
109+
CAFile: cacertWhitespace,
110+
}
111+
pool := x509.NewCertPool()
112+
require.NoError(conf.AppendCA(pool))
113+
114+
require.Len(pool.Subjects(), 1)
115+
}
116+
117+
// TestConfig_AppendCA_Invalid_MultipleCerts_Whitespace asserts that a PEM file
118+
// containing non-PEM data between certificate blocks is still valid.
119+
func TestConfig_AppendCA_Valid_MultipleCerts_ExtraData(t *testing.T) {
120+
require := require.New(t)
121+
122+
certs := `
123+
Did you know...
124+
-----BEGIN CERTIFICATE-----
74125
MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw
75126
eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh
76127
biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx
@@ -83,27 +134,50 @@ uNdZJZWSi4Q/4HojM5FTSBqYxNgSrmY/o3oQrCPlo0IwQDAOBgNVHQ8BAf8EBAMC
83134
AQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOjVq/BectnhcKn6EHUD4NJFm
84135
/UAwCgYIKoZIzj0EAwIDRwAwRAIgTemDJGSGtcQPXLWKiQNw4SKO9wAPhn/WoKW4
85136
Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4
86-
-----END CERTIFICATE-----`
137+
-----END CERTIFICATE-----
138+
139+
...PEM parsers don't care about data...
140+
141+
-----BEGIN CERTIFICATE-----
142+
MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb
143+
MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv
144+
bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT
145+
Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw
146+
EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/
147+
CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2
148+
obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK
149+
KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w
150+
HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB
151+
iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x
152+
sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl
153+
TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0=
154+
-----END CERTIFICATE-----
155+
156+
...outside of -----XXX----- blocks?
157+
`
87158

159+
tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file_extra")
160+
require.NoError(err)
161+
defer os.Remove(tmpCAFile.Name())
88162
_, err = tmpCAFile.Write([]byte(certs))
89-
require.Nil(err)
163+
require.NoError(err)
164+
tmpCAFile.Close()
90165

91166
conf := &Config{
92167
CAFile: tmpCAFile.Name(),
93168
}
94169
pool := x509.NewCertPool()
95170
err = conf.AppendCA(pool)
96171

97-
require.Nil(err)
172+
require.NoError(err)
173+
require.Len(pool.Subjects(), 2)
98174
}
99175

100-
func TestConfig_AppendCA_InValid_MultipleCerts(t *testing.T) {
176+
// TestConfig_AppendCA_Invalid_MultipleCerts asserts only the valid certificate
177+
// is returned.
178+
func TestConfig_AppendCA_Invalid_MultipleCerts(t *testing.T) {
101179
require := require.New(t)
102180

103-
tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file")
104-
require.Nil(err)
105-
defer os.Remove(tmpCAFile.Name())
106-
107181
certs := `
108182
-----BEGIN CERTIFICATE-----
109183
MIICMzCCAdqgAwIBAgIUNZ9L86Xp9EuDH0/qyAesh599LXQwCgYIKoZIzj0EAwIw
@@ -123,16 +197,20 @@ Ln2ZUe8CIDsQswBQS7URbqnKYDye2Y4befJkr4fmhhmMQb2ex9A4
123197
Invalid
124198
-----END CERTIFICATE-----`
125199

200+
tmpCAFile, err := ioutil.TempFile("/tmp", "test_ca_file")
201+
require.NoError(err)
202+
defer os.Remove(tmpCAFile.Name())
126203
_, err = tmpCAFile.Write([]byte(certs))
127-
require.Nil(err)
204+
require.NoError(err)
205+
tmpCAFile.Close()
128206

129207
conf := &Config{
130208
CAFile: tmpCAFile.Name(),
131209
}
132210
pool := x509.NewCertPool()
133-
err = conf.AppendCA(pool)
211+
require.NoError(conf.AppendCA(pool))
134212

135-
require.NotNil(err)
213+
require.Len(pool.Subjects(), 1)
136214
}
137215

138216
func TestConfig_AppendCA_Invalid(t *testing.T) {
@@ -160,8 +238,8 @@ func TestConfig_AppendCA_Invalid(t *testing.T) {
160238
}
161239
pool := x509.NewCertPool()
162240
err = conf.AppendCA(pool)
163-
require.NotNil(err)
164-
require.Contains(err.Error(), "Failed to decode CA file from pem format")
241+
require.Error(err)
242+
require.Contains(err.Error(), "Failed to parse any valid certificates in CA file:")
165243
require.Equal(len(pool.Subjects()), 0)
166244
}
167245
}

‎helper/tlsutil/testdata/ca-whitespace.pem

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIICNTCCAZagAwIBAgIRANjgoh5iVZI26+Hz/K65G0UwCgYIKoZIzj0EAwQwNjEb
3+
MBkGA1UEChMSSGFzaGlDb3JwIFRyYWluaW5nMRcwFQYDVQQDEw5zZXJ2aWNlLmNv
4+
bnN1bDAeFw0xODA4MjMxNzM0NTBaFw0xODA5MjIxNzM0NTBaMDYxGzAZBgNVBAoT
5+
Ekhhc2hpQ29ycCBUcmFpbmluZzEXMBUGA1UEAxMOc2VydmljZS5jb25zdWwwgZsw
6+
EAYHKoZIzj0CAQYFK4EEACMDgYYABAGjC4sWsOfirS/DQ9/e7PdQeJwlOjziiOx/
7+
CALjS6ryEDkZPqRqMuoFXfudAmfdk6tl8AT1IKMVcgiQU5jkm7fliwFIk48uh+n2
8+
obqZjwDyM76VYBVSYi6i3BPXown1ivIMJNQS1txnWZLZHsv+WxbHydS+GNOAwKDK
9+
KsXj9dEhd36pvaNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8w
10+
HQYDVR0OBBYEFIk3oG2hu0FxueW4e7fL+FdMOquBMAoGCCqGSM49BAMEA4GMADCB
11+
iAJCAPIPwPyk+8Ymj7Zlvb5qIUQg+UxoacAeJtFZrJ8xQjro0YjsM33O86rAfw+x
12+
sWWGul4Ews93KFBXvhbKCwb0F0PhAkIAh2z7COsKcQzvBoIy+Kx92+9j/sUjlzzl
13+
TttDu+g2VdbcBwVDZ49X2Md6OY2N3G8Irdlj+n+mCQJaHwVt52DRzz0=
14+
-----END CERTIFICATE-----
15+

0 commit comments

Comments
 (0)