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.

[aihuaxu]

@adutra @stevenzwu Want to follow up if this is required for 1.11.0 release. Can you help driving this? Thanks.

[stevenzwu]

@aihuaxu this is needed for 1.11.0, as the original change was added in the 1.11 scope. Waiting for the dev thread to get more tractions.

[RussellSpitzer]

I wrote on the ML list a bit about this, but do we really need this to be customizable?

It seems to be that there are only really 2 options that make sense.

Either
hostnameVerifier is replaced and we should use "client" mode
or
hostnameVerifier is not replaced and we should be using "builtin" mode

[stevenzwu]

@RussellSpitzer @adutra this is the method where I suggested adding Javadoc. or you think this is redundant and the intention is obvious from the API already?

/**
 * If overwritten, only the custom verifier is executed and
 * Java Secure Socket Extension (JSSE) built-in verifier won't be executed.
 */

I also like Russell's suggestion from email of making this method returning null so that we can get rid of the new method above.

[adutra]

@stevenzwu @RussellSpitzer I implemented the suggested approach, PTAL.

I want to stress though that returning null instead of HttpsSupport.getDefaultHostnameVerifier() here is a behavioral change.

The built-in verifier and Apache's default verifier both do roughly the same verifications, but the latter is more strict/complete. They also kick in in different phases and throw different exceptions.

[stevenzwu]

I want to stress though that returning null instead of HttpsSupport.getDefaultHostnameVerifier() here is a behavioral change.

Not really a behavior change btw 1.10 and 1.11 since the original change from Bryant hasn't been released it.

And returning null is essentially the same as using the default built-in hostname verifier.

[adutra]

Not really a behavior change btw 1.10 and 1.11 since the original change from Bryant hasn't been released it.

From what I see, TLSConfigurer has been released in 1.10.

[stevenzwu]

LGTM.

just a nit comment on the test code.

[RussellSpitzer]

Apparently this is wording JDK specific, May come up later if folks are testing on other architectures. But I think we are mostly testing on OpenJDK, Oracle JDK and the other OpenJDK based versions.

OpenJDK / Oracle JDK: "No subject alternative names matching IP address 127.0.0.1 found" — this is the message in sun.security.util.HostnameChecker.

IBM Semeru / J9: IBM's JSSE implementation has its own HostnameChecker equivalent with different wording. Historically it uses messages like "No name matching 127.0.0.1 found" or similar variations.

Eclipse Temurin / Azul Zulu: These are typically OpenJDK-based and share the message, but there's no guarantee across versions — the sun.security package is internal and its strings can change between releases without notice.

[RussellSpitzer]

Looks good to me, left a few nits on style

[RussellSpitzer]

@steveloughran - @nastra - Any remaining comments on this one?

[kevinjqliu]

original PR commit:
b3adeb1
part of 1.10.0 and 1.10.1

but as Alex mentioned in the community sync, the library update was NOT part of 1.10.x

This issue requires both the PR and the library update

[singhpk234]

It would be nice to know when we a choosing JSSE host name verifier, how much can we now influence with java ops / system prop and what are the new cases we are opening to when are shifting from lib to this one.

[RussellSpitzer]

It would be nice to know when we a choosing JSSE host name verifier, how much can we now influence with java ops / system prop and what are the new cases we are opening to when are shifting from lib to this one.

What are you looking for in particular?

I tried to do some Cursor assisted analysis and as far as I can tell neither of the verifiers are actually changeable via system properties or java opts. Neither of the OpenJDK HostnameChecker verifier nor the Apache DefaultHostnameVerifier use system.getProperty.

Cursor did list a few things that would indirectly change behaviors but they seem to be all a result of the fact that JSSE will will do hostname verification during the TLS handshake rather than the DefaultHostnameVerifier which checks after the handshake.

[singhpk234]

Thanks for analysis @RussellSpitzer, I was mostly curious about how jvm flags can influence host verification now, when JSSE is being used one such flag jdk.tls.trustNameService

switching the default verification engine from Apache's library to JSSE exposes a knob that was previously irrelevant. specially what if JVM was running with this flag on will now come into effect in rest client
But

• It defaults to false
• It's static final, set at JVM boot (not exploitable in runtime)
• Anyone setting it to true has already made a conscious decision to trust their DNS
• The attack requires controlling both the JVM launch flags AND a DNS server - at that point one have bigger problems

It was not meant to be a blocker of this pr, but more from curiously purpose and i think we should proceed if there is a concencus on the approach.

Though one thing I think while researching this flag, i would recommend we mention this in JSSE as default in the release notes explictly!

[adutra]

one such flag jdk.tls.trustNameService

This property lives in SSLSocketImpl.useImplicitHost() and only triggers when an SSLSocket is connected without an explicit hostname. I believe Apache HC5 always provide the target hostname explicitly from the URL when setting up connections (see AbstractClientTlsStrategy.upgrade()). So the incriminated code path is never reached in practice.

The attack requires controlling both the JVM launch flags AND a DNS server - at that point one have bigger problems

Exactly. If an attacker can set JVM launch flags, they can already disable TLS entirely, swap trust stores, etc. The game is already over.

[adutra]

Though one thing I think while researching this flag, i would recommend we mention this in JSSE as default in the release notes explictly!

That's a good idea! Here is a suggestion for the release notes:

When no custom HostnameVerifier is provided via TLSConfigurer, the REST client now uses JSSE's built-in hostname verification (during the TLS handshake) instead of Apache HttpClient's DefaultHostnameVerifier (after the handshake). Both verify the server certificate's SANs/CN against the target hostname. Users providing a custom HostnameVerifier are unaffected — their verifier continues to be used exclusively.

cc @aihuaxu