mtien-apache commented on a change in pull request #4767:
URL: https://github.com/apache/nifi/pull/4767#discussion_r561247262
##########
File path:
nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
##########
@@ -125,6 +146,63 @@ public static KeyStore loadKeyStore(String keystorePath,
char[] keystorePassword
}
}
+ /**
+ * Creates a temporary default Keystore and Truststore and returns it
wrapped in a TLS configuration.
+ *
+ * @return a {@link org.apache.nifi.security.util.TlsConfiguration}
+ */
+ public static TlsConfiguration createTlsConfigAndNewKeystoreTruststore()
throws IOException, GeneralSecurityException {
+ return createTlsConfigAndNewKeystoreTruststore(new
StandardTlsConfiguration());
+ }
+
+ /**
+ * Creates a temporary Keystore and Truststore and returns it wrapped in a
new TLS configuration with the given values.
+ *
+ * @param tlsConfiguration a {@link
org.apache.nifi.security.util.TlsConfiguration}
+ * @return a {@link org.apache.nifi.security.util.TlsConfiguration}
+ */
+ public static TlsConfiguration
createTlsConfigAndNewKeystoreTruststore(final TlsConfiguration
tlsConfiguration) throws IOException, GeneralSecurityException {
+ final Path keyStorePath;
+ final String keystorePassword =
StringUtils.isNotBlank(tlsConfiguration.getKeystorePassword()) ?
tlsConfiguration.getKeystorePassword() : generatePassword();
+ final String keyPassword =
StringUtils.isNotBlank(tlsConfiguration.getKeyPassword())?
tlsConfiguration.getKeyPassword() : keystorePassword;
+ final KeystoreType keystoreType = tlsConfiguration.getKeystoreType()
!= null ? tlsConfiguration.getKeystoreType() : KeystoreType.PKCS12;
+ final Path trustStorePath;
+ final String truststorePassword =
StringUtils.isNotBlank(tlsConfiguration.getTruststorePassword()) ?
tlsConfiguration.getTruststorePassword() : "";
+ final KeystoreType truststoreType =
tlsConfiguration.getTruststoreType() != null ?
tlsConfiguration.getTruststoreType() : KeystoreType.PKCS12;
+
+ // Create temporary Keystore file
+ try {
+ keyStorePath = generateTempKeystorePath(keystoreType);
+ } catch (IOException e) {
+ logger.error(KEYSTORE_ERROR_MSG);
+ throw new UncheckedIOException(KEYSTORE_ERROR_MSG, e);
+ }
+
+ // Create temporary Truststore file
+ try {
+ trustStorePath = generateTempTruststorePath(truststoreType);
+ } catch (IOException e) {
+ logger.error(TRUSTSTORE_ERROR_MSG);
+ throw new UncheckedIOException(TRUSTSTORE_ERROR_MSG, e);
+ }
+
+ // Create X509 Certificate
+ final X509Certificate clientCert =
createKeyStoreAndGetX509Certificate(KEY_ALIAS, keystorePassword, keyPassword,
keyStorePath.toString(), keystoreType);
+
+ // Create Truststore
+ createTrustStore(clientCert, CERT_ALIAS, truststorePassword,
trustStorePath.toString(), getKeystoreType(truststoreType.toString()));
+
+ return new StandardTlsConfiguration(
+ keyStorePath.toString(),
+ keystorePassword,
+ keyPassword,
+ getKeystoreType(keystoreType.toString()),
+ trustStorePath.toString(),
+ truststorePassword,
+ getKeystoreType(truststoreType.toString()),
Review comment:
The call is not necessary.
##########
File path:
nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
##########
@@ -245,7 +322,7 @@ public static TrustManagerFactory
loadTrustManagerFactory(TlsConfiguration tlsCo
*/
public static TrustManagerFactory loadTrustManagerFactory(String
truststorePath, String truststorePassword, String truststoreType) throws
TlsException {
// Legacy truststore passwords can be empty
- final char[] truststorePasswordChars =
StringUtils.isNotBlank(truststorePassword) ? truststorePassword.toCharArray() :
null;
+ final char[] truststorePasswordChars =
StringUtils.isNotBlank(truststorePassword) ? truststorePassword.toCharArray() :
"".toCharArray();
Review comment:
@exceptionfactory I received a Null Pointer Exception for an empty
password when the truststore type is PKCS12, so I changed it to an empty
string. But after some investigation, I found that the Bouncy Castle PKCS12
store type does not allow empty passwords.
Since we allow passwordless truststores, I'll add a check for the truststore
type. If it's PKCS12, then I'll throw an Illegal Argument Exception, otherwise
I'll set it back to `null`.
##########
File path:
nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
##########
@@ -245,7 +322,7 @@ public static TrustManagerFactory
loadTrustManagerFactory(TlsConfiguration tlsCo
*/
public static TrustManagerFactory loadTrustManagerFactory(String
truststorePath, String truststorePassword, String truststoreType) throws
TlsException {
// Legacy truststore passwords can be empty
- final char[] truststorePasswordChars =
StringUtils.isNotBlank(truststorePassword) ? truststorePassword.toCharArray() :
null;
+ final char[] truststorePasswordChars =
StringUtils.isNotBlank(truststorePassword) ? truststorePassword.toCharArray() :
"".toCharArray();
Review comment:
@exceptionfactory After making these changes, I found out the real issue
was that I received an NPE when loading a PKCS12 truststore and passing a
`null` password. For loading a JKS or BCFKS store type, a `null` password is
allowed.
After our discussion and finding out the behavior among keystore/truststore
types are different, we concluded to allow loading a store without a password,
but not persisting a store without a password. These changes affect the methods
I added to programmatically generate certificates, keystores, and truststores.
##########
File path:
nifi-commons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/util/KeyStoreUtilsGroovyTest.groovy
##########
@@ -141,4 +173,181 @@ class KeyStoreUtilsGroovyTest extends GroovyTestCase {
FileOutputStream fos = new
FileOutputStream("/Users/alopresto/Workspace/nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/truststore.no-password.jks")
truststore.store(fos, "".chars)
}
+
+ @Test
+ void testShouldValidateTempKeystorePath() {
+ // Act
+ Path testKeystorePath =
KeyStoreUtils.generateTempKeystorePath(JKS_STORE_TYPE)
+ deletePath(testKeystorePath)
+
+ // Assert
+ logger.info("Keystore path: ${testKeystorePath.toString()}")
+ assert testKeystorePath
+ }
+
+ @Test
+ void testShouldValidateTempTruststorePath() {
+ // Act
+ Path truststorePath =
KeyStoreUtils.generateTempTruststorePath(JKS_STORE_TYPE)
+ deletePath(truststorePath)
+
+ // Assert
+ logger.info("Truststore path: ${truststorePath.toString()}")
+ assert truststorePath
+ }
+
+ @Test
+ void testShouldValidateTlsConfigAndNewKeystoreTruststoreWithParams() {
+ // Arrange
+ TlsConfiguration tlsConfigParam = new StandardTlsConfiguration(null,
TEST_KEYSTORE_PASSWORD, TEST_KEY_PASSWORD, JKS_STORE_TYPE, null,
TEST_TRUSTSTORE_PASSWORD, JKS_STORE_TYPE)
+
+ // Act
+ final TlsConfiguration tlsConfig =
KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore(tlsConfigParam)
+ deleteKeystoreTruststore(tlsConfig)
+
+ // Assert
+ assert tlsConfig.getKeystorePath()
+ assert tlsConfig.getTruststorePath()
+ assert tlsConfig.getKeystoreType() == KeystoreType.JKS
+ assert tlsConfig.getTruststoreType() == KeystoreType.JKS
+ assert tlsConfig.getKeystorePassword() == TEST_KEYSTORE_PASSWORD
+ }
+
+ @Test
+ void testShouldValidateTlsConfigAndNewKeystoreTruststoreWithoutParams() {
+ // Act
+ TlsConfiguration tlsConfig =
KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore()
+ deleteKeystoreTruststore(tlsConfig)
+
+ // Assert
+ assert tlsConfig.getKeystorePath()
+ assert tlsConfig.getKeyPassword() == tlsConfig.getKeystorePassword()
+ assert tlsConfig.getKeystoreType() == KeystoreType.PKCS12
+ assert tlsConfig.getTruststoreType() == KeystoreType.PKCS12
+ }
+
+ @Test
+ void testShouldValidateTlsConfigWithoutKeyPasswordParam() {
+ // Arrange
+ TlsConfiguration tlsConfigParam = new StandardTlsConfiguration(null,
TEST_KEYSTORE_PASSWORD, null, JKS_STORE_TYPE, null, TEST_TRUSTSTORE_PASSWORD,
JKS_STORE_TYPE)
+
+ // Act
+ final TlsConfiguration tlsConfig =
KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore(tlsConfigParam)
+ deleteKeystoreTruststore(tlsConfig)
+
+ // Assert
+ assert tlsConfig.getKeyPassword() == tlsConfig.getKeystorePassword()
+ }
+
+ @Test
+ void testShouldReturnX509CertWithNewKeystore() {
+ // Arrange
+ Path keystorePath =
KeyStoreUtils.generateTempKeystorePath(JKS_STORE_TYPE)
+
+ // Act
+ X509Certificate x509Cert =
KeyStoreUtils.createKeyStoreAndGetX509Certificate(KEY_ALIAS,
TEST_KEYSTORE_PASSWORD, TEST_KEYSTORE_PASSWORD, keystorePath.toString(),
JKS_STORE_TYPE)
+
+ boolean isKeystoreValid =
KeyStoreUtils.isStoreValid(keystorePath.toUri().toURL(), JKS_STORE_TYPE,
TEST_KEYSTORE_PASSWORD.toCharArray())
+ deletePath(keystorePath)
+
+ // Assert
+ final String certDN = x509Cert.getIssuerDN().toString()
+ logger.info("Certificate DN: ${certDN}")
+ assert certDN == "CN=localhost,OU=NiFi,O=Apache"
Review comment:
I changed the check to assert the string contains `CN=`.
----------------------------------------------------------------
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
