Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-6617 Refactor Encrypted Repository Configuration #5407

Closed
wants to merge 2 commits into from

Conversation

exceptionfactory
Copy link
Contributor

Description of PR

NIFI-6617 Refactors the configuration properties for Encrypted Repositories and reduces duplication in implementing classes. Notable highlights include the addition of a nifi-repository-encryption module under nifi-common, rewritten Encrypted Repository configuration details in the User Guide, and significant reduction in implementation code.

The new configuration properties provide the ability to enable encryption for all repositories using a single set of properties. These properties include an Encryption Protocol Version, which represents the combination of cipher algorithms and repository classes that provide repository encryption. The new properties also support configuring a shared Key Provider, as opposed to a separate Key Provider for each type of repository. Based of the lack of documentation for the FileBasedKeyProvider, and the lack of inherent security in the StaticKeyProvider, the new configuration properties support the KEYSTORE Key Provider implementation based on the current java.security.KeyStore implementation.

The new configuration approach and implementation classes maintain backward compatibility with current repository encryption properties. The internal implementation supports falling back to the existing per-repository configuration properties when found, allowing existing deployments to be upgraded.

Current interfaces and classes supporting encryption and decryption contained a large amount of duplicate code, with some subtle differences. The new RepositoryEncryptor interface provides the high-level contract for encryption and decryption operations. Implementing classes support the current cipher algorithm approach, using AES/CTR for stream-based repositories and AES/GCM for byte-array-based repositories. The new RecordMetadataReader interface abstracts the process of reading serialized record metadata, and implementations provide an approach to reading serialized Java objects that maintains backward compatibility. These changes minimize the amount of code necessary to implement the current encryption strategy. Additional minor changes removed the need for the CryptoUtils class and associated unit tests.

The rewritten section of the User Guide removes the previous experimental warnings associated with repository encryption, but retains notes about potential performance impacts. Additional documentation updates a section of the Administrator's Guide describing the new Repository Encryption Properties. The User Guide no longer describes the StaticKeyProvider or FileBasedKeyProvider, which should be considered deprecated.

Both the User Guide and Administrator's Guide provide example property settings, which should be helpful in evaluating these changes.

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.

- Updated documentation with new properties
- Refactored cipher operations to common RepositoryEncryptor classes
- Abstracted record metadata serialization for better compatibility
@gresockj gresockj self-requested a review September 22, 2021 17:43
Copy link
Contributor

@gresockj gresockj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this excellent improvement, @exceptionfactory! This greatly reduces the complexity of the repository encryption code. I have a few minor suggestions surrounding the EncryptedRepositoryType and EncryptionKeyProvider enumerations that involve adding methods to the enums in order prefer polymorphism over conditionals. However, if you prefer to keep the enums simpler, you might consider introducing "strategy" interfaces/implementations and corresponding factories based on the simple enums.

I tested out both the new and legacy configuration properties, and ran a basic performance test between the new encryption repositories and the ones from main. In addition to being faster performance-wise, it was significantly easier to set up -- nice work!

Using the current encryption on main branch
image

Using the encryption from NIFI-6617
image

@exceptionfactory
Copy link
Contributor Author

Thanks for the great feedback and performance comparisons @gresockj! The previous implementation had a number of duplicative checks in cipher-related operations, so it is great to have confirmation of the performance improvements.

Thanks for pointing out issues with the KeyProvider Factory, I agree the implementation could be more straightforward, so I will look at reworking the approach based on your suggestions.

… Names

- Refactored StandardRepositoryKeyProviderFactory to reduce property resolution complexity
- Changed enum values from FLOW_FILE to FLOWFILE
- Added final keyword to several repository method parameters
@exceptionfactory
Copy link
Contributor Author

@gresockj I pushed an update that streamlines a number of details in StandardRepositoryKeyProviderFactory, in addition to addressing your other comments. Let me know if you have any additional feedback!

Copy link
Contributor

@gresockj gresockj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the updates, @exceptionfactory! LGTM

@gresockj
Copy link
Contributor

gresockj commented Oct 8, 2021

Seeing no further community feedback, I'm going to merge this. Nice work, @exceptionfactory!

@asfgit asfgit closed this in 7043250 Oct 8, 2021
krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022
- Updated documentation with new properties
- Refactored cipher operations to common RepositoryEncryptor classes
- Abstracted record metadata serialization for better compatibility

Signed-off-by: Joe Gresock <jgresock@gmail.com>

This closes apache#5407.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants