[fips] Remove the GOFIPS env var check#2562
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2562 +/- ##
==========================================
+ Coverage 61.58% 61.77% +0.19%
==========================================
Files 134 134
Lines 21763 21770 +7
==========================================
+ Hits 13403 13449 +46
+ Misses 6918 6873 -45
- Partials 1442 1448 +6 |
FIPSity, IMHO. Is the idea here that the FIPS configuration variable on the node object will be passed in from the caller (in the downstream case, from the docker engine)? |
Oh nice, I didn't think of that one.
@dperny Yep, so it's only checked in one place, rather than us checking it separately from the docker engine. |
|
Does it not work to store in/read this setting out of the raft store because we need to know if we're FIPS or not before we try reading the raft store? |
The FIPS setting in the raft store specifies whether the cluster requires mandatory FIPS. It's possible to disable FIPS mode for a single node - we want to enforce that a node that is part of a cluster that requires FIPS will not start up swarm components unless the engine is in FIPS-enabled mode. (So users won't be able to get a mixed cluster by accident, if the whole cluster is set to FIPS). We could read the cluster setting, check it against our own, and then shut down. But that only works for managers - the workers don't have access to a raft store. I was previously thinking the dispatcher could just reject the agents who self-report as non-FIPS, but it seems better to shut down early if possible with a nice error message. This also will also mean we don't have to change dispatcher session behavior. But also, we need the manager node to take a configuration value because currently that is how we determine whether the cluster requires mandatory FIPS - is if the first swarm node is FIPS or not. |
9d8ba87 to
9bd36e9
Compare
570d3c1 to
b2da71c
Compare
|
LGTM. |
…g requires FIPS:
(1) require that users of the keyutil package instead use a key formatter object,
which could either be the default non-FIPS utility or the FIPS utility.
(2) require that users that request encryption defaults specify whether FIPS compliance
is needed
Signed-off-by: Ying Li <ying.li@docker.com>
…or the root CA because we no longer support encrypting the root CA key, and PKCS8 vs PKCS1 only matters for fips if we encrypt. We want to keep the root key PKCS1 so that mixed version clusters will continue to work. Signed-off-by: Ying Li <ying.li@docker.com>
…o encrypt and decrypt keys. It can be set using a setter function. Signed-off-by: Ying Li <ying.li@docker.com>
KeyReadWriter used in the node object. Signed-off-by: Ying Li <ying.li@docker.com>
nishanttotla
left a comment
There was a problem hiding this comment.
Sorry for the delay on this review, but LGTM.
I don't have any particular comments. The code structure looks good, and the only question I had was already addressed in the discussion above.
|
Thanks so much @nishanttotla! I will merge and rebase the others. |
#2246 added FIPS support by checking the environment variable
GOFIPS. This has several drawbacks:GOFIPSenv var, and unset them. When tests run in parallel, this can cause flakiness, which we've seen some of in CIBetter if we can inject a boolean for FIPSness.
This changes everywhere that used to use
keyutilsspecifically to check for FIPSness now has to take akeyutils.Formatterobject which provides the same interface askeyutilsused to, and this changes everywhere that calledencryption.Defaultsto pass whether FIPS is required.Note that this does not pass the FIPS bool through from node to the raft storage - there's more work that needs to be done there, and this PR is really big already, so I'll do it in a separate PR.