You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by kotarot <gi...@git.apache.org> on 2018/10/25 10:49:14 UTC
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
GitHub user kotarot opened a pull request:
https://github.com/apache/nifi/pull/3110
NIFI-5752: Load balancing fails with wildcard certs
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)?
- [X] Is your initial contribution a single, squashed commit?
### 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?
- [ ] 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/kotarot/nifi NIFI-5752
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3110.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 #3110
----
commit 256edff0c4515c94093030efc8ad20e45819b963
Author: Kotaro Terada <ko...@...>
Date: 2018-10-25T10:46:06Z
NIFI-5752: Load balancing fails with wildcard certs
----
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228534325
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
+ if (verifier.verify(nodeId, sslSession)) {
+ logger.debug("Authorizing Client to Load Balance data");
return;
--- End diff --
I think falling back to the hostname from the socket whenever there is not an exact match (i.e., the wildcard matches but not an exact string comparison) is fair. Originally, we used the hostname directly from the socket, but as Koji mentioned in #3109 we changed that behavior. This was done because when you look at Provenance data (and in logs), what you may see is something like a RECEIVE event with a transit URI of nifi://s7302.r720.y8302.mydomain.com because that's the FQDN but the user typically references this node as say nifi-01.mydomain.com. If the UI shows the node as nifi-01.mydomain.com in the cluster table, then it is best to show that in the Provenance and logs as well. This is especially true in virtual environments, running in Docker or in a publish Cloud/VM where often the hostname reported by socket.getInetAddress() is very different than what we typically like to see.
Does that make sense?
Also @kotarot thank you for noticing this and submitting this contribution!
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r231397141
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -33,14 +42,27 @@
private final ClusterCoordinator clusterCoordinator;
private final EventReporter eventReporter;
+ private final HostnameVerifier hostnameVerifier;
public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator, final EventReporter eventReporter) {
this.clusterCoordinator = clusterCoordinator;
this.eventReporter = eventReporter;
+ this.hostnameVerifier = new DefaultHostnameVerifier();
}
@Override
- public String authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
+ public String authorize(SSLSocket sslSocket) throws NotAuthorizedException, IOException {
+ final SSLSession sslSession = sslSocket.getSession();
+
+ final Set<String> clientIdentities;
+ try {
+ clientIdentities = getCertificateIdentities(sslSession);
+ } catch (final CertificateException e) {
+ throw new IOException("Failed to extract Client Certificate", e);
+ }
+
+ logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);
+
if (clientIdentities == null) {
--- End diff --
@ijokarumawak OK, I get it now. Thanks for kindly telling me that. I pushed a new commit. Please check it. Thanks!
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228384110
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
--- End diff --
Is there any reason to use `toList` instead?
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r230287180
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -57,11 +79,35 @@ public String authorize(final Collection<String> clientIdentities) throws NotAut
}
}
- final String message = String.format("Authorization failed for Client ID's %s to Load Balance data because none of the ID's are known Cluster Node Identifiers",
- clientIdentities);
+ // If there are no matches of Client IDs, try to verify it by HostnameVerifier. In this way, we can support wildcard certificates.
+ for (final String nodeId : nodeIds) {
+ if (hostnameVerifier.verify(nodeId, sslSession)) {
+ final String clientId = sslSocket.getInetAddress().getHostName();
+ logger.debug("The request was verified with node '{}'. The hostname derived from the socket is '{}'. Authorizing Client to Load Balance data", nodeId, clientId);
+ return clientId;
+ }
+ }
+
+ final String message = String.format("Authorization failed for Client ID's to Load Balance data because none of the ID's are known Cluster Node Identifiers");
--- End diff --
We don't have to use `String.format()` here, please the String to `logger.warn()` directly.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/3110
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228428588
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
+ if (verifier.verify(nodeId, sslSession)) {
+ logger.debug("Authorizing Client to Load Balance data");
return;
--- End diff --
In my opinion, we just need to use HostnameVerifier to verify and use the hostname derived from the socket. The reason is that, anyway, HostnameVerifier could simply authorize a node using certs w/ or w/o wildcard, and I think the hostname derived from the socket is enough. If there are cases where the hostname derived from the socket and the hostname from Certificate Identities are different, please ignore my option.
I'd also like to hear comment from @markap14 . Thank you.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228428335
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
+ if (verifier.verify(nodeId, sslSession)) {
+ logger.debug("Authorizing Client to Load Balance data");
--- End diff --
I agree with your idea. I'll fix it so!
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r231011542
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -33,14 +42,27 @@
private final ClusterCoordinator clusterCoordinator;
private final EventReporter eventReporter;
+ private final HostnameVerifier hostnameVerifier;
public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator, final EventReporter eventReporter) {
this.clusterCoordinator = clusterCoordinator;
this.eventReporter = eventReporter;
+ this.hostnameVerifier = new DefaultHostnameVerifier();
}
@Override
- public String authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
+ public String authorize(SSLSocket sslSocket) throws NotAuthorizedException, IOException {
+ final SSLSession sslSession = sslSocket.getSession();
+
+ final Set<String> clientIdentities;
+ try {
+ clientIdentities = getCertificateIdentities(sslSession);
+ } catch (final CertificateException e) {
+ throw new IOException("Failed to extract Client Certificate", e);
+ }
+
+ logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);
+
if (clientIdentities == null) {
--- End diff --
The existing log message indicating that this block is meant for the case where NiFi clustering is not secured (not sending data via SSLSocket). This block contradicts with the other block such as following getCertificateIdentities method:
```
final Certificate[] certs = sslSession.getPeerCertificates();
if (certs == null || certs.length == 0) {
throw new SSLPeerUnverifiedException("No certificates found");
}
```
If we care about `clientIdentities` being null, then we should throw `SSLPeerUnverifiedException("No client identities found");` instead of authorizing it. Having said that, I believe removing this block is safe as `clientIdentities` are populated using `Collectors.toSet`. If no SAN is found, the value will be an empty set instead of null.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228428134
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
--- End diff --
In the existing code, we needed to search a target element in `Set<String> nodeIds` by the `contains` method. However, in this change, `nodeIds` is just iterated in a loop, so it is reasonable to change it `List`.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r230287682
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -33,14 +42,27 @@
private final ClusterCoordinator clusterCoordinator;
private final EventReporter eventReporter;
+ private final HostnameVerifier hostnameVerifier;
public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator, final EventReporter eventReporter) {
this.clusterCoordinator = clusterCoordinator;
this.eventReporter = eventReporter;
+ this.hostnameVerifier = new DefaultHostnameVerifier();
}
@Override
- public String authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
+ public String authorize(SSLSocket sslSocket) throws NotAuthorizedException, IOException {
+ final SSLSession sslSession = sslSocket.getSession();
+
+ final Set<String> clientIdentities;
+ try {
+ clientIdentities = getCertificateIdentities(sslSession);
+ } catch (final CertificateException e) {
+ throw new IOException("Failed to extract Client Certificate", e);
+ }
+
+ logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);
+
if (clientIdentities == null) {
--- End diff --
Now we only call this `authorize()` method if socket is a SSLSocket. We can remove this block.
---
[GitHub] nifi issue #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/3110
It looks good. +1. Merging. Thanks, @kotarot!
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r230637063
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -57,11 +79,35 @@ public String authorize(final Collection<String> clientIdentities) throws NotAut
}
}
- final String message = String.format("Authorization failed for Client ID's %s to Load Balance data because none of the ID's are known Cluster Node Identifiers",
- clientIdentities);
+ // If there are no matches of Client IDs, try to verify it by HostnameVerifier. In this way, we can support wildcard certificates.
+ for (final String nodeId : nodeIds) {
+ if (hostnameVerifier.verify(nodeId, sslSession)) {
+ final String clientId = sslSocket.getInetAddress().getHostName();
+ logger.debug("The request was verified with node '{}'. The hostname derived from the socket is '{}'. Authorizing Client to Load Balance data", nodeId, clientId);
+ return clientId;
+ }
+ }
+
+ final String message = String.format("Authorization failed for Client ID's to Load Balance data because none of the ID's are known Cluster Node Identifiers");
--- End diff --
Thanks for pointing it out. I fixed it by just removing `String.format` in this line because the next line also uses the `message` variable.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r230637213
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -33,14 +42,27 @@
private final ClusterCoordinator clusterCoordinator;
private final EventReporter eventReporter;
+ private final HostnameVerifier hostnameVerifier;
public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator, final EventReporter eventReporter) {
this.clusterCoordinator = clusterCoordinator;
this.eventReporter = eventReporter;
+ this.hostnameVerifier = new DefaultHostnameVerifier();
}
@Override
- public String authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
+ public String authorize(SSLSocket sslSocket) throws NotAuthorizedException, IOException {
+ final SSLSession sslSession = sslSocket.getSession();
+
+ final Set<String> clientIdentities;
+ try {
+ clientIdentities = getCertificateIdentities(sslSession);
+ } catch (final CertificateException e) {
+ throw new IOException("Failed to extract Client Certificate", e);
+ }
+
+ logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);
+
if (clientIdentities == null) {
--- End diff --
Do you mean the block L66-69? Do we always guarantee `clientIdentities` is not null if the socket is a SSLSocket? I suppose we still need this.
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228385558
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
--- End diff --
I think HostnameVerifier is thread-safe and can be an instance field instead of creating at each verification.
---
[GitHub] nifi issue #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on the issue:
https://github.com/apache/nifi/pull/3110
@ijokarumawak Thanks for reviewing and merging my PR!
---
[GitHub] nifi issue #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on the issue:
https://github.com/apache/nifi/pull/3110
@markap14 Thank you for your kind advice! That makes sense to me.
In the new commit, I have left the existing authorization codes, followed by the authorization using `HostnameVerifier` (which I added). The authorization is performed as follows:
- If the authorization with the string-match succeeded, then returns the node information from Client Identities (as we fixed in #3109 ).
- After that, if the authorization by `HostnameVerifier` succeeded, then returns the derived hostname from the socket is returned.
Does this change seem to be no problem?
Also, I modified a few tests related to `LoadBalanceAuthorizer` because the interface of `authorize` is changed.
@ijokarumawak Could you please review it?
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228428297
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
--- End diff --
Good point. Instantiating in every iteration is wasteful. I'll fix it!
---
[GitHub] nifi issue #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by kotarot <gi...@git.apache.org>.
Github user kotarot commented on the issue:
https://github.com/apache/nifi/pull/3110
@ijokarumawak Thank you again for your review. I fixed the PR based on your comments. Can you check it?
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228387841
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
+ if (verifier.verify(nodeId, sslSession)) {
+ logger.debug("Authorizing Client to Load Balance data");
return;
--- End diff --
By #3109, we need to return the client peer description when authorization passes. For the best informative result for data provenance, we need to do:
- If any SAN exists in the known nodeIds, then return the matched SAN (this can be done by the existing code), this way, we can identify which node sent the request at best. (If the cert contains multiple nodeIds as SAN, this logic can be broken, but I believe that is a corner-case that we don't need to support)
- If none of SAN matches with any nodeId, then use hostname verifier to support wildcard cert. In this case, return hostname derived from the socket address
Alternatively, we just need to use the hostname verifier and use the hostname derived from the socket address in any case for provenance data. How do you think @markap14 ?
---
[GitHub] nifi pull request #3110: NIFI-5752: Load balancing fails with wildcard certs
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3110#discussion_r228386235
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java ---
@@ -40,28 +42,23 @@ public ClusterLoadBalanceAuthorizer(final ClusterCoordinator clusterCoordinator,
}
@Override
- public void authorize(final Collection<String> clientIdentities) throws NotAuthorizedException {
- if (clientIdentities == null) {
- logger.debug("Client Identities is null, so assuming that Load Balancing communications are not secure. Authorizing client to participate in Load Balancing");
- return;
- }
-
- final Set<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
+ public void authorize(final SSLSession sslSession) throws NotAuthorizedException {
+ final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
.map(NodeIdentifier::getApiAddress)
- .collect(Collectors.toSet());
+ .collect(Collectors.toList());
- for (final String clientId : clientIdentities) {
- if (nodeIds.contains(clientId)) {
- logger.debug("Client ID '{}' is in the list of Nodes in the Cluster. Authorizing Client to Load Balance data", clientId);
+ for (final String nodeId : nodeIds) {
+ final HostnameVerifier verifier = new DefaultHostnameVerifier();
+ if (verifier.verify(nodeId, sslSession)) {
+ logger.debug("Authorizing Client to Load Balance data");
--- End diff --
In a case where the cert contains exact nodeId, the `nodeId` is still informative to be logged. I'd suggest logging message something like:
```suggestion
logger.debug("The request was verified with node ID '{}'. Authorizing Client to Load Balance data", nodeId);
```
---