exceptionfactory commented on a change in pull request #4250:
URL: https://github.com/apache/nifi/pull/4250#discussion_r502724637
##########
File path:
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -284,6 +291,7 @@
public static final String DEFAULT_ZOOKEEPER_CONNECT_TIMEOUT = "3 secs";
public static final String DEFAULT_ZOOKEEPER_SESSION_TIMEOUT = "3 secs";
public static final String DEFAULT_ZOOKEEPER_ROOT_NODE = "/nifi";
+ public static final Boolean DEFAULT_ZOOKEEPER_CLIENT_SECURE = false;
Review comment:
This could be a primitive boolean.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/cluster/ZooKeeperClientConfig.java
##########
@@ -70,6 +89,38 @@ public String getRootPath() {
return rootPath;
}
+ public boolean getClientSecure() {
Review comment:
Should this be named isClientSecure() to follow Java Bean conventions?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/leader/election/CuratorLeaderElectionManager.java
##########
@@ -589,4 +601,44 @@ public void takeLeadership(final CuratorFramework client)
throws Exception {
}
}
}
+
+ public static class SecureClientZooKeeperFactory implements
ZookeeperFactory {
+
+ private static final String NETTY_CLIENT_CNXN_SOCKET =
+ "org.apache.zookeeper.ClientCnxnSocketNetty";
+
+ private ZKClientConfig zkSecureClientConfig;
+
+ public SecureClientZooKeeperFactory(final ZooKeeperClientConfig
zkConfig) {
+ this.zkSecureClientConfig = new ZKClientConfig();
+
+ // Netty is required for the secure client config.
+ final String cnxnSocket = zkConfig.getConnectionSocket();
+ if (!NETTY_CLIENT_CNXN_SOCKET.equals(cnxnSocket)) {
Review comment:
If the secure client requires the ClientCnxnSocketNetty, it seems more
straightforward to set the correct value in zkSecureClientConfig and ignore the
ZooKeeperClientConfig.getConnectionSocket() altogether, instead of confirming
that it matches the only correct value in this scenario.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/leader/election/CuratorLeaderElectionManager.java
##########
@@ -589,4 +601,44 @@ public void takeLeadership(final CuratorFramework client)
throws Exception {
}
}
}
+
+ public static class SecureClientZooKeeperFactory implements
ZookeeperFactory {
+
+ private static final String NETTY_CLIENT_CNXN_SOCKET =
+ "org.apache.zookeeper.ClientCnxnSocketNetty";
+
+ private ZKClientConfig zkSecureClientConfig;
+
+ public SecureClientZooKeeperFactory(final ZooKeeperClientConfig
zkConfig) {
+ this.zkSecureClientConfig = new ZKClientConfig();
+
+ // Netty is required for the secure client config.
+ final String cnxnSocket = zkConfig.getConnectionSocket();
+ if (!NETTY_CLIENT_CNXN_SOCKET.equals(cnxnSocket)) {
+ throw new IllegalArgumentException(String.format("connection
factory set to '%s', %s required", String.valueOf(cnxnSocket),
NETTY_CLIENT_CNXN_SOCKET));
+ }
+
zkSecureClientConfig.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET,
cnxnSocket);
+
+ // This should never happen but won't get checked elsewhere.
+ final boolean clientSecure = zkConfig.getClientSecure();
+ if (!clientSecure) {
Review comment:
Similar to the previous comment, why not just set
ZKClientConfig.SECURE_CLIENT to true regardless of ZooKeeperClientConfig,
particularly since the value is already being checked before instantiating the
class?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/cluster/ZooKeeperClientConfig.java
##########
@@ -133,6 +207,30 @@ private static int getTimePeriod(final NiFiProperties
nifiProperties, final Stri
}
}
+ private static boolean getClientSecure(final NiFiProperties
nifiProperties, final String propertyName, final boolean defaultValue) {
Review comment:
It seems like it might be better to encapsulate this property evaluation
logic using a new method on NiFiProperties
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/cluster/ZooKeeperClientConfig.java
##########
@@ -133,6 +207,30 @@ private static int getTimePeriod(final NiFiProperties
nifiProperties, final Stri
}
}
+ private static boolean getClientSecure(final NiFiProperties
nifiProperties, final String propertyName, final boolean defaultValue) {
+ final String defaultValueStr = String.valueOf(defaultValue);
+
+ String propertyStr = nifiProperties.getProperty(propertyName,
defaultValueStr);
+ propertyStr = StringUtils.stripToEmpty(propertyStr);
+ propertyStr = StringUtils.defaultIfBlank(propertyStr, defaultValueStr);
+ propertyStr = StringUtils.lowerCase(propertyStr);
+
+ if (!"true".equalsIgnoreCase(propertyStr) &&
!"false".equalsIgnoreCase(propertyStr)) {
+ throw new IllegalArgumentException(String.format("%s was '%s',
expected true or false", NiFiProperties.ZOOKEEPER_CLIENT_SECURE, propertyStr));
+ }
+
+ return Boolean.parseBoolean(propertyStr);
+ }
+
+ private static String getKeyStoreType(final String keyStore, final
NiFiProperties nifiProperties, final String propertyName) {
+ String keyStoreType =
StringUtils.stripToNull(nifiProperties.getProperty(propertyName));
+ // ZooKeeper only recognizes the .p12 extension.
+ if (keyStoreType == null && StringUtils.endsWithIgnoreCase(keyStore,
".pkcs12")) {
Review comment:
This seems like an unnecessarily special case for assisting in key store
type determination. The pkcs12 file extension does not appear to be a common
standard, instead, Wikipedia lists [p12 and pfx
](https://en.wikipedia.org/wiki/PKCS_12) as the common file extensions. Since
the ZooKeeper KeyStoreFileType attempts automatic determination based on file
extension, it seems better to rely on that capability and otherwise require the
administrator to specify the store type explicitly.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/cluster/ZooKeeperClientConfig.java
##########
@@ -30,24 +30,43 @@
public class ZooKeeperClientConfig {
+ public static final String NETTY_CLIENT_CNXN_SOCKET =
"org.apache.zookeeper.ClientCnxnSocketNetty";
+ public static final String NIO_CLIENT_CNXN_SOCKET =
"org.apache.zookeeper.ClientCnxnSocketNIO";
Review comment:
For clarity of reference, would it be better to declare these variables
using the actual classes and calling class.getName()?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/leader/election/TestSecureClientZooKeeperFactory.java
##########
@@ -0,0 +1,192 @@
+/*
+ * 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.nifi.leader.election;
+
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.InstanceSpec;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+
+import org.apache.nifi.util.NiFiProperties;
+import org.apache.nifi.properties.StandardNiFiProperties;
+import org.apache.nifi.controller.cluster.ZooKeeperClientConfig;
+import
org.apache.nifi.controller.leader.election.CuratorLeaderElectionManager.SecureClientZooKeeperFactory;
+
+import org.apache.zookeeper.common.ClientX509Util;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZooKeeperServer;
+
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.testng.Assert;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Properties;
+
+public class TestSecureClientZooKeeperFactory {
+
+ private static final String NETTY_SERVER_CNXN_FACTORY =
+ "org.apache.zookeeper.server.NettyServerCnxnFactory";
+
+ private static final String CLIENT_KEYSTORE =
"src/test/resources/TestSecureClientZooKeeperFactory/client.keystore.p12";
+ private static final String CLIENT_TRUSTSTORE =
"src/test/resources/TestSecureClientZooKeeperFactory/client.truststore.p12";
+ private static final String SERVER_KEYSTORE =
"src/test/resources/TestSecureClientZooKeeperFactory/server.keystore.p12";
+ private static final String SERVER_TRUSTSTORE =
"src/test/resources/TestSecureClientZooKeeperFactory/server.truststore.p12";
Review comment:
Although it would take slightly longer to start the test, did you
consider using CertificateUtils to generate self-signed certificates and
writing temporary key store and trust store files, as opposed to including
additional test PKCS12 binaries?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
