[apache#2179] Improvement: Improve security when creating and droppin…

…g schemas and tables (apache#2335) ### What changes were proposed in this pull request? Improve security when creating and dropping schemas and tables. This PR adds the following checks for identifier names using the capability framework - Regex check - As a best practice, it's generally advised to avoid including spaces in database names. In this PR, database names that include space will be considered illegal. - String length check, since SQL injection usually requires using longer string - Mysql: at most 64 characters - Postgresql: at most 63 characters We refer to specifications of the earliest version of DB that gravitino currently supports: - Postgresql identifier rules: https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS - Mysql identifier naming: https://dev.mysql.com/doc/refman/5.7/en/identifiers.html - Mysql identifier length limit: https://dev.mysql.com/doc/refman/5.7/en/identifier-length.html ### Why are the changes needed? Fix: apache#2179 ### Does this PR introduce _any_ user-facing change? Add name identifier checks before attempting to create or drop schemas and tables. ### How was this patch tested? Add IT tests.
shaofengshi · Jun 24, 2024 · 5c15214 · 5c15214
1 parent 31c14c1
commit 5c15214
Show file tree

Hide file tree

Showing 13 changed files with 704 additions and 8 deletions.
diff --git a/.../catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java b/.../catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java
@@ -13,6 +13,7 @@
 import com.datastrato.gravitino.connector.CatalogOperations;
 import com.datastrato.gravitino.connector.PropertiesMetadata;
 import com.datastrato.gravitino.connector.PropertyEntry;
+import com.datastrato.gravitino.connector.capability.Capability;
 import java.util.Collections;
 import java.util.Map;
 
@@ -52,6 +53,11 @@ protected CatalogOperations newOps(Map<String, String> config) {
     return ops;
   }
 
+  @Override
+  public Capability newCapability() {
+    return new JdbcCatalogCapability();
+  }
+
   /** @return The {@link JdbcExceptionConverter} to be used by the catalog. */
   protected JdbcExceptionConverter createExceptionConverter() {
     return new JdbcExceptionConverter() {};

diff --git a/...dbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java b/...dbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2024 Datastrato Pvt Ltd.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.catalog.jdbc;
+
+import com.datastrato.gravitino.connector.capability.Capability;
+import com.datastrato.gravitino.connector.capability.CapabilityResult;
+
+public class JdbcCatalogCapability implements Capability {
+  /**
+   * Regular expression explanation: Regex that matches any string that maybe a filename with an
+   * optional extension We adopt a blacklist approach that excludes filename or extension that
+   * contains '.', '/', or '\' ^[^.\/\\]+(\.[^.\/\\]+)?$
+   *
+   * <p>^ - Start of the string
+   *
+   * <p>[^.\/\\]+ - matches any filename string that does not contain '.', '/', or '\'
+   *
+   * <p>(\.[^.\/\\]+)? - matches an optional extension
+   *
+   * <p>$ - End of the string
+   */
+  // We use sqlite name pattern to be the default pattern for JDBC catalog for testing purposes
+  public static final String SQLITE_NAME_PATTERN = "^[^.\\/\\\\]+(\\.[^.\\/\\\\]+)?$";
+
+  @Override
+  public CapabilityResult specificationOnName(Scope scope, String name) {
+    // TODO: Validate the name against reserved words
+    if (!name.matches(SQLITE_NAME_PATTERN)) {
+      return CapabilityResult.unsupported(
+          String.format("The %s name '%s' is illegal.", scope, name));
+    }
+    return CapabilityResult.SUPPORTED;
+  }
+}
diff --git a/...catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java b/...catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java
@@ -17,6 +17,7 @@
 import com.datastrato.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations;
 import com.datastrato.gravitino.catalog.jdbc.operation.JdbcTableOperations;
 import com.datastrato.gravitino.connector.CatalogOperations;
+import com.datastrato.gravitino.connector.capability.Capability;
 import java.util.Map;
 
 /** Implementation of a Doris catalog in Gravitino. */
@@ -38,6 +39,11 @@ protected CatalogOperations newOps(Map<String, String> config) {
         createJdbcColumnDefaultValueConverter());
   }
 
+  @Override
+  public Capability newCapability() {
+    return new DorisCatalogCapability();
+  }
+
   @Override
   protected JdbcExceptionConverter createExceptionConverter() {
     return new DorisExceptionConverter();

diff --git a/...bc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java b/...bc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2024 Datastrato Pvt Ltd.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.catalog.doris;
+
+import com.datastrato.gravitino.connector.capability.Capability;
+
+public class DorisCatalogCapability implements Capability {
+  // Doris best practice mention that the name should be in lowercase, separated by underscores
+  // https://doris.apache.org/docs/2.0/table-design/best-practice/
+  // We can use the more general DEFAULT_NAME_PATTERN for Doris and update as needed in the future
+}
diff --git a/...src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java b/...src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java
@@ -26,18 +26,22 @@
 import com.datastrato.gravitino.rel.expressions.NamedReference;
 import com.datastrato.gravitino.rel.expressions.distributions.Distribution;
 import com.datastrato.gravitino.rel.expressions.distributions.Distributions;
+import com.datastrato.gravitino.rel.expressions.sorts.SortOrder;
 import com.datastrato.gravitino.rel.expressions.transforms.Transforms;
 import com.datastrato.gravitino.rel.indexes.Index;
 import com.datastrato.gravitino.rel.indexes.Indexes;
 import com.datastrato.gravitino.rel.types.Types;
+import com.datastrato.gravitino.utils.RandomNameUtils;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import org.apache.commons.lang3.StringUtils;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
@@ -256,6 +260,65 @@ void testDropDorisSchema() {
         });
   }
 
+  @Test
+  void testSchemaWithIllegalName() {
+    SupportsSchemas schemas = catalog.asSchemas();
+    String databaseName = RandomNameUtils.genRandomName("it_db");
+    Map<String, String> properties = new HashMap<>();
+    String comment = "comment";
+
+    // should throw an exception with string that might contain SQL injection
+    String sqlInjection = databaseName + "`; DROP TABLE important_table; -- ";
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.createSchema(sqlInjection, comment, properties);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.dropSchema(sqlInjection, false);
+        });
+
+    String sqlInjection1 = databaseName + "`; SLEEP(10); -- ";
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.createSchema(sqlInjection1, comment, properties);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.dropSchema(sqlInjection1, false);
+        });
+
+    String sqlInjection2 =
+        databaseName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- ";
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.createSchema(sqlInjection2, comment, properties);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.dropSchema(sqlInjection2, false);
+        });
+
+    // should throw an exception with input that has more than 64 characters
+    String invalidInput = StringUtils.repeat("a", 65);
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.createSchema(invalidInput, comment, properties);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          schemas.dropSchema(invalidInput, false);
+        });
+  }
+
   @Test
   void testDorisTableBasicOperation() {
     // create a table
@@ -299,6 +362,118 @@ void testDorisTableBasicOperation() {
         newTableName, table_comment, Arrays.asList(columns), properties, indexes, renamedTable);
   }
 
+  @Test
+  void testDorisIllegalTableName() {
+    Map<String, String> properties = createTableProperties();
+    TableCatalog tableCatalog = catalog.asTableCatalog();
+    String table_name = "t123";
+
+    String t1_name = table_name + "`; DROP TABLE important_table; -- ";
+    Column t1_col = Column.of(t1_name, Types.LongType.get(), "id", false, false, null);
+    Column[] columns = {t1_col};
+    Index[] t1_indexes = {Indexes.unique("u1_key", new String[][] {{t1_name}})};
+    NameIdentifier tableIdentifier =
+        NameIdentifier.of(metalakeName, catalogName, schemaName, t1_name);
+
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          tableCatalog.createTable(
+              tableIdentifier,
+              columns,
+              table_comment,
+              properties,
+              Transforms.EMPTY_TRANSFORM,
+              Distributions.NONE,
+              new SortOrder[0],
+              t1_indexes);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          catalog.asTableCatalog().dropTable(tableIdentifier);
+        });
+
+    String t2_name = table_name + "`; SLEEP(10); -- ";
+    Column t2_col = Column.of(t2_name, Types.LongType.get(), "id", false, false, null);
+    Index[] t2_indexes = {Indexes.unique("u2_key", new String[][] {{t2_name}})};
+    Column[] columns2 = new Column[] {t2_col};
+    NameIdentifier tableIdentifier2 =
+        NameIdentifier.of(metalakeName, catalogName, schemaName, t2_name);
+
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          tableCatalog.createTable(
+              tableIdentifier2,
+              columns2,
+              table_comment,
+              properties,
+              Transforms.EMPTY_TRANSFORM,
+              Distributions.NONE,
+              new SortOrder[0],
+              t2_indexes);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          catalog.asTableCatalog().dropTable(tableIdentifier2);
+        });
+
+    String t3_name =
+        table_name + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- ";
+    Column t3_col = Column.of(t3_name, Types.LongType.get(), "id", false, false, null);
+    Index[] t3_indexes = {Indexes.unique("u3_key", new String[][] {{t3_name}})};
+    Column[] columns3 = new Column[] {t3_col};
+    NameIdentifier tableIdentifier3 =
+        NameIdentifier.of(metalakeName, catalogName, schemaName, t3_name);
+
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          tableCatalog.createTable(
+              tableIdentifier3,
+              columns3,
+              table_comment,
+              properties,
+              Transforms.EMPTY_TRANSFORM,
+              Distributions.NONE,
+              new SortOrder[0],
+              t3_indexes);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          catalog.asTableCatalog().dropTable(tableIdentifier3);
+        });
+
+    String invalidInput = StringUtils.repeat("a", 65);
+    Column t4_col = Column.of(invalidInput, Types.LongType.get(), "id", false, false, null);
+    Index[] t4_indexes = {Indexes.unique("u4_key", new String[][] {{invalidInput}})};
+    Column[] columns4 = new Column[] {t4_col};
+    NameIdentifier tableIdentifier4 =
+        NameIdentifier.of(metalakeName, catalogName, schemaName, invalidInput);
+
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          tableCatalog.createTable(
+              tableIdentifier4,
+              columns4,
+              table_comment,
+              properties,
+              Transforms.EMPTY_TRANSFORM,
+              Distributions.NONE,
+              new SortOrder[0],
+              t4_indexes);
+        });
+    Assertions.assertThrows(
+        IllegalArgumentException.class,
+        () -> {
+          catalog.asTableCatalog().dropTable(tableIdentifier4);
+        });
+  }
+
   @Test
   void testAlterDorisTable() {
     // create a table

diff --git a/...catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java b/...catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java
@@ -18,6 +18,7 @@
 import com.datastrato.gravitino.catalog.mysql.operation.MysqlTableOperations;
 import com.datastrato.gravitino.connector.CatalogOperations;
 import com.datastrato.gravitino.connector.PropertiesMetadata;
+import com.datastrato.gravitino.connector.capability.Capability;
 import java.util.Map;
 
 /** Implementation of a Mysql catalog in Gravitino. */
@@ -42,6 +43,11 @@ protected CatalogOperations newOps(Map<String, String> config) {
         createJdbcColumnDefaultValueConverter());
   }
 
+  @Override
+  public Capability newCapability() {
+    return new MysqlCatalogCapability();
+  }
+
   @Override
   protected JdbcExceptionConverter createExceptionConverter() {
     return new MysqlExceptionConverter();

diff --git a/...bc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java b/...bc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java
@@ -0,0 +1,37 @@
+/*
+ * Copyright 2024 Datastrato Pvt Ltd.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.catalog.mysql;
+
+import com.datastrato.gravitino.connector.capability.Capability;
+import com.datastrato.gravitino.connector.capability.CapabilityResult;
+
+public class MysqlCatalogCapability implements Capability {
+  /**
+   * Regular expression explanation: ^[\w\p{L}-$/=]{1,64}$
+   *
+   * <p>^ - Start of the string
+   *
+   * <p>[\w\p{L}-$/=]{1,64} - Consist of 1 to 64 characters of letters (both cases), digits,
+   * underscores, any kind of letter from any language, hyphens, dollar signs, slashes or equal
+   * signs
+   *
+   * <p>\w - matches [a-zA-Z0-9_]
+   *
+   * <p>\p{L} - matches any kind of letter from any language
+   *
+   * <p>$ - End of the string
+   */
+  public static final String MYSQL_NAME_PATTERN = "^[\\w\\p{L}-$/=]{1,64}$";
+
+  @Override
+  public CapabilityResult specificationOnName(Scope scope, String name) {
+    // TODO: Validate the name against reserved words
+    if (!name.matches(MYSQL_NAME_PATTERN)) {
+      return CapabilityResult.unsupported(
+          String.format("The %s name '%s' is illegal.", scope, name));
+    }
+    return CapabilityResult.SUPPORTED;
+  }
+}