NIFI-6026 - First commit which adds a new tls-toolkit mode called Key… #3340

thenatog · 2019-02-27T18:28:49Z

…store. Should instead integrate the functionality into standalone mode.

NIFI-6026 - Updated splitKeystore to use standalone mode with a -splitKeystore argument.

NIFI-6026 - Removed unused file and references.

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
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.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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 travis-ci for build issues and submit an update to your PR as soon as possible.

…store. Should instead integrate the functionality into standalone mode. NIFI-6026 - Updated splitKeystore to use standalone mode with a -splitKeystore argument. NIFI-6026 - Removed unused file and references.

…argument checking in the command line parsing.

alopresto · 2019-02-27T19:02:24Z

Reviewing...

alopresto · 2019-02-27T19:47:47Z

...ls/src/main/java/org/apache/nifi/toolkit/tls/standalone/TlsToolkitStandaloneCommandLine.java

-            tlsToolkitStandaloneCommandLine.printUsage("Error generating TLS configuration. (" + e.getMessage() + ")");
-            System.exit(ExitCode.ERROR_GENERATING_CONFIG.ordinal());
+
+        StandaloneConfig conf = tlsToolkitStandaloneCommandLine.createSplitKeystoreConfig();


I think it makes sense to check isSplitKeystore before creating this config object because currently this is always created whether it is needed or not.

Updated this to use the command line parsed args rather than the config, and will no longer create the config unless it's required.

alopresto · 2019-02-27T19:48:48Z

...ls/src/main/java/org/apache/nifi/toolkit/tls/standalone/TlsToolkitStandaloneCommandLine.java

@@ -168,6 +188,17 @@ protected CommandLine doParse(String... args) throws CommandLineParseException {
        clientPasswordsGenerated = commandLine.getOptionValues(CLIENT_CERT_PASSWORD_ARG) == null;
        overwrite = commandLine.hasOption(OVERWRITE_ARG);

+        if(commandLine.hasOption(SPLIT_KEYSTORE_ARG)) {
+            if(commandLine.hasOption(KEY_STORE_PASSWORD_ARG) && commandLine.hasOption(KEY_PASSWORD_ARG)) {


I think KEY_PASSWORD_ARG can legitimately be empty if the keystore password and key password are the same (i.e. no explicit key password was set).

Updated to use the keystore password if no key password is provided and added unit test.

alopresto · 2019-02-27T19:58:23Z

Why did nifi-commons/nifi-security-utils/src/test/resources/keystore.jks get added in this PR?

… if keystore and key passwords are the same. Added some more tests.

thenatog · 2019-03-12T16:21:25Z

Why did nifi-commons/nifi-security-utils/src/test/resources/keystore.jks get added in this PR?

Using this for unit tests as it has known keystore and key passwords.

alopresto · 2019-04-08T23:47:22Z

...ls/src/main/java/org/apache/nifi/toolkit/tls/standalone/TlsToolkitStandaloneCommandLine.java

@@ -113,6 +118,8 @@ protected TlsToolkitStandaloneCommandLine(PasswordUtil passwordUtil) {
        addOptionWithArg(null, NIFI_DN_SUFFIX_ARG, "String to append to hostname(s) when determining DN.", TlsConfig.DEFAULT_DN_SUFFIX);
        addOptionNoArg("O", OVERWRITE_ARG, "Overwrite existing host output.");
        addOptionWithArg(null, ADDITIONAL_CA_CERTIFICATE_ARG, "Path to additional CA certificate (used to sign toolkit CA certificate) in PEM format if necessary");
+        addOptionWithArg("splitKeystore", SPLIT_KEYSTORE_ARG, "Split out a given keystore into its unencrypted key and certificates. Use -S and -K to specify the keystore and key passwords.");


Providing the "splitKeystore" string as the first argument to this call means that -splitKeystore and --splitKeystore are both supported here. Please pass null as the first argument instead, otherwise this leads to ambiguity and breaks consistency.

alopresto · 2019-04-09T00:05:00Z

nifi-toolkit/nifi-toolkit-tls/src/test/java/org/apache/nifi/toolkit/tls/util/TlsHelperTest.java

+    }
+
+    @Test
+    public void testOutputCertsAsPem() throws Exception {


I'd prefer if these tests were more descriptive -- list the expectations for what is happening here. The keystore should have a known number of entries, and that number should match the number of outputs. In addition, the key signature should be compared with that queried from the keystore directly, not just asserted that it is non-null. The same goes for the key assertions in the subsequent test.

alopresto · 2019-04-09T00:06:29Z

nifi-toolkit/nifi-toolkit-tls/src/test/java/org/apache/nifi/toolkit/tls/util/TlsHelperTest.java

+        HashMap<String, Key> keys = TlsHelper.extractKeys(keyStore, "changeit".toCharArray());
+        assertEquals(1, keys.size());
+        keys.forEach((String alias, Key key) -> assertEquals("PKCS#8", key.getFormat()));
+    }


I would like to see test cases documenting the following use cases:

Run with no keystore provided

Run with no keystore password provided

Run with wrong keystore password provided

Run with keystore with no aliases

Run with keystore with multiple aliases

Run with correct keystore password but wrong key password provided

alopresto · 2019-04-09T00:24:33Z

I ran through a series of manual tests on the toolkit mode. I like the functionality, and it works with both keystore formats. I made some comments requesting some unit test improvements and the removal of the "single hyphen" long argument in the CLI invocation. Once those are done, I will +1 and merge. Thanks.

…ded a check for empty keystores. Made tests a bit cleaner.

alopresto · 2019-05-07T17:02:41Z

Ran contrib-check and all tests pass. +1, merging.

thenatog added 2 commits February 27, 2019 13:27

NIFI-6026 - First commit which adds a new tls-toolkit mode called Key…

d47fd55

…store. Should instead integrate the functionality into standalone mode. NIFI-6026 - Updated splitKeystore to use standalone mode with a -splitKeystore argument. NIFI-6026 - Removed unused file and references.

NIFI-6026 - Removed some code that is not necessary after doing some …

f79030b

…argument checking in the command line parsing.

alopresto reviewed Feb 27, 2019

View reviewed changes

NIFI-6026 - Made some small changes to only require keystore password…

f5a2b67

… if keystore and key passwords are the same. Added some more tests.

alopresto reviewed Apr 8, 2019

View reviewed changes

alopresto reviewed Apr 9, 2019

View reviewed changes

thenatog added 2 commits April 18, 2019 11:30

NIFI-6026 - Added some more unit tests as per Andy's request. Also ad…

33d32f1

…ded a check for empty keystores. Made tests a bit cleaner.

NIFI-6026 - Added empty keystore used by unit tests.

22c81d0

asfgit closed this in fdea4c5 May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-6026 - First commit which adds a new tls-toolkit mode called Key… #3340

NIFI-6026 - First commit which adds a new tls-toolkit mode called Key… #3340

thenatog commented Feb 27, 2019

alopresto commented Feb 27, 2019

alopresto Feb 27, 2019

thenatog Mar 12, 2019

alopresto Feb 27, 2019

thenatog Mar 12, 2019

alopresto commented Feb 27, 2019

thenatog commented Mar 12, 2019

alopresto Apr 8, 2019

alopresto Apr 9, 2019

alopresto Apr 9, 2019

alopresto commented Apr 9, 2019

alopresto commented May 7, 2019

NIFI-6026 - First commit which adds a new tls-toolkit mode called Key… #3340

NIFI-6026 - First commit which adds a new tls-toolkit mode called Key… #3340

Conversation

thenatog commented Feb 27, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

alopresto commented Feb 27, 2019

alopresto Feb 27, 2019

Choose a reason for hiding this comment

thenatog Mar 12, 2019

Choose a reason for hiding this comment

alopresto Feb 27, 2019

Choose a reason for hiding this comment

thenatog Mar 12, 2019

Choose a reason for hiding this comment

alopresto commented Feb 27, 2019

thenatog commented Mar 12, 2019

alopresto Apr 8, 2019

Choose a reason for hiding this comment

alopresto Apr 9, 2019

Choose a reason for hiding this comment

alopresto Apr 9, 2019

Choose a reason for hiding this comment

alopresto commented Apr 9, 2019

alopresto commented May 7, 2019