-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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-7668 Implemented support for additional AEAD property encryption methods #4809
Conversation
82cc88a
to
c136713
Compare
...framework-components/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java
Outdated
Show resolved
Hide resolved
...ifi-framework-components/src/main/java/org/apache/nifi/encrypt/PropertyEncryptionMethod.java
Outdated
Show resolved
Hide resolved
...ework-components/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...ework-components/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...mponents/src/test/java/org/apache/nifi/encrypt/PasswordBasedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...mponents/src/test/java/org/apache/nifi/encrypt/PasswordBasedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lehel44 Thanks for the feedback on syntax, will make some adjustments based on your feedback. Please feel free to follow up if you have any comments on the substance of the implementation.
...framework-components/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java
Outdated
Show resolved
Hide resolved
...ifi-framework-components/src/main/java/org/apache/nifi/encrypt/PropertyEncryptionMethod.java
Outdated
Show resolved
Hide resolved
...ework-components/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...ework-components/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...mponents/src/test/java/org/apache/nifi/encrypt/PasswordBasedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
...mponents/src/test/java/org/apache/nifi/encrypt/PasswordBasedCipherPropertyEncryptorTest.java
Outdated
Show resolved
Hide resolved
|
@exceptionfactory Thank you for the improvements! |
… methods - Added support for PBKDF2 and Scrypt property encryption methods in addition to Argon2 - Refactored StringEncryptor class to PropertyEncryptor interface with implementations - Added PasswordBasedCipherPropertyEncryptor and KeyedCipherPropertyEncryptor - Replaced direct instantiation of encryptor with PropertyEncryptorFactory - Refactored applicable unit tests to use mocked PropertyEncryptor
6d2fef1
to
0f52254
Compare
| import java.util.Arrays; | ||
|
|
||
| /** | ||
| * Extension of Bcrypt Secure Hasher used for Key Derivation support specified of Derived Key Length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Extension of Bcrypt Secure Hasher used for Key Derivation support. Allows specifying a Derived Key Length."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the correction.
| digest = messageDigest.digest(bcryptHash); | ||
| } else { | ||
| // Truncate bcrypt hash and remove algorithm parameters | ||
| byte[] hash = Arrays.copyOfRange(bcryptHash, HASH_START_INDEX, bcryptHash.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we truncate the bcrypt hash and what is the relevance of the HASH_START_INDEX=29?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is migrated from the BcryptCipherProvider. Apparently early versions of the BcryptCipherProvider digested all of the bytes returned from BcryptSecureHasher, however, the value returned includes not only the bcrypt hash itself, but also additional parameters include cost, salt, and version information. The bcrypt page on Wikipedia has a good example of what is returned. In summary, the reason for the flag is to provide backward compatibility, whereas the behavior should only use the actual hash section of what gets returned. Do you think additional details are necessary in the comment on this line of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, so just to confirm, on line 81 we extract only the hash (and dropping the hash params), and then on line 85 we return the hash truncated to the specified key length? Makes sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly as you described.
| /** | ||
| * Key Deriviation Bcrypt Secure Hasher with specified Derived Key Length and Cost Parameters | ||
| * | ||
| * @param derivedKeyLength Derived Key Length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derived key length in bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, derived key length in bytes, will update the documentation to state that explicitly.
| */ | ||
| @Override | ||
| public String encrypt(final String property) { | ||
| final byte[] binary = property.getBytes(PROPERTY_CHARSET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are NiFi properties always UTF_8 encoded regardless of language?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. This approach preserves the current behavior and makes it more explicit. StringEncryptor previously buried this fact in the encryptKeyed and encryptPBE methods when calling Cipher.doFinal().
|
Other than a few questions, things look great to me. |
Thanks for the feedback @thenatog! Pushed an update with clarifying comments for the bcrypt key derivation implementation. |
|
+1, will merge |
… methods - Added support for PBKDF2 and Scrypt property encryption methods in addition to Argon2 - Refactored StringEncryptor class to PropertyEncryptor interface with implementations - Added PasswordBasedCipherPropertyEncryptor and KeyedCipherPropertyEncryptor - Replaced direct instantiation of encryptor with PropertyEncryptorFactory - Refactored applicable unit tests to use mocked PropertyEncryptor NIFI-7668 Consolidated similar methods to CipherPropertyEncryptor NIFI-7668 Updated AbstractTimeBasedSchedulingAgent with PropertyEncryptor NIFI-7668 Added support for bcrypt secure hashing algorithm NIFI-7668 Updated comments to clarify implementation of bcrypt key derivation Signed-off-by: Nathan Gough <thenatog@gmail.com> This closes apache#4809.
… methods - Added support for PBKDF2 and Scrypt property encryption methods in addition to Argon2 - Refactored StringEncryptor class to PropertyEncryptor interface with implementations - Added PasswordBasedCipherPropertyEncryptor and KeyedCipherPropertyEncryptor - Replaced direct instantiation of encryptor with PropertyEncryptorFactory - Refactored applicable unit tests to use mocked PropertyEncryptor NIFI-7668 Consolidated similar methods to CipherPropertyEncryptor NIFI-7668 Updated AbstractTimeBasedSchedulingAgent with PropertyEncryptor NIFI-7668 Added support for bcrypt secure hashing algorithm NIFI-7668 Updated comments to clarify implementation of bcrypt key derivation Signed-off-by: Nathan Gough <thenatog@gmail.com> This closes apache#4809.
Description of PR
NIFI-7668 Implements support for the following additional AEAD property encryption methods:
NIFI_BCRYPT_AES_GCM_128NIFI_BCRYPT_AES_GCM_256NIFI_PBKDF2_AES_GCM_128NIFI_PBKDF2_AES_GCM_256NIFI_SCRYPT_AES_GCM_128NIFI_SCRYPT_AES_GCM_256This PR incorporates the following changes:
StringEncryptorclass toPropertyEncryptorinterface with implementationsPasswordBasedCipherPropertyEncryptorandKeyedCipherPropertyEncryptorPropertyEncryptorFactoryPropertyEncryptorIn 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
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
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.