Enforce that S3 credentials are vended when requested #2711

dimas-b · 2025-09-29T17:34:28Z

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.

flyrain · 2025-09-30T00:44:27Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

              tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
      Map<String, String> credentialConfig = accessConfig.credentials();
-      if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+      if (delegationModes.contains(VENDED_CREDENTIALS)) {


Thanks @dimas-b for the change. But I think this is not the right place to check. SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION is mainly for POC or test env, where we allow Polaris to skip credential vending no matter what clients ask.

I believe the right place to check is in AwsCredentialsStorageIntegration::getSubscopedCreds(). Clients ask for credential vending, but this storage doesn't have the capability to do so.

AwsCredentialsStorageIntegration.getSubscopedCreds() may be called when STS is disabled (so no credentials will be vended). It's a valid use case for non-AWS S3 storage. Clients still need non-credential config from the "storage integration" code (e.g. endpoints).

We should put the check within if (shouldUseSts(storageConfig)) { ... }, sorry, the method shouldUseSts() itself.

@flyrain : could you propose pseudo-code (or java example) for this check?

Updated. PTAL.

This is follow-up bugfix for apache#2589 The bugfix part apache#2711 is extracted here since apache#2711 proved to be non-trivial and may require extra time. * Use the `delegationModes` method parameter as intended (as opposed to a local constant).

…2713) This is follow-up bugfix for #2589 The bugfix part #2711 is extracted here since #2711 proved to be non-trivial and may require extra time. * Use the `delegationModes` method parameter as intended (as opposed to a local constant).

dimas-b · 2025-09-30T04:41:21Z

client/python/integration_tests/conftest.py

            "warehouse": test_catalog.name,
            "credential": f"{test_principal.principal.client_id}:{test_principal.credentials.client_secret.get_secret_value()}",
            "scope": "PRINCIPAL_ROLE:ALL",
-            "header.X-Iceberg-Access-Delegation": "vended-credentials",


Cf. https://github.com/apache/iceberg-python/blob/pyiceberg-0.10.0/pyiceberg/catalog/rest/__init__.py#L487

singhpk234

Thank you @dimas-b !

singhpk234 · 2025-09-30T05:11:57Z

CHANGELOG.md

 - The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
  in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+  no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated


Suggested change

no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated

no credentials). This can happen with FILE and S3 compatible storage that does not have STS. Clients should normally be updated

AWS S3 may be used without STS (admin's choice).

How about This can happen with FILE and S3 storage if STS is not available or disabled (not permitted).?

SG ! for my understanding, when is the case when its actual S3 but we don't credential vending ? is it for case where the client already has credentials to work on storage, but then would they be sending the header ? or we just want admins to have this knob

Admins sometimes put restrictions on various policies / permissions even in AWS where STS endpoints are available.

Clients should not request vended credentials when they are known to be unavailable. The extra check is basically to make this kind of error more visible, I guess.

A changelog entry is not needed for the reworked changeset... WDYT?

singhpk234 · 2025-09-30T05:14:53Z

.../src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogFileIntegrationTest.java

+  @Test
+  @Override
+  @Disabled // Polaris does not provide credentials for FILE storage
+  public void testLoadCredentials() {
+    super.testLoadCredentials();
+  }


should we run these test with the config to allow empty creds on ?

This is no longer disabled.

runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java

dimas-b · 2025-09-30T17:54:15Z

It looks like the ripple effects of validating that vended credentials are available at the REST API levels are too wide.

I'll reset this PR and take a different approach (as @flyrain suggested).

dimas-b · 2025-10-01T02:08:44Z

Reworked this PR completely. Please review again.

HonahX · 2025-10-01T20:52:01Z

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java

        allowedWriteLocations,
-        refreshCredentialsEndpoint);
+        refreshCredentialsEndpoint,
+        credentialsRequired);


[non-blocker] I think this implies some extra cache miss for azure and gcp where credentialsRequired does not make any difference.

Do we have some general use-case of the credentialsRequired on gcp and azure, such that we could unify the credential existence check at storageCredentialCache, such that the persistence/credentialVendor just do its best effort to generate an access config, and let the cache api to decide whether the result is valid?

Good point about cache 🤔 Thx!

Do we have some general use-case of the credentialsRequired on gcp and azure

My first attempt in this PR was do enforce credential presence close to the REST level, but it proved to have too wide a ripple effect because many use cases (including PyIceberg) request credential vending by default and that breaks on FILE storage.

I'll see if I can refactor the code to maintain cache validity, but still delegate config validation to storage integrations.

HonahX · 2025-10-01T20:58:25Z

...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java


-    if (shouldUseSts(storageConfig)) {
+    boolean shouldUseSts = shouldUseSts(storageConfig);
+    Preconditions.checkArgument(


I think this error will be wrapped in to an error SubScopedResult and finally be thrown as UnprocessableEntityException in the StorageCredentialCache line 155. Just check if this is intended

On the java client side it shows up as org.apache.iceberg.exceptions.RESTException: Unable to process: Failed to get subscoped credentials: Credential vending was requested, but STS is not available. Is that reasonable?

In the REST response it is indeed a 422 with type = UnprocessableEntityException

Sorry to clarify, I think this is ok since 422 implies the same error will occur again if client retries, which is the current case. But in the PR description

Add checks to AwsCredentialsStorageIntegration (leading to 400) that S3 storage credentials are vended when requested.

says this check will lead to 400. So just point out in case it is intended to be 400 : )

Good catch - thx! I'm reworking the impl. again... it will be 400 / IllegalArgumentException at the REST level.

Re-implemented. PTAL.

dimas-b · 2025-10-02T02:35:59Z

I revamped this PR again 😅 Since there were too many "undo" changes, I squashed and force-pushed. Sorry about losing change history, but I think this revision is has the smallest "diff" so far 😉 Please review again.

HonahX · 2025-10-02T20:37:27Z

polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java

+  public void validateCredentials(
+      @Nonnull AccessConfig accessConfig, boolean credentialsRequired) {}


Seems this is not used?

Thanks for catching this and sorry about leftovers from previous code revisions 😓 ... removed.

HonahX · 2025-10-02T20:44:08Z

...ava/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java

    PolarisStorageIntegration<PolarisStorageConfigurationInfo> storageIntegration =
        ms.loadPolarisStorageIntegrationInCurrentTxn(callCtx, reloadedEntity.getEntity());

+    storageIntegration.config();


I think we could remove this?

polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java

This is a follow-up change to apache#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.

HonahX

LGTM! Thanks @dimas-b for adding the enforcement!

HonahX · 2025-10-03T19:10:47Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

+          Preconditions.checkArgument(
+              !accessConfig.supportsCredentialVending() || skipCredIndirection,
+              "Credential vending was requested for table %s, but no credentials are available",
+              tableIdentifier);


WDYT of moving this check to getAccessConfig /FileIOUtil.refreshAccessConfig in the later refactoring PR: #2736, and we probably do not need the additional supportsCredentialVending field in AccessConfig, just adding an additional arg to let caller decide whether to require the credential to be vended.

But I think that should be in a follow-up and after the 1.2.0 cut to avoid additional noise on the release. If I understand correctly, we do not offer backward compatibility of these inner interface. So we could do this if needed in the next release

Sounds reasonable to me.

TBH, I do not like AccessConfig.supportsCredentialVending(), but it seemed to be the simplest way to make the check without breaking other things 😅

github-project-automation bot added this to Basic Kanban Board Sep 29, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Sep 29, 2025

dimas-b added this to the 1.2.0 milestone Sep 29, 2025

dimas-b mentioned this pull request Sep 29, 2025

Support S3 storage that does not have STS #2672

Merged

flyrain reviewed Sep 30, 2025

View reviewed changes

dimas-b mentioned this pull request Sep 30, 2025

Fix delegationModes parameter propagation in createTableStaged() #2713

Merged

dimas-b force-pushed the enforce-vended-creds-presence branch from 049867d to 68ccb0e Compare September 30, 2025 04:39

dimas-b requested review from HonahX, adutra, flyrain and singhpk234 September 30, 2025 04:39

dimas-b commented Sep 30, 2025

View reviewed changes

singhpk234 reviewed Sep 30, 2025

View reviewed changes

dimas-b force-pushed the enforce-vended-creds-presence branch from cc52cad to ee9b6f7 Compare October 1, 2025 02:07

dimas-b changed the title ~~Enforce that credentials are vended when requested~~ Enforce that S3 credentials are vended when requested Oct 1, 2025

dimas-b requested a review from singhpk234 October 1, 2025 02:10

dimas-b force-pushed the enforce-vended-creds-presence branch 2 times, most recently from ac8970d to 11f909f Compare October 1, 2025 14:40

dimas-b mentioned this pull request Oct 1, 2025

Extract IcebergCatalog.getAccessConfig to a separate class AccessConfigProvider #2736

Merged

HonahX reviewed Oct 1, 2025

View reviewed changes

dimas-b force-pushed the enforce-vended-creds-presence branch from 11f909f to cc5036f Compare October 2, 2025 02:33

dimas-b requested a review from HonahX October 2, 2025 02:36

dimas-b force-pushed the enforce-vended-creds-presence branch from cc5036f to 10b7187 Compare October 2, 2025 14:54

HonahX reviewed Oct 2, 2025

View reviewed changes

dimas-b added 3 commits October 3, 2025 12:53

review: remove validateCredentials

1d570ea

review: remove spurious storageIntegration.config();

ca7a928

review: restore protected config()

9169873

dimas-b force-pushed the enforce-vended-creds-presence branch from 10b7187 to 9169873 Compare October 3, 2025 16:55

dimas-b requested a review from HonahX October 3, 2025 17:27

HonahX approved these changes Oct 3, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Oct 3, 2025

dimas-b merged commit 927bb5e into apache:main Oct 3, 2025
16 of 17 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Oct 3, 2025

dimas-b deleted the enforce-vended-creds-presence branch October 3, 2025 22:05

	no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
	no credentials). This can happen with FILE and S3 compatible storage that does not have STS. Clients should normally be updated

		public void validateCredentials(
		@Nonnull AccessConfig accessConfig, boolean credentialsRequired) {}

Enforce that S3 credentials are vended when requested #2711

Enforce that S3 credentials are vended when requested #2711

Conversation

dimas-b commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dimas-b commented Sep 29, 2025 •

edited

Loading

dimas-b Sep 30, 2025 •

edited

Loading

flyrain Sep 30, 2025 •

edited

Loading

dimas-b Sep 30, 2025 •

edited

Loading

singhpk234 Sep 30, 2025 •

edited

Loading

dimas-b commented Sep 30, 2025 •

edited

Loading

dimas-b Oct 1, 2025 •

edited

Loading

dimas-b Oct 1, 2025 •

edited

Loading

HonahX Oct 1, 2025 •

edited

Loading

dimas-b Oct 2, 2025 •

edited

Loading

dimas-b Oct 3, 2025 •

edited

Loading