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/12/04 00:34:28 UTC
[GitHub] [hadoop] saintstack commented on a change in pull request #2470: HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode
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