ZOOKEEPER-3860: Avoid reverse DNS lookup for hostname verification when hostnames are provided in the connection url

Instead of altering the behaviour of `ZKTrustManager`, I'll add the following improvements:

1. `NetUtils.formatInetAddr()` should prefer using the hostname of resolved addresses when converting InetSocketAddress to String. Previously it only picked the hostname if address was missing, e.g. the hostname was unresolved. This change has the advantage of sending the hostname in the InitialMessage instead of the IP address, which will help avoiding unnecessary reverse DNS lookups in the election protocol when TLS is enabled.

2. Add extra debug logs to `ZKTrustManager` and remove logging the exception stack trace when IP verification failed. Logging of stack traces makes sense to me in error log entries only. Many times we hit into this when user turns on debug logging, seeing a big stack trace and believes it's an error in ZooKeeper.

Target branches: master, branch-3.8

Author: Andor Molnar <andor@cloudera.com>

Reviewers: eolivelli@apache.org

Closes #2005 from anmolnar/ZOOKEEPER-3860

zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java

@@ -27,17 +27,18 @@ import java.net.InetSocketAddress;
  */
 public class NetUtils {
 
+    /**
+     * Prefer using the hostname for formatting, but without requesting reverse DNS lookup.
+     * Fall back to IP address if hostname is unavailable and use [] brackets for IPv6 literal.
+     */
     public static String formatInetAddr(InetSocketAddress addr) {
+        String hostString = addr.getHostString();
         InetAddress ia = addr.getAddress();
 
-        if (ia == null) {
-            return String.format("%s:%s", addr.getHostString(), addr.getPort());
-        }
-
-        if (ia instanceof Inet6Address) {
-            return String.format("[%s]:%s", ia.getHostAddress(), addr.getPort());
+        if (ia instanceof Inet6Address && hostString.contains(":")) {
+            return String.format("[%s]:%s", hostString, addr.getPort());
         } else {
-            return String.format("%s:%s", ia.getHostAddress(), addr.getPort());
+            return String.format("%s:%s", hostString, addr.getPort());
         }
     }
 

zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java

@@ -39,11 +39,11 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
 
     private static final Logger LOG = LoggerFactory.getLogger(ZKTrustManager.class);
 
-    private X509ExtendedTrustManager x509ExtendedTrustManager;
-    private boolean serverHostnameVerificationEnabled;
-    private boolean clientHostnameVerificationEnabled;
+    private final X509ExtendedTrustManager x509ExtendedTrustManager;
+    private final boolean serverHostnameVerificationEnabled;
+    private final boolean clientHostnameVerificationEnabled;
 
-    private ZKHostnameVerifier hostnameVerifier;
+    private final ZKHostnameVerifier hostnameVerifier;
 
     /**
      * Instantiate a new ZKTrustManager.
@@ -87,6 +87,9 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
         Socket socket) throws CertificateException {
         x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
         if (clientHostnameVerificationEnabled) {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Check client trusted socket.getInetAddress(): {}, {}", socket.getInetAddress(), socket);
+            }
             performHostVerification(socket.getInetAddress(), chain[0]);
         }
     }
@@ -98,6 +101,9 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
         Socket socket) throws CertificateException {
         x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
         if (serverHostnameVerificationEnabled) {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Check server trusted socket.getInetAddress(): {}, {}", socket.getInetAddress(), socket);
+            }
             performHostVerification(socket.getInetAddress(), chain[0]);
         }
     }
@@ -110,6 +116,9 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
         x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
         if (clientHostnameVerificationEnabled) {
             try {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Check client trusted engine.getPeerHost(): {}, {}", engine.getPeerHost(), engine);
+                }
                 performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
             } catch (UnknownHostException e) {
                 throw new CertificateException("Failed to verify host", e);
@@ -126,6 +135,9 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
         x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
         if (serverHostnameVerificationEnabled) {
             try {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Check server trusted engine.getPeerHost(): {}, {}", engine.getPeerHost(), engine);
+                }
                 performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
             } catch (UnknownHostException e) {
                 throw new CertificateException("Failed to verify host", e);
@@ -144,9 +156,12 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
     }
 
     /**
-     * Compares peer's hostname with the one stored in the provided client certificate. Performs verification
+     * Compares peer's hostname with the one stored in the provided certificate. Performs verification
      * with the help of provided HostnameVerifier.
      *
+     * Attempts to verify the IP address first, if it fails, check the hostname. Performs reverse DNS lookup
+     * if hostname is not available. (Mostly the case in client verifications.)
+     *
      * @param inetAddress Peer's inet address.
      * @param certificate Peer's certificate
      * @throws CertificateException Thrown if the provided certificate doesn't match the peer hostname.
@@ -154,19 +169,23 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
     private void performHostVerification(
         InetAddress inetAddress,
         X509Certificate certificate
-                                        ) throws CertificateException {
+    ) throws CertificateException {
         String hostAddress = "";
         String hostName = "";
         try {
             hostAddress = inetAddress.getHostAddress();
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Trying to verify host address first: {}", hostAddress);
+            }
             hostnameVerifier.verify(hostAddress, certificate);
         } catch (SSLException addressVerificationException) {
             try {
-                LOG.debug(
-                    "Failed to verify host address: {} attempting to verify host name with reverse dns lookup",
-                    hostAddress,
-                    addressVerificationException);
                 hostName = inetAddress.getHostName();
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug(
+                        "Failed to verify host address: {}, trying to verify host name: {}",
+                        hostAddress, hostName);
+                }
                 hostnameVerifier.verify(hostName, certificate);
             } catch (SSLException hostnameVerificationException) {
                 LOG.error("Failed to verify host address: {}", hostAddress, addressVerificationException);
@@ -175,5 +194,4 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
             }
         }
     }
-
 }

zookeeper-server/src/test/java/org/apache/zookeeper/common/NetUtilsTest.java

@@ -18,7 +18,6 @@
 
 package org.apache.zookeeper.common;
 
-import static org.hamcrest.CoreMatchers.anyOf;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -40,7 +39,7 @@ public class NetUtilsTest extends ZKTestCase {
     @Test
     public void testFormatInetAddrGoodIpv4() {
         InetSocketAddress isa = new InetSocketAddress(v4addr, port);
-        assertEquals("127.0.0.1:1234", NetUtils.formatInetAddr(isa));
+        assertEquals(v4local, NetUtils.formatInetAddr(isa));
     }
 
     @Test
@@ -60,8 +59,7 @@ public class NetUtilsTest extends ZKTestCase {
     @Test
     public void testFormatInetAddrGoodHostname() {
         InetSocketAddress isa = new InetSocketAddress("localhost", 1234);
-
-        assertThat(NetUtils.formatInetAddr(isa), anyOf(equalTo(v4local), equalTo(v6local)));
+        assertThat(NetUtils.formatInetAddr(isa), equalTo("localhost:1234"));
     }
 
     @Test