From b43442c58a1a66f1c17f889c081fea159caaebf5 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Mon, 1 Feb 2016 09:48:16 -0800 Subject: [PATCH] HBASE-15200 ZooKeeper znode ACL checks should only compare the shortname Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java --- .../hadoop/hbase/zookeeper/ZooKeeperWatcher.java | 79 ++++++++++++++++++++-- .../java/org/apache/hadoop/hbase/AuthUtil.java | 5 ++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java index 7b591f8..983153f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java @@ -24,12 +24,15 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; +import org.apache.hadoop.hbase.AuthUtil; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.ZooKeeperConnectionException; import org.apache.hadoop.security.UserGroupInformation; @@ -126,6 +129,9 @@ public class ZooKeeperWatcher implements Watcher, Abortable, Closeable { private final Exception constructorCaller; + /* A pattern that matches a Kerberos name, borrowed from Hadoop's KerberosName */ + private static final Pattern NAME_PATTERN = Pattern.compile("([^/@]*)(/([^/@]*))?@([^/@]*)"); + /** * Instantiate a ZooKeeper connection and watcher. * @param identifier string that is passed to RecoverableZookeeper to be used as @@ -218,6 +224,7 @@ public class ZooKeeperWatcher implements Watcher, Abortable, Closeable { */ public void checkAndSetZNodeAcls() { if (!ZKUtil.isSecureZooKeeper(getConfiguration())) { + LOG.info("not a secure deployment, proceeding"); return; } @@ -262,13 +269,23 @@ public class ZooKeeperWatcher implements Watcher, Abortable, Closeable { * @throws IOException */ private boolean isBaseZnodeAclSetup(List acls) throws IOException { - String superUser = conf.get("hbase.superuser"); + if (LOG.isDebugEnabled()) { + LOG.debug("Checking znode ACLs"); + } + String superUser = conf.get(AuthUtil.SUPERUSER_CONF_KEY); + // Check whether ACL set for all superusers + if (superUser != null && !checkACLForSuperUsers(new String[] { superUser }, acls)) { + return false; + } // this assumes that current authenticated user is the same as zookeeper client user // configured via JAAS String hbaseUser = UserGroupInformation.getCurrentUser().getShortUserName(); if (acls.isEmpty()) { + if (LOG.isDebugEnabled()) { + LOG.debug("ACL is empty"); + } return false; } @@ -279,23 +296,73 @@ public class ZooKeeperWatcher implements Watcher, Abortable, Closeable { // and one for the hbase user if (Ids.ANYONE_ID_UNSAFE.equals(id)) { if (perms != Perms.READ) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("permissions for '%s' are not correct: have %0x, want %0x", + id, perms, Perms.READ)); + } return false; } - } else if (superUser != null && new Id("sasl", superUser).equals(id)) { - if (perms != Perms.ALL) { - return false; + } else if ("sasl".equals(id.getScheme())) { + String name = id.getId(); + // If ZooKeeper recorded the Kerberos full name in the ACL, use only the shortname + Matcher match = NAME_PATTERN.matcher(name); + if (match.matches()) { + name = match.group(1); } - } else if (new Id("sasl", hbaseUser).equals(id)) { - if (perms != Perms.ALL) { + if (name.equals(hbaseUser)) { + if (perms != Perms.ALL) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("permissions for '%s' are not correct: have %0x, want %0x", + id, perms, Perms.ALL)); + } + return false; + } + } else { + if (LOG.isDebugEnabled()) { + LOG.debug("Unexpected shortname in SASL ACL: " + id); + } return false; } } else { + if (LOG.isDebugEnabled()) { + LOG.debug("unexpected ACL id '" + id + "'"); + } return false; } } return true; } + /* + * Validate whether ACL set for all superusers. + */ + private boolean checkACLForSuperUsers(String[] superUsers, List acls) { + for (String user : superUsers) { + boolean hasAccess = false; + // TODO: Validate super group members also when ZK supports setting node ACL for groups. + if (!user.startsWith(AuthUtil.GROUP_PREFIX)) { + for (ACL acl : acls) { + if (user.equals(acl.getId().getId())) { + if (acl.getPerms() == Perms.ALL) { + hasAccess = true; + } else { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format( + "superuser '%s' does not have correct permissions: have %0x, want %0x", + acl.getId().getId(), acl.getPerms(), Perms.ALL)); + } + } + break; + } + } + if (!hasAccess) { + return false; + } + } + } + return true; + } + @Override public String toString() { return this.identifier + ", quorum=" + quorum + ", baseZNode=" + baseZNode; diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/AuthUtil.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/AuthUtil.java index cb7ab83..765a4b3 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/AuthUtil.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/AuthUtil.java @@ -38,6 +38,11 @@ import org.apache.hadoop.security.UserGroupInformation; @InterfaceAudience.Public @InterfaceStability.Evolving public class AuthUtil { + /** Prefix character to denote group names */ + public static final String GROUP_PREFIX = "@"; + /** Configuration key for superusers */ + public static final String SUPERUSER_CONF_KEY = "hbase.superuser"; + private static final Log LOG = LogFactory.getLog(AuthUtil.class); private AuthUtil() { -- 2.4.9 (Apple Git-60)