From 369b436bffc1dee149830e5ad47930cc4110d8de Mon Sep 17 00:00:00 2001 From: mbertozzi Date: Wed, 9 Jan 2013 05:45:33 +0000 Subject: [PATCH] HBASE-6386 Audit log messages do not include column family / qualifier information consistently (Marcelo Vanzin) git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1430691 13f79535-47bb-0310-9956-ffa450edef68 --- .../security/access/AccessController.java | 112 ++++++++++-------- .../hbase/security/access/AuthResult.java | 109 ++++++++++++++--- 2 files changed, 154 insertions(+), 67 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index 0c542673d1d..2ea58180f99 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -64,6 +64,7 @@ import org.apache.hadoop.hbase.security.access.Permission.Action; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.MapMaker; @@ -198,12 +199,14 @@ public class AccessController extends BaseRegionObserver // this is a very common operation, so deal with it quickly. if (hri.isRootRegion() || hri.isMetaRegion()) { if (permRequest == Permission.Action.READ) { - return AuthResult.allow(request, "All users allowed", user, permRequest, tableName); + return AuthResult.allow(request, "All users allowed", user, + permRequest, tableName, families); } } if (user == null) { - return AuthResult.deny(request, "No user associated with request!", null, permRequest, tableName); + return AuthResult.deny(request, "No user associated with request!", null, + permRequest, tableName, families); } // Users with CREATE/ADMIN rights need to modify .META. and _acl_ table @@ -217,12 +220,14 @@ public class AccessController extends BaseRegionObserver (authManager.authorize(user, Permission.Action.CREATE) || authManager.authorize(user, Permission.Action.ADMIN))) { - return AuthResult.allow(request, "Table permission granted", user, permRequest, tableName); + return AuthResult.allow(request, "Table permission granted", user, + permRequest, tableName, families); } // 2. check for the table-level, if successful we can short-circuit if (authManager.authorize(user, tableName, (byte[])null, permRequest)) { - return AuthResult.allow(request, "Table permission granted", user, permRequest, tableName); + return AuthResult.allow(request, "Table permission granted", user, + permRequest, tableName, families); } // 3. check permissions against the requested families @@ -244,7 +249,7 @@ public class AccessController extends BaseRegionObserver if (!authManager.authorize(user, tableName, family.getKey(), qualifier, permRequest)) { return AuthResult.deny(request, "Failed qualifier check", user, - permRequest, tableName, family.getKey(), qualifier); + permRequest, tableName, makeFamilyMap(family.getKey(), qualifier)); } } } else if (family.getValue() instanceof List) { // List @@ -253,25 +258,25 @@ public class AccessController extends BaseRegionObserver if (!authManager.authorize(user, tableName, family.getKey(), kv.getQualifier(), permRequest)) { return AuthResult.deny(request, "Failed qualifier check", user, - permRequest, tableName, family.getKey(), kv.getQualifier()); + permRequest, tableName, makeFamilyMap(family.getKey(), kv.getQualifier())); } } } } else { // no qualifiers and family-level check already failed return AuthResult.deny(request, "Failed family check", user, permRequest, - tableName, family.getKey(), null); + tableName, makeFamilyMap(family.getKey(), null)); } } // all family checks passed return AuthResult.allow(request, "All family checks passed", user, permRequest, - tableName); + tableName, families); } // 4. no families to check and table level access failed return AuthResult.deny(request, "No families to check and table permission failed", - user, permRequest, tableName); + user, permRequest, tableName, families); } private void logResult(AuthResult result) { @@ -344,34 +349,15 @@ public class AccessController extends BaseRegionObserver private void requirePermission(String request, Permission.Action perm) throws IOException { User user = getActiveUser(); if (authManager.authorize(user, perm)) { - logResult(AuthResult.allow(request, "Global check allowed", user, perm, null)); + logResult(AuthResult.allow(request, "Global check allowed", user, perm, null, null)); } else { - logResult(AuthResult.deny(request, "Global check failed", user, perm, null)); + logResult(AuthResult.deny(request, "Global check failed", user, perm, null, null)); throw new AccessDeniedException("Insufficient permissions for user '" + (user != null ? user.getShortName() : "null") +"' (global, action=" + perm.toString() + ")"); } } - /** - * Authorizes that the current user has permission to perform the given - * action on the set of table column families. - * @param perm Action that is required - * @param env The current coprocessor environment - * @param families The set of column families present/required in the request - * @throws AccessDeniedException if the authorization check failed - */ - private void requirePermission(String request, Permission.Action perm, - RegionCoprocessorEnvironment env, Collection families) - throws IOException { - // create a map of family-qualifier - HashMap> familyMap = new HashMap>(); - for (byte[] family : families) { - familyMap.put(family, null); - } - requirePermission(request, perm, env, familyMap); - } - /** * Authorizes that the current user has permission to perform the given * action on the set of table column families. @@ -389,23 +375,35 @@ public class AccessController extends BaseRegionObserver logResult(result); if (!result.isAllowed()) { - StringBuilder sb = new StringBuilder(""); - if ((families != null && families.size() > 0)) { - for (byte[] familyName : families.keySet()) { - if (sb.length() != 0) { - sb.append(", "); - } - sb.append(Bytes.toString(familyName)); - } - } throw new AccessDeniedException("Insufficient permissions (table=" + env.getRegion().getTableDesc().getNameAsString()+ ((families != null && families.size() > 0) ? ", family: " + - sb.toString() : "") + ", action=" + + result.toFamilyString() : "") + ", action=" + perm.toString() + ")"); } } + /** + * Checks that the user has the given global permission. The generated + * audit log message will contain context information for the operation + * being authorized, based on the given parameters. + * @param perm Action being requested + * @param tableName Affected table name. + * @param familiMap Affected column families. + */ + private void requireGlobalPermission(String request, Permission.Action perm, byte[] tableName, + Map> familyMap) throws IOException { + User user = getActiveUser(); + if (authManager.authorize(user, perm)) { + logResult(AuthResult.allow(request, "Global check allowed", user, perm, tableName, familyMap)); + } else { + logResult(AuthResult.deny(request, "Global check failed", user, perm, tableName, familyMap)); + throw new AccessDeniedException("Insufficient permissions for user '" + + (user != null ? user.getShortName() : "null") +"' (global, action=" + + perm.toString() + ")"); + } + } + /** * Returns true if the current user is allowed the given action * over at least one of the column qualifiers in the given column families. @@ -785,7 +783,7 @@ public class AccessController extends BaseRegionObserver assert family != null; //noinspection PrimitiveArrayArgumentToVariableArgMethod requirePermission("getClosestRowBefore", Permission.Action.READ, c.getEnvironment(), - Lists.newArrayList(family)); + makeFamilyMap(family, null)); } @Override @@ -815,7 +813,7 @@ public class AccessController extends BaseRegionObserver get.setFilter(filter); } logResult(AuthResult.allow("get", "Access allowed with filter", requestUser, - Permission.Action.READ, authResult.getTable())); + Permission.Action.READ, authResult.getTable(), get.getFamilyMap())); } else { logResult(authResult); throw new AccessDeniedException("Insufficient permissions (table=" + @@ -831,7 +829,7 @@ public class AccessController extends BaseRegionObserver public boolean preExists(final ObserverContext c, final Get get, final boolean exists) throws IOException { requirePermission("exists", Permission.Action.READ, c.getEnvironment(), - get.familySet()); + get.getFamilyMap()); return exists; } @@ -874,7 +872,7 @@ public class AccessController extends BaseRegionObserver final CompareFilter.CompareOp compareOp, final ByteArrayComparable comparator, final Put put, final boolean result) throws IOException { - Collection familyMap = Arrays.asList(new byte[][]{family}); + Map> familyMap = makeFamilyMap(family, qualifier); requirePermission("checkAndPut", Permission.Action.READ, c.getEnvironment(), familyMap); requirePermission("checkAndPut", Permission.Action.WRITE, c.getEnvironment(), familyMap); return result; @@ -886,7 +884,7 @@ public class AccessController extends BaseRegionObserver final CompareFilter.CompareOp compareOp, final ByteArrayComparable comparator, final Delete delete, final boolean result) throws IOException { - Collection familyMap = Arrays.asList(new byte[][]{family}); + Map> familyMap = makeFamilyMap(family, qualifier); requirePermission("checkAndDelete", Permission.Action.READ, c.getEnvironment(), familyMap); requirePermission("checkAndDelete", Permission.Action.WRITE, c.getEnvironment(), familyMap); return result; @@ -897,8 +895,8 @@ public class AccessController extends BaseRegionObserver final byte [] row, final byte [] family, final byte [] qualifier, final long amount, final boolean writeToWAL) throws IOException { - requirePermission("incrementColumnValue", Permission.Action.WRITE, c.getEnvironment(), - Arrays.asList(new byte[][]{family})); + Map> familyMap = makeFamilyMap(family, qualifier); + requirePermission("incrementColumnValue", Permission.Action.WRITE, c.getEnvironment(), familyMap); return -1; } @@ -913,8 +911,11 @@ public class AccessController extends BaseRegionObserver public Result preIncrement(final ObserverContext c, final Increment increment) throws IOException { - requirePermission("increment", Permission.Action.WRITE, c.getEnvironment(), - increment.getFamilyMap().keySet()); + Map> familyMap = Maps.newHashMap(); + for (Map.Entry> entry : increment.getFamilyMap().entrySet()) { + familyMap.put(entry.getKey(), entry.getValue().keySet()); + } + requirePermission("increment", Permission.Action.WRITE, c.getEnvironment(), familyMap); return null; } @@ -945,7 +946,7 @@ public class AccessController extends BaseRegionObserver scan.setFilter(filter); } logResult(AuthResult.allow("scannerOpen", "Access allowed with filter", user, - Permission.Action.READ, authResult.getTable())); + Permission.Action.READ, authResult.getTable(), scan.getFamilyMap())); } else { // no table/family level perms and no qualifier level perms, reject logResult(authResult); @@ -1204,4 +1205,15 @@ public class AccessController extends BaseRegionObserver throws IOException { requirePermission("preStopRegionServer", Permission.Action.ADMIN); } + + private Map> makeFamilyMap(byte[] family, + byte[] qualifier) { + if (family == null) { + return null; + } + + Map> familyMap = Maps.newHashMapWithExpectedSize(1); + familyMap.put(family, qualifier != null ? ImmutableSet.of(qualifier) : null); + return familyMap; + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java index ba1cb31f8d7..5e65fd9226a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java @@ -18,8 +18,12 @@ package org.apache.hadoop.hbase.security.access; +import java.util.Collection; +import java.util.Map; + import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.util.Bytes; @@ -32,14 +36,17 @@ import org.apache.hadoop.hbase.util.Bytes; public class AuthResult { private final boolean allowed; private final byte[] table; - private final byte[] family; - private final byte[] qualifier; private final Permission.Action action; private final String request; private final String reason; private final User user; - public AuthResult(boolean allowed, String request, String reason, User user, + // "family" and "qualifier" should only be used if "families" is null. + private final byte[] family; + private final byte[] qualifier; + private final Map> families; + + public AuthResult(boolean allowed, String request, String reason, User user, Permission.Action action, byte[] table, byte[] family, byte[] qualifier) { this.allowed = allowed; this.request = request; @@ -49,6 +56,21 @@ public class AuthResult { this.family = family; this.qualifier = qualifier; this.action = action; + this.families = null; + } + + public AuthResult(boolean allowed, String request, String reason, User user, + Permission.Action action, byte[] table, + Map> families) { + this.allowed = allowed; + this.request = request; + this.reason = reason; + this.user = user; + this.table = table; + this.family = null; + this.qualifier = null; + this.action = action; + this.families = families; } public boolean isAllowed() { @@ -83,34 +105,87 @@ public class AuthResult { return request; } + String toFamilyString() { + StringBuilder sb = new StringBuilder(); + if (families != null) { + boolean first = true; + for (Map.Entry> entry : families.entrySet()) { + String familyName = Bytes.toString(entry.getKey()); + if (entry.getValue() != null && !entry.getValue().isEmpty()) { + for (Object o : entry.getValue()) { + String qualifier; + if (o instanceof byte[]) { + qualifier = Bytes.toString((byte[])o); + } else if (o instanceof KeyValue) { + byte[] rawQualifier = ((KeyValue)o).getQualifier(); + qualifier = Bytes.toString(rawQualifier); + } else { + // Shouldn't really reach this? + qualifier = o.toString(); + } + if (!first) { + sb.append("|"); + } + first = false; + sb.append(familyName).append(":").append(qualifier); + } + } else { + if (!first) { + sb.append("|"); + } + first = false; + sb.append(familyName); + } + } + } else if (family != null) { + sb.append(Bytes.toString(family)); + if (qualifier != null) { + sb.append(":").append(Bytes.toString(qualifier)); + } + } + return sb.toString(); + } + public String toContextString() { - return "(user=" + (user != null ? user.getName() : "UNKNOWN") + ", " + - "scope=" + (table == null ? "GLOBAL" : Bytes.toString(table)) + ", " + - "family=" + (family != null ? Bytes.toString(family) : "") + ", " + - "qualifer=" + (qualifier != null ? Bytes.toString(qualifier) : "") + ", " + - "action=" + (action != null ? action.toString() : "") + ")"; + StringBuilder sb = new StringBuilder(); + sb.append("(user=") + .append(user != null ? user.getName() : "UNKNOWN") + .append(", "); + sb.append("scope=") + .append(table == null ? "GLOBAL" : Bytes.toString(table)) + .append(", "); + sb.append("family=") + .append(toFamilyString()) + .append(", "); + sb.append("action=") + .append(action != null ? action.toString() : "") + .append(")"); + return sb.toString(); } public String toString() { return "AuthResult" + toContextString(); } - public static AuthResult allow(String request, String reason, User user, Permission.Action action, - byte[] table, byte[] family, byte[] qualifier) { + public static AuthResult allow(String request, String reason, User user, + Permission.Action action, byte[] table, byte[] family, byte[] qualifier) { return new AuthResult(true, request, reason, user, action, table, family, qualifier); } - public static AuthResult allow(String request, String reason, User user, Permission.Action action, byte[] table) { - return new AuthResult(true, request, reason, user, action, table, null, null); - } - - public static AuthResult deny(String request, String reason, User user, - Permission.Action action, byte[] table) { - return new AuthResult(false, request, reason, user, action, table, null, null); + public static AuthResult allow(String request, String reason, User user, + Permission.Action action, byte[] table, + Map> families) { + return new AuthResult(true, request, reason, user, action, table, families); } public static AuthResult deny(String request, String reason, User user, Permission.Action action, byte[] table, byte[] family, byte[] qualifier) { return new AuthResult(false, request, reason, user, action, table, family, qualifier); } + + public static AuthResult deny(String request, String reason, User user, + Permission.Action action, byte[] table, + Map> families) { + return new AuthResult(false, request, reason, user, action, table, families); + } } \ No newline at end of file