exceptionfactory commented on a change in pull request #4991: URL: https://github.com/apache/nifi/pull/4991#discussion_r612878968
########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-ssl-autoloading-utils/src/main/java/org/apache/nifi/autoload/PrivateKeyInfo.java ########## @@ -0,0 +1,79 @@ +/* + * 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.autoload; + +import java.util.Objects; + +public class PrivateKeyInfo implements Comparable<PrivateKeyInfo> { Review comment: The properties of this class are associated with the certificate as opposed to the private key, so what do you think about changing the name to something like `CertificateEntryInfo` or `CertificateEntryDescription`? ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-ssl-autoloading-utils/src/test/java/org/apache/nifi/autoload/TestSSLContextFactoryAutoLoaderTask.java ########## @@ -0,0 +1,113 @@ +/* + * 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.autoload; + +import org.apache.nifi.security.util.KeyStoreUtils; +import org.apache.nifi.security.util.StandardTlsConfiguration; +import org.apache.nifi.security.util.TlsConfiguration; +import org.apache.nifi.security.util.TlsException; +import org.apache.nifi.util.NiFiProperties; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.nio.file.WatchService; +import java.security.GeneralSecurityException; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.UnrecoverableEntryException; + +public class TestSSLContextFactoryAutoLoaderTask { + + private static final Path KEYSTORE = new File("src/test/resources/keystore.jks").toPath(); + private static final String KEYSTORE_PASSWORD = "testtesttest"; + private static final String KEY_PASSWORD = "testtesttest"; + private static final String KEYSTORE_TYPE = "JKS"; + + private static final String DEFAULT_KEY_ALIAS = "nifi-key"; + private static final String DEFAULT_CERT_DN = "CN=localhost, OU=NIFI"; + + private SSLContextFactoryAutoLoaderTask task; + private NiFiProperties nifiProperties; + + @Before + public void init() throws GeneralSecurityException, IOException { + this.createKeystore(DEFAULT_KEY_ALIAS, DEFAULT_CERT_DN); + + nifiProperties = Mockito.mock(NiFiProperties.class); + Mockito.when(nifiProperties.getProperty(NiFiProperties.SECURITY_KEYSTORE_PASSWD)).thenReturn(KEYSTORE_PASSWORD); + Mockito.when(nifiProperties.getProperty(NiFiProperties.SECURITY_KEY_PASSWD)).thenReturn(KEY_PASSWORD); + Mockito.when(nifiProperties.getProperty(NiFiProperties.SECURITY_KEYSTORE_TYPE)).thenReturn(KEYSTORE_TYPE); + + task = new SSLContextFactoryAutoLoaderTask.Builder() + .keystorePath(KEYSTORE) + .keystoreWatchService(Mockito.mock(WatchService.class)) + .truststorePath(Mockito.mock(Path.class)) + .autoLoader(Mockito.mock(SSLContextFactoryAutoLoader.class)) + .nifiProperties(nifiProperties) + .pollIntervalMillis(100) + .build(); + } + + @Test + public void testIsReloadAllowed_true() throws IOException, GeneralSecurityException { + this.createKeystore(DEFAULT_KEY_ALIAS, DEFAULT_CERT_DN); // Creates a keystore with the same DNs and alias, so this is allowed + Assert.assertTrue(task.isReloadAllowed()); + } + + @Test + public void testIsReloadAllowed_differentAlias() throws IOException, GeneralSecurityException { + this.createKeystore("different-alias", DEFAULT_CERT_DN); + Assert.assertFalse(task.isReloadAllowed()); + } + + @Test + public void testIsReloadAllowed_differentSubjectDN() throws IOException, GeneralSecurityException { + this.createKeystore(DEFAULT_KEY_ALIAS, "CN=different"); + Assert.assertFalse(task.isReloadAllowed()); + } + + @Test + public void testIsReloadAllowed_differentIssuerSerialNumber() throws IOException, UnrecoverableEntryException, NoSuchAlgorithmException, KeyStoreException, TlsException { + // This is a keystore with the same key alias and cert DN, but with a different issuer cert in the cert chain, + // showing that we disallow cert updates with the same issuer DN but different actual issuer serial number + Files.copy(new File("src/test/resources/test-keystore.jks").toPath(), KEYSTORE, StandardCopyOption.REPLACE_EXISTING); + + Assert.assertFalse(task.isReloadAllowed()); + } + + private void createKeystore(String keyAlias, String certDn) throws IOException, GeneralSecurityException { + TlsConfiguration tlsConfiguration = KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore(new StandardTlsConfiguration( + null, KEYSTORE_PASSWORD, KEY_PASSWORD, KEYSTORE_TYPE, + null, KEYSTORE_PASSWORD, KEYSTORE_TYPE), keyAlias, "nifi-cert", certDn); + Files.move(new File(tlsConfiguration.getKeystorePath()).toPath(), KEYSTORE, StandardCopyOption.ATOMIC_MOVE); Review comment: According to the automated build, Windows does not support the `StandardCopyOption.ATOMIC_MOVE`, so it looks like replacing this with `StandardCopyOption.REPLACE_EXISTING` should work. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-ssl-autoloading-utils/src/main/java/org/apache/nifi/autoload/PrivateKeyInfo.java ########## @@ -0,0 +1,79 @@ +/* + * 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.autoload; + +import java.util.Objects; + +public class PrivateKeyInfo implements Comparable<PrivateKeyInfo> { + private static final String COMPARISON_FORMAT = "%s-%s-%s-%s"; + + /** + * Creates a PrivateKeyInfo containing the state we want to track. + * @param subjectDN The subject DN of the cert + * @param issuerDN The issuer DN + * @param alias The alias in the keystore + * @param issuerSerialNumber Optional serial number of the issuer, if the issuer's certificate was found + * in the cert chain + */ + public PrivateKeyInfo(String subjectDN, String issuerDN, String alias, String issuerSerialNumber) { + this.subjectDN = subjectDN; + this.issuerDN = issuerDN; + this.alias = alias; + this.issuerSerialNumber = issuerSerialNumber; + } + + private String subjectDN; + private String issuerDN; Review comment: Is there a reason for using `String` as opposed to `X500Principal` for these properties? Using the `X500Principal` class follows the property types from `X509Certificate` and preserves the original binary form as opposed to the string representation. Also recommend renaming them to `subjectPrincipal` and `issuerPrincipal` if the type is changed. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-ssl-autoloading-utils/src/main/java/org/apache/nifi/autoload/SSLContextFactoryAutoLoaderTask.java ########## @@ -0,0 +1,270 @@ +/* + * 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.autoload; + +import org.apache.nifi.security.util.KeyStoreUtils; +import org.apache.nifi.security.util.TlsException; +import org.apache.nifi.util.NiFiProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Path; +import java.nio.file.StandardWatchEventKinds; +import java.nio.file.WatchEvent; +import java.nio.file.WatchKey; +import java.nio.file.WatchService; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.UnrecoverableEntryException; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.TimeUnit; + +/** + * The runnable task that polls the WatchService for updates to the keystore and truststore. + * + */ +public class SSLContextFactoryAutoLoaderTask implements Runnable { + + private static final Logger LOGGER = LoggerFactory.getLogger(SSLContextFactoryAutoLoaderTask.class); + + private static final int MIN_FILE_AGE = 5000; + + private final Path keystorePath; + private final Path truststorePath; + private final WatchService keystoreWatchService; + private final WatchService truststoreWatchService; + private final long pollIntervalMillis; + private final SSLContextFactoryAutoLoader autoLoader; + private final NiFiProperties nifiProperties; + private final List<File> candidateStores; + + private final List<PrivateKeyInfo> existingKeystoreState; + + private volatile boolean stopped = false; + + private SSLContextFactoryAutoLoaderTask(final Builder builder) throws NoSuchAlgorithmException, UnrecoverableEntryException, + KeyStoreException, TlsException { + this.keystorePath = builder.keystorePath; + this.truststorePath = builder.truststorePath; + this.keystoreWatchService = builder.keystoreWatchService; + this.truststoreWatchService = builder.truststoreWatchService; + this.pollIntervalMillis = builder.pollIntervalMillis; + this.autoLoader = builder.autoLoader; + this.nifiProperties = builder.niFiProperties; + this.existingKeystoreState = this.getKeystoreState(); + this.candidateStores = new ArrayList<>(); + } + + private boolean poll(WatchService watchService, Collection<Path> storePaths) { + if (storePaths == null || storePaths.isEmpty()) { + throw new RuntimeException("A polling directory must be specified."); + } + WatchKey key; + try { + key = watchService.poll(pollIntervalMillis, TimeUnit.MILLISECONDS); + } catch (InterruptedException x) { + LOGGER.info("WatchService interrupted, returning..."); + return false; + } + + boolean storeChanged = false; + + // Key comes back as null when there are no new create events, but we still want to continue processing + // so we can consider files added to the candidateNars list in previous iterations + + if (key != null) { + for (WatchEvent<?> event : key.pollEvents()) { + final WatchEvent.Kind<?> kind = event.kind(); + if (kind == StandardWatchEventKinds.OVERFLOW) { + continue; + } + + final WatchEvent<Path> ev = (WatchEvent<Path>) event; + final Path filename = ev.context(); + + for(Path storePath : storePaths) { + + final Path autoLoadFile = storePath.getParent().resolve(filename); + final String autoLoadFilename = autoLoadFile.toFile().getName(); + + if (!storePath.getFileName().toString().equals(autoLoadFilename)) { + continue; + } + + LOGGER.info("Found update to {}", new Object[]{autoLoadFilename}); + storeChanged = true; + } + } + + final boolean valid = key.reset(); + if (!valid) { + LOGGER.error("{} auto-refresh directory is no longer valid", new Object[] {storePaths.iterator().next()}); + stop(); + } + return storeChanged; + } + return false; + } + + @Override + public void run() { + Set<Path> bothPaths = new HashSet<>(Arrays.asList(keystorePath, truststorePath)); + while (!stopped) { + try { + boolean storeChanged = false; + // Can we poll the same directory for updates? + if (keystoreWatchService == truststoreWatchService) { + LOGGER.debug("Polling for keystore updates at {} and truststore updates at {}", new Object[]{keystorePath, truststorePath}); + storeChanged = this.poll(keystoreWatchService, bothPaths); + } else { + // Otherwise, poll separate directories + LOGGER.debug("Polling for keystore updates at {}", new Object[]{keystorePath}); + storeChanged = this.poll(keystoreWatchService, Arrays.asList(keystorePath)); + + LOGGER.debug("Polling for truststore updates at {}", new Object[]{truststorePath}); + storeChanged |= this.poll(truststoreWatchService, Arrays.asList(truststorePath)); + } + + if (storeChanged) { + if (this.isReloadAllowed()) { + autoLoader.getSslContextFactoryReloadable().reloadSSLContextFactory(); + } else { + LOGGER.warn("For security reasons, the SSL Context Factory could not be reloaded because the " + + "keystore {} changed in a way that is disallowed.", new Object[] {keystorePath}); + } + } + + } catch (final Throwable t) { + LOGGER.error("Error reloading SSL context factory due to: " + t.getMessage(), t); + } + } + } + + /** + * Returns a list representing the state of the current keystore at the given path. This method uses the + * keystore properties from nifi.properties. The only state retrieved will be the alias, subject DN, + * issuer DN of each PrivateKeyEntry, and issuer certificate serial number if applicable, and the results + * will be a sorted list. + * @return A sorted list of information about each private key in the keystore + * @throws TlsException If the keystore could not be loaded + * @throws KeyStoreException If the keystore password was incorrect + * @throws UnrecoverableEntryException If a private key entry could not be recovered + * @throws NoSuchAlgorithmException If the default password algorithm is not supported + */ + private List<PrivateKeyInfo> getKeystoreState() throws TlsException, KeyStoreException, + UnrecoverableEntryException, NoSuchAlgorithmException { + List<PrivateKeyInfo> state = new ArrayList<>(); + + KeyStore keyStore = KeyStoreUtils.loadKeyStore(keystorePath.toString(), + nifiProperties.getProperty(NiFiProperties.SECURITY_KEYSTORE_PASSWD).toCharArray(), + nifiProperties.getProperty(NiFiProperties.SECURITY_KEYSTORE_TYPE)); + + Enumeration<String> aliasesEnum = keyStore.aliases(); + while(aliasesEnum.hasMoreElements()) { + String alias = aliasesEnum.nextElement(); + if (keyStore.isKeyEntry(alias)) { + KeyStore.PrivateKeyEntry entry = (KeyStore.PrivateKeyEntry) keyStore.getEntry(alias, new KeyStore.PasswordProtection(nifiProperties + .getProperty(NiFiProperties.SECURITY_KEY_PASSWD).toCharArray())); + X509Certificate cert = (X509Certificate) entry.getCertificateChain()[0]; + String issuerSerialNumber = (entry.getCertificateChain().length > 1) + ? ((X509Certificate) entry.getCertificateChain()[1]).getSerialNumber().toString() + : null; + state.add(new PrivateKeyInfo(cert.getSubjectDN().getName(), cert.getIssuerDN().getName(), alias, issuerSerialNumber)); + } + } + + Collections.sort(state); + return new ArrayList<>(state); + } + + /** + * This returns false if there were any changes to the keystore other than updating a PrivateKeyEntry with + * the same subject DN, issuer DN, alias, and issuer cert serial number if applicable. + * @return True if a reload should be allowed, meaning the keystore has not changed in a meaningful way + */ + boolean isReloadAllowed() throws NoSuchAlgorithmException, UnrecoverableEntryException, KeyStoreException, TlsException { + return existingKeystoreState.equals(this.getKeystoreState()); + } + + public void stop() { + LOGGER.info("Stopping SSL Context Factory Auto-loader"); + stopped = true; + } Review comment: Rather than having a `Runnable` with its own lifecycle method, what about leveraging `ScheduledThreadPoolExecutor.scheduleAtFixedRate()` and managing the lifecycle through the returned `ScheduledFuture`? That would make it much easier to test this class without handling threading concerns. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java ########## @@ -311,6 +315,15 @@ private Handler loadInitialWars(final Set<Bundle> bundles) { return gzip(webAppContextHandlers); } + @Override + public void reloadSSLContextFactory() throws Exception { Review comment: Minor adjustment, recommend renaming to follow class naming for the Jetty `SslContextFactory`: ```suggestion public void reloadSslContextFactory() throws Exception { ``` -- 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]
