[ca] Validate the Root CA certificate before updating the security config #2234
Conversation
This file contains 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
cyli
force-pushed
the
validate-root-ca-before-updating
branch
from
04f2fb3
to
095842f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2234 +/- ##
==========================================
+ Coverage 60.16% 60.24% +0.08%
==========================================
Files 124 124
Lines 20156 20184 +28
==========================================
+ Hits 12127 12160 +33
+ Misses 6661 6660 -1
+ Partials 1368 1364 -4 |
aaronlehmann
reviewed
Jun 9, 2017
ca/config.go
Outdated
| for i, derBytes := range tlsKeyPair.Certificate { | ||
| parsed, err := x509.ParseCertificate(derBytes) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not validate new roots because could not parse TLS cert") |
There was a problem hiding this comment.
could not validate new root certificates due to parse error?
|
LGTM |
…with a new RootCA, to be sure that the root CA certificate matches the TLS credentials already in the SecurityConfig. This will prevent, for instance, a manager from telling an agent to load an invalid root certificate, as can happen if an agent connects to a manager that is being caught up via raft and hence might be replaying old root rotations. Signed-off-by: Ying Li <ying.li@docker.com>
cyli
force-pushed
the
validate-root-ca-before-updating
branch
from
095842f
to
088c952
Compare
This was referenced Jun 12, 2017
silvin-lubecki
pushed a commit
to silvin-lubecki/docker-ce
that referenced
this pull request
Feb 3, 2020
To get the changes: * moby/swarmkit#2234 * moby/swarmkit#2237 * moby/swarmkit#2238 Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
silvin-lubecki
pushed a commit
to silvin-lubecki/engine-extract
that referenced
this pull request
Feb 3, 2020
To get the changes: * moby/swarmkit#2234 * moby/swarmkit#2237 * moby/swarmkit#2238 Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
silvin-lubecki
pushed a commit
to silvin-lubecki/engine-extract
that referenced
this pull request
Mar 10, 2020
To get the changes: * moby/swarmkit#2234 * moby/swarmkit#2237 * moby/swarmkit#2238 Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
silvin-lubecki
pushed a commit
to silvin-lubecki/engine-extract
that referenced
this pull request
Mar 23, 2020
To get the changes: * moby/swarmkit#2234 * moby/swarmkit#2237 * moby/swarmkit#2238 Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Validate the Root CA certificate before updating the security config with
a new RootCA, to be sure that the root CA certificate matches the TLS
credentials already in the SecurityConfig.
This will prevent, for instance, a manager from telling an agent to load
an invalid root certificate, as can happen if an agent connects to a
manager that is being caught up via raft and hence might be replaying
old root rotations.
Signed-off-by: Ying Li ying.li@docker.com
Without this change, a manager that is catching up (for instance if it has been promoted, or if it was behind) and replying raft messages might tell all the nodes connected to it to update their root CA to an older version. This will also prevent that manager from updating to an outdated root CA as it's catching up.
This is not the most ideal change, since we're only validating when updating the root CA in the security config, but would be a quick patch to fix the issue if we want to try to get this into 17.06.
Otherwise, maybe it'd make sense to refactor
SecurityConfiga bit to store thex509.Certificateand key instead of just thetls.Certificate, so that validation for everything can be moved into theSecurityConfig?cc @aaronlehmann @diogomonica