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.
---
|