NIFI-8230 Removed default Sensitive Properties Key

[exceptionfactory]

Description of PR

NIFI-8230 Removes the default internal Sensitive Properties Key which provided fallback support when the value of nifi.sensitive.props.key was empty. NiFi used the default internal value on new installations where the administrator had not configured a sensitive properties key.

To maintain ease of use, this PR includes an update to NiFiPropertiesLoader.get() that checks for a null or blank sensitive properties key and generates a random key when the flow configuration file is not found. The random key generation process uses java.util.SecureRandom to generate an array of 24 bytes, which is then encoded using Base64 without padding to produce a 32 character string. Using Base64 without padding avoids introducing unsupported property characters while also providing a reasonable range of potential random characters.

This approach supports generating a random key for installations of NiFi while avoiding random generation when upgrading. Existing flow configurations may already contain sensitive properties encrypted with the default internal key. For upgrades of existing installations that do not have a sensitive properties key, NiFi logs and error and throws an exception referencing the Administration Guide section on Migrating a Flow with Sensitive Properties. To support migration, the ConfigEncryptionTool retains the default internal value for reading existing flows.

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 main)?

• 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?
• Have you verified that the full build is successful on JDK 8?
• 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?
• 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.

[CefBoud]

Great job, @exceptionfactory !

[exceptionfactory]

Great job, @exceptionfactory !

Thanks for the helpful feedback @CefBoud! I made changes and pushed an update to address your comments.

[CefBoud]

Excellent work, @exceptionfactory. Nicely refactored!

[thenatog]

I'll test this out

[thenatog]

Sorry just came back to this. I tested it out and found that it works as described. The issue I foresee is that we will likely have people asking questions on the email distro when they try to use their old configurations when this is put in place.

Currently the error message directs the user as follows:
"Sensitive Properties Key [nifi.sensitive.props.key] not found: See Administration Guide section [Migrating a Flow with Sensitive Properties]"

I followed the steps given in that section and was able to update my configuration and get the flow working with a new password. I also provided empty configuration and a key was generated for me, and I verified the new key was then used to encrypt sensitive values.

As far as backwards compatibility, this could be a hurdle for users who are upgrading. I wouldn't be surprised if the majority of users haven't used the encrypt-config.sh tool at all before. We might need to get some others to chime in to decide if this is a reasonable change to make for a minor version. That being said, I do think it's an important change to be making.

Whilst I'm also raising a question about backwards compatibility, I'm at the same time wondering if we can go a step further and change the default sensitive properties algorithm if nothing in the flow is already encrypted and the algorithm property is empty (though I believe it's already populated by default)? Is it possible we could start using PBEWITHSHA256AND256BITAES-CBC-BC? I'll be honest, I'm not actually sure of how significant of a security benefit this might be or if it will have a negative performance impact, but removing the use of MD5 sounds like a good idea to me. Also, not sure if this algorithm is available in all regions so that would be something to check. The encrypt-config.sh tool appears to allow specifying an old and new flow encryption algorithm. Just a thought.

[exceptionfactory]

Sorry just came back to this. I tested it out and found that it works as described. The issue I foresee is that we will likely have people asking questions on the email distro when they try to use their old configurations when this is put in place.

Currently the error message directs the user as follows:
"Sensitive Properties Key [nifi.sensitive.props.key] not found: See Administration Guide section [Migrating a Flow with Sensitive Properties]"

I followed the steps given in that section and was able to update my configuration and get the flow working with a new password. I also provided empty configuration and a key was generated for me, and I verified the new key was then used to encrypt sensitive values.

As far as backwards compatibility, this could be a hurdle for users who are upgrading. I wouldn't be surprised if the majority of users haven't used the encrypt-config.sh tool at all before. We might need to get some others to chime in to decide if this is a reasonable change to make for a minor version. That being said, I do think it's an important change to be making.

Whilst I'm also raising a question about backwards compatibility, I'm at the same time wondering if we can go a step further and change the default sensitive properties algorithm if nothing in the flow is already encrypted and the algorithm property is empty (though I believe it's already populated by default)? Is it possible we could start using PBEWITHSHA256AND256BITAES-CBC-BC? I'll be honest, I'm not actually sure of how significant of a security benefit this might be or if it will have a negative performance impact, but removing the use of MD5 sounds like a good idea to me. Also, not sure if this algorithm is available in all regions so that would be something to check. The encrypt-config.sh tool appears to allow specifying an old and new flow encryption algorithm. Just a thought.

Thanks for the testing and detailed feedback @thenatog!

There are certainly tradeoffs when it comes to to backwards compatibility, but running with an empty sensitive properties key is a serious security risk, so it is important to move in this direction. Perhaps work could be done to make the encrypt-config.sh toolkit easier to use in this case, but that could be considered in a separate task if necessary. It would be worth noting in migration guidance that using the toolkit is necessary.

Regarding the default algorithm, we should move away from all of the existing PBE values. NIFI-8246 addresses this issue explicitly and I am planning to submit a PR for that issue as soon as this PR is merged. The internal default sensitive properties key does not meet the length requirements of the secure key derivation functions, but once this is merged, changing the default sensitive properties algorithm should be straightforward.

[bbende]

If we are setting up a cluster, I assume that this call is going to generate a different random key on each node, which I think would mean that when a node receives a fingerprint or flow from the cluster coordinator, there would be an issue since they don't have the same key.

Maybe it makes sense to also check if one of the clustering properties is set, and make it part of the error scenario above so that in a cluster, the user is required to enter a sensitive props key?

@markap14 do you have any thoughts?

[exceptionfactory]

Thanks for the feedback @bbende! Calling NiFiProperties.isClustered() is a simple enough check in addition to checking whether a Flow Configuration already exists. Setting a sensitive properties key should be required for a clustered configuration, and that could be called out separately prior to checking for an existing Flow Configuration.

I will add the check and associated exception scenario.

[exceptionfactory]

Just to summarize the latest changes, after some additional discussion with @markap14, he suggested that implementing a simplified command for setting the Sensitive Properties Key to make the migration path easier for current users with blank keys.

The most recent commit includes an update to nifi.sh to support the following:

nifi.sh set-sensitive-properties-key <sensitivePropertiesKey>

The script passes the new key to the command class, which reads the current key from nifi.properties and falls back to the internal default value if the existing value is blank.

This approach required refactoring the PropertyEncryptor classes to a separate module named nifi-property-encryptor. The new command class is located under nifi-flow-encryptor. The existing ConfigEncryptionTool continues to work, but contained a lot of the same code, so this change provided an opportunity to refactor ConfigEncryptionTool to leverage the PropertyEncryptor interface. This simplifies ConfigEncryptionTool and also removes code that diverged from the standard property encryption approach.

With these changes, the Admin Guide now includes a new section describing the set-sensitive-properties-key command.

[markap14]

@exceptionfactory I gave this a try. Loaded up a flow created on main and then tried to startup. It failed almost immediately, telling me that i had to set the sensitive props key, which is good.

So then I tried to run the command update the key:

nifi-1.14.0-SNAPSHOT $ bin/nifi.sh set-sensitive-properties-key "the quick brown fox jumped over the lazy dog"

Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_281.jdk/Contents/Home
NiFi home: /Users/mpayne/devel/nifi/nifi-assembly/target/nifi-1.14.0-SNAPSHOT-bin/nifi-1.14.0-SNAPSHOT

Bootstrap Config File: /Users/mpayne/devel/nifi/nifi-assembly/target/nifi-1.14.0-SNAPSHOT-bin/nifi-1.14.0-SNAPSHOT/conf/bootstrap.conf

Unexpected number of arguments [9]
Usage: SetSensitivePropertiesKey <sensitivePropertiesKey>

So it appears there's an issue of some sort in the way that the arguments are passed in the nifi.sh script to the Java program, as it appears that even though "the quick brown fox jumped over the lazy dog" was quoted, it was passed to the Java program as 9 separate arguments.

[exceptionfactory]

@exceptionfactory I gave this a try. Loaded up a flow created on main and then tried to startup. It failed almost immediately, telling me that i had to set the sensitive props key, which is good.

So then I tried to run the command update the key:

nifi-1.14.0-SNAPSHOT $ bin/nifi.sh set-sensitive-properties-key "the quick brown fox jumped over the lazy dog"

Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_281.jdk/Contents/Home
NiFi home: /Users/mpayne/devel/nifi/nifi-assembly/target/nifi-1.14.0-SNAPSHOT-bin/nifi-1.14.0-SNAPSHOT

Bootstrap Config File: /Users/mpayne/devel/nifi/nifi-assembly/target/nifi-1.14.0-SNAPSHOT-bin/nifi-1.14.0-SNAPSHOT/conf/bootstrap.conf

Unexpected number of arguments [9]
Usage: SetSensitivePropertiesKey <sensitivePropertiesKey>

So it appears there's an issue of some sort in the way that the arguments are passed in the nifi.sh script to the Java program, as it appears that even though "the quick brown fox jumped over the lazy dog" was quoted, it was passed to the Java program as 9 separate arguments.

Thanks for the feedback @markap14! I pushed an update to nifi.sh that passes quoted arguments to SetSensitivePropertiesKey to handle quoted values as a single argument.

I also adjusted the minimum required key length to 12, aligning with the existing length requirement of 12 in StandardPropertySecretKeyProvider.

[exceptionfactory]

@markap14 Do you have any additional feedback, or do the latest changes look good? Thanks!

[markap14]

All looks good to me at this point @exceptionfactory. Thanks! I rebased against main, resolved the merge conflict, and merged to main.

[exceptionfactory]

Thanks @markap14, and thanks to @bbende, @CefBoud, and @thenatog for the feedback along the way!