Fix resteasy-reactive-client hostname verification default

[polarctos]

Since introduction of the setting verifyHost in 2.15.0.Final the hostname verification was disabled by default for the resteasy-reactive-client, as the default value for boolean in Java (primitive) is false.
gsmet@941d3a6#diff-28d8221e6b920ee90f85249d3c250b932c207d6b2099fc9377984d7f550fe28d

A better configuration name value would have been a negated name like disableHostnameVerification, but renaming it is no longer possible as it would break compatibility.

This disabled default currently makes the reactive client vulnerable to MITM attacks.
E.g. the following rest client builder from the documentation with quarkus-rest-client-reactive-jackson is vulnerable, as it does not fail during the SSL handshake:

RestClientBuilder.newBuilder()
    .baseUri(URI.create("https://wrong.host.badssl.com"))
    .build(WrongHostClient.class);

The expected exception has the cause javax.net.ssl.SSLHandshakeException. The test case ExternalWrongHostTestCase.java currently only tests the disabling of hostname verification but not for a default secure behaviour.

In the meantime setting the config explicitly is a workaround e.g. with quarkus.rest-client.verify-host=true.
This enables hostname verification again also for the reactive client. But the default of Quarkus should instead be secure!

This change now adds a proper default both in the configuration and assigns safe initial values for the fields in the reactive client builder implementation.

Both custom HostnameVerifier and SSLContext are currently not supported by resteasy-reactive-client.
This is documented as known limitations.

This change also aligns the implementation for both to the already present state for SSLContext.
It makes the builder fail when HostnameVerifier is tried to be configured. The benefit is that it fails as early as
currently possible and not only when the custom hostname verification would be needed later on.

The general need for the configuration option of a custom hostname verifier still stays relevant, as it was also requested in #27901. Currently just having the possibility to completely disable hostname verification is preventing the chance to supply a suitable customised hostname verifier that implements custom verification rules.

[Sgitario]

lgtm

[geoand]

The test failures are definitely related

[polarctos]

The test failures are definitely related

@geoand

Sorry for that. Did not expect there to be tests for an unsupported feature.
I now removed the unsupported config tests for setting a hostnameVerifier.

On the rest-client-reactive config a custom hostname verifier was also before not supported, but did not throw an exception. Adapted the tests accordingly as the builder fails early now throwing UnsupportedOperationException.

The general discussion if failing here with an exception makes sense is discussed here: #32932 (comment)

In short: It should be the same behaviour for both unsupported features of the rest-client-reactive of HostnameVerifier and SSLContext.
Long term it would actually make sense to try to implement them, as they are part of the MP RestClientBuilder API.

[polarctos]

Should a CVE be opened for this for the affected versions so that affected users are notified by scanners?

The actual weakness would be local to default false value of verifyHost
in org.jboss.resteasy.reactive.client.impl.ClientBuilderImpl
from io.quarkus.resteasy.reactive:resteasy-reactive-client.

From first sight the affected versions include: >= 2.15.0.Final <= 3.0.1.Final

At least for other products CVEs were sometimes opened fur such trivial things,
e.g. where in the defaults it is running curl -k.

Applicable CWEs:
https://cwe.mitre.org/data/definitions/297.html / https://cwe.mitre.org/data/definitions/295.html
https://cwe.mitre.org/data/definitions/453.html

[geoand]

As I have 0️⃣ time for at least the next week, I'll leave this up to @Sgitario and @sberyozkin

[Sgitario]

Should a CVE be opened for this for the affected versions so that affected users are notified by scanners?

The actual weakness would be local to default false value of verifyHost in org.jboss.resteasy.reactive.client.impl.ClientBuilderImpl from io.quarkus.resteasy.reactive:resteasy-reactive-client.

From first sight the affected versions include: >= 2.15.0.Final <= 3.0.1.Final

At least for other products CVEs were sometimes opened fur such trivial things, e.g. where in the defaults it is running curl -k.

Applicable CWEs: https://cwe.mitre.org/data/definitions/297.html / https://cwe.mitre.org/data/definitions/295.html https://cwe.mitre.org/data/definitions/453.html

This sounds reasonable to me +1

[polarctos]

I added a test now to assert that certificate host name verification is now enabled by default and working.

Build is also green now, based on the GitHub actions in the forked repository.

[geoand]

cc @Sgitario

[Sgitario]

The changes look good to me.
Though, there are still some custom hostname verifier classes that should be deleted now:

• MyHostnameVerifier1 and MyHostnameVerifier2 in RestClientCDIDelegateBuilderTest
• MyHostnameVerifier in HelloClientWithBaseUri

Also, squash all the commits, so we can proceed with merging these changes.

Many thanks!

[polarctos]

Though, there are still some custom hostname verifier classes that should be deleted now:

• MyHostnameVerifier1 and MyHostnameVerifier2 in RestClientCDIDelegateBuilderTest
• MyHostnameVerifier in HelloClientWithBaseUri

Also, squash all the commits, so we can proceed with merging these changes.

@Sgitario I removed the unused hostname verifier classes from the tests now.

I squashed it down to two commits, but these two are actually independent of each other:

• Fail on attempt to set custom hostname verifier
• Enabling hostname verification by default

From my perspective, only the second commit for enabling the hostname verification is necessary as a security fix backport to other branches, as it fixes the current vulnerability.

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[polarctos]

I think the pull request is now ready to be merged.

[Sgitario]

@gastaldi would you mind a final review of this one and merging it if you're ok with it? Thanks!

[gastaldi]

@Sgitario it looks good to me but I'll let @sberyozkin have the final say on this 😉

[sberyozkin]

Hi, should this PR check if quarkus.tls.trust-all is configured and if yes, prefer quarkus.tls.trust-all ? This is how OIDC client code is configured, so that if for example, OIDC protected endpoint is using OIDC reactive token propagation which relies on Resteasy Reactive client, the hostname verification can be configured with a single property, as opposed to configuring it twice

[sberyozkin]

I suppose it can be handled in a follow up PR. @polarctos would you be OK with opening a new issue related to checking quarkus.tls.trust-all ? Thanks

[Sgitario]

Hi, should this PR check if quarkus.tls.trust-all is configured and if yes, prefer quarkus.tls.trust-all ? This is how OIDC client code is configured, so that if for example, OIDC protected endpoint is using OIDC reactive token propagation which relies on Resteasy Reactive client, the hostname verification can be configured with a single property, as opposed to configuring it twice

if 'verify-hostis true, buttrust-allis true, then theverify-host` will be turned off:

quarkus/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/ClientBuilderImpl.java

Line 213 in 5bca78d

if (trustAll) {

So, I think it's fine how it is now.