zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Date Sun, 28 Oct 2018 07:41:38 GMT
Github user anmolnar commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/678#discussion_r228737596
  
    --- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
    @@ -221,15 +279,45 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException
{
             }
         }
     
    -    public static X509KeyManager createKeyManager(String keyStoreLocation, String keyStorePassword)
    +    /**
    +     * Creates a key manager by loading the key store from the given file of the given
type, optionally decrypting it
    +     * using the given password.
    +     * @param keyStoreLocation the location of the key store file.
    +     * @param keyStorePassword optional password to decrypt the key store. If empty,
assumes the key store is not
    +     *                         encrypted.
    +     * @param keyStoreType must be JKS, PEM, or null. If null, attempts to autodetect
the key store type from the file
    +     *                     extension (.jks / .pem).
    +     * @return the key manager.
    +     * @throws KeyManagerException if something goes wrong.
    +     */
    +    public static X509KeyManager createKeyManager(String keyStoreLocation, String keyStorePassword,
StoreFileType keyStoreType)
                 throws KeyManagerException {
             FileInputStream inputStream = null;
    +        if (keyStorePassword == null) {
    +            keyStorePassword = "";
    +        }
             try {
                 char[] keyStorePasswordChars = keyStorePassword.toCharArray();
                 File keyStoreFile = new File(keyStoreLocation);
    -            KeyStore ks = KeyStore.getInstance("JKS");
    -            inputStream = new FileInputStream(keyStoreFile);
    -            ks.load(inputStream, keyStorePasswordChars);
    +            if (keyStoreType == null) {
    +                keyStoreType = detectStoreFileTypeFromFileExtension(keyStoreFile);
    +            }
    +            KeyStore ks;
    +            switch (keyStoreType) {
    --- End diff --
    
    Using switch cases to employ different implementations for the same purpose is usually
a code smell to me. What do you think of implementing an abstract class or an interface to
gather all keystore reader implementations and create child classes for PEM and JKS?
    
    In which case you can,
    1. pass only the interface KeystoreLoader here,
    2. avoid PemReader being implemented with all static methods,
    3. easily mock the impl in unit tests


---

Mime
View raw message