Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KNOX-2315 - Fix zookeeper Kerberos Auth #304

Merged

Conversation

moresandeep
Copy link
Contributor

What changes were proposed in this pull request?

This PR fixes issues with Zookeeper Kerberos authentication. This PR contains following changes.

  1. Fix issues in code that were causing bad configuration issues, such as typos and wrong variable names.
  2. For Znodes created by Knox the permissions are more restricted and only knox users can read and write the data to Znodes owned by Knox.
  3. Auth scheme for Kerberos authentication is now "sasl"
  4. A configuration boolean parameter "backwardsCompatible" is introduced which falls back to old behavior (likely broken behavior)

How was this patch tested?

This patch was locally tested on a secure cluster with Kerberos authentication between Knox and Zookeeper.

@moresandeep moresandeep self-assigned this Apr 6, 2020
@moresandeep moresandeep added the bug label Apr 6, 2020
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
aclProvider = new SASLOwnerACLProvider();
if (!StringUtils.isBlank(config.getAuthType()) && config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've defined a method authenticationType(). Why not use that here? Could also have a utility method isKerberosAuth(final String authType) that performs this evaluation. There is nearly identical logic in ZookeeperRemoteAliasService

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, agreed that would make things simpler and easier to read as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authenticationType() method is defined in the private class ClientAdapter so we can't use it. That is also the reason that reusing this code is tricky.

Copy link
Contributor

@pzampino pzampino Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking you can define a static method in a class that is accessible to all that need to reference it, which takes a String (the authenticationType() result). Something in the gateway-spi module ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually may be easier to simply change it to AUTH_TYPE_KERBEROS.equalsIgnoreCase(config.getAuthType()), which would handle the blank or null value check too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I did not know equalsIgnoreCase() is null safe, learnt something today :)

@moresandeep moresandeep force-pushed the KNOX-2315_Zookeeper_Kerberos branch 2 times, most recently from 340b732 to 19e4ebc Compare April 7, 2020 18:08
Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moresandeep moresandeep merged commit 38b56bb into apache:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants