Skip to content

Commit 33e5d72

Browse files
anmolnaranmolnar
authored
ZOOKEEPER-4986: Disable reverse DNS lookup in TLS client and server (branch-3.8)
Author: anmolnar Closes #2349 from anmolnar/ZOOKEEPER-4986_38
1 parent 2148f5f commit 33e5d72

File tree

13 files changed

+185
-40
lines changed

13 files changed

+185
-40
lines changed

‎zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,6 +1714,16 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
17141714
Disabling it only recommended for testing purposes.
17151715
Default: true
17161716

1717+
* *ssl.allowReverseDnsLookup* and *ssl.quorum.allowReverseDnsLookup* :
1718+
(Java system properties: **zookeeper.ssl.allowReverseDnsLookup** and **zookeeper.ssl.quorum.allowReverseDnsLookup**)
1719+
**New in 3.8.6:**
1720+
Allow reverse DNS lookup in both server- and client hostname verifications if the hostname verification is enabled in
1721+
`ZKTrustManager`. Supported in both quorum and client TLS protocols. Not supported in FIPS mode. Reverse DNS lookups are
1722+
expensive and unnecessary in most cases. Make sure that certificates are created with all required Subject Alternative
1723+
Names (SAN) for successful identity verification. It's recommended to add SAN:IP entries for identity verification
1724+
of client certificates.
1725+
Default: false (for Client connections), true (for Quorum connections)
1726+
17171727
* *ssl.crl* and *ssl.quorum.crl* :
17181728
(Java system properties: **zookeeper.ssl.crl** and **zookeeper.ssl.quorum.crl**)
17191729
**New in 3.5.5:**

‎zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ protected boolean shouldVerifyClientHostname() {
3232
return false;
3333
}
3434

35+
@Override
36+
protected boolean shouldAllowReverseDnsLookup() {
37+
return false;
38+
}
39+
3540
public String getSslAuthProviderProperty() {
3641
return sslAuthProviderProperty;
3742
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/common/QuorumX509Util.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,8 @@ protected boolean shouldVerifyClientHostname() {
3030
return true;
3131
}
3232

33+
@Override
34+
protected boolean shouldAllowReverseDnsLookup() {
35+
return true;
36+
}
3337
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ public io.netty.handler.ssl.ClientAuth toNettyClientAuth() {
160160
private final String sslTruststoreTypeProperty = getConfigPrefix() + "trustStore.type";
161161
private final String sslContextSupplierClassProperty = getConfigPrefix() + "context.supplier.class";
162162
private final String sslHostnameVerificationEnabledProperty = getConfigPrefix() + "hostnameVerification";
163+
private final String sslAllowReverseDnsLookupProperty = getConfigPrefix() + "allowReverseDnsLookup";
163164
private final String sslCrlEnabledProperty = getConfigPrefix() + "crl";
164165
private final String sslOcspEnabledProperty = getConfigPrefix() + "ocsp";
165166
private final String sslClientAuthProperty = getConfigPrefix() + "clientAuth";
@@ -178,6 +179,8 @@ public X509Util() {
178179

179180
protected abstract boolean shouldVerifyClientHostname();
180181

182+
protected abstract boolean shouldAllowReverseDnsLookup();
183+
181184
public String getSslProtocolProperty() {
182185
return sslProtocolProperty;
183186
}
@@ -234,6 +237,10 @@ public String getSslHostnameVerificationEnabledProperty() {
234237
return sslHostnameVerificationEnabledProperty;
235238
}
236239

240+
public String getSslAllowReverseDnsLookupProperty() {
241+
return sslAllowReverseDnsLookupProperty;
242+
}
243+
237244
public String getSslCrlEnabledProperty() {
238245
return sslCrlEnabledProperty;
239246
}
@@ -272,6 +279,10 @@ public boolean isClientHostnameVerificationEnabled(ZKConfig config) {
272279
return isServerHostnameVerificationEnabled(config) && shouldVerifyClientHostname();
273280
}
274281

282+
public boolean allowReverseDnsLookup(ZKConfig config) {
283+
return config.getBoolean(this.getSslAllowReverseDnsLookupProperty(), shouldAllowReverseDnsLookup());
284+
}
285+
275286
public SSLContext getDefaultSSLContext() throws X509Exception.SSLContextException {
276287
return getDefaultSSLContextAndOptions().getSSLContext();
277288
}
@@ -389,6 +400,7 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config
389400
boolean sslOcspEnabled = config.getBoolean(this.sslOcspEnabledProperty);
390401
boolean sslServerHostnameVerificationEnabled = isServerHostnameVerificationEnabled(config);
391402
boolean sslClientHostnameVerificationEnabled = isClientHostnameVerificationEnabled(config);
403+
boolean allowReverseDnsLookup = allowReverseDnsLookup(config);
392404
boolean fipsMode = getFipsMode(config);
393405

394406
if (trustStoreLocationProp.isEmpty()) {
@@ -398,7 +410,7 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config
398410
trustManagers = new TrustManager[]{
399411
createTrustManager(trustStoreLocationProp, trustStorePasswordProp, trustStoreTypeProp, sslCrlEnabled,
400412
sslOcspEnabled, sslServerHostnameVerificationEnabled, sslClientHostnameVerificationEnabled,
401-
fipsMode)};
413+
allowReverseDnsLookup, fipsMode)};
402414
} catch (TrustManagerException trustManagerException) {
403415
throw new SSLContextException("Failed to create TrustManager", trustManagerException);
404416
} catch (IllegalArgumentException e) {
@@ -534,6 +546,7 @@ public static X509TrustManager createTrustManager(
534546
boolean ocspEnabled,
535547
final boolean serverHostnameVerificationEnabled,
536548
final boolean clientHostnameVerificationEnabled,
549+
final boolean allowReverseDnsLookup,
537550
final boolean fipsMode) throws TrustManagerException {
538551
if (trustStorePassword == null) {
539552
trustStorePassword = "";
@@ -568,7 +581,7 @@ public static X509TrustManager createTrustManager(
568581
LOG.debug("FIPS mode is OFF: creating ZKTrustManager");
569582
}
570583
return new ZKTrustManager((X509ExtendedTrustManager) tm, serverHostnameVerificationEnabled,
571-
clientHostnameVerificationEnabled);
584+
clientHostnameVerificationEnabled, allowReverseDnsLookup);
572585
}
573586
}
574587
throw new TrustManagerException("Couldn't find X509TrustManager");

‎zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ private void putSSLProperties(X509Util x509Util) {
127127
properties.put(x509Util.getSslTruststoreTypeProperty(), System.getProperty(x509Util.getSslTruststoreTypeProperty()));
128128
properties.put(x509Util.getSslContextSupplierClassProperty(), System.getProperty(x509Util.getSslContextSupplierClassProperty()));
129129
properties.put(x509Util.getSslHostnameVerificationEnabledProperty(), System.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
130+
properties.put(x509Util.getSslAllowReverseDnsLookupProperty(), System.getProperty(x509Util.getSslAllowReverseDnsLookupProperty()));
130131
properties.put(x509Util.getSslCrlEnabledProperty(), System.getProperty(x509Util.getSslCrlEnabledProperty()));
131132
properties.put(x509Util.getSslOcspEnabledProperty(), System.getProperty(x509Util.getSslOcspEnabledProperty()));
132133
properties.put(x509Util.getSslClientAuthProperty(), System.getProperty(x509Util.getSslClientAuthProperty()));

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

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
4242
private final X509ExtendedTrustManager x509ExtendedTrustManager;
4343
private final boolean serverHostnameVerificationEnabled;
4444
private final boolean clientHostnameVerificationEnabled;
45+
private final boolean allowReverseDnsLookup;
4546

4647
private final ZKHostnameVerifier hostnameVerifier;
4748

@@ -57,22 +58,26 @@ public class ZKTrustManager extends X509ExtendedTrustManager {
5758
ZKTrustManager(
5859
X509ExtendedTrustManager x509ExtendedTrustManager,
5960
boolean serverHostnameVerificationEnabled,
60-
boolean clientHostnameVerificationEnabled) {
61+
boolean clientHostnameVerificationEnabled,
62+
boolean allowReverseDnsLookup) {
6163
this(x509ExtendedTrustManager,
6264
serverHostnameVerificationEnabled,
6365
clientHostnameVerificationEnabled,
64-
new ZKHostnameVerifier());
66+
new ZKHostnameVerifier(),
67+
allowReverseDnsLookup);
6568
}
6669

6770
ZKTrustManager(
6871
X509ExtendedTrustManager x509ExtendedTrustManager,
6972
boolean serverHostnameVerificationEnabled,
7073
boolean clientHostnameVerificationEnabled,
71-
ZKHostnameVerifier hostnameVerifier) {
74+
ZKHostnameVerifier hostnameVerifier,
75+
boolean allowReverseDnsLookup) {
7276
this.x509ExtendedTrustManager = x509ExtendedTrustManager;
7377
this.serverHostnameVerificationEnabled = serverHostnameVerificationEnabled;
7478
this.clientHostnameVerificationEnabled = clientHostnameVerificationEnabled;
7579
this.hostnameVerifier = hostnameVerifier;
80+
this.allowReverseDnsLookup = allowReverseDnsLookup;
7681
}
7782

7883
@Override
@@ -166,31 +171,46 @@ public void checkServerTrusted(X509Certificate[] chain, String authType) throws
166171
* @param certificate Peer's certificate
167172
* @throws CertificateException Thrown if the provided certificate doesn't match the peer hostname.
168173
*/
169-
private void performHostVerification(
170-
InetAddress inetAddress,
171-
X509Certificate certificate
172-
) throws CertificateException {
174+
private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
175+
throws CertificateException {
173176
String hostAddress = "";
174177
String hostName = "";
175178
try {
176179
hostAddress = inetAddress.getHostAddress();
177-
if (LOG.isDebugEnabled()) {
178-
LOG.debug("Trying to verify host address first: {}", hostAddress);
179-
}
180180
hostnameVerifier.verify(hostAddress, certificate);
181181
} catch (SSLException addressVerificationException) {
182+
// If we fail with hostAddress, we should try the hostname.
183+
// The inetAddress may have been created with a hostname, in which case getHostName() will
184+
// return quickly below. If not, a reverse lookup will happen, which can be expensive.
185+
// We provide the option to skip the reverse lookup if preferring to fail fast.
186+
187+
// Handle logging here to aid debugging. The easiest way to check for an existing
188+
// hostname is through toString, see javadoc.
189+
String inetAddressString = inetAddress.toString();
190+
if (!inetAddressString.startsWith("/")) {
191+
LOG.debug(
192+
"Failed to verify host address: {}, but inetAddress {} has a hostname, trying that",
193+
hostAddress, inetAddressString, addressVerificationException);
194+
} else if (allowReverseDnsLookup) {
195+
LOG.debug(
196+
"Failed to verify host address: {}, attempting to verify host name with reverse dns",
197+
hostAddress, addressVerificationException);
198+
} else {
199+
LOG.debug("Failed to verify host address: {}, but reverse dns lookup is disabled",
200+
hostAddress, addressVerificationException);
201+
throw new CertificateException(
202+
"Failed to verify host address, and reverse lookup is disabled",
203+
addressVerificationException);
204+
}
205+
182206
try {
183207
hostName = inetAddress.getHostName();
184-
if (LOG.isDebugEnabled()) {
185-
LOG.debug(
186-
"Failed to verify host address: {}, trying to verify host name: {}",
187-
hostAddress, hostName);
188-
}
189208
hostnameVerifier.verify(hostName, certificate);
190209
} catch (SSLException hostnameVerificationException) {
191210
LOG.error("Failed to verify host address: {}", hostAddress, addressVerificationException);
192211
LOG.error("Failed to verify hostname: {}", hostName, hostnameVerificationException);
193-
throw new CertificateException("Failed to verify both host address and host name", hostnameVerificationException);
212+
throw new CertificateException("Failed to verify both host address and host name",
213+
hostnameVerificationException);
194214
}
195215
}
196216
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ public X509AuthenticationProvider() throws X509Exception {
8080
boolean crlEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslCrlEnabledProperty()));
8181
boolean ocspEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslOcspEnabledProperty()));
8282
boolean hostnameVerificationEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
83+
boolean allowReverseDnsLookup = Boolean.parseBoolean(config.getProperty(x509Util.getSslAllowReverseDnsLookupProperty()));
8384

8485
X509KeyManager km = null;
8586
X509TrustManager tm = null;
@@ -112,6 +113,7 @@ public X509AuthenticationProvider() throws X509Exception {
112113
ocspEnabled,
113114
hostnameVerificationEnabled,
114115
false,
116+
allowReverseDnsLookup,
115117
fipsMode);
116118
} catch (TrustManagerException e) {
117119
LOG.error("Failed to create trust manager", e);

‎zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,7 @@ public void testLoadPEMTrustStore(
361361
false,
362362
true,
363363
true,
364+
false,
364365
false);
365366
}
366367

@@ -382,6 +383,7 @@ public void testLoadPEMTrustStoreNullPassword(
382383
false,
383384
true,
384385
true,
386+
false,
385387
false);
386388

387389
}
@@ -401,6 +403,7 @@ public void testLoadPEMTrustStoreAutodetectStoreFileType(
401403
false,
402404
true,
403405
true,
406+
false,
404407
false);
405408
}
406409

@@ -476,6 +479,7 @@ public void testLoadJKSTrustStore(
476479
true,
477480
true,
478481
true,
482+
false,
479483
false);
480484
}
481485

@@ -497,6 +501,7 @@ public void testLoadJKSTrustStoreNullPassword(
497501
false,
498502
true,
499503
true,
504+
false,
500505
false);
501506
}
502507

@@ -515,6 +520,7 @@ public void testLoadJKSTrustStoreAutodetectStoreFileType(
515520
true,
516521
true,
517522
true,
523+
false,
518524
false);
519525
}
520526

@@ -534,6 +540,7 @@ public void testLoadJKSTrustStoreWithWrongPassword(
534540
true,
535541
true,
536542
true,
543+
false,
537544
false);
538545
});
539546
}
@@ -609,6 +616,7 @@ public void testLoadPKCS12TrustStore(
609616
true,
610617
true,
611618
true,
619+
false,
612620
false);
613621
}
614622

@@ -630,6 +638,7 @@ public void testLoadPKCS12TrustStoreNullPassword(
630638
false,
631639
true,
632640
true,
641+
false,
633642
false);
634643
}
635644

@@ -648,6 +657,7 @@ public void testLoadPKCS12TrustStoreAutodetectStoreFileType(
648657
true,
649658
true,
650659
true,
660+
false,
651661
false);
652662
}
653663

@@ -667,6 +677,7 @@ public void testLoadPKCS12TrustStoreWithWrongPassword(
667677
true,
668678
true,
669679
true,
680+
false,
670681
false);
671682
});
672683
}

0 commit comments

Comments
 (0)