You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by thenatog <gi...@git.apache.org> on 2018/05/07 18:02:11 UTC
[GitHub] nifi pull request #2684: NIFI-5161 - Escape special characters in keystore a...
GitHub user thenatog opened a pull request:
https://github.com/apache/nifi/pull/2684
NIFI-5161 - Escape special characters in keystore alias for toolkit
Thank you for submitting a contribution to Apache NiFi.
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?
- [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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] 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?
- [ ] 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)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/thenatog/nifi NIFI-5161
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2684.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2684
----
commit e263209a8acaf7f5f38ba64a2204a988be04fcf9
Author: thenatog <th...@...>
Date: 2018-05-07T15:22:32Z
NIFI-5161 - Added a method to escape special characters in the alias name for keys in the truststore. This fixes an error with the TlsToolkit which occurs when extracting keys and writing them to file.
commit aaf9e7ebf79449a424bef2a599222e36f80d914e
Author: thenatog <th...@...>
Date: 2018-05-07T17:16:13Z
Added another test for special characters in the DN/output key filename.
commit 6474bf48d958a966ca9a17e972af8eecbf70e392
Author: thenatog <th...@...>
Date: 2018-05-07T17:24:50Z
NIFI-5161 - Cleaning up a bit
----
---
[GitHub] nifi pull request #2684: NIFI-5161 - Escape special characters in keystore a...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2684
---
[GitHub] nifi issue #2684: NIFI-5161 - Escape special characters in keystore alias fo...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2684
Reviewing...
---
[GitHub] nifi pull request #2684: NIFI-5161 - Escape special characters in keystore a...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2684#discussion_r186503676
--- Diff: nifi-toolkit/nifi-toolkit-tls/src/main/java/org/apache/nifi/toolkit/tls/standalone/TlsToolkitStandalone.java ---
@@ -236,6 +236,6 @@ public void createNifiKeystoresAndTrustStores(StandaloneConfig standaloneConfig)
}
protected static String getClientDnFile(String clientDn) {
- return clientDn.replace(',', '_').replace(' ', '_');
+ return clientDn.replace(',', '_').replace(' ', '_').replaceAll("[^\\w&&[^\\.]&&[^=]]", "_");
--- End diff --
This is also performing 3 different String replacement actions, which hurts performance. We should replace this with a single compiled `Pattern` using regex and perform the replacements at once.
---
[GitHub] nifi issue #2684: NIFI-5161 - Escape special characters in keystore alias fo...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2684
Aside from checkstyle fixes and some small notes on regex, the module builds successfully and all tests pass. When these changes are made, I will give +1 and merge.
---
[GitHub] nifi issue #2684: NIFI-5161 - Escape special characters in keystore alias fo...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2684
This is merged. Congratulations on your first contribution to Apache NiFi.
---
[GitHub] nifi pull request #2684: NIFI-5161 - Escape special characters in keystore a...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2684#discussion_r186502982
--- Diff: nifi-toolkit/nifi-toolkit-tls/src/main/java/org/apache/nifi/toolkit/tls/manager/TlsClientManager.java ---
@@ -112,4 +112,9 @@ public void write(OutputStreamFactory outputStreamFactory) throws IOException, G
public void addClientConfigurationWriter(ConfigurationWriter<TlsClientConfig> configurationWriter) {
configurationWriters.add(configurationWriter);
}
+
+
+ protected static String escapeAliasFilename(String alias) {
+ return alias.replaceAll("[^\\w&&[^\\.]]", "_");
--- End diff --
Recommend simplifying regular expression to `[^\\w\\.\\-\\n]+`.
Examples:
```
my/silly/filename my_silly_filename
my&silly&filename my_silly_filename
my silly alias my_silly_alias
my_silly_alias my_silly_alias
my.silly.alias my.silly.alias
my-silly-alias my-silly-alias
my silly alias my_silly_alias
my&/ silly*&$alias my_silly_alias
```
---
[GitHub] nifi issue #2684: NIFI-5161 - Escape special characters in keystore alias fo...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2684
Ran `contrib-check` and all tests pass. +1, merging.
---
[GitHub] nifi issue #2684: NIFI-5161 - Escape special characters in keystore alias fo...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2684
There are 8 checkstyle failures. You can identify these in IntelliJ using the `nifi-checkstyle.xml` file available on the Apache NiFi Wiki in the Contributor Guide, or on the command-line by running `mvn checkstyle:check`.
```
[INFO] --- maven-checkstyle-plugin:2.17:check (default-cli) @ nifi-toolkit-tls ---
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[3,8] (imports) UnusedImports: Unused import - org.apache.nifi.toolkit.tls.configuration.TlsClientConfig.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[4,8] (imports) UnusedImports: Unused import - org.apache.nifi.toolkit.tls.configuration.TlsConfig.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[5,8] (imports) UnusedImports: Unused import - org.junit.After.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[6,8] (imports) UnusedImports: Unused import - org.junit.Before.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[9,8] (imports) UnusedImports: Unused import - java.io.IOException.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[11,8] (imports) UnusedImports: Unused import - java.lang.reflect.Method.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[12,8] (imports) UnusedImports: Unused import - java.security.GeneralSecurityException.
[WARNING] src/test/java/org/apache/nifi/toolkit/tls/manager/TlsClientManagerTest.java:[14] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - org.junit.Assert.*.
```
---
[GitHub] nifi pull request #2684: NIFI-5161 - Escape special characters in keystore a...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2684#discussion_r186503174
--- Diff: nifi-toolkit/nifi-toolkit-tls/src/main/java/org/apache/nifi/toolkit/tls/standalone/TlsToolkitStandalone.java ---
@@ -236,6 +236,6 @@ public void createNifiKeystoresAndTrustStores(StandaloneConfig standaloneConfig)
}
protected static String getClientDnFile(String clientDn) {
- return clientDn.replace(',', '_').replace(' ', '_');
+ return clientDn.replace(',', '_').replace(' ', '_').replaceAll("[^\\w&&[^\\.]&&[^=]]", "_");
--- End diff --
Since the regular expression is being used in multiple places, I would recommend making this a `public static final` constant in one of the toolkit utility classes.
---