HBASE-28777 mTLS client hostname verification doesn't work with OptionalSslHandler (#6149)

Signed-off-by: Balazs Meszaros <[email protected]>

Author: anmolnar

hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java

@@ -92,8 +92,13 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket
   public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
     throws CertificateException {
     x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
-    if (hostnameVerificationEnabled) {
+    if (hostnameVerificationEnabled && engine != null) {
       try {
+        if (engine.getPeerHost() == null) {
+          LOG.warn(
+            "Cannot perform client hostname verification, because peer information is not available");
+          return;
+        }
         performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
       } catch (UnknownHostException e) {
         throw new CertificateException("Failed to verify host", e);

hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/tls/TestHBaseTrustManager.java

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase.io.crypto.tls;
 
 import static org.junit.Assert.assertThrows;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -36,6 +37,7 @@
 import java.util.Date;
 import java.util.List;
 import java.util.Random;
+import javax.net.ssl.SSLEngine;
 import javax.net.ssl.X509ExtendedTrustManager;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.MiscTests;
@@ -86,6 +88,8 @@ public class TestHBaseTrustManager {
   private InetAddress mockInetAddressWithHostname;
   private Socket mockSocketWithoutHostname;
   private Socket mockSocketWithHostname;
+  private SSLEngine mockSSLEngineWithoutPeerhost;
+  private SSLEngine mockSSLEngineWithPeerhost;
 
   @BeforeClass
   public static void createKeyPair() throws Exception {
@@ -126,6 +130,12 @@ public void setup() throws Exception {
     mockSocketWithHostname = mock(Socket.class);
     when(mockSocketWithHostname.getInetAddress())
       .thenAnswer((Answer<?>) invocationOnMock -> mockInetAddressWithHostname);
+
+    mockSSLEngineWithoutPeerhost = mock(SSLEngine.class);
+    doReturn(null).when(mockSSLEngineWithoutPeerhost).getPeerHost();
+
+    mockSSLEngineWithPeerhost = mock(SSLEngine.class);
+    doReturn(IP_ADDRESS).when(mockSSLEngineWithPeerhost).getPeerHost();
   }
 
   @SuppressWarnings("JavaUtilDate")
@@ -352,4 +362,35 @@ public void testClientTrustedWithHostnameVerificationEnabledWithHostnameNoRevers
       mockSocketWithHostname);
   }
 
+  @Test
+  public void testClientTrustedSslEngineWithPeerHostReverseLookup() throws Exception {
+    HBaseTrustManager trustManager =
+      new HBaseTrustManager(mockX509ExtendedTrustManager, true, true);
+    X509Certificate[] certificateChain = createSelfSignedCertificateChain(null, HOSTNAME);
+    trustManager.checkClientTrusted(certificateChain, null, mockSSLEngineWithPeerhost);
+  }
+
+  @Test(expected = CertificateException.class)
+  public void testClientTrustedSslEngineWithPeerHostNoReverseLookup() throws Exception {
+    HBaseTrustManager trustManager =
+      new HBaseTrustManager(mockX509ExtendedTrustManager, true, false);
+    X509Certificate[] certificateChain = createSelfSignedCertificateChain(null, HOSTNAME);
+    trustManager.checkClientTrusted(certificateChain, null, mockSSLEngineWithPeerhost);
+  }
+
+  @Test
+  public void testClientTrustedSslEngineWithoutPeerHost() throws Exception {
+    HBaseTrustManager trustManager =
+      new HBaseTrustManager(mockX509ExtendedTrustManager, true, false);
+    X509Certificate[] certificateChain = createSelfSignedCertificateChain(null, HOSTNAME);
+    trustManager.checkClientTrusted(certificateChain, null, mockSSLEngineWithoutPeerhost);
+  }
+
+  @Test
+  public void testClientTrustedSslEngineNotAvailable() throws Exception {
+    HBaseTrustManager trustManager =
+      new HBaseTrustManager(mockX509ExtendedTrustManager, true, false);
+    X509Certificate[] certificateChain = createSelfSignedCertificateChain(null, HOSTNAME);
+    trustManager.checkClientTrusted(certificateChain, null, (SSLEngine) null);
+  }
 }

hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/tls/X509TestContext.java

@@ -461,12 +461,13 @@ public X509TestContext cloneWithNewKeystoreCert(X509Certificate cert) {
   }
 
   public void regenerateStores(X509KeyType keyStoreKeyType, X509KeyType trustStoreKeyType,
-    KeyStoreFileType keyStoreFileType, KeyStoreFileType trustStoreFileType)
+    KeyStoreFileType keyStoreFileType, KeyStoreFileType trustStoreFileType,
+    String... subjectAltNames)
     throws GeneralSecurityException, IOException, OperatorCreationException {
 
     trustStoreKeyPair = X509TestHelpers.generateKeyPair(trustStoreKeyType);
     keyStoreKeyPair = X509TestHelpers.generateKeyPair(keyStoreKeyType);
-    createCertificates();
+    createCertificates(subjectAltNames);
 
     switch (keyStoreFileType) {
       case JKS:
@@ -499,7 +500,7 @@ public void regenerateStores(X509KeyType keyStoreKeyType, X509KeyType trustStore
     }
   }
 
-  private void createCertificates()
+  private void createCertificates(String... subjectAltNames)
     throws GeneralSecurityException, IOException, OperatorCreationException {
     X500NameBuilder caNameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
     caNameBuilder.addRDN(BCStyle.CN,
@@ -510,7 +511,7 @@ private void createCertificates()
     X500NameBuilder nameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
     nameBuilder.addRDN(BCStyle.CN,
       MethodHandles.lookup().lookupClass().getCanonicalName() + " Zookeeper Test");
-    keyStoreCertificate = newCert(nameBuilder.build());
+    keyStoreCertificate = newCert(nameBuilder.build(), subjectAltNames);
   }
 
   /**

hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java

@@ -21,7 +21,6 @@
 import static org.apache.hadoop.hbase.io.crypto.tls.X509Util.HBASE_SERVER_NETTY_TLS_ENABLED;
 import static org.apache.hadoop.hbase.io.crypto.tls.X509Util.HBASE_SERVER_NETTY_TLS_SUPPORTPLAINTEXT;
 import static org.apache.hadoop.hbase.io.crypto.tls.X509Util.HBASE_SERVER_NETTY_TLS_WRAP_SIZE;
-import static org.apache.hadoop.hbase.io.crypto.tls.X509Util.TLS_CONFIG_REVERSE_DNS_LOOKUP_ENABLED;
 
 import java.io.IOException;
 import java.io.InterruptedIOException;
@@ -386,30 +385,33 @@ private void initSSL(ChannelPipeline p, NettyServerRpcConnection conn, boolean s
     throws X509Exception, IOException {
     SslContext nettySslContext = getSslContext();
 
+    /*
+     * our HostnameVerifier gets the host name from SSLEngine, so we have to construct the engine
+     * properly by passing the remote address
+     */
+
     if (supportPlaintext) {
-      p.addLast("ssl", new OptionalSslHandler(nettySslContext));
+      SocketAddress remoteAddress = p.channel().remoteAddress();
+      OptionalSslHandler optionalSslHandler;
+
+      if (remoteAddress instanceof InetSocketAddress) {
+        InetSocketAddress remoteInetAddress = (InetSocketAddress) remoteAddress;
+        optionalSslHandler = new OptionalSslHandlerWithHostPort(nettySslContext,
+          remoteInetAddress.getHostString(), remoteInetAddress.getPort());
+      } else {
+        optionalSslHandler = new OptionalSslHandler(nettySslContext);
+      }
+
+      p.addLast("ssl", optionalSslHandler);
       LOG.debug("Dual mode SSL handler added for channel: {}", p.channel());
     } else {
       SocketAddress remoteAddress = p.channel().remoteAddress();
       SslHandler sslHandler;
 
       if (remoteAddress instanceof InetSocketAddress) {
         InetSocketAddress remoteInetAddress = (InetSocketAddress) remoteAddress;
-        String host;
-
-        if (conf.getBoolean(TLS_CONFIG_REVERSE_DNS_LOOKUP_ENABLED, true)) {
-          host = remoteInetAddress.getHostName();
-        } else {
-          host = remoteInetAddress.getHostString();
-        }
-
-        int port = remoteInetAddress.getPort();
-
-        /*
-         * our HostnameVerifier gets the host name from SSLEngine, so we have to construct the
-         * engine properly by passing the remote address
-         */
-        sslHandler = nettySslContext.newHandler(p.channel().alloc(), host, port);
+        sslHandler = nettySslContext.newHandler(p.channel().alloc(),
+          remoteInetAddress.getHostString(), remoteInetAddress.getPort());
       } else {
         sslHandler = nettySslContext.newHandler(p.channel().alloc());
       }

hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/OptionalSslHandlerWithHostPort.java

@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF 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
+ *
+ *     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 org.apache.hadoop.hbase.ipc;
+
+import org.apache.hbase.thirdparty.io.netty.channel.ChannelHandlerContext;
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.OptionalSslHandler;
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.SslContext;
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.SslHandler;
+
+class OptionalSslHandlerWithHostPort extends OptionalSslHandler {
+
+  private final String host;
+  private final int port;
+
+  public OptionalSslHandlerWithHostPort(SslContext sslContext, String host, int port) {
+    super(sslContext);
+    this.host = host;
+    this.port = port;
+  }
+
+  @Override
+  protected SslHandler newSslHandler(ChannelHandlerContext context, SslContext sslContext) {
+    return sslContext.newHandler(context.alloc(), host, port);
+  }
+}