Core: Expose HostnameVerificationPolicy in TLSConfigurer

[adutra]

Fixes #15598.

This change exposes HostnameVerificationPolicy in TLSConfigurer. The default policy is set to CLIENT.

I also added a unit test that intentionally uses a custom hostname verifier that is incompatible with JSSE hostname verification strategies. Thus, the test cannot pass unless the verification policy is set to CLIENT.

[adutra]

FYI @bryanck

[singhpk234]

This component is particularly useful when attempting to bypass hostname verification, e.g. by using the NoopHostnameVerifier.

Alex, why would we allow such a verification to be bypassed, do you have some real world use case for this ?

[nandorKollar]

Since this is new dependency, do we have to add the licence of this dependency to one of the LICENSE files in Iceberg?

[adutra]

No, since it's used only in tests.

[nandorKollar]

Thanks for clarifying.

[steveloughran]

that's good because bouncycastle complicates export rules, or at least due diligence

[stevenzwu]

I assumes the bouncycastle bcpkix is a transitive dep already pulled in by the mockerserver lib. is the purpose here just to upgrade the version?

[adutra]

Very good question! Short answer: yes.

MockServer does bring BouncyCastle transitively — bcprov, bcpkix, and bcutil – but at version 1.72, which is somewhat old.

The problem is that other deps (Hadoop) bring bcprov 1.82, and Gradle resolves the conflict by upgrading bcprov to 1.82. But bcpkix and bcutil stay at 1.72, since nothing else declares a higher version. This creates a mismatch: bcpkix 1.72 + bcprov 1.82 = NoClassDefFoundError.

I only upgraded bcpkix here, because that is enough for running the TLS tests. If you prefer, I can also explicitly upgrade bcutil.

[stevenzwu]

it is probably good to just ensure the same version for bouncycastle family of jars. That is the current practice. E.g., there is just one version number for the two jetty jars in the toml file.

[adutra]

You are right: I implemented your suggestion.

[adutra]

Alex, why would we allow such a verification to be bypassed, do you have some real world use case for this ?

You could find yourself in the situation where the catalog server has TLS enabled, and its certificate shows a SAN of e.g. catalog.bigcorp.com; but if the client/engine is in the same cluster/network, it could actually be contacting the catalog through its internal IP instead, e.g. https://1.2.3.4:8181/api/catalog. In that case, the hostname verification will fail.

(Please note: I mentioned NoopHostnameVerifier just as an example of possible usage of HostnameVerificationPolicy.)

[adutra]

And I forgot to mention: NoopHostnameVerifier is also quite useful in tests :-)

[adutra]

@bryanck since you introduced TLSConfigurer: are you OK with the changes in this PR?

[adutra]

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:

    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:

    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:

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.

[adutra]

@danielcweeks and @singhpk234 : as requested, I created an issue to better explain why I think we have a regression and how this PR fixes it: #15598. PTAL 🙏

[stevenzwu]

You could find yourself in the situation where the catalog server has TLS enabled, and its certificate shows a SAN of e.g. catalog.bigcorp.com; but if the client/engine is in the same cluster/network, it could actually be contacting the catalog through its internal IP instead, e.g. https://1.2.3.4:8181/api/catalog. In that case, the hostname verification will fail.

(Please note: I mentioned NoopHostnameVerifier just as an example of possible usage of HostnameVerificationPolicy.)

I am wondering about the default value of HostnameVerificationPolicy.CLIENT. I know the purpose is maintain the same behavior. Unit test can configure NoopHostnameVerifier to bypass the check or mapping from ip to the hostname.

Is HostnameVerificationPolicy.BOTH the safer config for prod env and used as the default? if prod env also wants IP address to work, the safer practice is to add the IP address to the Subject Alternative Name (SAN) .

[nastra]

I think it's worth at least adding a comment on why these dependencies are being added, since it's not obvious

[steveloughran]

Is HostnameVerificationPolicy.BOTH the safer config for prod env and used as the default?

Best to lock down and require relaxation in deployments than to unlock and regret it.

More formally CWE-1188: Initialization of a Resource with an Insecure Default

[adutra]

I am wondering about the default value of HostnameVerificationPolicy.CLIENT.

BOTH is safer, but also introduces a behavioral change compared to 1.10. Are we OK with that?

[stevenzwu]

I am wondering about the default value of HostnameVerificationPolicy.CLIENT.

BOTH is safer, but also introduces a behavioral change compared to 1.10. Are we OK with that?

Can you send an email to dev@ to get broader feedback? It might be ok introduce a slight behavior change. Most production use cases probably should use DNS anyway. For IP address usage, users can add the IP address to the cert SAN.

This is the only question I have for this PR. Exposing hostnameVerificationPolicy in the TLS configurer makes sense. Unit test can configure it to noop to use the loopback address.