Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RestClient Reactive - set HostnameVerifier or SSLContext #27901

Closed
Chexpir opened this issue Sep 13, 2022 · 16 comments · Fixed by #27956
Closed

RestClient Reactive - set HostnameVerifier or SSLContext #27901

Chexpir opened this issue Sep 13, 2022 · 16 comments · Fixed by #27956

Comments

@Chexpir
Copy link
Contributor

Chexpir commented Sep 13, 2022

Description

There is a known limitation on RestClient Reactive, we cannot set a HostnameVerifier or SSLContext.

We have switched back to rest-client-mutiny for now, even if it's deprecated, but it would be useful to get this done, so we can move back towards a fully reactive service.

Do we know the priority of this feature? At least for HostnameVerifier.

Context: It is very common to give user, when configuring webhooks, the option to use insecure SSL at their own expense, usually for testing purposes, and we want to give that to our users.

Implementation ideas

No response

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 13, 2022

/cc @Sgitario, @cescoffier, @geoand

@famod
Copy link
Member

famod commented Sep 13, 2022

In my current project, for reasons that are not under my or my team's control, we do have to call an external service via IP in production.
So we'd also need HostnameVerifier to switch to rest-client-reactive. I have therefore added the blocking label.

@geoand
Copy link
Contributor

geoand commented Sep 13, 2022

@Sgitario would you be willing to look into lifting this restriction?

I can also check if your backlog is too full

@geoand
Copy link
Contributor

geoand commented Sep 13, 2022

I should note that it's entirely possible that we will need changes in Vert.x to support this

@cescoffier
Copy link
Member

Vertx does not provide such a feature. So, I don't believe we can provide that feature. However, you can enable the trustall feature which will by-pass the verification.

@cescoffier
Copy link
Member

The SSL context approach may work with vertx 4.3.4 (some rework around ssl has been achieved, it should provide such an option)

@Chexpir
Copy link
Contributor Author

Chexpir commented Sep 14, 2022

Thanks everybody for your prompt responses. If HostnameVerifier support is not achievable, SSLContext support is still good for our use cases.

trust-all operates in the app level, and we have the need to trust only some of the Rest Clients.

When I said "at least HostnameVerifier" I thought it would be easier, but I am totally happy without HostnameVerifier, as SSLContext will perfectly be valid to get different Rest Clients in the same service with and without SSL verification.

@Sgitario
Copy link
Contributor

Sgitario commented Sep 14, 2022

Unfortunately, it's not expected that HostnameVerifier is supported by Vert.x: eclipse-vertx/vert.x#4065.
The alternative they give is to provide a TrustManager (see comment eclipse-vertx/vert.x#4065 (comment)). However, conceptually TrustManager and HostnameVerifier do not work the same.

And the same happens with supporting a custom SSLContext (see related issue vert-x3/vertx-mongo-client#208).

So, our options are:

  • Allow providing a custom TrustManager. Would this work for you?
  • And/or map the property verifyHost (it would be something like: quarkus.rest-client.client.verify-host=false). At the moment, we do this when we set quarkus.rest-client.client.trust-all=true). The ugly thing is that this would be a Vert.x specific property...

Wdyt? @geoand @Chexpir

Also, I've found that when using a truststore:

quarkus.rest-client.client.trust-store=META-INF/resources/server.truststore
quarkus.rest-client.client.trust-store-password=password

Makes the rest client to fail with the following exception:

 [client] Caused by: io.vertx.core.VertxException: java.security.UnrecoverableKeyException: Get Key failed: Given final block not properly padded. Such issues can arise if a bad key is used during decryption. 
[09:59:27.352] [INFO] [client]  at io.vertx.core.net.impl.SSLHelper.getContext(SSLHelper.java:480) 
[09:59:27.353] [INFO] [client]  at io.vertx.core.net.impl.SSLHelper.getContext(SSLHelper.java:469) 
[09:59:27.353] [INFO] [client]  at io.vertx.core.net.impl.SSLHelper.validate(SSLHelper.java:507) 
[09:59:27.353] [INFO] [client]  at io.vertx.core.net.impl.NetClientImpl.<init>(NetClientImpl.java:95) 
[09:59:27.353] [INFO] [client]  at io.vertx.core.http.impl.HttpClientImpl.<init>(HttpClientImpl.java:155) 
[09:59:27.354] [INFO] [client]  at io.vertx.core.impl.VertxImpl.createHttpClient(VertxImpl.java:338) 
[09:59:27.354] [INFO] [client]  at io.vertx.core.impl.VertxImpl.createHttpClient(VertxImpl.java:350) 
[09:59:27.354] [INFO] [client]  at org.jboss.resteasy.reactive.client.impl.ClientImpl.<init>(ClientImpl.java:170) 
[09:59:27.354] [INFO] [client]  at org.jboss.resteasy.reactive.client.impl.ClientBuilderImpl.build(ClientBuilderImpl.java:244) 
[09:59:27.354] [INFO] [client]  at io.quarkus.rest.client.reactive.runtime.RestClientBuilderImpl.build(RestClientBuilderImpl.java:332) 
[09:59:27.355] [INFO] [client]  at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.build(RestClientCDIDelegateBuilder.java:64) 
[09:59:27.355] [INFO] [client]  at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.createDelegate(RestClientCDIDelegateBuilder.java:42) 
[09:59:27.355] [INFO] [client]  at io.quarkus.rest.client.reactive.runtime.RestClientReactiveCDIWrapperBase.<init>(RestClientReactiveCDIWrapperBase.java:20) 
[09:59:27.355] [INFO] [client]  at io.jester.examples.quarkus.greetings.Client$$CDIWrapper.<init>(Unknown Source) 
[09:59:27.356] [INFO] [client]  at io.jester.examples.quarkus.greetings.Client$$CDIWrapper_ClientProxy.<init>(Unknown Source) 
[09:59:27.356] [INFO] [client]  at io.jester.examples.quarkus.greetings.Client$$CDIWrapper_Bean.proxy(Unknown Source) 
[09:59:27.356] [INFO] [client]  at io.jester.examples.quarkus.greetings.Client$$CDIWrapper_Bean.get(Unknown Source) 
[09:59:27.356] [INFO] [client]  at io.jester.examples.quarkus.greetings.Client$$CDIWrapper_Bean.get(Unknown Source) 
[09:59:27.357] [INFO] [client]  ... 26 more 

I have a fix for this: #27925

@famod
Copy link
Member

famod commented Sep 14, 2022

My 2 cents:

Allow providing a custom TrustManager.

How would an implemetation look like that tries to mimic io.quarkus.restclient.NoopHostnameVerifier?
As already mentioned, the manager approach would probably not verify much more than just the host?
Could Quarkus provide such a "workaround" manager OOTB?

Also, I assume that with this workaround, Quarkus would still properly set up the KeyManager?
Reason I'm asking is that it's too easy to shoot yourself in the foot when setting up SSLContext, e.g. passing null for the managers by mistake because you only want to set the TLS version (just had that in my project).

To me, the second option looks better for now.

@famod
Copy link
Member

famod commented Sep 14, 2022

trust-all operates in the app level, and we have the need to trust only some of the Rest Clients.

+💯

@geoand
Copy link
Contributor

geoand commented Sep 14, 2022

I personally think that if we properly document how to address the common concerns, we don't really need to have HostnameVerifier "emulation".

@famod
Copy link
Member

famod commented Sep 14, 2022

My point is that it's not that rare to skip hostname verification, so it would be much more developer friendly if Quarkus would close that gap by shipping something that can be used OOTB.
Otherwise people woud need to implement their own Manager and carry it around from project to project to cover a concern that most would expect to be just a flag to enable. Just saying. 😉

@geoand
Copy link
Contributor

geoand commented Sep 14, 2022

Sure yeah. My point is that we don't need to necessarily have a HostnameVerifier emulation layer - I think it's acceptable to have something different but equally easily usable.

@famod
Copy link
Member

famod commented Sep 14, 2022

My point is that we don't need to necessarily have a HostnameVerifier emulation layer - I think it's acceptable to have something different but equally easily usable.

Agreed!
In the end I just need the client to not verify the hostname (without skipping every other verification), so I don't need such a layer.

@Sgitario
Copy link
Contributor

If what we want is to skip the hostname verification, we can achieve this by setting 'verifyHost(false)'. All we would need is to add a new property for setting this flag.

Sgitario added a commit to Sgitario/quarkus that referenced this issue Sep 15, 2022
Before these changes, we only can disable the hostname verification in Rest Client classic by providing the following property:

```
quarkus.rest-client.extensions-api.hostname-verifier=io.quarkus.restclient.NoopHostnameVerifier
```

However, this is not working in Rest Client reactive because setting a hostname verifier strategy is not supported by the Vert-x HTTP Client. 

With these changes, we have added a new property in both Rest Client classic and reactive `quarkus.rest-client.extensions-api.verify-host=true or false`. In Rest Client classic, when disabling the verify host, internally it will add the `NoopHostnameVerifier` strategy. In Rest Client reactive, it will properly configure the Vert.x HTTP client to disable the hostname verification. Therefore, in both Rest Client implementations (classic and reactive), the behaviour is the same. 

Fix quarkusio#27901
@Chexpir
Copy link
Contributor Author

Chexpir commented Sep 16, 2022

Thanks everybody for your inputs!

Hi @Sgitario ! I am sure most of the developer use cases will be covered with that, and developer experience using it is very straightforward.

I am unsure if our specific use case is covered, as due to the dynamic base URLs we have, we are forced to use RestClientBuilders programatically.

Sgitario added a commit to Sgitario/quarkus that referenced this issue Nov 2, 2022
Before these changes, we only can disable the hostname verification in Rest Client classic by providing the following property:

```
quarkus.rest-client.extensions-api.hostname-verifier=io.quarkus.restclient.NoopHostnameVerifier
```

However, this is not working in Rest Client reactive because setting a hostname verifier strategy is not supported by the Vert-x HTTP Client. 

With these changes, we have added a new property in both Rest Client classic and reactive `quarkus.rest-client.extensions-api.verify-host=true or false`. In Rest Client classic, when disabling the verify host, internally it will add the `NoopHostnameVerifier` strategy. In Rest Client reactive, it will properly configure the Vert.x HTTP client to disable the hostname verification. Therefore, in both Rest Client implementations (classic and reactive), the behaviour is the same. 

Fix quarkusio#27901
geoand pushed a commit to Sgitario/quarkus that referenced this issue Dec 5, 2022
Before these changes, we only can disable the hostname verification in Rest Client classic by providing the following property:

```
quarkus.rest-client.extensions-api.hostname-verifier=io.quarkus.restclient.NoopHostnameVerifier
```

However, this is not working in Rest Client reactive because setting a hostname verifier strategy is not supported by the Vert-x HTTP Client. 

With these changes, we have added a new property in both Rest Client classic and reactive `quarkus.rest-client.extensions-api.verify-host=true or false`. In Rest Client classic, when disabling the verify host, internally it will add the `NoopHostnameVerifier` strategy. In Rest Client reactive, it will properly configure the Vert.x HTTP client to disable the hostname verification. Therefore, in both Rest Client implementations (classic and reactive), the behaviour is the same. 

Fix quarkusio#27901
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 5, 2022
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.0.Final Dec 6, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Dec 6, 2022
Before these changes, we only can disable the hostname verification in Rest Client classic by providing the following property:

```
quarkus.rest-client.extensions-api.hostname-verifier=io.quarkus.restclient.NoopHostnameVerifier
```

However, this is not working in Rest Client reactive because setting a hostname verifier strategy is not supported by the Vert-x HTTP Client.

With these changes, we have added a new property in both Rest Client classic and reactive `quarkus.rest-client.extensions-api.verify-host=true or false`. In Rest Client classic, when disabling the verify host, internally it will add the `NoopHostnameVerifier` strategy. In Rest Client reactive, it will properly configure the Vert.x HTTP client to disable the hostname verification. Therefore, in both Rest Client implementations (classic and reactive), the behaviour is the same.

Fix quarkusio#27901

(cherry picked from commit 941d3a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants