You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2020/11/18 04:50:24 UTC
[GitHub] [hadoop] bolerio opened a new pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
bolerio opened a new pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470
Addresses https://issues.apache.org/jira/browse/HADOOP-16524, in addition covering the DataNode use case.
- Following the existing ReloadingX509TrustManager, a new ReloadingX509KeystoreManager was created.
- Existing code slightly refactored so both trust manager and keystore reloading managers share the monitoring logic within a single java.util.Timer (and therefore a single thread).
- In HttpServer2, the same strategy as a previously proposed patch (see above) is used with SSLContextFactory.reload, but with the addition of cleanup upon stopping the server.
- A new config parameter which applies to all of the above FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY supersedes the existing FileBasedKeyStoresFactory.SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY which only applies to trust store reloading. Setting the value to 0 (default is 10s) to this parameter disables the reloading.
If this PR is accepted, I would need guidance where/how to update the docs with that new configuration parameter.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r534456483
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
Review comment:
nit: this.configurationChangeMonitor is already Optional.empty? So, change this to be if (storesReloadInterval > 0) {....}?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
+ java.util.Timer timer = new java.util.Timer("SSL Certificates Store Monitor", true);
+ //
+ // The Jetty SSLContextFactory provides a 'reload' method which will reload both
+ // truststore and keystore certificates.
+ //
+ timer.schedule(new FileMonitoringTimerTask(
+ Paths.get(keyStore),
+ path -> {
+ LOG.info("Reloading certificates from store keystore " + keyStore);
+ try {
+ sslContextFactory.reload(factory -> { });
+ }
+ catch (Exception ex) {
Review comment:
nit: See the formatting for catch in the rest of the file. It does not do this.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
+ java.util.Timer timer = new java.util.Timer("SSL Certificates Store Monitor", true);
Review comment:
Why the full qualification of Timer here and as function return? You have imported java.util.Timer so no need of these 'java.util' prefixes?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
Review comment:
This class is audience private so presuming it ok changing this public static's name.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
Review comment:
Does the rest of this class do this extreme rightward shifting of parameters?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -29,20 +29,20 @@
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.TrustManager;
import java.io.IOException;
-import java.io.InputStream;
-import java.nio.file.Files;
import java.nio.file.Paths;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
import java.text.MessageFormat;
+import java.util.Timer;
/**
* {@link KeyStoresFactory} implementation that reads the certificates from
* keystore files.
* <p>
- * if the trust certificates keystore file changes, the {@link TrustManager}
- * is refreshed with the new trust certificate entries (using a
- * {@link ReloadingX509TrustManager} trustmanager).
+ * If either the truststore or the keystore certificates file changes, it would be refreshed
Review comment:
nit: line lengths
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
+ }
+
+ trustManager = new ReloadingX509TrustManager(
+ truststoreType,
+ truststoreLocation,
+ truststorePassword);
Review comment:
A line per parameter is not how the background file does it?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
Review comment:
Keep the style of the backing class... It doesn't do these big right-shifts on parameters that go to the second line. Be careful w/ line lengths too. The backing file seems to do 80 chars.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538856198
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
Review comment:
Fixed. Also fixed line length issues in this file.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538850068
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
Review comment:
Yeah, but it's true the most of the code base doesn't have it. Should I change? I don't mind...just inadvertently sneaking in my old habits I suppose....
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538849365
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
Review comment:
Ok
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] sunchao commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-736068748
cc @kihwal @saintstack
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] amahussein commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
amahussein commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-758072073
@saintstack Thanks for your contribution.
This PR caused failures to other unit tests. Can you please take a look to the list of failures.
https://issues.apache.org/jira/browse/HADOOP-16524?focusedCommentId=17262765&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17262765
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r539789676
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
Review comment:
Seems like your code habits align w/ the backing file here. I'd leave it as is.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r535858254
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
Review comment:
getPassword will never return null?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
Review comment:
Log the interval found in config here too?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
##########
@@ -0,0 +1,88 @@
+/**
+ * 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.security.ssl;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.file.Path;
+import java.util.TimerTask;
+import java.util.function.Consumer;
+
+/**
+ * <p>
+ * Implements basic logic to track when a file changes on disk 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.
+ * </p>
+ */
+@InterfaceAudience.Private
+public class FileMonitoringTimerTask extends TimerTask {
+
+ static final Logger LOG = LoggerFactory.getLogger(FileMonitoringTimerTask.class);
+
+ @VisibleForTesting
+ static final String PROCESS_ERROR_MESSAGE =
+ "Could not process file change : ";
+
+ final private Path filePath;
+ final private Consumer<Path> onFileChange;
+ final Consumer<Throwable> onChangeFailure;
+ private long lastProcessed;
+
+ /**
+ * Create file monitoring task to be scheduled using a standard Java {@link java.util.Timer}
+ * instance.
+ *
+ * @param filePath The path to the file to monitor.
+ * @param onFileChange The function to call when the file has changed.
+ * @param onChangeFailure The function to call when an exception is thrown during the
+ * file change processing.
+ */
+ public FileMonitoringTimerTask(Path filePath,
+ Consumer<Path> onFileChange,
+ Consumer<Throwable> onChangeFailure) {
+ Preconditions.checkNotNull(filePath, "path to monitor disk file is not set");
+ Preconditions.checkNotNull(onFileChange, "action to monitor disk file is not set");
+
+ this.filePath = filePath;
+ this.lastProcessed = filePath.toFile().lastModified();
+ this.onFileChange = onFileChange;
+ this.onChangeFailure = onChangeFailure;
+ }
+
+ @Override
+ public void run() {
+ if (lastProcessed != filePath.toFile().lastModified()) {
+ try {
+ onFileChange.accept(filePath);
+ } catch (Throwable t) {
+ if (onChangeFailure != null) {
+ onChangeFailure.accept(t);
+ } else {
+ LOG.error(PROCESS_ERROR_MESSAGE + filePath.toString(), t);
+ }
+ }
+ lastProcessed = filePath.toFile().lastModified();
+ }
+ }
Review comment:
The tab offsets seem to be 4 spaces when should be 2 as it is elsewhere in hadoop.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
Review comment:
Can be final?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
+ }
+
+ trustManager = new ReloadingX509TrustManager(
+ truststoreType,
+ truststoreLocation,
+ truststorePassword);
+
+ if (truststoreReloadInterval > 0) {
+ fileMonitoringTimer.schedule(
+ new FileMonitoringTimerTask(
+ Paths.get(truststoreLocation),
+ path -> trustManager.loadFrom(path),
+ exception -> LOG.error(ReloadingX509TrustManager.RELOAD_ERROR_MESSAGE, exception)),
+ truststoreReloadInterval,
+ truststoreReloadInterval);
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " Loaded TrustStore: " + truststoreLocation);
+ }
+ trustManagers = new TrustManager[]{trustManager};
+ }
+
+ /**
+ * Implements logic of initializing the KeyManagers with the options
+ * to reload keystores.
+ * @param mode client or server
+ * @param keystoreType The keystore type.
+ * @param storesReloadInterval The interval to check if the keystore certificates
+ * file has changed.
+ */
+ private void createKeyManagersFromConfiguration(SSLFactory.Mode mode,
+ String keystoreType, long storesReloadInterval)
+ throws GeneralSecurityException, IOException {
+ String locationProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_LOCATION_TPL_KEY);
+ String keystoreLocation = conf.get(locationProperty, "");
+ if (keystoreLocation.isEmpty()) {
+ throw new GeneralSecurityException("The property '" + locationProperty +
+ "' has not been set in the ssl configuration file.");
+ }
+ String passwordProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_PASSWORD_TPL_KEY);
+ String keystorePassword = getPassword(conf, passwordProperty, "");
+ if (keystorePassword.isEmpty()) {
+ throw new GeneralSecurityException("The property '" + passwordProperty +
+ "' has not been set in the ssl configuration file.");
+ }
+ String keyPasswordProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_KEYPASSWORD_TPL_KEY);
+ // Key password defaults to the same value as store password for
+ // compatibility with legacy configurations that did not use a separate
+ // configuration property for key password.
+ String keystoreKeyPassword = getPassword(
+ conf, keyPasswordProperty, keystorePassword);
Review comment:
Do we have to check isEmpty here too?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
##########
@@ -0,0 +1,88 @@
+/**
+ * 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.security.ssl;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.file.Path;
+import java.util.TimerTask;
+import java.util.function.Consumer;
+
+/**
+ * <p>
+ * Implements basic logic to track when a file changes on disk 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.
+ * </p>
+ */
+@InterfaceAudience.Private
+public class FileMonitoringTimerTask extends TimerTask {
+
+ static final Logger LOG = LoggerFactory.getLogger(FileMonitoringTimerTask.class);
+
+ @VisibleForTesting
+ static final String PROCESS_ERROR_MESSAGE =
+ "Could not process file change : ";
+
+ final private Path filePath;
+ final private Consumer<Path> onFileChange;
+ final Consumer<Throwable> onChangeFailure;
+ private long lastProcessed;
+
Review comment:
nit: not consistent in where we locate private... sometimes at start (the usual) and other times as second qualifier. (Can we make all these datamembers private?)
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
##########
@@ -0,0 +1,88 @@
+/**
+ * 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.security.ssl;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.file.Path;
+import java.util.TimerTask;
+import java.util.function.Consumer;
+
+/**
+ * <p>
Review comment:
No need of the <p> wrappers.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
+
+ @Override
+ public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) {
+ return keyManagerRef.get().chooseEngineClientAlias(strings, principals, sslEngine);
+ }
+
+ @Override
+ public String chooseEngineServerAlias(String s, Principal[] principals, SSLEngine sslEngine) {
+ return keyManagerRef.get().chooseEngineServerAlias(s, principals, sslEngine);
+ }
+
+ @Override
+ public String[] getClientAliases(String s, Principal[] principals) {
+ return keyManagerRef.get().getClientAliases(s, principals);
+ }
+
+ @Override
+ public String chooseClientAlias(String[] strings, Principal[] principals, Socket socket) {
+ return keyManagerRef.get().chooseClientAlias(strings, principals, socket);
+ }
+
+ @Override
+ public String[] getServerAliases(String s, Principal[] principals) {
+ return keyManagerRef.get().getServerAliases(s, principals);
+ }
+
+ @Override
+ public String chooseServerAlias(String s, Principal[] principals, Socket socket) {
+ return keyManagerRef.get().chooseServerAlias(s, principals, socket);
+ }
+
+ @Override
+ public X509Certificate[] getCertificateChain(String s) {
+ return keyManagerRef.get().getCertificateChain(s);
+ }
+
+ @Override
+ public PrivateKey getPrivateKey(String s) {
+ return keyManagerRef.get().getPrivateKey(s);
+ }
+
+ public ReloadingX509KeystoreManager loadFrom(Path path) {
+ try {
+ this.keyManagerRef.set(loadKeyManager(path));
+ } catch (Exception ex) {
+ // The Consumer.accept interface forces us to convert to unchecked
+ throw new RuntimeException(ex);
Review comment:
What will happen when this comes out? Where will it be caught? Will it cause damage causing process exit or thread exit?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
+
+ @Override
+ public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) {
Review comment:
Line lengths?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
Review comment:
Hmm... maybe it does. Ignore the above comment then.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
Review comment:
Yeah, tab seems to be 4 when should be 2 spaces.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538853876
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
+ }
+
+ trustManager = new ReloadingX509TrustManager(
+ truststoreType,
+ truststoreLocation,
+ truststorePassword);
+
+ if (truststoreReloadInterval > 0) {
+ fileMonitoringTimer.schedule(
+ new FileMonitoringTimerTask(
+ Paths.get(truststoreLocation),
+ path -> trustManager.loadFrom(path),
+ exception -> LOG.error(ReloadingX509TrustManager.RELOAD_ERROR_MESSAGE, exception)),
+ truststoreReloadInterval,
+ truststoreReloadInterval);
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " Loaded TrustStore: " + truststoreLocation);
+ }
+ trustManagers = new TrustManager[]{trustManager};
+ }
+
+ /**
+ * Implements logic of initializing the KeyManagers with the options
+ * to reload keystores.
+ * @param mode client or server
+ * @param keystoreType The keystore type.
+ * @param storesReloadInterval The interval to check if the keystore certificates
+ * file has changed.
+ */
+ private void createKeyManagersFromConfiguration(SSLFactory.Mode mode,
+ String keystoreType, long storesReloadInterval)
+ throws GeneralSecurityException, IOException {
+ String locationProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_LOCATION_TPL_KEY);
+ String keystoreLocation = conf.get(locationProperty, "");
+ if (keystoreLocation.isEmpty()) {
+ throw new GeneralSecurityException("The property '" + locationProperty +
+ "' has not been set in the ssl configuration file.");
+ }
+ String passwordProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_PASSWORD_TPL_KEY);
+ String keystorePassword = getPassword(conf, passwordProperty, "");
+ if (keystorePassword.isEmpty()) {
+ throw new GeneralSecurityException("The property '" + passwordProperty +
+ "' has not been set in the ssl configuration file.");
+ }
+ String keyPasswordProperty =
+ resolvePropertyName(mode, SSL_KEYSTORE_KEYPASSWORD_TPL_KEY);
+ // Key password defaults to the same value as store password for
+ // compatibility with legacy configurations that did not use a separate
+ // configuration property for key password.
+ String keystoreKeyPassword = getPassword(
+ conf, keyPasswordProperty, keystorePassword);
Review comment:
Not sure: we have already enforced that keystorePassword not be empty so we'd get empty here only the config explicitly set it to "". I could add the check, but it will break past behavior in case someone is already deploying with and empty password.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538848840
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
Review comment:
Fixed.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
Review comment:
Fixed.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
+ java.util.Timer timer = new java.util.Timer("SSL Certificates Store Monitor", true);
+ //
+ // The Jetty SSLContextFactory provides a 'reload' method which will reload both
+ // truststore and keystore certificates.
+ //
+ timer.schedule(new FileMonitoringTimerTask(
+ Paths.get(keyStore),
+ path -> {
+ LOG.info("Reloading certificates from store keystore " + keyStore);
+ try {
+ sslContextFactory.reload(factory -> { });
+ }
+ catch (Exception ex) {
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-741592031
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 30m 48s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | | 0m 0s | [test4tests](test4tests) | The patch appears to include 2 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 35m 34s | | trunk passed |
| +1 :green_heart: | compile | 21m 27s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | compile | 18m 4s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | checkstyle | 0m 49s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 27s | | trunk passed |
| +1 :green_heart: | shadedclient | 19m 40s | | branch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 0m 57s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 29s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +0 :ok: | spotbugs | 2m 21s | | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 19s | | trunk passed |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 54s | | the patch passed |
| +1 :green_heart: | compile | 21m 55s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javac | 21m 55s | | the patch passed |
| +1 :green_heart: | compile | 20m 20s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | javac | 20m 20s | | the patch passed |
| -0 :warning: | checkstyle | 1m 0s | [/diff-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/3/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 168 new + 68 unchanged - 11 fixed = 236 total (was 79) |
| +1 :green_heart: | mvnsite | 1m 42s | | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | | The patch has no whitespace issues. |
| +1 :green_heart: | shadedclient | 19m 27s | | patch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 1m 11s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 39s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | findbugs | 2m 50s | | the patch passed |
|||| _ Other Tests _ |
| -1 :x: | unit | 10m 40s | [/patch-unit-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/3/artifact/out/patch-unit-hadoop-common-project_hadoop-common.txt) | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 57s | | The patch does not generate ASF License warnings. |
| | | 217m 30s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.security.TestFixKerberosTicketOrder |
| | hadoop.metrics2.source.TestJvmMetrics |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/2470 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle |
| uname | Linux 055faef77fb3 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / aaf9e3d320a |
| Default Java | Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/3/testReport/ |
| Max. process+thread count | 1345 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/3/console |
| versions | git=2.17.1 maven=3.6.0 findbugs=4.0.6 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-756573808
Rerunning tests to see if failures related (Getting ready to merge)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538859450
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
+
+ @Override
+ public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) {
+ return keyManagerRef.get().chooseEngineClientAlias(strings, principals, sslEngine);
+ }
+
+ @Override
+ public String chooseEngineServerAlias(String s, Principal[] principals, SSLEngine sslEngine) {
+ return keyManagerRef.get().chooseEngineServerAlias(s, principals, sslEngine);
+ }
+
+ @Override
+ public String[] getClientAliases(String s, Principal[] principals) {
+ return keyManagerRef.get().getClientAliases(s, principals);
+ }
+
+ @Override
+ public String chooseClientAlias(String[] strings, Principal[] principals, Socket socket) {
+ return keyManagerRef.get().chooseClientAlias(strings, principals, socket);
+ }
+
+ @Override
+ public String[] getServerAliases(String s, Principal[] principals) {
+ return keyManagerRef.get().getServerAliases(s, principals);
+ }
+
+ @Override
+ public String chooseServerAlias(String s, Principal[] principals, Socket socket) {
+ return keyManagerRef.get().chooseServerAlias(s, principals, socket);
+ }
+
+ @Override
+ public X509Certificate[] getCertificateChain(String s) {
+ return keyManagerRef.get().getCertificateChain(s);
+ }
+
+ @Override
+ public PrivateKey getPrivateKey(String s) {
+ return keyManagerRef.get().getPrivateKey(s);
+ }
+
+ public ReloadingX509KeystoreManager loadFrom(Path path) {
+ try {
+ this.keyManagerRef.set(loadKeyManager(path));
+ } catch (Exception ex) {
+ // The Consumer.accept interface forces us to convert to unchecked
+ throw new RuntimeException(ex);
Review comment:
It gets caught by the FileMonitoringTimerTask and from there it's logged. It won't cause any damage, but it will keep logging the error on every reload interval if it's not resolved, potentially filling the logs and disk space.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack merged pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack merged pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538855054
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
##########
@@ -0,0 +1,88 @@
+/**
+ * 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.security.ssl;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.file.Path;
+import java.util.TimerTask;
+import java.util.function.Consumer;
+
+/**
+ * <p>
+ * Implements basic logic to track when a file changes on disk 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.
+ * </p>
+ */
+@InterfaceAudience.Private
+public class FileMonitoringTimerTask extends TimerTask {
+
+ static final Logger LOG = LoggerFactory.getLogger(FileMonitoringTimerTask.class);
+
+ @VisibleForTesting
+ static final String PROCESS_ERROR_MESSAGE =
+ "Could not process file change : ";
+
+ final private Path filePath;
+ final private Consumer<Path> onFileChange;
+ final Consumer<Throwable> onChangeFailure;
+ private long lastProcessed;
+
+ /**
+ * Create file monitoring task to be scheduled using a standard Java {@link java.util.Timer}
+ * instance.
+ *
+ * @param filePath The path to the file to monitor.
+ * @param onFileChange The function to call when the file has changed.
+ * @param onChangeFailure The function to call when an exception is thrown during the
+ * file change processing.
+ */
+ public FileMonitoringTimerTask(Path filePath,
+ Consumer<Path> onFileChange,
+ Consumer<Throwable> onChangeFailure) {
+ Preconditions.checkNotNull(filePath, "path to monitor disk file is not set");
+ Preconditions.checkNotNull(onFileChange, "action to monitor disk file is not set");
+
+ this.filePath = filePath;
+ this.lastProcessed = filePath.toFile().lastModified();
+ this.onFileChange = onFileChange;
+ this.onChangeFailure = onChangeFailure;
+ }
+
+ @Override
+ public void run() {
+ if (lastProcessed != filePath.toFile().lastModified()) {
+ try {
+ onFileChange.accept(filePath);
+ } catch (Throwable t) {
+ if (onChangeFailure != null) {
+ onChangeFailure.accept(t);
+ } else {
+ LOG.error(PROCESS_ERROR_MESSAGE + filePath.toString(), t);
+ }
+ }
+ lastProcessed = filePath.toFile().lastModified();
+ }
+ }
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-756653114
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 23s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | | 0m 0s | [test4tests](test4tests) | The patch appears to include 2 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 36m 23s | | trunk passed |
| +1 :green_heart: | compile | 21m 56s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | compile | 18m 21s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | checkstyle | 0m 48s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 30s | | trunk passed |
| +1 :green_heart: | shadedclient | 19m 30s | | branch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 0m 59s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 33s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +0 :ok: | spotbugs | 2m 20s | | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 19s | | trunk passed |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 56s | | the patch passed |
| +1 :green_heart: | compile | 25m 46s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javac | 25m 46s | | the patch passed |
| +1 :green_heart: | compile | 22m 7s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | javac | 22m 7s | | the patch passed |
| -0 :warning: | checkstyle | 0m 49s | [/diff-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 167 new + 68 unchanged - 11 fixed = 235 total (was 79) |
| +1 :green_heart: | mvnsite | 1m 32s | | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | | The patch has no whitespace issues. |
| +1 :green_heart: | shadedclient | 17m 54s | | patch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 1m 0s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 32s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | findbugs | 2m 30s | | the patch passed |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 10m 19s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 50s | | The patch does not generate ASF License warnings. |
| | | 191m 58s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/2470 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle |
| uname | Linux 75ee856a1074 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 1b1791075a8 |
| Default Java | Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/testReport/ |
| Max. process+thread count | 2328 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/console |
| versions | git=2.17.1 maven=3.6.0 findbugs=4.0.6 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-742992123
> LGTM.
>
> Have you tried it? If so, want to dump in here what you've done?
>
> The original patch posted did not cover DN (See JIRA). This works for DN and NN?
>
> Thanks.
Yes, both DN and NN are covered and this was integration tested (manually) in a pre-prod environment on all nodes. DevOps team happy with it.
Thanks so much for the review!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r535747204
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
+ }
+
+ trustManager = new ReloadingX509TrustManager(
+ truststoreType,
+ truststoreLocation,
+ truststorePassword);
Review comment:
A line per parameter is not how the background file does it?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538851554
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
+ truststorePassword = null;
+ }
+
+ // Check if obsolete truststore specific reload interval is present for backward compatible
+ long truststoreReloadInterval =
+ conf.getLong(
+ resolvePropertyName(mode, SSL_TRUSTSTORE_RELOAD_INTERVAL_TPL_KEY),
+ storesReloadInterval);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation);
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538856621
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
Review comment:
Fixed.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
##########
@@ -0,0 +1,151 @@
+/**
+ * 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.security.ssl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.*;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * An implementation of <code>X509KeyManager</code> that exposes a method,
+ * {@link #loadFrom(Path)} 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.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+public class ReloadingX509KeystoreManager extends X509ExtendedKeyManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReloadingX509TrustManager.class);
+
+ static final String RELOAD_ERROR_MESSAGE =
+ "Could not load keystore (keep using existing one) : ";
+
+ private String type;
+ private String storePassword;
+ private String keyPassword;
+ private AtomicReference<X509ExtendedKeyManager> keyManagerRef;
+
+ /**
+ * Construct a <code>Reloading509KeystoreManager</code>
+ *
+ * @param type type of keystore file, typically 'jks'.
+ * @param location local path to the keystore file.
+ * @param storePassword password of the keystore file.
+ * @param keyPassword The password of the key.
+ * @throws IOException
+ * @throws GeneralSecurityException
+ */
+ public ReloadingX509KeystoreManager(String type, String location,
+ String storePassword, String keyPassword)
+ throws IOException, GeneralSecurityException {
+ this.type = type;
+ this.storePassword = storePassword;
+ this.keyPassword = keyPassword;
+ keyManagerRef = new AtomicReference<X509ExtendedKeyManager>();
+ keyManagerRef.set(loadKeyManager(Paths.get(location)));
+ }
+
+ @Override
+ public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) {
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538849072
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -569,12 +577,45 @@ private ServerConnector createHttpsChannelConnector(
}
setEnabledProtocols(sslContextFactory);
+
+ long storesReloadInterval =
+ conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY,
+ FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL);
+
+ this.configurationChangeMonitor = storesReloadInterval > 0
+ ? Optional.of(this.makeConfigurationChangeMonitor(storesReloadInterval, sslContextFactory))
+ : Optional.empty();
+
conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
HttpVersion.HTTP_1_1.asString()));
return conn;
}
+ private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval,
+ SslContextFactory.Server sslContextFactory) {
+ java.util.Timer timer = new java.util.Timer("SSL Certificates Store Monitor", true);
Review comment:
Fixed.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -29,20 +29,20 @@
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.TrustManager;
import java.io.IOException;
-import java.io.InputStream;
-import java.nio.file.Files;
import java.nio.file.Paths;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
import java.text.MessageFormat;
+import java.util.Timer;
/**
* {@link KeyStoresFactory} implementation that reads the certificates from
* keystore files.
* <p>
- * if the trust certificates keystore file changes, the {@link TrustManager}
- * is refreshed with the new trust certificate entries (using a
- * {@link ReloadingX509TrustManager} trustmanager).
+ * If either the truststore or the keystore certificates file changes, it would be refreshed
Review comment:
Fixed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538854372
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
##########
@@ -0,0 +1,88 @@
+/**
+ * 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.security.ssl;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.file.Path;
+import java.util.TimerTask;
+import java.util.function.Consumer;
+
+/**
+ * <p>
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] saintstack merged pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
saintstack merged pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] bolerio commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
bolerio commented on a change in pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#discussion_r538851004
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
##########
@@ -77,14 +84,118 @@
public static final String DEFAULT_KEYSTORE_TYPE = "jks";
/**
- * Reload interval in milliseconds.
+ * The default time interval in milliseconds used to check if either
+ * of the truststore or keystore certificates file has changed and needs reloading.
*/
- public static final int DEFAULT_SSL_TRUSTSTORE_RELOAD_INTERVAL = 10000;
+ public static final int DEFAULT_SSL_STORES_RELOAD_INTERVAL = 10000;
private Configuration conf;
private KeyManager[] keyManagers;
private TrustManager[] trustManagers;
private ReloadingX509TrustManager trustManager;
+ private Timer fileMonitoringTimer;
+
+
+ private void createTrustManagersFromConfiguration(SSLFactory.Mode mode,
+ String truststoreType,
+ String truststoreLocation,
+ long storesReloadInterval)
+ throws IOException, GeneralSecurityException {
+ String passwordProperty = resolvePropertyName(mode,
+ SSL_TRUSTSTORE_PASSWORD_TPL_KEY);
+ String truststorePassword = getPassword(conf, passwordProperty, "");
+ if (truststorePassword.isEmpty()) {
+ // An empty trust store password is legal; the trust store password
+ // is only required when writing to a trust store. Otherwise it's
+ // an optional integrity check.
Review comment:
No, it won't, it will return the default passed as parameter if nothing in config, and that is "".
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-729504733
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 25s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +1 :green_heart: | @author | 0m 1s | | The patch does not contain any @author tags. |
| +1 :green_heart: | | 0m 0s | [test4tests](test4tests) | The patch appears to include 2 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 36m 56s | | trunk passed |
| +1 :green_heart: | compile | 21m 45s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | compile | 18m 27s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | checkstyle | 0m 49s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 29s | | trunk passed |
| +1 :green_heart: | shadedclient | 19m 56s | | branch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 1m 28s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 33s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +0 :ok: | spotbugs | 2m 22s | | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 20s | | trunk passed |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 56s | | the patch passed |
| +1 :green_heart: | compile | 21m 14s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javac | 21m 14s | | the patch passed |
| +1 :green_heart: | compile | 18m 25s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | javac | 18m 25s | | the patch passed |
| -0 :warning: | checkstyle | 0m 49s | [/diff-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/1/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 270 new + 68 unchanged - 11 fixed = 338 total (was 79) |
| +1 :green_heart: | mvnsite | 1m 26s | | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | | The patch has no whitespace issues. |
| +1 :green_heart: | shadedclient | 17m 15s | | patch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 0m 57s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 28s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | findbugs | 2m 29s | | the patch passed |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 10m 7s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 47s | | The patch does not generate ASF License warnings. |
| | | 185m 34s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/2470 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle |
| uname | Linux c8a7c4d33c05 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / a7b923c80c6 |
| Default Java | Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/1/testReport/ |
| Max. process+thread count | 1879 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/1/console |
| versions | git=2.17.1 maven=3.6.0 findbugs=4.0.6 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-756653114
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 23s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | | 0m 0s | [test4tests](test4tests) | The patch appears to include 2 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 36m 23s | | trunk passed |
| +1 :green_heart: | compile | 21m 56s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | compile | 18m 21s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | checkstyle | 0m 48s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 30s | | trunk passed |
| +1 :green_heart: | shadedclient | 19m 30s | | branch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 0m 59s | | trunk passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 33s | | trunk passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +0 :ok: | spotbugs | 2m 20s | | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 19s | | trunk passed |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 56s | | the patch passed |
| +1 :green_heart: | compile | 25m 46s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javac | 25m 46s | | the patch passed |
| +1 :green_heart: | compile | 22m 7s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | javac | 22m 7s | | the patch passed |
| -0 :warning: | checkstyle | 0m 49s | [/diff-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 167 new + 68 unchanged - 11 fixed = 235 total (was 79) |
| +1 :green_heart: | mvnsite | 1m 32s | | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | | The patch has no whitespace issues. |
| +1 :green_heart: | shadedclient | 17m 54s | | patch has no errors when building and testing our client artifacts. |
| +1 :green_heart: | javadoc | 1m 0s | | the patch passed with JDK Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 |
| +1 :green_heart: | javadoc | 1m 32s | | the patch passed with JDK Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| +1 :green_heart: | findbugs | 2m 30s | | the patch passed |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 10m 19s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 50s | | The patch does not generate ASF License warnings. |
| | | 191m 58s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/2470 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle |
| uname | Linux 75ee856a1074 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 1b1791075a8 |
| Default Java | Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/testReport/ |
| Max. process+thread count | 2328 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/4/console |
| versions | git=2.17.1 maven=3.6.0 findbugs=4.0.6 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #2470:
URL: https://github.com/apache/hadoop/pull/2470#issuecomment-741496305
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 0s | | Docker mode activated. |
| -1 :x: | docker | 5m 36s | | Docker failed to build yetus/hadoop:4b312810ae0. |
| Subsystem | Report/Notes |
|----------:|:-------------|
| GITHUB PR | https://github.com/apache/hadoop/pull/2470 |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2470/2/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org