You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/02/02 20:06:41 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
exceptionfactory commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r568883952
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
##########
@@ -46,8 +58,24 @@
private static final Logger logger = LoggerFactory.getLogger(KeyStoreUtils.class);
public static final String SUN_PROVIDER_NAME = "SUN";
+ private static final String JKS_EXT = ".jks";
+ private static final String PKCS12_EXT = ".p12";
+ private static final String BCFKS_EXT = ".bcfks";
+ private static final String KEY_ALIAS = "nifi-key";
+ private static final String CERT_ALIAS = "nifi-cert";
+ private static final String CERT_DN = "CN=localhost,O=Apache,OU=NiFi";
Review comment:
Standard ordering of Relative Distinguished Names places `OU` before `O`. For simplicity, however, just using the Common Name should be sufficient for testing:
```suggestion
private static final String CERT_DN = "CN=localhost";
```
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenHTTP.java
##########
@@ -147,29 +126,84 @@
private int availablePort;
@BeforeClass
- public static void setUpSuite() throws TlsException {
- serverKeyStoreSslContext = SslContextFactory.createSslContext(SERVER_CONFIGURATION);
- final TrustManager[] defaultTrustManagers = SslContextFactory.getTrustManagers(SERVER_NO_TRUSTSTORE_CONFIGURATION);
- serverKeyStoreNoTrustStoreSslContext = SslContextFactory.createSslContext(SERVER_NO_TRUSTSTORE_CONFIGURATION, defaultTrustManagers);
+ public static void setUpSuite() throws GeneralSecurityException, IOException {
+ // generate new keystore and truststore
+ tlsConfiguration = KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore();
+
+ serverConfiguration = new StandardTlsConfiguration(
+ tlsConfiguration.getKeystorePath(),
+ tlsConfiguration.getKeystorePassword(),
+ tlsConfiguration.getKeyPassword(),
+ tlsConfiguration.getKeystoreType(),
+ tlsConfiguration.getTruststorePath(),
+ tlsConfiguration.getTruststorePassword(),
+ tlsConfiguration.getTruststoreType(),
+ TLS_1_2
+ );
+ serverTls_1_3_Configuration = new StandardTlsConfiguration(
+ tlsConfiguration.getKeystorePath(),
+ tlsConfiguration.getKeystorePassword(),
+ tlsConfiguration.getKeyPassword(),
+ tlsConfiguration.getKeystoreType(),
+ tlsConfiguration.getTruststorePath(),
+ tlsConfiguration.getTruststorePassword(),
+ tlsConfiguration.getTruststoreType(),
+ TLS_1_3
+ );
+ serverNoTruststoreConfiguration = new StandardTlsConfiguration(
+ tlsConfiguration.getKeystorePath(),
+ tlsConfiguration.getKeystorePassword(),
+ tlsConfiguration.getKeyPassword(),
+ tlsConfiguration.getKeystoreType(),
+ null,
+ null,
+ null,
+ TLS_1_2
+ );
+
+ serverKeyStoreSslContext = SslContextFactory.createSslContext(serverConfiguration);
+ final TrustManager[] defaultTrustManagers = SslContextFactory.getTrustManagers(serverNoTruststoreConfiguration);
+ serverKeyStoreNoTrustStoreSslContext = SslContextFactory.createSslContext(serverNoTruststoreConfiguration, defaultTrustManagers);
keyStoreSslContext = SslContextFactory.createSslContext(new StandardTlsConfiguration(
- CLIENT_KEYSTORE,
- KEYSTORE_PASSWORD,
- CLIENT_KEYSTORE_TYPE,
- TRUSTSTORE,
- TRUSTSTORE_PASSWORD,
- TRUSTSTORE_TYPE)
+ tlsConfiguration.getKeystorePath(),
+ tlsConfiguration.getKeystorePassword(),
+ tlsConfiguration.getKeystoreType(),
+ tlsConfiguration.getTruststorePath(),
+ tlsConfiguration.getTruststorePassword(),
+ tlsConfiguration.getTruststoreType())
);
trustStoreSslContext = SslContextFactory.createSslContext(new StandardTlsConfiguration(
null,
null,
null,
- TRUSTSTORE,
- TRUSTSTORE_PASSWORD,
- TRUSTSTORE_TYPE)
+ tlsConfiguration.getTruststorePath(),
+ tlsConfiguration.getTruststorePassword(),
+ tlsConfiguration.getTruststoreType())
);
}
+ @AfterClass
+ public static void afterClass() throws Exception {
+ if (tlsConfiguration != null) {
+ try {
+ if (StringUtils.isNotBlank(tlsConfiguration.getKeystorePath())) {
+ java.nio.file.Files.deleteIfExists(Path.of(tlsConfiguration.getKeystorePath()));
Review comment:
Can this qualified reference to `Files` be changed?
```suggestion
Files.deleteIfExists(Path.of(tlsConfiguration.getKeystorePath()));
```
##########
File path: nifi-commons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/util/KeyStoreUtilsGroovyTest.groovy
##########
@@ -61,9 +67,14 @@ class KeyStoreUtilsGroovyTest extends GroovyTestCase {
@Test
void testShouldVerifyKeystoreIsValid() {
// Arrange
+ TlsConfiguration tlsConfigParam = new StandardTlsConfiguration(null, TEST_KEYSTORE_PASSWORD, DEFAULT_STORE_TYPE, null, null, DEFAULT_STORE_TYPE)
+
+ TlsConfiguration tlsConfig = KeyStoreUtils.createTlsConfigAndNewKeystoreTruststore(tlsConfigParam)
Review comment:
Is it possible to move this call to a method annotated with `@BeforeClass` and then run the individual unit test methods with different `TlsConfiguration` properties? Generating and writing certificates is expensive, so optimizing this test to minimize uses of this method would be beneficial.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/util/TestInvokeHttpCommon.java
##########
@@ -70,6 +63,12 @@
import org.junit.Assert;
import org.junit.Test;
+import static org.apache.commons.codec.binary.Base64.encodeBase64;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
Review comment:
Recommend rolling back this change to import ordering.
##########
File path: nifi-commons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/util/KeyStoreUtilsGroovyTest.groovy
##########
@@ -141,4 +167,139 @@ 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(DEFAULT_STORE_TYPE)
+ deletePath(testKeystorePath)
+
+ // Assert
+ logger.info("Keystore path: ${testKeystorePath.toString()}")
+ assert testKeystorePath
+ }
+
+ @Test
+ void testShouldValidateTempTruststorePath() {
+ // Act
+ Path truststorePath = KeyStoreUtils.generateTempTruststorePath(DEFAULT_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, DEFAULT_STORE_TYPE, null, TEST_TRUSTSTORE_PASSWORD, DEFAULT_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.getTruststorePassword()
+ assert tlsConfig.getKeystoreType() == KeystoreType.PKCS12
+ assert tlsConfig.getTruststoreType() == KeystoreType.PKCS12
+ }
+
+ @Test
+ void testShouldValidateTlsConfigWithoutKeyPasswordParam() {
+ // Arrange
+ TlsConfiguration tlsConfigParam = new StandardTlsConfiguration(null, TEST_KEYSTORE_PASSWORD, null, DEFAULT_STORE_TYPE, null, TEST_TRUSTSTORE_PASSWORD, DEFAULT_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(DEFAULT_STORE_TYPE)
+
+ // Act
+ X509Certificate x509Cert = KeyStoreUtils.createKeyStoreAndGetX509Certificate(KEY_ALIAS, TEST_KEYSTORE_PASSWORD, TEST_KEYSTORE_PASSWORD, keystorePath.toString(), DEFAULT_STORE_TYPE)
+
+ boolean isKeystoreValid = KeyStoreUtils.isStoreValid(keystorePath.toUri().toURL(), DEFAULT_STORE_TYPE, TEST_KEYSTORE_PASSWORD.toCharArray())
+ deletePath(keystorePath)
+
+ // Assert
+ final String certDN = x509Cert.getIssuerDN().toString()
+ assert certDN =~ "CN="
+ assert isKeystoreValid
+ }
+
+ @Test
+ void testShouldValidateGetKeystoreExtension() {
Review comment:
See comment and question regarding testing of private methods.
##########
File path: nifi-commons/nifi-security-utils/src/test/java/org/apache/nifi/security/util/KeyStoreUtilsTest.java
##########
@@ -31,6 +28,8 @@
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
+import org.junit.BeforeClass;
+import org.junit.Test;
Review comment:
Recommend rolling back this change since it is only adjusting import ordering.
##########
File path: nifi-commons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/util/KeyStoreUtilsGroovyTest.groovy
##########
@@ -141,4 +167,139 @@ 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(DEFAULT_STORE_TYPE)
Review comment:
The `generateTempKeystorePath()` method is private, were you intending to make it public? If it should remain private, then this test and other tests for private methods should not be necessary.
##########
File path: nifi-commons/nifi-security-utils/src/test/groovy/org/apache/nifi/security/util/KeyStoreUtilsGroovyTest.groovy
##########
@@ -29,17 +30,22 @@ import org.slf4j.LoggerFactory
import javax.net.ssl.HttpsURLConnection
import javax.net.ssl.SSLSocket
import javax.net.ssl.SSLSocketFactory
+import java.nio.file.Files
+import java.nio.file.Path
+import java.nio.file.Paths
import java.security.KeyStore
import java.security.cert.Certificate
+import java.security.cert.X509Certificate
@RunWith(JUnit4.class)
class KeyStoreUtilsGroovyTest extends GroovyTestCase {
private static final Logger logger = LoggerFactory.getLogger(KeyStoreUtilsGroovyTest.class)
- private static final File KEYSTORE_FILE = new File("src/test/resources/keystore.jks")
- private static final String KEYSTORE_PASSWORD = "passwordpassword"
- private static final String KEY_PASSWORD = "keypassword"
- private static final KeystoreType KEYSTORE_TYPE = KeystoreType.JKS
+ private static final String TEST_KEYSTORE_PASSWORD = KeyStoreUtils.generatePassword()
+ private static final String TEST_KEY_PASSWORD = KeyStoreUtils.generatePassword()
+ private static final String TEST_TRUSTSTORE_PASSWORD = KeyStoreUtils.generatePassword()
Review comment:
Recommend using a static password, such as the class name, for these references instead of generating one. The `generatePassword()` method is `private` in `KeyStoreUtils` and calling `generatePassword()` takes additional time for random number generation.
----------------------------------------------------------------
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:
users@infra.apache.org