[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext URL: https://github.com/apache/zookeeper/pull/728#discussion_r249220918 ## File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ## @@ -17,10 +17,28 @@ */ package org.apache.zookeeper.common; +import org.apache.zookeeper.common.X509Exception.KeyManagerException; Review comment: nit: revert import reorder? Or fix your import organizer to do java.* -> javax.* -> everything else This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r249221188
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -403,6 +407,32 @@ public void
testGetSslHandshakeDetectionTimeoutMillisProperty() {
}
}
+@Test(expected = X509Exception.SSLContextException.class)
+public void testCreateSSLContext_invalidCustomSSLContextClass() throws
Exception {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
String.class.getCanonicalName());
+clientX509Util.createSSLContext(zkConfig);
+}
+
+@Test
+public void testCreateSSLContext_validNullCustomSSLContextClass() throws
X509Exception.SSLContextException {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
NullSslContextSupplier.class.getName());
+final SSLContext sslContext =
clientX509Util.createSSLContext(zkConfig);
+assertNull(sslContext);
Review comment:
nit: rest of the file doesn't use static import and calls
`Assert.assert*()`, maybe follow precedent?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext URL: https://github.com/apache/zookeeper/pull/728#discussion_r249221054 ## File path: zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java ## @@ -17,11 +17,30 @@ */ package org.apache.zookeeper.common; +import org.apache.zookeeper.PortAssignment; Review comment: nit: revert import reorder? Or fix your import organizer to do java.* -> javax.* -> everything else This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248851355
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -238,18 +239,19 @@ public int getSslHandshakeTimeoutMillis() {
return result;
}
+@SuppressWarnings("unchecked")
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
-if (config.getProperty(sslClientContextProperty) != null) {
-LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
-String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+if (config.getProperty(sslContextSupplierClassProperty) != null) {
+LOG.debug("Loading SSLContext supplier from property '" +
sslContextSupplierClassProperty + "'");
Review comment:
Also for readability, consider using `{}` formatting to build the log
message:
`LOG.debug("Loading SSLContext supplier from property '{}'},
sslContextSupplierClassProperty);`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248849244
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -238,18 +239,19 @@ public int getSslHandshakeTimeoutMillis() {
return result;
}
+@SuppressWarnings("unchecked")
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
-if (config.getProperty(sslClientContextProperty) != null) {
-LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
-String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+if (config.getProperty(sslContextSupplierClassProperty) != null) {
+LOG.debug("Loading SSLContext supplier from property '" +
sslContextSupplierClassProperty + "'");
+String supplierContextClassName =
config.getProperty(sslContextSupplierClassProperty);
Review comment:
super nit: get the property before the first `if` in this method and store
in a local variable, so you don't call `config.getProperty()` multiple times?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248847815
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -238,18 +239,19 @@ public int getSslHandshakeTimeoutMillis() {
return result;
}
+@SuppressWarnings("unchecked")
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
-if (config.getProperty(sslClientContextProperty) != null) {
-LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
-String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+if (config.getProperty(sslContextSupplierClassProperty) != null) {
+LOG.debug("Loading SSLContext supplier from property '" +
sslContextSupplierClassProperty + "'");
Review comment:
nit: enclose this in a `if (LOG.isDebugEnabled()) { ... }`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext URL: https://github.com/apache/zookeeper/pull/728#discussion_r248848163 ## File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java ## @@ -18,6 +18,12 @@ package org.apache.zookeeper.common; +import org.apache.zookeeper.Environment; +import org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException; +import org.apache.zookeeper.server.util.VerifyingFileFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; Review comment: nit: revert this import reorder This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248850022
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -406,26 +407,30 @@ public void
testGetSslHandshakeDetectionTimeoutMillisProperty() {
}
}
+@Test(expected = X509Exception.SSLContextException.class)
+public void testCreateSSLContext_invalidCustomSSLContextClass() throws
Exception {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
String.class.getCanonicalName());
+clientX509Util.createSSLContext(zkConfig);
+}
+
@Test
-public void testCreateSSLContext_invalidCustomSSLContextClass() {
+public void testCreateSSLContext_validNullCustomSSLContextClass() throws
X509Exception.SSLContextException {
ZKConfig zkConfig = new ZKConfig();
ClientX509Util clientX509Util = new ClientX509Util();
-zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
String.class.getCanonicalName());
-try {
-clientX509Util.createSSLContext(zkConfig);
-fail("SSLContextException expected.");
-} catch (X509Exception.SSLContextException e) {
-
assertTrue(e.getMessage().contains(clientX509Util.getSslClientContextProperty()));
-}
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
X509UtilTest.class.getCanonicalName() + "$" +
NullSslContextSupplier.class.getSimpleName());
Review comment:
I think `NullSslContextSupplier.class.getName()` will return the full class
name for the inner class so you don't need to manually concatenate.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248848936
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -513,4 +518,27 @@ private void setCustomCipherSuites() {
x509Util.close(); // remember to close old instance before replacing it
x509Util = new ClientX509Util();
}
+
+public static class NullSslContextSupplier implements Supplier
{
+
+@Override
+public SSLContext get() {
+return null;
+}
+
+}
+
+public static class SslContextSupplier implements Supplier {
+
+@Override
+public SSLContext get() {
+try {
+return SSLContext.getDefault();
+} catch (NoSuchAlgorithmException e) {
+return null;
Review comment:
nit: `throw new RuntimeException(e);` so if this ever happens (unlikely),
the error will be more obvious in the test
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248848580
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -406,26 +407,30 @@ public void
testGetSslHandshakeDetectionTimeoutMillisProperty() {
}
}
+@Test(expected = X509Exception.SSLContextException.class)
+public void testCreateSSLContext_invalidCustomSSLContextClass() throws
Exception {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
String.class.getCanonicalName());
+clientX509Util.createSSLContext(zkConfig);
+}
+
@Test
-public void testCreateSSLContext_invalidCustomSSLContextClass() {
+public void testCreateSSLContext_validNullCustomSSLContextClass() throws
X509Exception.SSLContextException {
ZKConfig zkConfig = new ZKConfig();
ClientX509Util clientX509Util = new ClientX509Util();
-zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
String.class.getCanonicalName());
-try {
-clientX509Util.createSSLContext(zkConfig);
-fail("SSLContextException expected.");
-} catch (X509Exception.SSLContextException e) {
-
assertTrue(e.getMessage().contains(clientX509Util.getSslClientContextProperty()));
-}
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
X509UtilTest.class.getCanonicalName() + "$" +
NullSslContextSupplier.class.getSimpleName());
+final SSLContext sslContext =
clientX509Util.createSSLContext(zkConfig);
+assertNull(sslContext);
}
@Test
public void testCreateSSLContext_validCustomSSLContextClass() throws
X509Exception.SSLContextException {
ZKConfig zkConfig = new ZKConfig();
ClientX509Util clientX509Util = new ClientX509Util();
-zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
ZKTestClientSSLContext.class.getCanonicalName());
+
zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(),
X509UtilTest.class.getCanonicalName() + "$" +
SslContextSupplier.class.getSimpleName());
final SSLContext sslContext =
clientX509Util.createSSLContext(zkConfig);
-assertNull(sslContext);
+assertNotNull(sslContext);
Review comment:
nit: I think this can be `assertEquals(SSLContext.getDefault(), sslContext)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248804741
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -220,6 +224,24 @@ public int getSslHandshakeTimeoutMillis() {
}
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
+if (config.getProperty(sslClientContextProperty) != null) {
+LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
+String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+try {
+Class sslContextClass =
Class.forName(sslClientContextClass);
+ZKClientSSLContext sslContext = (ZKClientSSLContext)
sslContextClass.getConstructor().newInstance();
Review comment:
do you still get the warning if you do an explicit `instanceof` check first,
and throw an exception if the check fails?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248781573
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTestClientSSLContext.java
##
@@ -0,0 +1,29 @@
+/**
+ * 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 javax.net.ssl.SSLContext;
+
+public class ZKTestClientSSLContext implements ZKClientSSLContext {
Review comment:
Another nit then - can you make the class package-private? Principle of
least privilege and all that (Not sure if a package-private class can be
instantiated using reflection though).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248781133
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTestClientSSLContext.java
##
@@ -0,0 +1,29 @@
+/**
+ * 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 javax.net.ssl.SSLContext;
+
+public class ZKTestClientSSLContext implements ZKClientSSLContext {
Review comment:
Ok, if instantiating an inner class by reflection is awkward, don't worry
about it and leave it as a standalone class.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248780258
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -220,6 +224,24 @@ public int getSslHandshakeTimeoutMillis() {
}
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
+if (config.getProperty(sslClientContextProperty) != null) {
+LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
+String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+try {
+Class sslContextClass =
Class.forName(sslClientContextClass);
+ZKClientSSLContext sslContext = (ZKClientSSLContext)
sslContextClass.getConstructor().newInstance();
Review comment:
Functional interfaces allow lambdas and method references but I don't think
they are restricted to it. But anyway, I don't feel too strongly about it. I
don't think `Client` should be in the class name (since this works for both
`QuorumX509Util` and `ClientX509Util`) so if you stick with your own class, I
would call the interface something like `SSLContextSupplier`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248512821
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKClientSSLContext.java
##
@@ -0,0 +1,35 @@
+/**
+ * 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 javax.net.ssl.SSLContext;
+
+/**
+ * An interface for providing a custom {@link SSLContext} object to {@link
X509Util} using {@link X509Util#getSslClientContextProperty()}
+ */
+public interface ZKClientSSLContext {
Review comment:
I don't think we need a new interface for this. Can just use a
`java.util.function.Supplier`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248513615
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -389,6 +393,27 @@ public void
testGetSslHandshakeDetectionTimeoutMillisProperty() {
}
}
+@Test
+public void testCreateSSLContext_invalidCustomSSLContextClass() {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
String.class.getCanonicalName());
+try {
+clientX509Util.createSSLContext(zkConfig);
+fail("SSLContextException expected.");
Review comment:
nit: can remove the try-catch and modify the annotation on the method to be
`@Test(expected = X509Exception.SSLContextException.class)`. Though this
approach does not check the message, I think it's probably ok - testing for
specific error messages is fragile anyway. We should either not care about the
message, or we should create a specific sublcass of SSLContextException for
this case and check for that exception type.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext URL: https://github.com/apache/zookeeper/pull/728#discussion_r248512738 ## File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ## @@ -90,6 +89,7 @@ private String sslTruststoreLocationProperty = getConfigPrefix() + "trustStore.location"; private String sslTruststorePasswdProperty = getConfigPrefix() + "trustStore.password"; private String sslTruststoreTypeProperty = getConfigPrefix() + "trustStore.type"; +private String sslClientContextProperty = getConfigPrefix() + "client.context"; Review comment: nits: - not sure why "client" is in the name - let's call the property something like "context.supplier.class"? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248513820
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
##
@@ -389,6 +393,27 @@ public void
testGetSslHandshakeDetectionTimeoutMillisProperty() {
}
}
+@Test
+public void testCreateSSLContext_invalidCustomSSLContextClass() {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
String.class.getCanonicalName());
+try {
+clientX509Util.createSSLContext(zkConfig);
+fail("SSLContextException expected.");
+} catch (X509Exception.SSLContextException e) {
+
assertTrue(e.getMessage().contains(clientX509Util.getSslClientContextProperty()));
+}
+}
+@Test
+public void testCreateSSLContext_validCustomSSLContextClass() throws
X509Exception.SSLContextException {
+ZKConfig zkConfig = new ZKConfig();
+ClientX509Util clientX509Util = new ClientX509Util();
+zkConfig.setProperty(clientX509Util.getSslClientContextProperty(),
ZKTestClientSSLContext.class.getCanonicalName());
+final SSLContext sslContext =
clientX509Util.createSSLContext(zkConfig);
+assertNull(sslContext);
+}
+
Review comment:
I would also add an inner class to this test that returns a non-null context
(can be a singleton), and add a test case that exercises that code path.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248512925
##
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
##
@@ -220,6 +224,24 @@ public int getSslHandshakeTimeoutMillis() {
}
public SSLContext createSSLContext(ZKConfig config) throws
SSLContextException {
+if (config.getProperty(sslClientContextProperty) != null) {
+LOG.debug("Loading SSLContext from property '" +
sslClientContextProperty + "'");
+String sslClientContextClass =
config.getProperty(sslClientContextProperty);
+try {
+Class sslContextClass =
Class.forName(sslClientContextClass);
+ZKClientSSLContext sslContext = (ZKClientSSLContext)
sslContextClass.getConstructor().newInstance();
Review comment:
I would use `java.util.function.Supplier` instead of your own
`ZKClientSSLContext` type here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom User SSLContext
ivmaykov commented on a change in pull request #728: ZOOKEEPER-3160: Custom
User SSLContext
URL: https://github.com/apache/zookeeper/pull/728#discussion_r248513947
##
File path:
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTestClientSSLContext.java
##
@@ -0,0 +1,29 @@
+/**
+ * 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 javax.net.ssl.SSLContext;
+
+public class ZKTestClientSSLContext implements ZKClientSSLContext {
Review comment:
I think this can be an inner class in X509UtilTest since it's not used
anywhere else
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
