[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS

[GitBox <gi...@apache.org>]

bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974216103


##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification

Review Comment:
   > This is not the default behavior of java's TrustManager implementation?
   
   There is some basic support for hostname verification in the default TrustManager. You enable it by calling `sslParameters.setEndpointIdentificationAlgorithm("HTTPS")`. It only supports HTTPS and LDAP algorithms, which enforces some specific rules on wildcard matching and other things. 
   
   The tricky thing with that is Netty does not expose the SSLParameters to us, so we can't pass that in. We could potentially get around this by creating our own wrapper HBaseSslContext which wraps netty's SslContext and overloads `newEngine`, etc methods. This would allow us to set the aboe method onto the SSLParameters passed into the created engine.... but it's a wrapper either way, and this one seems more simple and to the point. This also gives us more control over the verification itself (i.e. DNS/IP instead of HTTPS/LDAP)
   
   
   
   
   And the HTTPS/LDAP requirement might be restrictive. We could get arou
   It's not enabled by default and can only be enabled via SSLParameters. Netty doesn't expose this on the usual SslContextBuilder, we'd need to  The other problem is, it can only be enabled 
   No, and this is verified via the new tests I added -- when verify.hostname = false (on client or server), if default TrustManager had this default behavior, we'd still expect the VERIFIABLE_CERT_WITH_BAD_HOST test case to throw an exception or otherwise fail the handshake. But it only throws the exp



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org