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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
