You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/04/06 01:37:55 UTC
[GitHub] [knox] moresandeep opened a new pull request #304: KNOX-2315 - Fix
zookeeper Kerberos Auth
moresandeep opened a new pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304
## 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.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep merged pull request #304: KNOX-2315 - Fix
zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r403914761
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -127,9 +156,18 @@ private static void ensureEntry(final String path, final RemoteConfigurationRegi
// content may not be trustworthy.
if (remoteClient.isAuthenticationConfigured()) {
LOG.correctingSuspectWritableRemoteConfigurationEntry(path);
-
// Replace the existing ACL with one that permits only authenticated users
- remoteClient.setACL(path, Collections.singletonList(AUTHENTICATED_USERS_ALL));
+ if (remoteClient.authenticationType()
Review comment:
`.authenticationType()` may be NULL (see `LocalFileSystemRemoteConfigurationRegistryClientService` implementation below) -> `remoteClient.authenticationType().equalsIgnoreCase("Kerberos")` may throw a NPE.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404994220
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
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.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404507514
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
+ .getAuthType().equalsIgnoreCase("Kerberos") && !config
Review comment:
Yeah it should be, I will address it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r405008770
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
Interesting, I did not know equalsIgnoreCase() is null safe, learnt something today :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404351178
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -127,9 +156,18 @@ private static void ensureEntry(final String path, final RemoteConfigurationRegi
// content may not be trustworthy.
if (remoteClient.isAuthenticationConfigured()) {
LOG.correctingSuspectWritableRemoteConfigurationEntry(path);
-
// Replace the existing ACL with one that permits only authenticated users
- remoteClient.setACL(path, Collections.singletonList(AUTHENTICATED_USERS_ALL));
+ if (remoteClient.authenticationType()
Review comment:
Could authenticationType() return null?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404973322
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
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).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404869557
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
`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.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404508310
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -127,9 +156,18 @@ private static void ensureEntry(final String path, final RemoteConfigurationRegi
// content may not be trustworthy.
if (remoteClient.isAuthenticationConfigured()) {
LOG.correctingSuspectWritableRemoteConfigurationEntry(path);
-
// Replace the existing ACL with one that permits only authenticated users
- remoteClient.setACL(path, Collections.singletonList(AUTHENTICATED_USERS_ALL));
+ if (remoteClient.authenticationType()
Review comment:
I don't think it would be l will fix this, we need to guard against NPEs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r403911268
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/config/RemoteConfigurationRegistry.java
##########
@@ -32,6 +32,8 @@
private String keyTab;
private boolean useKeyTab;
private boolean useTicketCache;
+ /* If true ensures that the auth scheme used to create znodes is `auth` and not `sasl` */
+ private boolean isBackwardsCompatible;
Review comment:
The field name should be simply `backwardCompatible` (the method name `boolean isBackwardCompatible()`)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404352390
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/RemoteConfigurationRegistryJAASConfig.java
##########
@@ -164,8 +164,8 @@ private AppConfigurationEntry createEntry(RemoteConfigurationRegistryConfig conf
}
break;
case Kerberos:
- opts.put("isUseTicketCache", String.valueOf(config.isUseTicketCache()));
- opts.put("isUseKeyTab", String.valueOf(config.isUseKeyTab()));
+ opts.put("useTicketCache", String.valueOf(config.isUseTicketCache()));
Review comment:
That looks like it was the result of an overly aggressive find-and-replace
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404349002
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
+ .getAuthType().equalsIgnoreCase("Kerberos") && !config
Review comment:
Should "Kerberos" be a constant since it is also used elsewhere?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404350707
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
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
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404507837
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
Hmm, agreed that would make things simpler and easier to read as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404108495
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/config/RemoteConfigurationRegistry.java
##########
@@ -32,6 +32,8 @@
private String keyTab;
private boolean useKeyTab;
private boolean useTicketCache;
+ /* If true ensures that the auth scheme used to create znodes is `auth` and not `sasl` */
+ private boolean isBackwardsCompatible;
Review comment:
The `is` prefix is to indicate that this is a boolean, just a habit. The config variable exposed to users is `backwardCompatible`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404111034
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -127,9 +156,18 @@ private static void ensureEntry(final String path, final RemoteConfigurationRegi
// content may not be trustworthy.
if (remoteClient.isAuthenticationConfigured()) {
LOG.correctingSuspectWritableRemoteConfigurationEntry(path);
-
// Replace the existing ACL with one that permits only authenticated users
- remoteClient.setACL(path, Collections.singletonList(AUTHENTICATED_USERS_ALL));
+ if (remoteClient.authenticationType()
Review comment:
` LocalFileSystemRemoteConfigurationRegistryClientService` is used only for testing, that is why it returns null, but it is better to defend agains NPEs, will fix it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404973322
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/CuratorClientService.java
##########
@@ -124,7 +125,14 @@ private RemoteConfigurationRegistryClient createClient(RemoteConfigurationRegist
ACLProvider aclProvider;
if (config.isSecureRegistry()) {
configureSasl(config);
- aclProvider = new SASLOwnerACLProvider();
+ if (!StringUtils.isBlank(config.getAuthType()) && config
Review comment:
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 ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #304:
KNOX-2315 - Fix zookeeper Kerberos Auth
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #304: KNOX-2315 - Fix zookeeper Kerberos Auth
URL: https://github.com/apache/knox/pull/304#discussion_r404352390
##########
File path: gateway-service-remoteconfig/src/main/java/org/apache/knox/gateway/service/config/remote/zk/RemoteConfigurationRegistryJAASConfig.java
##########
@@ -164,8 +164,8 @@ private AppConfigurationEntry createEntry(RemoteConfigurationRegistryConfig conf
}
break;
case Kerberos:
- opts.put("isUseTicketCache", String.valueOf(config.isUseTicketCache()));
- opts.put("isUseKeyTab", String.valueOf(config.isUseKeyTab()));
+ opts.put("useTicketCache", String.valueOf(config.isUseTicketCache()));
Review comment:
That looks like it was the result of an overly aggressive find-and-replace. Good catch!
----------------------------------------------------------------
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
With regards,
Apache Git Services