From 824a9b494364fe9d43be37dba439064cbe1aa4e1 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 20 Jun 2018 12:37:36 +0100 Subject: [PATCH] Actually return Intermediate certificates bundled with a leaf! --- agent/connect/ca/provider.go | 18 +++++-- agent/consul/connect_ca_endpoint.go | 14 +++-- agent/consul/connect_ca_endpoint_test.go | 66 +++++++++++++++++++++--- agent/consul/leader.go | 15 +++--- agent/consul/server.go | 7 ++- 5 files changed, 98 insertions(+), 22 deletions(-) diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go index 0fdd3e41b..6027fc62a 100644 --- a/agent/connect/ca/provider.go +++ b/agent/connect/ca/provider.go @@ -13,15 +13,23 @@ type Provider interface { // ActiveIntermediate() ActiveRoot() (string, error) - // ActiveIntermediate returns the current signing cert used by this - // provider for generating SPIFFE leaf certs. + // ActiveIntermediate returns the current signing cert used by this provider + // for generating SPIFFE leaf certs. Note that this must not change except + // when Consul requests the change via GenerateIntermediate. Changing the + // signing cert will break Consul's assumptions about which validation paths + // are active. ActiveIntermediate() (string, error) - // GenerateIntermediate returns a new intermediate signing cert and - // sets it to the active intermediate. + // GenerateIntermediate returns a new intermediate signing cert and sets it to + // the active intermediate. If multiple intermediates are needed to complete + // the chain from the signing certificate back to the active root, they should + // all by bundled here. GenerateIntermediate() (string, error) - // Sign signs a leaf certificate used by Connect proxies from a CSR. + // Sign signs a leaf certificate used by Connect proxies from a CSR. The PEM + // returned should include only the leaf certificate as all Intermediates + // needed to validate it will be added by Consul based on the active + // intemediate and any cross-signed intermediates managed by Consul. Sign(*x509.CertificateRequest) (string, error) // CrossSignCA must accept a CA certificate signed by another CA's key diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index 1e24bac7b..4d0e1c95c 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "reflect" + "strings" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/connect" @@ -122,7 +123,7 @@ func (s *ConnectCA) ConfigurationSet( } // If the config has been committed, update the local provider instance - s.srv.setCAProvider(newProvider) + s.srv.setCAProvider(newProvider, newActiveRoot) s.srv.logger.Printf("[INFO] connect: CA provider config updated") @@ -150,7 +151,7 @@ func (s *ConnectCA) ConfigurationSet( } // Have the old provider cross-sign the new intermediate - oldProvider := s.srv.getCAProvider() + oldProvider, _ := s.srv.getCAProvider() if oldProvider == nil { return fmt.Errorf("internal error: CA provider is nil") } @@ -191,7 +192,7 @@ func (s *ConnectCA) ConfigurationSet( // If the config has been committed, update the local provider instance // and call teardown on the old provider - s.srv.setCAProvider(newProvider) + s.srv.setCAProvider(newProvider, newActiveRoot) if err := oldProvider.Cleanup(); err != nil { s.srv.logger.Printf("[WARN] connect: failed to clean up old provider %q", config.Provider) @@ -309,7 +310,7 @@ func (s *ConnectCA) Sign( return fmt.Errorf("SPIFFE ID in CSR must be a service ID") } - provider := s.srv.getCAProvider() + provider, caRoot := s.srv.getCAProvider() if provider == nil { return fmt.Errorf("internal error: CA provider is nil") } @@ -348,6 +349,11 @@ func (s *ConnectCA) Sign( return err } + // Append any intermediates needed by this root. + for _, p := range caRoot.IntermediateCerts { + pem = strings.TrimSpace(pem) + "\n" + p + } + // TODO(banks): when we implement IssuedCerts table we can use the insert to // that as the raft index to return in response. Right now we can rely on only // the built-in provider being supported and the implementation detail that we diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index eb7176b67..a56c2bc89 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -2,6 +2,7 @@ package consul import ( "crypto/x509" + "encoding/pem" "fmt" "os" "testing" @@ -138,6 +139,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { t.Parallel() assert := assert.New(t) + require := require.New(t) dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -151,7 +153,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { Datacenter: "dc1", } var rootList structs.IndexedCARoots - assert.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList)) + require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList)) assert.Len(rootList.Roots, 1) oldRoot := rootList.Roots[0] @@ -174,17 +176,18 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { } var reply interface{} - assert.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) } // Make sure the new root has been added along with an intermediate // cross-signed by the old root. + var newRootPEM string { args := &structs.DCSpecificRequest{ Datacenter: "dc1", } var reply structs.IndexedCARoots - assert.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply)) + require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply)) assert.Len(reply.Roots, 2) for _, r := range reply.Roots { @@ -197,6 +200,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { assert.Equal(r.SigningCert, oldRoot.SigningCert) assert.Equal(r.IntermediateCerts, oldRoot.IntermediateCerts) } else { + newRootPEM = r.RootCert // The new root should have a valid cross-signed cert from the old // root as an intermediate. assert.True(r.Active) @@ -225,15 +229,65 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { Datacenter: "dc1", } var reply structs.CAConfiguration - assert.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply)) + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply)) actual, err := ca.ParseConsulCAConfig(reply.Config) - assert.NoError(err) + require.NoError(err) expected, err := ca.ParseConsulCAConfig(newConfig.Config) - assert.NoError(err) + require.NoError(err) assert.Equal(reply.Provider, newConfig.Provider) assert.Equal(actual, expected) } + + // Verify that new leaf certs get the cross-signed intermediate bundled + { + // Generate a CSR and request signing + spiffeId := connect.TestSpiffeIDService(t, "web") + csr, _ := connect.TestCSR(t, spiffeId) + args := &structs.CASignRequest{ + Datacenter: "dc1", + CSR: csr, + } + var reply structs.IssuedCert + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) + + // Verify that the cert is signed by the new CA + { + roots := x509.NewCertPool() + require.True(roots.AppendCertsFromPEM([]byte(newRootPEM))) + leaf, err := connect.ParseCert(reply.CertPEM) + require.NoError(err) + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: roots, + }) + require.NoError(err) + } + + // And that it validates via the intermediate + { + roots := x509.NewCertPool() + assert.True(roots.AppendCertsFromPEM([]byte(oldRoot.RootCert))) + leaf, err := connect.ParseCert(reply.CertPEM) + require.NoError(err) + + // Make sure the intermediate was returned as well as leaf + _, rest := pem.Decode([]byte(reply.CertPEM)) + require.NotEmpty(rest) + + intermediates := x509.NewCertPool() + require.True(intermediates.AppendCertsFromPEM(rest)) + + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }) + require.NoError(err) + } + + // Verify other fields + assert.Equal("web", reply.Service) + assert.Equal(spiffeId.URI().String(), reply.ServiceURI) + } } // Test CA signing diff --git a/agent/consul/leader.go b/agent/consul/leader.go index f47dde83f..cf22c4418 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -236,7 +236,7 @@ func (s *Server) revokeLeadership() error { return err } - s.setCAProvider(nil) + s.setCAProvider(nil, nil) s.resetConsistentReadReady() s.autopilot.Stop() @@ -422,8 +422,6 @@ func (s *Server) initializeCA() error { return err } - s.setCAProvider(provider) - // Get the active root cert from the CA rootPEM, err := provider.ActiveRoot() if err != nil { @@ -435,6 +433,8 @@ func (s *Server) initializeCA() error { return err } + s.setCAProvider(provider, rootCA) + // Check if the CA root is already initialized and exit if it is. // Every change to the CA after this initial bootstrapping should // be done through the rotation process. @@ -507,12 +507,14 @@ func (s *Server) createCAProvider(conf *structs.CAConfiguration) (ca.Provider, e } } -func (s *Server) getCAProvider() ca.Provider { +func (s *Server) getCAProvider() (ca.Provider, *structs.CARoot) { retries := 0 var result ca.Provider + var resultRoot *structs.CARoot for result == nil { s.caProviderLock.RLock() result = s.caProvider + resultRoot = s.caProviderRoot s.caProviderLock.RUnlock() // In cases where an agent is started with managed proxies, we may ask @@ -527,13 +529,14 @@ func (s *Server) getCAProvider() ca.Provider { break } - return result + return result, resultRoot } -func (s *Server) setCAProvider(newProvider ca.Provider) { +func (s *Server) setCAProvider(newProvider ca.Provider, root *structs.CARoot) { s.caProviderLock.Lock() defer s.caProviderLock.Unlock() s.caProvider = newProvider + s.caProviderRoot = root } // reconcileReaped is used to reconcile nodes that have failed and been reaped diff --git a/agent/consul/server.go b/agent/consul/server.go index 7b589d753..169d76beb 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -99,7 +99,12 @@ type Server struct { // caProvider is the current CA provider in use for Connect. This is // only non-nil when we are the leader. - caProvider ca.Provider + caProvider ca.Provider + // caProviderRoot is the CARoot that was stored along with the ca.Provider + // active. It's only updated in lock-step with the caProvider. This prevents + // races between state updates to active roots and the fetch of the provider + // instance. + caProviderRoot *structs.CARoot caProviderLock sync.RWMutex // Consul configuration