From: Andrew Purtell Subject: [PATCH] HBASE-11077 [AccessController] Restore compatible early-out access denial --- .../security/access/AccessControlConstants.java | 22 +- .../hbase/security/access/AccessControlFilter.java | 52 +- .../hbase/security/access/AccessController.java | 779 +++++++++++++-------- .../hadoop/hbase/security/access/AuthResult.java | 12 +- .../hbase/security/access/TableAuthManager.java | 49 +- .../security/access/TestAccessController.java | 2 +- .../access/TestCellACLWithMultipleVersions.java | 2 +- .../hadoop/hbase/security/access/TestCellACLs.java | 2 +- 8 files changed, 565 insertions(+), 355 deletions(-) diff --git hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java index 751c744..4056b86 100644 --- hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java +++ hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java @@ -25,15 +25,29 @@ import org.apache.hadoop.classification.InterfaceStability; @InterfaceStability.Evolving public interface AccessControlConstants { + /** + * Configuration option that toggles whether EXEC permission checking is + * performed during coprocessor endpoint invocations. + */ + String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks"; + /** Default setting for hbase.security.exec.permission.checks; false */ + boolean DEFAULT_EXEC_PERMISSION_CHECKS = false; + + /** + * Configuration or CF schema option for early termination of access checks + * if table or CF permissions grant access. Pre-0.98 compatible behavior + */ + String CF_ATTRIBUTE_EARLY_OUT = "hbase.security.access.early_out"; + // Operation attributes for cell level security /** Cell level ACL */ - public static final String OP_ATTRIBUTE_ACL = "acl"; + String OP_ATTRIBUTE_ACL = "acl"; /** Cell level ACL evaluation strategy */ - public static final String OP_ATTRIBUTE_ACL_STRATEGY = "acl.strategy"; + String OP_ATTRIBUTE_ACL_STRATEGY = "acl.strategy"; /** Default cell ACL evaluation strategy: Table and CF first, then ACL */ - public static final byte[] OP_ATTRIBUTE_ACL_STRATEGY_DEFAULT = new byte[] { 0 }; + byte[] OP_ATTRIBUTE_ACL_STRATEGY_DEFAULT = new byte[] { 0 }; /** Alternate cell ACL evaluation strategy: Cell ACL first, then table and CF */ - public static final byte[] OP_ATTRIBUTE_ACL_STRATEGY_CELL_FIRST = new byte[] { 1 }; + byte[] OP_ATTRIBUTE_ACL_STRATEGY_CELL_FIRST = new byte[] { 1 }; } diff --git hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.java index 97c02b3..8955af3 100644 --- hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.java @@ -22,10 +22,10 @@ import java.io.IOException; import java.util.Map; import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.exceptions.DeserializationException; import org.apache.hadoop.hbase.filter.FilterBase; -import org.apache.hadoop.hbase.filter.Filter.ReturnCode; import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.util.ByteRange; import org.apache.hadoop.hbase.util.Bytes; @@ -48,11 +48,20 @@ import org.apache.hadoop.hbase.util.SimpleByteRange; */ class AccessControlFilter extends FilterBase { + public static enum Strategy { + /** Filter only by checking the table or CF permissions */ + CHECK_TABLE_AND_CF_ONLY, + /** Cell permissions can override table or CF permissions */ + CHECK_CELL_DEFAULT, + /** Cell permissions must authorize */ + CHECK_CELL_FIRST, + }; + private TableAuthManager authManager; private TableName table; private User user; private boolean isSystemTable; - private boolean cellFirstStrategy; + private Strategy strategy; private Map cfVsMaxVersions; private int familyMaxVersions; private int currentVersions; @@ -66,12 +75,12 @@ class AccessControlFilter extends FilterBase { } AccessControlFilter(TableAuthManager mgr, User ugi, TableName tableName, - boolean cellFirstStrategy, Map cfVsMaxVersions) { + Strategy strategy, Map cfVsMaxVersions) { authManager = mgr; table = tableName; user = ugi; isSystemTable = tableName.isSystemTable(); - this.cellFirstStrategy = cellFirstStrategy; + this.strategy = strategy; this.cfVsMaxVersions = cfVsMaxVersions; this.prevFam = new SimpleByteRange(); this.prevQual = new SimpleByteRange(); @@ -103,12 +112,37 @@ class AccessControlFilter extends FilterBase { if (currentVersions > familyMaxVersions) { return ReturnCode.SKIP; } - if (authManager.authorize(user, table, cell, cellFirstStrategy, Permission.Action.READ)) { - return ReturnCode.INCLUDE; + // XXX: Compare in place, don't clone + byte[] family = CellUtil.cloneFamily(cell); + byte[] qualifier = CellUtil.cloneQualifier(cell); + switch (strategy) { + // Filter only by checking the table or CF permissions + case CHECK_TABLE_AND_CF_ONLY: { + if (authManager.authorize(user, table, family, qualifier, Permission.Action.READ)) { + return ReturnCode.INCLUDE; + } + } + break; + // Cell permissions can override table or CF permissions + case CHECK_CELL_DEFAULT: { + if (authManager.authorize(user, table, family, qualifier, Permission.Action.READ) || + authManager.authorize(user, table, cell, Permission.Action.READ)) { + return ReturnCode.INCLUDE; + } + } + break; + // Cell permissions must authorize + case CHECK_CELL_FIRST: { + if (authManager.authorize(user, table, cell, Permission.Action.READ) || + authManager.authorize(user, table, family, qualifier, Permission.Action.READ)) { + return ReturnCode.INCLUDE; + } + } + break; + default: + throw new RuntimeException("Unhandled strategy " + strategy); } - // Before per cell ACLs we used to return the NEXT_COL hint, but we can - // no longer do that since, given the possibility of per cell ACLs - // anywhere, we now need to examine all KVs with this filter. + return ReturnCode.SKIP; } diff --git hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index 2f0b5ad..832b6ad 100644 --- hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -103,6 +103,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.MapMaker; +import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.protobuf.Message; import com.google.protobuf.RpcCallback; @@ -149,9 +150,6 @@ public class AccessController extends BaseRegionObserver private static final Log AUDITLOG = LogFactory.getLog("SecurityLogger."+AccessController.class.getName()); - static final String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks"; - static final boolean DEFAULT_EXEC_PERMISSION_CHECKS = false; - TableAuthManager authManager = null; // flags if we are running on a region of the _acl_ table @@ -167,12 +165,16 @@ public class AccessController extends BaseRegionObserver private UserProvider userProvider; - // flags if we are able to support cell ACLs - boolean canPersistCellACLs; + // if we are able to support cell ACLs + boolean cellFeaturesEnabled; - // flags if we should check EXEC permissions + // if we should check EXEC permissions boolean shouldCheckExecPermissions; + // if we should terminate access checks early as soon as table or CF grants + // allow access; pre-0.98 compatible behavior + boolean compatibleEarlyTermination; + private volatile boolean initialized = false; public HRegion getRegion() { @@ -196,8 +198,11 @@ public class AccessController extends BaseRegionObserver byte[] serialized = AccessControlLists.writePermissionsAsBytes(perms, e.getConfiguration()); this.authManager.getZKPermissionWatcher().writeToZookeeper(entry, serialized); } - shouldCheckExecPermissions = e.getConfiguration().getBoolean(EXEC_PERMISSION_CHECKS_KEY, - DEFAULT_EXEC_PERMISSION_CHECKS); + shouldCheckExecPermissions = e.getConfiguration() + .getBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, + AccessControlConstants.DEFAULT_EXEC_PERMISSION_CHECKS); + compatibleEarlyTermination = e.getConfiguration() + .getBoolean(AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT, false); initialized = true; } @@ -250,7 +255,7 @@ public class AccessController extends BaseRegionObserver * the request * @return an authorization result */ - AuthResult permissionGranted(String request, User user, Permission.Action permRequest, + AuthResult permissionGranted(String request, User user, Action permRequest, RegionCoprocessorEnvironment e, Map> families) { HRegionInfo hri = e.getRegion().getRegionInfo(); @@ -259,7 +264,7 @@ public class AccessController extends BaseRegionObserver // 1. All users need read access to hbase:meta table. // this is a very common operation, so deal with it quickly. if (hri.isMetaRegion()) { - if (permRequest == Permission.Action.READ) { + if (permRequest == Action.READ) { return AuthResult.allow(request, "All users allowed", user, permRequest, tableName, families); } @@ -275,11 +280,11 @@ public class AccessController extends BaseRegionObserver // so the user need to be allowed to write on it. // e.g. When a table is removed an entry is removed from hbase:meta and _acl_ // and the user need to be allowed to write on both tables. - if (permRequest == Permission.Action.WRITE && + if (permRequest == Action.WRITE && (hri.isMetaRegion() || Bytes.equals(tableName.getName(), AccessControlLists.ACL_GLOBAL_NAME)) && - (authManager.authorize(user, Permission.Action.CREATE) || - authManager.authorize(user, Permission.Action.ADMIN))) + (authManager.authorize(user, Action.CREATE) || + authManager.authorize(user, Action.ADMIN))) { return AuthResult.allow(request, "Table permission granted", user, permRequest, tableName, families); @@ -340,6 +345,29 @@ public class AccessController extends BaseRegionObserver user, permRequest, tableName, families); } + /** + * Check the current user for authorization to perform a specific action + * against the given set of row data. + * @param opType the operation type + * @param user the user + * @param e the coprocessor environment + * @param families the map of column families to qualifiers present in + * the request + * @param actions the desired actions + * @return an authorization result + */ + AuthResult permissionGranted(OpType opType, User user, RegionCoprocessorEnvironment e, + Map> families, Action... actions) { + AuthResult result = null; + for (Action action: actions) { + result = permissionGranted(opType.toString(), user, action, e, families); + if (!result.isAllowed()) { + return result; + } + } + return result; + } + private void logResult(AuthResult result) { if (AUDITLOG.isTraceEnabled()) { RequestContext ctx = RequestContext.get(); @@ -407,7 +435,7 @@ public class AccessController extends BaseRegionObserver * @throws IOException if obtaining the current user fails * @throws AccessDeniedException if authorization is denied */ - private void requirePermission(String request, Permission.Action perm) throws IOException { + private void requirePermission(String request, Action perm) throws IOException { requireGlobalPermission(request, perm, null, null); } @@ -419,7 +447,7 @@ public class AccessController extends BaseRegionObserver * @param families The map of column families-qualifiers. * @throws AccessDeniedException if the authorization check failed */ - private void requirePermission(String request, Permission.Action perm, + private void requirePermission(String request, Action perm, RegionCoprocessorEnvironment env, Map> families) throws IOException { @@ -444,7 +472,7 @@ public class AccessController extends BaseRegionObserver * @param tableName Affected table name. * @param familyMap Affected column families. */ - private void requireGlobalPermission(String request, Permission.Action perm, TableName tableName, + private void requireGlobalPermission(String request, Action perm, TableName tableName, Map> familyMap) throws IOException { User user = getActiveUser(); if (authManager.authorize(user, perm)) { @@ -464,7 +492,7 @@ public class AccessController extends BaseRegionObserver * @param perm Action being requested * @param namespace */ - private void requireGlobalPermission(String request, Permission.Action perm, + private void requireGlobalPermission(String request, Action perm, String namespace) throws IOException { User user = getActiveUser(); if (authManager.authorize(user, perm)) { @@ -477,8 +505,52 @@ public class AccessController extends BaseRegionObserver } } + /** + * Returns true if the current user is allowed the given action + * over at least one of the column qualifiers in the given column families. + */ + private boolean hasFamilyQualifierPermission(User user, + Action perm, + RegionCoprocessorEnvironment env, + Map> familyMap) + throws IOException { + HRegionInfo hri = env.getRegion().getRegionInfo(); + TableName tableName = hri.getTable(); + + if (user == null) { + return false; + } + + if (familyMap != null && familyMap.size() > 0) { + // at least one family must be allowed + for (Map.Entry> family : + familyMap.entrySet()) { + if (family.getValue() != null && !family.getValue().isEmpty()) { + for (byte[] qualifier : family.getValue()) { + if (authManager.matchPermission(user, tableName, + family.getKey(), qualifier, perm)) { + return true; + } + } + } else { + if (authManager.matchPermission(user, tableName, family.getKey(), + perm)) { + return true; + } + } + } + } else if (LOG.isDebugEnabled()) { + LOG.debug("Empty family map passed for permission check"); + } + + return false; + } + private enum OpType { GET_CLOSEST_ROW_BEFORE("getClosestRowBefore"), + GET("get"), + EXISTS("exists"), + SCAN("scan"), PUT("put"), DELETE("delete"), CHECK_AND_PUT("checkAndPut"), @@ -499,220 +571,175 @@ public class AccessController extends BaseRegionObserver } } - private void requireCoveringPermission(OpType request, RegionCoprocessorEnvironment e, + /** + * Determine if cell ACLs covered by the operation grant access. This is expensive. + * @return false if cell ACLs failed to grant access, true otherwise + * @throws IOException + */ + private boolean checkCoveringPermission(OpType request, RegionCoprocessorEnvironment e, byte[] row, Map> familyMap, long opTs, Action... actions) throws IOException { + if (!cellFeaturesEnabled) { + return false; + } + long cellGrants = 0; User user = getActiveUser(); - - // First check table or CF level permissions, if they grant access we can - // early out before needing to enumerate over per KV perms. - - List cellCheckActions = Lists.newArrayList(); - // TODO: permissionGranted should support checking multiple actions or - // we should convert actions into a bitmap and pass that around. See - // HBASE-7123. - AuthResult results[] = new AuthResult[actions.length]; - for (int i = 0; i < actions.length; i++) { - results[i] = permissionGranted(request.type, user, actions[i], e, familyMap); - if (!results[i].isAllowed()) { - if (LOG.isTraceEnabled()) { - LOG.trace("Got " + results[i] + ", added to cellCheckActions"); - } - cellCheckActions.add(actions[i]); - } - } - // If all permissions checks passed, we can early out - if (cellCheckActions.isEmpty()) { - if (LOG.isTraceEnabled()) { - LOG.trace("All permissions checks passed, we can early out"); - } - for (int i = 0; i < results.length; i++) { - logResult(results[i]); - } - return; - } - - // Table or CF permissions do not allow, enumerate the covered KVs. We - // can stop at the first which does not grant access. - int cellsChecked = 0; long latestCellTs = 0; - if (canPersistCellACLs) { - Get get = new Get(row); - // Only in case of Put/Delete op, consider TS within cell (if set for individual cells). - // When every cell, within a Mutation, can be linked with diff TS we can not rely on only one - // version. We have to get every cell version and check its TS against the TS asked for in - // Mutation and skip those Cells which is outside this Mutation TS.In case of Put, we have to - // consider only one such passing cell. In case of Delete we have to consider all the cell - // versions under this passing version. When Delete Mutation contains columns which are a - // version delete just consider only one version for those column cells. - boolean considerCellTs = (request == OpType.PUT || request == OpType.DELETE); - if (considerCellTs) { - get.setMaxVersions(); + Get get = new Get(row); + // Only in case of Put/Delete op, consider TS within cell (if set for individual cells). + // When every cell, within a Mutation, can be linked with diff TS we can not rely on only one + // version. We have to get every cell version and check its TS against the TS asked for in + // Mutation and skip those Cells which is outside this Mutation TS.In case of Put, we have to + // consider only one such passing cell. In case of Delete we have to consider all the cell + // versions under this passing version. When Delete Mutation contains columns which are a + // version delete just consider only one version for those column cells. + boolean considerCellTs = (request == OpType.PUT || request == OpType.DELETE); + if (considerCellTs) { + get.setMaxVersions(); + } else { + get.setMaxVersions(1); + } + boolean diffCellTsFromOpTs = false; + for (Map.Entry> entry: familyMap.entrySet()) { + byte[] col = entry.getKey(); + // TODO: HBASE-7114 could possibly unify the collection type in family + // maps so we would not need to do this + if (entry.getValue() instanceof Set) { + Set set = (Set)entry.getValue(); + if (set == null || set.isEmpty()) { + get.addFamily(col); + } else { + for (byte[] qual: set) { + get.addColumn(col, qual); + } + } + } else if (entry.getValue() instanceof List) { + List list = (List)entry.getValue(); + if (list == null || list.isEmpty()) { + get.addFamily(col); + } else { + // In case of family delete, a Cell will be added into the list with Qualifier as null. + for (Cell cell : list) { + if (cell.getQualifierLength() == 0 + && (cell.getTypeByte() == Type.DeleteFamily.getCode() + || cell.getTypeByte() == Type.DeleteFamilyVersion.getCode())) { + get.addFamily(col); + } else { + get.addColumn(col, CellUtil.cloneQualifier(cell)); + } + if (considerCellTs) { + long cellTs = cell.getTimestamp(); + latestCellTs = Math.max(latestCellTs, cellTs); + diffCellTsFromOpTs = diffCellTsFromOpTs || (opTs != cellTs); + } + } + } } else { - get.setMaxVersions(1); + throw new RuntimeException("Unhandled collection type " + + entry.getValue().getClass().getName()); } - boolean diffCellTsFromOpTs = false; - for (Map.Entry> entry: familyMap.entrySet()) { - byte[] col = entry.getKey(); - // TODO: HBASE-7114 could possibly unify the collection type in family - // maps so we would not need to do this - if (entry.getValue() instanceof Set) { - Set set = (Set)entry.getValue(); - if (set == null || set.isEmpty()) { - get.addFamily(col); - } else { - for (byte[] qual: set) { - get.addColumn(col, qual); - } + } + // We want to avoid looking into the future. So, if the cells of the + // operation specify a timestamp, or the operation itself specifies a + // timestamp, then we use the maximum ts found. Otherwise, we bound + // the Get to the current server time. We add 1 to the timerange since + // the upper bound of a timerange is exclusive yet we need to examine + // any cells found there inclusively. + long latestTs = Math.max(opTs, latestCellTs); + if (latestTs == 0 || latestTs == HConstants.LATEST_TIMESTAMP) { + latestTs = EnvironmentEdgeManager.currentTimeMillis(); + } + get.setTimeRange(0, latestTs + 1); + // In case of Put operation we set to read all versions. This was done to consider the case + // where columns are added with TS other than the Mutation TS. But normally this wont be the + // case with Put. There no need to get all versions but get latest version only. + if (!diffCellTsFromOpTs && request == OpType.PUT) { + get.setMaxVersions(1); + } + if (LOG.isTraceEnabled()) { + LOG.trace("Scanning for cells with " + get); + } + // This Map is identical to familyMap. The key is a BR rather than byte[]. + // It will be easy to do gets over this new Map as we can create get keys over the Cell cf by + // new SimpleByteRange(cell.familyArray, cell.familyOffset, cell.familyLen) + Map> familyMap1 = new HashMap>(); + for (Entry> entry : familyMap.entrySet()) { + if (entry.getValue() instanceof List) { + familyMap1.put(new SimpleByteRange(entry.getKey()), (List) entry.getValue()); + } + } + RegionScanner scanner = getRegion(e).getScanner(new Scan(get)); + List cells = Lists.newArrayList(); + Cell prevCell = null; + ByteRange curFam = new SimpleByteRange(); + boolean curColAllVersions = (request == OpType.DELETE); + long curColCheckTs = opTs; + boolean foundColumn = false; + try { + boolean more = false; + do { + cells.clear(); + // scan with limit as 1 to hold down memory use on wide rows + more = scanner.next(cells, 1); + for (Cell cell: cells) { + if (LOG.isTraceEnabled()) { + LOG.trace("Found cell " + cell); } - } else if (entry.getValue() instanceof List) { - List list = (List)entry.getValue(); - if (list == null || list.isEmpty()) { - get.addFamily(col); - } else { - // In case of family delete, a Cell will be added into the list with Qualifier as null. - for (Cell cell : list) { - if (cell.getQualifierLength() == 0 - && (cell.getTypeByte() == Type.DeleteFamily.getCode() - || cell.getTypeByte() == Type.DeleteFamilyVersion.getCode())) { - get.addFamily(col); - } else { - get.addColumn(col, CellUtil.cloneQualifier(cell)); - } - if (considerCellTs) { - long cellTs = cell.getTimestamp(); - latestCellTs = Math.max(latestCellTs, cellTs); - diffCellTsFromOpTs = diffCellTsFromOpTs || (opTs != cellTs); - } - } + boolean colChange = prevCell == null || !CellUtil.matchingColumn(prevCell, cell); + if (colChange) foundColumn = false; + prevCell = cell; + if (!curColAllVersions && foundColumn) { + continue; } - } else { - throw new RuntimeException("Unhandled collection type " + - entry.getValue().getClass().getName()); - } - } - // We want to avoid looking into the future. So, if the cells of the - // operation specify a timestamp, or the operation itself specifies a - // timestamp, then we use the maximum ts found. Otherwise, we bound - // the Get to the current server time. We add 1 to the timerange since - // the upper bound of a timerange is exclusive yet we need to examine - // any cells found there inclusively. - long latestTs = Math.max(opTs, latestCellTs); - if (latestTs == 0 || latestTs == HConstants.LATEST_TIMESTAMP) { - latestTs = EnvironmentEdgeManager.currentTimeMillis(); - } - get.setTimeRange(0, latestTs + 1); - // In case of Put operation we set to read all versions. This was done to consider the case - // where columns are added with TS other than the Mutation TS. But normally this wont be the - // case with Put. There no need to get all versions but get latest version only. - if (!diffCellTsFromOpTs && request == OpType.PUT) { - get.setMaxVersions(1); - } - if (LOG.isTraceEnabled()) { - LOG.trace("Scanning for cells with " + get); - } - // This Map is identical to familyMap. The key is a BR rather than byte[]. - // It will be easy to do gets over this new Map as we can create get keys over the Cell cf by - // new SimpleByteRange(cell.familyArray, cell.familyOffset, cell.familyLen) - Map> familyMap1 = new HashMap>(); - for (Entry> entry : familyMap.entrySet()) { - if (entry.getValue() instanceof List) { - familyMap1.put(new SimpleByteRange(entry.getKey()), (List) entry.getValue()); - } - } - RegionScanner scanner = getRegion(e).getScanner(new Scan(get)); - List cells = Lists.newArrayList(); - Cell prevCell = null; - ByteRange curFam = new SimpleByteRange(); - boolean curColAllVersions = (request == OpType.DELETE); - long curColCheckTs = opTs; - boolean foundColumn = false; - try { - boolean more = false; - do { - cells.clear(); - // scan with limit as 1 to hold down memory use on wide rows - more = scanner.next(cells, 1); - for (Cell cell: cells) { - if (LOG.isTraceEnabled()) { - LOG.trace("Found cell " + cell); - } - boolean colChange = prevCell == null || !CellUtil.matchingColumn(prevCell, cell); - if (colChange) foundColumn = false; - prevCell = cell; - if (!curColAllVersions && foundColumn) { - continue; - } - if (colChange && considerCellTs) { - curFam.set(cell.getFamilyArray(), cell.getFamilyOffset(), cell.getFamilyLength()); - List cols = familyMap1.get(curFam); - for (Cell col : cols) { - // null/empty qualifier is used to denote a Family delete. The TS and delete type - // associated with this is applicable for all columns within the family. That is - // why the below (col.getQualifierLength() == 0) check. - if ((col.getQualifierLength() == 0 && request == OpType.DELETE) - || CellUtil.matchingQualifier(cell, col)) { - byte type = col.getTypeByte(); - if (considerCellTs) - curColCheckTs = col.getTimestamp(); - // For a Delete op we pass allVersions as true. When a Delete Mutation contains - // a version delete for a column no need to check all the covering cells within - // that column. Check all versions when Type is DeleteColumn or DeleteFamily - // One version delete types are Delete/DeleteFamilyVersion - curColAllVersions = (KeyValue.Type.DeleteColumn.getCode() == type) - || (KeyValue.Type.DeleteFamily.getCode() == type); - break; + if (colChange && considerCellTs) { + curFam.set(cell.getFamilyArray(), cell.getFamilyOffset(), cell.getFamilyLength()); + List cols = familyMap1.get(curFam); + for (Cell col : cols) { + // null/empty qualifier is used to denote a Family delete. The TS and delete type + // associated with this is applicable for all columns within the family. That is + // why the below (col.getQualifierLength() == 0) check. + if ((col.getQualifierLength() == 0 && request == OpType.DELETE) + || CellUtil.matchingQualifier(cell, col)) { + byte type = col.getTypeByte(); + if (considerCellTs) { + curColCheckTs = col.getTimestamp(); } + // For a Delete op we pass allVersions as true. When a Delete Mutation contains + // a version delete for a column no need to check all the covering cells within + // that column. Check all versions when Type is DeleteColumn or DeleteFamily + // One version delete types are Delete/DeleteFamilyVersion + curColAllVersions = (KeyValue.Type.DeleteColumn.getCode() == type) + || (KeyValue.Type.DeleteFamily.getCode() == type); + break; } } - if (cell.getTimestamp() > curColCheckTs) { - // Just ignore this cell. This is not a covering cell. - continue; - } - foundColumn = true; - for (Action action: cellCheckActions) { - // Are there permissions for this user for the cell? - if (!authManager.authorize(user, getTableName(e), cell, false, action)) { - AuthResult authResult = AuthResult.deny(request.type, "Insufficient permissions", - user, action, getTableName(e), CellUtil.cloneFamily(cell), - CellUtil.cloneQualifier(cell)); - logResult(authResult); - throw new AccessDeniedException("Insufficient permissions " + - authResult.toContextString()); - } + } + if (cell.getTimestamp() > curColCheckTs) { + // Just ignore this cell. This is not a covering cell. + continue; + } + foundColumn = true; + for (Action action: actions) { + // Are there permissions for this user for the cell? + if (!authManager.authorize(user, getTableName(e), cell, action)) { + // We can stop if the cell ACL denies access + return false; } - cellsChecked++; } - } while (more); - } catch (AccessDeniedException ex) { - throw ex; - } catch (IOException ex) { - LOG.error("Exception while getting cells to calculate covering permission", ex); - } finally { - scanner.close(); - } - } - - // If there were no cells to check, throw the ADE - if (cellsChecked < 1) { - if (LOG.isTraceEnabled()) { - LOG.trace("No cells found with scan"); - } - AuthResult authResult = AuthResult.deny(request.type, "Insufficient permissions", - user, cellCheckActions.get(0), getTableName(e), familyMap); - logResult(authResult); - throw new AccessDeniedException("Insufficient permissions " + - authResult.toContextString()); - } - - // Log that authentication succeeded. We need to trade off logging maybe - // thousands of fine grained decisions with providing detail. - for (byte[] family: familyMap.keySet()) { - for (Action action: actions) { - logResult(AuthResult.allow(request.type, "Permission granted", user, action, - getTableName(e), family, null)); - } + cellGrants++; + } + } while (more); + } catch (AccessDeniedException ex) { + throw ex; + } catch (IOException ex) { + LOG.error("Exception while getting cells to calculate covering permission", ex); + } finally { + scanner.close(); } + // We should not authorize unless we have found one or more cell ACLs that + // grant access. This code is used to check for additional permissions + // after no table or CF grants are found. + return cellGrants > 0; } private void addCellPermissions(final byte[] perms, Map> familyMap) { @@ -743,38 +770,12 @@ public class AccessController extends BaseRegionObserver } } - private void internalPreRead(final ObserverContext c, - final Query query) throws IOException { - TableName tableName = getTableName(c.getEnvironment()); - User activeUser = getActiveUser(); - Filter filter = query.getFilter(); - boolean cellFirstStrategy = query.getACLStrategy(); - // Don't wrap an AccessControlFilter - if (filter != null && filter instanceof AccessControlFilter) { - return; - } - Map cfVsMaxVersions = new HashMap(); - HRegion region = c.getEnvironment().getRegion(); - for (HColumnDescriptor hcd : region.getTableDesc().getFamilies()) { - cfVsMaxVersions.put(new SimpleByteRange(hcd.getName()), hcd.getMaxVersions()); - } - Filter newFilter = (filter != null) - ? new FilterList(FilterList.Operator.MUST_PASS_ALL, - Lists.newArrayList( - new AccessControlFilter(authManager, activeUser, tableName, - cellFirstStrategy, cfVsMaxVersions), - filter)) - : new AccessControlFilter(authManager, activeUser, tableName, - cellFirstStrategy, cfVsMaxVersions); - query.setFilter(newFilter); - } - /* ---- MasterObserver implementation ---- */ public void start(CoprocessorEnvironment env) throws IOException { - canPersistCellACLs = HFile.getFormatVersion(env.getConfiguration()) >= + cellFeaturesEnabled = HFile.getFormatVersion(env.getConfiguration()) >= HFile.MIN_FORMAT_VERSION_WITH_TAGS; - if (!canPersistCellACLs) { + if (!cellFeaturesEnabled) { LOG.info("A minimum HFile version of " + HFile.MIN_FORMAT_VERSION_WITH_TAGS + " is required to persist cell ACLs. Consider setting " + HFile.FORMAT_VERSION_KEY + " accordingly."); @@ -821,7 +822,7 @@ public class AccessController extends BaseRegionObserver for (byte[] family: families) { familyMap.put(family, null); } - requireGlobalPermission("createTable", Permission.Action.CREATE, desc.getTableName(), familyMap); + requireGlobalPermission("createTable", Action.CREATE, desc.getTableName(), familyMap); } @Override @@ -940,8 +941,7 @@ public class AccessController extends BaseRegionObserver @Override public void postDeleteColumn(ObserverContext c, TableName tableName, byte[] col) throws IOException { - AccessControlLists.removeTablePermissions(c.getEnvironment().getConfiguration(), - tableName, col); + AccessControlLists.removeTablePermissions(c.getEnvironment().getConfiguration(), tableName, col); } @Override @@ -1033,8 +1033,9 @@ public class AccessController extends BaseRegionObserver @Override public void preBalance(ObserverContext c) throws IOException { - requirePermission("balance", Permission.Action.ADMIN); + requirePermission("balance", Action.ADMIN); } + @Override public void postBalance(ObserverContext c, List plans) throws IOException {} @@ -1042,7 +1043,7 @@ public class AccessController extends BaseRegionObserver @Override public boolean preBalanceSwitch(ObserverContext c, boolean newValue) throws IOException { - requirePermission("balanceSwitch", Permission.Action.ADMIN); + requirePermission("balanceSwitch", Action.ADMIN); return newValue; } @@ -1053,13 +1054,13 @@ public class AccessController extends BaseRegionObserver @Override public void preShutdown(ObserverContext c) throws IOException { - requirePermission("shutdown", Permission.Action.ADMIN); + requirePermission("shutdown", Action.ADMIN); } @Override public void preStopMaster(ObserverContext c) throws IOException { - requirePermission("stopMaster", Permission.Action.ADMIN); + requirePermission("stopMaster", Action.ADMIN); } @Override @@ -1078,7 +1079,7 @@ public class AccessController extends BaseRegionObserver public void preSnapshot(final ObserverContext ctx, final SnapshotDescription snapshot, final HTableDescriptor hTableDescriptor) throws IOException { - requirePermission("snapshot", Permission.Action.ADMIN); + requirePermission("snapshot", Action.ADMIN); } @Override @@ -1091,7 +1092,7 @@ public class AccessController extends BaseRegionObserver public void preCloneSnapshot(final ObserverContext ctx, final SnapshotDescription snapshot, final HTableDescriptor hTableDescriptor) throws IOException { - requirePermission("clone", Permission.Action.ADMIN); + requirePermission("clone", Action.ADMIN); } @Override @@ -1104,7 +1105,7 @@ public class AccessController extends BaseRegionObserver public void preRestoreSnapshot(final ObserverContext ctx, final SnapshotDescription snapshot, final HTableDescriptor hTableDescriptor) throws IOException { - requirePermission("restore", Permission.Action.ADMIN); + requirePermission("restore", Action.ADMIN); } @Override @@ -1116,7 +1117,7 @@ public class AccessController extends BaseRegionObserver @Override public void preDeleteSnapshot(final ObserverContext ctx, final SnapshotDescription snapshot) throws IOException { - requirePermission("deleteSnapshot", Permission.Action.ADMIN); + requirePermission("deleteSnapshot", Action.ADMIN); } @Override @@ -1254,20 +1255,129 @@ public class AccessController extends BaseRegionObserver final byte [] row, final byte [] family, final Result result) throws IOException { assert family != null; - requireCoveringPermission(OpType.GET_CLOSEST_ROW_BEFORE, c.getEnvironment(), row, - makeFamilyMap(family, null), HConstants.LATEST_TIMESTAMP, Permission.Action.READ); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = makeFamilyMap(family, null); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.GET_CLOSEST_ROW_BEFORE, user, env, families, + Action.READ); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.GET_CLOSEST_ROW_BEFORE, env, row, + families, HConstants.LATEST_TIMESTAMP, Action.READ)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } + } + + private void internalPreRead(final ObserverContext c, + final Query query, OpType opType) throws IOException { + Filter filter = query.getFilter(); + // Don't wrap an AccessControlFilter + if (filter != null && filter instanceof AccessControlFilter) { + return; + } + User user = getActiveUser(); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = null; + switch (opType) { + case GET: + case EXISTS: + families = ((Get)query).getFamilyMap(); + break; + case SCAN: + families = ((Scan)query).getFamilyMap(); + break; + default: + throw new RuntimeException("Unhandled operation " + opType); + } + AuthResult authResult = permissionGranted(opType, user, env, families, Action.READ); + HRegion region = getRegion(env); + TableName table = getTableName(region); + Map cfVsMaxVersions = Maps.newHashMap(); + for (HColumnDescriptor hcd : region.getTableDesc().getFamilies()) { + cfVsMaxVersions.put(new SimpleByteRange(hcd.getName()), hcd.getMaxVersions()); + } + if (!authResult.isAllowed()) { + if (!cellFeaturesEnabled || compatibleEarlyTermination) { + // Old behavior: Scan with only qualifier checks if we have partial + // permission. Backwards compatible behavior is to throw an + // AccessDeniedException immediately if there are no grants for table + // or CF or CF+qual. Only proceed with an injected filter if there are + // grants for qualifiers. Otherwise we will fall through below and log + // the result and throw an ADE. We may end up checking qualifier + // grants three times (permissionGranted above, here, and in the + // filter) but that's the price of backwards compatibility. + if (hasFamilyQualifierPermission(user, Action.READ, env, families)) { + Filter ourFilter = new AccessControlFilter(authManager, user, table, + AccessControlFilter.Strategy.CHECK_TABLE_AND_CF_ONLY, cfVsMaxVersions); + // wrap any existing filter + if (filter != null) { + ourFilter = new FilterList(FilterList.Operator.MUST_PASS_ALL, + Lists.newArrayList(ourFilter, filter)); + } + authResult.setAllowed(true);; + authResult.setReason("Access allowed with filter"); + switch (opType) { + case GET: + case EXISTS: + ((Get)query).setFilter(ourFilter); + break; + case SCAN: + ((Scan)query).setFilter(ourFilter); + break; + default: + throw new RuntimeException("Unhandled operation " + opType); + } + } + } else { + // New behavior: Any access we might be granted is more fine-grained + // than whole table or CF. Simply inject a filter and return what is + // allowed. We will not throw an AccessDeniedException. This is a + // behavioral change since 0.96. + Filter ourFilter = new AccessControlFilter(authManager, user, table, + query.getACLStrategy() ? AccessControlFilter.Strategy.CHECK_CELL_FIRST : + AccessControlFilter.Strategy.CHECK_CELL_DEFAULT, + cfVsMaxVersions); + // wrap any existing filter + if (filter != null) { + ourFilter = new FilterList(FilterList.Operator.MUST_PASS_ALL, + Lists.newArrayList(ourFilter, filter)); + } + authResult.setAllowed(true);; + authResult.setReason("Access allowed with filter"); + switch (opType) { + case GET: + case EXISTS: + ((Get)query).setFilter(ourFilter); + break; + case SCAN: + ((Scan)query).setFilter(ourFilter); + break; + default: + throw new RuntimeException("Unhandled operation " + opType); + } + } + } + + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions (table=" + table + + ", action=READ)"); + } } @Override public void preGetOp(final ObserverContext c, final Get get, final List result) throws IOException { - internalPreRead(c, get); + internalPreRead(c, get, OpType.GET); } @Override public boolean preExists(final ObserverContext c, final Get get, final boolean exists) throws IOException { - internalPreRead(c, get); + internalPreRead(c, get, OpType.EXISTS); return exists; } @@ -1281,11 +1391,22 @@ public class AccessController extends BaseRegionObserver // HBase value. A new ACL in a new Put applies to that Put. It doesn't // change the ACL of any previous Put. This allows simple evolution of // security policy over time without requiring expensive updates. - requireCoveringPermission(OpType.PUT, c.getEnvironment(), put.getRow(), - put.getFamilyCellMap(), put.getTimeStamp(), Permission.Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = put.getFamilyCellMap(); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.PUT, user, env, families, Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.PUT, env, put.getRow(), families, + put.getTimeStamp(), Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } byte[] bytes = put.getAttribute(AccessControlConstants.OP_ATTRIBUTE_ACL); if (bytes != null) { - if (canPersistCellACLs) { + if (cellFeaturesEnabled) { addCellPermissions(bytes, put.getFamilyCellMap()); } else { throw new DoNotRetryIOException("Cell ACLs cannot be persisted"); @@ -1314,8 +1435,19 @@ public class AccessController extends BaseRegionObserver // compaction could remove them. If the user doesn't have permission to // overwrite any of the visible versions ('visible' defined as not covered // by a tombstone already) then we have to disallow this operation. - requireCoveringPermission(OpType.DELETE, c.getEnvironment(), delete.getRow(), - delete.getFamilyCellMap(), delete.getTimeStamp(), Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = delete.getFamilyCellMap(); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.DELETE, user, env, families, Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.DELETE, env, delete.getRow(), families, + delete.getTimeStamp(), Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } } @Override @@ -1334,11 +1466,23 @@ public class AccessController extends BaseRegionObserver final ByteArrayComparable comparator, final Put put, final boolean result) throws IOException { // Require READ and WRITE permissions on the table, CF, and KV to update - requireCoveringPermission(OpType.CHECK_AND_PUT, c.getEnvironment(), row, - makeFamilyMap(family, qualifier), HConstants.LATEST_TIMESTAMP, Action.READ, Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = makeFamilyMap(family, qualifier); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.CHECK_AND_PUT, user, env, families, + Action.READ, Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.CHECK_AND_PUT, env, row, families, + HConstants.LATEST_TIMESTAMP, Action.READ, Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } byte[] bytes = put.getAttribute(AccessControlConstants.OP_ATTRIBUTE_ACL); if (bytes != null) { - if (canPersistCellACLs) { + if (cellFeaturesEnabled) { addCellPermissions(bytes, put.getFamilyCellMap()); } else { throw new DoNotRetryIOException("Cell ACLs cannot be persisted"); @@ -1360,8 +1504,20 @@ public class AccessController extends BaseRegionObserver } // Require READ and WRITE permissions on the table, CF, and the KV covered // by the delete - requireCoveringPermission(OpType.CHECK_AND_DELETE, c.getEnvironment(), row, - makeFamilyMap(family, qualifier), HConstants.LATEST_TIMESTAMP, Action.READ, Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = makeFamilyMap(family, qualifier); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.CHECK_AND_DELETE, user, env, families, + Action.READ, Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.CHECK_AND_DELETE, env, row, families, + HConstants.LATEST_TIMESTAMP, Action.READ, Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } return result; } @@ -1372,8 +1528,20 @@ public class AccessController extends BaseRegionObserver throws IOException { // Require WRITE permission to the table, CF, and the KV to be replaced by the // incremented value - requireCoveringPermission(OpType.INCREMENT_COLUMN_VALUE, c.getEnvironment(), row, - makeFamilyMap(family, qualifier), HConstants.LATEST_TIMESTAMP, Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = makeFamilyMap(family, qualifier); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.INCREMENT_COLUMN_VALUE, user, env, families, + Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.INCREMENT_COLUMN_VALUE, env, row, + families, HConstants.LATEST_TIMESTAMP, Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } return -1; } @@ -1381,11 +1549,22 @@ public class AccessController extends BaseRegionObserver public Result preAppend(ObserverContext c, Append append) throws IOException { // Require WRITE permission to the table, CF, and the KV to be appended - requireCoveringPermission(OpType.APPEND, c.getEnvironment(), append.getRow(), - append.getFamilyCellMap(), HConstants.LATEST_TIMESTAMP, Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = append.getFamilyCellMap(); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.APPEND, user, env, families, Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.APPEND, env, append.getRow(), + families, HConstants.LATEST_TIMESTAMP, Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } byte[] bytes = append.getAttribute(AccessControlConstants.OP_ATTRIBUTE_ACL); if (bytes != null) { - if (canPersistCellACLs) { + if (cellFeaturesEnabled) { addCellPermissions(bytes, append.getFamilyCellMap()); } else { throw new DoNotRetryIOException("Cell ACLs cannot be persisted"); @@ -1400,11 +1579,23 @@ public class AccessController extends BaseRegionObserver throws IOException { // Require WRITE permission to the table, CF, and the KV to be replaced by // the incremented value - requireCoveringPermission(OpType.INCREMENT, c.getEnvironment(), increment.getRow(), - increment.getFamilyCellMap(), increment.getTimeRange().getMax(), Action.WRITE); + RegionCoprocessorEnvironment env = c.getEnvironment(); + Map> families = increment.getFamilyCellMap(); + User user = getActiveUser(); + AuthResult authResult = permissionGranted(OpType.INCREMENT, user, env, families, + Action.WRITE); + if (!authResult.isAllowed() && cellFeaturesEnabled && !compatibleEarlyTermination) { + authResult.setAllowed(checkCoveringPermission(OpType.APPEND, env, increment.getRow(), + families, increment.getTimeRange().getMax(), Action.WRITE)); + authResult.setReason("Covering cell set"); + } + logResult(authResult); + if (!authResult.isAllowed()) { + throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); + } byte[] bytes = increment.getAttribute(AccessControlConstants.OP_ATTRIBUTE_ACL); if (bytes != null) { - if (canPersistCellACLs) { + if (cellFeaturesEnabled) { addCellPermissions(bytes, increment.getFamilyCellMap()); } else { throw new DoNotRetryIOException("Cell ACLs cannot be persisted"); @@ -1418,7 +1609,7 @@ public class AccessController extends BaseRegionObserver MutationType opType, Mutation mutation, Cell oldCell, Cell newCell) throws IOException { // If the HFile version is insufficient to persist tags, we won't have any // work to do here - if (!canPersistCellACLs) { + if (!cellFeaturesEnabled) { return newCell; } @@ -1486,7 +1677,7 @@ public class AccessController extends BaseRegionObserver @Override public RegionScanner preScannerOpen(final ObserverContext c, final Scan scan, final RegionScanner s) throws IOException { - internalPreRead(c, scan); + internalPreRead(c, scan, OpType.SCAN); return s; } @@ -1563,7 +1754,7 @@ public class AccessController extends BaseRegionObserver if (!authResult.isAllowed()) { for(UserPermission userPerm: AccessControlLists.getUserTablePermissions(regionEnv.getConfiguration(), tableName)) { - for(Permission.Action userAction: userPerm.getActions()) { + for(Action userAction: userPerm.getActions()) { if(userAction.equals(action)) { return AuthResult.allow(method, "Access allowed", requestUser, action, tableName, null, null); @@ -1769,7 +1960,7 @@ public class AccessController extends BaseRegionObserver for (Permission permission : permissions) { if (permission instanceof TablePermission) { TablePermission tperm = (TablePermission) permission; - for (Permission.Action action : permission.getActions()) { + for (Action action : permission.getActions()) { if (!tperm.getTableName().equals(tableName)) { throw new CoprocessorException(AccessController.class, String.format("This method " + "can only execute at the table specified in TablePermission. " + @@ -1792,7 +1983,7 @@ public class AccessController extends BaseRegionObserver } } else { - for (Permission.Action action : permission.getActions()) { + for (Action action : permission.getActions()) { requirePermission("checkPermissions", action); } } @@ -1815,17 +2006,19 @@ public class AccessController extends BaseRegionObserver private TableName getTableName(RegionCoprocessorEnvironment e) { HRegion region = e.getRegion(); - TableName tableName = null; - if (region != null) { - HRegionInfo regionInfo = region.getRegionInfo(); - if (regionInfo != null) { - tableName = regionInfo.getTable(); - } + return getTableName(region); } - return tableName; + return null; } + private TableName getTableName(HRegion region) { + HRegionInfo regionInfo = region.getRegionInfo(); + if (regionInfo != null) { + return regionInfo.getTable(); + } + return null; + } @Override public void preClose(ObserverContext e, boolean abortRequested) @@ -1855,7 +2048,7 @@ public class AccessController extends BaseRegionObserver public void preStopRegionServer( ObserverContext env) throws IOException { - requirePermission("preStopRegionServer", Permission.Action.ADMIN); + requirePermission("preStopRegionServer", Action.ADMIN); } private Map> makeFamilyMap(byte[] family, @@ -1876,7 +2069,7 @@ public class AccessController extends BaseRegionObserver // If the list is empty, this is a request for all table descriptors and requires GLOBAL // ADMIN privs. if (tableNamesList == null || tableNamesList.isEmpty()) { - requireGlobalPermission("getTableDescriptors", Permission.Action.ADMIN, null, null); + requireGlobalPermission("getTableDescriptors", Action.ADMIN, null, null); } // Otherwise, if the requestor has ADMIN or CREATE privs for all listed tables, the // request can be granted. @@ -1893,7 +2086,7 @@ public class AccessController extends BaseRegionObserver // We don't care about this } requirePermission("getTableDescriptors", tableName, null, null, - Permission.Action.ADMIN, Permission.Action.CREATE); + Action.ADMIN, Action.CREATE); } } } diff --git hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java index adcf49b..e330c14 100644 --- hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AuthResult.java @@ -33,12 +33,12 @@ import org.apache.hadoop.hbase.util.Bytes; */ @InterfaceAudience.Private public class AuthResult { - private final boolean allowed; + private boolean allowed; private final String namespace; private final TableName table; private final Permission.Action action; private final String request; - private final String reason; + private String reason; private final User user; // "family" and "qualifier" should only be used if "families" is null. @@ -121,6 +121,14 @@ public class AuthResult { return request; } + public void setAllowed(boolean allowed) { + this.allowed = allowed; + } + + public void setReason(String reason) { + this.reason = reason; + } + String toFamilyString() { StringBuilder sb = new StringBuilder(); if (families != null) { diff --git hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java index 6bb4e4e..394d8c1 100644 --- hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java @@ -28,7 +28,6 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Cell; -import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.exceptions.DeserializationException; import org.apache.hadoop.hbase.security.User; @@ -348,12 +347,14 @@ public class TableAuthManager { return false; } - private boolean checkCellPermissions(User user, Cell cell, Permission.Action action) { + /** + * Authorize a user for a given KV. This is called from AccessControlFilter. + */ + public boolean authorize(User user, TableName table, Cell cell, Permission.Action action) { try { List perms = AccessControlLists.getCellPermissionsForUser(user, cell); if (LOG.isTraceEnabled()) { - LOG.trace("Perms for user " + user.getShortName() + " in cell " + - cell + ": " + perms); + LOG.trace("Perms for user " + user.getShortName() + " in cell " + cell + ": " + perms); } for (Permission p: perms) { if (p.implies(action)) { @@ -369,46 +370,6 @@ public class TableAuthManager { return false; } - private boolean checkTableColumnPermissions(User user, TableName table, Cell cell, - Permission.Action action) { - // TODO: Do not clone here - byte[] family = CellUtil.cloneFamily(cell); - byte[] qualifier = CellUtil.cloneQualifier(cell); - // User is authorized at table or CF level - if (authorizeUser(user, table, family, qualifier, action)) { - return true; - } - String groupNames[] = user.getGroupNames(); - if (groupNames != null) { - for (String group: groupNames) { - // TODO: authorizeGroup should check qualifier too? - // Group is authorized at table or CF level - if (authorizeGroup(group, table, family, action)) { - return true; - } - } - } - return false; - } - - /** - * Authorize a user for a given KV. This is called from AccessControlFilter. - */ - public boolean authorize(User user, TableName table, Cell cell, boolean cellFirstStrategy, - Permission.Action action) { - if (cellFirstStrategy) { - if (checkCellPermissions(user, cell, action)) { - return true; - } - return checkTableColumnPermissions(user, table, cell, action); - } else { - if (checkTableColumnPermissions(user, table, cell, action)) { - return true; - } - return checkCellPermissions(user, cell, action); - } - } - public boolean authorize(User user, String namespace, Permission.Action action) { // Global authorizations supercede namespace level if (authorizeUser(user, action)) { diff --git hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java index 9ac3acd..dbee78d 100644 --- hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java +++ hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java @@ -170,7 +170,7 @@ public class TestAccessController extends SecureTestUtil { verifyConfiguration(conf); // Enable EXEC permission checking - conf.setBoolean(AccessController.EXEC_PERMISSION_CHECKS_KEY, true); + conf.setBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, true); TEST_UTIL.startMiniCluster(); MasterCoprocessorHost cpHost = diff --git hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java index f3f7260..1299418 100644 --- hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java +++ hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java @@ -100,7 +100,7 @@ public class TestCellACLWithMultipleVersions extends SecureTestUtil { verifyConfiguration(conf); // Enable EXEC permission checking - conf.setBoolean(AccessController.EXEC_PERMISSION_CHECKS_KEY, true); + conf.setBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, true); TEST_UTIL.startMiniCluster(); MasterCoprocessorHost cpHost = TEST_UTIL.getMiniHBaseCluster().getMaster() diff --git hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java index 18196c2..2133aea 100644 --- hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java +++ hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java @@ -100,7 +100,7 @@ public class TestCellACLs extends SecureTestUtil { verifyConfiguration(conf); // Enable EXEC permission checking - conf.setBoolean(AccessController.EXEC_PERMISSION_CHECKS_KEY, true); + conf.setBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, true); TEST_UTIL.startMiniCluster(); MasterCoprocessorHost cpHost = TEST_UTIL.getMiniHBaseCluster().getMaster() -- 1.9.1