NIFI-5752: Load balancing fails with wildcard certs by kotarot · Pull Request #3110 · apache/nifi

kotarot · 2018-10-25T10:49:11Z

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:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
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.
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?
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?
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.

ijokarumawak

Thanks @kotarot ! I think supporting wildcard cert is critical especially for better scalability. I'd like to hear comments from @markap14 , too, on what data we need to use for RECEIVE provenance events. I think using hostnames derived from socket address would be the most reliable one.

ijokarumawak · 2018-10-26T01:35:54Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+        final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
                .map(NodeIdentifier::getApiAddress)
-                .collect(Collectors.toSet());
+                .collect(Collectors.toList());


Is there any reason to use toList instead?

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.

ijokarumawak · 2018-10-26T01:47:54Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

-            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();


I think HostnameVerifier is thread-safe and can be an instance field instead of creating at each verification.

Good point. Instantiating in every iteration is wasteful. I'll fix it!

ijokarumawak · 2018-10-26T01:54:11Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+        for (final String nodeId : nodeIds) {
+            final HostnameVerifier verifier = new DefaultHostnameVerifier();
+            if (verifier.verify(nodeId, sslSession)) {
+                logger.debug("Authorizing Client to Load Balance data");


In a case where the cert contains exact nodeId, the nodeId is still informative to be logged. I'd suggest logging message something like:

Suggested change

logger.debug("Authorizing Client to Load Balance data");

logger.debug("The request was verified with node ID '{}'. Authorizing Client to Load Balance data", nodeId);

I agree with your idea. I'll fix it so!

ijokarumawak · 2018-10-26T02:09:24Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+            final HostnameVerifier verifier = new DefaultHostnameVerifier();
+            if (verifier.verify(nodeId, sslSession)) {
+                logger.debug("Authorizing Client to Load Balance data");
                return;


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 ?

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.

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!

kotarot

Thank you for your review @ijokarumawak san!
I just replied to your comments. I'll fix the code later.

kotarot · 2018-10-26T07:25:28Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+        final List<String> nodeIds = clusterCoordinator.getNodeIdentifiers().stream()
                .map(NodeIdentifier::getApiAddress)
-                .collect(Collectors.toSet());
+                .collect(Collectors.toList());


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.

kotarot · 2018-10-26T07:26:05Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

-            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();


Good point. Instantiating in every iteration is wasteful. I'll fix it!

kotarot · 2018-10-26T07:26:10Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+        for (final String nodeId : nodeIds) {
+            final HostnameVerifier verifier = new DefaultHostnameVerifier();
+            if (verifier.verify(nodeId, sslSession)) {
+                logger.debug("Authorizing Client to Load Balance data");


I agree with your idea. I'll fix it so!

kotarot · 2018-10-26T07:27:09Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+            final HostnameVerifier verifier = new DefaultHostnameVerifier();
+            if (verifier.verify(nodeId, sslSession)) {
+                logger.debug("Authorizing Client to Load Balance data");
                return;


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.

kotarot · 2018-10-31T09:31:21Z

@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 NIFI-5746: Use Node Identifier's node address instead of getting from… #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?

ijokarumawak

@kotarot Thanks for the updates, this PR now looks good. I was able to test load-balancing with a cluster using a wild card cert. I posted a few minor comments, though. Once those get addressed, I will merge this. Thanks again!

ijokarumawak · 2018-11-02T07:20:34Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+            }
+        }
+
+        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");


We don't have to use String.format() here, please the String to logger.warn() directly.

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.

ijokarumawak · 2018-11-02T07:24:06Z

...ain/java/org/apache/nifi/controller/queue/clustered/server/ClusterLoadBalanceAuthorizer.java

+
+        logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);
+
        if (clientIdentities == null) {


Now we only call this authorize() method if socket is a SSLSocket. We can remove this block.

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.

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.

@ijokarumawak OK, I get it now. Thanks for kindly telling me that. I pushed a new commit. Please check it. Thanks!

kotarot · 2018-11-05T05:35:17Z

@ijokarumawak Thank you again for your review. I fixed the PR based on your comments. Can you check it?

NIFI-5752: Remove an unnecessary block

ijokarumawak · 2018-11-07T08:13:06Z

It looks good. +1. Merging. Thanks, @kotarot!

kotarot · 2018-11-07T09:15:12Z

@ijokarumawak Thanks for reviewing and merging my PR!

ijokarumawak reviewed Oct 26, 2018

View reviewed changes

kotarot commented Oct 26, 2018

View reviewed changes

NIFI-5752: Load balancing fails with wildcard certs

e2a697a

kotarot force-pushed the NIFI-5752 branch from 256edff to e2a697a Compare October 31, 2018 09:27

ijokarumawak reviewed Nov 2, 2018

View reviewed changes

NIFI-5752: Remove an unnecessary String.format

8407cde

NIFI-5752: Remove an unnecessary block

kotarot force-pushed the NIFI-5752 branch from 7372dda to 8407cde Compare November 7, 2018 06:56

asfgit closed this in 13232c7 Nov 7, 2018

	logger.debug("Authorizing Client to Load Balance data");
	logger.debug("The request was verified with node ID '{}'. Authorizing Client to Load Balance data", nodeId);


		logger.debug("Will perform authorization against Client Identities '{}'", clientIdentities);

		if (clientIdentities == null) {

Conversation

kotarot commented Oct 25, 2018

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

ijokarumawak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotarot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotarot commented Oct 31, 2018

Uh oh!

ijokarumawak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotarot commented Nov 5, 2018

Uh oh!

ijokarumawak commented Nov 7, 2018

Uh oh!

kotarot commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants