> They do not have such a knob for EXTERNAL catalogs This by itself seems like a problem to me.
It seems like <https://github.com/apache/polaris/blob/02468e0bed230641893c2bd45cde790dfd2ca76c/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java#L540> we do check ALLOW_OVERLAPPING_CATALOG_URLS for both internal and external catalogs. The check for ALLOW_UNSTRUCTURED_TABLE_LOCATION is skipped here <https://github.com/apache/polaris/blob/02468e0bed230641893c2bd45cde790dfd2ca76c/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java#L173> for external catalogs. Why is that? If we respected ALLOW_UNSTRUCTURED_TABLE_LOCATION for external catalogs, it seems like admins would have that knob you mentioned. Since #264 <https://github.com/apache/polaris/pull/264> we allow overrides of this on the catalog level, so admins that wanted to allow unsecure overlapping table locations in external catalogs they could do so. On Tue, Oct 22, 2024 at 4:38 PM Michael Collado <collado.m...@gmail.com> wrote: > I responded to your comment on the PR already. I think a flag to disable > credential vending for INTERNAL catalogs does make sense, but I think it > should be a separate flag. > > Service admins already have the knobs to make an INTERNAL catalog secure by > default by enforcing non-overlapping table locations. They have to go out > of their way to allow table overlap. > > They do not have such a knob for EXTERNAL catalogs. A single flag for > disabling all credential vending means that an admin cannot securely > configure INTERNAL catalogs and support credential vending without also > allowing for potentially insecure EXTERNAL catalogs. The controls for each > type of catalog are different, so they should be configured differently. > > Again, I think such a flag is a good idea, but should be a separate flag > and belongs in another PR. > > Mike > > On Tue, Oct 22, 2024 at 4:25 PM Eric Maynard <eric.w.mayn...@gmail.com> > wrote: > > > I commented the same on the PR, but it’s not obvious to me why we would > > make this exclusive to external catalogs. You can create internal > catalogs > > with overlapping locations too. > > > > A single flag to disable credential vending altogether solves this > problem > > across the board and also allows for finer-grained control if these > > overrides are ever respected for namespaces or tables. > > > > Other flags like the ones to allow location overlap or to disable > > subscoping are not specific to a catalog type. > > > > On Tue, Oct 22, 2024 at 3:15 PM Michael Collado <collado.m...@gmail.com> > > wrote: > > > > > Hey folks > > > > > > I opened a PR at https://github.com/apache/polaris/pull/395 to support > > > disabling credential vending for external catalogs. > > > > > > Currently, some remote catalogs don't control the table location for > > > Iceberg tables, allowing people to create tables in overlapping > > directories > > > in storage. A person that has storage credentials to one table > > necessarily > > > has access to all of the tables that overlap. This is already an issue > > for > > > Iceberg users. > > > > > > However, with Polaris, some users expect that table-level RBAC and > > > credential vending will somehow magically resolve their security issues > > so > > > that they can tightly control access despite overlapping table > locations. > > > This is obviously a misconception. > > > > > > But in the spirit of making things secure by default, I am adding a > > > configuration flag to disable credential vending for external catalogs. > > As > > > with the current configuration to disable overlapping table locations, > > > there is a catalog-level override so that catalog admins can enable > > > credential vending on a case-by-case basis if they feel the risk is > > > tolerable. > > > > > > Changing the configuration to disable credential vending by default is > a > > > breaking change, so I'd like to see what everyone thinks about making > > that > > > change. Currently, the PR does _not_ change the default value, so we > can > > > merge it as is, without any impact on existing users. But unless users > > > really dig into the code or the documentation, they may not understand > > the > > > implications well enough to make the more secure configuration setting. > > > > > > WDYT? > > > > > > Mike > > > > > >
