You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "exceptionfactory (via GitHub)" <gi...@apache.org> on 2023/02/09 22:30:12 UTC
[GitHub] [nifi] exceptionfactory opened a new pull request, #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
exceptionfactory opened a new pull request, #6939:
URL: https://github.com/apache/nifi/pull/6939
# Summary
[NIFI-11161](https://issues.apache.org/jira/browse/NIFI-11161) Simplifies the `KeyedCipherPropertyEncryptor` implementation that supports encryption and decryption of sensitive flow property values.
The existing implementation delegates `Cipher` instantiation to the `AESKeyedCipherProvider`, which includes a number of checks that are not necessary for the `KeyedCipherPropertyEncryptor`.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [X] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [X] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [X] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [X] Pull Request based on current revision of the `main` branch
- [X] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [X] Build completed using `mvn clean install -P contrib-check`
- [X] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119417233
##########
nifi-commons/nifi-property-encryptor/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java:
##########
@@ -16,47 +16,36 @@
*/
package org.apache.nifi.encrypt;
-import org.apache.nifi.security.util.EncryptionMethod;
-import org.apache.nifi.security.util.crypto.KeyedCipherProvider;
-
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.Objects;
/**
- * Property Encryptor implementation using Keyed Cipher Provider
+ * Property Encryptor implementation using AES-GCM
*/
class KeyedCipherPropertyEncryptor extends CipherPropertyEncryptor {
private static final int INITIALIZATION_VECTOR_LENGTH = 16;
- private static final int ARRAY_START = 0;
-
- private static final boolean ENCRYPT = true;
+ private static final int GCM_TAG_LENGTH_BITS = 128;
- private static final boolean DECRYPT = false;
-
- private final KeyedCipherProvider cipherProvider;
+ private static final int ARRAY_START = 0;
- private final EncryptionMethod encryptionMethod;
+ private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding";
private final SecretKey secretKey;
private final SecureRandom secureRandom;
private final String description;
- protected KeyedCipherPropertyEncryptor(final KeyedCipherProvider cipherProvider,
- final EncryptionMethod encryptionMethod,
- final SecretKey secretKey) {
- this.cipherProvider = cipherProvider;
- this.encryptionMethod = encryptionMethod;
+ protected KeyedCipherPropertyEncryptor(final SecretKey secretKey) {
Review Comment:
Is `AES/GCM/NoPadding` the previous default? Curious about the upgrade implications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 closed pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 closed pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
URL: https://github.com/apache/nifi/pull/6939
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119403187
##########
nifi-commons/nifi-property-encryptor/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java:
##########
@@ -16,47 +16,36 @@
*/
package org.apache.nifi.encrypt;
-import org.apache.nifi.security.util.EncryptionMethod;
-import org.apache.nifi.security.util.crypto.KeyedCipherProvider;
-
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.Objects;
/**
- * Property Encryptor implementation using Keyed Cipher Provider
+ * Property Encryptor implementation using AES-GCM
*/
class KeyedCipherPropertyEncryptor extends CipherPropertyEncryptor {
private static final int INITIALIZATION_VECTOR_LENGTH = 16;
- private static final int ARRAY_START = 0;
-
- private static final boolean ENCRYPT = true;
+ private static final int GCM_TAG_LENGTH_BITS = 128;
- private static final boolean DECRYPT = false;
-
- private final KeyedCipherProvider cipherProvider;
+ private static final int ARRAY_START = 0;
- private final EncryptionMethod encryptionMethod;
+ private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding";
Review 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?
##########
nifi-commons/nifi-property-encryptor/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java:
##########
@@ -77,26 +70,26 @@ public void testEncryptHexadecimalEncoded() throws DecoderException {
@Test
public void testDecryptEncryptionException() {
final String encodedProperty = Hex.encodeHexString(PROPERTY.getBytes(DEFAULT_CHARSET));
- assertThrows(EncryptionException.class, () -> encryptor.decrypt(encodedProperty));
+ assertThrows(Exception.class, () -> encryptor.decrypt(encodedProperty));
Review Comment:
Why not retain the more specific exception type here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119424370
##########
nifi-commons/nifi-property-encryptor/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java:
##########
@@ -16,47 +16,36 @@
*/
package org.apache.nifi.encrypt;
-import org.apache.nifi.security.util.EncryptionMethod;
-import org.apache.nifi.security.util.crypto.KeyedCipherProvider;
-
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.Objects;
/**
- * Property Encryptor implementation using Keyed Cipher Provider
+ * Property Encryptor implementation using AES-GCM
*/
class KeyedCipherPropertyEncryptor extends CipherPropertyEncryptor {
private static final int INITIALIZATION_VECTOR_LENGTH = 16;
- private static final int ARRAY_START = 0;
-
- private static final boolean ENCRYPT = true;
+ private static final int GCM_TAG_LENGTH_BITS = 128;
- private static final boolean DECRYPT = false;
-
- private final KeyedCipherProvider cipherProvider;
+ private static final int ARRAY_START = 0;
- private final EncryptionMethod encryptionMethod;
+ private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding";
private final SecretKey secretKey;
private final SecureRandom secureRandom;
private final String description;
- protected KeyedCipherPropertyEncryptor(final KeyedCipherProvider cipherProvider,
- final EncryptionMethod encryptionMethod,
- final SecretKey secretKey) {
- this.cipherProvider = cipherProvider;
- this.encryptionMethod = encryptionMethod;
+ protected KeyedCipherPropertyEncryptor(final SecretKey secretKey) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119423078
##########
nifi-commons/nifi-property-encryptor/src/main/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptor.java:
##########
@@ -16,47 +16,36 @@
*/
package org.apache.nifi.encrypt;
-import org.apache.nifi.security.util.EncryptionMethod;
-import org.apache.nifi.security.util.crypto.KeyedCipherProvider;
-
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.Objects;
/**
- * Property Encryptor implementation using Keyed Cipher Provider
+ * Property Encryptor implementation using AES-GCM
*/
class KeyedCipherPropertyEncryptor extends CipherPropertyEncryptor {
private static final int INITIALIZATION_VECTOR_LENGTH = 16;
- private static final int ARRAY_START = 0;
-
- private static final boolean ENCRYPT = true;
+ private static final int GCM_TAG_LENGTH_BITS = 128;
- private static final boolean DECRYPT = false;
-
- private final KeyedCipherProvider cipherProvider;
+ private static final int ARRAY_START = 0;
- private final EncryptionMethod encryptionMethod;
+ private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding";
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119423748
##########
nifi-commons/nifi-property-encryptor/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java:
##########
@@ -77,26 +70,26 @@ public void testEncryptHexadecimalEncoded() throws DecoderException {
@Test
public void testDecryptEncryptionException() {
final String encodedProperty = Hex.encodeHexString(PROPERTY.getBytes(DEFAULT_CHARSET));
- assertThrows(EncryptionException.class, () -> encryptor.decrypt(encodedProperty));
+ assertThrows(Exception.class, () -> encryptor.decrypt(encodedProperty));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6939: NIFI-11161 Simplify KeyedCipherPropertyEncryptor
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #6939:
URL: https://github.com/apache/nifi/pull/6939#discussion_r1119423748
##########
nifi-commons/nifi-property-encryptor/src/test/java/org/apache/nifi/encrypt/KeyedCipherPropertyEncryptorTest.java:
##########
@@ -77,26 +70,26 @@ public void testEncryptHexadecimalEncoded() throws DecoderException {
@Test
public void testDecryptEncryptionException() {
final String encodedProperty = Hex.encodeHexString(PROPERTY.getBytes(DEFAULT_CHARSET));
- assertThrows(EncryptionException.class, () -> encryptor.decrypt(encodedProperty));
+ assertThrows(Exception.class, () -> encryptor.decrypt(encodedProperty));
Review Comment:
The thrown exception is different based the Java version, so switching to the more generic Exception provided the least amount of impact to the unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org