SNOW-1549292 Add limits and restriction to Principal, Role and Catalo…

…g names (apache#23) SNOW-1549292 Adding annotation and enforcing size limits for Principal, Role, Catalog and Catalog Role names. Also blocking "SYSTEM$" prefix from being used in names. Adding case-insensitive regex rule to block "SYSTEM$" ## Pre-review checklist - [ ] I attest that this change meets the bar for low risk without security requirements as defined in the [Accelerated Risk Assessment Criteria](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/accelerated-risk-assessment/#eligibility) and I have taken the [Risk Assessment Training in Workday](https://wd5.myworkday.com/snowflake/learning/course/6c613806284a1001f111fedf3e4e0000). - Checking this checkbox is mandatory if using the [Accelerated Risk Assessment](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/accelerated-risk-assessment/) to risk assess the changes in this Pull Request. - If this change does not meet the bar for low risk without security requirements (as confirmed by the peer reviewers of this pull request) then a [formal Risk Assessment](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/risk-assessment/) must be completed. Please note that a formal Risk Assessment will require you to spend extra time performing a security review for this change. Please account for this extra time earlier rather than later to avoid unnecessary delays in the release process. - [x] This change has code coverage for the new code added
eric-maynard · Jul 25, 2024 · 3a0771a · 3a0771a
1 parent b6d9607
commit 3a0771a
Show file tree

Hide file tree

Showing 2 changed files with 345 additions and 2 deletions.
diff --git a/...ris-service/src/test/java/io/polaris/service/admin/PolarisServiceImplIntegrationTest.java b/...ris-service/src/test/java/io/polaris/service/admin/PolarisServiceImplIntegrationTest.java
@@ -1,5 +1,6 @@
 package io.polaris.service.admin;
 
+import static io.dropwizard.jackson.Jackson.newObjectMapper;
 import static io.polaris.service.context.DefaultContextResolver.REALM_PROPERTY_KEY;
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -46,8 +47,10 @@
 import jakarta.ws.rs.client.Invocation;
 import jakarta.ws.rs.core.Response;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.iceberg.rest.responses.ErrorResponse;
 import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.jupiter.api.AfterEach;
@@ -57,6 +60,8 @@
 
 @ExtendWith({DropwizardExtensionsSupport.class, PolarisConnectionExtension.class})
 public class PolarisServiceImplIntegrationTest {
+  private static final int MAX_IDENTIFIER_LENGTH = 256;
+
   // TODO: Add a test-only hook that fully clobbers all persistence state so we can have a fresh
   // slate on every test case; otherwise, leftover state from one test from failures will interfere
   // with other test cases.
@@ -222,6 +227,69 @@ public void testCreateCatalog() {
     }
   }
 
+  @Test
+  public void testCreateCatalogWithInvalidName() {
+    AwsStorageConfigInfo awsConfigModel =
+        AwsStorageConfigInfo.builder()
+            .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+            .setExternalId("externalId")
+            .setUserArn("userArn")
+            .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+            .setAllowedLocations(List.of("s3://my-old-bucket/path/to/data"))
+            .build();
+
+    String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
+
+    ObjectMapper mapper = newObjectMapper();
+
+    Catalog catalog =
+        PolarisCatalog.builder()
+            .setType(Catalog.TypeEnum.INTERNAL)
+            .setName(goodName)
+            .setProperties(new CatalogProperties("s3://my-bucket/path/to/data"))
+            .setStorageConfigInfo(awsConfigModel)
+            .build();
+    try (Response response =
+        newRequest("http://localhost:%d/api/management/v1/catalogs")
+            .post(Entity.json(mapper.writeValueAsString(catalog)))) {
+      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+    } catch (JsonProcessingException e) {
+      throw new RuntimeException(e);
+    }
+
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidCatalogNames =
+        Arrays.asList(
+            longInvalidName,
+            "",
+            "system$catalog1",
+            "SYSTEM$TestCatalog",
+            "System$test_catalog",
+            "  SysTeM$ test catalog");
+
+    for (String invalidCatalogName : invalidCatalogNames) {
+      catalog =
+          PolarisCatalog.builder()
+              .setType(Catalog.TypeEnum.INTERNAL)
+              .setName(invalidCatalogName)
+              .setProperties(new CatalogProperties("s3://my-bucket/path/to/data"))
+              .setStorageConfigInfo(awsConfigModel)
+              .build();
+
+      try (Response response =
+          newRequest("http://localhost:%d/api/management/v1/catalogs")
+              .post(Entity.json(mapper.writeValueAsString(catalog)))) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      } catch (JsonProcessingException e) {
+        throw new RuntimeException(e);
+      }
+    }
+  }
+
   @Test
   public void testCreateCatalogWithNullBaseLocation() {
     AwsStorageConfigInfo awsConfigModel =
@@ -719,6 +787,74 @@ public void testGetCatalogNotFound() {
     }
   }
 
+  @Test
+  public void testGetCatalogInvalidName() {
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidCatalogNames =
+        Arrays.asList(
+            longInvalidName,
+            "system$catalog1",
+            "SYSTEM$TestCatalog",
+            "System$test_catalog",
+            "  SysTeM$ test catalog");
+
+    for (String invalidCatalogName : invalidCatalogNames) {
+      // there's no catalog yet. Expect 404
+      try (Response response =
+          newRequest("http://localhost:%d/api/management/v1/catalogs/" + invalidCatalogName)
+              .get()) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
+  @Test
+  public void testCatalogRoleInvalidName() {
+    Catalog catalog =
+        PolarisCatalog.builder()
+            .setType(Catalog.TypeEnum.INTERNAL)
+            .setName("mycatalog1")
+            .setProperties(new CatalogProperties("s3://required/base/location"))
+            .setStorageConfigInfo(
+                new AwsStorageConfigInfo(
+                    "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+            .build();
+    try (Response response =
+        newRequest("http://localhost:%d/api/management/v1/catalogs")
+            .post(Entity.json(new CreateCatalogRequest(catalog)))) {
+
+      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+    }
+
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidCatalogRoleNames =
+        Arrays.asList(
+            longInvalidName,
+            "system$catalog1",
+            "SYSTEM$TestCatalog",
+            "System$test_catalog",
+            "  SysTeM$ test catalog");
+
+    for (String invalidCatalogRoleName : invalidCatalogRoleNames) {
+      try (Response response =
+          newRequest(
+                  "http://localhost:%d/api/management/v1/catalogs/mycatalog1/catalog-roles/"
+                      + invalidCatalogRoleName)
+              .get()) {
+
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
   @Test
   public void testListPrincipalsUnauthorized() {
     Principal principal = new Principal("new_admin");
@@ -904,6 +1040,73 @@ public void testCreateListUpdateAndDeletePrincipal() {
     }
   }
 
+  @Test
+  public void testCreatePrincipalWithInvalidName() {
+    String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
+    Principal principal =
+        Principal.builder()
+            .setName(goodName)
+            .setProperties(Map.of("custom-tag", "good_principal"))
+            .build();
+    try (Response response =
+        newRequest("http://localhost:%d/api/management/v1/principals")
+            .post(Entity.json(new CreatePrincipalRequest(principal, null)))) {
+      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+    }
+
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidPrincipalNames =
+        Arrays.asList(
+            longInvalidName,
+            "",
+            "system$principal1",
+            "SYSTEM$TestPrincipal",
+            "System$test_principal",
+            "  SysTeM$ principal");
+
+    for (String invalidPrincipalName : invalidPrincipalNames) {
+      principal =
+          Principal.builder()
+              .setName(invalidPrincipalName)
+              .setProperties(Map.of("custom-tag", "bad_principal"))
+              .build();
+
+      try (Response response =
+          newRequest("http://localhost:%d/api/management/v1/principals")
+              .post(Entity.json(new CreatePrincipalRequest(principal, false)))) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
+  @Test
+  public void testGetPrincipalWithInvalidName() {
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidPrincipalNames =
+        Arrays.asList(
+            longInvalidName,
+            "system$principal1",
+            "SYSTEM$TestPrincipal",
+            "System$test_principal",
+            "  SysTeM$ principal");
+
+    for (String invalidPrincipalName : invalidPrincipalNames) {
+      try (Response response =
+          newRequest("http://localhost:%d/api/management/v1/principals/" + invalidPrincipalName)
+              .get()) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
   @Test
   public void testCreateListUpdateAndDeletePrincipalRole() {
     PrincipalRole principalRole =
@@ -998,6 +1201,70 @@ public void testCreateListUpdateAndDeletePrincipalRole() {
     }
   }
 
+  @Test
+  public void testCreatePrincipalRoleInvalidName() {
+    String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
+    PrincipalRole principalRole =
+        new PrincipalRole(goodName, Map.of("custom-tag", "good_principal_role"), 0L, 0L, 1);
+    try (Response response =
+        newRequest("http://localhost:%d/api/management/v1/principal-roles")
+            .post(Entity.json(new CreatePrincipalRoleRequest(principalRole)))) {
+      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+    }
+
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidPrincipalRoleNames =
+        Arrays.asList(
+            longInvalidName,
+            "",
+            "system$principalrole1",
+            "SYSTEM$TestPrincipalRole",
+            "System$test_principal_role",
+            "  SysTeM$ principal role");
+
+    for (String invalidPrincipalRoleName : invalidPrincipalRoleNames) {
+      principalRole =
+          new PrincipalRole(
+              invalidPrincipalRoleName, Map.of("custom-tag", "bad_principal_role"), 0L, 0L, 1);
+
+      try (Response response =
+          newRequest("http://localhost:%d/api/management/v1/principal-roles")
+              .post(Entity.json(new CreatePrincipalRoleRequest(principalRole)))) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
+  @Test
+  public void testGetPrincipalRoleInvalidName() {
+    String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
+    List<String> invalidPrincipalRoleNames =
+        Arrays.asList(
+            longInvalidName,
+            "system$principalrole1",
+            "SYSTEM$TestPrincipalRole",
+            "System$test_principal_role",
+            "  SysTeM$ principal role");
+
+    for (String invalidPrincipalRoleName : invalidPrincipalRoleNames) {
+      try (Response response =
+          newRequest(
+                  "http://localhost:%d/api/management/v1/principal-roles/"
+                      + invalidPrincipalRoleName)
+              .get()) {
+        assertThat(response)
+            .returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
+        assertThat(response.hasEntity()).isTrue();
+        ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
+        assertThat(errorResponse.message()).contains("Invalid value:");
+      }
+    }
+  }
+
   @Test
   public void testCreateListUpdateAndDeleteCatalogRole() {
     Catalog catalog =