ayushtkn commented on code in PR #3959: URL: https://github.com/apache/ozone/pull/3959#discussion_r1032928590
########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/ssl/MonitoringTimerTask.java: ########## @@ -0,0 +1,81 @@ +/** + * 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.hdds.security.ssl; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient; +import org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.thirdparty.com.google.common.base.Preconditions; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.TimerTask; +import java.util.function.Consumer; + +/** + * Implements basic logic to track when security materials changes and call + * the action passed to the constructor when it does. An exception handler + * can optionally also be specified in the constructor, otherwise any + * exception occurring during process will be logged using this class' logger. + */ [email protected] +public class MonitoringTimerTask extends TimerTask { + + static final Logger LOG = LoggerFactory.getLogger(MonitoringTimerTask.class); + + @VisibleForTesting + static final String PROCESS_ERROR_MESSAGE = + "Could not process security materials change : "; + + private final CertificateClient caClient; + private final Consumer<CertificateClient> onReload; + private final Consumer<Throwable> onReloadFailure; + + /** + * See {@link #MonitoringTimerTask(CertificateClient, Consumer, Consumer)}. Review Comment: This Javadoc of the method is pointing to itself only? ########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java: ########## @@ -0,0 +1,179 @@ +/** + * 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.hdds.security.ssl; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient; +import org.apache.hadoop.security.ssl.SSLFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.net.ssl.KeyManager; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.X509ExtendedKeyManager; +import java.io.IOException; +import java.net.Socket; +import java.security.GeneralSecurityException; +import java.security.KeyStore; +import java.security.Principal; +import java.security.PrivateKey; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; +import java.util.concurrent.atomic.AtomicReference; + +/** + * An implementation of <code>X509KeyManager</code> that exposes a method, + * {@link #loadFrom(CertificateClient)} to reload its configuration. + * Note that it is necessary to implement the + * <code>X509ExtendedKeyManager</code> to properly delegate + * the additional methods, otherwise the SSL handshake will fail. + */ [email protected] [email protected] +public class ReloadingX509KeyManager extends X509ExtendedKeyManager { + + public static final Logger LOG = + LoggerFactory.getLogger(ReloadingX509TrustManager.class); + + static final String RELOAD_ERROR_MESSAGE = + "Could not reload keystore (keep using existing one) : "; + + private final String type; + /** + * Default password. KeyStore and trustStore will not persist to disk, just in + * memory. + */ + static final char[] EMPTY_PASSWORD = new char[0]; + private AtomicReference<X509ExtendedKeyManager> keyManagerRef; Review Comment: This can be final ########## hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/ssl/TestReloadingX509KeyManager.java: ########## @@ -0,0 +1,81 @@ +/** + * 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.hdds.security.ssl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.security.x509.CertificateClientTest; +import org.apache.ozone.test.GenericTestUtils.LogCapturer; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.security.PrivateKey; +import java.util.Timer; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +/** + * Test ReloadingX509KeyManager. + */ +public class TestReloadingX509KeyManager { + private final LogCapturer reloaderLog = + LogCapturer.captureLogs(ReloadingX509KeyManager.LOG); + private static OzoneConfiguration conf; + private static CertificateClientTest caClient; + + @BeforeClass + public static void setUp() throws Exception { + conf = new OzoneConfiguration(); + caClient = new CertificateClientTest(conf); + } + + @Test (timeout = 3000000) + public void testReload() throws Exception { + long reloadInterval = 1000; + Timer fileMonitoringTimer = new Timer(true); + ReloadingX509KeyManager km = new ReloadingX509KeyManager("jks", caClient); + try { + fileMonitoringTimer.schedule(new MonitoringTimerTask(caClient, + km::loadFrom, null), reloadInterval, reloadInterval); + PrivateKey privateKey1 = caClient.getPrivateKey(); + assertEquals(privateKey1, + km.getPrivateKey(caClient.getComponentName() + "_key")); + + caClient.renewKey(); + PrivateKey privateKey2 = caClient.getPrivateKey(); + assertNotEquals(privateKey1, privateKey2); + + // Wait so that the new certificate get reloaded + Thread.sleep((reloadInterval + 1000)); + assertEquals(privateKey2, + km.getPrivateKey(caClient.getComponentName() + "_key")); + + assertTrue(reloaderLog.getOutput().contains( + "ReloadingX509KeyManager is reloaded")); + + // Make sure there is only one reload happened. + Thread.sleep((reloadInterval + 1000)); Review Comment: This sleep can sometime lead to test failures, can we use GenericTestUtils.waitFor instead and can have a better margin to safegaurd this test from flakiness? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
