Encryption at rest of manager keys and raft data by aaronlehmann · Pull Request #27967 · moby/moby

aaronlehmann · 2016-11-01T22:06:24Z

This implements the Docker side of moby/swarmkit#1598. Builds on earlier work by @tonistiigi.

It's feature-complete but needs docs and some more testing.

thaJeztah · 2016-11-03T02:23:51Z

aaronlehmann · 2016-11-03T02:26:45Z

Gah! It's not ready to merge. Super annoying how CI forces a merge against master and then falls over with compilation errors when there are conflicts.

aaronlehmann · 2016-11-04T17:26:56Z

ping @dnephin @thaJeztah @aluzzardi - it would be great to get some early design review

aluzzardi · 2016-11-07T21:03:09Z

api/server/router/swarm/cluster.go

Is this route supposed to return the unlock key or to rotate it?

Is this route supposed to return the unlock key or to rotate it?

Return the unlock key

aluzzardi · 2016-11-07T21:07:03Z

hack/vendor.sh

aluzzardi · 2016-11-07T21:10:46Z

What's the overall workflow UX-wise?

What happens while the swarm in locked?

aaronlehmann · 2016-11-07T21:31:16Z

What's the overall workflow UX-wise?

docker swarm init --autolock or docker swarm update --autolock to turn on manager locking. This prints out a key.

Then on restart docker swarm unlock is necessary to start the manager. This takes the key on stdin.

You can retrieve the current key with docker swarm unlock-key, or rotate it with docker swarm unlock-key --rotate.

What happens while the swarm in locked?

The swarm components do not operate until you run docker swarm unlock.

vieux · 2016-11-08T02:21:32Z

integration-cli/docker_cli_swarm_test.go

this is left here on purpose ?

aluzzardi · 2016-11-08T21:12:05Z

This needs help reviewing, specifically the UX workflow (see @aaronlehmann comment above).

Some comments:

autolock seems weird - there is no lock command, so it's not obvious what we're automating. autounlock would maybe make more sense? It's symmetric with docker swarm unlock. /cc @dnephin
Exposing the CA service and having the Engine talk to it seems weird. We should not talk to anything else but the Control API IMHO. Perhaps we should just return the unlock key in inspect and remove the unlockkey endpoint altogether?
Consistency: We redact sensitive data from the API (such as secret data), yet we allow retrieving the unlockkey. Does this constitute a security issue? /cc @diogomonica

/cc @icecrime @thaJeztah @vieux @tiborvass

tonistiigi · 2016-11-08T21:18:24Z

Tested and reviewed this and design LGTM. Previous version had a way to specify lock key and never returned it from remote api. I think it was changed for ease of use and I don't really have a preference about which is better. Current behavior is consistent with join-tokens.

The behavior of the down nodes after rotation/autolock change needs to be clearly documented as it isn't obvious.

@aaronlehmann Maybe in follow-up we could run all swarm tests with encryption enabled. Even if it requires an env switch it would give much more coverage.

aluzzardi · 2016-11-09T18:38:23Z

@icecrime @thaJeztah @vieux @tiborvass @diogomonica: Any thoughts regarding the comments above?

This is getting merged today, feedback much appreciated.

vieux · 2016-11-09T19:48:40Z

@aluzzardi --autolock seems ok to me, I don't have a better name

@aaronlehmann could you add API and CLI reference doc ?

Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Tonis Tiigi <[email protected]>

- Neither swarm init or swarm update should take an unlock key - Add an autolock flag to turn on autolock - Make the necessary docker api changes - Add SwarmGetUnlockKey API call and use it when turning on autolock - Add swarm unlock-key subcommand Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Aaron Lehmann <[email protected]>

thaJeztah · 2016-11-10T00:11:12Z

docs/reference/api/docker_remote_api_v1.25.md

for a follow up; remove the trailing comma, and add a comma after "CAConfig": {}

@thaJeztah: done

Signed-off-by: Aaron Lehmann <[email protected]>

thaJeztah · 2016-11-10T00:13:18Z

docs/reference/commandline/swarm_update.md

for follow-up: add bool here

edit: realized that Cobra won't output booleans, so we probably need to include it in the description)

@thaJeztah: bool doesn't appear in the --help output. Should I add it anyway.

Just realized that, hm probably in the comment I guess

thaJeztah · 2016-11-10T00:13:35Z

docs/reference/commandline/swarm_init.md

same here (bool)

also needs to mention the default if omitted (actually, if default is "disabled", we can change this description to just "Enable manager auto locking ..."

@thaJeztah: Can you suggest a phrasing for this? --autolock or --autolock=true will enable it, and --autolock=false will disable it (in cases where it was previously enabled).

This one is for init, so it can only "enable" it, so this can be simply;

Enable manager autolocking (requiring an unlock key to start a stopped manager)

aaronlehmann · 2016-11-10T02:21:19Z

Options:
--advertise-addr value Advertised address (format: <ip|interface>[:port])
 --autolock                        Enable or disable manager autolocking (requiring an unlock key to start a stopped manager)
This one is for init, so it can only "enable" it, so this can be simply;
Enable manager autolocking (requiring an unlock key to start a stopped manager)

The flag is shared between init and update right now. Do you think the phrasing of the description should be different for these connands?

aaronlehmann · 2016-11-10T03:13:57Z

Saw this failure in CI running on an unrelated PR:

02:44:57 ----------------------------------------------------------------------
02:44:57 FAIL: docker_cli_swarm_test.go:952: DockerSwarmSuite.TestSwarmRotateUnlockKey
02:44:57 
02:44:57 [d761dbbbf650b] waiting for daemon to start
02:44:57 [d761dbbbf650b] daemon started
02:44:57 [d761dbbbf650b] exiting daemon
02:44:57 [d761dbbbf650b] waiting for daemon to start
02:44:57 [d761dbbbf650b] daemon started
02:44:57 [d761dbbbf650b] exiting daemon
02:44:57 [d761dbbbf650b] waiting for daemon to start
02:44:57 [d761dbbbf650b] daemon started
02:44:57 docker_cli_swarm_test.go:1020:
02:44:57     c.Assert(err, checker.IsNil, check.Commentf("out: %v", string(out)))
02:44:57 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc421b583c0), Stderr:[]uint8(nil)} ("exit status 1")
02:44:57 ... out: Error response from daemon: swarm could not be unlocked: invalid key provided
02:44:57 
02:44:57 
02:44:57 [d761dbbbf650b] exiting daemon

I'll investigate this and provide a fix.

aaronlehmann added the status/needs-vendoring label Nov 1, 2016

aaronlehmann added this to the 1.13.0 milestone Nov 1, 2016

GordonTheTurtle added the status/0-triage label Nov 1, 2016

aaronlehmann force-pushed the encryption branch from de2d3bc to 012bc3f Compare November 1, 2016 22:06

aaronlehmann added area/swarm and removed status/0-triage labels Nov 1, 2016

aaronlehmann force-pushed the encryption branch from 012bc3f to 4d5c70b Compare November 1, 2016 22:54

thaJeztah added the status/1-design-review label Nov 3, 2016

thaJeztah added the status/needs-attention Calls for a collective discussion during a review session label Nov 4, 2016

aaronlehmann force-pushed the encryption branch 4 times, most recently from a787543 to 99b647e Compare November 5, 2016 01:16

aluzzardi reviewed Nov 7, 2016

View reviewed changes

aaronlehmann force-pushed the encryption branch from 99b647e to 148d217 Compare November 7, 2016 21:32

vieux reviewed Nov 8, 2016

View reviewed changes

integration-cli/docker_cli_swarm_test.go Outdated

Copy link

Contributor

vieux Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is left here on purpose ?

aaronlehmann force-pushed the encryption branch 2 times, most recently from f93d83a to d9916ee Compare November 8, 2016 18:52

tonistiigi mentioned this pull request Nov 9, 2016

Daemon can't be started after swarm certificates expire #24132

Closed

aaronlehmann force-pushed the encryption branch from d9916ee to 5f5cf7b Compare November 9, 2016 18:48

tonistiigi and others added 7 commits November 9, 2016 16:08

vendor: update swarmkit to bddd3f0

2a68f0f

Signed-off-by: Aaron Lehmann <[email protected]>

vendor: update errors to 839d9e91

472ecae

Signed-off-by: Tonis Tiigi <[email protected]>

Add support for swarm init lock and swarm unlock

b4a667c

Signed-off-by: Tonis Tiigi <[email protected]>

Add integration tests for encrypted swarm

8b1f72a

Signed-off-by: Tonis Tiigi <[email protected]>

Add unlock key rotation

a6030a5

Signed-off-by: Aaron Lehmann <[email protected]>

Lock/unlock integration tests

c3c70c4

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the encryption branch from 67ed7ef to 498cd47 Compare November 10, 2016 00:09

thaJeztah reviewed Nov 10, 2016

View reviewed changes

Document autolock/unlock/unlock-key

824db2c

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the encryption branch from 498cd47 to 824db2c Compare November 10, 2016 00:12

thaJeztah reviewed Nov 10, 2016

View reviewed changes

thaJeztah added docs/revisit impact/api impact/changelog impact/cli impact/documentation and removed status/needs-attention Calls for a collective discussion during a review session labels Nov 10, 2016

This was referenced Nov 10, 2016

Add plugin create functionality. #28164

Merged

opts/mount: add tmpfs-specific options #28150

Merged

vieux merged commit 18a07f5 into moby:master Nov 10, 2016

aaronlehmann deleted the encryption branch November 10, 2016 01:49

aaronlehmann mentioned this pull request Nov 10, 2016

Pin images by digest #28173

Merged

thaJeztah mentioned this pull request Nov 10, 2016

Flaky test: DockerSwarmSuite.TestSwarmRotateUnlockKey #28240

Closed

mdlinville mentioned this pull request Nov 11, 2016

Document at-rest encryption for swarm encryption keys docker/docs#548

Closed

This was referenced Nov 22, 2016

Document locking and unlocking of swarm managers docker/docs#689

Closed

Document locking and unlocking swarm managers docker/docs#694

Merged

Conversation

aaronlehmann commented Nov 1, 2016

Uh oh!

thaJeztah commented Nov 3, 2016

Uh oh!

aaronlehmann commented Nov 3, 2016

Uh oh!

aaronlehmann commented Nov 4, 2016

Uh oh!

aluzzardi Nov 7, 2016 • 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!

aluzzardi commented Nov 7, 2016

Uh oh!

aaronlehmann commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi commented Nov 8, 2016

Uh oh!

tonistiigi commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aluzzardi commented Nov 9, 2016

Uh oh!

vieux commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 10, 2016 • 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!

thaJeztah Nov 10, 2016 • 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!

aaronlehmann commented Nov 10, 2016

Uh oh!

aaronlehmann commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aluzzardi Nov 7, 2016 •

edited

Loading

tonistiigi commented Nov 8, 2016 •

edited

Loading

thaJeztah Nov 10, 2016 •

edited

Loading

thaJeztah Nov 10, 2016 •

edited

Loading