API changes to rotate swarm root CA by cyli · Pull Request #32993 · moby/moby

cyli · 2017-05-03T17:40:15Z

~~This is stacked on top of #32875.~~ Merged.

This is the first stab the CLI command to actually rotate the root CA cert. I added a new one, because we want to optionally take a root certificate and key, and/or external CAs, but we also want the user to be able to tell swarm to generate one. So:

root@ubuntu:~#  docker swarm ca
-----BEGIN CERTIFICATE-----
MIIBazCCARCgAwIBAgIUJPzo67QC7g8Ebg2ansjkZ8CbmaswCgYIKoZIzj0EAwIw
EzERMA8GA1UEAxMIc3dhcm0tY2EwHhcNMTcwNTAzMTcxMDAwWhcNMzcwNDI4MTcx
MDAwWjATMREwDwYDVQQDEwhzd2FybS1jYTBZMBMGByqGSM49AgEGCCqGSM49AwEH
A0IABKL6/C0sihYEb935wVPRA8MqzPLn3jzou0OJRXHsCLcVExigrMdgmLCC+Va4
+sJ+SLVO1eQbvLHH8uuDdF/QOU6jQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMB
Af8EBTADAQH/MB0GA1UdDgQWBBSfUy5bjUnBAx/B0GkOBKp91XvxzjAKBggqhkjO
PQQDAgNJADBGAiEAnbvh0puOS5R/qvy1PMHY1iksYKh2acsGLtL/jAIvO4ACIQCi
lIwQqLkJ48SQqCjG1DBTSBsHmMSRT+6mE2My+Z3GKA==
-----END CERTIFICATE-----

root@ubuntu:~# docker swarm ca --help

Usage:	docker swarm ca [OPTIONS]

Manage root CA

Options:
      --ca-cert pem-file          Path to the PEM-formatted root CA certificate to use for the new cluster
      --ca-key pem-file           Path to the PEM-formatted root CA key to use for the new cluster
      --cert-expiry duration      Validity period for node certificates (ns|us|ms|s|m|h) (default 2160h0m0s)
  -d, --detach                    Exit immediately instead of waiting for the root rotation to converge
      --external-ca external-ca   Specifications of one or more certificate signing endpoints
      --help                      Print usage
  -q, --quiet                     Suppress progress output
      --rotate                    Rotate the swarm CA - if no certificate or key are provided, new ones will be generated

Sample progress bar here, copied styling from the synchronous service create progress bar: https://asciinema.org/a/dcpy5y2fcueb8fpmokxvf3vec

If the design looks good, I'll add a bunch more tests and update the docs. cc @aaronlehmann @aluzzardi @diogomonica

Note: I originally thought about just doing docker swarm update --rotate-ca cert=/path/...,key=/path/... but I wasn't sure how to also do tell the CLI to just generate a cert and key - it'd probably have to look like docker swarm update --rotate-ca "" which seems weird, hence the new command.

Also a suggestion from @NathanMcCauley: if we have service identities, and those could be rotated, do we want to use the same command to view/rotate those as well?

aaronlehmann · 2017-05-03T18:24:13Z

daemon/cluster/convert/swarm.go

aaronlehmann · 2017-05-03T18:45:11Z

cli/command/swarm/progress/root_rotation.go

I'm sure there's a good reason, but why are there a different number of spaces before these?

I was trying to right-align the text, since they appear next to the progress bars. Happy to left-align instead, though.

Right now the output looks like this:

desired root digest: sha256:a594409c21f2339d4fa5a207549e500574d6a9872dcd040f4e63e461aa969302 nodes rotated TLS certificates: 1/1 done [==================================================>] nodes rotated CA certificates: 1/1 done [==================================================>]

I have a slight preference for:

desired root digest: sha256:a594409c21f2339d4fa5a207549e500574d6a9872dcd040f4e63e461aa969302 nodes rotated TLS certificates: 1/1 done [==================================================>] nodes rotated CA certificates: 1/1 done [==================================================>]

But this is not important

I am indifferent, so happy to change it :)

Updated, along with the asciinema recording

aaronlehmann · 2017-05-03T18:46:14Z

cli/command/swarm/progress/root_rotation.go

Note streamformatter was refactored and this line will need to be updated.

Thanks, I wouldn't have noticed that! :) Updated.

aaronlehmann · 2017-05-03T18:49:27Z

cli/command/swarm/progress/root_rotation.go

Is it necessary to wait like this after the root rotation? For a service create or service update, a very common failure mode is that tasks briefly reach the Running state, but immediately fail due to a misconfiguration, or even something like specifying an entrypoint that doesn't exist in the image. Is there any equivalent with root rotation, or would this only cover a new root rotation starting right after the old one finishes?

@aaronlehmann It was only to cover another root rotation - agree it's probably not necessary though.

I would suggest removing this phase. It would simplify the code and be a little more user-friendly.

aaronlehmann · 2017-05-03T22:26:57Z

pkg/jsonmessage/jsonmessage.go

Looks like something done by the editor

:( yeah it's picky. Sorry about that!

aaronlehmann · 2017-05-04T01:24:47Z

CI faliing:

22:42:42 --- FAIL: TestProgress (0.00s)
22:42:42 panic: runtime error: makeslice: len out of range [recovered]
22:42:42 	panic: runtime error: makeslice: len out of range

aaronlehmann · 2017-05-04T23:55:50Z

cli/command/swarm/progress/root_rotation.go

The lazy initialization doesn't seem necessary. Normally I'd say to just define an updater outside the for loop. However, I'm not sure the abstraction is justified here. In service create/update, there are two implementations. Here, there is only one implementation, so it seems like it could just be a function call updateProgress or something.done should become a return value. As for initialized, you could just move those initial progress.Update calls to happen before the for loop in this function.

aaronlehmann · 2017-05-04T23:56:58Z

Design LGTM

diogomonica · 2017-05-08T17:41:23Z

LGTM, still playing with it.

cyli · 2017-05-08T21:52:04Z

I've split out the CLI code to docker/cli#48.

mlaventure · 2017-05-09T22:28:25Z

@cyli

00:15:37 These files are not properly gofmt'd:
00:15:37  - integration-cli/docker_api_swarm_test.go
00:15:37 
00:15:37 Please reformat the above files using "gofmt -s -w" and commit the result.

cyli · 2017-05-10T06:38:37Z

@mlaventure Thank you - fixed.

cyli · 2017-05-10T21:44:57Z

Also updated the api version history here. cc @thaJeztah if you wouldn't mind checking the docs here as well :)

mlaventure · 2017-05-10T22:03:18Z

@cyli it looks like the cli revision is invalid now

aaronlehmann · 2017-05-10T22:10:13Z

That was my branch, which I had to rebase to get merged and allow moby to switch back to the official repo. I've restored that revision. A simple rebuild should work.

cyli · 2017-05-10T22:28:44Z

Thanks @aaronlehmann!

in the Docker REST APIs when viewing or updating the swarm spec info, and also propagate the desired CA key in the Docker REST APIs when updating swarm spec info only (it is not available for viewing). Signed-off-by: Ying Li <ying.li@docker.com>

cyli · 2017-05-11T17:23:28Z

Rebased again. PTAL.

Signed-off-by: Ying Li <ying.li@docker.com>

aaronlehmann · 2017-05-11T18:35:03Z

LGTM

cyli · 2017-05-12T00:12:22Z

Also pinging @cpuguy83 for review: since you reviewed the last API changes that included TLS info (sorry, no good deed goes unpunished :)) - this is the follow up that will trigger the root rotation that can make use of the TLS info.

Alternately @vdemeester or @tiborvass or may or may not be familiar with the feature.

cpuguy83

LGTM

cyli · 2017-05-12T17:23:33Z

Thanks @cpuguy83 and @aaronlehmann! :)

GordonTheTurtle added the status/0-triage label May 3, 2017

aaronlehmann added status/1-design-review and removed status/0-triage labels May 3, 2017

aaronlehmann reviewed May 3, 2017

View reviewed changes

cyli mentioned this pull request May 3, 2017

[feature] CA Root rotation moby/swarmkit#1990

Closed

10 tasks

aaronlehmann reviewed May 3, 2017

View reviewed changes

aaronlehmann added the status/failing-ci Indicates that the PR in its current state fails the test suite label May 4, 2017

cyli mentioned this pull request May 4, 2017

Propagate TLS Info in swarm info and node info REST endpoints #32875

Merged

aaronlehmann removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 4, 2017

aaronlehmann reviewed May 4, 2017

View reviewed changes

thaJeztah added the impact/cli label May 5, 2017

thaJeztah added this to the 17.06.0 milestone May 5, 2017

cyli mentioned this pull request May 8, 2017

Synchronous CLI command for root CA rotation docker/cli#48

Merged

cyli changed the title ~~WIP: Synchronous root rotation CLI command~~ WIP: API to rotate swarm root CA May 9, 2017

aaronlehmann added the rebuild/* label May 10, 2017

GordonTheTurtle removed the rebuild/* label May 10, 2017

cyli mentioned this pull request May 10, 2017

Root CA rotation CLI docs #33152

Merged

cyli changed the title ~~WIP: API to rotate swarm root CA~~ API changes to rotate swarm root CA May 10, 2017

cyli added 2 commits May 11, 2017 11:13

Update the stream formatter to display custom unit numbers.

a771c16

Signed-off-by: Ying Li <ying.li@docker.com>

Add API test to rotate the swarm CA certificate

376c75d

Signed-off-by: Ying Li <ying.li@docker.com>

aaronlehmann added status/2-code-review and removed status/1-design-review labels May 12, 2017

aaronlehmann requested review from thaJeztah and vdemeester May 12, 2017 00:04

cpuguy83 approved these changes May 12, 2017

View reviewed changes

aaronlehmann merged commit eb8abc9 into moby:master May 12, 2017

cyli deleted the root-rotation-cli branch May 12, 2017 17:23

thaJeztah added impact/api impact/changelog and removed impact/cli labels May 15, 2017

mlaventure mentioned this pull request Jun 6, 2017

Update 17.06 CHANGELOG with rc2 changes docker-archive/docker-ce#40

Merged

Conversation

cyli commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 4, 2017

Uh oh!

diogomonica commented May 8, 2017

Uh oh!

cyli commented May 8, 2017

Uh oh!

mlaventure commented May 9, 2017

Uh oh!

cyli commented May 10, 2017

Uh oh!

cyli commented May 10, 2017

Uh oh!

mlaventure commented May 10, 2017

Uh oh!

aaronlehmann commented May 10, 2017

Uh oh!

cyli commented May 10, 2017

Uh oh!

cyli commented May 11, 2017

Uh oh!

aaronlehmann commented May 11, 2017

Uh oh!

cyli commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cyli commented May 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cyli commented May 3, 2017 •

edited

Loading

cyli May 3, 2017 •

edited

Loading

cyli commented May 12, 2017 •

edited

Loading