Skip to content

Disallow the usage of invalid object names in HMS #17788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Disallow the usage of invalid object names in HMS #17788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f41575
Disallow the usage of invalid object names in HMS
findinpath Jun 1, 2023
d5dd61c
empty
findinpath Jun 12, 2023
9443393
empty
findinpath Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions ...n/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.plugin.hive.metastore.thrift;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -95,6 +96,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.Set;
Expand Down Expand Up @@ -132,6 +134,7 @@
import static io.trino.plugin.hive.metastore.thrift.ThriftMetastoreUtil.updateStatisticsParameters;
import static io.trino.plugin.hive.util.HiveUtil.makePartName;
import static io.trino.spi.StandardErrorCode.ALREADY_EXISTS;
import static io.trino.spi.StandardErrorCode.GENERIC_USER_ERROR;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.security.PrincipalType.USER;
import static java.lang.String.format;
Expand All @@ -145,6 +148,7 @@ public class ThriftHiveMetastore
private static final Logger log = Logger.get(ThriftHiveMetastore.class);

private static final String DEFAULT_METASTORE_USER = "presto";
private static final CharMatcher DOT_MATCHER = CharMatcher.is('.');

private final Optional<ConnectorIdentity> identity;
private final HdfsEnvironment hdfsEnvironment;
Expand Down Expand Up @@ -928,6 +932,7 @@ public Optional<List<SchemaTableName>> getAllViews()
@Override
public void createDatabase(Database database)
{
validateObjectName(database.getName());
try {
retry()
.stopOn(AlreadyExistsException.class, InvalidObjectException.class, MetaException.class)
Expand Down Expand Up @@ -978,6 +983,9 @@ public void dropDatabase(String databaseName, boolean deleteData)
@Override
public void alterDatabase(String databaseName, Database database)
{
if (!Objects.equals(databaseName, database.getName())) {
validateObjectName(database.getName());
}
try {
retry()
.stopOn(NoSuchObjectException.class, MetaException.class)
Expand All @@ -1003,6 +1011,7 @@ public void alterDatabase(String databaseName, Database database)
@Override
public void createTable(Table table)
{
validateObjectName(table.getTableName());
try {
retry()
.stopOn(AlreadyExistsException.class, InvalidObjectException.class, MetaException.class, NoSuchObjectException.class)
Expand Down Expand Up @@ -1093,6 +1102,12 @@ private static boolean isManagedTable(Table table)
@Override
public void alterTable(String databaseName, String tableName, Table table)
{
if (!Objects.equals(databaseName, table.getDbName())) {
validateObjectName(table.getDbName());
}
if (!Objects.equals(tableName, table.getTableName())) {
validateObjectName(table.getTableName());
}
try {
retry()
.stopOn(InvalidOperationException.class, MetaException.class)
Expand Down Expand Up @@ -2024,4 +2039,20 @@ private static RuntimeException propagate(Throwable throwable)
throwIfUnchecked(throwable);
throw new RuntimeException(throwable);
}

private static void validateObjectName(String objectName)
{
if (isNullOrEmpty(objectName)) {
throw new IllegalArgumentException("The provided objectName cannot be null or empty");
}
if (DOT_MATCHER.matchesAllOf(objectName)) {
// '.' or '..' object names can cause the object to have an inaccurate location on the object storage
throw new TrinoException(GENERIC_USER_ERROR, format("Invalid object name: '%s'", objectName));
}
if (objectName.contains("/")) {
// Older HMS instances may allow names like 'foo/bar' which can cause managed tables to be
// saved in a different location than its intended schema directory
throw new TrinoException(GENERIC_USER_ERROR, format("Invalid object name: '%s'", objectName));
}
}
}
68 changes: 68 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import io.trino.testing.QueryRunner;
import io.trino.testing.minio.MinioClient;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import java.text.DateFormat;
Expand Down Expand Up @@ -1651,6 +1652,73 @@ public void testExternalLocationWithTrailingSpace()
assertUpdate("DROP TABLE " + tableName);
}

@Test(dataProvider = "invalidObjectNames")
public void testCreateSchemaInvalidName(String schemaName)
{
assertThatThrownBy(() -> assertUpdate("CREATE SCHEMA \"" + schemaName + "\""))
.hasMessage(format("Invalid object name: '%s'", schemaName));
}

@DataProvider
public Object[][] invalidObjectNames()
{
return new Object[][] {
{"."},
{".."},
{"foo/bar"}};
}

@Test
public void testCreateTableInvalidName()
{
assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + HIVE_TEST_SCHEMA + ".\".\" (col integer)"))
.hasMessageContaining("Invalid table name");
assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + HIVE_TEST_SCHEMA + ".\"..\" (col integer)"))
.hasMessageContaining("Invalid table name");
assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + HIVE_TEST_SCHEMA + ".\"...\" (col integer)"))
.hasMessage("Invalid table name");

for (String tableName : Arrays.asList("foo/bar", "foo/./bar", "foo/../bar")) {
assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + HIVE_TEST_SCHEMA + ".\"" + tableName + "\" (col integer)"))
.hasMessage(format("Invalid object name: '%s'", tableName));
assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + HIVE_TEST_SCHEMA + ".\"" + tableName + "\" (col) AS VALUES 1"))
.hasMessage(format("Invalid object name: '%s'", tableName));
}
}

@Test
public void testRenameSchemaToInvalidObjectName()
{
String schemaName = "test_rename_schema_invalid_name_" + randomNameSuffix();
assertUpdate("CREATE SCHEMA " + schemaName);

for (String invalidSchemaName : Arrays.asList(".", "..", "foo/bar")) {
assertThatThrownBy(() -> assertUpdate("ALTER SCHEMA hive." + schemaName + " RENAME TO \"" + invalidSchemaName + "\""))
.hasMessage(format("Invalid object name: '%s'", invalidSchemaName));
}

assertUpdate("DROP SCHEMA " + schemaName);
}

@Test
public void testRenameTableToInvalidObjectName()
{
String tableName = "test_rename_table_invalid_name_" + randomNameSuffix();
assertUpdate("CREATE TABLE %s (a_varchar varchar)".formatted(getFullyQualifiedTestTableName(tableName)));

for (String invalidTableName : Arrays.asList(".", "..", "foo/bar")) {
assertThatThrownBy(() -> assertUpdate("ALTER TABLE " + getFullyQualifiedTestTableName(tableName) + " RENAME TO \"" + invalidTableName + "\""))
.hasMessage(format("Invalid object name: '%s'", invalidTableName));
}

for (String invalidSchemaName : Arrays.asList(".", "..", "foo/bar")) {
assertThatThrownBy(() -> assertUpdate("ALTER TABLE " + getFullyQualifiedTestTableName(tableName) + " RENAME TO \"" + invalidSchemaName + "\".validTableName"))
.hasMessage(format("Invalid object name: '%s'", invalidSchemaName));
}

assertUpdate("DROP TABLE " + getFullyQualifiedTestTableName(tableName));
}

private void renamePartitionResourcesOutsideTrino(String tableName, String partitionColumn, String regionKey)
{
String partitionName = format("%s=%s", partitionColumn, regionKey);
Expand Down