[1.10.x] rpc: authorize raft requests #10931
Merged
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
By checking the names in the verified certificate chain
In preparation for fixing Raft RPC authorization this change is made to the tlsutil.Configurator to store a CertPool which will omit the Connect CA certificate. This commit also removes the need to append slices of PEMs together. The append function can modify the backing array of the first slice passed to append, and that can often lead to difficult to understand bugs. It's not clear to me if the use of append here would actually cause such bugs, but it was easier to remove the use of append than to prove the bug existed. Removing the use of append should also prevent any such bugs from happening in the future.
Connect CA certs could potentially contain DNSNames that match Consul servers. So to prevent those certificates from being used to gain unauthorized access we verify that the connection performing the raft RPC has a certificate signed by the agent TLS CA. Also add tests cases for ConnectCA leaf cert, which fail without this change.
This test started to fail because the CertFile used is no longer allowed to perform Raft RPC (it must not have a valid DNSName). From my reading of this test, it is intended to limit client connections, so we can probably test with a different byte that would be used by clients. There's no special handling for the RPCRaft byte, so I believe the test is still valid after this change.
The requester is a "Server" in Consul's architecture, but is a Client from the perspective of this RPC request. Co-authored-by: Paul Banks <[email protected]>
eculver
added
theme/tls
mikemorris
removed
the
pr/no-changelog
picatz
reviewed
Aug 26, 2021
Co-authored-by: Kent 'picat' Gruber <[email protected]>
|
Grrrr, looks like I didn't quite get the bash syntax correct for the changelog checker when the target branch isn't |
…hicorp/consul into dnephin/backport-1.10-raft-authz-fix
mikemorris
approved these changes
Aug 26, 2021
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/435680. |
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.
Backport of #10925