-
Notifications
You must be signed in to change notification settings - Fork 110
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.
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
Add catalog-level override for ALLOW_TABLE_LOCATION_OVERLAP #264
Merged
RussellSpitzer
merged 10 commits into
apache:main
from
eric-maynard:catalog-level-overlap-allowed
Sep 10, 2024
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
68514df
check in odd behavior
eric-maynard 79e1c0d
check in
eric-maynard 41829f3
clean up
eric-maynard 19f66cc
some changes per review
eric-maynard 19af248
changes per review
eric-maynard 585174c
use catalogEntity correctly
eric-maynard b233a50
lint
eric-maynard ab8ff33
Merge branch 'main' of github.com:apache/polaris into catalog-level-o…
eric-maynard 6d01aab
fix a test
eric-maynard d3f1ca9
autolint
eric-maynard File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
294 changes: 294 additions & 0 deletions
294
...s-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,294 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.polaris.service.admin; | ||
|
|
||
| import static org.apache.polaris.service.admin.PolarisAuthzTestBase.SCHEMA; | ||
| import static org.apache.polaris.service.context.DefaultContextResolver.REALM_PROPERTY_KEY; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import io.dropwizard.testing.ConfigOverride; | ||
| import io.dropwizard.testing.ResourceHelpers; | ||
| import io.dropwizard.testing.junit5.DropwizardAppExtension; | ||
| import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; | ||
| import jakarta.ws.rs.client.Entity; | ||
| import jakarta.ws.rs.client.Invocation; | ||
| import jakarta.ws.rs.core.Response; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
| import java.util.stream.Stream; | ||
| import org.apache.iceberg.catalog.Namespace; | ||
| import org.apache.iceberg.rest.requests.CreateNamespaceRequest; | ||
| import org.apache.iceberg.rest.requests.CreateTableRequest; | ||
| import org.apache.polaris.core.PolarisConfiguration; | ||
| import org.apache.polaris.core.admin.model.Catalog; | ||
| import org.apache.polaris.core.admin.model.CatalogProperties; | ||
| import org.apache.polaris.core.admin.model.CreateCatalogRequest; | ||
| import org.apache.polaris.core.admin.model.FileStorageConfigInfo; | ||
| import org.apache.polaris.core.admin.model.StorageConfigInfo; | ||
| import org.apache.polaris.service.PolarisApplication; | ||
| import org.apache.polaris.service.config.PolarisApplicationConfig; | ||
| import org.apache.polaris.service.test.PolarisConnectionExtension; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| @ExtendWith({DropwizardExtensionsSupport.class, PolarisConnectionExtension.class}) | ||
| public class PolarisOverlappingTableTest { | ||
| private static final DropwizardAppExtension<PolarisApplicationConfig> BASE_EXT = | ||
| new DropwizardAppExtension<>( | ||
| PolarisApplication.class, | ||
| ResourceHelpers.resourceFilePath("polaris-server-integrationtest.yml"), | ||
| // Bind to random port to support parallelism | ||
| ConfigOverride.config("server.applicationConnectors[0].port", "0"), | ||
| ConfigOverride.config("server.adminConnectors[0].port", "0"), | ||
| // Enforce table location constraints | ||
| ConfigOverride.config("featureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION", "false"), | ||
| ConfigOverride.config("featureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP", "false")); | ||
|
|
||
| private static final DropwizardAppExtension<PolarisApplicationConfig> LAX_EXT = | ||
| new DropwizardAppExtension<>( | ||
| PolarisApplication.class, | ||
| ResourceHelpers.resourceFilePath("polaris-server-integrationtest.yml"), | ||
| // Bind to random port to support parallelism | ||
| ConfigOverride.config("server.applicationConnectors[0].port", "0"), | ||
| ConfigOverride.config("server.adminConnectors[0].port", "0"), | ||
| // Relax table location constraints | ||
| ConfigOverride.config("featureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION", "true"), | ||
| ConfigOverride.config("featureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP", "true")); | ||
|
|
||
| private static PolarisConnectionExtension.PolarisToken adminToken; | ||
| private static String userToken; | ||
| private static String realm; | ||
| private static String namespace; | ||
| private static final String baseLocation = "file:///tmp/PolarisOverlappingTableTest"; | ||
|
|
||
| private static final CatalogWrapper defaultCatalog = new CatalogWrapper("default"); | ||
| private static final CatalogWrapper laxCatalog = new CatalogWrapper("lax"); | ||
| private static final CatalogWrapper strictCatalog = new CatalogWrapper("strict"); | ||
|
|
||
| /** Used to define a parameterized test config */ | ||
| protected record TestConfig( | ||
| DropwizardAppExtension<PolarisApplicationConfig> extension, | ||
| CatalogWrapper catalogWrapper, | ||
| Response.Status response) { | ||
| public String catalog() { | ||
| return catalogWrapper.catalog; | ||
| } | ||
|
|
||
| private String extensionName() { | ||
| return (extension | ||
| .getConfiguration() | ||
| .getConfigurationStore() | ||
| .getConfiguration(null, PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP)) | ||
| ? "lax" | ||
| : "strict"; | ||
| } | ||
|
|
||
| /** Extract the first component of the catalog name; e.g. `default` from `default_123_xyz` */ | ||
| private String catalogShortName() { | ||
| int firstComponentEnd = catalog().indexOf('_'); | ||
| if (firstComponentEnd != -1) { | ||
| return catalog().substring(0, firstComponentEnd); | ||
| } else { | ||
| return catalog(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return String.format( | ||
| "extension=%s, catalog=%s, status=%s", | ||
| extensionName(), catalogShortName(), response.toString()); | ||
| } | ||
| } | ||
|
|
||
| /* Used to wrap a catalog name, so the TestConfig's final `catalog` field can be updated */ | ||
| protected static class CatalogWrapper { | ||
| public String catalog; | ||
|
|
||
| public CatalogWrapper(String catalog) { | ||
| this.catalog = catalog; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return catalog; | ||
| } | ||
| } | ||
|
|
||
| @BeforeEach | ||
| public void setup(PolarisConnectionExtension.PolarisToken adminToken) { | ||
| userToken = adminToken.token(); | ||
| realm = PolarisConnectionExtension.getTestRealm(PolarisServiceImplIntegrationTest.class); | ||
| defaultCatalog.catalog = String.format("default_catalog_%s", UUID.randomUUID().toString()); | ||
| laxCatalog.catalog = String.format("lax_catalog_%s", UUID.randomUUID().toString()); | ||
| strictCatalog.catalog = String.format("strict_catalog_%s", UUID.randomUUID().toString()); | ||
| for (var EXT : List.of(BASE_EXT, LAX_EXT)) { | ||
| for (var c : List.of(defaultCatalog, laxCatalog, strictCatalog)) { | ||
| CatalogProperties.Builder propertiesBuilder = | ||
| CatalogProperties.builder() | ||
| .setDefaultBaseLocation(String.format("%s/%s", baseLocation, c)); | ||
| if (!c.equals(defaultCatalog)) { | ||
| propertiesBuilder | ||
| .addProperty( | ||
| PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), | ||
| String.valueOf(c.equals(laxCatalog))) | ||
| .addProperty( | ||
| PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP.catalogConfig(), | ||
| String.valueOf(c.equals(laxCatalog))); | ||
| } | ||
| StorageConfigInfo config = | ||
| FileStorageConfigInfo.builder() | ||
| .setStorageType(StorageConfigInfo.StorageTypeEnum.FILE) | ||
| .build(); | ||
| Catalog catalogObject = | ||
| new Catalog( | ||
| Catalog.TypeEnum.INTERNAL, | ||
| c.catalog, | ||
| propertiesBuilder.build(), | ||
| 1725487592064L, | ||
| 1725487592064L, | ||
| 1, | ||
| config); | ||
| try (Response response = | ||
| request(EXT, "management/v1/catalogs") | ||
| .post(Entity.json(new CreateCatalogRequest(catalogObject)))) { | ||
| if (response.getStatus() != Response.Status.CREATED.getStatusCode()) { | ||
| throw new IllegalStateException( | ||
| "Failed to create catalog: " + response.readEntity(String.class)); | ||
| } | ||
| } | ||
|
|
||
| namespace = "ns"; | ||
| CreateNamespaceRequest createNamespaceRequest = | ||
| CreateNamespaceRequest.builder().withNamespace(Namespace.of(namespace)).build(); | ||
| try (Response response = | ||
| request(EXT, String.format("catalog/v1/%s/namespaces", c)) | ||
| .post(Entity.json(createNamespaceRequest))) { | ||
| if (response.getStatus() != Response.Status.OK.getStatusCode()) { | ||
| throw new IllegalStateException( | ||
| "Failed to create namespace: " + response.readEntity(String.class)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private Response createTable( | ||
| DropwizardAppExtension<PolarisApplicationConfig> extension, String catalog, String location) { | ||
| CreateTableRequest createTableRequest = | ||
| CreateTableRequest.builder() | ||
| .withName("table_" + UUID.randomUUID().toString()) | ||
| .withLocation(location) | ||
| .withSchema(SCHEMA) | ||
| .build(); | ||
| String prefix = String.format("catalog/v1/%s/namespaces/%s/tables", catalog, namespace); | ||
| try (Response response = request(extension, prefix).post(Entity.json(createTableRequest))) { | ||
| return response; | ||
| } | ||
| } | ||
|
|
||
| private static Invocation.Builder request( | ||
| DropwizardAppExtension<PolarisApplicationConfig> extension, String prefix) { | ||
| return extension | ||
| .client() | ||
| .target(String.format("http://localhost:%d/api/%s", extension.getLocalPort(), prefix)) | ||
| .request("application/json") | ||
| .header("Authorization", "Bearer " + userToken) | ||
| .header(REALM_PROPERTY_KEY, realm); | ||
| } | ||
|
|
||
| private static Stream<TestConfig> getTestConfigs() { | ||
| return Stream.of( | ||
RussellSpitzer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| new TestConfig(BASE_EXT, defaultCatalog, Response.Status.FORBIDDEN), | ||
| new TestConfig(BASE_EXT, strictCatalog, Response.Status.FORBIDDEN), | ||
| new TestConfig(BASE_EXT, laxCatalog, Response.Status.OK), | ||
| new TestConfig(LAX_EXT, defaultCatalog, Response.Status.OK), | ||
| new TestConfig(LAX_EXT, strictCatalog, Response.Status.FORBIDDEN), | ||
| new TestConfig(LAX_EXT, laxCatalog, Response.Status.OK)); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("getTestConfigs") | ||
| @DisplayName("Test restrictions on table locations") | ||
| void testTableLocationRestrictions(TestConfig config) { | ||
| // Original table | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s/%s/table_1", baseLocation, config.catalog(), namespace))) | ||
| .returns(Response.Status.OK.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Unrelated path | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s/%s/table_2", baseLocation, config.catalog(), namespace))) | ||
| .returns(Response.Status.OK.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Trailing slash makes this not overlap with table_1 | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s/%s/table_100", baseLocation, config.catalog(), namespace))) | ||
| .returns(Response.Status.OK.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Repeat location | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s/%s/table_100", baseLocation, config.catalog(), namespace))) | ||
| .returns(config.response.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Parent of existing location | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s/%s", baseLocation, config.catalog(), namespace))) | ||
| .returns(config.response.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Child of existing location | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format( | ||
| "%s/%s/%s/table_100/child", baseLocation, config.catalog(), namespace))) | ||
| .returns(config.response.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Outside the namespace | ||
| assertThat( | ||
| createTable( | ||
| config.extension, | ||
| config.catalog(), | ||
| String.format("%s/%s", baseLocation, config.catalog()))) | ||
| .returns(config.response.getStatusCode(), Response::getStatus); | ||
|
|
||
| // Outside the catalog | ||
| assertThat(createTable(config.extension, config.catalog(), String.format("%s", baseLocation))) | ||
| .returns(Response.Status.FORBIDDEN.getStatusCode(), Response::getStatus); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't in scope here, and we probably talked about it already but doesn't this check essentially let any user with table create permissions find out the table location of every other table in the namespace regardless of permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. I agree this is potentially an issue; maybe we should open up a discussion on it?
My $0.02 is that this is essentially the lesser of two evils; not doing this check would allow tables to share a location, undermining the role of credential vending and the value of scoping credentials to a prefix.
Perhaps in the future we can skip this check if credentials can somehow be scoped to just the files in a table e.g. by tagging or with file-level encryption. For now, we scope them to a prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incidentally, this highlights one of the reasons why the instantiation of the
PolarisResolutionManifestin this method is the odd one out and may feel out of place; normally the PolarisResolutionManifest is only instantiated in the auth-enforcement-aware layer (PolarisCatalogHandlerWrapper) to tie together an authz check with an explicit enumeration of the entities the request should be able to touch. So any time we instantiate one to perform lookups outside of that, we're intentionally forced to consider its consequences on the authz model.I agree in this scenario the information-disclosure vector is probably the lesser of the concerns, and in both cases the catalog admin needs to make a judgement call on trust level of anyone allowed to create or update tables.
Perhaps segregating out privileges for different table-update types could help as well -- some workflows may want to restrict some principals/roles to only being able to update snapshots of a table for example without being allowed to perform "DDL" on the table to change schema or file location.
Also, it's worth noting that the analogous "information leak" from createTable throwing "Table already exists" is actually addressed in the privilege model by virtue of
TABLE_CREATEbeing a super-privilege ofTABLE_LIST-- because creating a "conflicting" table name allows one to "fish" for whether an existing table of the same name already exists, and theTABLE_EXISTSoperation is conditioned on theTABLE_LISTprivilege with both having the parent namespace as the authorization "target".The analogous implicit privilege, I suppose, would be to say that both
TABLE_CREATEandTABLE_WRITE_PROPERTIESeffectively convey (a subset of)TABLE_READ_PROPERTIESat the parent-namespace level of the table in question, ifSetLocationis being used in the update.