You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by ivmaykov <gi...@git.apache.org> on 2018/10/25 02:20:54 UTC
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
GitHub user ivmaykov opened a pull request:
https://github.com/apache/zookeeper/pull/678
ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
Add support for loading key and trust stores from PEM files.
Unfortunately, this PR includes 2 JIRAs, because it was quite difficult
to untangle the two features as they were developed at the same time
originally:
- ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
- ZOOKEEPER-3175: Quorum TLS - test improvements
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3173
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/678.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #678
----
commit b8b687ae4dea912ef18ee2ee1ace406800f3fce7
Author: Ilya Maykov <il...@...>
Date: 2018-10-25T00:41:48Z
ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
ZOOKEEPER-3175: Quorum TLS - test improvements
Add support for loading key and trust stores from PEM files.
Also added test utils for testing X509-related code, because it
was very difficult to untangle them from the PEM support code.
----
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230421926
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -221,27 +229,47 @@ 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 keyStoreTypeProp 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,
+ String keyStoreTypeProp)
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);
+ KeyStoreFileType storeFileType =
--- End diff --
Will fix. This also made me realize that `JKSFileLoader` and `PEMFileLoader` are leaking file input streams. Will fix that as well.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230465652
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
--- End diff --
Also, loading private keys from arbitrary URLs goes against best security practices (unless that URL is for a local file, in which case there is no point in wrapping it in a URL). TLS private keys should never be transmitted across a network. The CA cert (i.e. Trust Store) could in theory be loaded from a remote URL though.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@eolivelli what does a "(non binding)" +1 mean? Can we merge this to upstream/master at this point?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
Merged to 3.5 and master branches. Thanks @ivmaykov !
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
Jenkins claims there is a findbugs failure, but there is no actual output of findbugs failures, and findbugs passes on my machine. ¯\_(ツ)_/¯
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
I don't understand why this PR started failing Jenkins builds all of a sudden. The next 2 PRs stacked on this one (#678 and #680) also fail, but the last one (#681) passes.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229202470
--- Diff: NOTICE.txt ---
@@ -3,3 +3,9 @@ Copyright 2009-2014 The Apache Software Foundation
This product includes software developed at
The Apache Software Foundation (http://www.apache.org/).
+
+This product includes software components originally
--- End diff --
Looks good.
Nit: Airlift with the uppercase A
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar it's certainly easier to keep track of the copied code if we don't modify it much. Moving the logic into PEMFileLoader and making it non-static would make it harder to trace the history.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2589/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2604/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230255350
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -254,23 +282,54 @@ public static X509KeyManager createKeyManager(String keyStoreLocation, String ke
}
}
- public static X509TrustManager createTrustManager(String trustStoreLocation, String trustStorePassword,
- boolean crlEnabled, boolean ocspEnabled,
- final boolean serverHostnameVerificationEnabled,
- final boolean clientHostnameVerificationEnabled)
+ /**
+ * Creates a trust manager by loading the trust store from the given file
+ * of the given type, optionally decrypting it using the given password.
+ * @param trustStoreLocation the location of the trust store file.
+ * @param trustStorePassword optional password to decrypt the trust store
+ * (only applies to JKS trust stores). If empty,
+ * assumes the trust store is not encrypted.
+ * @param trustStoreTypeProp must be JKS, PEM, or null. If null, attempts
+ * to autodetect the trust store type from the
+ * file extension (.jks / .pem).
+ * @param crlEnabled enable CRL (certificate revocation list) checks.
+ * @param ocspEnabled enable OCSP (online certificate status protocol)
+ * checks.
+ * @param serverHostnameVerificationEnabled if true, verify hostnames of
+ * remote servers that client
+ * sockets created by this
+ * X509Util connect to.
+ * @param clientHostnameVerificationEnabled if true, verify hostnames of
+ * remote clients that server
+ * sockets created by this
+ * X509Util accept connections
+ * from.
+ * @return the trust manager.
+ * @throws TrustManagerException if something goes wrong.
+ */
+ public static X509TrustManager createTrustManager(
+ String trustStoreLocation,
+ String trustStorePassword,
+ String trustStoreTypeProp,
+ boolean crlEnabled,
+ boolean ocspEnabled,
+ final boolean serverHostnameVerificationEnabled,
+ final boolean clientHostnameVerificationEnabled)
throws TrustManagerException {
FileInputStream inputStream = null;
+ if (trustStorePassword == null) {
+ trustStorePassword = "";
+ }
try {
- File trustStoreFile = new File(trustStoreLocation);
- KeyStore ts = KeyStore.getInstance("JKS");
- inputStream = new FileInputStream(trustStoreFile);
- if (trustStorePassword != null) {
- char[] trustStorePasswordChars = trustStorePassword.toCharArray();
- ts.load(inputStream, trustStorePasswordChars);
- } else {
- ts.load(inputStream, null);
- }
-
+ KeyStoreFileType storeFileType =
--- End diff --
IllegalArgumentException is possible here. I see it not handled
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229021572
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
For the NOTICE you can take a look to this example from Bookkeeper project
https://github.com/apache/bookkeeper/blob/master/bookkeeper-dist/src/main/resources/NOTICE-server.bin.txt
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230454818
--- 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 --
@anmolnar I believe this is done.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230468188
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
--- End diff --
Yaa.Thanks for explaining
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228805464
--- 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 --
Thanks @ivmaykov .
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230250524
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ this.keyStorePath = keyStorePath;
+ this.trustStorePath = trustStorePath;
+ this.keyStorePassword = keyStorePassword;
+ this.trustStorePassword = trustStorePassword;
+ }
+
+ /**
+ * Base class for builder pattern used by subclasses.
+ * @param <T> the subtype of FileKeyStoreLoader created by the Builder.
+ */
+ static abstract class Builder<T extends FileKeyStoreLoader> {
+ String keyStorePath;
+ String trustStorePath;
+ String keyStorePassword;
+ String trustStorePassword;
+
+ Builder() {}
+
+ Builder<T> setKeyStorePath(String keyStorePath) {
+ this.keyStorePath = Objects.requireNonNull(keyStorePath);
+ return this;
+ }
+
+ Builder<T> setTrustStorePath(String trustStorePath) {
+ this.trustStorePath = Objects.requireNonNull(trustStorePath);
+ return this;
+ }
+
+ Builder<T> setKeyStorePassword(String keyStorePassword) {
+ this.keyStorePassword = Objects.requireNonNull(keyStorePassword);
+ return this;
+ }
+
+ Builder<T> setTrustStorePassword(String trustStorePassword) {
+ this.trustStorePassword = Objects.requireNonNull(trustStorePassword);
+ return this;
+ }
+
+ abstract T build();
+ }
+
+ /**
+ * Returns a {@link FileKeyStoreLoader.Builder} that can build a loader
+ * which loads keys and certs from files of the given
+ * {@link KeyStoreFileType}.
+ *
+ * @param type the file type to load keys/certs from.
+ * @return a new Builder.
+ */
+ static Builder<? extends FileKeyStoreLoader> getBuilderForKeyStoreFileType(
--- End diff --
getBuilderForKeyStoreFileType is depending on loader implementation classes .This may lead to the subclasses of this also depend on other loader implementation classes. How about moving this to something like BuilderProvider?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2548/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229012101
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -167,47 +222,50 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
KeyManager[] keyManagers = null;
TrustManager[] trustManagers = null;
- String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty);
- String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty);
+ String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty, "");
+ String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty, "");
+ String keyStoreTypeProp = config.getProperty(sslKeystoreTypeProperty);
// There are legal states in some use cases for null KeyManager or TrustManager.
- // But if a user wanna specify one, location and password are required.
+ // But if a user wanna specify one, location is required. Password defaults to empty string if it is not
+ // specified by the user.
- if (keyStoreLocationProp == null && keyStorePasswordProp == null) {
+ if (keyStoreLocationProp.isEmpty()) {
LOG.warn(getSslKeystoreLocationProperty() + " not specified");
} else {
- if (keyStoreLocationProp == null) {
- throw new SSLContextException(getSslKeystoreLocationProperty() + " not specified");
- }
- if (keyStorePasswordProp == null) {
- throw new SSLContextException(getSslKeystorePasswdProperty() + " not specified");
- }
try {
+ StoreFileType keyStoreType = StoreFileType.fromPropertyValue(keyStoreTypeProp);
keyManagers = new KeyManager[]{
- createKeyManager(keyStoreLocationProp, keyStorePasswordProp)};
+ createKeyManager(keyStoreLocationProp, keyStorePasswordProp, keyStoreType)};
} catch (KeyManagerException keyManagerException) {
throw new SSLContextException("Failed to create KeyManager", keyManagerException);
+ } catch (IllegalArgumentException e) {
+ throw new SSLContextException("Bad value for " + sslKeystoreTypeProperty + ": " + keyStoreTypeProp, e);
}
}
- String trustStoreLocationProp = config.getProperty(sslTruststoreLocationProperty);
- String trustStorePasswordProp = config.getProperty(sslTruststorePasswdProperty);
+ String trustStoreLocationProp = config.getProperty(sslTruststoreLocationProperty, "");
--- End diff --
See above
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228806955
--- 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 --
@anmolnar actually I'll wait for the rest of your review so I can get through all the initial changes in one pass.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2525/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229102998
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -167,47 +222,50 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
KeyManager[] keyManagers = null;
TrustManager[] trustManagers = null;
- String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty);
- String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty);
+ String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty, "");
--- End diff --
Sorry, I misread this comment. You are talking about the location, not the store type. But it's the same thing - null location and empty location can be treated the same way, so by defaulting to empty string here we don't need to check for 2 different options below. The previous code actually didn't check for empty location at all.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228406071
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
We must update the NOTICE file
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar it works for me as far as I can tell, I think the contbuild is flaky? The contbuild on #681, which includes the same commit, passes.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228958680
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -167,47 +222,50 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
KeyManager[] keyManagers = null;
TrustManager[] trustManagers = null;
- String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty);
- String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty);
+ String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty, "");
+ String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty, "");
+ String keyStoreTypeProp = config.getProperty(sslKeystoreTypeProperty);
// There are legal states in some use cases for null KeyManager or TrustManager.
- // But if a user wanna specify one, location and password are required.
+ // But if a user wanna specify one, location is required. Password defaults to empty string if it is not
+ // specified by the user.
- if (keyStoreLocationProp == null && keyStorePasswordProp == null) {
+ if (keyStoreLocationProp.isEmpty()) {
LOG.warn(getSslKeystoreLocationProperty() + " not specified");
} else {
- if (keyStoreLocationProp == null) {
- throw new SSLContextException(getSslKeystoreLocationProperty() + " not specified");
- }
- if (keyStorePasswordProp == null) {
- throw new SSLContextException(getSslKeystorePasswdProperty() + " not specified");
- }
try {
+ StoreFileType keyStoreType = StoreFileType.fromPropertyValue(keyStoreTypeProp);
keyManagers = new KeyManager[]{
- createKeyManager(keyStoreLocationProp, keyStorePasswordProp)};
+ createKeyManager(keyStoreLocationProp, keyStorePasswordProp, keyStoreType)};
} catch (KeyManagerException keyManagerException) {
throw new SSLContextException("Failed to create KeyManager", keyManagerException);
+ } catch (IllegalArgumentException e) {
+ throw new SSLContextException("Bad value for " + sslKeystoreTypeProperty + ": " + keyStoreTypeProp, e);
}
}
- String trustStoreLocationProp = config.getProperty(sslTruststoreLocationProperty);
- String trustStorePasswordProp = config.getProperty(sslTruststorePasswdProperty);
+ String trustStoreLocationProp = config.getProperty(sslTruststoreLocationProperty, "");
--- End diff --
Same here. Previously it was checked against null value, why have you changed that?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2532/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/678
Maybe you should use docker.
As soon as possible I will copy the command line we are using in Jenkins job.
Maybe it is already available on the job log
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2521/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar the latest attempt does not have test failures, but claims a findbugs failure.
> [exec] -1 overall. GitHub Pull Request Build
> [exec]
> [exec]
> [exec] +1 @author. The patch does not contain any @author tags.
> [exec]
> [exec] +0 tests included. The patch appears to be a documentation patch that doesn't require tests.
> [exec]
> [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
> [exec]
> [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
> [exec]
> [exec] -1 findbugs. The patch appears to cause Findbugs (version 3.0.1) to fail.
> [exec]
> [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
> [exec]
> [exec] +1 core tests. The patch passed core unit tests.
> [exec]
> [exec] +1 contrib tests. The patch passed contrib unit tests.
Link to full output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2599/console
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2500/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228963670
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
You could be right, I'm not sure what's the implication of this. @hanm must be able to advise on this.
BouncyCastle library also having some PEM reader logic, but not sure how useful it is here.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230419175
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
--- End diff --
Should I rename the class to URLKeyStoreLoader then?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2559/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
The test failures don't look to be related to my changes. @anmolnar @hanm is there a way to trigger a Jenkins build re-run?
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229938699
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
notice file lgtm, thanks
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar is there a way to reproduce the exact same steps that jenkins runs on my macbook?
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/678
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2599/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230467749
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ this.keyStorePath = keyStorePath;
+ this.trustStorePath = trustStorePath;
+ this.keyStorePassword = keyStorePassword;
+ this.trustStorePassword = trustStorePassword;
+ }
+
+ /**
+ * Base class for builder pattern used by subclasses.
+ * @param <T> the subtype of FileKeyStoreLoader created by the Builder.
+ */
+ static abstract class Builder<T extends FileKeyStoreLoader> {
+ String keyStorePath;
+ String trustStorePath;
+ String keyStorePassword;
+ String trustStorePassword;
+
+ Builder() {}
+
+ Builder<T> setKeyStorePath(String keyStorePath) {
+ this.keyStorePath = Objects.requireNonNull(keyStorePath);
+ return this;
+ }
+
+ Builder<T> setTrustStorePath(String trustStorePath) {
+ this.trustStorePath = Objects.requireNonNull(trustStorePath);
+ return this;
+ }
+
+ Builder<T> setKeyStorePassword(String keyStorePassword) {
+ this.keyStorePassword = Objects.requireNonNull(keyStorePassword);
+ return this;
+ }
+
+ Builder<T> setTrustStorePassword(String trustStorePassword) {
+ this.trustStorePassword = Objects.requireNonNull(trustStorePassword);
+ return this;
+ }
+
+ abstract T build();
+ }
+
+ /**
+ * Returns a {@link FileKeyStoreLoader.Builder} that can build a loader
+ * which loads keys and certs from files of the given
+ * {@link KeyStoreFileType}.
+ *
+ * @param type the file type to load keys/certs from.
+ * @return a new Builder.
+ */
+ static Builder<? extends FileKeyStoreLoader> getBuilderForKeyStoreFileType(
--- End diff --
I see it. Thanks for making this change.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2544/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
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
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar thanks for merging this :) I've rebased #679, #680, and #681 on top of master. Let's get those in soon :)
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230455088
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ this.keyStorePath = keyStorePath;
+ this.trustStorePath = trustStorePath;
+ this.keyStorePassword = keyStorePassword;
+ this.trustStorePassword = trustStorePassword;
+ }
+
+ /**
+ * Base class for builder pattern used by subclasses.
+ * @param <T> the subtype of FileKeyStoreLoader created by the Builder.
+ */
+ static abstract class Builder<T extends FileKeyStoreLoader> {
+ String keyStorePath;
+ String trustStorePath;
+ String keyStorePassword;
+ String trustStorePassword;
+
+ Builder() {}
+
+ Builder<T> setKeyStorePath(String keyStorePath) {
+ this.keyStorePath = Objects.requireNonNull(keyStorePath);
+ return this;
+ }
+
+ Builder<T> setTrustStorePath(String trustStorePath) {
+ this.trustStorePath = Objects.requireNonNull(trustStorePath);
+ return this;
+ }
+
+ Builder<T> setKeyStorePassword(String keyStorePassword) {
+ this.keyStorePassword = Objects.requireNonNull(keyStorePassword);
+ return this;
+ }
+
+ Builder<T> setTrustStorePassword(String trustStorePassword) {
+ this.trustStorePassword = Objects.requireNonNull(trustStorePassword);
+ return this;
+ }
+
+ abstract T build();
+ }
+
+ /**
+ * Returns a {@link FileKeyStoreLoader.Builder} that can build a loader
+ * which loads keys and certs from files of the given
+ * {@link KeyStoreFileType}.
+ *
+ * @param type the file type to load keys/certs from.
+ * @return a new Builder.
+ */
+ static Builder<? extends FileKeyStoreLoader> getBuilderForKeyStoreFileType(
--- End diff --
@tumativ this is done, please see the latest version of the code
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230423126
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -254,23 +282,54 @@ public static X509KeyManager createKeyManager(String keyStoreLocation, String ke
}
}
- public static X509TrustManager createTrustManager(String trustStoreLocation, String trustStorePassword,
- boolean crlEnabled, boolean ocspEnabled,
- final boolean serverHostnameVerificationEnabled,
- final boolean clientHostnameVerificationEnabled)
+ /**
+ * Creates a trust manager by loading the trust store from the given file
+ * of the given type, optionally decrypting it using the given password.
+ * @param trustStoreLocation the location of the trust store file.
+ * @param trustStorePassword optional password to decrypt the trust store
+ * (only applies to JKS trust stores). If empty,
+ * assumes the trust store is not encrypted.
+ * @param trustStoreTypeProp must be JKS, PEM, or null. If null, attempts
+ * to autodetect the trust store type from the
+ * file extension (.jks / .pem).
+ * @param crlEnabled enable CRL (certificate revocation list) checks.
+ * @param ocspEnabled enable OCSP (online certificate status protocol)
+ * checks.
+ * @param serverHostnameVerificationEnabled if true, verify hostnames of
+ * remote servers that client
+ * sockets created by this
+ * X509Util connect to.
+ * @param clientHostnameVerificationEnabled if true, verify hostnames of
+ * remote clients that server
+ * sockets created by this
+ * X509Util accept connections
+ * from.
+ * @return the trust manager.
+ * @throws TrustManagerException if something goes wrong.
+ */
+ public static X509TrustManager createTrustManager(
+ String trustStoreLocation,
+ String trustStorePassword,
+ String trustStoreTypeProp,
+ boolean crlEnabled,
+ boolean ocspEnabled,
+ final boolean serverHostnameVerificationEnabled,
+ final boolean clientHostnameVerificationEnabled)
throws TrustManagerException {
FileInputStream inputStream = null;
+ if (trustStorePassword == null) {
+ trustStorePassword = "";
+ }
try {
- File trustStoreFile = new File(trustStoreLocation);
- KeyStore ts = KeyStore.getInstance("JKS");
- inputStream = new FileInputStream(trustStoreFile);
- if (trustStorePassword != null) {
- char[] trustStorePasswordChars = trustStorePassword.toCharArray();
- ts.load(inputStream, trustStorePasswordChars);
- } else {
- ts.load(inputStream, null);
- }
-
+ KeyStoreFileType storeFileType =
--- End diff --
Will fix.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228763521
--- 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 --
The fundamental problem with not following design patterns from day 0 is that people become reluctant to do things in the right way and eventually leads to classes like `X509Util.java`. Completely meaningless class which acts like a bin: people can add basically anything which fits in the "Util" naming (everything).
I don't feel splitting the code across multiple classes / files is a con: does the compiler or the IDE have some problem with some extra files? I feel it much more readable though: reading a 100-line class with single responsibility is a lot easier than reading 1000-line Util class with a mess in it.
"it would make mocking easier and adding more store types later would be potentially simplified, plus we could test the loaders in isolation" - That's my point basically.
"We could also land it like this in the interests of getting a working implementation out, and clean it up in a later PR." - That's the source of all evil. :)
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230421726
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
--- End diff --
Actually, the PemReader class doesn't currently support `URL`, only `File`. So I'd prefer to only support file paths here for now. We can modify it later to support loading from URLs, in a separate PR / JIRA.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229013179
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
BouncyCastle is currently a test-only dependency (used for creating test certificates), so we can't use it in production code. It's a large library and I'd rather not take it on as a full dependency if the only thing we need from it is the PEM reader. I think it's fine to copy the code (license allows it and the author of airlift recommended it to me), will wait for guidance on NOTICE file update from @eolivelli.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229524910
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
@hanm I've updated the NOTICE.txt file in the latest version of the code, please let me know if that looks good. I think it doesn't hurt to keep the comments in PemReader.java - will make it easier to backport fixes from airlift's PemReader in the future.
RE: httpcomponents. Yes, we did copy some httpcomponents code. I can put up a separate PR that adds a NOTICE.txt entry for that.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@eolivelli should the copied code be put into the zookeeper-contrib subproject? Or can it still live in zookeeper-server? I could also use help with wording the message in the NOTICE file. Thanks.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/678
For Apache httpcomponents there is nothing to write, the NOTICE already covers it, as it is an Apache Project
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
@ivmaykov It's in:
`zookeeper-server/src/test/resources/test-github-pr.sh`
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@eolivelli capitalized "Airlift" in NOTICE.txt
@anmolnar use `FileNameUtils.getExtension()` for file type detection
everyone: fixed some copy-paste bugs in PEMFileLoaderTest and JKSFileLoaderTest, and added more test cases.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229012448
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -360,4 +476,26 @@ private void configureSSLServerSocket(SSLServerSocket sslServerSocket) {
LOG.debug("Using Java8-optimized cipher suites for Java version {}", javaVersion);
return DEFAULT_CIPHERS_JAVA8;
}
+
+ /**
+ * Detects the type of KeyStore / TrustStore file from the file extension. If the file name ends with
+ * ".jks", returns <code>StoreFileType.JKS</code>. If the file name ends with ".pem", returns
+ * <code>StoreFileType.PEM</code>. Otherwise, throws an IOException.
+ * @param filename the filename of the key store or trust store file.
+ * @return a StoreFileType.
+ * @throws IOException if the filename does not end with ".jks" or ".pem".
+ */
+ public static StoreFileType detectStoreFileTypeFromFileExtension(File filename) throws IOException {
--- End diff --
How about I make it package-private and add a unit test?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
Now it's green. I'll merge it.
Previously the findbugs subprocess has been killed for some reason.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229011448
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -167,47 +222,50 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
KeyManager[] keyManagers = null;
TrustManager[] trustManagers = null;
- String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty);
- String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty);
+ String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty, "");
--- End diff --
Since empty string is not a valid keystore type, I treat null value and empty string values equivalently. Doing it this way simplifies the check below (only need to check for empty string instead of checking both null and empty).
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228957739
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -167,47 +222,50 @@ public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
KeyManager[] keyManagers = null;
TrustManager[] trustManagers = null;
- String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty);
- String keyStorePasswordProp = config.getProperty(sslKeystorePasswdProperty);
+ String keyStoreLocationProp = config.getProperty(sslKeystoreLocationProperty, "");
--- End diff --
Why do you need an empty string default value instead of checking for null?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2584/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228738342
--- 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 --
@anmolnar
IMHO in this specific case the switch can be acceptable.
With a fully generic solution maybe we should also add a generic way to detect the type from the filename or file contents (so that we can support new file types in the future without touching this code).
Anyway any future proof implementation is always welcome
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230251259
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/JKSFileLoader.java ---
@@ -0,0 +1,67 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+
+/**
+ * Implementation of {@link FileKeyStoreLoader} that loads from JKS files.
+ */
+class JKSFileLoader extends FileKeyStoreLoader {
+ private static final char[] EMPTY_CHAR_ARRAY = new char[0];
+
+ private JKSFileLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ super(keyStorePath, trustStorePath, keyStorePassword, trustStorePassword);
+ }
+
+ @Override
+ public KeyStore loadKeyStore() throws IOException, GeneralSecurityException {
+ KeyStore ks = KeyStore.getInstance("JKS");
--- End diff --
Can we declare global final variable for "JKS" and refer in code?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
What's wrong with this build?
@ivmaykov Does it work for you locally?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2555/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/678
'Non binding' means that I am not a 'committer' so I can't merge the patch by myself.
Btw the other guys which are reviewing this patch are committers so I think this patch will be merged very soon.
Thanks
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229523540
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java ---
@@ -0,0 +1,224 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.util;
+
+import javax.crypto.Cipher;
+import javax.crypto.EncryptedPrivateKeyInfo;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.security.auth.x500.X500Principal;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.GeneralSecurityException;
+import java.security.KeyFactory;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static java.util.Base64.getMimeDecoder;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
+import static javax.crypto.Cipher.DECRYPT_MODE;
+
+/**
+ * Note: this class is copied from io.airlift.security.pem.PemReader (see
+ * https://github.com/airlift/airlift/blob/master/security/src/main/java/io/airlift/security/pem/PemReader.java) with
--- End diff --
i agree with @eolivelli , it should be enough to declare airlift in our notice file as a dependency.
our notice file is here:
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/resources/NOTICE.txt
I vaguely remember we also copied some code from org.apache.httpcomponents for host verification to mitigate the perf regression of directly referencing the library as a dependency. Might worth to double check if we need patch notice file for that copy as well.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refactored common code shared by a bunch of tests that use `X509TestContext` into a new base class, `BaseX509ParameterizedTestCase`.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228760044
--- 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 --
That factory logic is already implemented in `detectStoreFileTypeFromFileExtension()` and which (I believe) should go into a static factory method or factory class.
Another potential weakness of such switch-case implementations is what you mentioned in terms of adding new container types: one has to implement the new type in each of every switch case related to this type. In this case it's only 2 which I think already quite error-prone, but there could be others later.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
@ivmaykov What was the problem with using `FileNameUtils`?
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
I should mention that this code has been internally reviewed at Facebook, has been landed on our internal fork, and has been running in production for weeks.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar added KeyStoreLoader classes
@eolivelli updated NOTICE file and PemReader.java
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228764349
--- 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 --
Fair enough, I'll do some refactoring next week
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228762467
--- 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 --
@anmolnar @eolivelli There are pros and cons to either approach. We only do the `switch (StoreFileType)` in two places, both of which are in the same file, which I feel is pretty well localized. If/when someone adds support for another key store type later (say, PKCS12), as long as they add decent tests in the same commit, I don't think there is much risk of forgetting to update one of the two switch statements. So, creating interfaces and implementing them in several classes kind of feels like over-engineering and will make it hard to see all the logic at once since it will be spread out across 7 files (assuming interface + 2 implementations for each of KeyStoreLoader / TrustStoreLoader) instead of 1 file.
But it would make mocking easier and adding more store types later would be potentially simplified, plus we could test the loaders in isolation, so I see pros and cons to both approaches. We could also land it like this in the interests of getting a working implementation out, and clean it up in a later PR. You guys let me know what you would prefer.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2514/
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by enixon <gi...@git.apache.org>.
Github user enixon commented on the issue:
https://github.com/apache/zookeeper/pull/678
Yeah, there aren't findbugs reports in the artifacts as I would expect https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2599/artifact/patchprocess/ , it's unclear what went wrong there. Maybe retrigger the build process ('retest this please')?
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230418985
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/JKSFileLoader.java ---
@@ -0,0 +1,67 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+
+/**
+ * Implementation of {@link FileKeyStoreLoader} that loads from JKS files.
+ */
+class JKSFileLoader extends FileKeyStoreLoader {
+ private static final char[] EMPTY_CHAR_ARRAY = new char[0];
+
+ private JKSFileLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ super(keyStorePath, trustStorePath, keyStorePassword, trustStorePassword);
+ }
+
+ @Override
+ public KeyStore loadKeyStore() throws IOException, GeneralSecurityException {
+ KeyStore ks = KeyStore.getInstance("JKS");
--- End diff --
Will do
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230418497
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ this.keyStorePath = keyStorePath;
+ this.trustStorePath = trustStorePath;
+ this.keyStorePassword = keyStorePassword;
+ this.trustStorePassword = trustStorePassword;
+ }
+
+ /**
+ * Base class for builder pattern used by subclasses.
+ * @param <T> the subtype of FileKeyStoreLoader created by the Builder.
+ */
+ static abstract class Builder<T extends FileKeyStoreLoader> {
+ String keyStorePath;
+ String trustStorePath;
+ String keyStorePassword;
+ String trustStorePassword;
+
+ Builder() {}
+
+ Builder<T> setKeyStorePath(String keyStorePath) {
+ this.keyStorePath = Objects.requireNonNull(keyStorePath);
+ return this;
+ }
+
+ Builder<T> setTrustStorePath(String trustStorePath) {
+ this.trustStorePath = Objects.requireNonNull(trustStorePath);
+ return this;
+ }
+
+ Builder<T> setKeyStorePassword(String keyStorePassword) {
+ this.keyStorePassword = Objects.requireNonNull(keyStorePassword);
+ return this;
+ }
+
+ Builder<T> setTrustStorePassword(String trustStorePassword) {
+ this.trustStorePassword = Objects.requireNonNull(trustStorePassword);
+ return this;
+ }
+
+ abstract T build();
+ }
+
+ /**
+ * Returns a {@link FileKeyStoreLoader.Builder} that can build a loader
+ * which loads keys and certs from files of the given
+ * {@link KeyStoreFileType}.
+ *
+ * @param type the file type to load keys/certs from.
+ * @return a new Builder.
+ */
+ static Builder<? extends FileKeyStoreLoader> getBuilderForKeyStoreFileType(
--- End diff --
Okay :( This is the part of java I dislike. The good old "AbstractBaseFooFactoryFactory" pattern :/
Relevant: https://github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2563/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228961395
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -360,4 +476,26 @@ private void configureSSLServerSocket(SSLServerSocket sslServerSocket) {
LOG.debug("Using Java8-optimized cipher suites for Java version {}", javaVersion);
return DEFAULT_CIPHERS_JAVA8;
}
+
+ /**
+ * Detects the type of KeyStore / TrustStore file from the file extension. If the file name ends with
+ * ".jks", returns <code>StoreFileType.JKS</code>. If the file name ends with ".pem", returns
+ * <code>StoreFileType.PEM</code>. Otherwise, throws an IOException.
+ * @param filename the filename of the key store or trust store file.
+ * @return a StoreFileType.
+ * @throws IOException if the filename does not end with ".jks" or ".pem".
+ */
+ public static StoreFileType detectStoreFileTypeFromFileExtension(File filename) throws IOException {
--- End diff --
nit: this can be private
nit: Apache Commons IO library has `FileNameUtils.getExtensions(String filename)` doing pretty much the same
This file type detection logic is good. Additionally, given that you're refactoring this to Factory pattern anyway, you could also do probing with the concrete implementations if file type cannot be detected from extension.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@eolivelli got it, thanks! I will use that terminology from now on :)
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
It's not findbugs, core and contrib tests are failing, but I cannot see why:
```
[exec] -1 core tests. The patch failed core unit tests.
[exec] -1 contrib tests. The patch failed contrib unit tests.
```
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/678
retest this please
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/678
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2536/
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230252932
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileKeyStoreLoader.java ---
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import java.util.Objects;
+
+/**
+ * Base class for instances of {@link KeyStoreLoader} which load the key/trust
+ * stores from files on a filesystem.
+ */
+abstract class FileKeyStoreLoader implements KeyStoreLoader {
+ final String keyStorePath;
+ final String trustStorePath;
+ final String keyStorePassword;
+ final String trustStorePassword;
+
+ FileKeyStoreLoader(String keyStorePath,
--- End diff --
How about changing the keyStorePath and trustStorePathto URL?So that it can be leveraged for other use cases?Let builder take file path and convert it to URL.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/678
retest this please
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r230255232
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -221,27 +229,47 @@ 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 keyStoreTypeProp 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,
+ String keyStoreTypeProp)
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);
+ KeyStoreFileType storeFileType =
--- End diff --
IllegalArgumentException is possible here. I see it is not handled.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
Revert `FileNameUtils` change as it seems to be breaking contbuild
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/678
@ivmaykov
The code can stay where you put it.
About the NOTICE file I will try to help past the weekend
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/678
@anmolnar on my machine it worked, but on Jenkins it could not resolve the import - not sure why. It's not a big deal I think, we don't need to use it.
---
[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/678
>> is there a way to trigger a Jenkins build re-run?
@ivmaykov Check out https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute, search 'Jenkins Pre-commit Check'. The simplest approach is to close and then reopen the pull request.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by ivmaykov <gi...@git.apache.org>.
Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r229011934
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -79,12 +82,56 @@
"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"
};
+ /**
+ * This enum represents the file type of a KeyStore or TrustStore. Currently, JKS (java keystore) and PEM types
+ * are supported.
+ */
+ public enum StoreFileType {
+ JKS(".jks"), PEM(".pem");
+
+ private final String defaultFileExtension;
+
+ StoreFileType(String defaultFileExtension) {
+ this.defaultFileExtension = defaultFileExtension;
+ }
+
+ /**
+ * The property string that specifies that a key store or trust store should use this store file type.
+ */
+ public String getPropertyValue() {
+ return this.name();
+ }
+
+ /**
+ * The file extension that is associated with this file type.
+ */
+ public String getDefaultFileExtension() {
+ return defaultFileExtension;
+ }
+
+ /**
+ * Converts a property value to a StoreFileType enum. If the property value is not set or is empty, returns
+ * null.
+ * @param prop the property value.
+ * @return the StoreFileType.
+ * @throws IllegalArgumentException if the property value is not "JKS", "PEM", or empty/null.
+ */
+ public static StoreFileType fromPropertyValue(String prop) {
+ if (prop == null || prop.length() == 0) {
+ return null;
+ }
+ return StoreFileType.valueOf(prop.toUpperCase());
+ }
+ }
+
private String sslProtocolProperty = getConfigPrefix() + "protocol";
private String cipherSuitesProperty = getConfigPrefix() + "ciphersuites";
private String sslKeystoreLocationProperty = getConfigPrefix() + "keyStore.location";
private String sslKeystorePasswdProperty = getConfigPrefix() + "keyStore.password";
+ private String sslKeystoreTypeProperty = getConfigPrefix() + "keyStore.type";
--- End diff --
I think it makes sense, if someone puts their JKS key in a file that they name "foobar.key" or something non-standard, we can still support it if they explicitly set the store type option. Most people will probably use the default extensions and leave this option unset.
---
[GitHub] zookeeper pull request #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/678#discussion_r228958388
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -79,12 +82,56 @@
"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"
};
+ /**
+ * This enum represents the file type of a KeyStore or TrustStore. Currently, JKS (java keystore) and PEM types
+ * are supported.
+ */
+ public enum StoreFileType {
+ JKS(".jks"), PEM(".pem");
+
+ private final String defaultFileExtension;
+
+ StoreFileType(String defaultFileExtension) {
+ this.defaultFileExtension = defaultFileExtension;
+ }
+
+ /**
+ * The property string that specifies that a key store or trust store should use this store file type.
+ */
+ public String getPropertyValue() {
+ return this.name();
+ }
+
+ /**
+ * The file extension that is associated with this file type.
+ */
+ public String getDefaultFileExtension() {
+ return defaultFileExtension;
+ }
+
+ /**
+ * Converts a property value to a StoreFileType enum. If the property value is not set or is empty, returns
+ * null.
+ * @param prop the property value.
+ * @return the StoreFileType.
+ * @throws IllegalArgumentException if the property value is not "JKS", "PEM", or empty/null.
+ */
+ public static StoreFileType fromPropertyValue(String prop) {
+ if (prop == null || prop.length() == 0) {
+ return null;
+ }
+ return StoreFileType.valueOf(prop.toUpperCase());
+ }
+ }
+
private String sslProtocolProperty = getConfigPrefix() + "protocol";
private String cipherSuitesProperty = getConfigPrefix() + "ciphersuites";
private String sslKeystoreLocationProperty = getConfigPrefix() + "keyStore.location";
private String sslKeystorePasswdProperty = getConfigPrefix() + "keyStore.password";
+ private String sslKeystoreTypeProperty = getConfigPrefix() + "keyStore.type";
--- End diff --
Do we need the ability to override the keystore type that we otherwise detect from the file extension?
---