luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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>
This commit is contained in:
Dhia Ayachi 2021-06-30 20:48:29 -04:00 committed by GitHub
parent 1ec131924a
commit b57cf27e8f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 74 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/10411.txt Normal file
View File

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

13
agent/connect/ca/common.go
View File

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

1
agent/connect/ca/provider_aws_test.go
View File

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

2
agent/connect/ca/provider_consul_test.go
View File

@ -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])

8
agent/connect/ca/provider_vault.go
View File

@ -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

12
agent/connect/ca/provider_vault_test.go
View File

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

9
agent/connect/ca/testing.go
View File

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

6
agent/connect_ca_endpoint.go
View File

@ -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

28
agent/connect_ca_endpoint_test.go
View File

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

8
agent/consul/leader_connect_ca_test.go
View File

@ -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 }

17
agent/consul/server_connect.go
View File

@ -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

1
agent/consul/state/connect_ca.go
View File

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

7
agent/consul/state/connect_ca_test.go
View File

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

8
agent/xds/listeners.go
View File

@ -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),
},
},
},