-
Notifications
You must be signed in to change notification settings - Fork 356
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
Hostname verifier for the Jetty connector #5278
Conversation
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
private static final ClientConfig getClientConfig(boolean enableSslHostnameVerification) { | ||
final ClientConfig config = new ClientConfig(); | ||
return enableSslHostnameVerification ? config | ||
.property(JettyClientProperties.ENABLE_SSL_HOSTNAME_VERIFICATION, Boolean.FALSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused with this.
If enableSslHostnameVerification == true, then the config contains the property JettyClientProperties.ENABLE_SSL_HOSTNAME_VERIFICATION = Boolean.FALSE
It looks to me it should be in vice-versa, is this intentional?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the other way around. Does the user need to set property(JettyClientProperties.ENABLE_SSL_HOSTNAME_VERIFICATION, Boolean.FALSE)
when the custom hostnameverifier is registered? Maybe it can be set to false automatically, can it not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the given test is built in a way for default hostname verification to be failed.
private static final String CLIENT_TRUST_STORE = "truststore-example_com-client";
private static final String SERVER_KEY_STORE = "keystore-example_com-server";
given trust and key stores are for the example.com hostname instead of the localhost. This is done because most of the connectors work in a way that when default hostname verification is failed, the validation is passed to custom hostname verification if any. However Jetty connector works in the opposite way - when default hostname verification is failed, the flow is finished with an exception without passing validation to a custom hostname verifier. That is why default hostname validation should be disabled and only then custom hostname verifier can be tested.
Regarding the other way around - probably not, because hostnames are not necessarily different - those could be localhost/localhost and all would be fine, the custom hostname verifier would be involved in this case. This particular test actually demonstrates only a corner case for generally invalid hostname vs certificate. But in most cases, default hostname validation would pass.
No description provided.