From 8e8670627d4f5dd64d76c4b8456931280036d487 Mon Sep 17 00:00:00 2001 From: cyli Date: Wed, 29 Mar 2017 11:59:50 -0700 Subject: [PATCH] Add RequestAndSaveNewCertificates test that handles intermediates, and fix some issues with thest TestCA not supporting intermediates. Signed-off-by: cyli --- ca/certificates.go | 4 +- ca/certificates_test.go | 128 +++++++++++++++++++++++++--------------- ca/testutils/cautils.go | 31 ++-------- 3 files changed, 89 insertions(+), 74 deletions(-) diff --git a/ca/certificates.go b/ca/certificates.go index 21e7338f65..61e8d57d31 100644 --- a/ca/certificates.go +++ b/ca/certificates.go @@ -284,9 +284,9 @@ func (rca *RootCA) RequestAndSaveNewCertificates(ctx context.Context, kw KeyWrit return &tlsKeyPair, nil } -func (rca *RootCA) getKEKUpdate(ctx context.Context, cert *x509.Certificate, keypair tls.Certificate, connBroker *connectionbroker.Broker) (*KEKData, error) { +func (rca *RootCA) getKEKUpdate(ctx context.Context, leafCert *x509.Certificate, keypair tls.Certificate, connBroker *connectionbroker.Broker) (*KEKData, error) { var managerRole bool - for _, ou := range cert.Subject.OrganizationalUnit { + for _, ou := range leafCert.Subject.OrganizationalUnit { if ou == ManagerRole { managerRole = true break diff --git a/ca/certificates_test.go b/ca/certificates_test.go index ab488ad505..83b653052d 100644 --- a/ca/certificates_test.go +++ b/ca/certificates_test.go @@ -314,43 +314,83 @@ func TestGetRemoteCAInvalidHash(t *testing.T) { assert.Error(t, err) } -func TestRequestAndSaveNewCertificates(t *testing.T) { - tc := testutils.NewTestCA(t) +func testRequestAndSaveNewCertificates(t *testing.T, tc *testutils.TestCA, numIntermediates int) { defer tc.Stop() // Copy the current RootCA without the signer rca := ca.RootCA{Certs: tc.RootCA.Certs, Pool: tc.RootCA.Pool} - cert, err := rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, + tlsCert, err := rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, ca.CertificateRequestConfig{ Token: tc.ManagerToken, ConnBroker: tc.ConnBroker, }) - assert.NoError(t, err) - assert.NotNil(t, cert) + require.NoError(t, err) + require.NotNil(t, tlsCert) perms, err := permbits.Stat(tc.Paths.Node.Cert) - assert.NoError(t, err) - assert.False(t, perms.GroupWrite()) - assert.False(t, perms.OtherWrite()) + require.NoError(t, err) + require.False(t, perms.GroupWrite()) + require.False(t, perms.OtherWrite()) - // there was no encryption config in the remote, so the key should be unencrypted - unencryptedKeyReader := ca.NewKeyReadWriter(tc.Paths.Node, nil, nil) - _, _, err = unencryptedKeyReader.Read() + certs, err := ioutil.ReadFile(tc.Paths.Node.Cert) require.NoError(t, err) - // the worker token is also unencrypted - cert, err = rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, - ca.CertificateRequestConfig{ - Token: tc.WorkerToken, - ConnBroker: tc.ConnBroker, - }) - assert.NoError(t, err) - assert.NotNil(t, cert) - _, _, err = unencryptedKeyReader.Read() + // ensure that the same number of certs was written + parsedCerts, err := helpers.ParseCertificatesPEM(certs) + require.NoError(t, err) + require.Len(t, parsedCerts, 1+numIntermediates) +} + +func TestRequestAndSaveNewCertificatesNoIntermediate(t *testing.T) { + t.Parallel() + + tc := testutils.NewTestCA(t) + testRequestAndSaveNewCertificates(t, tc, 0) +} + +func TestRequestAndSaveNewCertificatesWithIntermediates(t *testing.T) { + t.Parallel() + + // use a RootCA with an intermediate + rca, err := ca.NewRootCA(testutils.ECDSACertChain[2], testutils.ECDSACertChain[1], testutils.ECDSACertChainKeys[1], + ca.DefaultNodeCertExpiration, testutils.ECDSACertChain[1]) require.NoError(t, err) + tempdir, err := ioutil.TempDir("", "test-request-and-save-new-certificates") + require.NoError(t, err) + defer os.RemoveAll(tempdir) + + tc := testutils.NewTestCAFromRootCA(t, tempdir, rca, nil) + testRequestAndSaveNewCertificates(t, tc, 1) +} + +func TestRequestAndSaveNewCertificatesWithKEKUpdate(t *testing.T) { + t.Parallel() + + tc := testutils.NewTestCA(t) + defer tc.Stop() + + // Copy the current RootCA without the signer + rca := ca.RootCA{Certs: tc.RootCA.Certs, Pool: tc.RootCA.Pool} + + unencryptedKeyReader := ca.NewKeyReadWriter(tc.Paths.Node, nil, nil) + + // key for the manager and worker are both unencrypted + for _, token := range []string{tc.ManagerToken, tc.WorkerToken} { + _, err := rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, + ca.CertificateRequestConfig{ + Token: token, + ConnBroker: tc.ConnBroker, + }) + require.NoError(t, err) + + // there was no encryption config in the remote, so the key should be unencrypted + _, _, err = unencryptedKeyReader.Read() + require.NoError(t, err) + } + // If there is a different kek in the remote store, when TLS certs are renewed the new key will // be encrypted with that kek - assert.NoError(t, tc.MemoryStore.Update(func(tx store.Tx) error { + require.NoError(t, tc.MemoryStore.Update(func(tx store.Tx) error { cluster := store.GetCluster(tx, tc.Organization) cluster.Spec.EncryptionConfig.AutoLockManagers = true cluster.UnlockKeys = []*api.EncryptionKey{{ @@ -359,37 +399,31 @@ func TestRequestAndSaveNewCertificates(t *testing.T) { }} return store.UpdateCluster(tx, cluster) })) - assert.NoError(t, os.RemoveAll(tc.Paths.Node.Cert)) - assert.NoError(t, os.RemoveAll(tc.Paths.Node.Key)) - - _, err = rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, - ca.CertificateRequestConfig{ - Token: tc.ManagerToken, - ConnBroker: tc.ConnBroker, - }) - assert.NoError(t, err) + require.NoError(t, os.RemoveAll(tc.Paths.Node.Cert)) + require.NoError(t, os.RemoveAll(tc.Paths.Node.Key)) - // key can no longer be read without a kek - _, _, err = unencryptedKeyReader.Read() - require.Error(t, err) + // key for the manager will be encrypted, but certs for the worker will not be + for _, token := range []string{tc.ManagerToken, tc.WorkerToken} { + _, err := rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, + ca.CertificateRequestConfig{ + Token: token, + ConnBroker: tc.ConnBroker, + }) + require.NoError(t, err) - _, _, err = ca.NewKeyReadWriter(tc.Paths.Node, []byte("kek!"), nil).Read() - require.NoError(t, err) + // there was no encryption config in the remote, so the key should be unencrypted + _, _, err = unencryptedKeyReader.Read() - // if it's a worker though, the key is always unencrypted, even though the manager key is encrypted - _, err = rca.RequestAndSaveNewCertificates(tc.Context, tc.KeyReadWriter, - ca.CertificateRequestConfig{ - Token: tc.WorkerToken, - ConnBroker: tc.ConnBroker, - }) - assert.NoError(t, err) - _, _, err = unencryptedKeyReader.Read() - require.NoError(t, err) + if token == tc.ManagerToken { + require.Error(t, err) + _, _, err = ca.NewKeyReadWriter(tc.Paths.Node, []byte("kek!"), nil).Read() + require.NoError(t, err) + } else { + require.NoError(t, err) + } + } } -// TODO(cyli): add test for RequestAndSaveNewCertificates but with intermediates - this involves adding -// support for appending intermediates on the CA server first - func TestIssueAndSaveNewCertificates(t *testing.T) { tc := testutils.NewTestCA(t) defer tc.Stop() diff --git a/ca/testutils/cautils.go b/ca/testutils/cautils.go index fb5f02039c..8cedbd4685 100644 --- a/ca/testutils/cautils.go +++ b/ca/testutils/cautils.go @@ -15,7 +15,6 @@ import ( cfcsr "github.com/cloudflare/cfssl/csr" "github.com/cloudflare/cfssl/helpers" "github.com/cloudflare/cfssl/initca" - cfsigner "github.com/cloudflare/cfssl/signer" "github.com/docker/swarmkit/api" "github.com/docker/swarmkit/ca" "github.com/docker/swarmkit/connectionbroker" @@ -284,31 +283,12 @@ func genSecurityConfig(s *store.MemoryStore, rootCA ca.RootCA, krw *ca.KeyReadWr // Obtain a signed Certificate nodeID := identity.NewID() - // All managers get added the subject-alt-name of CA, so they can be used for cert issuance - hosts := []string{role} - if role == ca.ManagerRole { - hosts = append(hosts, ca.CARole) - } - signer, err := rootCA.Signer() - if err != nil { - return nil, err - } - cert, err := signer.Sign(cfsigner.SignRequest{ - Request: string(csr), - // OU is used for Authentication of the node type. The CN has the random - // node ID. - Subject: &cfsigner.Subject{CN: nodeID, Names: []cfcsr.Name{{OU: role, O: org}}}, - // Adding ou as DNS alt name, so clients can connect to ManagerRole and CARole - Hosts: hosts, - }) + certChain, err := rootCA.ParseValidateAndSignCSR(csr, nodeID, role, org) if err != nil { return nil, err } - // Append the root CA Key to the certificate, to create a valid chain - certChain := append(cert, rootCA.Certs...) - // If we were instructed to persist the files if tmpDir != "" { paths := ca.NewConfigPaths(tmpDir) @@ -336,16 +316,17 @@ func genSecurityConfig(s *store.MemoryStore, rootCA ca.RootCA, krw *ca.KeyReadWr return nil, err } - err = createNode(s, nodeID, role, csr, cert) + err = createNode(s, nodeID, role, csr, certChain) if err != nil { return nil, err } if nonSigningRoot { rootCA = ca.RootCA{ - Certs: rootCA.Certs, - Digest: rootCA.Digest, - Pool: rootCA.Pool, + Certs: rootCA.Certs, + Digest: rootCA.Digest, + Pool: rootCA.Pool, + Intermediates: rootCA.Intermediates, } }