Skip to content

Allow only root user to read fate and scanref tables by default #6203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlmarion merged 2 commits into from
Mar 11, 2026
Merged

Allow only root user to read fate and scanref tables by default #6203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a58a743
Allow only root user to read fate and scanref tables by default
dlmarion Mar 9, 2026
81203ca
Implemented PR suggestions
dlmarion Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.apache.accumulo.core.client.TableNotFoundException;
import org.apache.accumulo.core.client.security.tokens.AuthenticationToken;
import org.apache.accumulo.core.clientImpl.Credentials;
import org.apache.accumulo.core.clientImpl.Namespace;
import org.apache.accumulo.core.clientImpl.thrift.SecurityErrorCode;
import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException;
import org.apache.accumulo.core.conf.Property;
Expand Down Expand Up @@ -355,6 +354,7 @@ private boolean _hasTablePermission(String user, TableId table, TablePermission
boolean useCached) throws ThriftSecurityException {
targetUserExists(user);

// Allow all users to read root and metadata tables
if ((table.equals(SystemTables.METADATA.tableId()) || table.equals(SystemTables.ROOT.tableId()))
&& permission.equals(TablePermission.READ)) {
return true;
Expand Down Expand Up @@ -384,10 +384,6 @@ private boolean _hasNamespacePermission(String user, NamespaceId namespace,

targetUserExists(user);

if (namespace.equals(Namespace.ACCUMULO.id()) && permission.equals(NamespacePermission.READ)) {
return true;
}

try {
if (useCached) {
return permHandle.hasCachedNamespacePermission(user, namespace.canonical(), permission);
Expand Down
62 changes: 61 additions & 1 deletion test/src/main/java/org/apache/accumulo/test/functional/PermissionsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

Expand Down Expand Up @@ -694,6 +695,42 @@ public void tablePermissionTest() throws Exception {
}
loginAs(rootUser);
try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) {

// check root user permissions on FATE and SCAN_REF tables
verifyHasOnlyTheseTablePermissions(c, c.whoami(), SystemTables.FATE.tableName());
verifyHasOnlyTheseTablePermissions(c, c.whoami(), SystemTables.SCAN_REF.tableName());
for (String tn : SystemTables.tableNames()) {
try (Scanner s = c.createScanner(tn)) {
if (SystemTables.ROOT.tableName().equals(tn)
|| SystemTables.METADATA.tableName().equals(tn)) {
@SuppressWarnings("unused")
var unused = s.iterator().hasNext();
} else {
IllegalStateException ise =
assertThrows(IllegalStateException.class, () -> s.iterator().hasNext());
assertTrue(ise.getMessage().contains("Error PERMISSION_DENIED for user"),
"Permission denied error not thrown for root user scan of table: " + tn
+ ". Message = " + ise.getMessage());
}
}
}

// Give the root user the read permission and try again.
c.securityOperations().grantTablePermission(rootUser.getPrincipal(),
SystemTables.FATE.tableName(), TablePermission.READ);
c.securityOperations().grantTablePermission(rootUser.getPrincipal(),
SystemTables.SCAN_REF.tableName(), TablePermission.READ);
verifyHasOnlyTheseTablePermissions(c, c.whoami(), SystemTables.FATE.tableName(),
TablePermission.READ);
verifyHasOnlyTheseTablePermissions(c, c.whoami(), SystemTables.SCAN_REF.tableName(),
TablePermission.READ);
for (String tn : SystemTables.tableNames()) {
try (Scanner s = c.createScanner(tn)) {
@SuppressWarnings("unused")
var unused = s.iterator().hasNext();
}
}

c.securityOperations().createLocalUser(principal, passwordToken);
loginAs(testUser);
try (AccumuloClient test_user_client =
Expand All @@ -703,8 +740,31 @@ public void tablePermissionTest() throws Exception {
loginAs(rootUser);
verifyHasOnlyTheseTablePermissions(c, c.whoami(), SystemTables.METADATA.tableName(),
TablePermission.READ, TablePermission.ALTER_TABLE);
String tableName = getUniqueNames(1)[0] + "__TABLE_PERMISSION_TEST__";

// check test user permissions on FATE and SCAN_REF tables
loginAs(testUser);
verifyHasOnlyTheseTablePermissions(c, test_user_client.whoami(),
SystemTables.FATE.tableName());
verifyHasOnlyTheseTablePermissions(c, test_user_client.whoami(),
SystemTables.SCAN_REF.tableName());
for (String tn : SystemTables.tableNames()) {
try (Scanner s = test_user_client.createScanner(tn)) {
if (SystemTables.ROOT.tableName().equals(tn)
|| SystemTables.METADATA.tableName().equals(tn)) {
@SuppressWarnings("unused")
var unused = s.iterator().hasNext();
} else {
IllegalStateException ise =
assertThrows(IllegalStateException.class, () -> s.iterator().hasNext());
assertTrue(ise.getMessage().contains("Error PERMISSION_DENIED for user"),
"Permission denied error not thrown for root user scan of table: " + tn
+ ". Message = " + ise.getMessage());
}
}
}

Copy link
Copy Markdown
Contributor

@keith-turner keith-turner Mar 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test scans the fate table here it will probably succeed because the namespace grants permission here. Would be good to also add a scan attempt here.

Seems like we need to remove this namespace code because there is table code that already grants anyone access to read metadata and root. Was not sure if removing that would impact the system user, but it does not seem it will because SecurityOperation has an explicit check that gives the system user all permissions.

Copy link
Copy Markdown
Member

@ctubbsii ctubbsii Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because SecurityOperation has an explicit check that gives the system user all permissions

Even that should probably be locked down. It doesn't need access to most tables, just read/write access to the system tables. But, that's out of scope of this PR and can be done separately.

loginAs(rootUser);
String tableName = getUniqueNames(1)[0] + "__TABLE_PERMISSION_TEST__";
// test each permission
for (TablePermission perm : TablePermission.values()) {
if (perm == TablePermission.ALTER_TABLE) {
Expand Down