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 use of custom HostnameVerifier on clients. #1664

Merged
merged 1 commit into from Aug 4, 2016

Conversation

Projects
None yet
4 participants
@tbartley
Contributor

tbartley commented Aug 2, 2016

While the improvements to TLS configuration of HTTP clients in 1.0.0
(maybe prior) are awesome, as part of that process the ability to set a
custom HostnameVerifier easily on the HTTP client has been lost.

You used to be able to do e.g. as:

JerseyClientConfiguration myJerseyClientConfiguration = ;
HostnameVerifier verifier = new MyCustomHostnameVerifier();
JerseyClientBuilder clientBuilder = new JerseyClientBuilder(env);
clientBuilder.using(myJerseyClientConfiguration).using(verifier);
Client httpClient = clientBuilder.build();
Same is true for HttpClientBuilder too.

You can still do it by creating a custom Apache
Registry but you need to set up socket
factories for every scheme.

This change restores the ability to set a custom HostnameVerifier
for clients.

[Fixes #1663]

@tbartley

This comment has been minimized.

Contributor

tbartley commented Aug 3, 2016

Also, I wasn't sure how to approach writing a unit test in HttpClientBuilderTest.java. Some guidance there would be appreciated.

@coveralls

This comment has been minimized.

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.05%) to 82.153% when pulling bc5c482 on anonyome:tbartley/dw-client-custom-hostname-verifier into 0880fcd on dropwizard:release/1.0.x.

public DropwizardSSLConnectionSocketFactory(TlsConfiguration configuration) {
this.configuration = configuration;
this.verifier = null;

This comment has been minimized.

@arteam

arteam Aug 3, 2016

Member

I think we could reduce this constructor to a call to another constructor:

this(configuration, null);

This comment has been minimized.

@tbartley

tbartley Aug 3, 2016

Contributor

Sure

@@ -48,7 +55,12 @@ public SSLConnectionSocketFactory getSocketFactory() throws SSLInitializationExc
private HostnameVerifier chooseHostnameVerifier() {
HostnameVerifier hostnameVerifier;
if (configuration.isVerifyHostname()) {
hostnameVerifier = SSLConnectionSocketFactory.getDefaultHostnameVerifier();
if (verifier == null) {

This comment has been minimized.

@arteam

arteam Aug 3, 2016

Member

What do you think about reducing the size of this method by using the ternary operator and several returns?
Something like that:

if (configuration.isVerifyHostname()) {
    return verifier != null ? verifier : SSLConnectionSocketFactory.getDefaultHostnameVerifier();
} else {
    return new NoopHostnameVerifier();
}

This comment has been minimized.

@tbartley

tbartley Aug 3, 2016

Contributor

Sure

@@ -378,9 +396,15 @@ protected InstrumentedHttpClientConnectionManager createConnectionManager(Regist
final SSLConnectionSocketFactory sslConnectionSocketFactory;
if (configuration.getTlsConfiguration() == null) {
sslConnectionSocketFactory = SSLConnectionSocketFactory.getSocketFactory();
if (verifier == null) {

This comment has been minimized.

@arteam

arteam Aug 3, 2016

Member

If I understand correctly, setting a HostnameVerifier means using a TlsConfiguration. What do you think about extracting TlsConfiguration to a local mutable variable?

TlsConfiguration tlsConfiguration = configuration.getTlsConfiguration();
if (tlsConfiguration == null && verifier != null) {
    tlsConfiguration = new TlsConfiguration();
}

final SSLConnectionSocketFactory sslConnectionSocketFactory;
if (tlsConfiguration == null) {
    sslConnectionSocketFactory = SSLConnectionSocketFactory.getSocketFactory();
} else {
    sslConnectionSocketFactory = new DropwizardSSLConnectionSocketFactory(tlsConfiguration,
        verifier).getSocketFactory();
}

This comment has been minimized.

@tbartley

tbartley Aug 3, 2016

Contributor

Sure

private static class FailVerifier implements HostnameVerifier {
@Override
public boolean verify(String arg0, SSLSession arg1) {

This comment has been minimized.

@arteam

arteam Aug 3, 2016

Member

Could you please replace tabs to spaces? A nitpick, but we prefer to stick to spaces across the project.

This comment has been minimized.

@tbartley

tbartley Aug 3, 2016

Contributor

Ach! I'd been diligent about that elsewhere. Missed it here. Thanks.

@arteam

This comment has been minimized.

Member

arteam commented Aug 3, 2016

Also, I wasn't sure how to approach writing a unit test in HttpClientBuilderTest.java. Some guidance there would be appreciated.

I think you could mark the method createConfiguredRegistry as package protected and put the @VisibleForTesting annotation on it. Then you could invoke it in tests:

HostnameVerifier customVerifier = (s, sslSession) -> false;
Registry<ConnectionSocketFactory> configuredRegistry = builder
    .using(customVerifier)
    .createConfiguredRegistry();

Then we could retrieve the SSL socket factory from the registry and extract the verifier from it via reflection. Something like that:

SSLConnectionSocketFactory socketFactory = 
(SSLConnectionSocketFactory) configuredRegistry.lookup("https");
final Field hostnameVerifierField =
    FieldUtils.getField(SSLConnectionSocketFactory.class, "hostnameVerifier", true);
Allow use of custom HostnameVerifier on clients.
While the improvements to TLS configuration of HTTP clients in 1.0.0
(maybe prior) are awesome, as part of that process the ability to set a
custom HostnameVerifier easily on the HTTP client has been lost.

You used to be able to do e.g. as:

JerseyClientConfiguration myJerseyClientConfiguration = <some
configuration>;
HostnameVerifier verifier = new MyCustomHostnameVerifier();
JerseyClientBuilder clientBuilder = new JerseyClientBuilder(env);
clientBuilder.using(myJerseyClientConfiguration).using(verifier);
Client httpClient = clientBuilder.build();
Same is true for HttpClientBuilder too.

You can still do it by creating a custom Apache
Registry<ConnectionSocketFactory> but you need to set up socket
factories for every scheme.

This change restores the ability to set a custom HostnameVerifier
for clients.

[Fixes #1663]
@tbartley

This comment has been minimized.

Contributor

tbartley commented Aug 3, 2016

Thanks for the feedback @arteam I've made your suggested changes and implemented additional unit test coverage following your suggested pattern.

I ummed and ahhed over the DropwizardSSLConnectionSocketFactory constructor unit test using reflection to check private field value but I liked using a getter for the verifier field on this class even less since it would imply getter for tlsConfiguration field for consistency which exposes that to internal modification which implies need for immutable TlsConfiguration .... Also, the getter would've needed to be public rather than package protected since unit test is in a different package.

I have also squashed my commits.

@coveralls

This comment has been minimized.

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.03%) to 82.23% when pulling 160502f on anonyome:tbartley/dw-client-custom-hostname-verifier into 0880fcd on dropwizard:release/1.0.x.

@jplock jplock added this to the 1.0.1 milestone Aug 3, 2016

@jplock jplock added the improvement label Aug 3, 2016

@arteam arteam merged commit b536012 into dropwizard:release/1.0.x Aug 4, 2016

@arteam

This comment has been minimized.

Member

arteam commented Aug 4, 2016

Thank you very much for your contribution!

@tbartley

This comment has been minimized.

Contributor

tbartley commented Aug 4, 2016

No problem at all. This software works so well for us. Thank you for the far greater contribution you make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment