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 19:27:02 UTC
[GitHub] [nifi] mtien-apache opened a new pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
mtien-apache opened a new pull request #4801:
URL: https://github.com/apache/nifi/pull/4801
…ly-generate certificates, Keystores, and Truststores and return it wrapped in a TLS configuration.
Updated TestInvokeHTTP, TestInvokeHttpSSL, TestInvokeHttpTwoWaySSL, and TestListenHTTP to use new Keystore functionality.
Uncommented code to enable a password-protected truststore in TestInvokeHttpSSL.
Added unit tests.
NIFI-1355-rebase Fixed a contrib check error.
NIFI-1355-rebase2 Removed unnecessary Truststore only method.
Cleaned up code.
NIFI-1355-rebase2 Fixed a delete Path method causing a build failure.
Update nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
Co-authored-by: exceptionfactory <ex...@gmail.com>
Update nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
Co-authored-by: exceptionfactory <ex...@gmail.com>
Update nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java
Co-authored-by: exceptionfactory <ex...@gmail.com>
Update nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestInvokeHttpTwoWaySSL.java
Co-authored-by: exceptionfactory <ex...@gmail.com>
NIFI-1355-rebase2 Revised based on PR feedback.
Added a password requirement when creating a new truststore.
Handled exception when loading a passwordless truststore type of Bouncy Castle PKCS12.
NIFI-1355-rebase2 Revised based on PR feedback.
Added the exception's full stacktrace to a delete method for easier troubleshooting.
Reverted unnecessary re-ordering of class imports.
Replaced a TLS config setting to use the getProtocol() method.
Fixed the usage of a qualified name for java.nio.file.Files by refactoring a deprecated method in TestListenHTTP.
Refactored some code.
NIFI-1355-rebase3 - Created a new branch and updated InvokeHttp Tests due to recent changes from NIFI-8171.
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Created a new PR due to significant test changes from NIFI-8171. Updated the InvokeHttp Tests with my changes._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [x] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [nifi] exceptionfactory commented on pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#issuecomment-772563416
Thanks for making the updates @mtien-apache looks good! +1 Merging.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r568903634
##########
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:
@exceptionfactory same as above comment- I'll remove the test.
----------------------------------------------------------------
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
[GitHub] [nifi] asfgit closed pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4801:
URL: https://github.com/apache/nifi/pull/4801
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r568903159
##########
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:
@exceptionfactory I'll keep the method private and remove the test.
##########
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:
@exceptionfactory same as above comment- I'll remove the test.
##########
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:
@exceptionfactory Fixed.
##########
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:
@exceptionfactory Re-factored the call to run with `@BeforeClass`. Only 2 tests are generating its own certificates and keystores because I'm validating specific parameters with the method.
##########
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:
@exceptionfactory Reverted.
##########
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:
@exceptionfactory Reverted.
##########
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:
@exceptionfactory Fixed.
##########
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:
@exceptionfactory Changed to use static passwords.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#issuecomment-772117918
@exceptionfactory Just pushed the suggested changes. Thanks for reviewing again!
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569028334
##########
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:
@exceptionfactory Fixed.
----------------------------------------------------------------
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [nifi] mtien-apache commented on pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#issuecomment-772117918
@exceptionfactory Just pushed the suggested changes. Thanks for reviewing again!
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569028938
##########
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:
@exceptionfactory Re-factored the call to run with `@BeforeClass`. Only 2 tests are generating its own certificates and keystores because I'm validating specific parameters with the method.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r568903159
##########
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:
@exceptionfactory I'll keep the method private and remove the test.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569029140
##########
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:
@exceptionfactory Reverted.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569029549
##########
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:
@exceptionfactory Changed to use static passwords.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569029365
##########
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:
@exceptionfactory Fixed.
----------------------------------------------------------------
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
[GitHub] [nifi] mtien-apache commented on a change in pull request #4801: NIFI-1355 Implemented new methods in KeyStoreUtils to programmatical…
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #4801:
URL: https://github.com/apache/nifi/pull/4801#discussion_r569029214
##########
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:
@exceptionfactory Reverted.
----------------------------------------------------------------
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