From b57cf27e8fccc2cd24ad458484c74671800749ed Mon Sep 17 00:00:00 2001 From: Dhia Ayachi Date: Wed, 30 Jun 2021 20:48:29 -0400 Subject: [PATCH] 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 * 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 --- .changelog/10411.txt | 3 +++ agent/connect/ca/common.go | 13 +++++++++++ agent/connect/ca/provider_aws_test.go | 1 + agent/connect/ca/provider_consul_test.go | 2 +- agent/connect/ca/provider_vault.go | 8 +++---- agent/connect/ca/provider_vault_test.go | 12 +++------- agent/connect/ca/testing.go | 9 ++++++++ agent/connect_ca_endpoint.go | 6 ++--- agent/connect_ca_endpoint_test.go | 28 ++++++++++-------------- agent/consul/leader_connect_ca_test.go | 8 +++++-- agent/consul/server_connect.go | 17 +++++++++----- agent/consul/state/connect_ca.go | 1 - agent/consul/state/connect_ca_test.go | 7 +++--- agent/xds/listeners.go | 8 ++++--- 14 files changed, 74 insertions(+), 49 deletions(-) create mode 100644 .changelog/10411.txt diff --git a/.changelog/10411.txt b/.changelog/10411.txt new file mode 100644 index 000000000..a2dc01b50 --- /dev/null +++ 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 index a2cd325f0..d0a17b456 100644 --- 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 index e4c86d4d7..5055fba55 100644 --- 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 index 7a58c12fb..852e6387d 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/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]) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index e0dd6647e..369185083 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -366,7 +366,7 @@ func (v *VaultProvider) getCA(path string) (string, error) { return "", err } - root := string(bytes) + root := EnsureTrailingNewline(string(bytes)) if root == "" { return "", ErrBackendNotInitialized } @@ -440,7 +440,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 @@ -477,7 +477,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 @@ -517,7 +517,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 index d3b36da96..3354ba2d4 100644 --- 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 index 0bd837f7f..7b79f62f0 100644 --- 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 index 3075c8fa1..383757e85 100644 --- 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 index 373180da1..dd5a2911f 100644 --- a/agent/connect_ca_endpoint_test.go +++ b/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) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 535c00496..c69d983e8 100644 --- 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 index 080c2f24e..33d5fb283 100644 --- a/agent/consul/server_connect.go +++ b/agent/consul/server_connect.go @@ -5,7 +5,6 @@ import ( "crypto/x509" "fmt" "net/url" - "strings" "sync" memdb "github.com/hashicorp/go-memdb" @@ -105,6 +104,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. @@ -116,8 +122,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, @@ -222,7 +228,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. @@ -234,9 +240,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 diff --git a/agent/consul/state/connect_ca.go b/agent/consul/state/connect_ca.go index e3e89f852..7e9ac11f9 100644 --- a/agent/consul/state/connect_ca.go +++ b/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" diff --git a/agent/consul/state/connect_ca_test.go b/agent/consul/state/connect_ca_test.go index c3509f404..3136a32da 100644 --- a/agent/consul/state/connect_ca_test.go +++ b/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) { diff --git a/agent/xds/listeners.go b/agent/xds/listeners.go index 91f4cf61b..4e528cf73 100644 --- a/agent/xds/listeners.go +++ b/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), }, }, },