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: us...@infra.apache.org