NIFI-11161 Simplify KeyedCipherPropertyEncryptor#6939
NIFI-11161 Simplify KeyedCipherPropertyEncryptor#6939exceptionfactory wants to merge 1 commit intoapache:mainfrom
Conversation
- Replaced KeyedCipherProvider references with direct Cipher instantiation
exceptionfactory
force-pushed
the
NIFI-11161
branch
from
5eb9f0f to
92cc2a1
Compare
greyp9
left a comment
greyp9
left a comment
There was a problem hiding this comment.
Nice refactor! Couple of questions for follow-up...
| private static final int ARRAY_START = 0; | ||
|
|
||
| private final EncryptionMethod encryptionMethod; | ||
| private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding"; |
There was a problem hiding this comment.
The IV length, tag length, and algorithm are each declared multiple times in the code base. Do you envision consolidating those usages at some point in the future?
There was a problem hiding this comment.
That's a good question. Theoretically, all the values should be the same across the board, but we would need to implement this in a way that avoid introducing unnecessary dependencies. There is other work to do for removing support for deprecated legacy algorithms. That might be an opportunity for consolidation.
| public void testDecryptEncryptionException() { | ||
| final String encodedProperty = Hex.encodeHexString(PROPERTY.getBytes(DEFAULT_CHARSET)); | ||
| assertThrows(EncryptionException.class, () -> encryptor.decrypt(encodedProperty)); | ||
| assertThrows(Exception.class, () -> encryptor.decrypt(encodedProperty)); |
There was a problem hiding this comment.
Why not retain the more specific exception type here?
There was a problem hiding this comment.
The thrown exception is different based on the Java version, so switching to the more generic Exception provided the least amount of impact to the unit test.
| final SecretKey secretKey) { | ||
| this.cipherProvider = cipherProvider; | ||
| this.encryptionMethod = encryptionMethod; | ||
| protected KeyedCipherPropertyEncryptor(final SecretKey secretKey) { |
There was a problem hiding this comment.
Is AES/GCM/NoPadding the previous default? Curious about the upgrade implications.
There was a problem hiding this comment.
Yes, AES/GCM/NoPadding is the current value for all instances of the KeyedCipherProvider, so this change does not impact compatibility with existing AES-GCM sensitive property algorithms.
- Replaced KeyedCipherProvider references with direct Cipher instantiation This closes #6939 Signed-off-by: Paul Grey <greyp@apache.org>
2 participants
Summary
NIFI-11161 Simplifies the
KeyedCipherPropertyEncryptorimplementation that supports encryption and decryption of sensitive flow property values.The existing implementation delegates
Cipherinstantiation to theAESKeyedCipherProvider, which includes a number of checks that are not necessary for theKeyedCipherPropertyEncryptor.Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation