Format certificates properly (rfc7468) with a trailing new line (#10411)

* trim carriage return from certificates when inserting rootCA in the inMemDB * format rootCA properly when returning the CA on the connect CA endpoint * Fix linter warnings * Fix providers to trim certs before returning it * trim newlines on write when possible * add changelog * make sure all provider return a trailing newline after the root and intermediate certs * Fix endpoint to return trailing new line * Fix failing test with vault provider * make test more robust * make sure all provider return a trailing newline after the leaf certs * Check for suffix before removing newline and use function * Add comment to consul provider * Update change log Co-authored-by: R.B. Boyer <[email protected]> * fix typo * simplify code callflow Co-authored-by: R.B. Boyer <[email protected]> * extract requireNewLine as shared func * remove dependency to testify in testing file * remove extra newline in vault provider * Add cert newline fix to envoy xds * remove new line from mock provider * Remove adding a new line from provider and fix it when the cert is read * Add a comment to explain the fix * Add missing for leaf certs * fix missing new line * fix missing new line in leaf certs * remove extra new line in test * updage changelog Co-authored-by: Daniel Nephin <[email protected]> * fix in vault provider and when reading cache (RPC call) * fix AWS provider * fix failing test in the provider * remove comments and empty lines * add check for empty cert in test * fix linter warnings * add new line for leaf and private key * use string concat instead of Sprintf * fix new lines for leaf signing * preallocate slice and remove append * Add new line to `SignIntermediate` and `CrossSignCA` Co-authored-by: R.B. Boyer <[email protected]> Co-authored-by: Daniel Nephin <[email protected]>
hashicorp · Jul 6, 2021 · 506d93a · 506d93a
1 parent 8ffa9ce
commit 506d93a
Show file tree

Hide file tree

Showing 13 changed files with 74 additions and 48 deletions.
diff --git a/.changelog/10411.txt b/.changelog/10411.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ca: Fixed a bug that returned a malformed certificate chain when the certificate did not having a trailing newline.
+```
diff --git a/agent/connect/ca/common.go b/agent/connect/ca/common.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/x509"
 	"fmt"
+	"strings"
 
 	"github.com/hashicorp/consul/agent/connect"
 )
@@ -89,3 +90,15 @@ func validateSignIntermediate(csr *x509.CertificateRequest, spiffeID *connect.Sp
 	}
 	return nil
 }
+
+// EnsureTrailingNewline this is used to fix a case where the provider do not return a new line after
+// the certificate as per the specification see GH-8178 for more context
+func EnsureTrailingNewline(cert string) string {
+	if cert == "" {
+		return cert
+	}
+	if strings.HasSuffix(cert, "\n") {
+		return cert
+	}
+	return fmt.Sprintf("%s\n", cert)
+}
diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go
@@ -83,6 +83,7 @@ func testSignAndValidate(t *testing.T, p Provider, rootPEM string, intermediateP
 
 	err = connect.ValidateLeaf(rootPEM, leafPEM, intermediatePEMs)
 	require.NoError(t, err)
+	requireTrailingNewline(t, leafPEM)
 }
 
 func TestAWSBootstrapAndSignSecondary(t *testing.T) {

diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go
@@ -145,7 +145,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 
 				cert, err := provider.Sign(csr)
 				require.NoError(err)
-
+				requireTrailingNewline(t, cert)
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
 				require.Equal(spiffeService.URI(), parsed.URIs[0])

diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go
@@ -365,7 +365,7 @@ func (v *VaultProvider) getCA(path string) (string, error) {
 		return "", err
 	}
 
-	root := string(bytes)
+	root := EnsureTrailingNewline(string(bytes))
 	if root == "" {
 		return "", ErrBackendNotInitialized
 	}
@@ -437,7 +437,7 @@ func (v *VaultProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 		return "", fmt.Errorf("issuing_ca was not a string")
 	}
 
-	return fmt.Sprintf("%s\n%s", cert, ca), nil
+	return EnsureTrailingNewline(cert) + EnsureTrailingNewline(ca), nil
 }
 
 // SignIntermediate returns a signed CA certificate with a path length constraint
@@ -474,7 +474,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 		return "", fmt.Errorf("signed intermediate result is not a string")
 	}
 
-	return intermediate, nil
+	return EnsureTrailingNewline(intermediate), nil
 }
 
 // CrossSignCA takes a CA certificate and cross-signs it to form a trust chain
@@ -514,7 +514,7 @@ func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
 		return "", fmt.Errorf("certificate was not a string")
 	}
 
-	return xcCert, nil
+	return EnsureTrailingNewline(xcCert), nil
 }
 
 // SupportsCrossSigning implements Provider

diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go
@@ -36,7 +36,6 @@ func TestVaultCAProvider_VaultTLSConfig(t *testing.T) {
 }
 
 func TestVaultCAProvider_SecondaryActiveIntermediate(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -50,7 +49,7 @@ func TestVaultCAProvider_SecondaryActiveIntermediate(t *testing.T) {
 }
 
 func TestVaultCAProvider_RenewToken(t *testing.T) {
-	t.Parallel()
+
 	SkipIfVaultNotPresent(t)
 
 	testVault, err := runTestVault(t)
@@ -86,7 +85,6 @@ func TestVaultCAProvider_RenewToken(t *testing.T) {
 }
 
 func TestVaultCAProvider_Bootstrap(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -119,7 +117,7 @@ func TestVaultCAProvider_Bootstrap(t *testing.T) {
 		require.NoError(err)
 		bytes, err := ioutil.ReadAll(resp.Body)
 		require.NoError(err)
-		require.Equal(cert, string(bytes))
+		require.Equal(cert, string(bytes)+"\n")
 
 		// Should be a valid CA cert
 		parsed, err := connect.ParseCert(cert)
@@ -147,7 +145,6 @@ func assertCorrectKeyType(t *testing.T, want, certPEM string) {
 }
 
 func TestVaultCAProvider_SignLeaf(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -200,6 +197,7 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) {
 
 				// Make sure we can validate the cert as expected.
 				require.NoError(connect.ValidateLeaf(rootPEM, cert, []string{intPEM}))
+				requireTrailingNewline(t, cert)
 			}
 
 			// Generate a new cert for another service and make sure
@@ -231,7 +229,6 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) {
 }
 
 func TestVaultCAProvider_CrossSignCA(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -286,7 +283,6 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) {
 }
 
 func TestVaultProvider_SignIntermediate(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -315,7 +311,6 @@ func TestVaultProvider_SignIntermediate(t *testing.T) {
 }
 
 func TestVaultProvider_SignIntermediateConsul(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 
@@ -360,7 +355,6 @@ func TestVaultProvider_SignIntermediateConsul(t *testing.T) {
 }
 
 func TestVaultProvider_Cleanup(t *testing.T) {
-	t.Parallel()
 
 	SkipIfVaultNotPresent(t)
 

diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go
@@ -263,3 +263,12 @@ func ApplyCARequestToStore(store *state.Store, req *structs.CARequest) (interfac
 		return nil, fmt.Errorf("Invalid CA operation '%s'", req.Op)
 	}
 }
+func requireTrailingNewline(t testing.T, leafPEM string) {
+	t.Helper()
+	if len(leafPEM) == 0 {
+		t.Fatalf("cert is empty")
+	}
+	if '\n' != rune(leafPEM[len(leafPEM)-1]) {
+		t.Fatalf("cert do not end with a new line")
+	}
+}
diff --git a/agent/connect_ca_endpoint.go b/agent/connect_ca_endpoint.go
@@ -37,6 +37,7 @@ func (s *HTTPHandlers) ConnectCARoots(resp http.ResponseWriter, req *http.Reques
 
 	// defined in RFC 8555 and registered with the IANA
 	resp.Header().Set("Content-Type", "application/pem-certificate-chain")
+
 	for _, root := range reply.Roots {
 		if _, err := resp.Write([]byte(root.RootCert)); err != nil {
 			return nil, err
@@ -47,7 +48,6 @@ func (s *HTTPHandlers) ConnectCARoots(resp http.ResponseWriter, req *http.Reques
 			}
 		}
 	}
-
 	return nil, nil
 }
 
@@ -58,7 +58,7 @@ func (s *HTTPHandlers) ConnectCAConfiguration(resp http.ResponseWriter, req *htt
 		return s.ConnectCAConfigurationGet(resp, req)
 
 	case "PUT":
-		return s.ConnectCAConfigurationSet(resp, req)
+		return s.ConnectCAConfigurationSet(req)
 
 	default:
 		return nil, MethodNotAllowedError{req.Method, []string{"GET", "POST"}}
@@ -83,7 +83,7 @@ func (s *HTTPHandlers) ConnectCAConfigurationGet(resp http.ResponseWriter, req *
 }
 
 // PUT /v1/connect/ca/configuration
-func (s *HTTPHandlers) ConnectCAConfigurationSet(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+func (s *HTTPHandlers) ConnectCAConfigurationSet(req *http.Request) (interface{}, error) {
 	// Method is tested in ConnectCAConfiguration
 
 	var args structs.CARequest

diff --git a/agent/connect_ca_endpoint_test.go b/agent/connect_ca_endpoint_test.go
@@ -20,22 +20,21 @@ import (
 func TestConnectCARoots_empty(t *testing.T) {
 	t.Parallel()
 
-	require := require.New(t)
 	a := NewTestAgent(t, "connect { enabled = false }")
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
 	req, _ := http.NewRequest("GET", "/v1/connect/ca/roots", nil)
 	resp := httptest.NewRecorder()
 	_, err := a.srv.ConnectCARoots(resp, req)
-	require.Error(err)
-	require.Contains(err.Error(), "Connect must be enabled")
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "Connect must be enabled")
 }
 
 func TestConnectCARoots_list(t *testing.T) {
 	t.Parallel()
 
-	assert := assert.New(t)
+	assertion := assert.New(t)
 	a := NewTestAgent(t, "")
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
@@ -48,16 +47,16 @@ func TestConnectCARoots_list(t *testing.T) {
 	req, _ := http.NewRequest("GET", "/v1/connect/ca/roots", nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.ConnectCARoots(resp, req)
-	assert.NoError(err)
+	assertion.NoError(err)
 
 	value := obj.(structs.IndexedCARoots)
-	assert.Equal(value.ActiveRootID, ca2.ID)
-	assert.Len(value.Roots, 2)
+	assertion.Equal(value.ActiveRootID, ca2.ID)
+	assertion.Len(value.Roots, 2)
 
 	// We should never have the secret information
 	for _, r := range value.Roots {
-		assert.Equal("", r.SigningCert)
-		assert.Equal("", r.SigningKey)
+		assertion.Equal("", r.SigningCert)
+		assertion.Equal("", r.SigningKey)
 	}
 }
 
@@ -213,7 +212,6 @@ func TestConnectCAConfig(t *testing.T) {
 	for _, tc := range tests {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
 			hcl := ""
 			if tc.initialState != "" {
 				hcl = `
@@ -239,23 +237,23 @@ func TestConnectCAConfig(t *testing.T) {
 				resp := httptest.NewRecorder()
 				_, err := a.srv.ConnectCAConfiguration(resp, req)
 				if tc.wantErr {
-					require.Error(err)
+					require.Error(t, err)
 					return
 				}
-				require.NoError(err)
+				require.NoError(t, err)
 			}
 			// The config should be updated now.
 			{
 				req, _ := http.NewRequest("GET", "/v1/connect/ca/configuration", nil)
 				resp := httptest.NewRecorder()
 				obj, err := a.srv.ConnectCAConfiguration(resp, req)
-				require.NoError(err)
+				require.NoError(t, err)
 
 				got := obj.(structs.CAConfiguration)
 				// Reset Raft indexes to make it non flaky
 				got.CreateIndex = 0
 				got.ModifyIndex = 0
-				require.Equal(tc.wantCfg, got)
+				require.Equal(t, tc.wantCfg, got)
 			}
 		})
 	}
@@ -284,9 +282,7 @@ func TestConnectCARoots_PEMEncoding(t *testing.T) {
 
 	data, err := ioutil.ReadAll(resp.Body)
 	require.NoError(t, err)
-
 	pool := x509.NewCertPool()
-
 	require.True(t, pool.AppendCertsFromPEM(data))
 	// expecting the root cert from dc1 and an intermediate in dc2
 	require.Len(t, pool.Subjects(), 2)

diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go
@@ -123,7 +123,9 @@ type mockCAProvider struct {
 func (m *mockCAProvider) Configure(cfg ca.ProviderConfig) error { return nil }
 func (m *mockCAProvider) State() (map[string]string, error)     { return nil, nil }
 func (m *mockCAProvider) GenerateRoot() error                   { return nil }
-func (m *mockCAProvider) ActiveRoot() (string, error)           { return m.rootPEM, nil }
+func (m *mockCAProvider) ActiveRoot() (string, error) {
+	return m.rootPEM, nil
+}
 func (m *mockCAProvider) GenerateIntermediateCSR() (string, error) {
 	m.callbackCh <- "provider/GenerateIntermediateCSR"
 	return "", nil
@@ -132,7 +134,9 @@ func (m *mockCAProvider) SetIntermediate(intermediatePEM, rootPEM string) error
 	m.callbackCh <- "provider/SetIntermediate"
 	return nil
 }
-func (m *mockCAProvider) ActiveIntermediate() (string, error)                       { return m.rootPEM, nil }
+func (m *mockCAProvider) ActiveIntermediate() (string, error) {
+	return m.rootPEM, nil
+}
 func (m *mockCAProvider) GenerateIntermediate() (string, error)                     { return "", nil }
 func (m *mockCAProvider) Sign(*x509.CertificateRequest) (string, error)             { return "", nil }
 func (m *mockCAProvider) SignIntermediate(*x509.CertificateRequest) (string, error) { return "", nil }

diff --git a/agent/consul/server_connect.go b/agent/consul/server_connect.go
@@ -5,7 +5,6 @@ import (
 	"crypto/x509"
 	"fmt"
 	"net/url"
-	"strings"
 	"sync"
 
 	"github.com/hashicorp/consul/agent/connect"
@@ -104,6 +103,13 @@ func (s *Server) getCARoots(ws memdb.WatchSet, state *state.Store) (*structs.Ind
 	// data such as key material stored within the struct. So here we
 	// pull out some of the fields and copy them into
 	for i, r := range indexedRoots.Roots {
+		var intermediates []string
+		if r.IntermediateCerts != nil {
+			intermediates = make([]string, len(r.IntermediateCerts))
+			for i, intermediate := range r.IntermediateCerts {
+				intermediates[i] = intermediate
+			}
+		}
 		// IMPORTANT: r must NEVER be modified, since it is a pointer
 		// directly to the structure in the memdb store.
 
@@ -115,8 +121,8 @@ func (s *Server) getCARoots(ws memdb.WatchSet, state *state.Store) (*structs.Ind
 			ExternalTrustDomain: r.ExternalTrustDomain,
 			NotBefore:           r.NotBefore,
 			NotAfter:            r.NotAfter,
-			RootCert:            r.RootCert,
-			IntermediateCerts:   r.IntermediateCerts,
+			RootCert:            ca.EnsureTrailingNewline(r.RootCert),
+			IntermediateCerts:   intermediates,
 			RaftIndex:           r.RaftIndex,
 			Active:              r.Active,
 			PrivateKeyType:      r.PrivateKeyType,
@@ -219,7 +225,7 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
 
 	// Append any intermediates needed by this root.
 	for _, p := range caRoot.IntermediateCerts {
-		pem = strings.TrimSpace(pem) + "\n" + p
+		pem = pem + ca.EnsureTrailingNewline(p)
 	}
 
 	// Append our local CA's intermediate if there is one.
@@ -231,9 +237,8 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
 	if err != nil {
 		return nil, err
 	}
-
 	if inter != root {
-		pem = strings.TrimSpace(pem) + "\n" + inter
+		pem = pem + ca.EnsureTrailingNewline(inter)
 	}
 
 	// TODO(banks): when we implement IssuedCerts table we can use the insert to