You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/05/06 16:21:41 UTC
[GitHub] [nifi] JonathanKessler opened a new pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
JonathanKessler opened a new pull request #5061:
URL: https://github.com/apache/nifi/pull/5061
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Allows users to configure which algorithms, message authentication codes and ciphers to allow GetSFTP, PutSFTP, FetchSFTP and ListSFTP to use when establishing an SSH connection.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message? Yes
- [X] 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. Yes
- [X] Has your PR been rebased against the latest commit within the target branch (typically `main`)? Yes
- [X] 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 `squash` or use `--force` when pushing to allow for clean monitoring of changes._ Yes
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on JDK 8?
- [X] Have you verified that the full build is successful on JDK 11?
- [X] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? N/A
- [X] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`? N/A
- [X] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`? N/A
- [X] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [X] Have you ensured that format looks appropriate for the output in which it is rendered? N/A
### 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.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r656205757
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +712,44 @@ public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddre
return sftpClient;
}
+ void updateConfigAlgorithms(Config config) {
+ if(ctx.getProperty(CIPHERS_ALLOWED).isSet()) {
+ Set<String> allowedCiphers = Arrays.stream(ctx.getProperty(CIPHERS_ALLOWED).evaluateAttributeExpressions().getValue().split(","))
+ .map(String::trim)
+ .collect(Collectors.toSet());
Review comment:
Thanks for the reply, it is fairly succinct as it stands, and I agree that it would probably be helpful to consider implementing a standardized approach for handling multiple values in a property.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-867676830
> > When attempting to verify the most recent updates at runtime, the `ListSFTP` worked as expected with the default blank values for the new properties. However, when specifying a value for `Ciphers Allowed`, NiFi marked the processor as invalid with the following message:
> > `'Ciphers Allowed' validated against 'aes256-cbc' is invalid because 'Ciphers Allowed' is not supported or has no Validator associated with it.`
> > It appears that recent updates to remove the default values also removed any kind of validator, but I missed that detail last time around. Based on previous comments and support for Expression Language, it seems like using StandardValidators.NON_EMPTY_EL_VALIDATOR would be a good option.
>
> Good catch but NON_EMPTY_EL_VALIDATOR ensures that EL is present. It's supported but not required Going with NON_BLANK_VALIDATOR.
Thanks for the update, that sounds good, I will take another look at verifying the changes.
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-841874983
Thanks for the updates, especially the Variable Registry.
LGTM
--
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r651115437
##########
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:
No reason to hold on to a reference I suppose. Done.
--
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:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5061:
URL: https://github.com/apache/nifi/pull/5061
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r628795099
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchSFTP.java
##########
@@ -81,6 +81,10 @@
properties.add(SFTPTransfer.STRICT_HOST_KEY_CHECKING);
properties.add(SFTPTransfer.USE_COMPRESSION);
properties.add(SFTPTransfer.PROXY_CONFIGURATION_SERVICE);
+ properties.add(SFTPTransfer.CIPHERS_ALLOWED);
Review comment:
I would change the order of properties so these new ones are not in the middle of the proxy-related props. Arguably, the new ones could go at the end since many or most users will not be interested in changing the settings. The same is true for the other modified processors.
--
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-867674263
> When attempting to verify the most recent updates at runtime, the `ListSFTP` worked as expected with the default blank values for the new properties. However, when specifying a value for `Ciphers Allowed`, NiFi marked the processor as invalid with the following message:
>
> `'Ciphers Allowed' validated against 'aes256-cbc' is invalid because 'Ciphers Allowed' is not supported or has no Validator associated with it.`
>
> It appears that recent updates to remove the default values also removed any kind of validator, but I missed that detail last time around. Based on previous comments and support for Expression Language, it seems like using StandardValidators.NON_EMPTY_EL_VALIDATOR would be a good option.
Good catch but NON_EMPTY_EL_VALIDATOR ensures that EL is present. It's supported but not required Going with NON_BLANK_VALIDATOR.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-867088252
When attempting to verify the most recent updates at runtime, the `ListSFTP` worked as expected with the default blank values for the new properties. However, when specifying a value for `Ciphers Allowed`, NiFi marked the processor as invalid with the following message:
`'Ciphers Allowed' validated against 'aes256-cbc' is invalid because 'Ciphers Allowed' is not supported or has no Validator associated with it.`
It appears that recent updates to remove the default values also removed any kind of validator, but I missed that detail last time around. Based on previous comments and support for Expression Language, it seems like using StandardValidators.NON_EMPTY_EL_VALIDATOR would be a good option.
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r628795099
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchSFTP.java
##########
@@ -81,6 +81,10 @@
properties.add(SFTPTransfer.STRICT_HOST_KEY_CHECKING);
properties.add(SFTPTransfer.USE_COMPRESSION);
properties.add(SFTPTransfer.PROXY_CONFIGURATION_SERVICE);
+ properties.add(SFTPTransfer.CIPHERS_ALLOWED);
Review comment:
I would change the order of properties so these new ones are not in the middle of the proxy-related props. Arguably, the new ones could go at the end since many or most users will not be interested in changing the settings.
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-835511797
This is a great new feature! I've seen this requested from more than one user very recently. They will be thrilled. Thanks.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r655660187
##########
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,49 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
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 {
+ private static final Set<String> DEFAULT_KEY_ALGORITHM_NAMES;
+ private static final Set<String> DEFAULT_CIPHER_NAMES;
+ private static final Set<String> DEFAULT_MESSAGE_AUTHENTICATION_CODE_NAMES;
+ private static final Set<String> DEFAULT_KEY_EXCHANGE_ALGORITHM_NAMES;
+
+ static {
+ DefaultConfig defaultConfig = new DefaultConfig();
+
+ DEFAULT_KEY_ALGORITHM_NAMES = defaultConfig.getKeyAlgorithms().stream()
+ .map(Factory.Named::getName).collect(Collectors.toUnmodifiableSet());
Review comment:
Unfortunately `Collectors.toUnmodifiableSet()` is not supported on Java 8, so GitHub automated builds failed. Switching to `Collectors.toSet()` and wrapping it with `Collections.unmodifableSet()` should work for both Java 8 and 11.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +712,44 @@ public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddre
return sftpClient;
}
+ void updateConfigAlgorithms(Config config) {
+ if(ctx.getProperty(CIPHERS_ALLOWED).isSet()) {
+ Set<String> allowedCiphers = Arrays.stream(ctx.getProperty(CIPHERS_ALLOWED).evaluateAttributeExpressions().getValue().split(","))
+ .map(String::trim)
+ .collect(Collectors.toSet());
Review comment:
To reduce duplication and ensure the same behavior, what do you think about refactoring similar property processing behavior to a shared method? Something along the following lines:
```
private Set<String> getCommaSeparatedValues(final PropertyValue propertyValue) {
Set<String> values = Collections.emptySet();
if (propertyValue.isSet()) {
values = Arrays.stream(propertyValue.evaluateAttributeExpressions().getValue().split(","))
.map(String::trim)
.collect(Collectors.toSet());
}
return values;
}
```
It may also be possible to create a shared method for collected allowed values, but at least creating a shared method for parsing property values would be helpful.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r655660187
##########
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,49 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
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 {
+ private static final Set<String> DEFAULT_KEY_ALGORITHM_NAMES;
+ private static final Set<String> DEFAULT_CIPHER_NAMES;
+ private static final Set<String> DEFAULT_MESSAGE_AUTHENTICATION_CODE_NAMES;
+ private static final Set<String> DEFAULT_KEY_EXCHANGE_ALGORITHM_NAMES;
+
+ static {
+ DefaultConfig defaultConfig = new DefaultConfig();
+
+ DEFAULT_KEY_ALGORITHM_NAMES = defaultConfig.getKeyAlgorithms().stream()
+ .map(Factory.Named::getName).collect(Collectors.toUnmodifiableSet());
Review comment:
Unfortunately `Collectors.toUnmodifiableSet()` is not supported on Java 8, so GitHub automated builds failed. Switching to `Collectors.toSet()` and wrapping it with `Collections.unmodifableSet()` should work for both Java 8 and 11.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +712,44 @@ public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddre
return sftpClient;
}
+ void updateConfigAlgorithms(Config config) {
+ if(ctx.getProperty(CIPHERS_ALLOWED).isSet()) {
+ Set<String> allowedCiphers = Arrays.stream(ctx.getProperty(CIPHERS_ALLOWED).evaluateAttributeExpressions().getValue().split(","))
+ .map(String::trim)
+ .collect(Collectors.toSet());
Review comment:
To reduce duplication and ensure the same behavior, what do you think about refactoring similar property processing behavior to a shared method? Something along the following lines:
```
private Set<String> getCommaSeparatedValues(final PropertyValue propertyValue) {
Set<String> values = Collections.emptySet();
if (propertyValue.isSet()) {
values = Arrays.stream(propertyValue.evaluateAttributeExpressions().getValue().split(","))
.map(String::trim)
.collect(Collectors.toSet());
}
return values;
}
```
It may also be possible to create a shared method for collected allowed values, but at least creating a shared method for parsing property values would be helpful.
--
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r651115263
##########
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:
Done.
--
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r656171065
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +712,44 @@ public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddre
return sftpClient;
}
+ void updateConfigAlgorithms(Config config) {
+ if(ctx.getProperty(CIPHERS_ALLOWED).isSet()) {
+ Set<String> allowedCiphers = Arrays.stream(ctx.getProperty(CIPHERS_ALLOWED).evaluateAttributeExpressions().getValue().split(","))
+ .map(String::trim)
+ .collect(Collectors.toSet());
Review comment:
I think that's unnecessary. The stream api already reduces boiler plate and I'd still need to either check if each property was set or if the returned set was empty prior to using it to set a value.
If you were going to do something along these lines, I think a better use of your time would be to add some CSV capabilities to the PropertyValue class itself, allong with the StandardValidators.
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#issuecomment-841874983
Thanks for the updates, especially the Variable Registry.
LGTM
--
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r628795647
##########
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,43 @@
.required(true)
.build();
+ public static final PropertyDescriptor KEY_ALGORITHMS_ALLOWED = new PropertyDescriptor.Builder()
Review comment:
Recommend adding support for Variable Registry EL support to the new properties.
--
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on a change in pull request #5061: NIFI-8523 Update secure ftp processors to make configurable which algorithms, ciphers and message authentication codes are allowed to be used by the SSH Client.
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on a change in pull request #5061:
URL: https://github.com/apache/nifi/pull/5061#discussion_r656171065
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
##########
@@ -642,6 +712,44 @@ public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddre
return sftpClient;
}
+ void updateConfigAlgorithms(Config config) {
+ if(ctx.getProperty(CIPHERS_ALLOWED).isSet()) {
+ Set<String> allowedCiphers = Arrays.stream(ctx.getProperty(CIPHERS_ALLOWED).evaluateAttributeExpressions().getValue().split(","))
+ .map(String::trim)
+ .collect(Collectors.toSet());
Review comment:
I think that's unnecessary. The stream api already reduces boiler plate and I'd need to either check if each property was set or if the returned set was empty prior to using it to set a value.
If you were going to do something along these lines, I think a better use of your time would be to add some CSV capabilities to the PropertyValue class itself, allong with the StandardValidators.
--
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:
users@infra.apache.org