Client connections created for TLS-based replication use SNI if possible#11458
Merged
yossigo merged 1 commit intoredis:unstablefrom Dec 7, 2022
CatboxParadox:replication-sni
Merged
Client connections created for TLS-based replication use SNI if possible#11458yossigo merged 1 commit intoredis:unstablefrom CatboxParadox:replication-sni
yossigo merged 1 commit intoredis:unstablefrom
CatboxParadox:replication-sni
Conversation
yossigo
added
the
release-notes
yossigo
reviewed
Nov 2, 2022
Collaborator
yossigo
left a comment
yossigo
left a comment
There was a problem hiding this comment.
@CatboxParadox Thanks, this makes sense. Technically, this could be considered a breaking change in extreme cases but I think we can accept it as-is and not make it optional. @redis/core-team any other opinions?
yossigo
added
the
breaking-change
Member
|
@yossigo for completeness, can you describe the case and how it'll break? |
Collaborator
|
@oranagra It's a breaking change because Redis did not include an SNI in the past and will include it now (when connecting to a hostname, not an IP). Forwarding the connections through a proxy may result in different routing and subsequent connectivity issues, depending on how the proxy is configured. |
zeekling
approved these changes
Nov 5, 2022
Member
|
conceptually approved in a core team meeting for 7.2 |
yossigo
approved these changes
Nov 27, 2022
madolson
pushed a commit
to madolson/redis
that referenced
this pull request
Apr 19, 2023
When establishing an outgoing TLS connection using a hostname as a target, use TLS SNI extensions to include the hostname in use.
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
When establishing an outgoing TLS connection using a hostname as a target, use TLS SNI extensions to include the hostname in use.
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
4 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.
It is not possible to reliably select the target server instance for TLS replication in case that the target sits behind a TLS (terminating or pass-through) proxy, as there's no SNI extension in the ClientHello message sent via the connection created for the sake of replication.
This is a simple attempt at setting the SNI to the value of the master server's address, in case that address is not actually an IP