From 652cc50adf9a8542d3f5c9bcb72157f80ee3ca01 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 30 Sep 2025 22:07:10 -0400 Subject: [PATCH 1/4] Enforce that S3 credentials are vended when requested This is a follow-up change to #2672 striving to improve user-facing error reporting for S3 storage systems without STS. * Add property to `AccessConfig` to indicate whether the backing storage integration can produce credentials. * Add a check to `IcebergCatalogHandler` (leading to 400) that storage credentials are vended when requested and the backend is capable of vending credentials in principle. * Update `PolarisStorageIntegrationProviderImpl` to indicate that FILE storage does not support credential vending (requesitng redential vending with FILE storage does not produce any credentials and does not flag an error, which matches current Polaris behaviour). * Only those S3 systems where STS is not available (or disabled / not permitted) are affected. * Other storage integrations are not affected by this PR. --- .../TransactionalMetaStoreManagerImpl.java | 1 + .../polaris/core/storage/AccessConfig.java | 13 +++++ .../storage/PolarisStorageIntegration.java | 10 +++- .../service/it/RestCatalogMinIOSpecialIT.java | 57 +++++++++++++++++++ .../catalog/iceberg/IcebergCatalog.java | 2 +- .../iceberg/IcebergCatalogHandler.java | 23 +++++--- ...PolarisStorageIntegrationProviderImpl.java | 2 +- 7 files changed, 98 insertions(+), 10 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 3bb0061255..04a1218134 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -2093,6 +2093,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( PolarisStorageIntegration storageIntegration = ms.loadPolarisStorageIntegrationInCurrentTxn(callCtx, reloadedEntity.getEntity()); + storageIntegration.config(); // cannot be null getDiagnostics() .checkNotNull( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/AccessConfig.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/AccessConfig.java index e15fd2e916..94e74a3d66 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/AccessConfig.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/AccessConfig.java @@ -23,6 +23,7 @@ import java.util.Map; import java.util.Optional; import org.apache.polaris.immutables.PolarisImmutable; +import org.immutables.value.Value; @PolarisImmutable public interface AccessConfig { @@ -38,6 +39,15 @@ public interface AccessConfig { Optional expiresAt(); + /** + * Indicates whether the storage integration subsystem that produced this object is capable of + * credential vending in principle. + */ + @Value.Default + default boolean supportsCredentialVending() { + return true; + } + default String get(StorageAccessProperty key) { if (key.isCredential()) { return credentials().get(key.getPropertyName()); @@ -64,6 +74,9 @@ interface Builder { @CanIgnoreReturnValue Builder expiresAt(Instant expiresAt); + @CanIgnoreReturnValue + Builder supportsCredentialVending(boolean supportsCredentialVending); + default Builder put(StorageAccessProperty key, String value) { if (key.isExpirationTimestamp()) { expiresAt(Instant.ofEpochMilli(Long.parseLong(value))); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java index 1828d01c81..a2889da5e2 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java @@ -40,7 +40,7 @@ public PolarisStorageIntegration(T config, String identifierOrId) { this.integrationIdentifierOrId = identifierOrId; } - protected T config() { + public T config() { return config; } @@ -108,6 +108,14 @@ public abstract AccessConfig getSubscopedCreds( @Nonnull Set actions, @Nonnull Set locations); + /** + * @param credentialsRequired if {@code true} indicates that the caller requires the returned + * {@link AccessConfig} to have storage credentials; if {@code false} the returned {@link + * AccessConfig} may or may not contain credentials. + */ + public void validateCredentials( + @Nonnull AccessConfig accessConfig, boolean credentialsRequired) {} + /** * Result of calling {@link #validateAccessToLocations(RealmConfig, * PolarisStorageConfigurationInfo, Set, Set)} diff --git a/runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java b/runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java index 561e76938f..3cc2ac9648 100644 --- a/runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java +++ b/runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java @@ -31,6 +31,7 @@ import static org.apache.polaris.service.catalog.AccessDelegationMode.VENDED_CREDENTIALS; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.collect.ImmutableMap; import io.quarkus.test.junit.QuarkusIntegrationTest; @@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException { } } + @Test + public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException { + try (RESTCatalog restCatalog = + createCatalog( + Optional.of(endpoint), + Optional.of("http://sts.example.com"), // not called + false, + Optional.of(VENDED_CREDENTIALS), + false)) { + StorageConfigInfo storageConfig = + managementApi.getCatalog(catalogName).getStorageConfigInfo(); + assertThat((AwsStorageConfigInfo) storageConfig) + .extracting( + AwsStorageConfigInfo::getEndpoint, + AwsStorageConfigInfo::getStsEndpoint, + AwsStorageConfigInfo::getEndpointInternal, + AwsStorageConfigInfo::getPathStyleAccess, + AwsStorageConfigInfo::getStsUnavailable) + .containsExactly(endpoint, "http://sts.example.com", null, false, true); + + catalogApi.createNamespace(catalogName, "test-ns"); + TableIdentifier id = TableIdentifier.of("test-ns", "t2"); + // Credential vending is not supported without STS + assertThatThrownBy(() -> restCatalog.createTable(id, SCHEMA)) + .hasMessageContaining("but no credentials are available") + .hasMessageContaining(id.toString()); + } + } + + @Test + public void testLoadTableFailureWithCredentialVendingWithoutSts() throws IOException { + try (RESTCatalog restCatalog = + createCatalog( + Optional.of(endpoint), + Optional.of("http://sts.example.com"), // not called + false, + Optional.empty(), + false)) { + + catalogApi.createNamespace(catalogName, "test-ns"); + TableIdentifier id = TableIdentifier.of("test-ns", "t3"); + restCatalog.createTable(id, SCHEMA); + + // Credential vending is not supported without STS + assertThatThrownBy( + () -> + catalogApi.loadTable( + catalogName, + id, + "ALL", + Map.of("X-Iceberg-Access-Delegation", VENDED_CREDENTIALS.protocolValue()))) + .hasMessageContaining("but no credentials are available") + .hasMessageContaining(id.toString()); + } + } + public LoadTableResponse doTestCreateTable( RESTCatalog restCatalog, Optional dm) { catalogApi.createNamespace(catalogName, "test-ns"); diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index a338c007c8..0e21cdf128 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -844,7 +844,7 @@ public AccessConfig getAccessConfig( .atWarn() .addKeyValue("tableIdentifier", tableIdentifier) .log("Table entity has no storage configuration in its hierarchy"); - return AccessConfig.builder().build(); + return AccessConfig.builder().supportsCredentialVending(false).build(); } return FileIOUtil.refreshAccessConfig( callContext, diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 67e3c75ea0..604792da4c 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -804,13 +804,22 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential credentialDelegation.getAccessConfig( tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint); Map credentialConfig = accessConfig.credentials(); - if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) { - responseBuilder.addAllConfig(credentialConfig); - responseBuilder.addCredential( - ImmutableCredential.builder() - .prefix(tableMetadata.location()) - .config(credentialConfig) - .build()); + if (delegationModes.contains(VENDED_CREDENTIALS)) { + if (!credentialConfig.isEmpty()) { + responseBuilder.addAllConfig(credentialConfig); + responseBuilder.addCredential( + ImmutableCredential.builder() + .prefix(tableMetadata.location()) + .config(credentialConfig) + .build()); + } else { + Boolean skipCredIndirection = + realmConfig.getConfig(FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION); + Preconditions.checkArgument( + !accessConfig.supportsCredentialVending() || skipCredIndirection, + "Credential vending was requested for table %s, but no credentials are available", + tableIdentifier); + } } responseBuilder.addAllConfig(accessConfig.extraProperties()); } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java index e04a9525ba..23ec20abc3 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java @@ -115,7 +115,7 @@ public AccessConfig getSubscopedCreds( @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, Optional refreshCredentialsEndpoint) { - return AccessConfig.builder().build(); + return AccessConfig.builder().supportsCredentialVending(false).build(); } @Override From 1d570ea8723b0b38bfb0b5ab7db17673d0506898 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Fri, 3 Oct 2025 12:51:27 -0400 Subject: [PATCH 2/4] review: remove validateCredentials --- .../polaris/core/storage/PolarisStorageIntegration.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java index a2889da5e2..e2d9af7bf9 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java @@ -108,14 +108,6 @@ public abstract AccessConfig getSubscopedCreds( @Nonnull Set actions, @Nonnull Set locations); - /** - * @param credentialsRequired if {@code true} indicates that the caller requires the returned - * {@link AccessConfig} to have storage credentials; if {@code false} the returned {@link - * AccessConfig} may or may not contain credentials. - */ - public void validateCredentials( - @Nonnull AccessConfig accessConfig, boolean credentialsRequired) {} - /** * Result of calling {@link #validateAccessToLocations(RealmConfig, * PolarisStorageConfigurationInfo, Set, Set)} From ca7a92835b23542bb49c3fd2b04c5568505b411f Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Fri, 3 Oct 2025 12:52:26 -0400 Subject: [PATCH 3/4] review: remove spurious storageIntegration.config(); --- .../transactional/TransactionalMetaStoreManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 04a1218134..3bb0061255 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -2093,7 +2093,6 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( PolarisStorageIntegration storageIntegration = ms.loadPolarisStorageIntegrationInCurrentTxn(callCtx, reloadedEntity.getEntity()); - storageIntegration.config(); // cannot be null getDiagnostics() .checkNotNull( From 91698731d88b9484dde30866fdd9cfa8abd31130 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Fri, 3 Oct 2025 12:53:11 -0400 Subject: [PATCH 4/4] review: restore `protected config()` --- .../apache/polaris/core/storage/PolarisStorageIntegration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java index e2d9af7bf9..1828d01c81 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java @@ -40,7 +40,7 @@ public PolarisStorageIntegration(T config, String identifierOrId) { this.integrationIdentifierOrId = identifierOrId; } - public T config() { + protected T config() { return config; }