This is an automated email from the ASF dual-hosted git repository.
andor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 3702a4545 ZOOKEEPER-3860: Avoid reverse DNS lookup for hostname
verification when hostnames are provided in the connection url
3702a4545 is described below
commit 3702a45459b4946d3c45d29c6f201077b66bf2ea
Author: Andor Molnar <[email protected]>
AuthorDate: Wed Jun 14 14:58:35 2023 +0200
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 <[email protected]>
Reviewers: [email protected]
Closes #2005 from anmolnar/ZOOKEEPER-3860
---
.../java/org/apache/zookeeper/common/NetUtils.java | 15 ++++----
.../apache/zookeeper/common/ZKTrustManager.java | 40 ++++++++++++++++------
.../org/apache/zookeeper/common/NetUtilsTest.java | 6 ++--
3 files changed, 39 insertions(+), 22 deletions(-)
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java
index be8cb9a63..a02499a68 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/NetUtils.java
+++ b/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());
}
}
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
index 2573e3fce..cbadd1e0a 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
+++
b/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 {
}
}
}
-
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/common/NetUtilsTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/common/NetUtilsTest.java
index a7006bf70..fbe492214 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/common/NetUtilsTest.java
+++
b/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
