You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2026/04/27 18:24:19 UTC
[PR] Service: stage create location validation [polaris]
flyrain opened a new pull request, #4300:
URL: https://github.com/apache/polaris/pull/4300
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] README Update [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149455953
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -594,11 +600,18 @@ public LoadTableResponse createTableStaged(
TableIdentifier ident = TableIdentifier.of(namespace, request.name());
TableMetadata metadata = stageTableCreateHelper(namespace, request);
+ if (!isFederated) {
+ ((IcebergCatalog) baseCatalog).validateTableLocationsForCreate(ident, metadata);
+ }
Review Comment:
Should we gate this with `ALLOW_TABLE_LOCATION_OVERLAP`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149413447
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1054,6 +1054,41 @@ private void validateLocationForTableLike(
realmConfig, identifier, Set.of(location), resolvedStorageEntity);
}
+ /**
+ * Runs the full set of location-safety checks that must pass before a new table's locations are
+ * treated as valid: allowed-locations prefix check, overlap with existing sibling entities, and
+ * (if a metadata file location is set) metadata-file containment. Callers that create tables
+ * (including staged creation which vends delegated credentials before {@code doCommit} runs) must
+ * invoke this to avoid issuing credentials for an unvalidated, caller-influenced location set.
+ */
+ public void validateTableLocationsForCreate(
+ TableIdentifier tableIdentifier, TableMetadata metadata) {
+ PolarisResolvedPathWrapper resolvedStorageEntity =
+ CatalogUtils.findResolvedStorageEntity(resolvedEntityView, tableIdentifier);
+ if (resolvedStorageEntity == null) {
+ return;
Review Comment:
Allowing the operation to proceed here doesn't feel safe.
```suggestion
throw noSuchNamespaceException(tableIdentifier.namespace());
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -519,8 +519,14 @@ private TableMetadata stageTableCreateHelper(Namespace namespace, CreateTableReq
properties.put("created-at", OffsetDateTime.now(ZoneOffset.UTC).toString());
properties.putAll(reservedProperties().removeReservedProperties(request.properties()));
+ boolean allowCustomLocation =
+ realmConfig()
+ .getConfig(
+ FeatureConfiguration.ALLOW_STAGE_CREATE_CUSTOM_LOCATION,
+ getResolvedCatalogEntity());
+
String location;
- if (request.location() != null) {
+ if (allowCustomLocation && request.location() != null) {
Review Comment:
If `allowCustomLocation` is false and `request.location()` is non-null, shouldn't we fail the operation rather than silently overwriting the custom location?
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
Shouldn't this be false by default?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergAllowedLocationTest.java:
##########
@@ -406,4 +406,114 @@ private void createNamespace(TestServices services, String location) {
assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode());
}
}
+
+ // Regression tests for staged table creation credential-vending ordering. Staged creation must
+ // validate the effective location set (base location plus write.data.path / write.metadata.path
+ // overrides) before issuing delegated credentials; without this check, a caller could mint broad
+ // temporary credentials for an attacker-chosen reachable location.
Review Comment:
I wouldn't leave this kind of comment in the code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] README Update [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149413447
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1054,6 +1054,41 @@ private void validateLocationForTableLike(
realmConfig, identifier, Set.of(location), resolvedStorageEntity);
}
+ /**
+ * Runs the full set of location-safety checks that must pass before a new table's locations are
+ * treated as valid: allowed-locations prefix check, overlap with existing sibling entities, and
+ * (if a metadata file location is set) metadata-file containment. Callers that create tables
+ * (including staged creation which vends delegated credentials before {@code doCommit} runs) must
+ * invoke this to avoid issuing credentials for an unvalidated, caller-influenced location set.
+ */
+ public void validateTableLocationsForCreate(
+ TableIdentifier tableIdentifier, TableMetadata metadata) {
+ PolarisResolvedPathWrapper resolvedStorageEntity =
+ CatalogUtils.findResolvedStorageEntity(resolvedEntityView, tableIdentifier);
+ if (resolvedStorageEntity == null) {
+ return;
Review Comment:
Allowing the operation to proceed here doesn't feel safe.
```suggestion
throw noSuchNamespaceException(tableIdentifier.namespace());
```
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
Shouldn't this be false by default?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -519,8 +519,14 @@ private TableMetadata stageTableCreateHelper(Namespace namespace, CreateTableReq
properties.put("created-at", OffsetDateTime.now(ZoneOffset.UTC).toString());
properties.putAll(reservedProperties().removeReservedProperties(request.properties()));
+ boolean allowCustomLocation =
+ realmConfig()
+ .getConfig(
+ FeatureConfiguration.ALLOW_STAGE_CREATE_CUSTOM_LOCATION,
+ getResolvedCatalogEntity());
+
String location;
- if (request.location() != null) {
+ if (allowCustomLocation && request.location() != null) {
Review Comment:
If `allowCustomLocation` is false and `request.location()` is non-null, shouldn't we fail the operation rather than silently overwriting the custom location?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] README Update [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149525978
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
Imho this is not safe enough, I think this should be opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain closed pull request #4300: Service: stage create location validation
URL: https://github.com/apache/polaris/pull/4300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] README Update [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149486505
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
this is mainly to avoid behavior changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149441097
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergAllowedLocationTest.java:
##########
@@ -406,4 +406,114 @@ private void createNamespace(TestServices services, String location) {
assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode());
}
}
+
+ // Regression tests for staged table creation credential-vending ordering. Staged creation must
+ // validate the effective location set (base location plus write.data.path / write.metadata.path
+ // overrides) before issuing delegated credentials; without this check, a caller could mint broad
+ // temporary credentials for an attacker-chosen reachable location.
Review Comment:
I wouldn't leave this kind of comment in the code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149455953
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -594,11 +600,18 @@ public LoadTableResponse createTableStaged(
TableIdentifier ident = TableIdentifier.of(namespace, request.name());
TableMetadata metadata = stageTableCreateHelper(namespace, request);
+ if (!isFederated) {
+ ((IcebergCatalog) baseCatalog).validateTableLocationsForCreate(ident, metadata);
+ }
Review Comment:
Should we gate this with `ALLOW_TABLE_LOCATION_OVERLAP`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149525978
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
Imho this is not safe enough, I think this should be opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b closed pull request #4300: Service: stage create location validation
URL: https://github.com/apache/polaris/pull/4300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] README Update [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b closed pull request #4300: README Update
URL: https://github.com/apache/polaris/pull/4300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Service: stage create location validation [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4300:
URL: https://github.com/apache/polaris/pull/4300#discussion_r3149486505
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
this is mainly to prevent behavior changes
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -203,6 +203,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean> ALLOW_STAGE_CREATE_CUSTOM_LOCATION =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_STAGE_CREATE_CUSTOM_LOCATION")
+ .catalogConfig("polaris.config.allow.stage.create.custom.location")
+ .description(
+ "If true (default), honors a caller-supplied location in staged table creation"
+ + " requests. If false, the location in the request is ignored and the catalog"
+ + " default is used. Disable this to prevent callers from steering delegated"
+ + " storage credentials during staged create.")
+ .defaultValue(true)
Review Comment:
this is mainly to avoid behavior changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org