[ https://issues.apache.org/jira/browse/HADOOP-18709?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17721948#comment-17721948 ]
ASF GitHub Bot commented on HADOOP-18709: ----------------------------------------- szilard-nemeth commented on code in PR #5638: URL: https://github.com/apache/hadoop/pull/5638#discussion_r1191770472 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/curator/ZKCuratorManager.java: ########## @@ -157,12 +175,44 @@ public void start(List<AuthInfo> authInfos) throws IOException { authInfos.add(new AuthInfo(zkAuth.getScheme(), zkAuth.getAuth())); } + /* Pre-check on SSL/TLS client connection requirements to emit the name of the + configuration missing. It improves supportability. */ + if(sslEnabled) { + if (StringUtils.isEmpty(conf.get(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION))) { + throw new ConfigurationException( Review Comment: Nit: Can you extract this to a helper method? Only the config name should be passed, the rest of the method body (even the exception message) can be the same. ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/curator/ZKCuratorManager.java: ########## @@ -478,10 +558,53 @@ public ZooKeeper newZooKeeper(String connectString, int sessionTimeout, if (zkClientConfig.isSaslClientEnabled() && !isJaasConfigurationSet(zkClientConfig)) { setJaasConfiguration(zkClientConfig); } + if (sslEnabled) { + setSslConfiguration(zkClientConfig); + } return new ZooKeeper(connectString, sessionTimeout, watcher, canBeReadOnly, zkClientConfig); } + /** + * Configure ZooKeeper Client with SSL/TLS connection. + * @param zkClientConfig ZooKeeper Client configuration + * */ + private void setSslConfiguration(ZKClientConfig zkClientConfig) throws ConfigurationException { + this.setSslConfiguration(zkClientConfig, new ClientX509Util()); + } + public void setSslConfiguration(ZKClientConfig zkClientConfig, ClientX509Util x509Util ) Review Comment: Nit: Line break between methods. Also it seems your formatter seems to be off, as method parentheses has an additional space before it after the parameters. ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/curator/ZKCuratorManager.java: ########## @@ -478,10 +558,53 @@ public ZooKeeper newZooKeeper(String connectString, int sessionTimeout, if (zkClientConfig.isSaslClientEnabled() && !isJaasConfigurationSet(zkClientConfig)) { setJaasConfiguration(zkClientConfig); } + if (sslEnabled) { + setSslConfiguration(zkClientConfig); + } return new ZooKeeper(connectString, sessionTimeout, watcher, canBeReadOnly, zkClientConfig); } + /** + * Configure ZooKeeper Client with SSL/TLS connection. + * @param zkClientConfig ZooKeeper Client configuration + * */ + private void setSslConfiguration(ZKClientConfig zkClientConfig) throws ConfigurationException { + this.setSslConfiguration(zkClientConfig, new ClientX509Util()); + } + public void setSslConfiguration(ZKClientConfig zkClientConfig, ClientX509Util x509Util ) + throws ConfigurationException { + LOG.info("Configuring the ZooKeeper client to use SSL/TLS encryption for connecting to the ZooKeeper server."); + if (StringUtils.isEmpty(this.keystoreLocation)) { Review Comment: Nit: Here you could also extract the isempty check + throwing the exception to a method. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; Review Comment: can be static final ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/curator/ZKCuratorManager.java: ########## @@ -478,10 +558,53 @@ public ZooKeeper newZooKeeper(String connectString, int sessionTimeout, if (zkClientConfig.isSaslClientEnabled() && !isJaasConfigurationSet(zkClientConfig)) { setJaasConfiguration(zkClientConfig); } + if (sslEnabled) { + setSslConfiguration(zkClientConfig); + } return new ZooKeeper(connectString, sessionTimeout, watcher, canBeReadOnly, zkClientConfig); } + /** + * Configure ZooKeeper Client with SSL/TLS connection. + * @param zkClientConfig ZooKeeper Client configuration + * */ + private void setSslConfiguration(ZKClientConfig zkClientConfig) throws ConfigurationException { + this.setSslConfiguration(zkClientConfig, new ClientX509Util()); + } + public void setSslConfiguration(ZKClientConfig zkClientConfig, ClientX509Util x509Util ) + throws ConfigurationException { + LOG.info("Configuring the ZooKeeper client to use SSL/TLS encryption for connecting to the ZooKeeper server."); + if (StringUtils.isEmpty(this.keystoreLocation)) { + throw new ConfigurationException( + "The keystore location parameter is empty for the ZooKeeper client connection."); + } + if (StringUtils.isEmpty(this.keystorePassword)) { + throw new ConfigurationException( + "The keystore password parameter is empty for the ZooKeeper client connection."); + } + if (StringUtils.isEmpty(this.truststoreLocation)) { + throw new ConfigurationException( + "The truststore location parameter is empty for the ZooKeeper client connection."); + } + if (StringUtils.isEmpty(this.truststorePassword)) { + throw new ConfigurationException( + "The truststore password parameter is empty for the ZooKeeper client connection."); + } + LOG.debug("Configuring the ZooKeeper client with {} {} location.", + this.keystoreLocation, CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION); Review Comment: maybe it's better / more straightforward as: ```suggestion LOG.debug("Configuring the ZooKeeper client with {} location: {}", this.keystoreLocation, CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION); ``` ########## hadoop-common-project/hadoop-common/pom.xml: ########## @@ -342,6 +342,14 @@ </exclusion> </exclusions> </dependency> + <dependency> Review Comment: What's the reason of adding the netty dependencies here? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); Review Comment: can be static final ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, Review Comment: For better readability, these could be extracted to static final fields or local variables. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, + -1, + true, + 1, + 100, + 10, + customConfiguration); + this.server = new TestingServer(spec, true); + hadoopConf.set(CommonConfigurationKeys.ZK_ADDRESS, this.server.getConnectString()); + this.curator = new ZKCuratorManager(hadoopConf); + this.curator.start(new ArrayList<>(), true); + } + + public Configuration setUpSecure() throws Exception { + Configuration hadoopConf = new Configuration(); + String testDataPath = "src/test/java/org/apache/hadoop/util/curator/resources/data"; + System.setProperty("zookeeper.serverCnxnFactory", "org.apache.zookeeper.server.NettyServerCnxnFactory"); + //System.setProperty("zookeeper.client.secure", "true"); Review Comment: Can this be removed? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, + -1, + true, + 1, + 100, + 10, + customConfiguration); + this.server = new TestingServer(spec, true); + hadoopConf.set(CommonConfigurationKeys.ZK_ADDRESS, this.server.getConnectString()); + this.curator = new ZKCuratorManager(hadoopConf); + this.curator.start(new ArrayList<>(), true); + } + + public Configuration setUpSecure() throws Exception { + Configuration hadoopConf = new Configuration(); + String testDataPath = "src/test/java/org/apache/hadoop/util/curator/resources/data"; + System.setProperty("zookeeper.serverCnxnFactory", "org.apache.zookeeper.server.NettyServerCnxnFactory"); + //System.setProperty("zookeeper.client.secure", "true"); + + + System.setProperty("zookeeper.ssl.keyStore.location", testDataPath + "/ssl/keystore.jks"); + System.setProperty("zookeeper.ssl.keyStore.password", "password"); + System.setProperty("zookeeper.ssl.trustStore.location", testDataPath + "/ssl/truststore.jks"); + System.setProperty("zookeeper.ssl.trustStore.password", "password"); + System.setProperty("zookeeper.request.timeout", "12345"); + + System.setProperty("jute.maxbuffer", "469296129"); Review Comment: Is there a meaning of this number? Can it be calculated and saved to a static final int constant? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, + -1, + true, + 1, + 100, + 10, + customConfiguration); + this.server = new TestingServer(spec, true); + hadoopConf.set(CommonConfigurationKeys.ZK_ADDRESS, this.server.getConnectString()); + this.curator = new ZKCuratorManager(hadoopConf); + this.curator.start(new ArrayList<>(), true); + } + + public Configuration setUpSecure() throws Exception { + Configuration hadoopConf = new Configuration(); + String testDataPath = "src/test/java/org/apache/hadoop/util/curator/resources/data"; + System.setProperty("zookeeper.serverCnxnFactory", "org.apache.zookeeper.server.NettyServerCnxnFactory"); + //System.setProperty("zookeeper.client.secure", "true"); + + + System.setProperty("zookeeper.ssl.keyStore.location", testDataPath + "/ssl/keystore.jks"); + System.setProperty("zookeeper.ssl.keyStore.password", "password"); + System.setProperty("zookeeper.ssl.trustStore.location", testDataPath + "/ssl/truststore.jks"); + System.setProperty("zookeeper.ssl.trustStore.password", "password"); + System.setProperty("zookeeper.request.timeout", "12345"); + + System.setProperty("jute.maxbuffer", "469296129"); + + System.setProperty("javax.net.debug", "ssl"); + System.setProperty("zookeeper.authProvider.x509", "org.apache.zookeeper.server.auth.X509AuthenticationProvider"); + + + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION, testDataPath + "/ssl/keystore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_PASSWORD, "password"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_LOCATION, testDataPath + "/ssl/truststore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_PASSWORD, "password"); + return hadoopConf; + } + + @After + public void teardown() throws Exception { + this.curator.close(); + if (this.server != null) { + this.server.close(); + this.server = null; Review Comment: what's the reason of setting it to null? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, + -1, + true, + 1, + 100, + 10, + customConfiguration); + this.server = new TestingServer(spec, true); + hadoopConf.set(CommonConfigurationKeys.ZK_ADDRESS, this.server.getConnectString()); + this.curator = new ZKCuratorManager(hadoopConf); + this.curator.start(new ArrayList<>(), true); + } + + public Configuration setUpSecure() throws Exception { + Configuration hadoopConf = new Configuration(); + String testDataPath = "src/test/java/org/apache/hadoop/util/curator/resources/data"; + System.setProperty("zookeeper.serverCnxnFactory", "org.apache.zookeeper.server.NettyServerCnxnFactory"); + //System.setProperty("zookeeper.client.secure", "true"); + + + System.setProperty("zookeeper.ssl.keyStore.location", testDataPath + "/ssl/keystore.jks"); + System.setProperty("zookeeper.ssl.keyStore.password", "password"); + System.setProperty("zookeeper.ssl.trustStore.location", testDataPath + "/ssl/truststore.jks"); + System.setProperty("zookeeper.ssl.trustStore.password", "password"); + System.setProperty("zookeeper.request.timeout", "12345"); + + System.setProperty("jute.maxbuffer", "469296129"); + + System.setProperty("javax.net.debug", "ssl"); + System.setProperty("zookeeper.authProvider.x509", "org.apache.zookeeper.server.auth.X509AuthenticationProvider"); + + + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION, testDataPath + "/ssl/keystore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_PASSWORD, "password"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_LOCATION, testDataPath + "/ssl/truststore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_PASSWORD, "password"); + return hadoopConf; + } + + @After + public void teardown() throws Exception { + this.curator.close(); + if (this.server != null) { + this.server.close(); + this.server = null; + } + } + + @Test + public void testSecureZKConfiguration() throws Exception { + LOG.info("Entered to the testSecureZKConfiguration test case."); + // Validate that HadoopZooKeeperFactory will set ZKConfig with given principals + ZKCuratorManager.HadoopZookeeperFactory factory1 = + new ZKCuratorManager.HadoopZookeeperFactory( + null, + null, + null, + true, + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_KEYSTORE_PASSWORD), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_LOCATION), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_PASSWORD)); + ZooKeeper zk1 = factory1.newZooKeeper(this.server.getConnectString(), 1000, null, false); + validateSSLConfiguration( + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_KEYSTORE_PASSWORD), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_LOCATION), + this.hadoopConf.get(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_PASSWORD), + zk1); + } + + private void validateSSLConfiguration( + String keystoreLocation, + String keystorePassword, + String truststoreLocation, + String truststorePassword, + ZooKeeper zk){ + ClientX509Util x509Util = new ClientX509Util(); Review Comment: IntelliJ gives me a warning: 'ClientX509Util' used without 'try'-with-resources statement ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/curator/TestSecureZKCuratorManager.java: ########## @@ -0,0 +1,157 @@ +/** + * 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.hadoop.util.curator; + +import org.apache.curator.test.InstanceSpec; +import org.apache.curator.test.TestingServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.ClientX509Util; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.hadoop.fs.FileContext.LOG; +import static org.junit.Assert.assertEquals; + + +/** + * Test the manager for ZooKeeper Curator when SSL/TLS is enabled for the ZK server-client connection. + */ +public class TestSecureZKCuratorManager { + + private TestingServer server; + private ZKCuratorManager curator; + private Configuration hadoopConf; + private Integer secureClientPort = 2281; + private File zkDataDir = new File("testZkSSLClientConnectionDataDir"); + + @Before + public void setup() throws Exception { + //set zkServer + this.hadoopConf = setUpSecure(); + Map<String, Object> customConfiguration = new HashMap<>(); + customConfiguration.put("secureClientPort",this.secureClientPort.toString()); + customConfiguration.put("audit.enable",true); + + InstanceSpec spec = new InstanceSpec( + this.zkDataDir, + this.secureClientPort, + -1, + -1, + true, + 1, + 100, + 10, + customConfiguration); + this.server = new TestingServer(spec, true); + hadoopConf.set(CommonConfigurationKeys.ZK_ADDRESS, this.server.getConnectString()); + this.curator = new ZKCuratorManager(hadoopConf); + this.curator.start(new ArrayList<>(), true); + } + + public Configuration setUpSecure() throws Exception { + Configuration hadoopConf = new Configuration(); + String testDataPath = "src/test/java/org/apache/hadoop/util/curator/resources/data"; + System.setProperty("zookeeper.serverCnxnFactory", "org.apache.zookeeper.server.NettyServerCnxnFactory"); + //System.setProperty("zookeeper.client.secure", "true"); + + + System.setProperty("zookeeper.ssl.keyStore.location", testDataPath + "/ssl/keystore.jks"); + System.setProperty("zookeeper.ssl.keyStore.password", "password"); + System.setProperty("zookeeper.ssl.trustStore.location", testDataPath + "/ssl/truststore.jks"); + System.setProperty("zookeeper.ssl.trustStore.password", "password"); + System.setProperty("zookeeper.request.timeout", "12345"); + + System.setProperty("jute.maxbuffer", "469296129"); + + System.setProperty("javax.net.debug", "ssl"); + System.setProperty("zookeeper.authProvider.x509", "org.apache.zookeeper.server.auth.X509AuthenticationProvider"); + + + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_LOCATION, testDataPath + "/ssl/keystore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_KEYSTORE_PASSWORD, "password"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_LOCATION, testDataPath + "/ssl/truststore.jks"); + hadoopConf.set(CommonConfigurationKeys.ZK_SSL_TRUSTSTORE_PASSWORD, "password"); + return hadoopConf; + } + + @After + public void teardown() throws Exception { + this.curator.close(); + if (this.server != null) { + this.server.close(); + this.server = null; + } + } + + @Test + public void testSecureZKConfiguration() throws Exception { + LOG.info("Entered to the testSecureZKConfiguration test case."); + // Validate that HadoopZooKeeperFactory will set ZKConfig with given principals + ZKCuratorManager.HadoopZookeeperFactory factory1 = Review Comment: nit: Can you call the variables "factory" and "zk"? > Add curator based ZooKeeper communication support over SSL/TLS into the > common library > -------------------------------------------------------------------------------------- > > Key: HADOOP-18709 > URL: https://issues.apache.org/jira/browse/HADOOP-18709 > Project: Hadoop Common > Issue Type: Improvement > Reporter: Ferenc Erdelyi > Assignee: Ferenc Erdelyi > Priority: Major > Labels: pull-request-available > > With HADOOP-16579 the ZooKeeper client is capable of securing communication > with SSL. > To follow the convention introduced in HADOOP-14741, proposing to add to the > core-default.xml the following configurations, as the groundwork for the > components to enable encrypted communication between the individual > components and ZooKeeper: > * hadoop.zk.ssl.keystore.location > * hadoop.zk.ssl.keystore.password > * hadoop.zk.ssl.truststore.location > * hadoop.zk.ssl.truststore.password > These parameters along with the component-specific ssl.client.enable option > (e.g. yarn.zookeeper.ssl.client.enable) should be passed to the > ZKCuratorManager to build the CuratorFramework. The ZKCuratorManager needs a > new overloaded start() method to build the encrypted communication. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org