feat: set hostname for SNI

[billy34]

This pull request enhances TLSTransport to allow Server Name Identification (SNI) on server.

The hostname must be set in the SSLContext which is not available as a parameter on RedisConnection.

To reduce the changes introduced, I decided to forward the host given as a parameter to transport function to the call of TLSTransport and so added a host parameter to it also.

Then according to the MbedTLS documentation

• I do not set hostname if it was provided as an IPv4 or IPv6 address as it is unsupported.
• I set hostname otherwise as it is harmless even if SNI is not used

[jkaye2012]

Hey, thanks for the PR.

What's the behavior if localhost is passed?

[billy34]

Good point but I think it is only a matter of how the server will handle it.
SNI is used to select a certificate if the server is set to handle multiple subdomains with separate certificates.
If no certificate is found with subdomain set to localhost then it won't be able to terminate the TLS handshake and refuse the connection.

If you use "localhost" as hostname, this information will only be used if you have set your local server to use TLS protocol. If so you must have provided a certificate, be it self-signed or not to secure the communication. After it depends of how the server name was defined in the certificate and if the server has been told to validate the hostname or not.

As I already check if host was provided as an IP address, I can easily reject also "localhost".

My use case
client -----(TCP over mutualTLS)------ reverse proxy (traefik) -----(TCP)------ Redis
The reverse proxy handle multiple virtual hosts with a SAN certificate for TLS and require a valid client certificate for mutual authentication.

[jkaye2012]

Yeah, it makes sense that this path should only kick in if someone requests TLS on localhost. I'm not sure why they would do that, or what impact SNI would have (SNI just isn't something I have much experience with at all, personally). Do you think it would be better to exempt localhost specifically, or leave it as-is? I'm just not sure if this could somehow break someone's workflow that is functional currently (and thus require a major version bump).

[billy34]

hostname is just a label added to the informations sent to server during initial TLS handshake to ease the selection of proper certificate in case it handles multiple virtual hosts.
It will be set only if TLSconnection is used to connect to the server.
I could have added an additional boolean parameter such as setHostname but I didn't want to alter the parameters of the user facing functions to reduce the probability of incompatible change.
It is easy to exempt also localhost and I can provide a commit if it is better.

[jkaye2012]

Okay, let's go with this as-is then. I agree that a boolean parameter is not the way.