NIFI-5481 Additional Sensitive Property Providers #3672

natural · 2019-08-26T19:33:36Z

Description of PR

The code in this change-set provides the following:

refactors Sensitive Property Providers
adds AWS KMS sensitive property provider (https://issues.apache.org/jira/browse/NIFI-6325)
adds Key Store sensitive property provider (https://issues.apache.org/jira/browse/NIFI-6326)
adds HashiCorp Vault sensitive property provider (https://issues.apache.org/jira/browse/NIFI-6324)
adds GCP sensitive property provider (https://issues.apache.org/jira/browse/NIFI-6616)
adds Azure sensitive property provider (https://issues.apache.org/jira/browse/NIFI-6515)
integrates the above with the Config Encryption Toolkit (https://issues.apache.org/jira/browse/NIFI-6363)
unit tests, integration tests, documentation for the above

This PR replaces #3542.

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? 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:

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.

alopresto

Reviewed code dealing directly with SPP implementations; have not reviewed ConfigEncryptTool or core loading production code.

nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CipherUtils.java

alopresto · 2019-08-27T14:42:25Z

nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CipherUtils.java

+     * @return integer value such that upper >= value >= lower
+     */
+    public static int getRandomInt(int lower, int upper) {
+        int value = random.nextInt(upper);


I would suggest a construct of random.nextInt(upper - lower) + lower; which avoids the while() loop.

...commons/nifi-security-utils/src/test/java/org/apache/nifi/security/util/CipherUtilsTest.java

...properties-loader/src/main/java/org/apache/nifi/properties/sensitive/ExternalProperties.java

...rties-loader/src/main/java/org/apache/nifi/properties/sensitive/ProtectedNiFiProperties.java

alopresto · 2019-08-27T17:51:55Z

.../java/org/apache/nifi/properties/sensitive/keystore/KeyStoreSensitivePropertyProviderIT.java

+        String keyPassword;    // and that key has a random password, too
+    }
+
+    private static final String[] keyAlgos = {"RSA", "DSA", "ECDSA", "AES"};


If the wrapped SPP in the KeyStoreSensitivePropertyProvider is AESSensitivePropertyProvider, I am confused about what actually happens when an RSA, DSA, or ECDSA key is retrieved from the keystore and used -- if it's just raw key bytes, the lengths still shouldn't match the 128, 192, or 256 bit expected key lengths.

We should enforce a validation check on line 104 of KeyStoreSensitivePropertyProvider which ensures the returned SecretKey from the keystore is an AES key (.secretKey.getAlgorithm()).

alopresto · 2019-08-27T17:57:08Z

.../java/org/apache/nifi/properties/sensitive/keystore/KeyStoreSensitivePropertyProviderIT.java

+
+    @Before
+    public void setUpTest() throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException {
+        final byte[] keyBytes = new byte[32];


This will fail on systems that do not have the unlimited strength crypto jurisdiction policies installed. As more people move to 1.8 112+ we will be ok, but some users are still on 1.8 prior. Recommend using 16 byte / 128 bit key length.

alopresto · 2019-08-27T17:59:52Z

.../java/org/apache/nifi/properties/sensitive/keystore/KeyStoreSensitivePropertyProviderIT.java

+        }
+    }
+
+    // These tests show we can use an AWS KMS key to encrypt/decrypt property values.


Copy/paste error on comment.

alopresto · 2019-08-27T18:00:36Z

...mework-bundle/nifi-framework/nifi-properties-loader/src/test/resources/vault_it/run-vault.sh

+mkdir -p /runtime
+
+openssl genrsa -out /runtime/root.key 4096
+openssl req -x509 -new -nodes -sha256 -days 1024 -subj "/C=US/ST=AK/O=Data in Motion/CN=localhost"  -key /runtime/root.key -out /runtime/root.crt


Change "Data in Motion" to "Apache NiFi" on these two lines.

alopresto · 2019-08-27T18:01:15Z

...es/nifi-framework-bundle/nifi-framework/nifi-runtime/src/main/java/org/apache/nifi/NiFi.java

@@ -353,14 +353,13 @@ private static String loadFormattedKey(String[] args) {
        if (parsedArgs.contains(KEY_FILE_FLAG)) {
            key = getKeyFromKeyFileAndPrune(parsedArgs);
            // Format the key (check hex validity and remove spaces)
-            key = formatHexKey(key);
-
+            // key = formatHexKey(key);


Remove commented lines if not necessary.

Removed, thanks for catching that.

pvillard31

I had a very quick look on the GCP part of the code and left a comment. Will try to find some time to test it.

pvillard31 · 2019-08-28T14:10:50Z

nifi-docs/src/main/asciidoc/administration-guide.adoc

@@ -1720,9 +1720,9 @@ For each Node, the minimum properties to configure are as follows:
   thread pool will never have fewer than this number of threads. It will grow as needed up to the maximum value set by the `nifi.cluster.node.protocol.max.threads`
   property.
 ** `nifi.cluster.node.protocol.max.threads` - The maximum number of threads that should be used to communicate with other nodes in the cluster. This property
-	defaults to `50`. A thread pool is used for replication requests to all nodes, and the thread pool will have a "core" size that is configured by the


Could you revert all the unnecessary changes on the documentation?

Oof, yeah. I would blame my tools if I could. Thanks for the catch.

.../main/java/org/apache/nifi/properties/sensitive/gcp/kms/GCPKMSSensitivePropertyProvider.java

NIFI-6325 Adds support for AWS KMS Sensitive Property Provider.

natural · 2019-12-04T17:31:05Z

Resubmitting in #3916.

natural mentioned this pull request Aug 26, 2019

NIFI-6363 Integrates AWS KMS SPP. Refactors SSPP. #3542

Closed

11 tasks

alopresto requested changes Aug 27, 2019

View reviewed changes

pvillard31 reviewed Aug 28, 2019

View reviewed changes

natural force-pushed the nifi-6363-draft branch from 0740d97 to 54a1100 Compare September 3, 2019 20:09

natural changed the title ~~NIFI-6363 Additional Sensitive Property Providers~~ NIFI-5481 Additional Sensitive Property Providers Sep 5, 2019

natural added 20 commits September 30, 2019 14:36

NIFI-6363 Integrates AWS KMS SPP. Refactors SSPP.

56b0cfb

NIFI-6325 Adds support for AWS KMS Sensitive Property Provider.

NIFI-6363 Additional SSPP refactorings.

3b53d1a

NIFI-6326 GCP + Key Store + Vault + AWS combined WIP.

498aa5c

NIFI-6363 (local) checkpoint.

ba6ccf0

NIFI-6363 (local) checkpoint.

a9be393

NIFI-6363 (local) checkpoint.

5f44ab7

NIFI-6363 (local) Changes from peer review.

1ed223b

NIFI-6363 (local) Checkpoint.

7e5a543

NIFI-6363 (local) Simplified scheme usage.

ffa6d53

NIFI-6363 (local) Trim.

35dcb80

NIFI-6363 (local) Restores whitespace.

51ba19d

NIFI-6363 (local) Changes from PR feedback.

250f448

NIFI-6363 Additional docs.

a5febb6

NIFI-6363 (local) More bits.

f1bb1d3

NIFI-6363 (local) again.

7f769e1

NIFI-6363 (local) Additional property to reset.

da2788d

NIFI-6363 (local) docs.

d15614a

NIFI-6363 (local) unneeded.

15fd6ca

NIFI-6363 (local) reverted.

a5a9c4a

NIFI-6363 (local) Moved CipherUtils class.

63d998b

natural force-pushed the nifi-6363-draft branch from 92fda89 to 63d998b Compare September 30, 2019 22:50

NIFI-6326 Adds Hadoop Credentials Sensitive Property Provider.

bc2600b

natural mentioned this pull request Dec 4, 2019

NIFI-5481 Additional Sensitive Property Providers (#3) #3916

Closed

12 tasks

natural closed this Dec 4, 2019

natural mentioned this pull request Feb 24, 2020

NIFI-5481 Additional Sensitive Property Providers (#4) #4080

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-5481 Additional Sensitive Property Providers #3672

NIFI-5481 Additional Sensitive Property Providers #3672

natural commented Aug 26, 2019 •

edited

Loading

alopresto left a comment

alopresto Aug 27, 2019

alopresto Aug 27, 2019

alopresto Aug 27, 2019

alopresto Aug 27, 2019

alopresto Aug 27, 2019

alopresto Aug 27, 2019

alopresto Aug 27, 2019

natural Sep 3, 2019

pvillard31 left a comment •

edited

Loading

pvillard31 Aug 28, 2019

natural Sep 3, 2019

natural commented Dec 4, 2019

NIFI-5481 Additional Sensitive Property Providers #3672

NIFI-5481 Additional Sensitive Property Providers #3672

Conversation

natural commented Aug 26, 2019 • edited Loading

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

alopresto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvillard31 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natural commented Dec 4, 2019

natural commented Aug 26, 2019 •

edited

Loading

pvillard31 left a comment •

edited

Loading