You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "collado-mike (via GitHub)" <gi...@apache.org> on 2024/10/22 22:03:42 UTC
[PR] Add a flag to allow disabling credential vending for external catalogs [polaris]
collado-mike opened a new pull request, #395:
URL: https://github.com/apache/polaris/pull/395
# Description
Some external catalogs do not enforce any kind of directory structure for tables, allowing for table locations to overlap. Some admins may _mistakenly_ believe that vended credentials and table-level RBAC will save them from loose configurations and uncontrolled table locations. Thus, they may be encouraged to grant overly permissive privileges to the role used to generate the session token returned by the `loadTable` command without realizing that a user in the source catalog could create a table that intentionally overlaps with one or more tables in the catalog. If that user is granted read access to the table in Polaris, the user could take advantage of the generated session token to read tables they didn't have access to.
This PR adds a configuration flag to disable credential vending for _all_ EXTERNAL catalogs with a catalog-level override so that admins can support credential vending, provided they are aware of the security implications.
Right now, the default value for the flag does not change the current default behavior. We should consider changing the default so that users must explicitly allow credential vending for these cases.
Fixes # (issue)
## Type of change
Please delete options that are not relevant.
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation update
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
# How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [X] PolarisRestCatalogIntegrationTest
**Test Configuration**:
* Hardware:
* Toolchain:
* SDK:
# Checklist:
Please delete options that are not relevant.
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1811507839
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
I think admins should be allowed to control it for INTERNAL and/or EXTERNAL catalogs independently. For now, this change focuses on EXTERNAL catalogs, but I think a subsequent PR that allows for disabling for INTERNAL catalogs as well would be a good approach.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1815559914
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING")
+ .catalogConfig("enable.credential.vending")
Review Comment:
It looks like this name is not specific to external catalogs: `enable.credential.vending`
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813416392
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
The current impl allows setting the default value for the service and allowing for catalog-level overrides. This is confirmed in the tests [here](https://github.com/apache/polaris/pull/395/files#diff-38ed2dc48574ff5a3bf83463278cfa4e40ab8bb40abc4fe2aa58b46e6beb927fR793-R800) and [here](https://github.com/apache/polaris/pull/395/files#diff-38ed2dc48574ff5a3bf83463278cfa4e40ab8bb40abc4fe2aa58b46e6beb927fR860-R870). This allows admins to turn off credential vending by default, but to enable it on a case-by-case basis if they trust the external catalog or if the potential for credential overreach is not a security concern.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813274939
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
I'd also prefer a catalog property for this at this moment. We could consider policy-based one if there are solid use cases in the future.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1811516476
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
Admins could do that by just setting the flag on either INTERNAL or EXTERNAL catalogs, couldn't they? I don't see a reason to make the parameter specific to one type or the other.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813252657
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
I would agree with @eric-maynard and @sfc-gh-dhuo . Ideally I would probably like to be able to set this on a per catalog basis but having blanket coverage is nice too. In the future I would like to have a more flexible way defining policies like this.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1811503054
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
Since this is a catalog-level property, is there a need to make it specific to external catalogs? Admins could just choose to set this on only external catalogs.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "sfc-gh-dhuo (via GitHub)" <gi...@apache.org>.
sfc-gh-dhuo commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813199324
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
Looks like the current code will support different keys for yaml-level system-wide settings, while using the same catalogConfig for per-catalog overrides, which does seem to achieve both goals.
I would agree that the yaml-level config is the main thing that needs to be configurable separately by whoever is running the Polaris service.
If we can verify that it won't cause any problems to have the same `catalogConfig("enable.credential.vending")` for both `ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING` and `ALLOW_INTERNAL_CATALOG_CREDENTIAL_VENDING` then this seems like the best approach.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813501253
##########
polaris-service/src/test/java/org/apache/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java:
##########
@@ -747,6 +789,107 @@ public void testCreateTableWithOverriddenBaseLocationMustResideInNsDirectory(
.isInstanceOf(ForbiddenException.class);
}
+ /**
+ * Create an EXTERNAL catalog. The test configuration, by default, disables access delegation for
+ * EXTERNAL catalogs, so register a table and try to load it with the REST client configured to
+ * try to fetch vended credentials. Expect a ForbiddenException.
+ */
+ @CatalogConfig(Catalog.TypeEnum.EXTERNAL)
+ @RestCatalogConfig({"header.X-Iceberg-Access-Delegation", "vended-credentials"})
+ @Test
+ public void testLoadTableWithAccessDelegationForExternalCatalogWithConfigDisabled() {
+ Namespace ns1 = Namespace.of("ns1");
+ restCatalog.createNamespace(ns1);
+ TableMetadata tableMetadata =
+ TableMetadata.newTableMetadata(
+ new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))),
+ PartitionSpec.unpartitioned(),
+ "file:///tmp/ns1/my_table",
+ Map.of());
+ try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
+ resolvingFileIO.initialize(Map.of());
+ resolvingFileIO.setConf(new Configuration());
+ String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json";
+ TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation));
+ restCatalog.registerTable(TableIdentifier.of(ns1, "my_table"), fileLocation);
+ try {
+ try {
Review Comment:
Should be some flavor of assertThrows
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1811503054
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -154,6 +154,14 @@ public static <T> Builder<T> builder() {
.defaultValue(false)
.build();
+ public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
Review Comment:
Since this is a catalog-level property, is there a need to make it specific to external catalogs?
I would prefer just `ALLOW_CREDENTIAL_VENDING`. Admins could just choose to set this on only external catalogs.
This would also extend nicely if we ever introduce overrides on the namespace or table level.
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #395:
URL: https://github.com/apache/polaris/pull/395#discussion_r1813758406
##########
polaris-service/src/test/java/org/apache/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java:
##########
@@ -747,6 +789,107 @@ public void testCreateTableWithOverriddenBaseLocationMustResideInNsDirectory(
.isInstanceOf(ForbiddenException.class);
}
+ /**
+ * Create an EXTERNAL catalog. The test configuration, by default, disables access delegation for
+ * EXTERNAL catalogs, so register a table and try to load it with the REST client configured to
+ * try to fetch vended credentials. Expect a ForbiddenException.
+ */
+ @CatalogConfig(Catalog.TypeEnum.EXTERNAL)
+ @RestCatalogConfig({"header.X-Iceberg-Access-Delegation", "vended-credentials"})
+ @Test
+ public void testLoadTableWithAccessDelegationForExternalCatalogWithConfigDisabled() {
+ Namespace ns1 = Namespace.of("ns1");
+ restCatalog.createNamespace(ns1);
+ TableMetadata tableMetadata =
+ TableMetadata.newTableMetadata(
+ new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))),
+ PartitionSpec.unpartitioned(),
+ "file:///tmp/ns1/my_table",
+ Map.of());
+ try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
+ resolvingFileIO.initialize(Map.of());
+ resolvingFileIO.setConf(new Configuration());
+ String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json";
+ TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation));
+ restCatalog.registerTable(TableIdentifier.of(ns1, "my_table"), fileLocation);
+ try {
+ try {
+ restCatalog.loadTable(TableIdentifier.of(ns1, "my_table"));
+ Assertions.fail(
+ "Expected exception to be thrown when loading table from external catalog with access delegation disabled");
+ } catch (ForbiddenException e) {
Review Comment:
Are we sure this is going to be related to our lack of credentials? Just wondering if we should also be checking the message or contents
--
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] Add a flag to allow disabling credential vending for external catalogs [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike merged PR #395:
URL: https://github.com/apache/polaris/pull/395
--
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