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

Rest Client: Add property to skip hostname verification #27956

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

Sgitario
Copy link
Contributor

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 #27901

@geoand
Copy link
Contributor

geoand commented Sep 15, 2022

@famod mind trying this out?

@famod
Copy link
Member

famod commented Sep 15, 2022

@geoand I'd love to, but right now I'm standing in the middle of chaos because on the weekend we're finally relocating to our new house.
So better not wait for me.

@geoand
Copy link
Contributor

geoand commented Sep 15, 2022

Hope all goes well with the move :)

@geoand
Copy link
Contributor

geoand commented Sep 15, 2022

@Chexpir would you like to test this before we merge it?

gsmet
gsmet previously requested changes Sep 19, 2022
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked an important question :).

@Chexpir
Copy link
Contributor

Chexpir commented Sep 23, 2022

@geoand thanks for the proposition, no need to test it. I've reviewed the code and I trust that it will work!
Anyway, it will be useful to know the process to build and test github branches, it might become handy in the future, as we are into Quarkus for the long term! (I have searched and could not find anything).

@geoand
Copy link
Contributor

geoand commented Sep 23, 2022

I would say that the easiest way to checkout a Pull Request is following this.
Once that is done, you can build it locally by following this.

@geoand
Copy link
Contributor

geoand commented Oct 19, 2022

@Chexpir were you able to test this one?

@geoand
Copy link
Contributor

geoand commented Nov 1, 2022

Were are we with this one?

I personally think we should get it in

@Sgitario
Copy link
Contributor Author

Sgitario commented Nov 2, 2022

Were are we with this one?

I personally think we should get it in

My +100 to get it in.

@cescoffier
Copy link
Member

My last comment was to make sure we clearly say in the doc that skipping host name verification should not be used in prod ('use it to your own risk')

@geoand
Copy link
Contributor

geoand commented Nov 2, 2022

We should 100% add that

@Sgitario
Copy link
Contributor Author

Sgitario commented Nov 2, 2022

PR updated with adding this warning to the docs.

@quarkus-bot

This comment has been minimized.

@Sgitario Sgitario requested a review from gsmet November 8, 2022 12:44
@Sgitario
Copy link
Contributor Author

Any updates on this pull request? I would like to proceed to merge it if everybody agrees.

@fasuke85
Copy link

Hi, it would be awsome if you merged this PR 😄

@oven
Copy link

oven commented Nov 30, 2022

@Sgitario This issue is a blocker for us because we have to communicate with an HTTPS service over an ssh tunnel. Please merge :)

@geoand
Copy link
Contributor

geoand commented Nov 30, 2022

@gsmet any last words or should I dismiss the old review?

@geoand geoand dismissed gsmet’s stale review December 5, 2022 07:40

removing stale review

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
Copy link
Contributor

geoand commented Dec 5, 2022

Since the last CI run of this was a while ago, I rebased onto main.

@geoand geoand added triage/backport? triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/backport-2.13 labels Dec 5, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 5, 2022

Failing Jobs - Building 941d3a6

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@geoand geoand merged commit 032d694 into quarkusio:main Dec 5, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 5, 2022
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Dec 5, 2022
@Sgitario Sgitario deleted the skip_verify_host branch December 5, 2022 09:16
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.0.Final Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RestClient Reactive - set HostnameVerifier or SSLContext
8 participants