Add credential auto-refresh support for web_identity/STS chains#1051
Add credential auto-refresh support for web_identity/STS chains#1051HaoXuAI wants to merge 2 commits into
Conversation
39b711e to
e2d667c
Compare
Tmonster
left a comment
There was a problem hiding this comment.
I would like to see some test for this, even if they are guarded by mode skip + a description of what kind of environment should be set up.
I think there is also a nicer way to handle the Vcpkg ports, I'll come back to this
| refresh_input.storage_type = secret_entry->storage_mode; | ||
| refresh_input.scope = kv_secret_pre.GetScope(); | ||
|
|
||
| auto child_count = StructType::GetChildCount(refresh_info.type()); |
There was a problem hiding this comment.
Can you leave a comment as to what children are needed in a secret refresh? I can't think of any
There was a problem hiding this comment.
Done — added a comment explaining that refresh_info is a STRUCT containing the original named parameters used to create the secret (e.g., chain, region, profile). These are stored by the aws extension when refresh='auto' is set. Re-passing them to SecretManager::CreateSecret re-runs the credential chain provider, which fetches fresh STS tokens.
| //! Attempt to refresh the catalog's storage secret if it supports auto-refresh. | ||
| //! Rebuilds the secret via its original provider (e.g., web_identity/sts credential chain) | ||
| //! which fetches fresh STS tokens. No-op if the secret lacks refresh_info. | ||
| static bool TryRefreshStorageSecret(ClientContext &context, const SecretEntry &secret_entry) { |
There was a problem hiding this comment.
I think this can be renamed to TryRefreshCatalogSecret?
There was a problem hiding this comment.
Done — renamed to TryRefreshCatalogSecret.
77625e9 to
b817338
Compare
Tmonster
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me, are we worried at all that this might refresh the secret unnecessarily? I.e if are credentials are valid for an hour, we don't need to refresh every time we read the table within that hour right?
b817338 to
89eb657
Compare
|
@Tmonster actually the biggest problem for us it got into a race situation when multiple queries landed and all try to refresh the creds. And I'm trying to resolve this issue now |
89eb657 to
ac82ffd
Compare
65b7b7b to
b395689
Compare
Tmonster
left a comment
There was a problem hiding this comment.
I don't think having one sigv4 refresh for all sigv4 authenticated catalogs is a good idea. Can we make the refresh a per catalog thing?
Also, if you can figure out a way to add tests that would be awesome. I understand it will require some more setup, but if you could include the setup in some script unders scripts, you can guard your test with a mode skip
|
|
||
| // Static member definitions — shared across all SIGV4Authorization instances | ||
| // AND TryRefreshStorageSecret (via GetRefreshMutex/GetLastRefreshTime accessors) | ||
| std::mutex SIGV4Authorization::refresh_mutex; |
There was a problem hiding this comment.
Is there a reason we need this shared across all SigV4Authorization instances? If I attach different sigv4 catalogs with different secrets, the refresh logic for both of them should not interfere
There was a problem hiding this comment.
Good catch on the wording — the mutex and last_refresh_time are actually instance members of SIGV4Authorization (declared in sigv4.hpp), not static/shared. Each attached catalog gets its own SIGV4Authorization instance, so the refresh logic is already per-catalog and won't interfere between different catalogs with different secrets.
The #include <mutex> at the top of the header is just the standard library include — it doesn't make anything shared.
|
|
||
| // Fast path: skip if refreshed recently | ||
| auto now = std::chrono::steady_clock::now(); | ||
| auto elapsed = std::chrono::duration_cast<std::chrono::seconds>(now - last_refresh_time).count(); |
There was a problem hiding this comment.
Seems like we run the if we refreshed recently return check here and also in MaybeRefreshSecret? I think we can gfet away with only doing this once?
There was a problem hiding this comment.
You're right — these two serve different purposes but the duplication is confusing:
MaybeRefreshSecret()in sigv4.cpp — called on every catalog API request, time-gated to 300s intervals. This is the hot path.TryRefreshCatalogSecret()in iceberg_table_information.cpp — called once duringGetVendedCredentials()to ensure fresh creds are snapshotted into per-table secrets. This isn't called per-request.
I'll simplify by removing the time-gate from TryRefreshCatalogSecret (it doesn't need one since it's not in the hot path) and just keeping it as a simple "refresh if refresh_info exists" helper. The time-gating only matters in MaybeRefreshSecret where it's called on every request.
b395689 to
9e37263
Compare
- Add per-catalog MaybeRefreshSecret() on SIGV4Authorization with: - Per-instance mutex (catalogs with different secrets don't interfere) - 300s time-gate (avoids refreshing on every request) - try_to_lock (concurrent callers skip instead of blocking) - Call MaybeRefreshSecret before SigV4 signing in CreateAWSInput - Call MaybeRefreshSecret before snapshotting credentials into per-table secrets - Propagate refresh_info to per-table secrets for httpfs auto-refresh
Extends the existing scripts/sigv4_mock_server.py with an STS AssumeRoleWithWebIdentity endpoint that issues rotating credentials (MOCKKEY0, MOCKKEY1, ...) and a /refresh_count endpoint, reusing the upstream mock instead of adding a second one. The test verifies the SigV4 credential refresh is time-gated: it fires on first use (ATTACH) and does not re-fetch on subsequent requests within the 300s window. Guarded by require-env CREDENTIAL_REFRESH_MOCK_AVAILABLE; source scripts/credential_refresh_test_setup.sh to run.
9e37263 to
6926e1a
Compare
Summary
Adds credential auto-refresh support for
web_identityandstscredential chains (e.g., IRSA on EKS). Without this, STS credentials expire after ~1 hour and Iceberg queries fail withExpiredToken.Problem
The iceberg extension was:
refresh_infofrom the catalog secret when building per-table secretsEven with duckdb-aws#136 (
web_identitychain with auto-refresh) and duckdb-httpfs#165 (refresh in more locations), the iceberg extension bypassed the refresh path entirely.Changes
sigv4.cpp/sigv4.hpp— Serialized credential refresh before SigV4 signingAdds
MaybeRefreshSecret()method toSIGV4Authorizationthat:REFRESH_INTERVAL_SECONDS)try_to_lock— concurrent callers skip instead of blockingCreateSecretInputfrom the secret'srefresh_infostruct and re-runs the credential chain viaSecretManager::CreateSecretto get fresh STS tokensThe
refresh_infois a STRUCT containing the original named parameters used to create the secret (e.g.,chain,region,profile). These are stored by the aws extension whenrefresh='auto'is set (automatically forweb_identityandstschains).iceberg_table_information.cpp— Propagate refresh_info to per-table secretsTryRefreshCatalogSecret()helper that mirrors httpfs's refresh logic using public DuckDB APIs (no cross-extension dependency)refresh_infoso httpfs can auto-refresh per-table secrets during long-running data readsDependencies
web_identitychain) — mergedTesting
test_web_identity_credential_refresh.testguarded bymode skipwith environment documentationExpiredTokenerrors, credentials refreshing transparently