Add a flag to allow disabling credential vending for external catalogs#395
Conversation
collado-mike
requested review from
RussellSpitzer,
ashvina,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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 and here. 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.
There was a problem hiding this 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.
...vice/src/test/java/org/apache/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java
Outdated
Show resolved
Hide resolved
...vice/src/test/java/org/apache/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java
Outdated
Show resolved
Hide resolved
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Overall I think this is good to go, I have a few nits on the test that I think we can resolve quickly.
collado-mike
force-pushed
the
mcollado-control-credentialvending-external
branch
from
ae72746 to
3bb4eb7
Compare
| public static final PolarisConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING = | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING") | ||
| .catalogConfig("enable.credential.vending") |
There was a problem hiding this comment.
It looks like this name is not specific to external catalogs: enable.credential.vending
5 participants
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
loadTablecommand 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.
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
Test Configuration:
Checklist:
Please delete options that are not relevant.