Repository navigation
[raft] mTLS implementation - #1810
Open
ahmadelrouby wants to merge 1 commit into
Open
ahmadelrouby wants to merge 1 commit into
ahmadelrouby wants to merge 1 commit into
Conversation
Signed-off-by: Ahmad ElRouby <18573669+ahmadelrouby@users.noreply.github.com>
|
|
the-glu
reviewed
Oct 22, 2026
| func (c *Consensus) initTransport(ctx context.Context, nodeID uint64, clusterID uint64, peers map[uint64]*url.URL, tlsInfo etcdtransport.TLSInfo) error { | ||
| nodeIDStr := fmt.Sprintf("%d", nodeID) | ||
| if tlsInfo.Empty() { | ||
| c.logger.Warn("Raft nodes do not authenticate each other: set --raft_cert_file, --raft_key_file and --raft_trusted_ca_file to enable mutual TLS") |
Member
There was a problem hiding this comment.
I think we want to prevent insecure operation unless user specify something like --insecure explicitly.
|
|
||
| // newTLSInfo returns the mutual TLS settings between Raft nodes, empty when mutual TLS is disabled. | ||
| // A peer must present a certificate signed by a CA of the trusted CA file. The certificate is not matched against the | ||
| // peer's source IP, which differs from the addresses in the certificate behind NAT or a load balancer. |
Member
There was a problem hiding this comment.
it's the peer target address no? not the source ip ?
|
|
||
| flag.StringVar(&connectParameters.CertFile, "raft_cert_file", "", "PEM certificate of this node, presented to the other Raft nodes. Setting it alongside --raft_key_file and --raft_trusted_ca_file enables mutual TLS between Raft nodes. Peer URLs must then use https.") | ||
| flag.StringVar(&connectParameters.KeyFile, "raft_key_file", "", "PEM private key of --raft_cert_file.") | ||
| flag.StringVar(&connectParameters.TrustedCAFile, "raft_trusted_ca_file", "", "PEM bundle of the CA certificates trusted to sign the certificates of the other Raft nodes, usually one CA per participant.") |
Member
There was a problem hiding this comment.
I would have parameters in a raft_tls_* to group them + remove 'trusted' (it's a CA so it's trusted).
| - A connecting peer is accepted when its certificate is signed by a trusted CA. The certificate is not matched against the peer's source IP, which differs from the addresses in the certificate behind NAT or a load balancer. Other connections are dropped during the TLS handshake, before any Raft message is read. | ||
| - With mutual TLS enabled every peer URL must use `https`, otherwise `http`. A node with a peer URL of the other scheme, or with only some of the three flags set, does not start. | ||
| - Restart the node after replacing its certificate or the trusted CA bundle. | ||
| - Without the flags, Raft traffic is unauthenticated plain HTTP and a warning is logged. Mutual TLS will become mandatory (see [#1734](https://github.com/interuss/dss/issues/1734)). |
Member
There was a problem hiding this comment.
I would left the option to be in insecure mode for development purpose (with the explicit flag, like CRDB).
Contributor
|
/easycla |
Closed
This branch has not been deployed
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
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.
Part of #1734. This is the base implementation: mTLS between Raft nodes is optional for now. A stack of follow-up PRs will make it required and run the end-to-end tests (CI
raftandraft-clusterjobs) with mTLS.TLSInfois used for the transport and the listener, as etcd does for its own peersSkipClientSANVerify)httpswith mTLS andhttpwithoutTested with unit tests for the flags and peer URLs.