zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From geek101 <...@git.apache.org>
Subject [GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...
Date Thu, 06 Apr 2017 07:01:43 GMT
Github user geek101 commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/184#discussion_r110089020
  
    --- 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 --
    
    For Quorum server connection I am not entirely sure if getInetAddres().getHostName() is
a good idea when on server side this will force a reverse DNS lookup. When customer has only
provided a ip address as config perhaps using hostname is not correct. And if customer has
provided a hostname performing reverse dns lookup is not necessary and can be argued as not
safe. Since the trust anchor is the user provided config and not the DNS service. Let me know
what you think.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message