Add catalog-level override for ALLOW_TABLE_LOCATION_OVERLAP #264
Conversation
eric-maynard
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau and
jackye1995
as code owners
| @@ -1144,7 +1148,7 @@ private void validateNoLocationOverlap( | |||
| URI existing = URI.create(siblingLocation); | |||
| if (isUnderParentLocation(target, existing) | |||
| || isUnderParentLocation(existing, target)) { | |||
| throw new org.apache.iceberg.exceptions.BadRequestException( | |||
| throw new org.apache.iceberg.exceptions.ForbiddenException( | |||
| "Unable to create table at location '%s' because it conflicts with existing table or namespace at location '%s'", | |||
There was a problem hiding this comment.
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.
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.
Incidentally, this highlights one of the reasons why the instantiation of the PolarisResolutionManifest in 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_CREATE being a super-privilege of TABLE_LIST -- because creating a "conflicting" table name allows one to "fish" for whether an existing table of the same name already exists, and the TABLE_EXISTS operation is conditioned on the TABLE_LIST privilege with both having the parent namespace as the authorization "target".
The analogous implicit privilege, I suppose, would be to say that both TABLE_CREATE and TABLE_WRITE_PROPERTIES effectively convey (a subset of) TABLE_READ_PROPERTIES at the parent-namespace level of the table in question, if SetLocation is being used in the update.
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
Outdated
Show resolved
Hide resolved
| Catalog.TypeEnum.INTERNAL, | ||
| c.catalog, | ||
| propertiesBuilder.build(), | ||
| System.currentTimeMillis(), |
There was a problem hiding this comment.
nit: hardcode timestamp for deterministic testing
There was a problem hiding this comment.
lots of UUIDs here too ... would be nice to lock down random to a specific seed
There was a problem hiding this comment.
Agreed on the UUIDs... to be honest, we need a better way of making sure tests & suites don't step on each others' toes. We already use UUIDs in a few places so I feel okay keeping them around for now, but something deterministic / idiomatic would be better.
If you want, I can replace this with a (seeded) random number, just let me know.
There was a problem hiding this comment.
I think we should wait till we centralize some of this test code
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
Outdated
Show resolved
Hide resolved
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
Show resolved
Hide resolved
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
Outdated
Show resolved
Hide resolved
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingTableTest.java
Outdated
Show resolved
Hide resolved
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java
Outdated
Show resolved
Hide resolved
|
Thanks @eric-maynard and @dennishuo for reviewing! |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Description
ALLOW_TABLE_LOCATION_OVERLAPcan be used to allow two tables to share a location. Combined withALLOW_UNSTRUCTURED_TABLE_LOCATION, this can be used to allow essentially arbitrary table locations. This also unlocks support ofwrite.object-storage.enabled.These configs should have a catalog-level override so certain catalogs can be created with lax location requirements while other catalogs keep stricter requirements.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added a new suite
PolarisOverlappingTableTestChecklist:
Please delete options that are not relevant.