NIFI-7401 Add ZooKeeper client TLS to CuratorLeaderElectionManager by jfrazee · Pull Request #4250 · apache/nifi

jfrazee · 2020-05-01T20:04:53Z

Description of PR

This adds built-in support for ZooKeeper client TLS to the CuratorLeaderElectionManager so clusters can use TLS-enabled ZooKeepers. This is part of NIFI-7203 and is required for the TLS-enabled embedded ZooKeeper work in #4216.

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? 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 folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on both JDK 8 and 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?
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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

YolandaMDavis · 2020-09-02T16:29:49Z

@jfrazee was doing some research around this issue and found this PR. Does it need to be targeted to main or was this perhaps merged to main in the context of another PR?

jfrazee · 2020-09-02T17:09:20Z

@YolandaMDavis No it wasn't merged I have time to rebase and retest today actually.

I previously had a PR that included support for the embedded server too, but that got split apart because of some additional work and expanded scope.

jfrazee · 2020-09-08T17:24:27Z

@YolandaMDavis I rebased/retargeted this and ran through the manual testing.

thenatog · 2020-09-16T16:01:36Z

Reviewing this one, testing it out now. Also checking out #4216, not sure what the differences are there.

jfrazee · 2020-09-16T16:30:34Z

#4216 was intended to handle the embedded ZK path and is about forcing TLS for both the embedded server and the client whenever the embedded server is used.

thenatog · 2020-09-17T23:44:33Z

Tested the cluster leader election over TLS, looks good. I see there's nothing in this PR for state management over TLS. Is there any PRs open that you know of which cover TLS + state management or is that something I should create a ticket for and work on?

jfrazee · 2020-09-18T17:07:29Z

@thenatog No, I don't think anyone has worked on the state management/I don't have any code for that in progress either, so yeah, I think it'd make sense for you to do if you want.

.../nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/nifi.properties

thenatog · 2020-09-18T18:20:37Z

Right now for this PR I'm thinking we need some consolidation across the TLS + Zookeeper work. It seems like the embedded zookeeper + external zookeeper configuration isn't handled in the most cohesive way, which is fair because they've been developed separately, but it may result in unexpected behavior for users.

I'd like to get my PR for state management up before we merge this one so that all external-zookeeper interactions are secured, instead of only the cluster election. We may need to break out the SecureClientZooKeeperFactory class into a separate class and put it somewhere so it can be used by both election and state management.

It also looks like there needs to be more work on the embedded side to fix a few issues Joey raised in his review - I can look at adding some commits there next week.

jfrazee · 2020-09-18T19:34:06Z

@thenatog I think for SecureClientZooKeeperFactory to be usable in the state provider, it'd need a small refactor to use Curator, which actually looks more straightforward than I thought.

…ecurity.*

...ork-core/src/test/java/org/apache/nifi/leader/election/TestSecureClientZooKeeperFactory.java

exceptionfactory

Aside from some suggestions on implementation adjustments, it would be great to get this merged as a step to securing this fundamental aspect of cluster communication.

nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java

...i-framework-core/src/main/java/org/apache/nifi/controller/cluster/ZooKeeperClientConfig.java

exceptionfactory · 2020-10-10T01:33:35Z

...e/src/main/java/org/apache/nifi/controller/leader/election/CuratorLeaderElectionManager.java

+
+            // Netty is required for the secure client config.
+            final String cnxnSocket = zkConfig.getConnectionSocket();
+            if (!NETTY_CLIENT_CNXN_SOCKET.equals(cnxnSocket)) {


If the secure client requires the ClientCnxnSocketNetty, it seems more straightforward to set the correct value in zkSecureClientConfig and ignore the ZooKeeperClientConfig.getConnectionSocket() altogether, instead of confirming that it matches the only correct value in this scenario.

exceptionfactory · 2020-10-10T01:35:31Z

...e/src/main/java/org/apache/nifi/controller/leader/election/CuratorLeaderElectionManager.java

+
+            // This should never happen but won't get checked elsewhere.
+            final boolean clientSecure = zkConfig.getClientSecure();
+            if (!clientSecure) {


Similar to the previous comment, why not just set ZKClientConfig.SECURE_CLIENT to true regardless of ZooKeeperClientConfig, particularly since the value is already being checked before instantiating the class?

@exceptionfactory This is the one change I didn't make based on your suggestions (thanks for them BTW). While, yes, the secure factory knows what it needs, I don't/didn't want to ignore or change the config the user provides since their intent is either different or confused. I want them to know they're using it wrong.

Thanks for the explanation, that makes sense, glad to help move things forward.

...ork-core/src/test/java/org/apache/nifi/leader/election/TestSecureClientZooKeeperFactory.java

thenatog · 2020-10-19T16:47:24Z

+1 from me, keen to get this merged so I can fix up my PR so we can get it all merged it.

jfrazee mentioned this pull request May 1, 2020

NIFI-7203 Add ZooKeeper client and server TLS #4207

Closed

12 tasks

jfrazee force-pushed the NIFI-7401 branch from 96f0053 to 0575b37 Compare September 4, 2020 05:35

jfrazee changed the base branch from master to main September 4, 2020 05:38

thenatog reviewed Sep 18, 2020

View reviewed changes

.../nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/nifi.properties Show resolved Hide resolved

jfrazee added 3 commits September 29, 2020 08:51

NIFI-7401 Add ZooKeeper client TLS to CuratorLeaderElectionManager

3635008

NIFI-7401 Rebased to 1.13.0-SNAPSHOT and simplified tests

c99a05d

NIFI-7401 Added keystore types and changed properties to match nifi.s…

7b93b32

…ecurity.*

jfrazee force-pushed the NIFI-7401 branch from 0575b37 to 7b93b32 Compare September 29, 2020 20:21

thenatog reviewed Oct 7, 2020

View reviewed changes

...ork-core/src/test/java/org/apache/nifi/leader/election/TestSecureClientZooKeeperFactory.java Outdated Show resolved Hide resolved

NIFI-7401 Removed dead code from SecureClientZooKeeperFactory test

97541e0

thenatog mentioned this pull request Oct 9, 2020

NiFi-7819 - Add Zookeeper client TLS (external zookeeper) for cluster state management #4592

Closed

13 tasks

exceptionfactory reviewed Oct 10, 2020

View reviewed changes

jfrazee added 2 commits October 14, 2020 09:20

NIFI-7401 Renamed bean methods, moved helper code into NiFiProperties

b70e42e

NIFI-7401 Changed connection socket constants to use .class.getName()

04c9fcb

thenatog closed this Oct 20, 2020

Conversation

jfrazee commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

YolandaMDavis commented Sep 2, 2020

Uh oh!

jfrazee commented Sep 2, 2020

Uh oh!

jfrazee commented Sep 8, 2020

Uh oh!

thenatog commented Sep 16, 2020

Uh oh!

jfrazee commented Sep 16, 2020

Uh oh!

thenatog commented Sep 17, 2020

Uh oh!

jfrazee commented Sep 18, 2020

Uh oh!

Uh oh!

thenatog commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfrazee commented Sep 18, 2020

Uh oh!

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exceptionfactory Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

jfrazee Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thenatog commented Oct 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jfrazee commented May 1, 2020 •

edited

Loading

thenatog commented Sep 18, 2020 •

edited

Loading