From c72124270516f20b373f890ecdf7ccc52148af77 Mon Sep 17 00:00:00 2001
From: Truc Nguyen <119303279+trnguyencflt@users.noreply.github.com>
Date: Mon, 11 Sep 2023 17:04:46 +0100
Subject: [PATCH] KREST-7955 421 misdirected request if host does not match SNI
(#410)
* SniHandler for checking SNI against host name
* Add config sni.check.enabled to control the SniHandler feature
* Integration tests for SniHandler
* Make tests also parameterized on http2 enabled
---
.../java/io/confluent/rest/Application.java | 5 +
.../java/io/confluent/rest/RestConfig.java | 16 +
.../confluent/rest/handlers/SniHandler.java | 75 +++++
.../rest/SniHandlerIntegrationTest.java | 310 ++++++++++++++++++
4 files changed, 406 insertions(+)
create mode 100644 core/src/main/java/io/confluent/rest/handlers/SniHandler.java
create mode 100644 core/src/test/java/io/confluent/rest/SniHandlerIntegrationTest.java
diff --git a/core/src/main/java/io/confluent/rest/Application.java b/core/src/main/java/io/confluent/rest/Application.java
index 4d954e7fd0..1ef2c75b9b 100644
--- a/core/src/main/java/io/confluent/rest/Application.java
+++ b/core/src/main/java/io/confluent/rest/Application.java
@@ -29,6 +29,7 @@
import io.confluent.rest.exceptions.JsonParseExceptionMapper;
import io.confluent.rest.extension.ResourceExtension;
import io.confluent.rest.filters.CsrfTokenProtectionFilter;
+import io.confluent.rest.handlers.SniHandler;
import io.confluent.rest.metrics.Jetty429MetricsDosFilterListener;
import io.confluent.rest.metrics.JettyRequestMetricsFilter;
import io.confluent.rest.metrics.MetricsResourceMethodApplicationListener;
@@ -412,6 +413,10 @@ public Handler configureHandler() {
requestLogHandler.setRequestLog(requestLog);
context.insertHandler(requestLogHandler);
+ if (config.getSniCheckEnable()) {
+ context.insertHandler(new SniHandler());
+ }
+
HandlerCollection handlers = new HandlerCollection();
handlers.setHandlers(new Handler[]{context});
diff --git a/core/src/main/java/io/confluent/rest/RestConfig.java b/core/src/main/java/io/confluent/rest/RestConfig.java
index 470c97cb71..2e8acb4be5 100644
--- a/core/src/main/java/io/confluent/rest/RestConfig.java
+++ b/core/src/main/java/io/confluent/rest/RestConfig.java
@@ -495,6 +495,12 @@ public class RestConfig extends AbstractConfig {
+ "Java 11 JVM or later. Default is true.";
protected static final boolean HTTP2_ENABLED_DEFAULT = true;
+ public static final String SNI_CHECK_ENABLED_CONFIG = "sni.check.enabled";
+ protected static final String SNI_CHECK_ENABLED_DOC =
+ "Whether or not to check the SNI against the Host header. If the values don't match, "
+ + "returns a 421 misdirected response. Default is false.";
+ protected static final boolean SNI_CHECK_ENABLED_DEFAULT = false;
+
public static final String PROXY_PROTOCOL_ENABLED_CONFIG =
"proxy.protocol.enabled";
protected static final String PROXY_PROTOCOL_ENABLED_DOC =
@@ -1053,6 +1059,12 @@ private static ConfigDef incompleteBaseConfigDef() {
HTTP2_ENABLED_DEFAULT,
Importance.LOW,
HTTP2_ENABLED_DOC
+ ).define(
+ SNI_CHECK_ENABLED_CONFIG,
+ Type.BOOLEAN,
+ SNI_CHECK_ENABLED_DEFAULT,
+ Importance.LOW,
+ SNI_CHECK_ENABLED_DOC
).define(
LISTENER_PROTOCOL_MAP_CONFIG,
Type.LIST,
@@ -1391,6 +1403,10 @@ public final int getNetworkTrafficRateLimitBytesPerSec() {
return getInt(NETWORK_TRAFFIC_RATE_LIMIT_BYTES_PER_SEC_CONFIG);
}
+ public final boolean getSniCheckEnable() {
+ return getBoolean(SNI_CHECK_ENABLED_CONFIG);
+ }
+
/**
*
A helper method for extracting multi-instance application configuration,
* specified via property names of the form PREFIX[.LISTENER_NAME].PROPERTY.
diff --git a/core/src/main/java/io/confluent/rest/handlers/SniHandler.java b/core/src/main/java/io/confluent/rest/handlers/SniHandler.java
new file mode 100644
index 0000000000..7909c028e8
--- /dev/null
+++ b/core/src/main/java/io/confluent/rest/handlers/SniHandler.java
@@ -0,0 +1,75 @@
+/*
+ * Copyright 2014 - 2023 Confluent 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
+ *
+ * http://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 io.confluent.rest.handlers;
+
+import static org.eclipse.jetty.http.HttpStatus.Code.MISDIRECTED_REQUEST;
+
+import java.io.IOException;
+import java.util.List;
+import javax.net.ssl.ExtendedSSLSession;
+import javax.net.ssl.SNIHostName;
+import javax.net.ssl.SNIServerName;
+import javax.net.ssl.SSLSession;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.io.EndPoint;
+import org.eclipse.jetty.io.ssl.SslConnection.DecryptedEndPoint;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.handler.HandlerWrapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SniHandler extends HandlerWrapper {
+ private static final Logger log = LoggerFactory.getLogger(SniHandler.class);
+
+ @Override
+ public void handle(String target, Request baseRequest,
+ HttpServletRequest request,
+ HttpServletResponse response) throws IOException, ServletException {
+ String serverName = request.getServerName();
+ String sniServerName = getSniServerName(baseRequest);
+ if (sniServerName != null && !sniServerName.equals(serverName)) {
+ log.debug("Sni check failed, host header: {}, sni value: {}", serverName, sniServerName);
+ baseRequest.setHandled(true);
+ response.sendError(MISDIRECTED_REQUEST.getCode(), MISDIRECTED_REQUEST.getMessage());
+ }
+ super.handle(target, baseRequest, request, response);
+ }
+
+ private static String getSniServerName(Request baseRequest) {
+ EndPoint endpoint = baseRequest.getHttpChannel().getEndPoint();
+ if (endpoint instanceof DecryptedEndPoint) {
+ SSLSession session = ((DecryptedEndPoint) endpoint)
+ .getSslConnection()
+ .getSSLEngine()
+ .getSession();
+ if (session instanceof ExtendedSSLSession) {
+ List servers = ((ExtendedSSLSession) session).getRequestedServerNames();
+ if (servers != null) {
+ return servers.stream()
+ .findAny()
+ .filter(SNIHostName.class::isInstance)
+ .map(SNIHostName.class::cast)
+ .map(SNIHostName::getAsciiName)
+ .orElse(null);
+ }
+ }
+ }
+ return null;
+ }
+}
diff --git a/core/src/test/java/io/confluent/rest/SniHandlerIntegrationTest.java b/core/src/test/java/io/confluent/rest/SniHandlerIntegrationTest.java
new file mode 100644
index 0000000000..eb34295a72
--- /dev/null
+++ b/core/src/test/java/io/confluent/rest/SniHandlerIntegrationTest.java
@@ -0,0 +1,310 @@
+/*
+ * Copyright 2014 - 2023 Confluent 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
+ *
+ * http://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 io.confluent.rest;
+
+import static io.confluent.rest.TestUtils.getFreePort;
+import static org.eclipse.jetty.http.HttpStatus.Code.MISDIRECTED_REQUEST;
+import static org.eclipse.jetty.http.HttpStatus.Code.OK;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.security.KeyPair;
+import java.security.cert.X509Certificate;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.stream.Stream;
+import javax.net.ssl.SSLContext;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.core.Configurable;
+import javax.ws.rs.core.MediaType;
+import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
+import org.apache.http.ssl.SSLContextBuilder;
+import org.apache.http.ssl.SSLContexts;
+import org.apache.kafka.common.config.types.Password;
+import org.apache.kafka.test.TestSslUtils;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+@Tag("IntegrationTest")
+public class SniHandlerIntegrationTest {
+
+ public static final String TEST_SSL_PASSWORD = "test1234";
+ public static final String KAFKA_REST_HOST = "localhost";
+ public static final String KSQL_HOST = "anotherhost";
+
+ private Server server;
+ private HttpClient httpClient;
+ private Properties props;
+ private File clientKeystore;
+
+ @BeforeEach
+ public void setup(TestInfo info) throws Exception {
+ props = new Properties();
+ }
+
+ @AfterEach
+ public void tearDown() throws Exception {
+ httpClient.stop();
+ server.stop();
+ server.join();
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void test_http_SniHandlerEnabled_no_effect(boolean http2Enabled) throws Exception {
+ props.setProperty(RestConfig.HTTP2_ENABLED_CONFIG, String.valueOf(http2Enabled));
+ props.setProperty(RestConfig.SNI_CHECK_ENABLED_CONFIG, "true");
+ // http doesn't have SNI concept, SNI is an extension for TLS
+ startHttpServer("http");
+ startHttpClient("http");
+
+ ContentResponse response = httpClient.newRequest(server.getURI())
+ .path("/resource")
+ .accept(MediaType.TEXT_HTML)
+ // make Host different from SNI
+ .header(HttpHeader.HOST, "host.value.does.not.matter")
+ .send();
+
+ assertEquals(OK.getCode(), response.getStatus());
+ }
+
+ @ParameterizedTest
+ @MethodSource("provideParameters")
+ public void test_https_SniHandlerDisabled_wrong_host_pass(boolean mTLSEnabled,
+ boolean http2Enabled) throws Exception {
+ props.setProperty(RestConfig.SNI_CHECK_ENABLED_CONFIG, "false");
+ if (mTLSEnabled) {
+ props.setProperty(RestConfig.SSL_CLIENT_AUTHENTICATION_CONFIG,
+ RestConfig.SSL_CLIENT_AUTHENTICATION_REQUIRED);
+ }
+ props.setProperty(RestConfig.HTTP2_ENABLED_CONFIG, String.valueOf(http2Enabled));
+ startHttpServer("https");
+ startHttpClient("https");
+
+ ContentResponse response = httpClient.newRequest(server.getURI())
+ .path("/resource")
+ .accept(MediaType.TEXT_PLAIN)
+ // SNI is localhost but Host is anotherhost
+ .header(HttpHeader.HOST, KSQL_HOST)
+ .send();
+
+ // the request is successful because anotherhost is SAN in certificate
+ assertEquals(OK.getCode(), response.getStatus());
+ }
+
+ @ParameterizedTest
+ @MethodSource("provideParameters")
+ public void test_https_SniHandlerEnabled_wrong_host_421(boolean mTLSEnabled, boolean http2Enabled)
+ throws Exception {
+ props.setProperty(RestConfig.SNI_CHECK_ENABLED_CONFIG, "true");
+ if (mTLSEnabled) {
+ props.setProperty(RestConfig.SSL_CLIENT_AUTHENTICATION_CONFIG,
+ RestConfig.SSL_CLIENT_AUTHENTICATION_REQUIRED);
+ }
+ props.setProperty(RestConfig.HTTP2_ENABLED_CONFIG, String.valueOf(http2Enabled));
+ startHttpServer("https");
+ startHttpClient("https");
+
+ ContentResponse response = httpClient.newRequest(server.getURI())
+ .path("/resource")
+ .accept(MediaType.TEXT_PLAIN)
+ // SNI is localhost but Host is anotherhost
+ .header(HttpHeader.HOST, KSQL_HOST)
+ .send();
+
+ // 421 because SNI check is enabled
+ assertEquals(MISDIRECTED_REQUEST.getCode(), response.getStatus());
+ String responseContent = response.getContentAsString();
+ assertThat(responseContent, containsString(MISDIRECTED_REQUEST.getMessage()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("provideParameters")
+ public void test_https_SniHandlerEnabled_same_host_pass(boolean mTLSEnabled, boolean http2Enabled)
+ throws Exception {
+ props.setProperty(RestConfig.SNI_CHECK_ENABLED_CONFIG, "true");
+ if (mTLSEnabled) {
+ props.setProperty(RestConfig.SSL_CLIENT_AUTHENTICATION_CONFIG,
+ RestConfig.SSL_CLIENT_AUTHENTICATION_REQUIRED);
+ }
+ props.setProperty(RestConfig.HTTP2_ENABLED_CONFIG, String.valueOf(http2Enabled));
+ startHttpServer("https");
+ startHttpClient("https");
+
+ ContentResponse response = httpClient.newRequest(server.getURI())
+ .path("/resource")
+ .accept(MediaType.TEXT_PLAIN)
+ .send();
+
+ assertEquals(OK.getCode(), response.getStatus());
+
+ response = httpClient.newRequest(server.getURI())
+ .path("/resource")
+ .accept(MediaType.TEXT_PLAIN)
+ .header(HttpHeader.HOST, KAFKA_REST_HOST)
+ .send();
+
+ assertEquals(OK.getCode(), response.getStatus());
+ }
+
+ // generate mTLS enablement and http2 enablement parameters for tests
+ private static Stream provideParameters() {
+ return Stream.of(
+ Arguments.of(false, false),
+ Arguments.of(false, true),
+ Arguments.of(true, false),
+ Arguments.of(true, true)
+ );
+ }
+
+ private void startHttpClient(String scheme) throws Exception {
+ // allow setting Host header
+ System.setProperty("sun.net.http.allowRestrictedHeaders", "true");
+
+ if (scheme.equals("https")) {
+ // trust all self-signed certs.
+ SSLContextBuilder sslContextBuilder = SSLContexts.custom()
+ .loadTrustMaterial(new TrustSelfSignedStrategy());
+ // add the client keystore if it's configured.
+ sslContextBuilder.loadKeyMaterial(new File(clientKeystore.getAbsolutePath()),
+ TEST_SSL_PASSWORD.toCharArray(),
+ TEST_SSL_PASSWORD.toCharArray());
+ SSLContext sslContext = sslContextBuilder.build();
+
+ SslContextFactory.Client sslContextFactory = new SslContextFactory.Client();
+ // this forces non-standard domains (localhost) in SNI and X509,
+ // see https://github.com/eclipse/jetty.project/pull/6296
+ sslContextFactory.setSNIProvider(
+ SslContextFactory.Client.SniProvider.NON_DOMAIN_SNI_PROVIDER);
+ sslContextFactory.setSslContext(sslContext);
+
+ httpClient = new HttpClient(sslContextFactory);
+ } else {
+ httpClient = new HttpClient();
+ }
+
+ httpClient.start();
+ }
+
+ private void startHttpServer(String scheme) throws Exception {
+ String url = scheme + "://" + KAFKA_REST_HOST + ":" + getFreePort();
+ props.setProperty(RestConfig.LISTENERS_CONFIG, url);
+
+ if (scheme.equals("https")) {
+ File serverKeystore;
+ File trustStore;
+ try {
+ trustStore = File.createTempFile("SslTest-truststore", ".jks");
+ serverKeystore = File.createTempFile("SslTest-server-keystore", ".jks");
+ clientKeystore = File.createTempFile("SslTest-client-keystore", ".jks");
+ } catch (IOException ioe) {
+ throw new RuntimeException(
+ "Unable to create temporary files for truststores and keystores.");
+ }
+ Map certs = new HashMap<>();
+ createKeystoreWithCert(clientKeystore, ServiceType.CLIENT, certs);
+ createKeystoreWithCert(serverKeystore, ServiceType.SERVER, certs);
+ TestSslUtils.createTrustStore(trustStore.getAbsolutePath(), new Password(TEST_SSL_PASSWORD),
+ certs);
+
+ configServerKeystore(props, serverKeystore);
+ configServerTruststore(props, trustStore);
+ }
+ TestApp application = new TestApp(new TestRestConfig(props), "/");
+ server = application.createServer();
+
+ server.start();
+ }
+
+ private void configServerKeystore(Properties props, File serverKeystore) {
+ props.put(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG, serverKeystore.getAbsolutePath());
+ props.put(RestConfig.SSL_KEYSTORE_PASSWORD_CONFIG, TEST_SSL_PASSWORD);
+ props.put(RestConfig.SSL_KEY_PASSWORD_CONFIG, TEST_SSL_PASSWORD);
+ }
+
+ private void configServerTruststore(Properties props, File trustStore) {
+ props.put(RestConfig.SSL_TRUSTSTORE_LOCATION_CONFIG, trustStore.getAbsolutePath());
+ props.put(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG, TEST_SSL_PASSWORD);
+ }
+
+ private void createKeystoreWithCert(File file, ServiceType type,
+ Map certs)
+ throws Exception {
+ KeyPair keypair = TestSslUtils.generateKeyPair("RSA");
+ TestSslUtils.CertificateBuilder certificateBuilder = new TestSslUtils.CertificateBuilder(30,
+ "SHA1withRSA");
+
+ X509Certificate cCert = certificateBuilder
+ // create two SANs (Subject Alternative Name) in the certificate,
+ // imagine "localhost" is kafka rest, and "anotherhost" is ksql
+ .sanDnsNames(KAFKA_REST_HOST, KSQL_HOST)
+ .generate("CN=mymachine.local, O=A client", keypair);
+
+ String alias = type.toString().toLowerCase();
+ TestSslUtils.createKeyStore(file.getPath(), new Password(TEST_SSL_PASSWORD),
+ new Password(TEST_SSL_PASSWORD), alias, keypair.getPrivate(), cCert);
+ certs.put(alias, cCert);
+ }
+
+ private static class TestApp extends Application implements AutoCloseable {
+
+ TestApp(TestRestConfig config, String path) {
+ super(config, path);
+ }
+
+ @Override
+ public void setupResources(final Configurable> config, final TestRestConfig appConfig) {
+ config.register(RestResource.class);
+ }
+
+ @Override
+ public void close() throws Exception {
+ stop();
+ }
+ }
+
+ @Path("/")
+ public static class RestResource {
+
+ @GET
+ @Path("/resource")
+ public String get() {
+ return "Hello";
+ }
+ }
+
+ private enum ServiceType {
+ CLIENT,
+ SERVER
+ }
+}