exceptionfactory commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r650932423
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -72,17 +74,65 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import static
org.apache.nifi.processors.standard.util.FTPTransfer.createComponentProxyConfigSupplier;
public class SFTPTransfer implements FileTransfer {
+ // The following DefaultConfig is used for configuration purposes. There
are PropertyDescriptors associated with this class
+ // that allow an operator to choose various algorithms and ciphers to
allow to be used for their SSH connections.
+ private static final DefaultConfig DEFAULT_CONFIG = new DefaultConfig();
+
+ private static final Set<String> AVAILABLE_KEY_ALGORITHM_NAMES_SET =
Collections.unmodifiableSet(DEFAULT_CONFIG.getKeyAlgorithms().stream()
+ .map(Factory.Named::getName)
+ .collect(Collectors.toSet()));
+
+ private static final Set<String> AVAILABLE_CIPHER_NAMES_SET =
Collections.unmodifiableSet(DEFAULT_CONFIG.getCipherFactories().stream()
+ .map(Factory.Named::getName)
+ .collect(Collectors.toSet()));
+
+ private static final Set<String>
AVAILABLE_MESSAGE_AUTHENTICATION_CODE_NAMES_SET =
Collections.unmodifiableSet(DEFAULT_CONFIG.getMACFactories().stream()
+ .map(Factory.Named::getName)
+ .collect(Collectors.toSet()));
+
+ private static final Set<String>
AVAILABLE_KEY_EXCHANGE_ALGORITHM_NAMES_SET =
Collections.unmodifiableSet(DEFAULT_CONFIG.getKeyExchangeFactories().stream()
+ .map(Factory.Named::getName)
+ .collect(Collectors.toSet()));
+
+ /**
+ * Converts a set of names into an alphabetically ordered comma separated
value list.
+ *
+ * @param factorySetNames The set of names
+ * @return An alphabetically ordered comma separated value list of names
+ */
+ private static String convertFactorySetToString(Set<String>
factorySetNames) {
+ return factorySetNames
+ .stream()
+ .sorted()
+ .collect(Collectors.toList())
+ .toString().replaceAll("[\\[\\]]", "");
Review comment:
Relying on `toString()` for formatting a list does not seem like the
safest approach. The Spring Framework `StringUtils` includes a
`collectionToCommaDelimitedString` method that should provide the same
functionality.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -72,17 +74,65 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import static
org.apache.nifi.processors.standard.util.FTPTransfer.createComponentProxyConfigSupplier;
public class SFTPTransfer implements FileTransfer {
+ // The following DefaultConfig is used for configuration purposes. There
are PropertyDescriptors associated with this class
+ // that allow an operator to choose various algorithms and ciphers to
allow to be used for their SSH connections.
+ private static final DefaultConfig DEFAULT_CONFIG = new DefaultConfig();
Review comment:
Instead making this a class variable, what do you think about using a
static initializer block to create a temporary instance instead?
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -131,6 +181,47 @@
.required(true)
.build();
+ public static final PropertyDescriptor KEY_ALGORITHMS_ALLOWED = new
PropertyDescriptor.Builder()
+ .name("Key Algorithms Allowed")
+ .displayName("Key Algorithms Allowed")
+ .description("A comma-separated list of Key Algorithms to allow
your SFTP connection to use. Available options are: " +
convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
Review comment:
Recommend adjusting the wording slightly in this and other properties:
```suggestion
.description("A comma-separated list of Key Algorithms allowed
for SFTP connections. Available options are: " +
convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
```
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -72,17 +74,65 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import static
org.apache.nifi.processors.standard.util.FTPTransfer.createComponentProxyConfigSupplier;
public class SFTPTransfer implements FileTransfer {
+ // The following DefaultConfig is used for configuration purposes. There
are PropertyDescriptors associated with this class
+ // that allow an operator to choose various algorithms and ciphers to
allow to be used for their SSH connections.
+ private static final DefaultConfig DEFAULT_CONFIG = new DefaultConfig();
+
+ private static final Set<String> AVAILABLE_KEY_ALGORITHM_NAMES_SET =
Collections.unmodifiableSet(DEFAULT_CONFIG.getKeyAlgorithms().stream()
Review comment:
Recommend removing the `_SET` suffix from this and other variable names.
In light of the values coming from `DefaultConfig`, it seems like it would be
more accurate to use `DEFAULT` instead of `AVAILABLE`.
```suggestion
private static final Set<String> DEFAULT_KEY_ALGORITHM_NAMES =
Collections.unmodifiableSet(DEFAULT_CONFIG.getKeyAlgorithms().stream()
```
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -131,6 +181,47 @@
.required(true)
.build();
+ public static final PropertyDescriptor KEY_ALGORITHMS_ALLOWED = new
PropertyDescriptor.Builder()
+ .name("Key Algorithms Allowed")
+ .displayName("Key Algorithms Allowed")
+ .description("A comma-separated list of Key Algorithms to allow
your SFTP connection to use. Available options are: " +
convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
+
.defaultValue(convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
+ .required(false)
+
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+
.addValidator(StandardValidators.createRegexMatchingValidator(convertFactorySetToPattern(AVAILABLE_KEY_ALGORITHM_NAMES_SET)))
Review comment:
Although this approach to validation avoids some unexpected values, it
could create problems for newer versions of SSHJ as default algorithm values
change. For example, if SSHJ removed a default algorithm value in a newer
version of the library, this validator would mark a processor invalid. Given
that the property is marked as supporting Expression Language, it seems better
to use a simpler NiFi validator and allow SSHJ to validate the configured
values during SSH client initialization.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +735,38 @@ public Socket createSocket(InetAddress inetAddress, int i,
InetAddress inetAddre
return sftpClient;
}
+ void restrictSSHOptions(Config config) {
Review comment:
Recommend renaming to something along the lines of `setAlgorithms` or
`setConfig` since this is already in the context of an SSH class.
```suggestion
private void setConfig(final Config config) {
```
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +735,38 @@ public Socket createSocket(InetAddress inetAddress, int i,
InetAddress inetAddre
return sftpClient;
}
+ void restrictSSHOptions(Config config) {
+ final String csvRegex = " *, *";
Review comment:
Recommend replacing the pattern and `split()` with Spring Framework's
`StringUtils.commaDelimitedListToSet()`
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -131,6 +181,47 @@
.required(true)
.build();
+ public static final PropertyDescriptor KEY_ALGORITHMS_ALLOWED = new
PropertyDescriptor.Builder()
+ .name("Key Algorithms Allowed")
+ .displayName("Key Algorithms Allowed")
+ .description("A comma-separated list of Key Algorithms to allow
your SFTP connection to use. Available options are: " +
convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
+
.defaultValue(convertFactorySetToString(AVAILABLE_KEY_ALGORITHM_NAMES_SET))
Review comment:
Instead of configuring all available values as the default setting, it
seems like a better approach is to avoid setting a default value. This
preserves the existing behavior.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
