adutra commented on PR #15500:
URL: https://github.com/apache/iceberg/pull/15500#issuecomment-4033352180
Update: I was able to find the exact change that caused this regression:
In 1.10 we use httpclient5 version 5.5, where the `DefaultClientTlsStrategy`
constructor used by Iceberg is as follows:
```java
public DefaultClientTlsStrategy(
final SSLContext sslContext,
final String[] supportedProtocols,
final String[] supportedCipherSuites,
final SSLBufferMode sslBufferManagement,
final HostnameVerifier hostnameVerifier) {
this(sslContext, supportedProtocols, supportedCipherSuites,
sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier);
}
```
https://github.com/apache/httpcomponents-client/blob/c5bd9af6a47af3f2683209f0b818f1cf109026f6/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java#L124-L131
But in 1.11 we upgraded httpclient5 to version 5.6, where the same
constructor becomes:
```java
public DefaultClientTlsStrategy(
final SSLContext sslContext,
final String[] supportedProtocols,
final String[] supportedCipherSuites,
final SSLBufferMode sslBufferManagement,
final HostnameVerifier hostnameVerifier) {
this(sslContext, supportedProtocols, supportedCipherSuites,
sslBufferManagement, null, hostnameVerifier);
}
```
https://github.com/apache/httpcomponents-client/blob/cee67d86809aa23577968f9e7e7bf922a9892512/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java#L127
Passing `null` instead of `HostnameVerificationPolicy.CLIENT` is not the
same when there is a non-null `hostnameVerifier`:
https://github.com/apache/httpcomponents-client/blob/master/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java#L101
So, this is imho clearly a regression, and in fact the default value for
`TLSConfigurer.hostnameVerificationPolicy()` should be `CLIENT`, not `BOTH` if
we want to restore the 1.10 behavior:
```java
default HostnameVerificationPolicy hostnameVerificationPolicy() {
return HostnameVerificationPolicy.CLIENT;
}
```
I will change that.
@singhpk234 could you please add this to the 1.11 milestone? Now I'm really
convinced it's a regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
