Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fips] Propagate the FIPS boolean to the manager, raft storage layer, and raft DEK management #2586

Merged
merged 2 commits into from
May 7, 2018
Merged

[fips] Propagate the FIPS boolean to the manager, raft storage layer, and raft DEK management #2586

merged 2 commits into from
May 7, 2018

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Apr 3, 2018 •
edited
Loading

This PR is based on #2562, and propagates the FIPS boolean to other parts of the manager that need it. This also documents the KeyReadWriter and RaftDEKManager better, so that reviewers can understand how they interact.

To view these changes alone, please see https://github.com/docker/swarmkit/pull/2586/files/b2da71c62612e2109d563fa05a764284caf0ec74..7003b358d8051bce2615d2ae5cd1e30c585250b4. (no need, #2562 was merged)

The code changes are actually fairly small - most of the change is comments and a prose doc about raft encryption. There are also some test changes to make sure that the fips boolean is handled properly, and a refactor to make one particular test more readable (table-driven).

Docs for how raft encryption works, which may help reviewers, has been cherry-picked into #2622

@codecov
Copy link

codecov bot commented Apr 3, 2018 •
edited
Loading

Codecov Report

Merging #2586 into master will decrease coverage by 5.5%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2586      +/-   ##
==========================================
- Coverage   67.34%   61.84%   -5.51%     
==========================================
  Files          98      134      +36     
  Lines       14554    21829    +7275     
==========================================
+ Hits         9802    13500    +3698     
- Misses       3743     6892    +3149     
- Partials     1009     1437     +428

@cyli cyli changed the title WIP: [fips] Propagate the FIPS boolean to the manager, raft storage layer, and raft DEK management [fips] Propagate the FIPS boolean to the manager, raft storage layer, and raft DEK management Apr 30, 2018
@cyli
Copy link
Contributor Author

cyli commented Apr 30, 2018 •
edited
Loading

Rebased! The first commit is the main change, and a lot of it is a test refactor.

The other two changes are:

  1. a lot more documentation for raft encryption, so that reviewers can understand the first commit
  2. propagating the FIPS var from the node to the manager

This should be ready for review now.

@cyli cyli mentioned this pull request Apr 30, 2018
Merged
@cyli cyli mentioned this pull request May 3, 2018
Merged
anshulpundir
anshulpundir reviewed May 4, 2018
View reviewed changes
Copy link
Contributor

@anshulpundir anshulpundir left a comment

Choose a reason for hiding this comment

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

Minor comments. Didn't review test code yet btw.

manager/deks.go
@@ -112,6 +118,7 @@ func compareKEKs(oldKEK, candidateKEK ca.KEKData) (bool, bool, error) {
type RaftDEKManager struct {
kw ca.KeyWriter
rotationCh chan struct{}
FIPS bool
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a comment on what this flag indicates.

manager/deks.go
@@ -156,8 +165,9 @@ func (r *RaftDEKManager) NeedsRotation() bool {
}

// GetKeys returns the current set of DEKs. If NeedsRotation is true, and there
// is no existing PendingDEK, it will try to create one. If there are any errors
// doing so, just return the original.
// is no existing PendingDEK, it will try to create one. If it successfully creates
Copy link
Contributor

Choose a reason for hiding this comment

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

The name 'GetKeys' implies a read-only operation so I would suggest avoid doing operations within this function that have side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to revisit this at a later point, but that's probably beyond the scope of this PR.

manager/manager.go Outdated
@@ -121,6 +121,9 @@ type Config struct {

// PluginGetter provides access to docker's plugin inventory.
PluginGetter plugingetter.PluginGetter

// FIPS specifies whether the manager should run in FIPS-compliant mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does it make sense to say that the mode is specified during startup and can't be changed ?

@dperny
Copy link
Collaborator

dperny commented May 4, 2018

To confirm, the diff on the test is only huge because it basically takes every test and wraps them in a loop to check both fips and non-fips, right?

LGTM.

@cyli
Copy link
Contributor Author

cyli commented May 4, 2018

@dperny Yes pretty much. For TestRaftDEKManagerNeedsRotateGetKeys I also refactored it to be a table-driven test, but wrapped it in a loop to test both FIPS and non-FIPS as well.

@cyli
Copy link
Contributor Author

cyli commented May 4, 2018 •
edited
Loading

Thanks @anshulpundir and @dperny. I've think I've addressed @anshulpundir's comments, except for https://github.com/docker/swarmkit/pull/2586/files/04230609c0ddb58a391bfa9defb837f6ac532299#diff-c837f367ebb298ae3e59a7a95ab64429, since that'd be a pretty big change and we might want to do that in another PR. I'd also have to think about it some more - it's mutating to avoid having to do a bunch of extra rotations.

@anshulpundir
Copy link
Contributor

LGTM

@dperny
Copy link
Collaborator

dperny commented May 7, 2018

Needs a rebase, then LGTM

@cyli
Propagate the FIPS bool to the raft DEK manager so that the DEK is en…
905d35b 
…crypted

using fernet.

Signed-off-by: Ying Li <ying.li@docker.com>
@cyli cyli force-pushed the fips-in-raft branch 3 times, most recently from cfd0d07 to 14b04a2 Compare May 7, 2018 18:26
@cyli
Propagate the FIPS boolean from node.go to the manager and hence to t…
ba11e51 
…he raft

storage layer.  Also propagate it to the RaftDEKData objects in node.go
and to the RaftDEKManager in the manager.

Signed-off-by: Ying Li <ying.li@docker.com>
@cyli cyli merged commit 8aa9c33 into moby:master May 7, 2018
@cyli cyli deleted the fips-in-raft branch May 7, 2018 19:07
@cyli cyli mentioned this pull request May 7, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request May 14, 2018
Closed
vdemeester pushed a commit to thaJeztah/docker that referenced this pull request Jun 5, 2018
@thaJeztah @vdemeester
Bump swarmkit to 8aa9c33
bc43ee3 
Changes included:

- moby/swarmkit#2594 [fips] Support a flag that indicates that the cluster requires mandatory FIPS mode on all nodes
- moby/swarmkit#2586 [fips] Propagate the FIPS boolean to the manager, raft storage layer, and raft DEK management

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@cyli cyli mentioned this pull request Jun 7, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anshulpundir anshulpundir anshulpundir left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cyli @dperny @anshulpundir