Include server name indication (SNI) in outgoing TLS connections. #409
Conversation
| return sniHost; | ||
| } | ||
|
|
||
| public String getSniHost() { |
There was a problem hiding this comment.
Is this necessary, or used for anything? Consider removing if not.
There was a problem hiding this comment.
It is used for the serialization to JSON. I think it was implemented the same way in HealthCheckConfig for the URI(). We could also implement a different serialization method for Optionals or remove the optional for these JSON-serializable objects and use nullability instead, but I decided to follow conventions.
There was a problem hiding this comment.
Okay. Let's put a comment.
| @@ -56,6 +59,8 @@ private TlsSettings(Builder builder) { | |||
| this.trustStorePassword = toCharArray(builder.trustStorePassword); | |||
| this.protocols = ImmutableList.copyOf(builder.protocols); | |||
| this.cipherSuites = ImmutableList.copyOf(builder.cipherSuites); | |||
| this.sendSni = builder.sendSni; | |||
| this.sniHost = builder.sniHost; | |||
There was a problem hiding this comment.
Consider storing sniHost as a String in builder, and using Optional.ofNullable to wrap it inside Optional.
| @@ -144,6 +165,8 @@ public int hashCode() { | |||
| private String trustStorePassword = System.getProperty("javax.net.ssl.trustStorePassword"); | |||
| private List<String> protocols = Collections.emptyList(); | |||
| private List<String> cipherSuites = Collections.emptyList(); | |||
| private boolean sendSni = true; | |||
| private Optional<String> sniHost = Optional.empty(); | |||
There was a problem hiding this comment.
The sniHost doesn't really have to be wrapped inside Optional in this builder. You can create an optional in TlsSettings constructor using Optional.ofNullable, also avoiding unnecessary requireNonNull checks.
| ChannelPipeline pipeline = channel.pipeline(); | ||
|
|
||
| if (sslContext != null) { | ||
| pipeline.addLast("ssl", sslContext.newHandler(channel.alloc())); | ||
| SslHandler sslHandler = sendSni | ||
| ? sslContext.newHandler(channel.alloc(), targetHost, -1) |
There was a problem hiding this comment.
Is it correct to pass -1 as a peer port? Should we instead pass in the configured port number?
There was a problem hiding this comment.
Also, is it necessary to test for sendSni flag given that the call site has already set the target accordingly? This is from the NettyConnection constructor:
sniHost.orElse(origin.host())
There was a problem hiding this comment.
The sendSni flag is to disable sni, whereas the 'orElse' is to send the target hostname in the SNI ServerName instead of a preconfigured override.
It seems that -1 is a valid value as it is the one that will be set in the code with no SNI. The SNI extension doesn't specify a port number but only a server name. It's a shame that netty does not have an overload without a port number.
* Include server name indication (SNI) in outgoing TLS connections.
Add support for SNI in Styx-client, so the destination ServerName will be sent in outgoing TLS requests as per RFC 6066 section 3 (https://tools.ietf.org/html/rfc6066#section-3).