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 2020/09/11 18:50:41 UTC
[GitHub] [nifi-registry] kevdoran opened a new pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
kevdoran opened a new pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302
- Adds detection and encoding of non-ascii characters to creation of chain
- Adds unit tests and integration tests that use proxied entities with Unicode
- Refactors ProxiedEntityRequestConfig to compute header value in constructor
## Overview of these changes.
This changes the creation the `X-ProxiedEntitiesChain` header values to:
1. Detect non-ascii characters in an input identity/DN
2. If non-ascii characters are present, base64 encode the value and wrap it in additional `<angle braces>`
For example, a proxy chain of
```Алйс, CN=nifi.apache.org```
Will result in:
```X-ProxiedEntitiesChain: <<0JDQu9C50YE=>><CN=nifi.apache.org>```
Additionally, logic for tokenizing these header values was updated to:
1. Detect if a token is encoded (by the presence of additional `<angle braces>`)
2. If encoded, decode it as part of the tokenization process.
These changes should be completely backwards compatible, as entities containing non-ascii characters are encoded exactly the same as they previously were. (Previously, entities containing non-ascii characters would not survive a round trip hop between nodes, as per [NIFI-7744](https://issues.apache.org/jira/browse/NIFI-7744), so I am not worried about that case.)
A number of implementation approaches were considered, including ones based on RFC-2231 and RFC-8187. Ultimately, approaches that relied on percent encoding were rejected as percent-encoding outside of URL encoding are not widespread in standard libraries and difficult to implement correctly given complexity of unicode and things such as surragote characters. Given that in theory, third-party components such as proxies may need to implement this logic, base64 was chosen as is it widely available in all languages and frameworks (particularly in reverse proxies, where base64 is used as part of basic auth header formation).
The use of double `<<angle braces>>` to indicate encoding was chosen as it allows for an easy way for a decoder to determine if the value is encoded (necessary because encoding is optional for purely ascii entities), and because the reserved characters `<` and `>` are already protected/escaped in our sanitization process.
I considered but decided against always base64 encoding the header value, because (1) this maintains backwards compatibility between nifi and registry on different versions and (2) plaintext, non-encoded values, which are still the majority of use-cases, are easier for users to troubleshoot.
If this approach is satisfactory, let me know and I will make corresponding changes in NiFi. As it stands, I will have to re-implement most of this in the NiFi. Another option would be extracting most of this logic into a more sharable module in NiFi registry (or move to `nifi-registry-utils` perhaps), so that it could be imported and used by NiFi. I decided against this for now, as ultimately this is a good candidate for `nifi-standard-libs` and refactoring short-term would be more messy/risky (the current implementation in `nifi-registry-security-utils` requires apache commons which `nifi-registry-utils` does not have).
---
Thank you for submitting a contribution to Apache NiFi Registry.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFIREG-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFIREG-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] 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._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi-registry` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] 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-registry-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-registry-assembly`?
### 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 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-registry] alopresto commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490509341
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
Review comment:
I think this accepts `String[]` because that's how the values were previously stored, but now that the field is a plain `String` in the config object, does it make sense to just use `List<String>` here for consistency & convenience?
----------------------------------------------------------------
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-registry] kevdoran commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-707231581
@alopresto I know you are busy with other items, but if you have a chance in the next couple weeks, would be great to get a sign-off on this
----------------------------------------------------------------
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-registry] alopresto commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490510560
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
+ if (proxiedEntities == null) {
+ return null;
+ }
+
+ final List<String> proxiedEntityChain = Arrays.stream(proxiedEntities)
+ .map(ProxiedEntitiesUtils::formatProxyDn)
+ .collect(Collectors.toList());
+ return StringUtils.join(proxiedEntityChain, "");
+ }
+
+ /**
+ * Tokenizes the specified proxy chain.
+ *
+ * @param rawProxyChain raw chain
+ * @return tokenized proxy chain
+ */
+ public static List<String> tokenizeProxiedEntitiesChain(final String rawProxyChain) {
+ final List<String> proxyChain = new ArrayList<>();
+ if (!StringUtils.isEmpty(rawProxyChain)) {
+ // Split the String on the >< token
+ rawProxyChain.split("><");
Review comment:
What does this unsaved split call do?
----------------------------------------------------------------
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-registry] alopresto commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490514473
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
+ if (proxiedEntities == null) {
+ return null;
+ }
+
+ final List<String> proxiedEntityChain = Arrays.stream(proxiedEntities)
+ .map(ProxiedEntitiesUtils::formatProxyDn)
+ .collect(Collectors.toList());
+ return StringUtils.join(proxiedEntityChain, "");
+ }
+
+ /**
+ * Tokenizes the specified proxy chain.
+ *
+ * @param rawProxyChain raw chain
+ * @return tokenized proxy chain
+ */
+ public static List<String> tokenizeProxiedEntitiesChain(final String rawProxyChain) {
+ final List<String> proxyChain = new ArrayList<>();
+ if (!StringUtils.isEmpty(rawProxyChain)) {
+ // Split the String on the >< token
+ rawProxyChain.split("><");
+ List<String> elements = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(rawProxyChain, "><"));
+
+ // Remove the leading < from the first element
+ elements.set(0, elements.get(0).replaceFirst(LT, ""));
Review comment:
I'm a little confused by this process; if the first `<` is removed, shouldn't the last element always have a second trailing `>` to maintain pairing?
Also, could this be simplified to `Arrays.asList(StringUtils.splitBy...(rawProxyChain.substring(1, rawProxyChain.length() - 1))` and avoid the first/last element modification?
----------------------------------------------------------------
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-registry] kevdoran commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-694903957
Thanks for the review, @alopresto. I've updated my PR to incorporate your feedback. Turns out the weird handling of leading/trailing `<`/`>` was to properly tokenize the anonymous proxied entity `<>`, so I had to account for that in order to apply your simplification, but I think that is more readable and explicit this way. I also added some test cases that specifically cover that.
----------------------------------------------------------------
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-registry] kevdoran commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490533774
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
+ if (proxiedEntities == null) {
+ return null;
+ }
+
+ final List<String> proxiedEntityChain = Arrays.stream(proxiedEntities)
+ .map(ProxiedEntitiesUtils::formatProxyDn)
+ .collect(Collectors.toList());
+ return StringUtils.join(proxiedEntityChain, "");
+ }
+
+ /**
+ * Tokenizes the specified proxy chain.
+ *
+ * @param rawProxyChain raw chain
+ * @return tokenized proxy chain
+ */
+ public static List<String> tokenizeProxiedEntitiesChain(final String rawProxyChain) {
+ final List<String> proxyChain = new ArrayList<>();
+ if (!StringUtils.isEmpty(rawProxyChain)) {
+ // Split the String on the >< token
+ rawProxyChain.split("><");
Review comment:
This was unintentional. I had started refactoring code to remove the dep on commons, but the commons method semantics are necessary. Good catch, I will remove this.
----------------------------------------------------------------
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-registry] alopresto commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490514473
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
+ if (proxiedEntities == null) {
+ return null;
+ }
+
+ final List<String> proxiedEntityChain = Arrays.stream(proxiedEntities)
+ .map(ProxiedEntitiesUtils::formatProxyDn)
+ .collect(Collectors.toList());
+ return StringUtils.join(proxiedEntityChain, "");
+ }
+
+ /**
+ * Tokenizes the specified proxy chain.
+ *
+ * @param rawProxyChain raw chain
+ * @return tokenized proxy chain
+ */
+ public static List<String> tokenizeProxiedEntitiesChain(final String rawProxyChain) {
+ final List<String> proxyChain = new ArrayList<>();
+ if (!StringUtils.isEmpty(rawProxyChain)) {
+ // Split the String on the >< token
+ rawProxyChain.split("><");
+ List<String> elements = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(rawProxyChain, "><"));
+
+ // Remove the leading < from the first element
+ elements.set(0, elements.get(0).replaceFirst(LT, ""));
Review comment:
I'm a little confused by this process; if the first `<` is removed, shouldn't the last element always have a second trailing `>` (that must be removed to maintain pairing)?
Also, could this be simplified to `Arrays.asList(StringUtils.splitBy...(rawProxyChain.substring(1, rawProxyChain.length() - 1))` and avoid the first/last element modification?
----------------------------------------------------------------
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-registry] kevdoran commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-692225242
Rebased on `main` to (hopefully) fix GHA CI builds
----------------------------------------------------------------
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-registry] kevdoran commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490534340
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
Review comment:
Yep, good call. Will change
----------------------------------------------------------------
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-registry] kevdoran commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-698367045
Whoops, missed some changes in my last commit. Updated and should be good now.
----------------------------------------------------------------
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-registry] bbende commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-707760615
Updates look good, all integration tests passing locally, going to merge, 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-registry] kevdoran commented on pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#issuecomment-698367045
Whoops, missed some changes in my last commit. Updated and should be good now.
----------------------------------------------------------------
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-registry] asfgit closed pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302
----------------------------------------------------------------
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-registry] kevdoran commented on a change in pull request #302: NIFIREG-415 Add Support for Unicode in X-ProxiedEntitiesChain
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #302:
URL: https://github.com/apache/nifi-registry/pull/302#discussion_r490535141
##########
File path: nifi-registry-core/nifi-registry-security-utils/src/main/java/org/apache/nifi/registry/security/util/ProxiedEntitiesUtils.java
##########
@@ -39,6 +41,55 @@
private static final String ANONYMOUS_CHAIN = "<>";
+ /**
+ * Formats a list of DN/usernames to be set as a HTTP header using well known conventions.
+ *
+ * @param proxiedEntities the raw identities (usernames and DNs) to be formatted as a chain
+ * @return the value to use in the X-ProxiedEntitiesChain header
+ */
+ public static String getProxiedEntitiesChain(final String[] proxiedEntities) {
+ if (proxiedEntities == null) {
+ return null;
+ }
+
+ final List<String> proxiedEntityChain = Arrays.stream(proxiedEntities)
+ .map(ProxiedEntitiesUtils::formatProxyDn)
+ .collect(Collectors.toList());
+ return StringUtils.join(proxiedEntityChain, "");
+ }
+
+ /**
+ * Tokenizes the specified proxy chain.
+ *
+ * @param rawProxyChain raw chain
+ * @return tokenized proxy chain
+ */
+ public static List<String> tokenizeProxiedEntitiesChain(final String rawProxyChain) {
+ final List<String> proxyChain = new ArrayList<>();
+ if (!StringUtils.isEmpty(rawProxyChain)) {
+ // Split the String on the >< token
+ rawProxyChain.split("><");
+ List<String> elements = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(rawProxyChain, "><"));
+
+ // Remove the leading < from the first element
+ elements.set(0, elements.get(0).replaceFirst(LT, ""));
Review comment:
I left this logic unchanged (just moved it from down below to keep all the `public` methods at the top of the file and to keep it close to the reverse method which is `getProxiedEntitiesChain()`, but now that you point it out, I will look at possibly simplifying this when I make the other suggestions you had. 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