CASSANDRA-19556 trunk Stefan's take

CHANGES.txt

@@ -1,4 +1,5 @@
 5.1
+ * Guardrail to block DDL/DCL queries (CASSANDRA-19556)
  * Improve handling of transient replicas during range movements (CASSANDRA-19344)
  * Enable debounced internode log requests to be cancelled at shutdown (CASSANDRA-19514)
  * Correctly set last modified epoch when combining multistep operations into a single step (CASSANDRA-19538)

conf/cassandra.yaml

@@ -2013,6 +2013,10 @@ drop_compact_storage_enabled: false
 # Guardrail to allow/disallow bulk load of SSTables
 # bulk_load_enabled: true
 #
+# Guardrail to allow/disallow DDL/DCL statements
+# ddl_enabled: true
+# dcl_enabled: true
+#
 # Guardrail to warn or fail when using a page size greater than threshold.
 # The two thresholds default to -1 to disable.
 # page_size_warn_threshold: -1

src/java/org/apache/cassandra/config/Config.java

@@ -871,6 +871,8 @@ public static void setClientMode(boolean clientMode)
     public volatile boolean alter_table_enabled = true;
     public volatile boolean group_by_enabled = true;
     public volatile boolean bulk_load_enabled = true;
+    public volatile boolean ddl_enabled = true;
+    public volatile boolean dcl_enabled = true;
     public volatile boolean drop_truncate_table_enabled = true;
     public volatile boolean drop_keyspace_enabled = true;
     public volatile boolean secondary_indexes_enabled = true;

src/java/org/apache/cassandra/config/GuardrailsOptions.java

@@ -382,6 +382,34 @@ public void setBulkLoadEnabled(boolean enabled)
                                   x -> config.bulk_load_enabled = x);
     }
 
+    @Override
+    public boolean getDDLEnabled()
+    {
+        return config.ddl_enabled;
+    }
+
+    public void setDDLEnabled(boolean enabled)
+    {
+        updatePropertyWithLogging("ddl_enabled",
+                                  enabled,
+                                  () -> config.ddl_enabled,
+                                  x -> config.ddl_enabled = x);
+    }
+
+    @Override
+    public boolean getDCLEnabled()
+    {
+        return config.dcl_enabled;
+    }
+
+    public void setDCLEnabled(boolean enabled)
+    {
+        updatePropertyWithLogging("dcl_enabled",
+                                  enabled,
+                                  () -> config.dcl_enabled,
+                                  x -> config.dcl_enabled = x);
+    }
+
     @Override
     public boolean getSecondaryIndexesEnabled()
     {

src/java/org/apache/cassandra/cql3/CQLStatement.java

@@ -99,6 +99,16 @@ default public boolean hasConditions()
         return false;
     }
 
+    default boolean isDDLStatement()
+    {
+        return false;
+    }
+
+    default boolean isDCLStatement()
+    {
+        return false;
+    }
+
     public static abstract class Raw
     {
         protected VariableSpecifications bindVariables;

src/java/org/apache/cassandra/cql3/QueryProcessor.java

@@ -61,6 +61,7 @@
 import org.apache.cassandra.db.ReadResponse;
 import org.apache.cassandra.db.SinglePartitionReadQuery;
 import org.apache.cassandra.db.SystemKeyspace;
+import org.apache.cassandra.db.guardrails.Guardrails;
 import org.apache.cassandra.db.marshal.AbstractType;
 import org.apache.cassandra.db.partitions.PartitionIterator;
 import org.apache.cassandra.db.partitions.PartitionIterators;
@@ -278,6 +279,11 @@ public ResultMessage processStatement(CQLStatement statement, QueryState querySt
         statement.authorize(clientState);
         statement.validate(clientState);
 
+        if (statement.isDDLStatement())
+            Guardrails.ddlEnabled.ensureEnabled(clientState);
+        else if (statement.isDCLStatement())
+            Guardrails.dclEnabled.ensureEnabled(clientState);
+
         ResultMessage result = options.getConsistency() == ConsistencyLevel.NODE_LOCAL
                              ? processNodeLocalStatement(statement, queryState, options)
                              : statement.execute(queryState, options, queryStartNanoTime);
@@ -459,6 +465,11 @@ public static Prepared parseAndPrepare(String query, ClientState clientState, bo
         CQLStatement statement = raw.prepare(clientState);
         statement.validate(clientState);
 
+        if (statement.isDDLStatement())
+            Guardrails.ddlEnabled.ensureEnabled(clientState);
+        else if (statement.isDCLStatement())
+            Guardrails.dclEnabled.ensureEnabled(clientState);
+
         // Set CQL string for AlterSchemaStatement as this is used to serialize the transformation
         // in the cluster metadata log
         if (statement instanceof AlterSchemaStatement)

src/java/org/apache/cassandra/cql3/statements/AuthenticationStatement.java

@@ -68,5 +68,11 @@ public String obfuscatePassword(String query)
     {
         return query;
     }
+
+    @Override
+    public boolean isDCLStatement()
+    {
+        return true;
+    }
 }
 

src/java/org/apache/cassandra/cql3/statements/AuthorizationStatement.java

@@ -67,4 +67,10 @@ public String toString()
     {
         return ToStringBuilder.reflectionToString(this, ToStringStyle.SHORT_PREFIX_STYLE);
     }
+
+    @Override
+    public boolean isDCLStatement()
+    {
+        return true;
+    }
 }

src/java/org/apache/cassandra/cql3/statements/schema/AlterSchemaStatement.java

@@ -51,6 +51,12 @@ protected AlterSchemaStatement(String keyspaceName)
         this.keyspaceName = keyspaceName;
     }
 
+    @Override
+    public boolean isDDLStatement()
+    {
+        return true;
+    }
+
     public void setCql(String cql)
     {
         this.cql = cql;

src/java/org/apache/cassandra/db/guardrails/Guardrails.java

@@ -193,6 +193,24 @@ public final class Guardrails implements GuardrailsMBean
                    state -> CONFIG_PROVIDER.getOrCreate(state).getBulkLoadEnabled(),
                    "Bulk loading of SSTables").throwOnNullClientState(true);
 
+    /**
+     * Guardrail disabling DDL statements
+     */
+    public static final EnableFlag ddlEnabled =
+    new EnableFlag("ddl_enabled",
+                   null,
+                   state -> CONFIG_PROVIDER.getOrCreate(state).getDDLEnabled(),
+                   "DDL statement");
+
+    /**
+     * Guardrail disabling DCL statements
+     */
+    public static final EnableFlag dclEnabled =
+    new EnableFlag("dcl_enabled",
+                   null,
+                   state -> CONFIG_PROVIDER.getOrCreate(state).getDCLEnabled(),
+                   "DCL statement");
+
     /**
      * Guardrail disabling user's ability to turn off compression
      */
@@ -866,6 +884,30 @@ public void setBulkLoadEnabled(boolean enabled)
         DEFAULT_CONFIG.setBulkLoadEnabled(enabled);
     }
 
+    @Override
+    public boolean getDDLEnabled()
+    {
+        return DEFAULT_CONFIG.getDDLEnabled();
+    }
+
+    @Override
+    public void setDDLEnabled(boolean enabled)
+    {
+        DEFAULT_CONFIG.setDDLEnabled(enabled);
+    }
+
+    @Override
+    public boolean getDCLEnabled()
+    {
+        return DEFAULT_CONFIG.getDCLEnabled();
+    }
+
+    @Override
+    public void setDCLEnabled(boolean enabled)
+    {
+        DEFAULT_CONFIG.setDCLEnabled(enabled);
+    }
+
     @Override
     public int getPageSizeWarnThreshold()
     {

src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java

@@ -182,6 +182,20 @@ public interface GuardrailsConfig
      */
     boolean getBulkLoadEnabled();
 
+    /**
+     * Returns whether DDL statement is allowed
+     *
+     * @return {@code true} if allowed, {@code false} otherwise.
+     */
+    boolean getDDLEnabled();
+
+    /**
+     * Returns whether DCL statement is allowed
+     *
+     * @return {@code true} if allowed, {@code false} otherwise.
+     */
+    boolean getDCLEnabled();
+
     /**
      * @return The threshold to warn when page size exceeds given size.
      */

src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java

@@ -328,6 +328,30 @@ public interface GuardrailsMBean
      */
     void setBulkLoadEnabled(boolean enabled);
 
+    /**
+     * Returns whether DDL statement is allowed
+     *
+     * @return {@code true} if allowed, {@code false} otherwise.
+     */
+    boolean getDDLEnabled();
+
+    /**
+     * Sets whether DDL statement is allowed
+     */
+    void setDDLEnabled(boolean enabled);
+
+    /**
+     * Returns whether DCL statement is allowed
+     *
+     * @return {@code true} if allowed, {@code false} otherwise.
+     */
+    boolean getDCLEnabled();
+
+    /**
+     * Sets whether DCL statement is allowed
+     */
+    void setDCLEnabled(boolean enabled);
+
     /**
      * @return The threshold to warn when requested page size greater than threshold.
      * -1 means disabled.

test/unit/org/apache/cassandra/db/guardrails/GuardrailDCLEnabledTest.java

@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.guardrails;
+
+import java.net.InetSocketAddress;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.service.ClientState;
+
+import static java.lang.String.format;
+
+public class GuardrailDCLEnabledTest extends GuardrailTester
+{
+    private static final String TEST_USER = "testuser";
+    private static final String TEST_PW = "testpassword";
+    private static final String TEST_USER1 = "testuser1";
+    private static final String TEST_PW1 = "testpassword1";
+    private static final String TEST_KS = "dclks";
+    private static final String TEST_TABLE = "dcltbl";
+    private static final String DCL_ERROR_MSG = "DCL statement is not allowed";
+    private ClientState loginUserClientState;
+
+    private void setGuardrail(boolean enabled)
+    {
+        Guardrails.instance.setDCLEnabled(enabled);
+    }
+
+    @Before
+    public void beforeGuardrailTest() throws Throwable
+    {
+        super.beforeGuardrailTest();
+        // create user in login state
+        useSuperUser();
+        executeNet(getCreateRoleCQL(TEST_USER, true, false, TEST_PW));
+        executeNet(format("GRANT ALL ON KEYSPACE %s TO %s", KEYSPACE, TEST_USER));
+        useUser(TEST_USER, TEST_PW);
+
+        loginUserClientState = ClientState.forExternalCalls(InetSocketAddress.createUnresolved("127.0.0.1", 1234));
+        loginUserClientState.login(new AuthenticatedUser(TEST_USER));
+        execute(loginUserClientState, "USE " + keyspace());
+
+        execute(superClientState, String.format("CREATE KEYSPACE IF NOT EXISTS %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", TEST_KS));
+        execute(superClientState, String.format("CREATE TABLE IF NOT EXISTS %s.%s (key text PRIMARY KEY, col1 int, col2 int)", TEST_KS, TEST_TABLE));
+    }
+
+    @After
+    public void afterTest()
+    {
+        setGuardrail(true);
+    }
+
+    @Test
+    public void testCannotCreateRoleWhileFeatureDisabled() throws Throwable
+    {
+        setGuardrail(false);
+        assertFails(() -> execute(loginUserClientState,
+                                  getCreateRoleCQL(TEST_USER1, true, false, TEST_PW1)),
+                    DCL_ERROR_MSG);
+        // no role is created
+        assertEmpty(execute(String.format("SELECT * FROM system_auth.roles WHERE role='%s'", TEST_USER1)));
+    }
+
+    @Test
+    public void testCannotGrantPermissionWhileFeatureDisabled() throws Throwable
+    {
+        setGuardrail(false);
+        assertFails(() -> execute(loginUserClientState,
+                                  getGrantPermissionCQL(TEST_USER, TEST_KS, TEST_TABLE)),
+                    DCL_ERROR_MSG);
+        // TEST_USER don't get permission on TEST_KS.TEST_TABLE
+        assertEmpty(execute(String.format("SELECT * FROM system_auth.role_permissions WHERE role='%s' AND resource='data/%s/%s'",
+                                          TEST_USER, TEST_KS, TEST_TABLE)));
+    }
+
+    @Test
+    public void testCannotRevokePermissionWhileFeatureDisabled() throws Throwable
+    {
+        setGuardrail(false);
+        assertFails(() -> execute(loginUserClientState,
+                                  String.format("REVOKE ALL ON KEYSPACE %s FROM %s", KEYSPACE, TEST_USER)),
+                    DCL_ERROR_MSG);
+        // TEST_USER permission wasn't revoked on KEYSPACE
+        assertRowCount(execute(String.format("SELECT * FROM system_auth.role_permissions WHERE role='%s' AND resource='data/%s'", TEST_USER, KEYSPACE)),
+                       1);
+    }
+
+    private static String getCreateRoleCQL(String role, boolean login, boolean superUser, String password)
+    {
+        return String.format("CREATE ROLE IF NOT EXISTS %s WITH LOGIN = %s AND SUPERUSER = %s AND PASSWORD = '%s'",
+                             role, login, superUser, password);
+    }
+
+    private static String getGrantPermissionCQL(String role, String ks, String tbl)
+    {
+        return String.format("GRANT ALL PERMISSIONS ON %s.%s TO %s;", ks, tbl, role);
+    }
+}