Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/184#discussion_r110263282 --- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java --- @@ -171,21 +226,100 @@ public static X509KeyManager createKeyManager(String keyStoreLocation, String ke } } - public static X509TrustManager createTrustManager(String trustStoreLocation, String trustStorePassword) + public static X509TrustManager createTrustManager(String trustStoreLocation, String trustStorePassword, + boolean crlEnabled, boolean ocspEnabled, + final boolean hostnameVerificationEnabled, + final boolean shouldVerifyClientHostname) throws TrustManagerException { FileInputStream inputStream = null; try { - char[] trustStorePasswordChars = trustStorePassword.toCharArray(); File trustStoreFile = new File(trustStoreLocation); KeyStore ts = KeyStore.getInstance("JKS"); inputStream = new FileInputStream(trustStoreFile); - ts.load(inputStream, trustStorePasswordChars); - TrustManagerFactory tmf = TrustManagerFactory.getInstance("SunX509"); - tmf.init(ts); + if (trustStorePassword != null) { + char[] trustStorePasswordChars = trustStorePassword.toCharArray(); + ts.load(inputStream, trustStorePasswordChars); + } else { + ts.load(inputStream, null); + } - for (TrustManager tm : tmf.getTrustManagers()) { - if (tm instanceof X509TrustManager) { - return (X509TrustManager) tm; + PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector()); + if (crlEnabled || ocspEnabled) { + pbParams.setRevocationEnabled(true); + System.setProperty("com.sun.net.ssl.checkRevocation", "true"); + System.setProperty("com.sun.security.enableCRLDP", "true"); + if (ocspEnabled) { + Security.setProperty("ocsp.enable", "true"); + } + + } else { + pbParams.setRevocationEnabled(false); + } + + TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(new CertPathTrustManagerParameters(pbParams)); + + for (final TrustManager tm : tmf.getTrustManagers()) { + if (tm instanceof X509ExtendedTrustManager) { + return new X509ExtendedTrustManager() { + X509ExtendedTrustManager x509ExtendedTrustManager = (X509ExtendedTrustManager) tm; + DefaultHostnameVerifier hostnameVerifier = new DefaultHostnameVerifier(); + + @Override + public X509Certificate[] getAcceptedIssuers() { + return x509ExtendedTrustManager.getAcceptedIssuers(); + } + + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException { + if (hostnameVerificationEnabled && shouldVerifyClientHostname) { + performHostnameVerification(socket.getInetAddress().getHostName(), chain[0]); --- End diff -- I agree that we should not do reverse DNS lookups when we do not need to. I changed the host verification logic to only perform a reverse dns lookup when host verification fails using the ip address. I think it is still fair to try the reverse dns lookup at that point as the value given in the altname of the certificate is not necessarily tied to the zookeeper configuration. Let me know if you think this is reasonable.
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---