From 31d629245855d54294e04c6e7202f21901da5310 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 2 Jul 2018 16:53:23 -0700 Subject: [PATCH] Propagate the provided external CA certificate to the external CA object in swarm. Also, fix some CLI command confusions: 1. If the --external-ca flag is provided, require a --ca-cert flag as well, otherwise the external CA is set but the CA certificate is actually rotated to an internal cert 2. If a --ca-cert flag is provided, require a --ca-key or --external-ca flag be provided as well, otherwise either the server will say that the request is invalid, or if there was previously an external CA corresponding to the cert, it will succeed. While that works, it's better to require the user to explicitly set all the parameters of the new desired root CA. This also changes the `swarm update` function to set the external CA's CACert field, which while not strictly necessary, makes the CA list more explicit. Signed-off-by: Ying Li (cherry picked from commit 4243440e1fad0f65a525dba86ced608dcea6918f) Signed-off-by: Sebastiaan van Stijn --- cli/command/swarm/ca.go | 24 +-- cli/command/swarm/ca_test.go | 250 ++++++++++++++++++++++++------- cli/command/swarm/opts.go | 11 +- cli/command/swarm/update.go | 2 +- cli/command/swarm/update_test.go | 9 +- 5 files changed, 229 insertions(+), 67 deletions(-) diff --git a/cli/command/swarm/ca.go b/cli/command/swarm/ca.go index 1aa619ecf357..961e7e89441b 100644 --- a/cli/command/swarm/ca.go +++ b/cli/command/swarm/ca.go @@ -68,6 +68,17 @@ func runCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) error { return displayTrustRoot(dockerCli.Out(), swarmInspect) } + if flags.Changed(flagExternalCA) && len(opts.externalCA.Value()) > 0 && !flags.Changed(flagCACert) { + return fmt.Errorf( + "rotating to an external CA requires the `--%s` flag to specify the external CA's cert - "+ + "to add an external CA with the current root CA certificate, use the `update` command instead", flagCACert) + } + + if flags.Changed(flagCACert) && len(opts.externalCA.Value()) == 0 && !flags.Changed(flagCAKey) { + return fmt.Errorf("the --%s flag requires that a --%s flag and/or --%s flag be provided as well", + flagCACert, flagCAKey, flagExternalCA) + } + updateSwarmSpec(&swarmInspect.Spec, flags, opts) if err := client.SwarmUpdate(ctx, swarmInspect.Version, swarmInspect.Spec, swarm.UpdateFlags{}); err != nil { return err @@ -80,20 +91,15 @@ func runCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) error { } func updateSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet, opts caOptions) { - opts.mergeSwarmSpecCAFlags(spec, flags) caCert := opts.rootCACert.Contents() caKey := opts.rootCAKey.Contents() + opts.mergeSwarmSpecCAFlags(spec, flags, caCert) + + spec.CAConfig.SigningCACert = caCert + spec.CAConfig.SigningCAKey = caKey - if caCert != "" { - spec.CAConfig.SigningCACert = caCert - } - if caKey != "" { - spec.CAConfig.SigningCAKey = caKey - } if caKey == "" && caCert == "" { spec.CAConfig.ForceRotate++ - spec.CAConfig.SigningCACert = "" - spec.CAConfig.SigningCAKey = "" } } diff --git a/cli/command/swarm/ca_test.go b/cli/command/swarm/ca_test.go index 499090477e4b..d0eff1164708 100644 --- a/cli/command/swarm/ca_test.go +++ b/cli/command/swarm/ca_test.go @@ -13,6 +13,28 @@ import ( is "gotest.tools/assert/cmp" ) +const ( + cert = ` +-----BEGIN CERTIFICATE----- +MIIBuDCCAV4CCQDOqUYOWdqMdjAKBggqhkjOPQQDAzBjMQswCQYDVQQGEwJVUzEL +MAkGA1UECAwCQ0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDzANBgNVBAoMBkRv +Y2tlcjEPMA0GA1UECwwGRG9ja2VyMQ0wCwYDVQQDDARUZXN0MCAXDTE4MDcwMjIx +MjkxOFoYDzMwMTcxMTAyMjEyOTE4WjBjMQswCQYDVQQGEwJVUzELMAkGA1UECAwC +Q0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDzANBgNVBAoMBkRvY2tlcjEPMA0G +A1UECwwGRG9ja2VyMQ0wCwYDVQQDDARUZXN0MFkwEwYHKoZIzj0CAQYIKoZIzj0D +AQcDQgAEgvvZl5Vqpr1e+g5IhoU6TZHgRau+BZETVFTmqyWYajA/mooRQ1MZTozu +s9ZZZA8tzUhIqS36gsFuyIZ4YiAlyjAKBggqhkjOPQQDAwNIADBFAiBQ7pCPQrj8 +8zaItMf0pk8j1NU5XrFqFEZICzvjzUJQBAIhAKq2gFwoTn8KH+cAAXZpAGJPmOsT +zsBT8gBAOHhNA6/2 +-----END CERTIFICATE-----` + key = ` +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEICyheZpw70pbgO4hEuwhZTETWyTpNJmJ3TyFaWT6WTRkoAoGCCqGSM49 +AwEHoUQDQgAEgvvZl5Vqpr1e+g5IhoU6TZHgRau+BZETVFTmqyWYajA/mooRQ1MZ +Tozus9ZZZA8tzUhIqS36gsFuyIZ4YiAlyg== +-----END EC PRIVATE KEY-----` +) + func swarmSpecWithFullCAConfig() *swarm.Spec { return &swarm.Spec{ CAConfig: swarm.CAConfig{ @@ -37,51 +59,79 @@ func TestDisplayTrustRootNoRoot(t *testing.T) { assert.Error(t, err, "No CA information available") } +type invalidCATestCases struct { + args []string + errorMsg string +} + +func writeFile(data string) (string, error) { + tmpfile, err := ioutil.TempFile("", "testfile") + if err != nil { + return "", err + } + _, err = tmpfile.Write([]byte(data)) + if err != nil { + return "", err + } + tmpfile.Close() + return tmpfile.Name(), nil +} + func TestDisplayTrustRootInvalidFlags(t *testing.T) { // we need an actual PEMfile to test - tmpfile, err := ioutil.TempFile("", "pemfile") + tmpfile, err := writeFile(cert) assert.NilError(t, err) - defer os.Remove(tmpfile.Name()) - tmpfile.Write([]byte(` ------BEGIN CERTIFICATE----- -MIIBajCCARCgAwIBAgIUe0+jYWhxN8fFOByC7yveIYgvx1kwCgYIKoZIzj0EAwIw -EzERMA8GA1UEAxMIc3dhcm0tY2EwHhcNMTcwNjI3MTUxNDAwWhcNMzcwNjIyMTUx -NDAwWjATMREwDwYDVQQDEwhzd2FybS1jYTBZMBMGByqGSM49AgEGCCqGSM49AwEH -A0IABGgbOZLd7b4b262+6m4ignIecbAZKim6djNiIS1Kl5IHciXYn7gnSpsayjn7 -GQABpgkdPeM9TEQowmtR1qSnORujQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMB -Af8EBTADAQH/MB0GA1UdDgQWBBQ6Rtcn823/fxRZyheRDFpDzuBMpTAKBggqhkjO -PQQDAgNIADBFAiEAqD3Kb2rgsy6NoTk+zEgcUi/aGBCsvQDG3vML1PXN8j0CIBjj -4nDj+GmHXcnKa8wXx70Z8OZEpRQIiKDDLmcXuslp ------END CERTIFICATE----- -`)) - tmpfile.Close() + defer os.Remove(tmpfile) - errorTestCases := [][]string{ + errorTestCases := []invalidCATestCases{ { - "--ca-cert=" + tmpfile.Name(), + args: []string{"--ca-cert=" + tmpfile}, + errorMsg: "flag requires the `--rotate` flag to update the CA", }, { - "--ca-key=" + tmpfile.Name(), + args: []string{"--ca-key=" + tmpfile}, + errorMsg: "flag requires the `--rotate` flag to update the CA", }, { // to make sure we're not erroring because we didn't provide a CA key along with the CA cert - - "--ca-cert=" + tmpfile.Name(), - "--ca-key=" + tmpfile.Name(), + args: []string{ + "--ca-cert=" + tmpfile, + "--ca-key=" + tmpfile, + }, + errorMsg: "flag requires the `--rotate` flag to update the CA", }, { - "--cert-expiry=2160h0m0s", + args: []string{"--cert-expiry=2160h0m0s"}, + errorMsg: "flag requires the `--rotate` flag to update the CA", }, { - "--external-ca=protocol=cfssl,url=https://some.com/https/url", + args: []string{"--external-ca=protocol=cfssl,url=https://some.com/https/url"}, + errorMsg: "flag requires the `--rotate` flag to update the CA", }, { // to make sure we're not erroring because we didn't provide a CA cert and external CA - - "--ca-cert=" + tmpfile.Name(), - "--external-ca=protocol=cfssl,url=https://some.com/https/url", + args: []string{ + "--ca-cert=" + tmpfile, + "--external-ca=protocol=cfssl,url=https://some.com/https/url", + }, + errorMsg: "flag requires the `--rotate` flag to update the CA", + }, + { + args: []string{ + "--rotate", + "--external-ca=protocol=cfssl,url=https://some.com/https/url", + }, + errorMsg: "rotating to an external CA requires the `--ca-cert` flag to specify the external CA's cert - " + + "to add an external CA with the current root CA certificate, use the `update` command instead", + }, + { + args: []string{ + "--rotate", + "--ca-cert=" + tmpfile, + }, + errorMsg: "the --ca-cert flag requires that a --ca-key flag and/or --external-ca flag be provided as well", }, } - for _, args := range errorTestCases { + for _, testCase := range errorTestCases { cmd := newCACommand( test.NewFakeCli(&fakeClient{ swarmInspectFunc: func() (swarm.Swarm, error) { @@ -94,9 +144,9 @@ PQQDAgNIADBFAiEAqD3Kb2rgsy6NoTk+zEgcUi/aGBCsvQDG3vML1PXN8j0CIBjj }, nil }, })) - assert.Check(t, cmd.Flags().Parse(args)) + assert.Check(t, cmd.Flags().Parse(testCase.args)) cmd.SetOutput(ioutil.Discard) - assert.ErrorContains(t, cmd.Execute(), "flag requires the `--rotate` flag to update the CA") + assert.ErrorContains(t, cmd.Execute(), testCase.errorMsg) } } @@ -112,43 +162,139 @@ func TestDisplayTrustRoot(t *testing.T) { assert.Check(t, is.Equal(trustRoot+"\n", buffer.String())) } +type swarmUpdateRecorder struct { + spec swarm.Spec +} + +func (s *swarmUpdateRecorder) swarmUpdate(sp swarm.Spec, _ swarm.UpdateFlags) error { + s.spec = sp + return nil +} + +func swarmInspectFuncWithFullCAConfig() (swarm.Swarm, error) { + return swarm.Swarm{ + ClusterInfo: swarm.ClusterInfo{ + Spec: *swarmSpecWithFullCAConfig(), + }, + }, nil +} + func TestUpdateSwarmSpecDefaultRotate(t *testing.T) { - spec := swarmSpecWithFullCAConfig() - flags := newCACommand(nil).Flags() - updateSwarmSpec(spec, flags, caOptions{}) + s := &swarmUpdateRecorder{} + cli := test.NewFakeCli(&fakeClient{ + swarmInspectFunc: swarmInspectFuncWithFullCAConfig, + swarmUpdateFunc: s.swarmUpdate, + }) + cmd := newCACommand(cli) + cmd.SetArgs([]string{"--rotate", "--detach"}) + cmd.SetOutput(cli.OutBuffer()) + assert.NilError(t, cmd.Execute()) expected := swarmSpecWithFullCAConfig() expected.CAConfig.ForceRotate = 2 expected.CAConfig.SigningCACert = "" expected.CAConfig.SigningCAKey = "" - assert.Check(t, is.DeepEqual(expected, spec)) + assert.Check(t, is.DeepEqual(*expected, s.spec)) } -func TestUpdateSwarmSpecPartial(t *testing.T) { - spec := swarmSpecWithFullCAConfig() - flags := newCACommand(nil).Flags() - updateSwarmSpec(spec, flags, caOptions{ - rootCACert: PEMFile{contents: "cacert"}, +func TestUpdateSwarmSpecCertAndKey(t *testing.T) { + certfile, err := writeFile(cert) + assert.NilError(t, err) + defer os.Remove(certfile) + + keyfile, err := writeFile(key) + assert.NilError(t, err) + defer os.Remove(keyfile) + + s := &swarmUpdateRecorder{} + cli := test.NewFakeCli(&fakeClient{ + swarmInspectFunc: swarmInspectFuncWithFullCAConfig, + swarmUpdateFunc: s.swarmUpdate, }) + cmd := newCACommand(cli) + cmd.SetArgs([]string{ + "--rotate", + "--detach", + "--ca-cert=" + certfile, + "--ca-key=" + keyfile, + "--cert-expiry=3m"}) + cmd.SetOutput(cli.OutBuffer()) + assert.NilError(t, cmd.Execute()) expected := swarmSpecWithFullCAConfig() - expected.CAConfig.SigningCACert = "cacert" - assert.Check(t, is.DeepEqual(expected, spec)) + expected.CAConfig.SigningCACert = cert + expected.CAConfig.SigningCAKey = key + expected.CAConfig.NodeCertExpiry = 3 * time.Minute + assert.Check(t, is.DeepEqual(*expected, s.spec)) } -func TestUpdateSwarmSpecFullFlags(t *testing.T) { - flags := newCACommand(nil).Flags() - flags.Lookup(flagCertExpiry).Changed = true - spec := swarmSpecWithFullCAConfig() - updateSwarmSpec(spec, flags, caOptions{ - rootCACert: PEMFile{contents: "cacert"}, - rootCAKey: PEMFile{contents: "cakey"}, - swarmCAOptions: swarmCAOptions{nodeCertExpiry: 3 * time.Minute}, +func TestUpdateSwarmSpecCertAndExternalCA(t *testing.T) { + certfile, err := writeFile(cert) + assert.NilError(t, err) + defer os.Remove(certfile) + + s := &swarmUpdateRecorder{} + cli := test.NewFakeCli(&fakeClient{ + swarmInspectFunc: swarmInspectFuncWithFullCAConfig, + swarmUpdateFunc: s.swarmUpdate, }) + cmd := newCACommand(cli) + cmd.SetArgs([]string{ + "--rotate", + "--detach", + "--ca-cert=" + certfile, + "--external-ca=protocol=cfssl,url=https://some.external.ca"}) + cmd.SetOutput(cli.OutBuffer()) + assert.NilError(t, cmd.Execute()) expected := swarmSpecWithFullCAConfig() - expected.CAConfig.SigningCACert = "cacert" - expected.CAConfig.SigningCAKey = "cakey" - expected.CAConfig.NodeCertExpiry = 3 * time.Minute - assert.Check(t, is.DeepEqual(expected, spec)) + expected.CAConfig.SigningCACert = cert + expected.CAConfig.SigningCAKey = "" + expected.CAConfig.ExternalCAs = []*swarm.ExternalCA{ + { + Protocol: swarm.ExternalCAProtocolCFSSL, + URL: "https://some.external.ca", + CACert: cert, + Options: make(map[string]string), + }, + } + assert.Check(t, is.DeepEqual(*expected, s.spec)) +} + +func TestUpdateSwarmSpecCertAndKeyAndExternalCA(t *testing.T) { + certfile, err := writeFile(cert) + assert.NilError(t, err) + defer os.Remove(certfile) + + keyfile, err := writeFile(key) + assert.NilError(t, err) + defer os.Remove(keyfile) + + s := &swarmUpdateRecorder{} + cli := test.NewFakeCli(&fakeClient{ + swarmInspectFunc: swarmInspectFuncWithFullCAConfig, + swarmUpdateFunc: s.swarmUpdate, + }) + cmd := newCACommand(cli) + cmd.SetArgs([]string{ + "--rotate", + "--detach", + "--ca-cert=" + certfile, + "--ca-key=" + keyfile, + "--external-ca=protocol=cfssl,url=https://some.external.ca"}) + cmd.SetOutput(cli.OutBuffer()) + assert.NilError(t, cmd.Execute()) + + expected := swarmSpecWithFullCAConfig() + expected.CAConfig.SigningCACert = cert + expected.CAConfig.SigningCAKey = key + expected.CAConfig.ExternalCAs = []*swarm.ExternalCA{ + { + Protocol: swarm.ExternalCAProtocolCFSSL, + URL: "https://some.external.ca", + CACert: cert, + Options: make(map[string]string), + }, + } + assert.Check(t, is.DeepEqual(*expected, s.spec)) } diff --git a/cli/command/swarm/opts.go b/cli/command/swarm/opts.go index b3baba273ea6..b2a4de1f692e 100644 --- a/cli/command/swarm/opts.go +++ b/cli/command/swarm/opts.go @@ -230,7 +230,7 @@ func addSwarmFlags(flags *pflag.FlagSet, opts *swarmOptions) { addSwarmCAFlags(flags, &opts.swarmCAOptions) } -func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet) { +func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet, caCert string) { if flags.Changed(flagTaskHistoryLimit) { spec.Orchestration.TaskHistoryRetentionLimit = &opts.taskHistoryLimit } @@ -246,7 +246,7 @@ func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet) if flags.Changed(flagAutolock) { spec.EncryptionConfig.AutoLockManagers = opts.autolock } - opts.mergeSwarmSpecCAFlags(spec, flags) + opts.mergeSwarmSpecCAFlags(spec, flags, caCert) } type swarmCAOptions struct { @@ -254,17 +254,20 @@ type swarmCAOptions struct { externalCA ExternalCAOption } -func (opts *swarmCAOptions) mergeSwarmSpecCAFlags(spec *swarm.Spec, flags *pflag.FlagSet) { +func (opts *swarmCAOptions) mergeSwarmSpecCAFlags(spec *swarm.Spec, flags *pflag.FlagSet, caCert string) { if flags.Changed(flagCertExpiry) { spec.CAConfig.NodeCertExpiry = opts.nodeCertExpiry } if flags.Changed(flagExternalCA) { spec.CAConfig.ExternalCAs = opts.externalCA.Value() + for _, ca := range spec.CAConfig.ExternalCAs { + ca.CACert = caCert + } } } func (opts *swarmOptions) ToSpec(flags *pflag.FlagSet) swarm.Spec { var spec swarm.Spec - opts.mergeSwarmSpec(&spec, flags) + opts.mergeSwarmSpec(&spec, flags, "") return spec } diff --git a/cli/command/swarm/update.go b/cli/command/swarm/update.go index 52dc335fdc54..6b9ad728f6cf 100644 --- a/cli/command/swarm/update.go +++ b/cli/command/swarm/update.go @@ -48,7 +48,7 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, opts swarmOptions) e prevAutoLock := swarmInspect.Spec.EncryptionConfig.AutoLockManagers - opts.mergeSwarmSpec(&swarmInspect.Spec, flags) + opts.mergeSwarmSpec(&swarmInspect.Spec, flags, swarmInspect.ClusterInfo.TLSInfo.TrustRoot) curAutoLock := swarmInspect.Spec.EncryptionConfig.AutoLockManagers diff --git a/cli/command/swarm/update_test.go b/cli/command/swarm/update_test.go index 256c9de6d0db..20a5624a897e 100644 --- a/cli/command/swarm/update_test.go +++ b/cli/command/swarm/update_test.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/swarm" "github.com/pkg/errors" + // Import builders to get the builder function as package function . "github.com/docker/cli/internal/test/builders" "gotest.tools/assert" @@ -82,6 +83,9 @@ func TestSwarmUpdateErrors(t *testing.T) { } func TestSwarmUpdate(t *testing.T) { + swarmInfo := Swarm() + swarmInfo.ClusterInfo.TLSInfo.TrustRoot = "trustroot" + testCases := []struct { name string args []string @@ -105,6 +109,9 @@ func TestSwarmUpdate(t *testing.T) { flagAutolock: "true", flagQuiet: "true", }, + swarmInspectFunc: func() (swarm.Swarm, error) { + return *swarmInfo, nil + }, swarmUpdateFunc: func(swarm swarm.Spec, flags swarm.UpdateFlags) error { if *swarm.Orchestration.TaskHistoryRetentionLimit != 10 { return errors.Errorf("historyLimit not correctly set") @@ -123,7 +130,7 @@ func TestSwarmUpdate(t *testing.T) { if swarm.CAConfig.NodeCertExpiry != certExpiryDuration { return errors.Errorf("certExpiry not correctly set") } - if len(swarm.CAConfig.ExternalCAs) != 1 { + if len(swarm.CAConfig.ExternalCAs) != 1 || swarm.CAConfig.ExternalCAs[0].CACert != "trustroot" { return errors.Errorf("externalCA not correctly set") } if *swarm.Raft.KeepOldSnapshots != 10 {