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