Allow all catalog access by default in file-based access control

If no catalog rules are defined, allow all catalog access
trinodb · Oct 7, 2020 · 78025d0 · 78025d0
1 parent 66e0952
commit 78025d0
Show file tree

Hide file tree

Showing 18 changed files with 70 additions and 96 deletions.
diff --git a/presto-docs/src/main/sphinx/security/file-system-access-control.rst b/presto-docs/src/main/sphinx/security/file-system-access-control.rst
@@ -127,11 +127,6 @@ ownership of any schema, you can use the following rules:
 .. code-block:: json
 
     {
-      "catalogs": [
-        {
-          "allow": true
-        }
-      ],
       "schemas": [
         {
           "user": "admin",
@@ -179,11 +174,6 @@ The example below defines the following table access policy:
 .. code-block:: json
 
     {
-      "catalogs": [
-        {
-          "allow": true
-        }
-      ],
       "tables": [
         {
           "user": "admin",
@@ -299,11 +289,6 @@ and Kerberos authentication:
 .. code-block:: json
 
     {
-      "catalogs": [
-        {
-          "allow": true
-        }
-      ],
       "principals": [
         {
           "principal": "(.*)",
@@ -325,11 +310,6 @@ name, and allow ``alice`` and ``bob`` to use a group principal named as
 .. code-block:: json
 
     {
-      "catalogs": [
-        {
-          "allow": true
-        }
-      ],
       "principals": [
         {
           "principal": "([^/]+)/?.*@example.net",

diff --git a/presto-docs/src/main/sphinx/security/query-access.json b/presto-docs/src/main/sphinx/security/query-access.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "queries": [
     {
       "user": "admin",

diff --git a/presto-docs/src/main/sphinx/security/system-information-access.json b/presto-docs/src/main/sphinx/security/system-information-access.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "system_information": [
     {
       "user": "admin",

diff --git a/presto-docs/src/main/sphinx/security/user-impersonation.json b/presto-docs/src/main/sphinx/security/user-impersonation.json
@@ -1,9 +1,4 @@
 {
-    "catalogs": [
-        {
-            "allow": true
-        }
-    ],
     "impersonation": [
         {
             "original_user": "alice",

diff --git a/presto-main/src/test/resources/catalog_impersonation.json b/presto-main/src/test/resources/catalog_impersonation.json
@@ -1,9 +1,4 @@
 {
-    "catalogs": [
-        {
-            "allow": true
-        }
-    ],
     "impersonation": [
         {
             "original_user": "alice",

diff --git a/presto-main/src/test/resources/catalog_principal.json b/presto-main/src/test/resources/catalog_principal.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "principals": [
     {
       "principal": "(.*)",

diff --git a/presto-main/src/test/resources/system_information.json b/presto-main/src/test/resources/system_information.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "system_information": [
     {
       "user": "admin",

diff --git a/...gin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java b/...gin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java
@@ -25,10 +25,17 @@
 import java.util.Set;
 import java.util.regex.Pattern;
 
+import static io.prestosql.plugin.base.security.CatalogAccessControlRule.AccessMode.ALL;
 import static java.util.Objects.requireNonNull;
 
 public class CatalogAccessControlRule
 {
+    public static final CatalogAccessControlRule ALLOW_ALL = new CatalogAccessControlRule(
+            ALL,
+            Optional.empty(),
+            Optional.empty(),
+            Optional.empty());
+
     private final AccessMode accessMode;
     private final Optional<Pattern> userRegex;
     private final Optional<Pattern> groupRegex;

diff --git a/...toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java b/...toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java
@@ -182,20 +182,26 @@ private static PrestoException invalidRefreshPeriodException(Map<String, String>
         private static SystemAccessControl create(String configFileName)
         {
             FileBasedSystemAccessControlRules rules = parseJson(Paths.get(configFileName), FileBasedSystemAccessControlRules.class);
-
-            ImmutableList.Builder<CatalogAccessControlRule> catalogRulesBuilder = ImmutableList.builder();
-            catalogRulesBuilder.addAll(rules.getCatalogRules());
-
-            // Hack to allow Presto Admin to access the "system" catalog for retrieving server status.
-            // todo Change userRegex from ".*" to one particular user that Presto Admin will be restricted to run as
-            catalogRulesBuilder.add(new CatalogAccessControlRule(
-                    ALL,
-                    Optional.of(Pattern.compile(".*")),
-                    Optional.empty(),
-                    Optional.of(Pattern.compile("system"))));
-
+            List<CatalogAccessControlRule> catalogAccessControlRules;
+            if (rules.getCatalogRules().isPresent()) {
+                ImmutableList.Builder<CatalogAccessControlRule> catalogRulesBuilder = ImmutableList.builder();
+                catalogRulesBuilder.addAll(rules.getCatalogRules().get());
+
+                // Hack to allow Presto Admin to access the "system" catalog for retrieving server status.
+                // todo Change userRegex from ".*" to one particular user that Presto Admin will be restricted to run as
+                catalogRulesBuilder.add(new CatalogAccessControlRule(
+                        ALL,
+                        Optional.of(Pattern.compile(".*")),
+                        Optional.empty(),
+                        Optional.of(Pattern.compile("system"))));
+                catalogAccessControlRules = catalogRulesBuilder.build();
+            }
+            else {
+                // if no rules are defined then all access is allowed
+                catalogAccessControlRules = ImmutableList.of(CatalogAccessControlRule.ALLOW_ALL);
+            }
             return new FileBasedSystemAccessControl(
-                    catalogRulesBuilder.build(),
+                    catalogAccessControlRules,
                     rules.getQueryAccessRules(),
                     rules.getImpersonationRules(),
                     rules.getPrincipalUserMatchRules(),

diff --git a/...it/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControlRules.java b/...it/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControlRules.java
@@ -22,7 +22,7 @@
 
 public class FileBasedSystemAccessControlRules
 {
-    private final List<CatalogAccessControlRule> catalogRules;
+    private final Optional<List<CatalogAccessControlRule>> catalogRules;
     private final Optional<List<QueryAccessRule>> queryAccessRules;
     private final Optional<List<ImpersonationRule>> impersonationRules;
     private final Optional<List<PrincipalUserMatchRule>> principalUserMatchRules;
@@ -40,7 +40,7 @@ public FileBasedSystemAccessControlRules(
             @JsonProperty("schemas") Optional<List<CatalogSchemaAccessControlRule>> schemaAccessControlRules,
             @JsonProperty("tables") Optional<List<CatalogTableAccessControlRule>> tableAccessControlRules)
     {
-        this.catalogRules = catalogRules.map(ImmutableList::copyOf).orElse(ImmutableList.of());
+        this.catalogRules = catalogRules.map(ImmutableList::copyOf);
         this.queryAccessRules = queryAccessRules.map(ImmutableList::copyOf);
         this.principalUserMatchRules = principalUserMatchRules.map(ImmutableList::copyOf);
         this.impersonationRules = impersonationRules.map(ImmutableList::copyOf);
@@ -49,7 +49,7 @@ public FileBasedSystemAccessControlRules(
         this.tableRules = tableAccessControlRules.map(ImmutableList::copyOf);
     }
 
-    public List<CatalogAccessControlRule> getCatalogRules()
+    public Optional<List<CatalogAccessControlRule>> getCatalogRules()
     {
         return catalogRules;
     }

diff --git a/...kit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java b/...kit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java
@@ -73,6 +73,7 @@ public class TestFileBasedSystemAccessControl
     private static final SystemSecurityContext CHARLIE = new SystemSecurityContext(charlie, queryId);
     private static final SystemSecurityContext ALICE = new SystemSecurityContext(alice, queryId);
     private static final SystemSecurityContext JOE = new SystemSecurityContext(joe, queryId);
+    private static final SystemSecurityContext UNKNOWN = new SystemSecurityContext(Identity.ofUser("some-unknown-user-id"), queryId);
 
     private static final String CREATE_SCHEMA_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot create schema .*";
     private static final String DROP_SCHEMA_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot drop schema .*";
@@ -94,6 +95,45 @@ public class TestFileBasedSystemAccessControl
     private static final String GRANT_DELETE_PRIVILEGE_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot grant privilege DELETE on table .*";
     private static final String REVOKE_DELETE_PRIVILEGE_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot revoke privilege DELETE on table .*";
 
+    @Test
+    public void testEmptyFile()
+    {
+        SystemAccessControl accessControl = newFileBasedSystemAccessControl("empty.json");
+
+        accessControl.checkCanCreateSchema(UNKNOWN, new CatalogSchemaName("some-catalog", "unknown"));
+        accessControl.checkCanDropSchema(UNKNOWN, new CatalogSchemaName("some-catalog", "unknown"));
+        accessControl.checkCanRenameSchema(UNKNOWN, new CatalogSchemaName("some-catalog", "unknown"), "new_unknown");
+        accessControl.checkCanSetSchemaAuthorization(UNKNOWN,
+                new CatalogSchemaName("some-catalog", "unknown"),
+                new PrestoPrincipal(PrincipalType.ROLE, "some_role"));
+        accessControl.checkCanShowCreateSchema(UNKNOWN, new CatalogSchemaName("some-catalog", "unknown"));
+
+        accessControl.checkCanSelectFromColumns(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"), ImmutableSet.of());
+        accessControl.checkCanShowColumns(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"));
+        accessControl.checkCanInsertIntoTable(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"));
+        accessControl.checkCanDeleteFromTable(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"));
+
+        accessControl.checkCanCreateTable(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"));
+        accessControl.checkCanDropTable(UNKNOWN, new CatalogSchemaTableName("some-catalog", "unknown", "unknown"));
+        accessControl.checkCanRenameTable(UNKNOWN,
+                new CatalogSchemaTableName("some-catalog", "unknown", "unknown"),
+                new CatalogSchemaTableName("some-catalog", "unknown", "new_unknown"));
+
+        accessControl.checkCanSetUser(Optional.empty(), "unknown");
+        accessControl.checkCanSetUser(Optional.of(new KerberosPrincipal("[email protected]")), "unknown");
+
+        accessControl.checkCanSetSystemSessionProperty(UNKNOWN, "anything");
+        accessControl.checkCanSetCatalogSessionProperty(UNKNOWN, "unknown", "anything");
+
+        accessControl.checkCanExecuteQuery(UNKNOWN);
+        accessControl.checkCanViewQueryOwnedBy(UNKNOWN, "anyone");
+        accessControl.checkCanKillQueryOwnedBy(UNKNOWN, "anyone");
+
+        // system information access is denied by default
+        assertThrows(AccessDeniedException.class, () -> accessControl.checkCanReadSystemInformation(UNKNOWN));
+        assertThrows(AccessDeniedException.class, () -> accessControl.checkCanWriteSystemInformation(UNKNOWN));
+    }
+
     @Test
     public void testSchemaRulesForCheckCanCreateSchema()
     {

diff --git a/presto-plugin-toolkit/src/test/resources/catalog_principal.json b/presto-plugin-toolkit/src/test/resources/catalog_principal.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "principals": [
     {
       "principal": "(.*)",

diff --git a/presto-plugin-toolkit/src/test/resources/empty.json b/presto-plugin-toolkit/src/test/resources/empty.json
@@ -0,0 +1 @@
+{}
diff --git a/presto-plugin-toolkit/src/test/resources/file-based-system-access-schema.json b/presto-plugin-toolkit/src/test/resources/file-based-system-access-schema.json
@@ -1,9 +1,4 @@
 {
-    "catalogs": [
-        {
-            "allow": true
-        }
-    ],
     "schemas": [
         {
             "catalog": "unknown",

diff --git a/presto-plugin-toolkit/src/test/resources/file-based-system-access-table.json b/presto-plugin-toolkit/src/test/resources/file-based-system-access-table.json
@@ -1,9 +1,4 @@
 {
-    "catalogs": [
-        {
-            "allow": true
-        }
-    ],
   "tables": [
     {
       "catalog": "unknown",

diff --git a/presto-plugin-toolkit/src/test/resources/file-based-system-no-access.json b/presto-plugin-toolkit/src/test/resources/file-based-system-no-access.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "schemas": [
   ],
   "tables": [

diff --git a/presto-plugin-toolkit/src/test/resources/query.json b/presto-plugin-toolkit/src/test/resources/query.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "queries": [
     {
       "user": "admin",

diff --git a/presto-plugin-toolkit/src/test/resources/system-information.json b/presto-plugin-toolkit/src/test/resources/system-information.json
@@ -1,9 +1,4 @@
 {
-  "catalogs": [
-    {
-      "allow": true
-    }
-  ],
   "system_information": [
     {
       "user": "admin",