exceptionfactory commented on a change in pull request #4991:
URL: https://github.com/apache/nifi/pull/4991#discussion_r622242626
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-server-nar/pom.xml
##########
@@ -37,6 +37,11 @@
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-jetty</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-framework-ssl-autoloading-utils</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </dependency>
Review comment:
Should this be removed?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/util/TrustStoreScanner.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.nifi.web.server.util;
+
+import org.eclipse.jetty.util.Scanner;
+import org.eclipse.jetty.util.annotation.ManagedAttribute;
+import org.eclipse.jetty.util.annotation.ManagedOperation;
+import org.eclipse.jetty.util.component.ContainerLifeCycle;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.resource.Resource;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Collections;
+
+/**
+ * <p>The {@link TrustStoreScanner} is used to monitor the TrustStore file
used by the {@link SslContextFactory}.
+ * It will reload the {@link SslContextFactory} if it detects that the
TrustStore file has been modified.</p>
+ * <p>
+ * Though it would have been more ideal to simply extend KeyStoreScanner and
override the keystore resource
+ * with the truststore resource, KeyStoreScanner's constructor was written in
a way that doesn't make this possible.
+ */
+public class TrustStoreScanner extends ContainerLifeCycle implements
Scanner.DiscreteListener {
Review comment:
Understanding that this essentially follows the implementation of
`KeyStoreScanner`, what do you think about adding a unit test for this class?
##########
File path:
nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
##########
@@ -835,7 +835,7 @@ class ConfigEncryptionTool {
tempFlowXmlWriter.close()
// Overwrite the original flow file with the migrated flow file
- Files.move(tempFlowXmlFile.toPath(), Paths.get(outputFlowXmlPath),
StandardCopyOption.ATOMIC_MOVE)
+ Files.move(tempFlowXmlFile.toPath(), Paths.get(outputFlowXmlPath),
StandardCopyOption.REPLACE_EXISTING)
Review comment:
Can this change be reverted?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/nifi.properties
##########
@@ -178,6 +178,8 @@
nifi.sensitive.props.algorithm=${nifi.sensitive.props.algorithm}
nifi.sensitive.props.provider=${nifi.sensitive.props.provider}
nifi.sensitive.props.additional.keys=${nifi.sensitive.props.additional.keys}
+nifi.security.autorefresh.enabled=${nifi.security.autorefresh.enabled}
+nifi.security.autorefresh.interval=${nifi.security.autorefresh.interval}
Review comment:
Should these properties be named autoreload or just reload to follow the
method semantics of the Jetty implementation? I realize this will involve
changes in multiple places.
```suggestion
nifi.security.reload.enabled=${nifi.security.autorefresh.enabled}
nifi.security.reload.interval=${nifi.security.autorefresh.interval}
```
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -199,6 +199,26 @@ Now that the User Interface has been secured, we can
easily secure Site-to-Site
accomplished by setting the `nifi.remote.input.secure` and
`nifi.cluster.protocol.is.secure` properties, respectively, to `true`. These
communications
will always REQUIRE two way SSL as the nodes will use their configured
keystore/truststore for authentication.
+Automatic refreshing of NiFi's web SSL context factory can be enabled using
the following properties:
+
+[options="header,footer"]
+|==================================================================================================================================================
+| Property Name | Description
+|`nifi.security.autorefresh.enabled`|Specifies whether the SSL context factory
should be automatically refreshed if updates to the keystore and truststore are
detected. By default, it is set to `false`.
+|`nifi.security.autorefresh.interval`|Specifies the interval at which the
keystore and truststore are checked for updates. Only applies if
`nifi.security.autorefresh.enabled` is set to `true`. The default value is `10
secs`.
+|==================================================================================================================================================
+
+Once the `nifi.security.autorefresh.enabled` property is set to `true`, any
valid changes to the configured keystore and truststore will cause NiFi's SSL
context factory to be reloaded, allowing clients to pick up the changes. This
is intended to allow expired certificates to be updated in the keystore and new
trusted certificates to be added in the truststore, all without having to
restart the NiFi server.
+
+NOTE: Changes to any of the `nifi.security.keystore*` or
`nifi.security.truststore*` properties will not be picked up by the
auto-refreshing logic, which assumes the passwords and store paths will remain
the same.
+
+There are no restrictions on updates to certificates in the trust store, but
for security reasons, changes to the keystore are considered "valid" only if:
+
+* No entries are added to the keystore
+* There are no changes to the aliases of existing entries
+* There are no changes to the subject principal of existing entries
+* There are no changes to the issuer principal or serial number of existing
entries
Review comment:
With the change to use Jetty's `KeyStoreScanner`, these lines should be
removed.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -880,6 +884,29 @@ private void configureHttpsConnector(Server server,
HttpConfiguration httpConfig
ServerConnectorCreator<Server, HttpConfiguration, ServerConnector> scc
= (s, c) -> createUnconfiguredSslServerConnector(s, c, port);
configureGenericConnector(server, httpConfiguration, hostname, port,
connectorLabel, httpsNetworkInterfaces, scc);
+
+ if (props.isSecurityAutoRefreshEnabled()) {
+ configureSslContextFactoryReloading(server);
+ }
+ }
+
+ /**
+ * Configures a KeyStoreScanner and TrustStoreScanner at the configured
refresh intervals. This will
+ * reload the SSLContextFactory if any changes are detected to the
keystore or truststore.
+ * @param server The Jetty server
+ */
+ private void configureSslContextFactoryReloading(Server server) {
+ final int scanIntervalSeconds =
Double.valueOf(FormatUtils.getPreciseTimeDuration(
+ props.getSecurityAutoRefreshInterval(), TimeUnit.SECONDS))
+ .intValue();
+
+ KeyStoreScanner keyStoreScanner = new
KeyStoreScanner(sslContextFactory);
Review comment:
This and `trustStoreScanner` could be marked as `final`.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -150,6 +152,7 @@
private ExtensionMapping extensionMapping;
private NarAutoLoader narAutoLoader;
private DiagnosticsFactory diagnosticsFactory;
+ private SslContextFactory sslContextFactory;
Review comment:
Should the type be `SslContextFactory.Server`?
```suggestion
private SslContextFactory.Server sslContextFactory;
```
--
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]
