Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. #1561

chemicL · 2021-05-13T12:56:05Z

Motivation:

There are cases when clients require connecting to a secure server which doesn't
identify itself in the TLS certificate with the same address as used for
connection establishment, or multiple identities can be presented from
diffrent underlying backends. In such case the hostname verification would fail
and would need to be disabled. However, the peerHost which is inferred from the
connection address would unnecessarily cache SSL sessions, which could lead
to issues in session resumption. Therefore, we want to allow explicitly disabling
peerHost inference and either allow setting it to a desired address or to null.

Modifications:

Added ClientSslConfig#isPeerHostSet method,
ClientSslConfigBuilder#setPeerHost now accepts null and disables address inference,
Added test in SslAndNonSslConnectionsTest validating the mentioned scenario,
Added test in SslAndNonSslConnectionsTest for ensuring hostname validation
works as expected when hostname is used (existing tests used only IP).

Result:

It is now possible to explicitly use null for peerHost
and disable its' inference.

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ClientSslConfig.java

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/SslAndNonSslConnectionsTest.java

Scottmitch · 2021-05-25T17:37:21Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SingleAddressHttpClientBuilder.java

+     * via {@link #sslConfig(ClientSslConfig)}.
+     * @return {@code this}
+     */
+    public abstract SingleAddressHttpClientBuilder<U, R> disableSni();


disableSni -> disableSniInference? Can you clarify why this method is different than the other new methods?

Consider the following names to allow users set/unset the value on the builder:

peerHostInference(boolean) peerPortInference(boolean) sniHostnameInference(boolean)

or

inferPeerHost(boolean) inferPeerPort(boolean) inferSniHostname(boolean)

@Scottmitch my reasoning was the following: the inference for SNI is less straightforward than for peerHost – internally, the X509TrustManagerImpl does a fallback to peerHost when checking the certificate identity. The internals also explicitly add the peerHost to the SNI list.
The existing fallback may seem confusing, hence I decided disabling SNI is the functionality that might be useful. Yet still, my attempt at allowing disabling is not really effective because of the fallback in JDK and Netty. I'll change to the inference related method.

idelpivnitskiy · 2021-05-26T05:48:31Z

...tty-internal/src/main/java/io/servicetalk/transport/netty/internal/ClientSecurityConfig.java

@@ -256,7 +256,7 @@ public ClientSslConfig asSslConfig() {
        }

        if (hostnameVerificationAlgorithm == null) {
-            builder.disableHostnameVerification();
+            builder.hostnameVerificationAlgorithm("");


Can you please add at the end of @deprecated description of ClientSslConfigBuilder#disableHostnameVerification() that if users are sure they need to disable it, they can use hostnameVerificationAlgorithm("").

idelpivnitskiy · 2021-05-26T05:52:38Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SingleAddressHttpClientBuilder.java

+     * via {@link #sslConfig(ClientSslConfig)}.
+     * @return {@code this}
+     */
+    public abstract SingleAddressHttpClientBuilder<U, R> disableSni();


Consider the following names to allow users set/unset the value on the builder:

peerHostInference(boolean) peerPortInference(boolean) sniHostnameInference(boolean)

or

inferPeerHost(boolean) inferPeerPort(boolean) inferSniHostname(boolean)

Motivation: There are cases when clients require connecting to a secure server which doesn't identify itself in the TLS certificate with the same address as used for connection establishment, or multiple identities can be presented from diffrent underlying backends. In such case the hostname verification would fail and would need to be disabled. However, the peerHost which is inferred from the connection address would unnecessarily cache SSL sessions, which could lead to issues in session resumption. Therefore, we want to allow explicitly disabling peerHost inference and either allow setting it to a desired address or to null. Modifications: - Added ClientSslConfig#isPeerHostSet method, - ClientSslConfigBuilder#setPeerHost now accepts null and disables address inference, - Added test in SslAndNonSslConnectionsTest validating the mentioned scenario, - Added test in SslAndNonSslConnectionsTest for ensuring hostname validation works as expected when hostname is used (existing tests used only IP). Result: It is now possible to explicitly use `null` for `peerHost` and disable its' inference.

…rence and SNI altogether. Motivation: There are cases when clients require connecting to a secure server which doesn't identify itself in the TLS certificate with the same address as used for connection establishment, or multiple identities can be presented from diffrent underlying backends. In such case the hostname verification would fail and would need to be disabled. However, the peerHost which is inferred from the connection address would unnecessarily cache SSL sessions, which could lead to issues in session resumption. Therefore, we want to allow explicitly disabling peerHost inference and either allow setting it to a desired address or leaving `null`. Modifications: - Added SingleAddressHttpClientBuilder#disablePeerHostInference method, - Added SingleAddressHttpClientBuilder#disablePeerPortInference method, - Added SingleAddressHttpClientBuilder#disableSni method, - ClientSslConfigBuilder#setPeerHost now accepts explicit `null`, - Added case in SslAndNonSslConnectionsTest validating the mentioned scenario, - Added case in SslAndNonSslConnectionsTest for ensuring hostname validation works as expected when hostname is used (existing tests used only IP), - Added case in SniTest validating behaviour for disabled SNI and explicitly disabling it in existing `noSni` cases. Result: It is now possible to explicitly use `null` for `peerHost` in `ClientSslConfig` and disable its' inference using `SingleAddressHttpClientBuilder`'s API.

...icetalk-transport-api/src/main/java/io/servicetalk/transport/api/ClientSslConfigBuilder.java

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/SslAndNonSslConnectionsTest.java

chemicL · 2021-05-27T13:12:38Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SingleAddressHttpClientBuilder.java

+     * via {@link #sslConfig(ClientSslConfig)}.
+     * @return {@code this}
+     */
+    public abstract SingleAddressHttpClientBuilder<U, R> disableSni();


@Scottmitch my reasoning was the following: the inference for SNI is less straightforward than for peerHost – internally, the X509TrustManagerImpl does a fallback to peerHost when checking the certificate identity. The internals also explicitly add the peerHost to the SNI list.
The existing fallback may seem confusing, hence I decided disabling SNI is the functionality that might be useful. Yet still, my attempt at allowing disabling is not really effective because of the fallback in JDK and Netty. I'll change to the inference related method.

Scottmitch

one comment then lgtm

Scottmitch · 2021-06-05T01:44:51Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java

+
+                @Nullable
+                private String evaluatePeerHost() {
+                    return (configPeerHost == null && inferPeerHost) ? fallbackPeerHost : configPeerHost;


consider inlining these definitions or making the anonymous type non-anonymous and adding constructor to clarify we don't need to retain references to configPeerHost and related variables outside the scope of this class.

Scottmitch · 2021-06-05T01:50:01Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/SniTest.java

+                )
+                .listenBlockingAndAwait((ctx, request, responseFactory) -> responseFactory.ok());
+             BlockingHttpClient client = HttpClients.forSingleAddress(
+                     HostAndPort.of(


nit: consider removing HostAndPort.of( and use the method overload that takes String, int

idelpivnitskiy

Good work!

Doesn't look like this PR breaks any API now. Should we remove breaking-change label now?

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/SniTest.java

idelpivnitskiy · 2021-06-05T02:01:59Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SingleAddressHttpClientBuilder.java

+     * @param shouldInfer value indicating whether inference is on ({@code true}) or off ({@code false}).
+     * @return {@code this}
+     */
+    public abstract SingleAddressHttpClientBuilder<U, R> inferPeerHost(boolean shouldInfer);


We previously discussed in #1561 (comment), but doesn't look like we come to a conclusion. Does it make sense to join peer host and port together on ClientSslConfig API into a single method HostAndPort peerHostAndPort()? If yes, we can have a single method here: inferPeerHostAndPort(boolean) instead of two.

@Scottmitch wdyt about single ClientSslConfig#peerHostAndPort() and inferPeerHostAndPort(boolean)?

To my understanding the peer port is not used when peer host is not inferred (and also is null). I think it's reasonable to provide an API where both are affected. And yes, the breaking API change would need to go to ClientSslConfig. Perhaps we could do it in a separate PR and for now add this feature with no breakage?

I was thinking that if we merge host and port into one method, we can provide only one "infer" method and won't need to change client API later.
Let's make the final decision tomorrow morning. Everything else looks good in the PR.

Discussed with @Scottmitch offline. He suggested to keep host and port independent because sometimes users don't know what is the port number (SRV resolutions or use of Unix Domain Sockets).

idelpivnitskiy

Great work!

FAILURE: Build failed with an exception.
428 actionable tasks: 353 executed, 75 up-to-date
What went wrong:
Execution failed for task ':servicetalk-http-netty:pmdTest'.
> 4 PMD rule violations were found. See the report at: file:///home/runner/work/servicetalk/servicetalk/servicetalk-http-netty/build/reports/pmd/test.html

The build fails because of #1596. Please, fix PMD warnings and 🚢

chemicL · 2021-06-08T10:36:21Z

Fixed PMD errors, merged with main branch and ready to go.

…rence and SNI altogether. (apple#1561) Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. Motivation: There are cases when clients require connecting to a secure server which doesn't identify itself in the TLS certificate with the same address as used for connection establishment, or multiple identities can be presented from diffrent underlying backends. In such case the hostname verification would fail and would need to be disabled. However, the peerHost which is inferred from the connection address would unnecessarily cache SSL sessions, which could lead to issues in session resumption. Therefore, we want to allow explicitly disabling peerHost inference and either allow setting it to a desired address or leaving `null`. Modifications: - Added `SingleAddressHttpClientBuilder#inferPeerHost` method, - Added `SingleAddressHttpClientBuilder#inferPeerPort` method, - Added `SingleAddressHttpClientBuilder#inferSniHostname` method, - `ClientSslConfigBuilder#setPeerHost` now accepts explicit `null`, - Added case in `SslAndNonSslConnectionsTest` validating the mentioned scenario, - Added case in `SslAndNonSslConnectionsTest` for ensuring hostname validation works as expected when hostname is used (existing tests used only IP), - Added case in SniTest validating behaviour for disabled SNI and explicitly disabling it in existing `noSni` cases. Result: It is now possible to explicitly use `null` for `peerHost` in `ClientSslConfig` and disable its' inference using `SingleAddressHttpClientBuilder`'s API.

…1561 Motivation: `ClientSslConfigBuilder#disableHostnameVerification()` option was deprecated in apple#1561 and can be removed now. Users can use `hostnameVerificationAlgorithm("")` as an alternative, but should avoid disabling hostname verification in general. Modifications: - Remove `ClientSslConfigBuilder#disableHostnameVerification()`; Result: No deprecated `ClientSslConfigBuilder#disableHostnameVerification()` API.

…1637) Motivation: `ClientSslConfigBuilder#disableHostnameVerification()` option was deprecated in #1561 and can be removed now. Users can use `hostnameVerificationAlgorithm("")` as an alternative, but should avoid disabling hostname verification in general. Modifications: - Remove `ClientSslConfigBuilder#disableHostnameVerification()`; Result: No deprecated `ClientSslConfigBuilder#disableHostnameVerification()` API.

Motivation: This change follows from [a change to HTTP client builders](apple#1561) and exposes the same capabilities to GRPC client builders. Modifications: - `SingleAddressGrpcClientBuilder` has new methods: `inferPeerHost`, `inferPeerPort`, and `inferSniHostname` which allow configuring the behaviour of the TLS handshake and inference of the corresponding configurations. Result: Users are able to disable peerHost and peerPort inference and can now use an empty peerHost when they so desire. The SNI hostname inference from the provided address can also be disabled in cases when the address used for connection should not be passed in the SNI extension.

…nts (#1678) Motivation: This change follows from [a change to HTTP client builders](#1561) and exposes the same capabilities to GRPC client builders. Modifications: - `SingleAddressGrpcClientBuilder` has new methods: `inferPeerHost`, `inferPeerPort`, and `inferSniHostname` which allow configuring the behaviour of the TLS handshake and inference of the corresponding configurations. Result: Users are able to disable peerHost and peerPort inference and can now use an empty peerHost when they so desire. The SNI hostname inference from the provided address can also be disabled in cases when the address used for connection should not be passed in the SNI extension.

chemicL requested review from Scottmitch and idelpivnitskiy May 13, 2021 12:56

tkountis assigned chemicL May 13, 2021

Scottmitch reviewed May 13, 2021

View reviewed changes

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ClientSslConfig.java Outdated Show resolved Hide resolved

idelpivnitskiy reviewed May 14, 2021

View reviewed changes

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ClientSslConfig.java Outdated Show resolved Hide resolved

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/SslAndNonSslConnectionsTest.java Show resolved Hide resolved

chemicL requested review from idelpivnitskiy and Scottmitch May 18, 2021 12:51

chemicL force-pushed the allow-null-peerhost branch from 5727fea to 0d5e0a3 Compare May 18, 2021 14:10

chemicL force-pushed the allow-null-peerhost branch from 0d5e0a3 to 31043de Compare May 25, 2021 14:57

chemicL changed the title ~~Allow setting peerHost to null in ClientSslConfig.~~ Allow using null peer host for TLS and disabling host and port inference and SNI altogether. May 25, 2021

tkountis added breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. API PR with API changes (New or Deprecated) labels May 25, 2021

chemicL marked this pull request as ready for review May 25, 2021 16:35

Scottmitch reviewed May 25, 2021

View reviewed changes

idelpivnitskiy reviewed May 26, 2021

View reviewed changes

chemicL and others added 2 commits May 27, 2021 13:36

chemicL force-pushed the allow-null-peerhost branch from 31043de to c2c8e1a Compare May 27, 2021 11:40

chemicL commented May 27, 2021

View reviewed changes

chemicL force-pushed the allow-null-peerhost branch from dc204ed to ef843cf Compare May 27, 2021 14:39

Changed methods to accept boolean value, restored deprecated method

a725289

chemicL force-pushed the allow-null-peerhost branch from ef843cf to a725289 Compare May 27, 2021 14:43

chemicL requested review from idelpivnitskiy and Scottmitch June 4, 2021 19:56

Scottmitch reviewed Jun 5, 2021

View reviewed changes

idelpivnitskiy reviewed Jun 5, 2021

View reviewed changes

Dariusz Jędrzejczyk added 2 commits June 7, 2021 18:10

Feedback from review

5369566

Merge branch 'main' into allow-null-peerhost

90553c4

chemicL force-pushed the allow-null-peerhost branch from 064d4ce to 90553c4 Compare June 7, 2021 16:34

idelpivnitskiy approved these changes Jun 8, 2021

View reviewed changes

idelpivnitskiy removed the breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. label Jun 8, 2021

Dariusz Jędrzejczyk added 2 commits June 8, 2021 12:12

Junit5 compliance restored

db0f010

Merge branch 'main' into allow-null-peerhost

b060984

chemicL merged commit 4c8e681 into apple:main Jun 8, 2021

idelpivnitskiy mentioned this pull request Jun 21, 2021

Remove disableHostnameVerification() that was deprecated in #1561 #1637

Merged

chemicL mentioned this pull request Jul 19, 2021

Support controlling peerHost and SNI hostname inference for GRPC clients #1678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. #1561

Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. #1561

chemicL commented May 13, 2021

Scottmitch May 25, 2021

idelpivnitskiy May 26, 2021

chemicL May 27, 2021

idelpivnitskiy May 26, 2021

chemicL May 27, 2021

idelpivnitskiy May 26, 2021

chemicL May 27, 2021

Scottmitch left a comment

Scottmitch Jun 5, 2021

Scottmitch Jun 5, 2021

idelpivnitskiy left a comment

idelpivnitskiy Jun 5, 2021

idelpivnitskiy Jun 5, 2021

chemicL Jun 7, 2021 •

edited

Loading

idelpivnitskiy Jun 7, 2021

idelpivnitskiy Jun 8, 2021

idelpivnitskiy left a comment

chemicL commented Jun 8, 2021

Allow using null peer host for TLS and disabling host and port inference and SNI altogether. #1561

Allow using null peer host for TLS and disabling host and port inference and SNI altogether. #1561

Conversation

chemicL commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemicL Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

chemicL commented Jun 8, 2021

Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. #1561

Allow using `null` peer host for TLS and disabling host and port inference and SNI altogether. #1561

chemicL Jun 7, 2021 •

edited

Loading