diff --git a/CHANGES.txt b/CHANGES.txt index 6794a0606744..06a6c2b08c66 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -24,6 +24,10 @@ Merged from 4.0: * Fix a race condition where a keyspace can be oopened while it is being removed (CASSANDRA-17658) +4.0.17 + * Tighten up permission on system keyspaces (CASSANDRA-20040) + + 4.0.12 * Migrate Python optparse to argparse (CASSANDRA-17914) Merged from 3.11: diff --git a/src/java/org/apache/cassandra/auth/Permission.java b/src/java/org/apache/cassandra/auth/Permission.java index d552280e64bd..303ab60e2642 100644 --- a/src/java/org/apache/cassandra/auth/Permission.java +++ b/src/java/org/apache/cassandra/auth/Permission.java @@ -66,4 +66,11 @@ public enum Permission public static final Set ALL = Sets.immutableEnumSet(EnumSet.range(Permission.CREATE, Permission.EXECUTE)); public static final Set NONE = ImmutableSet.of(); + + /** + * Set of Permissions which may never be granted on any system keyspace, or table in a system keyspace, to any role. + * (Only SELECT, DESCRIBE and ALTER may ever be granted). + */ + public static final Set INVALID_FOR_SYSTEM_KEYSPACES = + Sets.immutableEnumSet(EnumSet.complementOf(EnumSet.of(Permission.SELECT, Permission.DESCRIBE, Permission.ALTER))); } diff --git a/src/java/org/apache/cassandra/auth/Resources.java b/src/java/org/apache/cassandra/auth/Resources.java index 653cd46e324b..87a05bb52961 100644 --- a/src/java/org/apache/cassandra/auth/Resources.java +++ b/src/java/org/apache/cassandra/auth/Resources.java @@ -19,26 +19,43 @@ import java.util.ArrayList; import java.util.List; +import java.util.function.Predicate; import org.apache.cassandra.utils.Hex; public final class Resources { + /** * Construct a chain of resource parents starting with the resource and ending with the root. * - * @param resource The staring point. + * @param resource The starting point. * @return list of resource in the chain form start to the root. */ public static List chain(IResource resource) { - List chain = new ArrayList(); + return chain(resource, (r) -> true); + } + + /** + * Construct a chain of resource parents starting with the resource and ending with the root. Only resources which + * satisfy the supplied predicate will be included. + * + * @param resource The starting point. + * @param filter can be used to omit specific resources from the chain + * @return list of resource in the chain form start to the root. + */ + public static List chain(IResource resource, Predicate filter) + { + + List chain = new ArrayList<>(4); while (true) { - chain.add(resource); - if (!resource.hasParent()) - break; - resource = resource.getParent(); + if (filter.test(resource)) + chain.add(resource); + if (!resource.hasParent()) + break; + resource = resource.getParent(); } return chain; } diff --git a/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java b/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java index 824c4856d575..82706efd4ed4 100644 --- a/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java @@ -17,11 +17,13 @@ */ package org.apache.cassandra.cql3.statements; +import java.util.Collections; import java.util.Set; import java.util.stream.Collectors; import org.apache.cassandra.audit.AuditLogContext; import org.apache.cassandra.audit.AuditLogEntryType; +import org.apache.cassandra.auth.DataResource; import org.apache.cassandra.auth.IAuthorizer; import org.apache.cassandra.auth.IResource; import org.apache.cassandra.auth.Permission; @@ -29,8 +31,11 @@ import org.apache.cassandra.cql3.RoleName; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.exceptions.RequestValidationException; +import org.apache.cassandra.exceptions.UnauthorizedException; +import org.apache.cassandra.schema.SchemaConstants; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.service.ClientWarn; +import org.apache.cassandra.service.QueryState; import org.apache.cassandra.transport.messages.ResultMessage; public class GrantPermissionsStatement extends PermissionsManagementStatement @@ -40,6 +45,23 @@ public GrantPermissionsStatement(Set permissions, IResource resource super(permissions, resource, grantee); } + public void validate(QueryState state) throws RequestValidationException + { + super.validate(state); + if (resource instanceof DataResource) + { + DataResource data = (DataResource) resource; + // Only a subset of permissions can be granted on system keyspaces + if (!data.isRootLevel() + && SchemaConstants.isNonVirtualSystemKeyspace(data.getKeyspace()) + && !Collections.disjoint(permissions, Permission.INVALID_FOR_SYSTEM_KEYSPACES)) + { + throw new UnauthorizedException("Granting permissions on system keyspaces is strictly limited, " + + "this operation is not permitted"); + } + } + } + public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException { IAuthorizer authorizer = DatabaseDescriptor.getAuthorizer(); diff --git a/src/java/org/apache/cassandra/schema/SchemaConstants.java b/src/java/org/apache/cassandra/schema/SchemaConstants.java index 8fa1e76cbf20..a9bf09a44f2f 100644 --- a/src/java/org/apache/cassandra/schema/SchemaConstants.java +++ b/src/java/org/apache/cassandra/schema/SchemaConstants.java @@ -172,7 +172,17 @@ public static boolean isSystemKeyspace(String keyspaceName) || isReplicatedSystemKeyspace(keyspaceName) || isVirtualSystemKeyspace(keyspaceName); } - + + /** + * @return whether or not the keyspace is a non-virtual, system keyspace + */ + public static boolean isNonVirtualSystemKeyspace(String keyspaceName) + { + final String lowercaseKeyspaceName = keyspaceName.toLowerCase(); + return LOCAL_SYSTEM_KEYSPACE_NAMES.contains(lowercaseKeyspaceName) + || REPLICATED_SYSTEM_KEYSPACE_NAMES.contains(lowercaseKeyspaceName); + } + /** * @return whether or not the keyspace is a virtual keyspace (system_virtual_schema, system_views) */ diff --git a/src/java/org/apache/cassandra/service/ClientState.java b/src/java/org/apache/cassandra/service/ClientState.java index ca7dc3d44572..62c833a8a589 100644 --- a/src/java/org/apache/cassandra/service/ClientState.java +++ b/src/java/org/apache/cassandra/service/ClientState.java @@ -22,6 +22,7 @@ import java.net.SocketAddress; import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; @@ -403,9 +404,12 @@ private void ensurePermission(String keyspace, Permission perm, DataResource res preventSystemKSSchemaModification(keyspace, resource, perm); + // Some system data is always readable if ((perm == Permission.SELECT) && READABLE_SYSTEM_RESOURCES.contains(resource)) return; + // Modifications to any resource upon which the authenticator, authorizer or role manager depend should not be + // be performed by users if (PROTECTED_AUTH_RESOURCES.contains(resource)) if ((perm == Permission.CREATE) || (perm == Permission.ALTER) || (perm == Permission.DROP)) throw new UnauthorizedException(String.format("%s schema is protected", resource)); @@ -422,6 +426,24 @@ public void ensurePermission(Permission perm, IResource resource) if (((FunctionResource)resource).getKeyspace().equals(SchemaConstants.SYSTEM_KEYSPACE_NAME)) return; + if (resource instanceof DataResource && !(user.isSuper() || user.isSystem())) + { + DataResource dataResource = (DataResource)resource; + if (!dataResource.isRootLevel()) + { + String keyspace = dataResource.getKeyspace(); + // A user may have permissions granted on ALL KEYSPACES, but this should exclude system keyspaces. Any + // permission on those keyspaces or their tables must be granted to the user either explicitly or + // transitively. The set of grantable permissions for system keyspaces is further limited, + // see the Permission enum for details. + if (SchemaConstants.isSystemKeyspace(keyspace)) + { + ensurePermissionOnResourceChain(perm, Resources.chain(dataResource, IResource::hasParent)); + return; + } + } + } + ensurePermissionOnResourceChain(perm, resource); } @@ -444,7 +466,13 @@ public void ensurePermission(Permission permission, Function function) private void ensurePermissionOnResourceChain(Permission perm, IResource resource) { - for (IResource r : Resources.chain(resource)) + ensurePermissionOnResourceChain(perm, Resources.chain(resource)); + } + + private void ensurePermissionOnResourceChain(Permission perm, List resources) + { + IResource resource = resources.get(0); + for (IResource r : resources) if (authorize(r).contains(perm)) return; diff --git a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java index 9ae79426ff63..8c064cf5e134 100644 --- a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java +++ b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java @@ -17,22 +17,43 @@ */ package org.apache.cassandra.auth; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +import com.google.common.collect.Iterables; import org.junit.After; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; +import com.datastax.driver.core.Cluster; +import com.datastax.driver.core.PlainTextAuthProvider; import com.datastax.driver.core.ResultSet; +import com.datastax.driver.core.Session; +import com.datastax.driver.core.exceptions.UnauthorizedException; import org.apache.cassandra.Util; import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.SchemaConstants; +import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.db.SystemKeyspace; import org.apache.cassandra.service.StorageService; +import org.apache.cassandra.utils.Pair; import static org.junit.Assert.assertTrue; +import static java.lang.String.format; +import static org.apache.cassandra.schema.SchemaConstants.LOCAL_SYSTEM_KEYSPACE_NAMES; +import static org.apache.cassandra.schema.SchemaConstants.REPLICATED_SYSTEM_KEYSPACE_NAMES; public class GrantAndRevokeTest extends CQLTester { private static final String user = "user"; private static final String pass = "12345"; + private static final Pair USER = Pair.create(user, pass); + private static final Pair SUPERUSER = Pair.create("cassandra", "cassandra"); + private static int counter = 0; @BeforeClass public static void setUpClass() @@ -47,8 +68,20 @@ public static void setUpClass() @After public void tearDown() throws Throwable { - useSuperUser(); - executeNet("DROP ROLE " + user); + session(SUPERUSER).execute("DROP ROLE " + user); + Roles.clearCache(); + } + + private Session session(Pair credentials) + { + Cluster cluster = Cluster.builder() + .addContactPoints(nativeAddr) + .withClusterName("Test Cluster " + counter++) + .withPort(nativePort) + .withAuthProvider(new PlainTextAuthProvider(credentials.left, credentials.right)) + .build(); + + return cluster.connect(); } @Test @@ -75,4 +108,282 @@ public void testWarnings() throws Throwable res = executeNet("REVOKE MODIFY ON KEYSPACE revoke_yeah FROM " + user); assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted MODIFY on "); } + + @Test + public void testGrantedKeyspace() throws Throwable + { + Session superuser = session(SUPERUSER); + superuser.execute(String.format("CREATE ROLE %s WITH LOGIN = TRUE AND password='%s'", user, pass)); + superuser.execute("GRANT CREATE ON KEYSPACE " + KEYSPACE_PER_TEST + " TO " + user); + String table = KEYSPACE_PER_TEST + '.' + createTable(KEYSPACE_PER_TEST, "CREATE TABLE %s (pk int, ck int, val int, val_2 text, PRIMARY KEY (pk, ck))"); + String index = KEYSPACE_PER_TEST + ".idx_01"; + createIndex(KEYSPACE_PER_TEST, "CREATE INDEX idx_01 ON %s (val_2)"); + String type = KEYSPACE_PER_TEST + '.' + createType(KEYSPACE_PER_TEST, "CREATE TYPE %s (a int, b text)"); + String mv = KEYSPACE_PER_TEST + ".ks_mv_01"; + superuser.execute("CREATE MATERIALIZED VIEW " + mv + " AS SELECT * FROM " + table + " WHERE val IS NOT NULL AND pk IS NOT NULL AND ck IS NOT NULL PRIMARY KEY (val, pk, ck)"); + + Session nonsuperuser = session(USER); + // ALTER and DROP tables created by somebody else + // Spin assert for effective auth changes. + final String spinAssertTable = table; + Util.spinAssertEquals(false, () -> { + try + { + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "INSERT INTO %s (pk, ck, val, val_2) VALUES (1, 1, 1, '1')")); + } + catch(Throwable e) + { + return true; + } + return false; + }, 10); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "UPDATE %s SET val = 1 WHERE pk = 1 AND ck = 1")); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "DELETE FROM %s WHERE pk = 1 AND ck = 2")); + assertUnauthorizedQuery(nonsuperuser, "User user has no SELECT permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "SELECT * FROM %s WHERE pk = 1 AND ck = 1")); + assertUnauthorizedQuery(nonsuperuser, "User user has no SELECT permission on
or any of its parents", + "SELECT * FROM " + mv + " WHERE val = 1 AND pk = 1 AND ck = 1"); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "TRUNCATE TABLE %s")); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "ALTER TABLE %s ADD val_3 int")); + assertUnauthorizedQuery(nonsuperuser, "User user has no DROP permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "DROP TABLE %s")); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on or any of its parents", + "ALTER TYPE " + type + " ADD c bigint"); + assertUnauthorizedQuery(nonsuperuser, "User user has no DROP permission on or any of its parents", + "DROP TYPE " + type); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + "DROP MATERIALIZED VIEW " + mv); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + "DROP INDEX " + index); + + superuser.execute("GRANT ALTER ON KEYSPACE " + KEYSPACE_PER_TEST + " TO " + user); + superuser.execute("GRANT DROP ON KEYSPACE " + KEYSPACE_PER_TEST + " TO " + user); + superuser.execute("GRANT SELECT ON KEYSPACE " + KEYSPACE_PER_TEST + " TO " + user); + superuser.execute("GRANT MODIFY ON KEYSPACE " + KEYSPACE_PER_TEST + " TO " + user); + // Spin assert for effective auth changes. + Util.spinAssertEquals(false, () -> { + try + { + nonsuperuser.execute("ALTER KEYSPACE " + KEYSPACE_PER_TEST + " WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}"); + } + catch(Throwable e) + { + return true; + } + return false; + }, 10); + + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "INSERT INTO %s (pk, ck, val, val_2) VALUES (1, 1, 1, '1')")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "UPDATE %s SET val = 1 WHERE pk = 1 AND ck = 1")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "DELETE FROM %s WHERE pk = 1 AND ck = 2")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "SELECT * FROM %s WHERE pk = 1 AND ck = 1")); + nonsuperuser.execute("SELECT * FROM " + mv + " WHERE val = 1 AND pk = 1"); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "TRUNCATE TABLE %s")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "ALTER TABLE %s ADD val_3 int")); + nonsuperuser.execute("DROP MATERIALIZED VIEW " + mv); + nonsuperuser.execute("DROP INDEX " + index); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "DROP TABLE %s")); + nonsuperuser.execute("ALTER TYPE " + type + " ADD c bigint"); + nonsuperuser.execute("DROP TYPE " + type); + + // calling creatTableName to create a new table name that will be used by the formatQuery + table = createTableName(); + type = KEYSPACE_PER_TEST + "." + createTypeName(); + mv = KEYSPACE_PER_TEST + ".ks_mv_02"; + nonsuperuser.execute("CREATE TYPE " + type + " (a int, b text)"); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "CREATE TABLE %s (pk int, ck int, val int, val_2 text, PRIMARY KEY (pk, ck))")); + nonsuperuser.execute("CREATE MATERIALIZED VIEW " + mv + " AS SELECT * FROM " + table + " WHERE val IS NOT NULL AND pk IS NOT NULL AND ck IS NOT NULL PRIMARY KEY (val, pk, ck)"); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "INSERT INTO %s (pk, ck, val, val_2) VALUES (1, 1, 1, '1')")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "UPDATE %s SET val = 1 WHERE pk = 1 AND ck = 1")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "DELETE FROM %s WHERE pk = 1 AND ck = 2")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "SELECT * FROM %s WHERE pk = 1 AND ck = 1")); + nonsuperuser.execute("SELECT * FROM " + mv + " WHERE val = 1 AND pk = 1"); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "TRUNCATE TABLE %s")); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "ALTER TABLE %s ADD val_3 int")); + nonsuperuser.execute("DROP MATERIALIZED VIEW " + mv); + nonsuperuser.execute(formatQuery(KEYSPACE_PER_TEST, "DROP TABLE %s")); + nonsuperuser.execute("ALTER TYPE " + type + " ADD c bigint"); + nonsuperuser.execute("DROP TYPE " + type); + + superuser.execute("REVOKE ALTER ON KEYSPACE " + KEYSPACE_PER_TEST + " FROM " + user); + superuser.execute("REVOKE DROP ON KEYSPACE " + KEYSPACE_PER_TEST + " FROM " + user); + superuser.execute("REVOKE SELECT ON KEYSPACE " + KEYSPACE_PER_TEST + " FROM " + user); + superuser.execute("REVOKE MODIFY ON KEYSPACE " + KEYSPACE_PER_TEST + " FROM " + user); + + table = KEYSPACE_PER_TEST + "." + createTable(KEYSPACE_PER_TEST, "CREATE TABLE %s (pk int, ck int, val int, val_2 text, PRIMARY KEY (pk, ck))"); + type = KEYSPACE_PER_TEST + "." + createType(KEYSPACE_PER_TEST, "CREATE TYPE %s (a int, b text)"); + index = KEYSPACE_PER_TEST + ".idx_02"; + createIndex(KEYSPACE_PER_TEST, "CREATE INDEX idx_02 ON %s (val_2)"); + mv = KEYSPACE_PER_TEST + ".ks_mv_03"; + superuser.execute("CREATE MATERIALIZED VIEW " + mv + " AS SELECT * FROM " + table + " WHERE val IS NOT NULL AND pk IS NOT NULL AND ck IS NOT NULL PRIMARY KEY (val, pk, ck)"); + + // Spin assert for effective auth changes. + final String spinAssertTable2 = table; + Util.spinAssertEquals(false, () -> { + try + { + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "INSERT INTO " + spinAssertTable2 + " (pk, ck, val, val_2) VALUES (1, 1, 1, '1')"); + } + catch(Throwable e) + { + return true; + } + return false; + }, 10); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "UPDATE " + table + " SET val = 1 WHERE pk = 1 AND ck = 1"); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "DELETE FROM " + table + " WHERE pk = 1 AND ck = 2"); + assertUnauthorizedQuery(nonsuperuser, "User user has no SELECT permission on
or any of its parents", + "SELECT * FROM " + table + " WHERE pk = 1 AND ck = 1"); + assertUnauthorizedQuery(nonsuperuser, "User user has no SELECT permission on
or any of its parents", + "SELECT * FROM " + mv + " WHERE val = 1 AND pk = 1 AND ck = 1"); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "TRUNCATE TABLE " + table); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "ALTER TABLE " + table + " ADD val_3 int")); + assertUnauthorizedQuery(nonsuperuser, "User user has no DROP permission on
or any of its parents", + formatQuery(KEYSPACE_PER_TEST, "DROP TABLE " + table)); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on or any of its parents", + "ALTER TYPE " + type + " ADD c bigint"); + assertUnauthorizedQuery(nonsuperuser, "User user has no DROP permission on or any of its parents", + "DROP TYPE " + type); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + "DROP MATERIALIZED VIEW " + mv); + assertUnauthorizedQuery(nonsuperuser, "User user has no ALTER permission on
or any of its parents", + "DROP INDEX " + index); + } + + + @Test + public void testSpecificGrantsOnSystemKeyspaces() throws Throwable + { + // Granting specific permissions on system keyspaces should not be allowed if those permissions include any from + // the denylist Permission.INVALID_FOR_SYSTEM_KEYSPACES. By this definition, GRANT ALL on any system keyspace, + // or a table within one, should be rejected. + Session superuser = session(SUPERUSER); + superuser.execute("CREATE ROLE '" + user + "'"); + String responseMsg = "Granting permissions on system keyspaces is strictly limited, this operation is not permitted"; + for (String keyspace : Iterables.concat(LOCAL_SYSTEM_KEYSPACE_NAMES, REPLICATED_SYSTEM_KEYSPACE_NAMES)) + { + assertUnauthorizedQuery(superuser, responseMsg, format("GRANT ALL PERMISSIONS ON KEYSPACE %s TO %s", keyspace, user)); + DataResource keyspaceResource = DataResource.keyspace(keyspace); + for (Permission p : keyspaceResource.applicablePermissions()) + maybeRejectGrant(superuser, p, responseMsg, format("GRANT %s ON KEYSPACE %s TO %s", p.name(), keyspace, user)); + + for (TableMetadata table : Schema.instance.getKeyspaceMetadata(keyspace).tables) + { + DataResource tableResource = DataResource.table(keyspace, table.name); + assertUnauthorizedQuery(superuser, responseMsg, format("GRANT ALL PERMISSIONS ON %s.\"%s\" TO %s", table.keyspace, table.name, user)); + for (Permission p : tableResource.applicablePermissions()) + maybeRejectGrant(superuser, p, responseMsg, format("GRANT %s ON %s.\"%s\" TO %s", p.name(), table.keyspace, table.name, user)); + } + } + } + + @Test + public void testGrantOnAllKeyspaces() throws Throwable + { + // Granting either specific or ALL permissions on ALL KEYSPACES is allowed, however these permissions are + // effective for non-system keyspaces only. If for any reason it is necessary to modify permissions on + // on a system keyspace, it must be done using keyspace specific grant statements. + Session superuser = session(SUPERUSER); + superuser.execute(String.format("CREATE ROLE %s WITH LOGIN = TRUE AND password='%s'", user, pass)); + superuser.execute(String.format("ALTER KEYSPACE %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", SchemaConstants.TRACE_KEYSPACE_NAME)); + superuser.execute("CREATE KEYSPACE user_keyspace WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}"); + superuser.execute("CREATE TABLE user_keyspace.t1 (k int PRIMARY KEY)"); + + Session nonsuperuser = session(USER); + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "INSERT INTO user_keyspace.t1 (k) VALUES (0)"); + /* FIXME: 'User user has no MODIFY permission on
or any of its parents' + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "INSERT INTO system.local(key) VALUES ('invalid')"); + */ + + superuser.execute(format("GRANT MODIFY ON ALL KEYSPACES TO %s", user)); + // User now has write permission on non-system keyspaces only + nonsuperuser.execute("INSERT INTO user_keyspace.t1 (k) VALUES (0)"); + /* FIXME: 'User user has no MODIFY permission on
or any of its parents' + assertUnauthorizedQuery(nonsuperuser, "User user has no MODIFY permission on
or any of its parents", + "INSERT INTO system.local(key) VALUES ('invalid')"); + */ + + // A non-superuser only has read access to a pre-defined set of system tables and all system_schema/traces + // tables and granting ALL permissions on ALL keyspaces also does not affect this. + maybeReadSystemTables(nonsuperuser, false); + superuser.execute(format("GRANT ALL PERMISSIONS ON ALL KEYSPACES TO %s", user)); + maybeReadSystemTables(nonsuperuser, false); + + // A superuser can still read system tables + maybeReadSystemTables(superuser, true); + // and also write to them, though this is still strongly discouraged + /* FIXME: Modification is not supported by table system_views.peer_nodes + superuser.execute("INSERT INTO system.peers(peer, data_center) VALUES ('127.0.100.100', 'invalid_dc')"); + */ + + } + + private void maybeReadSystemTables(Session session, boolean isSuper) throws Throwable + { + Set readableKeyspaces = new HashSet<>(Arrays.asList(SchemaConstants.SCHEMA_KEYSPACE_NAME)); + Set readableSystemTables = new HashSet<>(Arrays.asList(SystemKeyspace.LOCAL, + SystemKeyspace.PEERS_V2, + SystemKeyspace.LEGACY_PEERS + // SystemKeyspace.LEGACY_SIZE_ESTIMATES, + // SystemKeyspace.TABLE_ESTIMATES + )); + + for (String keyspace : Iterables.concat(LOCAL_SYSTEM_KEYSPACE_NAMES, REPLICATED_SYSTEM_KEYSPACE_NAMES)) + { + for (TableMetadata table : Schema.instance.getKeyspaceMetadata(keyspace).tables) + { + if (isSuper || (readableKeyspaces.contains(keyspace) || (keyspace.equals(SchemaConstants.SYSTEM_KEYSPACE_NAME) && readableSystemTables.contains(table.name)))) + { + session.execute(format("SELECT * FROM %s.\"%s\" LIMIT 1", table.keyspace, table.name)); + } + else + { + assertUnauthorizedQuery(session, format("User %s has no SELECT permission on %s or any of its parents", user, DataResource.table(table.keyspace, table.name)), + format("SELECT * FROM %s.\"%s\" LIMIT 1", table.keyspace, table.name)); + } + } + } + } + + private void maybeRejectGrant(Session session, Permission p, String errorResponse, String grant) throws Throwable + { + if (Permission.INVALID_FOR_SYSTEM_KEYSPACES.contains(p)) + assertUnauthorizedQuery(session, errorResponse, grant); + else + session.execute(grant); + } + + private void assertUnauthorizedQuery(Session session, String errorMessage, String query) throws Throwable + { + try + { + session.execute(query); + Assert.fail("Query should be invalid but no error was thrown. Query is: " + query); + } + catch (Exception e) + { + if (!UnauthorizedException.class.isAssignableFrom(e.getClass())) + { + Assert.fail("Query should be invalid but wrong error was thrown. " + + "Expected: " + UnauthorizedException.class.getName() + ", got: " + e.getClass().getName() + ". " + + "Query is: " + query); + } + if (errorMessage != null) + { + assertMessageContains(errorMessage, e); + } + } + } } diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java index bba0a52774f2..b7037d67ecd4 100644 --- a/test/unit/org/apache/cassandra/cql3/CQLTester.java +++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java @@ -2154,7 +2154,7 @@ protected void assertInvalidSyntaxMessage(String errorMessage, String query, Obj * @param text the text that the exception message must contains * @param e the exception to check */ - private static void assertMessageContains(String text, Exception e) + protected static void assertMessageContains(String text, Exception e) { Assert.assertTrue("Expected error message to contain '" + text + "', but got '" + e.getMessage() + "'", e.getMessage().contains(text));