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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Hostname verifier for the Jetty connector #5278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ...rs/jetty-connector/src/main/java/org/glassfish/jersey/jetty/connector/JettyConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2022 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2023 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -154,16 +154,20 @@ class JettyConnector implements Connector {
}
if (httpClient == null) {
final SSLContext sslContext = jaxrsClient.getSslContext();
final SslContextFactory sslContextFactory = new SslContextFactory();
final SslContextFactory.Client sslContextFactory = new SslContextFactory.Client(false);
sslContextFactory.setSslContext(sslContext);
httpClient = new HttpClient(sslContextFactory);
}
this.client = httpClient;

Boolean enableHostnameVerification = (Boolean) config.getProperties()
.get(JettyClientProperties.ENABLE_SSL_HOSTNAME_VERIFICATION);
if (enableHostnameVerification != null && enableHostnameVerification) {
client.getSslContextFactory().setEndpointIdentificationAlgorithm("https");
if (enableHostnameVerification != null) {
final String verificationAlgorithm = enableHostnameVerification ? "HTTPS" : null;
client.getSslContextFactory().setEndpointIdentificationAlgorithm(verificationAlgorithm);
}
if (jaxrsClient.getHostnameVerifier() != null) {
client.getSslContextFactory().setHostnameVerifier(jaxrsClient.getHostnameVerifier());
}

final Object connectTimeout = config.getProperties().get(ClientProperties.CONNECT_TIMEOUT);
Expand Down
23 changes: 16 additions & 7 deletions ...org/glassfish/jersey/tests/e2e/client/connector/ssl/SslConnectorHostnameVerifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2022 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2023 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -31,8 +31,9 @@
import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
import org.glassfish.jersey.client.spi.ConnectorProvider;
import org.glassfish.jersey.grizzly.connector.GrizzlyConnectorProvider;
import org.glassfish.jersey.jetty.connector.JettyConnectorProvider;
import org.glassfish.jersey.jetty.connector.JettyClientProperties;

import org.glassfish.jersey.jetty.connector.JettyConnectorProvider;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -66,32 +67,40 @@ protected String clientTrustStore() {
@ParameterizedTest
@MethodSource("testData")
public void testHostnameVerifierApplied(ConnectorProvider connectorProvider) throws Exception {
// Grizzly and Jetty connectors don't support Hostname Verification
if (isExcluded(Arrays.asList(GrizzlyConnectorProvider.class, JettyConnectorProvider.class), connectorProvider)) {
// Grizzly connector does not support Hostname Verification
if (isExcluded(Arrays.asList(GrizzlyConnectorProvider.class), connectorProvider)) {
return;
}

final Client client = ClientBuilder.newBuilder()
.withConfig(new ClientConfig().connectorProvider(connectorProvider))
.withConfig(getClientConfig(JettyConnectorProvider.class.isAssignableFrom(connectorProvider.getClass()))
.connectorProvider(connectorProvider))
.register(HttpAuthenticationFeature.basic("user", "password"))
.hostnameVerifier(new CustomHostnameVerifier())
.sslContext(getSslContext())
.build();

try {
client.target(Server.BASE_URI).request().get(Response.class);
fail("HostnameVerifier was not applied.");
fail("HostnameVerifier was not applied by " + connectorProvider.getClass());
} catch (ProcessingException pex) {
CustomHostnameVerifier.HostnameVerifierException hve = getHVE(pex);

if (hve != null) {
assertEquals(CustomHostnameVerifier.EX_VERIFIER_MESSAGE, hve.getMessage());
} else {
fail("Invalid wrapped exception.");
fail("Invalid wrapped exception.", pex);
}
}
}

private static final ClientConfig getClientConfig(boolean enableSslHostnameVerification) {
final ClientConfig config = new ClientConfig();
return enableSslHostnameVerification ? config
.property(JettyClientProperties.ENABLE_SSL_HOSTNAME_VERIFICATION, Boolean.FALSE)
Copy link
Member

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?.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

: config;
}

private boolean isExcluded(List<Class<? extends ConnectorProvider>> excluded, ConnectorProvider connectorProvider) {
for (Class<?> clazz : excluded) {
if (clazz.isAssignableFrom(connectorProvider.getClass())) {
Expand Down