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
5 changes: 5 additions & 0 deletions NOTICE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ This product contains a modified part of Twitter Commons, distributed by Twitter
* License: license/LICENSE.twitter.al20.txt (Apache License v2.0)
* Homepage: https://twitter.github.io/commons/

This product contains a modified part of OkHttp, distributed by Square, Inc:

* License: license/LICENSE.okhttp.al20.txt (Apache License v2.0)
* Homepage: https://square.github.io/okhttp


Dependencies
============
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@
import java.net.ProxySelector;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.ToIntFunction;
Expand Down Expand Up @@ -103,6 +106,8 @@ public final class ClientFactoryBuilder {
private int maxNumEventLoopsPerEndpoint;
private int maxNumEventLoopsPerHttp1Endpoint;
private final List<ToIntFunction<Endpoint>> maxNumEventLoopsFunctions = new ArrayList<>();
private boolean tlsNoVerifySet;
private final Set<String> insecureHosts = new HashSet<>();

ClientFactoryBuilder() {
connectTimeoutMillis(Flags.defaultConnectTimeoutMillis());
Expand Down Expand Up @@ -242,15 +247,30 @@ private void channelOptions(Map<ChannelOption<?>, Object> newChannelOptions) {
}

/**
* Disables the verification of server's key certificate chain. This method is a shortcut for:
* {@code tlsCustomizer(b -> b.trustManager(InsecureTrustManagerFactory.INSTANCE))}.
* Disables the verification of server's TLS certificate chain. If you want to disable verification for
* only specific hosts, use {@link #tlsNoVerifyHosts(String...)}.
* <strong>Note:</strong> You should never use this in production but only for a testing purpose.
*
* @see InsecureTrustManagerFactory
* @see #tlsCustomizer(Consumer)
*/
public ClientFactoryBuilder tlsNoVerify() {
tlsCustomizer(b -> b.trustManager(InsecureTrustManagerFactory.INSTANCE));
checkState(insecureHosts.isEmpty(), "tlsNoVerify() and tlsNoVerifyHosts() are mutually exclusive.");
tlsNoVerifySet = true;
return this;
}

/**
* Disables the verification of server's TLS certificate chain for specific hosts. If you want to disable
* all verification, use {@link #tlsNoVerify()} .
* <strong>Note:</strong> You should never use this in production but only for a testing purpose.
*
* @see IgnoreHostsTrustManager
minwoox marked this conversation as resolved.
Show resolved Hide resolved
* @see #tlsCustomizer(Consumer)
*/
public ClientFactoryBuilder tlsNoVerifyHosts(String... insecureHosts) {
checkState(!tlsNoVerifySet, "tlsNoVerify() and tlsNoVerifyHosts() are mutually exclusive.");
this.insecureHosts.addAll(Arrays.asList(insecureHosts));
return this;
}

Expand Down Expand Up @@ -602,6 +622,12 @@ private ClientFactoryOptions buildOptions() {
return ClientFactoryOptions.ADDRESS_RESOLVER_GROUP_FACTORY.newValue(addressResolverGroupFactory);
});

if (tlsNoVerifySet) {
tlsCustomizer(b -> b.trustManager(InsecureTrustManagerFactory.INSTANCE));
} else if (!insecureHosts.isEmpty()) {
tlsCustomizer(b -> b.trustManager(IgnoreHostsTrustManager.of(insecureHosts)));
}

final ClientFactoryOptions newOptions = ClientFactoryOptions.of(options.values());
final long idleTimeoutMillis = newOptions.idleTimeoutMillis();
final long pingIntervalMillis = newOptions.pingIntervalMillis();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* 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.
*/
/*
* Copyright (C) 2020 Square, Inc.
*
* Licensed 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.InetSocketAddress;
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;

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

// Forked from okhttp-4.9.0
// https://github.com/square/okhttp/blob/1364ea44ae1f1c4b5a1cc32e4e7b51d23cb78517/okhttp-tls/src/main/kotlin/okhttp3/tls/internal/InsecureExtendedTrustManager.kt

/**
* Returns new {@link IgnoreHostsTrustManager} instance.
*/
static IgnoreHostsTrustManager of(Set<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, insecureHosts);
minwoox marked this conversation as resolved.
Show resolved Hide resolved
}

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

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

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String authType, Socket socket)
throws CertificateException {
if (!insecureHosts.contains(((InetSocketAddress) socket.getRemoteSocketAddress()).getHostString())) {
delegate.checkServerTrusted(x509Certificates, authType, socket);
}
}

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

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String authType)
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 authType, Socket socket)
throws CertificateException {
throw new UnsupportedOperationException();
}

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

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

@Override
public X509Certificate[] getAcceptedIssuers() {
return delegate.getAcceptedIssuers();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,23 @@ void maxNumEventLoopsAndEventLoopSchedulerFactoryAreMutuallyExclusive() {
.hasMessageContaining("mutually exclusive");
}

@Test
void tlsNoVerifyAndTlsNoVerifyHostsAreMutuallyExclusive() {
final ClientFactoryBuilder builder1 = ClientFactory.builder();
builder1.tlsNoVerify();

assertThatThrownBy(() -> builder1.tlsNoVerifyHosts("localhost"))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("mutually exclusive");

final ClientFactoryBuilder builder2 = ClientFactory.builder();
builder2.tlsNoVerifyHosts("localhost");

assertThatThrownBy(builder2::tlsNoVerify)
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("mutually exclusive");
}

@Test
void shouldInheritClientFactoryOptions() {
try (ClientFactory factory1 = ClientFactory.builder()
Expand Down
106 changes: 51 additions & 55 deletions core/src/test/java/com/linecorp/armeria/client/HttpClientSniTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
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;

import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpHeaderNames;
Expand All @@ -35,64 +36,42 @@
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.internal.testing.MockAddressResolverGroup;
import com.linecorp.armeria.server.AbstractHttpService;
import com.linecorp.armeria.server.Server;
import com.linecorp.armeria.server.ServerBuilder;
import com.linecorp.armeria.server.ServerPort;
import com.linecorp.armeria.server.ServiceRequestContext;

import io.netty.handler.ssl.util.SelfSignedCertificate;
import com.linecorp.armeria.testing.junit5.server.ServerExtension;

class HttpClientSniTest {

private static final Server server;

private static int httpsPort;
private static ClientFactory clientFactory;
private static final SelfSignedCertificate sscA;
private static final SelfSignedCertificate sscB;

static {
try {
final ServerBuilder sb = Server.builder();
sscA = new SelfSignedCertificate("a.com");
sscB = new SelfSignedCertificate("b.com");

@RegisterExtension
static ServerExtension server = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
sb.virtualHost("a.com")
.service("/", new SniTestService("a.com"))
.tls(sscA.certificate(), sscA.privateKey())
.and()
.defaultVirtualHost()
.defaultHostname("b.com")
.service("/", new SniTestService("b.com"))
.tls(sscB.certificate(), sscB.privateKey());

server = sb.build();
} catch (Exception e) {
throw new Error(e);
.service("/", new SniTestService("a.com"))
.tlsSelfSigned()
.and()
.defaultVirtualHost()
.defaultHostname("b.com")
.service("/", new SniTestService("b.com"))
.tlsSelfSigned();
minwoox marked this conversation as resolved.
Show resolved Hide resolved
}
}
};

@BeforeAll
static void init() throws Exception {
server.start().get();
httpsPort = server.activePorts().values().stream()
.filter(ServerPort::hasHttps).findAny().get().localAddress()
.getPort();
clientFactory =
ClientFactory.builder()
.tlsNoVerify()
.addressResolverGroupFactory(group -> MockAddressResolverGroup.localhost())
.build();
static void init() {
httpsPort = server.httpsPort();
clientFactory = ClientFactory.builder()
.tlsNoVerify()
.addressResolverGroupFactory(group -> MockAddressResolverGroup.localhost())
.build();
}

@AfterAll
static void destroy() throws Exception {
CompletableFuture.runAsync(() -> {
clientFactory.close();
server.stop();
sscA.delete();
sscB.delete();
});
static void destroy() {
clientFactory.close();
}

@Test
Expand All @@ -115,17 +94,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 +120,34 @@ void testCustomAuthorityWithAdditionalHeaders() throws Exception {
}
}

@Test
void testTlsNoVerifyHosts() throws Exception {
try (ClientFactory clientFactoryIgnoreHosts = ClientFactory.builder()
.tlsNoVerifyHosts("a.com", "b.com")
.tlsNoVerifyHosts("c.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("d.com", clientFactoryIgnoreHosts))
.hasStackTraceContaining("javax.net.ssl.SSLHandshakeException");
}
}

private static String get(String fqdn) throws Exception {
return get(fqdn, clientFactory);
}

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