From 53f61ef8d5eb0e9d29bb50fa4b2a3ac40a26751f Mon Sep 17 00:00:00 2001 From: YutSean <33572832+YutSean@users.noreply.github.com> Date: Fri, 18 Jun 2021 11:15:45 +0800 Subject: [PATCH] HBASE-26001 When turn on access control, the cell level TTL of Increment and Append operations is invalid (#3385) Signed-off-by: Reid Chan --- .../security/access/AccessController.java | 47 ++---- .../TestPostIncrementAndAppendBeforeWAL.java | 137 +++++++++++++++++- 2 files changed, 147 insertions(+), 37 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 75bc73ccdcd..ec5a1b8a312 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 @@ -127,7 +127,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; -import org.apache.hbase.thirdparty.com.google.common.collect.ArrayListMultimap; import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet; import org.apache.hbase.thirdparty.com.google.common.collect.ListMultimap; import org.apache.hbase.thirdparty.com.google.common.collect.Lists; @@ -1740,7 +1739,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, List> cellPairs) throws IOException { // If the HFile version is insufficient to persist tags, we won't have any // work to do here - if (!cellFeaturesEnabled) { + if (!cellFeaturesEnabled || mutation.getACL() == null) { return cellPairs; } return cellPairs.stream().map(pair -> new Pair<>(pair.getFirst(), @@ -1754,7 +1753,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, List> cellPairs) throws IOException { // If the HFile version is insufficient to persist tags, we won't have any // work to do here - if (!cellFeaturesEnabled) { + if (!cellFeaturesEnabled || mutation.getACL() == null) { return cellPairs; } return cellPairs.stream().map(pair -> new Pair<>(pair.getFirst(), @@ -1763,50 +1762,28 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, } private Cell createNewCellWithTags(Mutation mutation, Cell oldCell, Cell newCell) { - // Collect any ACLs from the old cell + // As Increment and Append operations have already copied the tags of oldCell to the newCell, + // there is no need to rewrite them again. Just extract non-acl tags of newCell if we need to + // add a new acl tag for the cell. Actually, oldCell is useless here. List tags = Lists.newArrayList(); - List aclTags = Lists.newArrayList(); - ListMultimap perms = ArrayListMultimap.create(); - if (oldCell != null) { - Iterator tagIterator = PrivateCellUtil.tagsIterator(oldCell); + if (newCell != null) { + Iterator tagIterator = PrivateCellUtil.tagsIterator(newCell); while (tagIterator.hasNext()) { Tag tag = tagIterator.next(); if (tag.getType() != PermissionStorage.ACL_TAG_TYPE) { // Not an ACL tag, just carry it through if (LOG.isTraceEnabled()) { - LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType() - + " length " + tag.getValueLength()); + LOG.trace("Carrying forward tag from " + newCell + ": type " + tag.getType() + + " length " + tag.getValueLength()); } tags.add(tag); - } else { - aclTags.add(tag); } } } - // Do we have an ACL on the operation? - byte[] aclBytes = mutation.getACL(); - if (aclBytes != null) { - // Yes, use it - tags.add(new ArrayBackedTag(PermissionStorage.ACL_TAG_TYPE, aclBytes)); - } else { - // No, use what we carried forward - if (perms != null) { - // TODO: If we collected ACLs from more than one tag we may have a - // List of size > 1, this can be collapsed into a single - // Permission - if (LOG.isTraceEnabled()) { - LOG.trace("Carrying forward ACLs from " + oldCell + ": " + perms); - } - tags.addAll(aclTags); - } - } - - // If we have no tags to add, just return - if (tags.isEmpty()) { - return newCell; - } - + // We have checked the ACL tag of mutation is not null. + // So that the tags could not be empty. + tags.add(new ArrayBackedTag(PermissionStorage.ACL_TAG_TYPE, mutation.getACL())); return PrivateCellUtil.createCell(newCell, tags); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java index 031960b1513..f1e206b09b2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.IOException; +import java.util.Iterator; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -29,10 +30,15 @@ import java.util.stream.Collectors; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellBuilderType; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.ExtendedCellBuilderFactory; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.PrivateCellUtil; import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.Tag; +import org.apache.hadoop.hbase.TagBuilderFactory; +import org.apache.hadoop.hbase.TagType; import org.apache.hadoop.hbase.client.Append; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.Connection; @@ -45,6 +51,8 @@ import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.client.TestFromClientSide; import org.apache.hadoop.hbase.regionserver.NoSuchColumnFamilyException; +import org.apache.hadoop.hbase.security.access.AccessController; +import org.apache.hadoop.hbase.security.access.Permission; import org.apache.hadoop.hbase.testclassification.CoprocessorTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -64,7 +72,7 @@ import org.slf4j.LoggerFactory; * {@link RegionObserver#postIncrementBeforeWAL(ObserverContext, Mutation, List)} and * {@link RegionObserver#postAppendBeforeWAL(ObserverContext, Mutation, List)}. These methods may * change the cells which will be applied to memstore and WAL. So add unit test for the case which - * change the cell's column family. + * change the cell's column family and tags. */ @Category({CoprocessorTests.class, MediumTests.class}) public class TestPostIncrementAndAppendBeforeWAL { @@ -92,6 +100,10 @@ public class TestPostIncrementAndAppendBeforeWAL { private static final byte[] CQ1 = Bytes.toBytes("cq1"); private static final byte[] CQ2 = Bytes.toBytes("cq2"); private static final byte[] VALUE = Bytes.toBytes("value"); + private static final byte[] VALUE2 = Bytes.toBytes("valuevalue"); + private static final String USER = "User"; + private static final Permission PERMS = + Permission.newBuilder().withActions(Permission.Action.READ).build(); @BeforeClass public static void setupBeforeClass() throws Exception { @@ -161,6 +173,94 @@ public class TestPostIncrementAndAppendBeforeWAL { } } + @Test + public void testIncrementTTLWithACLTag() throws Exception { + TableName tableName = TableName.valueOf(name.getMethodName()); + createTableWithCoprocessor(tableName, ChangeCellWithACLTagObserver.class.getName()); + try (Table table = connection.getTable(tableName)) { + // Increment without TTL + Increment firstIncrement = new Increment(ROW).addColumn(CF1_BYTES, CQ1, 1) + .setACL(USER, PERMS); + Result result = table.increment(firstIncrement); + assertEquals(1, result.size()); + assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1))); + + // Check if the new cell can be read + Get get = new Get(ROW).addColumn(CF1_BYTES, CQ1); + result = table.get(get); + assertEquals(1, result.size()); + assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1))); + + // Increment with TTL + Increment secondIncrement = new Increment(ROW).addColumn(CF1_BYTES, CQ1, 1).setTTL(1000) + .setACL(USER, PERMS); + result = table.increment(secondIncrement); + + // We should get value 2 here + assertEquals(1, result.size()); + assertEquals(2, Bytes.toLong(result.getValue(CF1_BYTES, CQ1))); + + // Wait 4s to let the second increment expire + Thread.sleep(4000); + get = new Get(ROW).addColumn(CF1_BYTES, CQ1); + result = table.get(get); + + // The value should revert to 1 + assertEquals(1, result.size()); + assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1))); + } + } + + @Test + public void testAppendTTLWithACLTag() throws Exception { + TableName tableName = TableName.valueOf(name.getMethodName()); + createTableWithCoprocessor(tableName, ChangeCellWithACLTagObserver.class.getName()); + try (Table table = connection.getTable(tableName)) { + // Append without TTL + Append firstAppend = new Append(ROW).addColumn(CF1_BYTES, CQ2, VALUE).setACL(USER, PERMS); + Result result = table.append(firstAppend); + assertEquals(1, result.size()); + assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2))); + + // Check if the new cell can be read + Get get = new Get(ROW).addColumn(CF1_BYTES, CQ2); + result = table.get(get); + assertEquals(1, result.size()); + assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2))); + + // Append with TTL + Append secondAppend = new Append(ROW).addColumn(CF1_BYTES, CQ2, VALUE).setTTL(1000) + .setACL(USER, PERMS); + result = table.append(secondAppend); + + // We should get "valuevalue"" + assertEquals(1, result.size()); + assertTrue(Bytes.equals(VALUE2, result.getValue(CF1_BYTES, CQ2))); + + // Wait 4s to let the second append expire + Thread.sleep(4000); + get = new Get(ROW).addColumn(CF1_BYTES, CQ2); + result = table.get(get); + + // The value should revert to "value" + assertEquals(1, result.size()); + assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2))); + } + } + + private static boolean checkAclTag(byte[] acl, Cell cell) { + Iterator iter = PrivateCellUtil.tagsIterator(cell); + while (iter.hasNext()) { + Tag tag = iter.next(); + if (tag.getType() == TagType.ACL_TAG_TYPE) { + Tag temp = TagBuilderFactory.create(). + setTagType(TagType.ACL_TAG_TYPE).setTagValue(acl).build(); + return Tag.matchingValue(tag, temp); + } + } + return false; + } + public static class ChangeCellWithDifferntColumnFamilyObserver implements RegionCoprocessor, RegionObserver { @Override @@ -232,4 +332,37 @@ public class TestPostIncrementAndAppendBeforeWAL { .collect(Collectors.toList()); } } -} \ No newline at end of file + + public static class ChangeCellWithACLTagObserver extends AccessController { + @Override + public Optional getRegionObserver() { + return Optional.of(this); + } + + @Override + public List> postIncrementBeforeWAL( + ObserverContext ctx, Mutation mutation, + List> cellPairs) throws IOException { + List> result = super.postIncrementBeforeWAL(ctx, mutation, cellPairs); + for (Pair pair : result) { + if (mutation.getACL() != null && !checkAclTag(mutation.getACL(), pair.getSecond())) { + throw new DoNotRetryIOException("Unmatched ACL tag."); + } + } + return result; + } + + @Override + public List> postAppendBeforeWAL( + ObserverContext ctx, Mutation mutation, + List> cellPairs) throws IOException { + List> result = super.postAppendBeforeWAL(ctx, mutation, cellPairs); + for (Pair pair : result) { + if (mutation.getACL() != null && !checkAclTag(mutation.getACL(), pair.getSecond())) { + throw new DoNotRetryIOException("Unmatched ACL tag."); + } + } + return result; + } + } +}