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 <4903+rboyer@users.noreply.github.com>

* fix typo

* simplify code callflow

Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>

* 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 <dnephin@hashicorp.com>

* 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 <4903+rboyer@users.noreply.github.com>
Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>

.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.
+```

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)
+}

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) {

agent/connect/ca/provider_consul_test.go

@@ -149,7 +149,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])

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

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)
 

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")
+	}
+}

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

agent/connect_ca_endpoint_test.go

@@ -24,7 +24,6 @@ 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")
@@ -32,8 +31,8 @@ func TestConnectCARoots_empty(t *testing.T) {
 	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) {
@@ -43,7 +42,7 @@ 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")
@@ -56,16 +55,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)
 	}
 }
 
@@ -225,7 +224,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 = `
@@ -251,23 +249,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)
 			}
 		})
 	}
@@ -300,9 +298,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)

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 }

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

agent/consul/state/connect_ca.go

@@ -2,7 +2,6 @@ package state
 
 import (
 	"fmt"
-
 	"github.com/hashicorp/go-memdb"
 
 	"github.com/hashicorp/consul/agent/structs"

agent/consul/state/connect_ca_test.go

@@ -195,7 +195,7 @@ func TestStore_CARootSetList(t *testing.T) {
 
 	// Build a valid value
 	ca1 := connect.TestCA(t, nil)
-
+	expected := *ca1
 	// Set
 	ok, err := s.CARootSetCAS(1, 0, []*structs.CARoot{ca1})
 	assert.Nil(err)
@@ -206,18 +206,17 @@ func TestStore_CARootSetList(t *testing.T) {
 	assert.True(watchFired(ws), "watch fired")
 
 	// Read it back out and verify it.
-	expected := *ca1
+
 	expected.RaftIndex = structs.RaftIndex{
 		CreateIndex: 1,
 		ModifyIndex: 1,
 	}
-
 	ws = memdb.NewWatchSet()
 	_, roots, err := s.CARoots(ws)
 	assert.Nil(err)
 	assert.Len(roots, 1)
 	actual := roots[0]
-	assert.Equal(&expected, actual)
+	assertDeepEqual(t, expected, *actual)
 }
 
 func TestStore_CARootSet_emptyID(t *testing.T) {

agent/xds/listeners.go

@@ -11,6 +11,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/consul/agent/connect/ca"
+
 	envoy_core_v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	envoy_listener_v3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
 	envoy_route_v3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
@@ -1737,7 +1739,7 @@ func makeCommonTLSContextFromLeaf(cfgSnap *proxycfg.ConfigSnapshot, leaf *struct
 	// TODO(banks): verify this actually works with Envoy (docs are not clear).
 	rootPEMS := ""
 	for _, root := range cfgSnap.Roots.Roots {
-		rootPEMS += root.RootCert
+		rootPEMS += ca.EnsureTrailingNewline(root.RootCert)
 	}
 
 	return &envoy_tls_v3.CommonTlsContext{
@@ -1746,12 +1748,12 @@ func makeCommonTLSContextFromLeaf(cfgSnap *proxycfg.ConfigSnapshot, leaf *struct
 			{
 				CertificateChain: &envoy_core_v3.DataSource{
 					Specifier: &envoy_core_v3.DataSource_InlineString{
-						InlineString: leaf.CertPEM,
+						InlineString: ca.EnsureTrailingNewline(leaf.CertPEM),
 					},
 				},
 				PrivateKey: &envoy_core_v3.DataSource{
 					Specifier: &envoy_core_v3.DataSource_InlineString{
-						InlineString: leaf.PrivateKeyPEM,
+						InlineString: ca.EnsureTrailingNewline(leaf.PrivateKeyPEM),
 					},
 				},
 			},