From 481574072c1f794dad0c4455aa98a3f3c5725481 Mon Sep 17 00:00:00 2001 From: Toshihiro Suzuki Date: Thu, 8 Oct 2020 17:04:48 +0900 Subject: [PATCH] HBASE-25160 Refactor AccessController and VisibilityController (#2506) Signed-off-by: stack --- .../security/access/AccessController.java | 66 ++++--------------- .../visibility/VisibilityController.java | 66 +------------------ 2 files changed, 13 insertions(+), 119 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 6cac67a4f73..1f53e2784bf 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 @@ -426,7 +426,6 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, DELETE("delete"), CHECK_AND_PUT("checkAndPut"), CHECK_AND_DELETE("checkAndDelete"), - INCREMENT_COLUMN_VALUE("incrementColumnValue"), APPEND("append"), INCREMENT("increment"); @@ -1503,18 +1502,27 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, // We have a failure with table, cf and q perm checks and now giving a chance for cell // perm check OpType opType; + long timestamp; if (m instanceof Put) { checkForReservedTagPresence(user, m); opType = OpType.PUT; + timestamp = m.getTimestamp(); } else if (m instanceof Delete) { opType = OpType.DELETE; + timestamp = m.getTimestamp(); + } else if (m instanceof Increment) { + opType = OpType.INCREMENT; + timestamp = ((Increment) m).getTimeRange().getMax(); + } else if (m instanceof Append) { + opType = OpType.APPEND; + timestamp = ((Append) m).getTimeRange().getMax(); } else { - // If the operation type is not Put or Delete, do nothing + // If the operation type is not Put/Delete/Increment/Append, do nothing continue; } AuthResult authResult = null; if (checkCoveringPermission(user, opType, c.getEnvironment(), m.getRow(), - m.getFamilyCellMap(), m.getTimestamp(), Action.WRITE)) { + m.getFamilyCellMap(), timestamp, Action.WRITE)) { authResult = AuthResult.allow(opType.toString(), "Covering cell set", user, Action.WRITE, table, m.getFamilyCellMap()); } else { @@ -1695,32 +1703,6 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, return null; } - @Override - public Result preAppendAfterRowLock(final ObserverContext c, - final Append append) throws IOException { - if (append.getAttribute(CHECK_COVERING_PERM) != null) { - // We had failure with table, cf and q perm checks and now giving a chance for cell - // perm check - TableName table = c.getEnvironment().getRegion().getRegionInfo().getTable(); - AuthResult authResult = null; - User user = getActiveUser(c); - if (checkCoveringPermission(user, OpType.APPEND, c.getEnvironment(), append.getRow(), - append.getFamilyCellMap(), append.getTimeRange().getMax(), Action.WRITE)) { - authResult = AuthResult.allow(OpType.APPEND.toString(), - "Covering cell set", user, Action.WRITE, table, append.getFamilyCellMap()); - } else { - authResult = AuthResult.deny(OpType.APPEND.toString(), - "Covering cell set", user, Action.WRITE, table, append.getFamilyCellMap()); - } - AccessChecker.logResult(authResult); - if (authorizationEnabled && !authResult.isAllowed()) { - throw new AccessDeniedException("Insufficient permissions " + - authResult.toContextString()); - } - } - return null; - } - @Override public Result preIncrement(final ObserverContext c, final Increment increment) @@ -1756,32 +1738,6 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, return null; } - @Override - public Result preIncrementAfterRowLock(final ObserverContext c, - final Increment increment) throws IOException { - if (increment.getAttribute(CHECK_COVERING_PERM) != null) { - // We had failure with table, cf and q perm checks and now giving a chance for cell - // perm check - TableName table = c.getEnvironment().getRegion().getRegionInfo().getTable(); - AuthResult authResult = null; - User user = getActiveUser(c); - if (checkCoveringPermission(user, OpType.INCREMENT, c.getEnvironment(), increment.getRow(), - increment.getFamilyCellMap(), increment.getTimeRange().getMax(), Action.WRITE)) { - authResult = AuthResult.allow(OpType.INCREMENT.toString(), "Covering cell set", - user, Action.WRITE, table, increment.getFamilyCellMap()); - } else { - authResult = AuthResult.deny(OpType.INCREMENT.toString(), "Covering cell set", - user, Action.WRITE, table, increment.getFamilyCellMap()); - } - AccessChecker.logResult(authResult); - if (authorizationEnabled && !authResult.isAllowed()) { - throw new AccessDeniedException("Insufficient permissions " + - authResult.toContextString()); - } - } - return null; - } - @Override public List> postIncrementBeforeWAL( ObserverContext ctx, Mutation mutation, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java index 01f36348662..305c6d18d9f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java @@ -49,11 +49,9 @@ import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.Tag; import org.apache.hadoop.hbase.TagType; import org.apache.hadoop.hbase.client.Admin; -import org.apache.hadoop.hbase.client.Append; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Get; -import org.apache.hadoop.hbase.client.Increment; import org.apache.hadoop.hbase.client.MasterSwitchType; import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; @@ -73,7 +71,6 @@ import org.apache.hadoop.hbase.coprocessor.RegionCoprocessor; import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; import org.apache.hadoop.hbase.coprocessor.RegionObserver; import org.apache.hadoop.hbase.exceptions.DeserializationException; -import org.apache.hadoop.hbase.exceptions.FailedSanityCheckException; import org.apache.hadoop.hbase.filter.Filter; import org.apache.hadoop.hbase.filter.FilterBase; import org.apache.hadoop.hbase.filter.FilterList; @@ -322,7 +319,7 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso } } } - if (!sanityFailure) { + if (!sanityFailure && (m instanceof Put || m instanceof Delete)) { if (cellVisibility != null) { String labelsExp = cellVisibility.getExpression(); List visibilityTags = labelCache.get(labelsExp); @@ -359,7 +356,7 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso if (m instanceof Put) { Put p = (Put) m; p.add(cell); - } else if (m instanceof Delete) { + } else { Delete d = (Delete) m; d.add(cell); } @@ -469,35 +466,6 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso return pair; } - /** - * Checks whether cell contains any tag with type as VISIBILITY_TAG_TYPE. This - * tag type is reserved and should not be explicitly set by user. There are - * two versions of this method one that accepts pair and other without pair. - * In case of preAppend and preIncrement the additional operations are not - * needed like checking for STRING_VIS_TAG_TYPE and hence the API without pair - * could be used. - * - * @param cell - * @throws IOException - */ - private boolean checkForReservedVisibilityTagPresence(Cell cell) throws IOException { - // Bypass this check when the operation is done by a system/super user. - // This is done because, while Replication, the Cells coming to the peer - // cluster with reserved - // typed tags and this is fine and should get added to the peer cluster - // table - if (isSystemOrSuperUser()) { - return true; - } - Iterator tagsItr = PrivateCellUtil.tagsIterator(cell); - while (tagsItr.hasNext()) { - if (RESERVED_VIS_TAG_TYPES.contains(tagsItr.next().getType())) { - return false; - } - } - return true; - } - private void removeReplicationVisibilityTag(List tags) throws IOException { Iterator iterator = tags.iterator(); while (iterator.hasNext()) { @@ -656,36 +624,6 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso return Superusers.isSuperUser(VisibilityUtils.getActiveUser()); } - @Override - public Result preAppend(ObserverContext e, Append append) - throws IOException { - // If authorization is not enabled, we don't care about reserved tags - if (!authorizationEnabled) { - return null; - } - for (CellScanner cellScanner = append.cellScanner(); cellScanner.advance();) { - if (!checkForReservedVisibilityTagPresence(cellScanner.current())) { - throw new FailedSanityCheckException("Append contains cell with reserved type tag"); - } - } - return null; - } - - @Override - public Result preIncrement(ObserverContext e, Increment increment) - throws IOException { - // If authorization is not enabled, we don't care about reserved tags - if (!authorizationEnabled) { - return null; - } - for (CellScanner cellScanner = increment.cellScanner(); cellScanner.advance();) { - if (!checkForReservedVisibilityTagPresence(cellScanner.current())) { - throw new FailedSanityCheckException("Increment contains cell with reserved type tag"); - } - } - return null; - } - @Override public List> postIncrementBeforeWAL( ObserverContext ctx, Mutation mutation,