[fips] Support a flag that indicates that the cluster requires mandatory FIPS mode on all nodes #2594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2594 +/- ##
==========================================
+ Coverage 61.7% 61.82% +0.12%
==========================================
Files 134 134
Lines 21788 21818 +30
==========================================
+ Hits 13444 13489 +45
+ Misses 6895 6880 -15
Partials 1449 1449 |
cyli
force-pushed
the
cluster-mandatory-fips
branch
from
d6a39a1 to
4ec152c
Compare
anshulpundir
reviewed
May 4, 2018
api/objects.proto
Outdated
|
|
||
| // FIPS specifies whether this cluster should be in FIPS mode. This changes | ||
| // the format of the join tokens, and nodes that are not FIPS-enabled should | ||
| // reject joining the cluster. Nodes that report themselves to be non-FIPs |
| // certificate or root certificate request. | ||
| GetCertRetryInterval = 2 * time.Second | ||
|
|
||
| errInvalidJoinToken = errors.New("invalid join token") |
Contributor
There was a problem hiding this comment.
nit: Maybe a note on where this error should be used ?
ca/config.go
Outdated
| type ParsedJoinToken struct { | ||
| Version int | ||
| RootDigest digest.Digest | ||
| Secret string |
Contributor
There was a problem hiding this comment.
Not sure if it makes sense to add a comment to clarify each field, but I'll let you decide.
ca/config.go
Outdated
|
|
||
| // GenerateJoinToken creates a new join token. | ||
| func GenerateJoinToken(rootCA *RootCA) string { | ||
| // GenerateJoinToken creates a new join token. If FIPS is enabled, the join token is of a new format. |
Contributor
There was a problem hiding this comment.
this is prob a dumb question, but can you please clarify what you mean by 'a new format' ?
ca/config.go
Outdated
| NodeCertificateStatusRequestTimeout time.Duration | ||
| // RetryInterval specifies how long to delay between retries, if non-zero. | ||
| RetryInterval time.Duration | ||
| // Organization is the the organization to use when creating a security config |
Contributor
There was a problem hiding this comment.
nit: say how this is used ?
| return strings.HasPrefix(securityConfig.ClientTLSCreds.Organization(), "FIPS.") | ||
| } | ||
|
|
||
| func isMandatoryFIPSClusterJoinToken(joinToken string) bool { |
node/node.go
Outdated
| ConnBroker: n.connBroker, | ||
| }) | ||
| } | ||
| // If this is a new cluster, we want to name the cluster ID FIPS-something |
Contributor
There was a problem hiding this comment.
super nit: FIPS-something => 'FIPS-something'
Collaborator
|
LGTM, but needs a squash. |
…en a cluster is first created, the FIPS value should be set and it should not be changed through the lifetime of the cluster, because converting from non-FIPS to FIPS should not be possible (to avoid compliance issues, even if there were a migration process, we'd have to provide a validation tool to ensure that the migration was complete across the cluster). Signed-off-by: Ying Li <[email protected]>
… reflect this property. So all TLS certs will have the cluster ID, which says whether the cluster is FIPS, in the Org field. If a node loads up its TLS cert, sees that that the cluster requires FIPS, and FIPS mode is not enabled on that node, the node will shut down. If a non-FIPS node gets a join token that indicate that the cluster mandates FIPS, it will refuse to join. Signed-off-by: Ying Li <[email protected]>
cyli
force-pushed
the
cluster-mandatory-fips
branch
from
4ec152c to
9943770
Compare
Contributor
Author
|
Thanks @anshulpundir and @dperny - I think the comments are addressed, and I've squashed it to 2 commits (generated API changes and code changes) |
Contributor
|
LGTM |
Merged
vdemeester
pushed a commit
to thaJeztah/docker
that referenced
this pull request
Jun 5, 2018
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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is implemented by:
This may conflict a little against #2586, since both propagate the FIPS boolean to the manager.