[GitHub] [nifi] gresockj commented on a change in pull request #5650: NIFI-9438 Refactor sensitive-property-provider to multiple modules

[GitBox <gi...@apache.org>]

gresockj commented on a change in pull request #5650:
URL: https://github.com/apache/nifi/pull/5650#discussion_r783330593



##########
File path: nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
##########
@@ -128,8 +132,7 @@ class ConfigEncryptionTool {
     private static final String NEW_FLOW_PROVIDER_ARG = "newFlowProvider"
     private static final String TRANSLATE_CLI_ARG = "translateCli"
 
-    private static final String PROTECTION_SCHEME_DESC = String.format("Selects the protection scheme for encrypted properties.  " +
-            "Valid values are: [%s] (default is %s)", PropertyProtectionScheme.values().join(", "), DEFAULT_PROTECTION_SCHEME.name())
+    private static final String PROTECTION_SCHEME_DESC = "Selects the protection scheme for encrypted properties. Default is AES_GCM"

Review comment:
       How is the user to know the valid values for protection scheme?

##########
File path: nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
##########
@@ -245,7 +250,7 @@ class ConfigEncryptionTool {
         options.addOption(Option.builder("S").longOpt(PROTECTION_SCHEME_ARG).hasArg(true).argName("protectionScheme").desc(PROTECTION_SCHEME_DESC).build())
         options.addOption(Option.builder("k").longOpt(KEY_ARG).hasArg(true).argName("keyhex").desc("The raw hexadecimal key to use to encrypt the sensitive properties").build())
         options.addOption(Option.builder("e").longOpt(KEY_MIGRATION_ARG).hasArg(true).argName("keyhex").desc("The old raw hexadecimal key to use during key migration").build())
-        options.addOption(Option.builder("H").longOpt(PROTECTION_SCHEME_MIGRATION_ARG).hasArg(true).argName("protectionScheme").desc("The old protection scheme to use during encryption migration (see --protectionScheme for possible values).  Default is " + DEFAULT_PROTECTION_SCHEME.name()).build())
+        options.addOption(Option.builder("H").longOpt(PROTECTION_SCHEME_MIGRATION_ARG).hasArg(true).argName("protectionScheme").desc("The old protection scheme to use during encryption migration (see --protectionScheme for possible values). Default is AES_GCM").build())

Review comment:
       The comment about seeing `--protectionScheme` for valid values no longer appears to be accurate.

##########
File path: nifi-commons/nifi-sensitive-property-provider/src/test/java/org/apache/nifi/properties/AwsKmsSensitivePropertyProviderIT.java
##########
@@ -1,142 +0,0 @@
-/*

Review comment:
       What do you think about restoring the integration tests?  I found them useful for manually exercising the sensitive property providers without doing a full deployment of nifi-toolkit and nifi.

##########
File path: nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/toolkit/encryptconfig/NiFiRegistryMode.groovy
##########
@@ -210,7 +213,7 @@ class NiFiRegistryMode implements ToolMode {
         cli.S(longOpt: 'protectionScheme',
                 args: 1,
                 argName: 'protectionScheme',
-                "Selects the protection scheme for encrypted properties.  Valid values are: [${PropertyProtectionScheme.values().join(", ")}] (default is ${ConfigEncryptionTool.DEFAULT_PROTECTION_SCHEME.name()})")
+                "Selects the protection scheme for encrypted properties.  (default is ${ConfigEncryptionTool.DEFAULT_PROTECTION_SCHEME.path})")

Review comment:
       Same question as above

##########
File path: nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/toolkit/encryptconfig/util/XmlEncryptor.groovy
##########
@@ -83,17 +83,11 @@ abstract class XmlEncryptor {
                 throw new IllegalStateException("Input XML is encrypted, but decryption capability is not enabled. " +
                         "Usually this means a decryption password / key was not provided to the tool.")
             }
-            String supportedDecryptionScheme = decryptionProvider.getIdentifierKey()
 
             logger.debug("Found ${encryptedNodes.size()} encrypted XML elements. Will attempt to decrypt using the provided decryption key.")
 
             encryptedNodes.each { node ->
                 logger.debug("Attempting to decrypt ${node.text()}")
-                if (node.@encryption != supportedDecryptionScheme) {

Review comment:
       This check ensured that the selected protectionScheme from the CLI matched the actual protection scheme used to encrypt the property.  Is the thought here to simply fall back to whatever failure results from attempting to decrypt with a different scheme?

##########
File path: nifi-toolkit/nifi-toolkit-encrypt-config/src/main/groovy/org/apache/nifi/toolkit/encryptconfig/NiFiRegistryMode.groovy
##########
@@ -224,7 +227,7 @@ class NiFiRegistryMode implements ToolMode {
         cli.H(longOpt: 'oldProtectionScheme',
                 args: 1,
                 argName: 'protectionScheme',
-                "The old protection scheme to use during encryption migration (see --protectionScheme for possible values).  Default is ${ConfigEncryptionTool.DEFAULT_PROTECTION_SCHEME.name()}.")
+                "The old protection scheme to use during encryption migration (see --protectionScheme for possible values). Default is AES_GCM.")

Review comment:
       Same comment as above




-- 
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