From b4803eca5984e6325f3d4c5fc613db9c06597cd6 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 9 May 2018 17:15:29 +0100 Subject: [PATCH] Generate CSR using real trust-domain --- agent/agent_endpoint_test.go | 26 ++++++---- agent/cache-types/connect_ca_leaf.go | 46 +++++++++++++----- agent/cache-types/connect_ca_leaf_test.go | 5 +- agent/connect/ca/provider_consul.go | 4 +- agent/connect/csr.go | 59 +++++++++++++++++++++++ agent/connect/testing_ca.go | 5 +- agent/connect/uri_signing.go | 3 +- agent/consul/connect_ca_endpoint.go | 14 ++++-- agent/consul/connect_ca_endpoint_test.go | 2 +- agent/testagent.go | 3 +- 10 files changed, 136 insertions(+), 31 deletions(-) create mode 100644 agent/connect/csr.go diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index d4b55a50f0..4749148e1a 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -2106,13 +2106,14 @@ func TestAgentConnectCARoots_empty(t *testing.T) { t.Parallel() assert := assert.New(t) + require := require.New(t) a := NewTestAgent(t.Name(), "connect { enabled = false }") defer a.Shutdown() req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/roots", nil) resp := httptest.NewRecorder() obj, err := a.srv.AgentConnectCARoots(resp, req) - assert.Nil(err) + require.NoError(err) value := obj.(structs.IndexedCARoots) assert.Equal(value.ActiveRootID, "") @@ -2122,6 +2123,7 @@ func TestAgentConnectCARoots_empty(t *testing.T) { func TestAgentConnectCARoots_list(t *testing.T) { t.Parallel() + assert := assert.New(t) require := require.New(t) a := NewTestAgent(t.Name(), "") defer a.Shutdown() @@ -2137,30 +2139,34 @@ func TestAgentConnectCARoots_list(t *testing.T) { req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/roots", nil) resp := httptest.NewRecorder() obj, err := a.srv.AgentConnectCARoots(resp, req) - require.Nil(err) + require.NoError(err) value := obj.(structs.IndexedCARoots) - require.Equal(value.ActiveRootID, ca2.ID) - require.Len(value.Roots, 2) + assert.Equal(value.ActiveRootID, ca2.ID) + // Would like to assert that it's the same as the TestAgent domain but the + // only way to access that state via this package is by RPC to the server + // implementation running in TestAgent which is more or less a tautology. + assert.NotEmpty(value.TrustDomain) + assert.Len(value.Roots, 2) // We should never have the secret information for _, r := range value.Roots { - require.Equal("", r.SigningCert) - require.Equal("", r.SigningKey) + assert.Equal("", r.SigningCert) + assert.Equal("", r.SigningKey) } // That should've been a cache miss, so no hit change - require.Equal(cacheHits, a.cache.Hits()) + assert.Equal(cacheHits, a.cache.Hits()) // Test caching { // List it again obj2, err := a.srv.AgentConnectCARoots(httptest.NewRecorder(), req) - require.Nil(err) - require.Equal(obj, obj2) + require.NoError(err) + assert.Equal(obj, obj2) // Should cache hit this time and not make request - require.Equal(cacheHits+1, a.cache.Hits()) + assert.Equal(cacheHits+1, a.cache.Hits()) cacheHits++ } diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 1058ec26a3..4070298df8 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -1,6 +1,7 @@ package cachetype import ( + "errors" "fmt" "sync" "sync/atomic" @@ -9,9 +10,7 @@ import ( "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" - // NOTE(mitcehllh): This is temporary while certs are stubbed out. - "github.com/mitchellh/go-testing-interface" ) // Recommended name for registration. @@ -97,16 +96,41 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache // by the above channel). } - // Create a CSR. - // TODO(mitchellh): This is obviously not production ready! The host - // needs a correct host ID, and we probably don't want to use TestCSR - // and want a non-test-specific way to create a CSR. - csr, pk := connect.TestCSR(&testing.RuntimeT{}, &connect.SpiffeIDService{ - Host: "11111111-2222-3333-4444-555555555555.consul", - Namespace: "default", + // Need to lookup RootCAs response to discover trust domain. First just lookup + // with no blocking info - this should be a cache hit most of the time. + rawRoots, err := c.Cache.Get(ConnectCARootName, &structs.DCSpecificRequest{ Datacenter: reqReal.Datacenter, - Service: reqReal.Service, }) + if err != nil { + return result, err + } + roots, ok := rawRoots.(*structs.IndexedCARoots) + if !ok { + return result, errors.New("invalid RootCA response type") + } + if roots.TrustDomain == "" { + return result, errors.New("cluster has no CA bootstrapped") + } + + // Build the service ID + serviceID := &connect.SpiffeIDService{ + Host: roots.TrustDomain, + Datacenter: reqReal.Datacenter, + Namespace: "default", + Service: reqReal.Service, + } + + // Create a new private key + pk, pkPEM, err := connect.GeneratePrivateKey() + if err != nil { + return result, err + } + + // Create a CSR. + csr, err := connect.CreateCSR(serviceID, pk) + if err != nil { + return result, err + } // Request signing var reply structs.IssuedCert @@ -117,7 +141,7 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache if err := c.RPC.RPC("ConnectCA.Sign", &args, &reply); err != nil { return result, err } - reply.PrivateKeyPEM = pk + reply.PrivateKeyPEM = pkPEM // Lock the issued certs map so we can insert it. We only insert if // we didn't happen to get a newer one. This should never happen since diff --git a/agent/cache-types/connect_ca_leaf_test.go b/agent/cache-types/connect_ca_leaf_test.go index 0612aed21e..d55caf408d 100644 --- a/agent/cache-types/connect_ca_leaf_test.go +++ b/agent/cache-types/connect_ca_leaf_test.go @@ -25,10 +25,11 @@ func TestConnectCALeaf_changingRoots(t *testing.T) { defer close(rootsCh) rootsCh <- structs.IndexedCARoots{ ActiveRootID: "1", + TrustDomain: "fake-trust-domain.consul", QueryMeta: structs.QueryMeta{Index: 1}, } - // Instrument ConnectCA.Sign to + // Instrument ConnectCA.Sign to return signed cert var resp *structs.IssuedCert var idx uint64 rpc.On("RPC", "ConnectCA.Sign", mock.Anything, mock.Anything).Return(nil). @@ -67,6 +68,7 @@ func TestConnectCALeaf_changingRoots(t *testing.T) { // Let's send in new roots, which should trigger the sign req rootsCh <- structs.IndexedCARoots{ ActiveRootID: "2", + TrustDomain: "fake-trust-domain.consul", QueryMeta: structs.QueryMeta{Index: 2}, } select { @@ -101,6 +103,7 @@ func TestConnectCALeaf_expiringLeaf(t *testing.T) { defer close(rootsCh) rootsCh <- structs.IndexedCARoots{ ActiveRootID: "1", + TrustDomain: "fake-trust-domain.consul", QueryMeta: structs.QueryMeta{Index: 1}, } diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index d88a58bfc3..20641a16cb 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -79,7 +79,7 @@ func NewConsulProvider(rawConfig map[string]interface{}, delegate ConsulProvider // Generate a private key if needed if conf.PrivateKey == "" { - pk, err := connect.GeneratePrivateKey() + _, pk, err := connect.GeneratePrivateKey() if err != nil { return nil, err } @@ -247,7 +247,7 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) { } err = pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: bs}) if err != nil { - return "", fmt.Errorf("error encoding private key: %s", err) + return "", fmt.Errorf("error encoding certificate: %s", err) } err = c.incrementProviderIndex(providerState) diff --git a/agent/connect/csr.go b/agent/connect/csr.go new file mode 100644 index 0000000000..4b975d06c5 --- /dev/null +++ b/agent/connect/csr.go @@ -0,0 +1,59 @@ +package connect + +import ( + "bytes" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "encoding/pem" + "fmt" + "net/url" +) + +// CreateCSR returns a CSR to sign the given service along with the PEM-encoded +// private key for this certificate. +func CreateCSR(uri CertURI, privateKey crypto.Signer) (string, error) { + template := &x509.CertificateRequest{ + URIs: []*url.URL{uri.URI()}, + SignatureAlgorithm: x509.ECDSAWithSHA256, + } + + // Create the CSR itself + var csrBuf bytes.Buffer + bs, err := x509.CreateCertificateRequest(rand.Reader, template, privateKey) + if err != nil { + return "", err + } + + err = pem.Encode(&csrBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: bs}) + if err != nil { + return "", err + } + + return csrBuf.String(), nil +} + +// GeneratePrivateKey generates a new Private key +func GeneratePrivateKey() (crypto.Signer, string, error) { + var pk *ecdsa.PrivateKey + + pk, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + return nil, "", fmt.Errorf("error generating private key: %s", err) + } + + bs, err := x509.MarshalECPrivateKey(pk) + if err != nil { + return nil, "", fmt.Errorf("error generating private key: %s", err) + } + + var buf bytes.Buffer + err = pem.Encode(&buf, &pem.Block{Type: "EC PRIVATE KEY", Bytes: bs}) + if err != nil { + return nil, "", fmt.Errorf("error encoding private key: %s", err) + } + + return pk, buf.String(), nil +} diff --git a/agent/connect/testing_ca.go b/agent/connect/testing_ca.go index 552c575353..ba2d29203b 100644 --- a/agent/connect/testing_ca.go +++ b/agent/connect/testing_ca.go @@ -161,7 +161,10 @@ func TestLeaf(t testing.T, service string, root *structs.CARoot) (string, string } // Generate fresh private key - pkSigner, pkPEM := testPrivateKey(t) + pkSigner, pkPEM, err := GeneratePrivateKey() + if err != nil { + t.Fatalf("failed to generate private key: %s", err) + } // Cert template for generation template := x509.Certificate{ diff --git a/agent/connect/uri_signing.go b/agent/connect/uri_signing.go index 843f955965..d934360ebe 100644 --- a/agent/connect/uri_signing.go +++ b/agent/connect/uri_signing.go @@ -62,7 +62,8 @@ func (id *SpiffeIDSigning) CanSign(cu CertURI) bool { } // SpiffeIDSigningForCluster returns the SPIFFE signing identifier (trust -// domain) representation of the given CA config. +// domain) representation of the given CA config. If config is nil this function +// will panic. // // NOTE(banks): we intentionally fix the tld `.consul` for now rather than tie // this to the `domain` config used for DNS because changing DNS domain can't diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index a72a4998b4..1e24bac7be 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -224,9 +224,17 @@ func (s *ConnectCA) Roots( if err != nil { return err } - // Build TrustDomain based on the ClusterID stored. - signingID := connect.SpiffeIDSigningForCluster(config) - reply.TrustDomain = signingID.Host() + // Check CA is actually bootstrapped... + if config != nil { + // Build TrustDomain based on the ClusterID stored. + signingID := connect.SpiffeIDSigningForCluster(config) + if signingID == nil { + // If CA is bootstrapped at all then this should never happen but be + // defensive. + return errors.New("no cluster trust domain setup") + } + reply.TrustDomain = signingID.Host() + } } return s.srv.blockingQuery( diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index f209358777..ac64ceb303 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -157,7 +157,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { // Update the provider config to use a new private key, which should // cause a rotation. - newKey, err := connect.GeneratePrivateKey() + _, newKey, err := connect.GeneratePrivateKey() assert.NoError(err) newConfig := &structs.CAConfiguration{ Provider: "consul", diff --git a/agent/testagent.go b/agent/testagent.go index c2e4ddf016..724b0c80e0 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -16,6 +16,8 @@ import ( "time" metrics "github.com/armon/go-metrics" + uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" @@ -23,7 +25,6 @@ import ( "github.com/hashicorp/consul/lib/freeport" "github.com/hashicorp/consul/logger" "github.com/hashicorp/consul/testutil/retry" - uuid "github.com/hashicorp/go-uuid" ) func init() {