Tighten up permissions on system keyspaces

beobal · frankgh · beobal · commit f33267c8fae6 · 2025-01-17T09:02:35.000Z
* Restrict which permissions can be granted on system keyspaces * Ensure that GRANT... ON ALL KEYSPACES excludes system keyspaces * Add system_traces to the always readable set Patch by Sam Tunnicliffe and Francisco Guerrero; reviewed by Sam Tunnicliffe and Francisco Guerrero for CASSANDRA-20090 Co-authored-by: Francisco Guerrero <frankgh@apache.org>
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,5 +1,6 @@
 3.0.31
- * Fix incorrect column identifier bytes problem when renaming a column (CASSANDRA-18965)
+ * Tighten up permissions on system keyspaces (CASSANDRA-20090)
+ * Fix incorrect column identifier bytes problem when renaming a column (CASSANDRA-18956)
  * Upgrade OWASP to 10.0.0 (CASSANDRA-19738)
 Merged from 2.2:
  * Add termin-8-jdk as a valid jdk8 candidate in the debian package (CASSANDRA-19752)
diff --git 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<Permission> ALL =
             Sets.immutableEnumSet(EnumSet.range(Permission.CREATE, Permission.EXECUTE));
     public static final Set<Permission> 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<Permission> 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
@@ -19,6 +19,7 @@
 
 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<? extends IResource> chain(IResource resource)
     {
-        List<IResource> chain = new ArrayList<IResource>();
+        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<? extends IResource> chain(IResource resource, Predicate<IResource> filter)
+    {
+
+        List<IResource> 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/config/DatabaseDescriptor.java b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java
@@ -862,16 +862,31 @@ public static IAuthenticator getAuthenticator()
         return authenticator;
     }
 
+    public static void setAuthenticator(IAuthenticator authenticator)
+    {
+        DatabaseDescriptor.authenticator = authenticator;
+    }
+
     public static IAuthorizer getAuthorizer()
     {
         return authorizer;
     }
 
+    public static void setAuthorizer(IAuthorizer authorizer)
+    {
+        DatabaseDescriptor.authorizer = authorizer;
+    }
+
     public static IRoleManager getRoleManager()
     {
         return roleManager;
     }
 
+    public static void setRoleManager(IRoleManager roleManager)
+    {
+        DatabaseDescriptor.roleManager = roleManager;
+    }
+
     public static int getPermissionsValidity()
     {
         return conf.permissions_validity_in_ms;
diff --git a/src/java/org/apache/cassandra/config/Schema.java b/src/java/org/apache/cassandra/config/Schema.java
@@ -125,6 +125,12 @@ public static boolean isReplicatedSystemKeyspace(String keyspaceName)
         return REPLICATED_SYSTEM_KEYSPACE_NAMES.contains(keyspaceName.toLowerCase());
     }
 
+    public static boolean isSystemKeyspace(String keyspaceName)
+    {
+        final String lowercaseKeyspaceName = keyspaceName.toLowerCase();
+        return LOCAL_SYSTEM_KEYSPACE_NAMES.contains(lowercaseKeyspaceName)
+               || REPLICATED_SYSTEM_KEYSPACE_NAMES.contains(lowercaseKeyspaceName);
+    }
     /**
      * load keyspace (keyspace) definitions, but do not initialize the keyspace instances.
      * Schema version may be updated as the result.
diff --git a/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java b/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java
@@ -17,14 +17,18 @@
  */
 package org.apache.cassandra.cql3.statements;
 
+import java.util.Collections;
 import java.util.Set;
 
+import org.apache.cassandra.auth.DataResource;
 import org.apache.cassandra.auth.IResource;
 import org.apache.cassandra.auth.Permission;
 import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.config.Schema;
 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.service.ClientState;
 import org.apache.cassandra.transport.messages.ResultMessage;
 
@@ -35,6 +39,23 @@ public GrantPermissionsStatement(Set<Permission> permissions, IResource resource
         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 system keyspaces
+            if (!data.isRootLevel()
+                && Schema.isSystemKeyspace(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
     {
         DatabaseDescriptor.getAuthorizer().grant(state.getUser(), permissions, resource, grantee);
diff --git a/src/java/org/apache/cassandra/service/ClientState.java b/src/java/org/apache/cassandra/service/ClientState.java
@@ -20,10 +20,11 @@
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.util.Arrays;
-import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
+import com.google.common.collect.ImmutableSet;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -41,6 +42,7 @@
 import org.apache.cassandra.exceptions.UnauthorizedException;
 import org.apache.cassandra.schema.SchemaKeyspace;
 import org.apache.cassandra.thrift.ThriftValidation;
+import org.apache.cassandra.tracing.TraceKeyspace;
 import org.apache.cassandra.utils.FBUtilities;
 import org.apache.cassandra.utils.JVMStabilityInspector;
 import org.apache.cassandra.utils.CassandraVersion;
@@ -54,29 +56,40 @@ public class ClientState
     private static final Logger logger = LoggerFactory.getLogger(ClientState.class);
     public static final CassandraVersion DEFAULT_CQL_VERSION = org.apache.cassandra.cql3.QueryProcessor.CQL_VERSION;
 
-    private static final Set<IResource> READABLE_SYSTEM_RESOURCES = new HashSet<>();
-    private static final Set<IResource> PROTECTED_AUTH_RESOURCES = new HashSet<>();
-    private static final Set<IResource> DROPPABLE_SYSTEM_AUTH_TABLES = new HashSet<>();
+    public static final ImmutableSet<IResource> READABLE_SYSTEM_RESOURCES;
+    public static final ImmutableSet<IResource> PROTECTED_AUTH_RESOURCES;
+    public static final ImmutableSet<IResource> DROPPABLE_SYSTEM_AUTH_TABLES;
     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.PEERS))
-            READABLE_SYSTEM_RESOURCES.add(DataResource.table(SystemKeyspace.NAME, cf));
+        ImmutableSet.Builder<IResource> readableBuilder = ImmutableSet.builder();
+        for (String cf : Arrays.asList(SystemKeyspace.LOCAL, SystemKeyspace.PEERS, SystemKeyspace.SIZE_ESTIMATES))
+            readableBuilder.add(DataResource.table(SystemKeyspace.NAME, cf));
 
-        SchemaKeyspace.ALL.forEach(table -> READABLE_SYSTEM_RESOURCES.add(DataResource.table(SchemaKeyspace.NAME, table)));
+        // make all schema tables readable by default (required by the drivers)
+        SchemaKeyspace.ALL.forEach(table -> readableBuilder.add(DataResource.table(SchemaKeyspace.NAME, table)));
 
+        // make system_traces readable by all or else tracing will require explicit grants
+        readableBuilder.add(DataResource.table(TraceKeyspace.NAME, TraceKeyspace.EVENTS));
+        readableBuilder.add(DataResource.table(TraceKeyspace.NAME, TraceKeyspace.SESSIONS));
+        READABLE_SYSTEM_RESOURCES = readableBuilder.build();
+
+        ImmutableSet.Builder<IResource> protectedBuilder = ImmutableSet.builder();
+        // neither clients nor tools need authentication/authorization
         if (!Config.isClientMode())
         {
-            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();
+        ImmutableSet.Builder<IResource> droppableBuilder = ImmutableSet.builder();
         // allow users with sufficient privileges to drop legacy tables (users, credentials, permissions) from AUTH_KS
-        DROPPABLE_SYSTEM_AUTH_TABLES.add(DataResource.table(AuthKeyspace.NAME, PasswordAuthenticator.LEGACY_CREDENTIALS_TABLE));
-        DROPPABLE_SYSTEM_AUTH_TABLES.add(DataResource.table(AuthKeyspace.NAME, CassandraRoleManager.LEGACY_USERS_TABLE));
-        DROPPABLE_SYSTEM_AUTH_TABLES.add(DataResource.table(AuthKeyspace.NAME, CassandraAuthorizer.USER_PERMISSIONS));
+        droppableBuilder.add(DataResource.table(AuthKeyspace.NAME, PasswordAuthenticator.LEGACY_CREDENTIALS_TABLE));
+        droppableBuilder.add(DataResource.table(AuthKeyspace.NAME, CassandraRoleManager.LEGACY_USERS_TABLE));
+        droppableBuilder.add(DataResource.table(AuthKeyspace.NAME, CassandraAuthorizer.USER_PERMISSIONS));
+        DROPPABLE_SYSTEM_AUTH_TABLES = droppableBuilder.build();
     }
 
     // Current user for the session
@@ -328,9 +341,12 @@ private void hasAccess(String keyspace, Permission perm, DataResource resource)
 
         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));
@@ -348,7 +364,25 @@ public void ensureHasPermission(Permission perm, IResource resource) throws Unau
             if (((FunctionResource)resource).getKeyspace().equals(SystemKeyspace.NAME))
                 return;
 
-        checkPermissionOnResourceChain(perm, resource);
+        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 (Schema.isSystemKeyspace(keyspace))
+                {
+                    ensurePermissionOnResourceChain(perm, Resources.chain(dataResource, IResource::hasParent));
+                    return;
+                }
+            }
+        }
+
+        ensurePermissionOnResourceChain(perm, resource);
     }
 
     // Convenience method called from checkAccess method of CQLStatement
@@ -363,14 +397,20 @@ public void ensureHasPermission(Permission permission, Function function)
         if (function.isNative())
             return;
 
-        checkPermissionOnResourceChain(permission, FunctionResource.function(function.name().keyspace,
-                                                                             function.name().name,
-                                                                             function.argTypes()));
+        ensurePermissionOnResourceChain(permission, FunctionResource.function(function.name().keyspace,
+                                                                              function.name().name,
+                                                                              function.argTypes()));
+    }
+
+    private void ensurePermissionOnResourceChain(Permission perm, IResource resource)
+    {
+        ensurePermissionOnResourceChain(perm, Resources.chain(resource));
     }
 
-    private void checkPermissionOnResourceChain(Permission perm, IResource resource)
+    private void ensurePermissionOnResourceChain(Permission perm, List<? extends IResource> resources)
     {
-        for (IResource r : Resources.chain(resource))
+        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
diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java

Original file line number	Diff line number	Diff line change
`@@ -862,16 +862,31 @@ public static IAuthenticator getAuthenticator()`
`862`	`862`	`return authenticator;`
`863`	`863`	`}`
`864`	`864`
	`865`	`+ public static void setAuthenticator(IAuthenticator authenticator)`
	`866`	`+ {`
	`867`	`+ DatabaseDescriptor.authenticator = authenticator;`
	`868`	`+ }`
	`869`	`+`
`865`	`870`	`public static IAuthorizer getAuthorizer()`
`866`	`871`	`{`
`867`	`872`	`return authorizer;`
`868`	`873`	`}`
`869`	`874`
	`875`	`+ public static void setAuthorizer(IAuthorizer authorizer)`
	`876`	`+ {`
	`877`	`+ DatabaseDescriptor.authorizer = authorizer;`
	`878`	`+ }`
	`879`	`+`
`870`	`880`	`public static IRoleManager getRoleManager()`
`871`	`881`	`{`
`872`	`882`	`return roleManager;`
`873`	`883`	`}`
`874`	`884`
	`885`	`+ public static void setRoleManager(IRoleManager roleManager)`
	`886`	`+ {`
	`887`	`+ DatabaseDescriptor.roleManager = roleManager;`
	`888`	`+ }`
	`889`	`+`
`875`	`890`	`public static int getPermissionsValidity()`
`876`	`891`	`{`
`877`	`892`	`return conf.permissions_validity_in_ms;`