Background
PR #217 added an LdapIdentityProvider (AssumeRoleWithLDAPIdentity STS) to src/s3/creds.rs. It works, but it could only be a partial transparent implementation because the Provider trait is synchronous and infallible:
// src/s3/creds.rs
pub trait Provider: std::fmt::Debug {
fn fetch(&self) -> Credentials;
}
STS credential retrieval is an async, fallible HTTP call. Since fetch() cannot await or return an error, the LDAP provider currently exposes a separate async refresh() that must prime the cache before the provider is used for signing; the sync fetch() then returns the cached credentials and cannot transparently re-fetch when they expire.
Goal
Make credential refresh transparent: a provider should be able to fetch/refresh credentials on demand (including on expiry) from within the request-signing path, without the caller having to pre-prime a cache.
Proposed change
Convert Provider to an async, fallible trait:
#[async_trait::async_trait]
pub trait Provider: std::fmt::Debug + Send + Sync {
async fn fetch(&self) -> Result<Credentials, Error>;
}
This is a breaking change to a public trait (downstream Provider implementors must update), so it warrants a minor/major version bump.
Scope / call sites to update
All .fetch() call sites are already inside async fn, so awaiting is mechanical:
src/s3/client/mod.rs:771 (execute_internal), :1020 (execute_with_custom_path), :1238 (get_object_fast)
src/s3/builders/get_presigned_object_url.rs:92
src/s3/builders/get_presigned_policy_form_data.rs:47
src/s3/rdma/protocol.rs:226, :342 (behind the rdma feature)
Impls to update:
StaticProvider (src/s3/creds.rs:71) — trivially returns Ok(...).
LdapIdentityProvider (src/s3/creds.rs:273) — move the real refetch-on-expiry into fetch() and drop the prime-the-cache-first requirement.
Alternatives considered
- Background refresh task (no API break): spawn a tokio task that refreshes the cache before expiry; keeps sync
fetch(). Avoids the breaking change but adds task lifecycle/cancellation complexity and can still return just-expired creds in a race.
- Blocking HTTP in sync
fetch() (reqwest blocking): rejected — fetch() runs inside the async signing path, so a blocking call stalls/deadlocks the executor.
The async-trait approach is preferred: it's mostly mechanical (call sites are already async) and the only real cost is the public-API bump.
Acceptance criteria
Provider::fetch is async + fallible; all call sites and built-in impls updated.
LdapIdentityProvider refreshes transparently on expiry with no pre-priming.
- Errors from credential retrieval propagate as
Error rather than being swallowed.
- Tests cover transparent refresh-on-expiry.
Context: follow-up to #217.
Background
PR #217 added an
LdapIdentityProvider(AssumeRoleWithLDAPIdentity STS) tosrc/s3/creds.rs. It works, but it could only be a partial transparent implementation because theProvidertrait is synchronous and infallible:STS credential retrieval is an async, fallible HTTP call. Since
fetch()cannotawaitor return an error, the LDAP provider currently exposes a separate asyncrefresh()that must prime the cache before the provider is used for signing; the syncfetch()then returns the cached credentials and cannot transparently re-fetch when they expire.Goal
Make credential refresh transparent: a provider should be able to fetch/refresh credentials on demand (including on expiry) from within the request-signing path, without the caller having to pre-prime a cache.
Proposed change
Convert
Providerto an async, fallible trait:This is a breaking change to a public trait (downstream
Providerimplementors must update), so it warrants a minor/major version bump.Scope / call sites to update
All
.fetch()call sites are already insideasync fn, so awaiting is mechanical:src/s3/client/mod.rs:771(execute_internal),:1020(execute_with_custom_path),:1238(get_object_fast)src/s3/builders/get_presigned_object_url.rs:92src/s3/builders/get_presigned_policy_form_data.rs:47src/s3/rdma/protocol.rs:226,:342(behind therdmafeature)Impls to update:
StaticProvider(src/s3/creds.rs:71) — trivially returnsOk(...).LdapIdentityProvider(src/s3/creds.rs:273) — move the real refetch-on-expiry intofetch()and drop the prime-the-cache-first requirement.Alternatives considered
fetch(). Avoids the breaking change but adds task lifecycle/cancellation complexity and can still return just-expired creds in a race.fetch()(reqwestblocking): rejected —fetch()runs inside the async signing path, so a blocking call stalls/deadlocks the executor.The async-trait approach is preferred: it's mostly mechanical (call sites are already async) and the only real cost is the public-API bump.
Acceptance criteria
Provider::fetchis async + fallible; all call sites and built-in impls updated.LdapIdentityProviderrefreshes transparently on expiry with no pre-priming.Errorrather than being swallowed.Context: follow-up to #217.