Support S3 storage that does not have STS #2672

dimas-b · 2025-09-24T21:41:17Z

This change is backward compatible with old catalogs that have storage configuration for S3 systems with STS.

Add new property to S3 storage config: stsUnavailable (defaults to "available").
Do not call STS when unavailable in AwsCredentialsStorageIntegration, but still put other properties (e.g. s3.endpoint) into AccessConfig

Relates to #2615
Relates #2207

dennishuo · 2025-09-25T03:58:42Z

CHANGELOG.md


 - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag.

+- Added support for S3-compatible storage that does not have STS (use `stsUavailable: true` in catalog storage configuration)


Could be worth expanding this sentence with a bit more of what's in the yaml for easy reference if people are reading the notes without wanting to read the full spec change -- specifically to clarify that it disables credential vending rather than vending some kind of root credentials after skipping STS subscoping.

How does the behavior compare to setting SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION=true?

SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION=true disables all storage config in API responses. Disabling STS only removes credentials.

Re: message changes: I'm not sure it's worth adding YAML to CHANGELOG... Would it not make it too verbose? In the end CHANGELOG has notes for many releases 🤔

How about I update it with a CLI example when I add CLI support for the new config entry?

I agree that from an user standpoint, CLI (or curl :) ) and updating the configuration list on the documentation would be more than welcome.

I will certainly add a docs page about this to "getting started" once the impl. is approved :)

@dennishuo : are you ok with this path forward? 🙂

Yeah, to clarify, I didn't mean to add too much, just maybe adding a ; credential vending will then be disabled in the parentheses so that the change note would say

(use `stsUavailable: true` in catalog storage configuration; credential vending will then be disabled);

In any case, non-blocking.

Incidentally, SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION really was only intended to pertain to the "credential" aspect, so it's unfortunate that it sort of fell into fully disabling StorageConfig mix-ins even for the non-credential-related config. But we can sort that out some other time.

singhpk234 · 2025-09-26T18:36:34Z

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

-                  String.valueOf(i.toEpochMilli()));
-            });
+
+    if (storageConfig.shouldUseSts()) {


[doubt] what if the client says it wan't to do x-Iceberg-vended-credentials, indication it wants credentials vending, but the storage doesn't support it, should we throw a 400 instead ?

Ideally yes, and that was in my first revision of this PR. Unfortunately many existing use cases excessively request credential vending for FILE storage and would be broken if we were to enforce this logic "in general".

We cannot enforce it here, because "storage integration" code has valid call paths without credential vending, where it exposes non-credential config to clients (e.g. S3 endpoints).

I'd like to enforce stricter checks as you suggested, but due to risk of breaking existing clients (I saw that in our reg. tests), I propose to defer it. WDYT?

I see, thanks for the explanation, I totally missed taking FILE into consideration ! IMHO its relatively easy to start with a stricter behaviour first and relax it later rather doing the other way as now there might be clients out there who would expect the behaviour to fail open and now expect 200, which we later plan to change to 400 anyways.

I would have appreciated to discussed this more !

I'll make a follow-up PR for this presently.

@singhpk234 : thanks for the ping and sorry about missing a related bug (should be fixed in #2711)

flyrain · 2025-09-26T18:49:51Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java


+  /**
+   * Flag indicating whether STS is available or not. It is modeled in the negative to simplify
+   * support for unset values ({@code null} being interpreted as {@code false}).


Suggested change

* support for unset values ({@code null} being interpreted as {@code false}).

* support for unset values ({@code null} being interpreted as {@code true}).

Javadoc is correct, I believe 😅 A null (unset) value would be treated as StsUnavailable being false , meaning that STS is available (current default). Cf. shouldUseSts().

I believe interpreting null as false is quite common.

You are right, sorry for the misleading. I was confused by two new methods introduced, getStsUnavailable(), shouldUseSts(), I think it'd be nice to only keep one of them. For example, the check brought up by @singhpk234 here #2672 (comment), would be reasonable to apply when we check getStsUnavailable() purely. Wrapping getStsUnavailable() with shouldUseSts() makes it even more ambiguous, so that the logic become less obviously when we need to check if users ask for credential vending.

Ok, I removed shouldUseSts() from the config class. @flyrain : PTAL.

Thanks for doing that, this LGTM.

This change is backward compatible with old catalogs that have storage configuration for S3 systems with STS. * Add new property to S3 storage config: `stsUnavailable` (defaults to "available"). * Do not call STS when unavailable in `AwsCredentialsStorageIntegration`, but still put other properties (e.g. s3.endpoint) into `AccessConfig` Relates to apache#2615 Closes apache#2207

…s `true`

snazy

LGTM

The CLI should support the new option as well.

This is a follow-up change to apache#2672 and apache#2589 * Fix a big in `delegationModes` parameter propagation in `createTableStaged()` * Add checks (leading to 400) that credentials are vended when requested. * The check is disabled if `SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION` is set * Disable credential vending tests in file-based catalog since the "FILE" storage integration code never vends any credentials. These tests are still executed under `PolarisRestCatalogMinIOIT`

This is a follow-up change to apache#2672 * Add checks (leading to 400) that credentials are vended when requested. * The check is disabled if `SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION` or `ALLOW_EMPTY_VENDED_CREDENTIALS` are set * Disable credential vending tests in file-based catalog since the "FILE" storage integration code never vends any credentials. These tests are still executed under `PolarisRestCatalogMinIOIT` * Use `ALLOW_EMPTY_VENDED_CREDENTIALS=true` in `PolarisAuthzTestBase` since those tests make direct java calls to "load with access delegation" methods, but the test infra is not set up for credential vending. * Enable `ALLOW_EMPTY_VENDED_CREDENTIALS` in python client tests since PyIceberg requests credential vending by default, but the tests use FILE storage, for which Polaris does not vend credentials.

This is a follow-up change to apache#2672 striving to improve user-facing error reporting for S3 storage systems without STS. * Update call paths leading to `AccessConfig` to indicate whether the caller requires credentials to be vended or not. * Add checks to `AwsCredentialsStorageIntegration` (leading to 400) that S3 storage credentials are vended when requested. * Only those S3 systems where STS is not available (or disabled / not permitted) are affected. * Other storage integrations are not affected by this PR.

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.

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.

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

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

dimas-b force-pushed the non-sts-s3-catalog branch from e912210 to 8c393cb Compare September 24, 2025 21:42

dimas-b mentioned this pull request Sep 24, 2025

[ENHANCEMENT] Support non-AWS S3 storage that does not have STS #2207

Open

dimas-b force-pushed the non-sts-s3-catalog branch from 8c393cb to a28accd Compare September 25, 2025 00:00

dennishuo reviewed Sep 25, 2025

View reviewed changes

dimas-b force-pushed the non-sts-s3-catalog branch from a28accd to 4b7c9e2 Compare September 25, 2025 15:22

dimas-b requested a review from dennishuo September 25, 2025 15:22

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

singhpk234 reviewed Sep 26, 2025

View reviewed changes

flyrain reviewed Sep 26, 2025

View reviewed changes

dimas-b added 3 commits September 26, 2025 18:14

Allow setting stsUnavailable only if ALLOW_SETTING_S3_ENDPOINTS i…

69603b8

…s `true`

review: inline shouldUseSts()

60746be

dimas-b force-pushed the non-sts-s3-catalog branch from 4b7c9e2 to 60746be Compare September 26, 2025 22:14

dennishuo approved these changes Sep 28, 2025

View reviewed changes

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

snazy approved these changes Sep 29, 2025

View reviewed changes

dimas-b merged commit f97c5eb into apache:main Sep 29, 2025
14 checks passed

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

dimas-b deleted the non-sts-s3-catalog branch September 29, 2025 14:11

dimas-b mentioned this pull request Sep 29, 2025

Enforce that S3 credentials are vended when requested #2711

Merged

rohangoli mentioned this pull request Oct 1, 2025

Support Dell ECS On-Premise S3 Object Store with STS support #2743

Open


		- Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag.

		- Added support for S3-compatible storage that does not have STS (use `stsUavailable: true` in catalog storage configuration)

	* support for unset values ({@code null} being interpreted as {@code false}).
	* support for unset values ({@code null} being interpreted as {@code true}).

Support S3 storage that does not have STS #2672

Support S3 storage that does not have STS #2672

Uh oh!

Conversation

dimas-b commented Sep 24, 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!

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!

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!

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!

dimas-b Sep 26, 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!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dimas-b commented Sep 24, 2025 •

edited

Loading

dimas-b Sep 26, 2025 •

edited

Loading