From: Andrew Purtell Subject: [PATCH] HBASE-10823 Resolve LATEST_TIMESTAMP to current server time before scanning for ACLs --- .../hbase/security/access/AccessController.java | 11 ++- .../access/TestCellACLWithMultipleVersions.java | 90 ++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) 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 ee30a7d..ff37475 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 @@ -92,6 +92,7 @@ import org.apache.hadoop.hbase.security.UserProvider; import org.apache.hadoop.hbase.security.access.Permission.Action; import org.apache.hadoop.hbase.util.ByteRange; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.Pair; import org.apache.hadoop.hbase.util.SimpleByteRange; import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher; @@ -513,7 +514,15 @@ public class AccessController extends BaseRegionObserver int cellsChecked = 0; if (canPersistCellACLs) { Get get = new Get(row); - if (timestamp != HConstants.LATEST_TIMESTAMP) get.setTimeStamp(timestamp); + if (timestamp != HConstants.LATEST_TIMESTAMP) { + get.setTimeRange(0, timestamp + 1); + } else { + // Bound our search to the current (server) time. Storing values with timestamps in the + // future is bad practice and can lead to surprises but we allow that. If cells with + // timestamps in the future have ACLs, permissions from those ACLs will incorrectly be + // considered for authorizing the pending mutation at current server time. + get.setTimeRange(0, EnvironmentEdgeManager.currentTimeMillis() + 1); + } if (allVersions) { get.setMaxVersions(); } else { 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 582ea41..4b67be5 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 @@ -42,6 +42,7 @@ import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.regionserver.RegionServerCoprocessorHost; import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.TestTableName; import org.apache.log4j.Level; @@ -71,6 +72,7 @@ public class TestCellACLWithMultipleVersions extends SecureTestUtil { private static final byte[] TEST_FAMILY = Bytes.toBytes("f1"); private static final byte[] TEST_ROW = Bytes.toBytes("cellpermtest"); private static final byte[] TEST_Q1 = Bytes.toBytes("q1"); + private static final byte[] TEST_Q2 = Bytes.toBytes("q2"); private static final byte[] ZERO = Bytes.toBytes(0L); private static Configuration conf; @@ -349,6 +351,94 @@ public class TestCellACLWithMultipleVersions extends SecureTestUtil { }); } + + @Test + public void testDeleteWithFutureTimestamp() throws Exception { + // Store two values, one in the future + + verifyAllowed(new AccessTestAction() { + @Override + public Object run() throws Exception { + HTable t = new HTable(conf, TEST_TABLE.getTableName()); + try { + // Store read only ACL at a future time + Put p = new Put(TEST_ROW).add(TEST_FAMILY, TEST_Q1, + EnvironmentEdgeManager.currentTimeMillis() + 1000000, + ZERO); + p.setACL(USER_OTHER.getShortName(), new Permission(Permission.Action.READ)); + t.put(p); + // Store a read write ACL without a timestamp, server will use current time + p = new Put(TEST_ROW).add(TEST_FAMILY, TEST_Q2, ZERO); + p.setACL(USER_OTHER.getShortName(), new Permission(Permission.Action.READ, + Permission.Action.WRITE)); + t.put(p); + } finally { + t.close(); + } + return null; + } + }, USER_OWNER); + + // Confirm stores are visible + + AccessTestAction getQ1 = new AccessTestAction() { + @Override + public Object run() throws Exception { + Get get = new Get(TEST_ROW).addColumn(TEST_FAMILY, TEST_Q1); + HTable t = new HTable(conf, TEST_TABLE.getTableName()); + try { + return t.get(get).listCells(); + } finally { + t.close(); + } + } + }; + + AccessTestAction getQ2 = new AccessTestAction() { + @Override + public Object run() throws Exception { + Get get = new Get(TEST_ROW).addColumn(TEST_FAMILY, TEST_Q2); + HTable t = new HTable(conf, TEST_TABLE.getTableName()); + try { + return t.get(get).listCells(); + } finally { + t.close(); + } + } + }; + + verifyAllowed(getQ1, USER_OWNER, USER_OTHER); + verifyAllowed(getQ2, USER_OWNER, USER_OTHER); + + + // Issue a DELETE for the family, should succeed because the future ACL is + // not considered + + AccessTestAction deleteFamily = new AccessTestAction() { + @Override + public Object run() throws Exception { + Delete delete = new Delete(TEST_ROW).deleteFamily(TEST_FAMILY); + HTable t = new HTable(conf, TEST_TABLE.getTableName()); + try { + t.delete(delete); + } finally { + t.close(); + } + return null; + } + }; + + verifyAllowed(deleteFamily, USER_OTHER); + + // The future put should still exist + + verifyAllowed(getQ1, USER_OWNER, USER_OTHER); + + // The other put should be covered by the tombstone + + verifyDenied(getQ2, USER_OTHER); + } + @After public void tearDown() throws Exception { // Clean the _acl_ table -- 1.8.3.2