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

Allow hostname verification to be disabled through arquillian.xml #48

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

codingchili
Copy link

Short description of what this resolves:

Allows disabling hostname verification for the WLPRestClient used in the remote adapter.

Changes proposed in this pull request:

  • adds 'disableHostnameVerification', boolean: true|false as a configuration option to arquillian.xml
  • hostname verification is enabled by default.
  • configures the SSLContext with a trust manager that trusts all certificates.
  • sets the NoopHostnameVerifier on the http client.

Fixes: #47

Copy link
Contributor

@aguibert aguibert left a comment

Choose a reason for hiding this comment

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

hi, thanks for the PR!

Code changes look good, but lets change the property name from disableHostnameVerification to hostnameVerificationEnabled to avoid the double-negative.

@codingchili
Copy link
Author

Hi, thanks!

Updating next Tuesday.

Copy link
Member

@Azquelt Azquelt left a comment

Choose a reason for hiding this comment

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

Disabling hostname verification is not the same as trusting all certificates.

If the intent is just to disable hostname verification, it needs to do just that (i.e. just the change to add the NoopHostnameVerifier).

If the intent is to allow untrusted certificates, then I think the name of the configuration parameter should be changed. Maybe something like connectionSecurityEnabled or useInsecureConnection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants