[manager/state] Add fernet as an option for raft encryption #2535

cyli · 2018-03-01T23:54:51Z

This PR does the following:

vendors https://github.com/fernet/fernet-go
adds a fernet encrypter and decrypter to raft encryption
sets it so that if FIPS is enabled, fernet is the only raft encryption/decryption mechanism. otherwise, swarm uses XSalsa20/Poly1305 from secretbox, but can decrypt both.

Possibly we want to avoid using an environment variable, and just have FIPS be a configuration value when initializing the swarm. However, this would make it more annoying to pass the fips vs non-fips bool through all the layers to the key management facilities as well as raft encryption.

I don't see a use case for going from non-FIPS -> FIPS, because that means that it's possible you have some old data lying around that is not FIPS compliant. Hence if FIPS is set, only fernet can be used, so if you had previous non-FIPS raft data, you would not be able to decrypt it.

But I made it possible for the default decrypter to decrypt both NACL Secretbox and Fernet, because possibly someone would want to go from FIPS -> non-FIPS? cc @endophage

Also I am happy to split this into two PRs - the vendoring+ fernet encrypter/decrypter, and then the actual usage of the fernet encrypter/decrypter.

codecov · 2018-03-02T00:33:50Z

Codecov Report

Merging #2535 into master will increase coverage by 0.06%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
+ Coverage   61.47%   61.53%   +0.06%     
==========================================
  Files         133      134       +1     
  Lines       21765    21800      +35     
==========================================
+ Hits        13379    13415      +36     
  Misses       6946     6946              
+ Partials     1440     1439       -1

aaronlehmann

Looks great.

cyli · 2018-03-08T21:00:48Z

@aaronlehmann Thanks for reviewing! I was thinking about actually removing the env var and propagating a FIPS boolean through the code, as per #2544. Unfortunately this means a large-ish change, but it also makes it easier to test mixed settings and run more tests in parallel.

cyli · 2018-03-19T17:35:26Z

In the interests of avoiding conflicts, I'm ok with merging this and removing the go env var in a different PR, rather than the reverse. So if anyone else can have a look, that'd be awesome. :) cc @anshulpundir @nishanttotla @dperny

dperny · 2018-03-19T17:46:54Z

I would be fine with removing the envvar in a different PR. I'm reviewing in depth now.

cyli · 2018-03-19T17:51:05Z

@dperny Thanks so much!

dperny

Mostly looks good, but a couple of comments.

dperny · 2018-03-19T17:53:06Z

manager/encryption/encryption.go

+}
+
+// MultiDecrypter is a decrypter that will attempts to decrypt with multiple decrypters
+type MultiDecrypter map[api.MaybeEncryptedRecord_Algorithm][]Decrypter


I think newtyping maps and then defining methods on them is... a recipe for confusion later on. I was already confused on line 97 when I saw us iterating over what seemed to me to be an object.

Is there any advantage to newtyping a map over defining a struct with a map field?

Also, I am unclear why we need a slice of Decrypters for each algorithm. It seems to me like mapping 1:1 algorithim->decrypter would be sufficient?

I agree with @dperny here. Would prefer to have a struct with a map here.

Also, I am unclear why we need a slice of Decrypters for each algorithm. It seems to me like mapping 1:1 algorithim->decrypter would be sufficient?

It's possible to have multiple keys per algorithm, if you have an old and new key. This is needed for instance when the encryption key is rotated, which happens when you go from unlocked (when the key was stored unencrypted in the TLS key header) to locked (when the key is encrypted in the TLS key header). Not everything may have finished porting over to use the new key, so some WALs will be encrypted with the old key, and some with the new. So 1 decrypter with the new key, 1 with the old, but same algorithm.

That makes perfect sense. Could you add a comment to that effect?

Done, and updated to a struct

dperny · 2018-03-19T17:55:23Z

manager/encryption/fernet.go

+	return frnt
+}
+
+// Algorithm returns the type of algorithm this is (NACL Secretbox using XSalsa20 and Poly1305)


Comment seems to be a carryover from the non-fips algo.

Yes, thanks for catching that!

nishanttotla · 2018-03-19T18:08:50Z

fips/fips.go

+
+// Enabled returns true when FIPS mode is enabled
+func Enabled() bool {
+	return os.Getenv(EnvVar) != ""


@cyli is it sufficient to just check this?

@nishanttotla The requirements were previously that the GOFIPS environment variable is set to something at all, but the current plan is to remove this particular environment variable checking support from swarmkit, and just propagate a boolean through. The caller of node/node.go will configure FIPS to be on or off - this should make it easier to test mixed environments in our integration tests.

It's just a big-ish change, so I was going to do it in a separate PR :|

nishanttotla · 2018-03-19T18:10:21Z

manager/encryption/encryption.go

+	Algorithm() api.MaybeEncryptedRecord_Algorithm
+}
+
+// MultiDecrypter is a decrypter that will attempts to decrypt with multiple decrypters


nit: attempts -> attempt

Fixed, thanks!

dperny · 2018-03-20T19:59:27Z

LGTM!

Signed-off-by: cyli <cyli@twistedmatrix.com>

…round constructing one. Also make it a map instead of a list so that as available algorithms increase not every single algorithm needs to be tried to decrypt. Signed-off-by: Ying Li <ying.li@docker.com>

…eck it from the encryption package to determine the encryption defaults. Signed-off-by: Ying Li <ying.li@docker.com>

cyli · 2018-03-21T18:52:31Z

Thanks @dperny!

cyli force-pushed the fernet-encryption branch 2 times, most recently from e81dd14 to 541bc19 Compare March 2, 2018 00:23

cyli changed the title ~~WIP: Add fernet as an option for raft encryption~~ WIP: [manager/state] Add fernet as an option for raft encryption Mar 2, 2018

cyli force-pushed the fernet-encryption branch from 541bc19 to bd74157 Compare March 2, 2018 00:33

cyli force-pushed the fernet-encryption branch from bd74157 to 3ef27f6 Compare March 2, 2018 00:47

cyli mentioned this pull request Mar 8, 2018

FIPS support design #2544

Closed

2 tasks

marcusmartins requested review from aaronlehmann and removed request for aaronlehmann March 8, 2018 05:02

aaronlehmann approved these changes Mar 8, 2018

View reviewed changes

cyli changed the title ~~WIP: [manager/state] Add fernet as an option for raft encryption~~ [manager/state] Add fernet as an option for raft encryption Mar 19, 2018

dperny requested changes Mar 19, 2018

View reviewed changes

nishanttotla reviewed Mar 19, 2018

View reviewed changes

cyli force-pushed the fernet-encryption branch 2 times, most recently from be33611 to 37667b9 Compare March 20, 2018 17:09

dperny approved these changes Mar 20, 2018

View reviewed changes

cyli and others added 4 commits March 21, 2018 10:38

Vendor in fernet-go to provide AES-128-CBC

0f21845

Signed-off-by: cyli <cyli@twistedmatrix.com>

Add fernet encryption mechanism for raft WAL and snapshots.

25c6575

Signed-off-by: cyli <cyli@twistedmatrix.com>

Move MultiDecrypter to encryption package and provide some niceties a…

34ac12e

…round constructing one. Also make it a map instead of a list so that as available algorithms increase not every single algorithm needs to be tried to decrypt. Signed-off-by: Ying Li <ying.li@docker.com>

Move fips environment variable check to its own package, so we can ch…

899202e

…eck it from the encryption package to determine the encryption defaults. Signed-off-by: Ying Li <ying.li@docker.com>

cyli force-pushed the fernet-encryption branch from 37667b9 to 899202e Compare March 21, 2018 17:39

dperny merged commit 3336081 into moby:master Mar 21, 2018

cyli deleted the fernet-encryption branch March 21, 2018 18:52

thaJeztah mentioned this pull request Mar 26, 2018

Bump SwarmKit to 831df679a0b8a21b4dccd5791667d030642de7ff moby/moby#36701

Merged

thaJeztah mentioned this pull request Apr 19, 2018

[18.03] update swarmkit docker-archive/docker-ce#521

Closed

cyli mentioned this pull request May 7, 2018

[18.03] FIPS #2625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[manager/state] Add fernet as an option for raft encryption #2535

[manager/state] Add fernet as an option for raft encryption #2535

cyli commented Mar 1, 2018 •

edited

Loading

codecov bot commented Mar 2, 2018 •

edited

Loading

aaronlehmann left a comment

cyli commented Mar 8, 2018 •

edited

Loading

cyli commented Mar 19, 2018

dperny commented Mar 19, 2018

cyli commented Mar 19, 2018

dperny left a comment

dperny Mar 19, 2018

dperny Mar 19, 2018

nishanttotla Mar 19, 2018

cyli Mar 19, 2018

dperny Mar 19, 2018

cyli Mar 20, 2018

dperny Mar 19, 2018

cyli Mar 20, 2018

nishanttotla Mar 19, 2018

cyli Mar 19, 2018

nishanttotla Mar 19, 2018

cyli Mar 20, 2018

dperny commented Mar 20, 2018

cyli commented Mar 21, 2018

[manager/state] Add fernet as an option for raft encryption #2535

[manager/state] Add fernet as an option for raft encryption #2535

Conversation

cyli commented Mar 1, 2018 • edited Loading

codecov bot commented Mar 2, 2018 • edited Loading

Codecov Report

aaronlehmann left a comment

Choose a reason for hiding this comment

cyli commented Mar 8, 2018 • edited Loading

cyli commented Mar 19, 2018

dperny commented Mar 19, 2018

cyli commented Mar 19, 2018

dperny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Mar 20, 2018

cyli commented Mar 21, 2018

cyli commented Mar 1, 2018 •

edited

Loading

codecov bot commented Mar 2, 2018 •

edited

Loading

cyli commented Mar 8, 2018 •

edited

Loading