Check catalog & schema access in USE statement #14209

huberty89 · 2022-09-20T07:30:20Z

Description

This PR add checks if user has an access to catalog and schema in USE statement.

Related issues, pull requests, and links

Fixes USE statement leaks names of catalogs and schemas that the user has no access to #14208

Non-technical explanation

There was a lack of check in USE statement if user has an access to catalog or schema.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Security
* Check access to catalog and schema during execution of USE statement. ({issue}`14208`)

kokosing

Add tests

kokosing · 2022-09-20T11:42:45Z

core/trino-main/src/main/java/io/trino/execution/UseTask.java

            throw new TrinoException(NOT_FOUND, "Catalog does not exist: " + catalog);
        }

        String schema = statement.getSchema().getValue().toLowerCase(ENGLISH);

        CatalogSchemaName name = new CatalogSchemaName(catalog, schema);
-        if (!metadata.schemaExists(session, name)) {
+        if (hasSchemaAccess(securityContext, catalog, schema) ||
+                !metadata.schemaExists(session, name)) {
            throw new TrinoException(NOT_FOUND, "Schema does not exist: " + name);


I think the error message has to be different and so the error code should be different between not existing case or access denied case.

IMO error message needs to be unambiguous. With different error messages there will be no difference what we have today.

That is the way Trino works today. For example see io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitUpdate if you use not existing table you will get different error than when using a table without an access.

Ok, I check if catalog/schema exists first, then I check if user has an access to those

ksobolew

Commit message needs a rationale for this change. AFAIR it's to prevent surprises when someone switches to a schema they don't have access to and then all queries are denied, right?

core/trino-main/src/main/java/io/trino/execution/UseTask.java

ksobolew · 2022-10-10T09:52:55Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+    @Test
+    public void testUseStatementAccessControl()
+    {
+        executeExclusively(() -> {


I have a feeling that executeExclusively is overused in this class. If we're using entities with random names, and make sure that the deny rules in the TestingAccessControlManager only reference these random names, then they should be able to run concurrently. That's out of scope for this change, though.

Here I also change the behavior via TestingAccessControlManager so this executeExclusively prevent to break other tests

ksobolew · 2022-10-10T09:56:22Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+            assertThatThrownBy(() -> getQueryRunner().execute("USE not_exists_catalog.tiny"))
+                    .hasMessageMatching("Catalog does not exist: not_exists_catalog");


Do we need a test where we deny access to a not existing catalog/schema and see that the message is "catalog/schema does not exist" instead of "access denied"? Is this part of the contract or just a consistency thing? @kokosing

#14209 (comment)

ksobolew · 2022-10-10T09:57:05Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+                getQueryRunner().getAccessControl().denyCatalogs(catalog -> !catalog.equals("tpch"));
+                assertThatThrownBy(() -> getQueryRunner().execute("USE tpch.tiny"))
+                        .hasMessageMatching("Access Denied: Cannot access catalog tpch");
+                assertThatThrownBy(() -> getQueryRunner().execute("USE tpch.not_exists_schema"))
+                        .hasMessageMatching("Access Denied: Cannot access catalog tpch");


There also needs to be a test that allows access to catalog but denies access to a schema

Is it possible to add such a test?

core/trino-main/src/main/java/io/trino/execution/UseTask.java

kokosing · 2022-10-10T11:27:04Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -617,4 +617,32 @@ public void testSetViewAuthorizationWithSecurityInvoker()
    {
        assertQuerySucceeds("ALTER VIEW mock.default.test_view_invoker SET AUTHORIZATION some_other_user");
    }
+
+    @Test
+    public void testUseStatementAccessControl()


nit: We can divide this test method into few smaller ones.

Spitted into 3 test methods

huberty89 · 2022-11-16T15:50:17Z

@ksobolew @kokosing Updated, please take a look

kokosing · 2022-11-17T14:33:49Z

Merged, thanks!

cla-bot bot added the cla-signed label Sep 20, 2022

huberty89 requested review from dain, electrum and kokosing September 20, 2022 07:30

kokosing reviewed Sep 20, 2022

View reviewed changes

huberty89 force-pushed the hubert/add-use-access-checks branch from 2be8124 to efef211 Compare October 7, 2022 12:23

huberty89 requested a review from ksobolew October 7, 2022 12:30

ksobolew reviewed Oct 10, 2022

View reviewed changes

kokosing approved these changes Oct 10, 2022

View reviewed changes

huberty89 force-pushed the hubert/add-use-access-checks branch from efef211 to e5550be Compare November 16, 2022 15:49

ksobolew approved these changes Nov 16, 2022

View reviewed changes

Check catalog & schema access in USE statement

18343e4

huberty89 force-pushed the hubert/add-use-access-checks branch from e5550be to 18343e4 Compare November 17, 2022 10:07

kokosing merged commit 120299e into trinodb:master Nov 17, 2022

github-actions bot added this to the 404 milestone Nov 17, 2022

colebow mentioned this pull request Nov 21, 2022

Add Trino 405 release notes #15139

Merged

huberty89 mentioned this pull request Nov 22, 2022

Release notes for 405 #15058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check catalog & schema access in USE statement #14209

Check catalog & schema access in USE statement #14209

huberty89 commented Sep 20, 2022 •

edited

Loading

kokosing left a comment

kokosing Sep 20, 2022

huberty89 Sep 20, 2022

kokosing Sep 20, 2022

huberty89 Oct 7, 2022

ksobolew left a comment

ksobolew Oct 10, 2022

huberty89 Nov 16, 2022

ksobolew Oct 10, 2022

huberty89 Oct 10, 2022

ksobolew Oct 10, 2022

ksobolew Oct 13, 2022

huberty89 Nov 16, 2022

kokosing Oct 10, 2022

huberty89 Nov 16, 2022

huberty89 commented Nov 16, 2022

kokosing commented Nov 17, 2022

		assertThatThrownBy(() -> getQueryRunner().execute("USE not_exists_catalog.tiny"))
		.hasMessageMatching("Catalog does not exist: not_exists_catalog");

Check catalog & schema access in USE statement #14209

Check catalog & schema access in USE statement #14209

Conversation

huberty89 commented Sep 20, 2022 • edited Loading

Description

Related issues, pull requests, and links

Non-technical explanation

Release notes

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksobolew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huberty89 commented Nov 16, 2022

kokosing commented Nov 17, 2022

huberty89 commented Sep 20, 2022 •

edited

Loading