diff --git a/CHANGES.txt b/CHANGES.txt index 48ba5a2bae..17f31338ae 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -19,6 +19,9 @@ Merged from 4.0: * Fix text containing "/*" being interpreted as multiline comment in cqlsh (CASSANDRA-17667) * Fix indexing of a frozen collection that is the clustering key and reversed (CASSANDRA-19889) * Emit error when altering a table with non-frozen UDTs with nested non-frozen collections the same way as done upon table creation (CASSANDRA-19925) +Merged from 3.0 + * Tighten up permissions on system keyspaces (CASSANDRA-20090) + * Fix incorrect column identifier bytes problem when renaming a column (CASSANDRA-18956) 4.1.7 diff --git a/src/java/org/apache/cassandra/auth/Permission.java b/src/java/org/apache/cassandra/auth/Permission.java index d552280e64..11c7aeb05b 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 653cd46e32..2863710632 100644 --- a/src/java/org/apache/cassandra/auth/Resources.java +++ b/src/java/org/apache/cassandra/auth/Resources.java @@ -19,6 +19,7 @@ package org.apache.cassandra.auth; import java.util.ArrayList; import java.util.List; +import java.util.function.Predicate; import org.apache.cassandra.utils.Hex; @@ -27,18 +28,33 @@ 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 824c4856d5..5e165f5841 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,6 +31,8 @@ import org.apache.cassandra.config.DatabaseDescriptor; 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.transport.messages.ResultMessage; @@ -40,6 +44,23 @@ public class GrantPermissionsStatement extends PermissionsManagementStatement super(permissions, resource, grantee); } + public void validate(ClientState state) throws RequestValidationException + { + super.validate(state); + if (resource instanceof DataResource) + { + DataResource data = (DataResource) resource; + // Only a subset of permissions can be granted on non-virtual 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 888ea4a31f..d4cf888a9f 100644 --- a/src/java/org/apache/cassandra/schema/SchemaConstants.java +++ b/src/java/org/apache/cassandra/schema/SchemaConstants.java @@ -116,6 +116,16 @@ public final class SchemaConstants || isReplicatedSystemKeyspace(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); + } + /** * Returns the set of all system keyspaces * @return all system keyspaces diff --git a/src/java/org/apache/cassandra/service/ClientState.java b/src/java/org/apache/cassandra/service/ClientState.java index 9e35c7f4b0..21243cd68c 100644 --- a/src/java/org/apache/cassandra/service/ClientState.java +++ b/src/java/org/apache/cassandra/service/ClientState.java @@ -21,7 +21,6 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.Arrays; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -31,6 +30,7 @@ import java.util.concurrent.atomic.AtomicLong; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import org.slf4j.Logger; @@ -54,6 +54,7 @@ import org.apache.cassandra.dht.Datacenters; import org.apache.cassandra.exceptions.AuthenticationException; import org.apache.cassandra.exceptions.InvalidRequestException; import org.apache.cassandra.exceptions.UnauthorizedException; +import org.apache.cassandra.tracing.TraceKeyspace; import org.apache.cassandra.utils.FBUtilities; import org.apache.cassandra.utils.JVMStabilityInspector; import org.apache.cassandra.utils.MD5Digest; @@ -67,29 +68,39 @@ public class ClientState { private static final Logger logger = LoggerFactory.getLogger(ClientState.class); - private static final Set READABLE_SYSTEM_RESOURCES = new HashSet<>(); - private static final Set PROTECTED_AUTH_RESOURCES = new HashSet<>(); + public static final ImmutableSet READABLE_SYSTEM_RESOURCES; + public static final ImmutableSet PROTECTED_AUTH_RESOURCES; static { // We want these system cfs to be always readable to authenticated users since many tools rely on them // (nodetool, cqlsh, bulkloader, etc.) - for (String cf : Arrays.asList(SystemKeyspace.LOCAL, SystemKeyspace.LEGACY_PEERS, SystemKeyspace.PEERS_V2)) - READABLE_SYSTEM_RESOURCES.add(DataResource.table(SchemaConstants.SYSTEM_KEYSPACE_NAME, cf)); + ImmutableSet.Builder readableBuilder = ImmutableSet.builder(); + for (String cf : Arrays.asList(SystemKeyspace.LOCAL, SystemKeyspace.LEGACY_PEERS, SystemKeyspace.PEERS_V2, + SystemKeyspace.LEGACY_SIZE_ESTIMATES, SystemKeyspace.TABLE_ESTIMATES)) + readableBuilder.add(DataResource.table(SchemaConstants.SYSTEM_KEYSPACE_NAME, cf)); // make all schema tables readable by default (required by the drivers) - SchemaKeyspaceTables.ALL.forEach(table -> READABLE_SYSTEM_RESOURCES.add(DataResource.table(SchemaConstants.SCHEMA_KEYSPACE_NAME, table))); + SchemaKeyspaceTables.ALL.forEach(table -> readableBuilder.add(DataResource.table(SchemaConstants.SCHEMA_KEYSPACE_NAME, table))); + + // make system_traces readable by all or else tracing will require explicit grants + readableBuilder.add(DataResource.table(SchemaConstants.TRACE_KEYSPACE_NAME, TraceKeyspace.EVENTS)); + readableBuilder.add(DataResource.table(SchemaConstants.TRACE_KEYSPACE_NAME, TraceKeyspace.SESSIONS)); // make all virtual schema tables readable by default as well - VirtualSchemaKeyspace.instance.tables().forEach(t -> READABLE_SYSTEM_RESOURCES.add(t.metadata().resource)); + VirtualSchemaKeyspace.instance.tables().forEach(t -> readableBuilder.add(t.metadata().resource)); + READABLE_SYSTEM_RESOURCES = readableBuilder.build(); + ImmutableSet.Builder protectedBuilder = ImmutableSet.builder(); // neither clients nor tools need authentication/authorization if (DatabaseDescriptor.isDaemonInitialized()) { - PROTECTED_AUTH_RESOURCES.addAll(DatabaseDescriptor.getAuthenticator().protectedResources()); - PROTECTED_AUTH_RESOURCES.addAll(DatabaseDescriptor.getAuthorizer().protectedResources()); - PROTECTED_AUTH_RESOURCES.addAll(DatabaseDescriptor.getRoleManager().protectedResources()); + protectedBuilder.addAll(DatabaseDescriptor.getAuthenticator().protectedResources()); + protectedBuilder.addAll(DatabaseDescriptor.getAuthorizer().protectedResources()); + protectedBuilder.addAll(DatabaseDescriptor.getRoleManager().protectedResources()); } + + PROTECTED_AUTH_RESOURCES = protectedBuilder.build(); } // Current user for the session @@ -425,12 +436,16 @@ public class ClientState 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)); + ensurePermission(perm, resource); } @@ -444,6 +459,24 @@ public class ClientState if (((FunctionResource)resource).getKeyspace().equals(SchemaConstants.SYSTEM_KEYSPACE_NAME)) return; + if (resource instanceof DataResource && isOrdinaryUser()) + { + 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 non-virtual 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); } @@ -466,7 +499,12 @@ public class ClientState private void ensurePermissionOnResourceChain(Permission perm, IResource resource) { - List resources = Resources.chain(resource); + ensurePermissionOnResourceChain(perm, Resources.chain(resource)); + } + + private void ensurePermissionOnResourceChain(Permission perm, List resources) + { + IResource resource = resources.get(0); if (DatabaseDescriptor.getAuthFromRoot()) resources = Lists.reverse(resources); diff --git a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java index 5c1c2a0298..73923ea9d4 100644 --- a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java +++ b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java @@ -17,6 +17,11 @@ */ 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.BeforeClass; import org.junit.Test; @@ -25,7 +30,19 @@ import com.datastax.driver.core.ResultSet; import org.apache.cassandra.Util; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.db.ConsistencyLevel; +import org.apache.cassandra.db.SystemKeyspace; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.SchemaConstants; +import org.apache.cassandra.schema.TableMetadata; +import org.apache.cassandra.transport.ProtocolVersion; +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; +import static org.apache.cassandra.schema.SchemaConstants.SCHEMA_KEYSPACE_NAME; +import static org.apache.cassandra.schema.SchemaConstants.SYSTEM_KEYSPACE_NAME; +import static org.apache.cassandra.schema.SchemaConstants.TRACE_KEYSPACE_NAME; import static org.junit.Assert.assertTrue; public class GrantAndRevokeTest extends CQLTester @@ -202,7 +219,6 @@ public class GrantAndRevokeTest extends CQLTester "DROP MATERIALIZED VIEW " + mv); assertUnauthorizedQuery("User user has no ALTER permission on or any of its parents", "DROP INDEX " + index); - } @Test @@ -385,4 +401,111 @@ public class GrantAndRevokeTest extends CQLTester res = executeNet("REVOKE SELECT, MODIFY ON KEYSPACE revoke_yeah FROM " + user); assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted MODIFY on "); } + + @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. + useSuperUser(); + executeNet("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(responseMsg, format("GRANT ALL PERMISSIONS ON KEYSPACE %s TO %s", keyspace, user)); + DataResource keyspaceResource = DataResource.keyspace(keyspace); + for (Permission p : keyspaceResource.applicablePermissions()) + maybeRejectGrant(p, responseMsg, format("GRANT %s ON KEYSPACE %s TO %s", p.name(), keyspace, user)); + + assertUnauthorizedQuery(responseMsg, format("GRANT ALL PERMISSIONS ON ALL TABLES IN KEYSPACE %s TO %s", keyspace, user)); + for (TableMetadata table : Schema.instance.getKeyspaceMetadata(keyspace).tables) + { + DataResource tableResource = DataResource.table(keyspace, table.name); + assertUnauthorizedQuery(responseMsg, format("GRANT ALL PERMISSIONS ON %s TO %s", table, user)); + for (Permission p : tableResource.applicablePermissions()) + maybeRejectGrant(p, responseMsg, format("GRANT %s ON %s TO %s", p.name(), table, 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. + useSuperUser(); + executeNet(String.format("CREATE ROLE %s WITH LOGIN = TRUE AND password='%s'", user, pass)); + executeNet(String.format("ALTER KEYSPACE %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", SchemaConstants.TRACE_KEYSPACE_NAME)); + executeNet("CREATE KEYSPACE user_keyspace WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}"); + executeNet("CREATE TABLE user_keyspace.t1 (k int PRIMARY KEY)"); + useUser(user, pass); + + assertUnauthorizedQuery("User user has no MODIFY permission on
or any of its parents", + "INSERT INTO user_keyspace.t1 (k) VALUES (0)"); + assertUnauthorizedQuery("User user has no MODIFY permission on
or any of its parents", + "INSERT INTO system.local(key) VALUES ('invalid')"); + + useSuperUser(); + executeNet(ProtocolVersion.CURRENT, format("GRANT MODIFY ON ALL KEYSPACES TO %s", user)); + + useUser(user, pass); + // User now has write permission on non-system keyspaces only + executeNet(ProtocolVersion.CURRENT, "INSERT INTO user_keyspace.t1 (k) VALUES (0)"); + assertUnauthorizedQuery("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(false); + useSuperUser(); + executeNet(ProtocolVersion.CURRENT, format("GRANT ALL PERMISSIONS ON ALL KEYSPACES TO %s", user)); + maybeReadSystemTables(false); + + // A superuser can still read system tables + useSuperUser(); + maybeReadSystemTables(true); + // and also write to them, though this is still strongly discouraged + executeNet(ProtocolVersion.CURRENT, "INSERT INTO system.peers_v2(peer, peer_port, data_center) VALUES ('127.0.100.100', 7012, 'invalid_dc')"); + } + + private void maybeReadSystemTables(boolean superuser) throws Throwable + { + if (superuser) + useSuperUser(); + else + useUser(user, pass); + + Set readableKeyspaces = new HashSet<>(Arrays.asList(SCHEMA_KEYSPACE_NAME, TRACE_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 (superuser || (readableKeyspaces.contains(keyspace) || (keyspace.equals(SYSTEM_KEYSPACE_NAME) && readableSystemTables.contains(table.name)))) + { + executeNet(ProtocolVersion.CURRENT, ConsistencyLevel.ONE, format("SELECT * FROM %s LIMIT 1", table)); + } + else + { + assertUnauthorizedQuery(format("User %s has no SELECT permission on %s or any of its parents", user, table.resource), + format("SELECT * FROM %s LIMIT 1", table)); + } + } + } + } + + private void maybeRejectGrant(Permission p, String errorResponse, String grant) throws Throwable + { + if (Permission.INVALID_FOR_SYSTEM_KEYSPACES.contains(p)) + assertUnauthorizedQuery(errorResponse, grant); + else + executeNet(ProtocolVersion.CURRENT, grant); + } } diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java index 61bb4839b8..c20a0cea2a 100644 --- a/test/unit/org/apache/cassandra/cql3/CQLTester.java +++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java @@ -71,6 +71,7 @@ import org.apache.cassandra.concurrent.ScheduledExecutors; import org.apache.cassandra.concurrent.Stage; import org.apache.cassandra.config.DataStorageSpec; import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.virtual.VirtualKeyspaceRegistry; import org.apache.cassandra.db.virtual.VirtualSchemaKeyspace; import org.apache.cassandra.exceptions.InvalidRequestException; @@ -1258,6 +1259,13 @@ public abstract class CQLTester return Schema.instance.getTableMetadata(KEYSPACE, currentTable()); } + protected com.datastax.driver.core.ResultSet executeNet(ProtocolVersion protocolVersion, ConsistencyLevel consistency, String query) throws Throwable + { + Statement statement = new SimpleStatement(formatQuery(query)); + statement = statement.setConsistencyLevel(com.datastax.driver.core.ConsistencyLevel.valueOf(consistency.name())); + return sessionNet(protocolVersion).execute(statement); + } + protected com.datastax.driver.core.ResultSet executeNet(ProtocolVersion protocolVersion, String query, Object... values) throws Throwable { return sessionNet(protocolVersion).execute(formatQuery(query), values); diff --git a/test/unit/org/apache/cassandra/service/ClientStateTest.java b/test/unit/org/apache/cassandra/service/ClientStateTest.java index 56d0893356..05d1f78036 100644 --- a/test/unit/org/apache/cassandra/service/ClientStateTest.java +++ b/test/unit/org/apache/cassandra/service/ClientStateTest.java @@ -55,7 +55,9 @@ public class ClientStateTest SchemaLoader.createKeyspace(SchemaConstants.AUTH_KEYSPACE_NAME, KeyspaceParams.simple(1), Iterables.toArray(AuthKeyspace.metadata().tables, TableMetadata.class)); - + DatabaseDescriptor.setRoleManager(new AuthTestUtils.LocalCassandraRoleManager()); + DatabaseDescriptor.getRoleManager().setup(); + Roles.init(); AuthCacheService.initializeAndRegisterCaches(); }