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

Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

Merged
merged 13 commits into from
Sep 22, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,17 @@ public ClientFactoryBuilder tlsNoVerify() {
return this;
}

/**
* Disables the verification of server's key certificate chain for specific hosts.
tumile marked this conversation as resolved.
Show resolved Hide resolved
* <strong>Note:</strong> You should never use this in production but only for a testing purpose.
*
* @see #tlsCustomizer(Consumer)
*/
public ClientFactoryBuilder tlsNoVerifyHosts(String... insecureHosts) {
tlsCustomizer(b -> b.trustManager(IgnoreHostsTrustManager.of(insecureHosts)));
return this;
}
Copy link
Member

Choose a reason for hiding this comment

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

A user might want to do the following:

cfb.tlsNoVerifyHosts("a.com");
cfb.tlsNoVerifyHosts("b.com");
...

How about keeping the list of the insecure hosts and setting the trust manager when the ClientFactory is built? We will also have to make sure tlsNoVerify() and tlsNoVerifyHosts() are mutually exclusive, throwing an IllegalStateException (or should we make tlsNoVerifyHost() always win?)

Copy link
Contributor Author

@tumile tumile Sep 20, 2020

Choose a reason for hiding this comment

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

I agree with making them exclusive. Makes it less ambiguous about which hosts are being ignored (or all).


/**
* Adds the {@link Consumer} which can arbitrarily configure the {@link SslContextBuilder} that will be
* applied to the SSL session. For example, use {@link SslContextBuilder#trustManager(TrustManagerFactory)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
* Copyright 2020 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client;

import static java.util.Objects.requireNonNull;

import java.net.Socket;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Set;

import javax.net.ssl.SSLEngine;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedTrustManager;

import com.google.common.collect.ImmutableSet;

/**
* An implementation of {@link X509ExtendedTrustManager} that skips verification on whitelisted hosts.
*/
final class IgnoreHostsTrustManager extends X509ExtendedTrustManager {

private final X509ExtendedTrustManager delegate;
private final Set<String> insecureHosts;

IgnoreHostsTrustManager(X509ExtendedTrustManager delegate, Set<String> insecureHosts) {
this.delegate = delegate;
this.insecureHosts = insecureHosts;
}

/**
* Returns new {@link IgnoreHostsTrustManager} instance.
*/
static IgnoreHostsTrustManager of(String... insecureHosts) {
tumile marked this conversation as resolved.
Show resolved Hide resolved
X509ExtendedTrustManager delegate = null;
try {
final TrustManagerFactory trustManagerFactory = TrustManagerFactory
.getInstance(TrustManagerFactory.getDefaultAlgorithm());
trustManagerFactory.init((KeyStore) null);
final TrustManager[] trustManagers = trustManagerFactory.getTrustManagers();
for (TrustManager tm : trustManagers) {
if (tm instanceof X509ExtendedTrustManager) {
delegate = (X509ExtendedTrustManager) tm;
break;
}
}
} catch (GeneralSecurityException ignored) {
// ignore
}
requireNonNull(delegate, "cannot resolve default trust manager");
return new IgnoreHostsTrustManager(delegate, ImmutableSet.copyOf(insecureHosts));
}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s, Socket socket)
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
throws CertificateException {
if (!insecureHosts.contains(socket.getInetAddress().getHostName())) {
tumile marked this conversation as resolved.
Show resolved Hide resolved
delegate.checkServerTrusted(x509Certificates, s, socket);
}
}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s, SSLEngine sslEngine)
throws CertificateException {
if (!insecureHosts.contains(sslEngine.getPeerHost())) {
delegate.checkServerTrusted(x509Certificates, s, sslEngine);
}
}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
throws CertificateException {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also implement this method?

Copy link
Contributor Author

@tumile tumile Sep 19, 2020

Choose a reason for hiding this comment

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

It does not give access to the hostname so I wasn't sure if it should be implemented.
It's just me if you think otherwise I'll add it 😃

Copy link
Member

Choose a reason for hiding this comment

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

It's probably fine not to implement this method and the method that accepts Socket because we always use an SSLEngine for TLS communication. If we have to implement this, we could retrieve the host name from x509Certificates[0].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for chiming in. I'm not entirely familiar with TLS so advice is much appreciated here. But it looks to me that we don't need this right now, so maybe it can be added down the road if needed?

Copy link
Member

@minwoox minwoox Sep 21, 2020

Choose a reason for hiding this comment

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

SGTM. 👍

}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s, Socket socket)
throws CertificateException {
throw new UnsupportedOperationException();
}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s, SSLEngine sslEngine)
throws CertificateException {
throw new UnsupportedOperationException();
}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
throws CertificateException {
throw new UnsupportedOperationException();
}

@Override
public X509Certificate[] getAcceptedIssuers() {
return delegate.getAcceptedIssuers();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.linecorp.armeria.client;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.security.cert.X509Certificate;
import java.util.concurrent.CompletableFuture;
Expand Down Expand Up @@ -115,17 +116,6 @@ private static void testMismatch(String fqdn) throws Exception {
assertThat(get(fqdn)).isEqualTo("b.com: CN=b.com");
}

private static String get(String fqdn) throws Exception {
final WebClient client = WebClient.builder("https://" + fqdn + ':' + httpsPort)
.factory(clientFactory)
.build();

final AggregatedHttpResponse response = client.get("/").aggregate().get();

assertThat(response.status()).isEqualTo(HttpStatus.OK);
return response.contentUtf8();
}

@Test
void testCustomAuthority() throws Exception {
final WebClient client = WebClient.builder(SessionProtocol.HTTPS,
Expand All @@ -152,6 +142,40 @@ void testCustomAuthorityWithAdditionalHeaders() throws Exception {
}
}

@Test
void testTlsNoVerifyHosts() throws Exception {
try (ClientFactory clientFactoryIgnoreHosts = ClientFactory.builder()
.tlsNoVerifyHosts("a.com", "b.com")
.addressResolverGroupFactory(group -> MockAddressResolverGroup.localhost())
.build()) {
assertThat(get("a.com", clientFactoryIgnoreHosts)).isEqualTo("a.com: CN=a.com");
minwoox marked this conversation as resolved.
Show resolved Hide resolved
assertThatThrownBy(() -> get("c.com", clientFactoryIgnoreHosts))
.hasStackTraceContaining("javax.net.ssl.SSLHandshakeException");
}
}

private static String get(String fqdn) throws Exception {
final WebClient client = WebClient.builder("https://" + fqdn + ':' + httpsPort)
.factory(clientFactory)
.build();

final AggregatedHttpResponse response = client.get("/").aggregate().get();

assertThat(response.status()).isEqualTo(HttpStatus.OK);
return response.contentUtf8();
tumile marked this conversation as resolved.
Show resolved Hide resolved
}

private static String get(String fqdn, ClientFactory clientFactory) throws Exception {
final WebClient client = WebClient.builder("https://" + fqdn + ':' + httpsPort)
.factory(clientFactory)
.build();

final AggregatedHttpResponse response = client.get("/").aggregate().get();

assertThat(response.status()).isEqualTo(HttpStatus.OK);
return response.contentUtf8();
}

private static class SniTestService extends AbstractHttpService {

private final String domainName;
Expand Down
Loading