diff --git a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java index 6c28fae23a1..a621088b9c4 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java +++ b/catalogs/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 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/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java new file mode 100644 index 00000000000..32cba18b8cd --- /dev/null +++ b/catalogs/catalog-jdbc-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 '\' ^[^.\/\\]+(\.[^.\/\\]+)?$ + * + *

^ - Start of the string + * + *

[^.\/\\]+ - matches any filename string that does not contain '.', '/', or '\' + * + *

(\.[^.\/\\]+)? - matches an optional extension + * + *

$ - 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/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java index 035182f4e97..b6a322ed65c 100644 --- a/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java +++ b/catalogs/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 config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new DorisCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new DorisExceptionConverter(); diff --git a/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java new file mode 100644 index 00000000000..ffa7ec057ed --- /dev/null +++ b/catalogs/catalog-jdbc-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/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java b/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java index 361c1bfc4d7..2dffdc0e26e 100644 --- a/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java +++ b/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java @@ -30,14 +30,17 @@ 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 +259,65 @@ void testDropDorisSchema() { }); } + @Test + void testSchemaWithIllegalName() { + SupportsSchemas schemas = catalog.asSchemas(); + String databaseName = RandomNameUtils.genRandomName("it_db"); + Map 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 diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java index 0ed1c3e8441..c8bb6bc1b7c 100644 --- a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java +++ b/catalogs/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 config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new MysqlCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new MysqlExceptionConverter(); diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java new file mode 100644 index 00000000000..44dafbdd389 --- /dev/null +++ b/catalogs/catalog-jdbc-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}$ + * + *

^ - Start of the string + * + *

[\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 + * + *

\w - matches [a-zA-Z0-9_] + * + *

\p{L} - matches any kind of letter from any language + * + *

$ - 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; + } +} diff --git a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java index e12382ba806..aba70e6719a 100644 --- a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java +++ b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java @@ -43,6 +43,7 @@ import com.datastrato.gravitino.rel.indexes.Indexes; import com.datastrato.gravitino.rel.types.Decimal; 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 com.google.common.collect.Sets; @@ -50,6 +51,7 @@ import java.sql.SQLException; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -1350,8 +1352,68 @@ void testNameSpec() { Assertions.assertTrue(catalog.asTableCatalog().dropTable(tableIdent)); Assertions.assertFalse(catalog.asTableCatalog().tableExists(tableIdent)); - Assertions.assertFalse(catalog.asTableCatalog().purgeTable(tableIdent)); + Assertions.assertThrows( + UnsupportedOperationException.class, + () -> { + catalog.asTableCatalog().purgeTable(tableIdent); + }); catalog.asSchemas().dropSchema(testSchemaName, true); + + // sql injection + String schemaName = RandomNameUtils.genRandomName("ct_db"); + Map properties = new HashMap<>(); + String comment = null; + + // should throw an exception with string that might contain SQL injection + String sqlInjection = schemaName + "`; DROP TABLE important_table; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(sqlInjection, false); + }); + + String sqlInjection1 = schemaName + "`; SLEEP(10); -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection1, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(sqlInjection1, false); + }); + + String sqlInjection2 = + schemaName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection2, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().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, + () -> { + catalog.asSchemas().createSchema(invalidInput, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(invalidInput, false); + }); } @Test diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java index c6293500af8..ecf9e7b510d 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java @@ -16,6 +16,7 @@ import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlSchemaOperations; import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlTableOperations; import com.datastrato.gravitino.connector.CatalogOperations; +import com.datastrato.gravitino.connector.capability.Capability; import java.util.Map; public class PostgreSqlCatalog extends JdbcCatalog { @@ -36,6 +37,11 @@ protected CatalogOperations newOps(Map config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new PostgreSqlCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new PostgreSqlExceptionConverter(); diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java new file mode 100644 index 00000000000..79bc47aae70 --- /dev/null +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java @@ -0,0 +1,33 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ +package com.datastrato.gravitino.catalog.postgresql; + +import com.datastrato.gravitino.connector.capability.Capability; +import com.datastrato.gravitino.connector.capability.CapabilityResult; + +public class PostgreSqlCatalogCapability implements Capability { + /** + * Regular expression explanation: ^[_a-zA-Z\p{L}/][\w\p{L}-$/=]{0,62}$ + * + *

^[_a-zA-Z\p{L}/] - Start with an underscore, a letter, or a letter from any language + * + *

[\w\p{L}-$/=]{0,62} - Consist of 0 to 62 characters (making the total length at most 63) of + * letters (both cases), digits, underscores, any kind of letter from any language, hyphens, + * dollar signs, slashes or equal signs + * + *

$ - End of the string + */ + public static final String POSTGRESQL_NAME_PATTERN = "^[_a-zA-Z\\p{L}/][\\w\\p{L}-$/=]{0,62}$"; + + @Override + public CapabilityResult specificationOnName(Scope scope, String name) { + // TODO: Validate the name against reserved words + if (!name.matches(POSTGRESQL_NAME_PATTERN)) { + return CapabilityResult.unsupported( + String.format("The %s name '%s' is illegal.", scope, name)); + } + return CapabilityResult.SUPPORTED; + } +} diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java b/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java index f369ba518d9..20563c2975e 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java @@ -42,6 +42,7 @@ import com.datastrato.gravitino.rel.types.Decimal; import com.datastrato.gravitino.rel.types.Types; import com.datastrato.gravitino.rel.types.Types.IntegerType; +import com.datastrato.gravitino.utils.RandomNameUtils; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -356,6 +357,76 @@ void testOperationPostgreSqlSchema() { Assertions.assertTrue(schemaNames.contains(schemaName)); } + @Test + void testSchemaWithIllegalName() { + SupportsSchemas schemas = catalog.asSchemas(); + String schemaName = RandomNameUtils.genRandomName("ct_db"); + + // should throw an exception with string that might contain SQL injection + String sqlInjection = schemaName + "; DROP TABLE important_table; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection, false); + }); + + String sqlInjection1 = schemaName + "; SELECT pg_sleep(10);"; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection1, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection1, false); + }); + + String sqlInjection2 = + schemaName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection2, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection2, false); + }); + + // should throw an exception with input that has more than 63 characters + String invalidInput = StringUtils.repeat("a", 64); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(invalidInput, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(invalidInput, false); + }); + + // should throw an exception with schema name that starts with special character + String invalidInput2 = RandomNameUtils.genRandomName("$test_db"); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(invalidInput2, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(invalidInput2, false); + }); + } + @Test void testCreateAndLoadPostgreSqlTable() { // Create table from Gravitino API diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java index 9c004989e80..62ad378d826 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java @@ -65,10 +65,7 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) @Override public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmptySchemaException { - // The constraints of the name spec may be more strict than underlying catalog, - // and for compatibility reasons, we only apply case-sensitive capabilities here. - return dispatcher.dropSchema( - applyCaseSensitive(ident, Capability.Scope.SCHEMA, dispatcher), cascade); + return dispatcher.dropSchema(normalizeNameIdentifier(ident), cascade); } private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) {