feat(rest): introduce AuthManager/AuthSession and migrate OAuth2 by plusplusjiajia · Pull Request #2838 · apache/iceberg-rust

plusplusjiajia · 2026-07-17T06:44:29Z

Modeled on Java's AuthManager API (the init/catalog session lifecycle, the Noop/OAuth2 manager set, and the SigV4-wraps-a-delegate composition coming in the follow-up), adapted to Rust idioms.

What it does

AuthManager/AuthSession traits in a new auth/ module: init_session() serves the GET /v1/config handshake, catalog_session(merged_props) serves everything after, so a manager can rebuild its session from server-merged properties.
Noop/OAuth2 managers, selected via a new rest.auth.type property (oauth2 is the default and behaves as no auth when neither token nor credential is set), injectable through RestCatalogBuilder::with_auth_manager.
OAuth2 token handling moves out of HttpClient into OAuth2Manager, with the cached token surviving the config handshake; OAuth2Manager is publicly constructible (new() + with_*).
Review items from feat(catalog-rest): introduce AuthManager/AuthSession, rework SigV4 as a wrapping auth manager #2815 folded in: the config field is auth_manager, and the test-only fake-request token shim is gone — tests observe the session's cached bearer (#[cfg(test)] bearer_token()) and assert the header the mock server receives.

No new dependencies; no public API removed (additions only, public-api.txt regenerated).

Java reference: org.apache.iceberg.rest.auth.

Deviations from Java

No tableSession/contextualSession yet — in Java they are default methods falling back to the catalog/parent session, and the Rust REST catalog has no call sites for them (contextualSession also needs a SessionCatalog concept that doesn't exist here yet). Adding defaulted trait methods later is non-breaking.
No close() — Rust relies on Drop, and this OAuth2 implementation has no background refresh executor to shut down.
AuthSession gains invalidate()/refresh() (not in Java) to back the existing RestCatalog::invalidate_token/regenerate_token APIs.
authenticate mutates the request in place instead of returning a new one.

plusplusjiajia · 2026-07-22T10:44:17Z

Split from #2815 per your review, @CTTY — first of two: the AuthManager/AuthSession traits + OAuth2 migration (pure refactor, existing OAuth2 tests pass unmodified). SigV4 follows by reworking #2660 on top.
Would appreciate a review when you have a chance.

DerGut · 2026-07-22T21:41:14Z

+    /// The auth manager living for the lifetime of the catalog.
+    auth_manager: Arc<dyn AuthManager>,
+    /// The session authenticating requests in the current phase.
+    session: Arc<dyn AuthSession>,


Carrying my feedback from the other PR over.

I think right now having the AuthManager and AuthSession as fields of the HttpClient is perfectly fine. However, we're going to need to add additional methods to the AuthManager trait which complicate this:

fn table_session(_: TableIdent, parent: Arc<dyn AuthSession>) -> Arc<dyn AuthSession>; fn contextual_session(_: SessionContext, parent: Arc<dyn AuthSession>) -> Arc<dyn AuthSession;

table_session() and contextual_session() will help us to enable credentials vending at table-level and query-level authentication respectively.
Both take arguments that a low-level HttpClient should have no business in dealing with IMO. E.g. TableIdent is Iceberg-specific and not HTTP-specific. Same goes for the SessionContext. In that sense, I feel like the RestCatalog may be a better place to host these two fields.

@DerGut Agreed it gets awkward once table_session/contextual_session land. They're on HttpClient because that's where requests execute and init→catalog mirrors new()→update_with(). Since those methods are deferred here, I'd move both to RestCatalog when they arrive — non-breaking. @CTTY's call if you'd rather do it now.

Works for me! 👍

+1 I don't see us keeping auth session and manager in the long term. I'm happy if we could address this in the follow up PR

DerGut · 2026-07-24T18:00:36Z

+    /// Drops any cached credentials so the next request re-authenticates.
+    async fn invalidate(&self) -> Result<()> {
+        Ok(())
+    }
+
+    /// Proactively refreshes cached credentials (e.g. re-exchanges an OAuth2
+    /// client credential for a new token), leaving them intact on failure.
+    async fn refresh(&self) -> Result<()> {
+        Ok(())
+    }


I wonder whether we should call out in the comments that these methods are only exposed for backwards-compatibility and that they aren't the intended main interface to work with going forward.

To give implementers of custom AuthManagers some guidance.

@DerGut Good call — documented : both back the existininvalidate_token/regenerate_token APIs, not the intended extension surface.

RestCatalog::invalidate_token/regenerate_token were implemented in the first place as a workaround, because we had no mechanism to allow users configure token expiry and regeneration, and we still don't have that now :). So I'm guessing existing users are adding custom code to build their own invalidation/refreshing logic to use it. (the original discussions of adding them can be found in #437)

I think we should drop these two APIs based on the following thoughts:

With AuthManager, users can implement/inject their own AuthManager to refresh/invalidate the token

These APIs won't make sense to non-oauth2 authenticators

It will be somewhat a breaking change, but it's more like users will need a different custom code to work with it and the change won't block users from doing what they do with a bit more code

With above said, I do think refreshing token is a basic feature that should come out of the box, and we should use #301 to track that work separately

Would love to hear other perspectives here!

@CTTY Good call — done in the latest push: dropped both APIs; AuthSession is now just authenticate. +1 on #301 for out-of-the-box refresh (until then a cached token lives for the catalog's lifetime; a custom AuthManager can do its own renewal).

DerGut · 2026-07-24T19:19:55Z

+    /// The bearer token this session would attach, if any. Test-only: lets
+    /// tests observe the cached token without issuing a request.
+    #[cfg(test)]
+    async fn bearer_token(&self) -> Option<String> {


IIUC this method is only (indirectly) used in three tests.

I'd argue that its somewhat redundant with the fn authenticate() and test helpers that simplify the ergonomics should probably live closer to the tests rather than extending the trait (which is public API).

Test helpers could rebuild this functionality in a test module. A shortened version:

async fn bearer_token_from_session(session: &dyn AuthSession) -> Result<Option<String>> { let header = authorization_header_from_session(session).await?; let bearer_token = header .map(|header| header.strip_prefix("Bearer ")) Ok(bearer_token) } async fn authorization_header_from_session(session: &dyn AuthSession) -> Result<Option<String>> { let req = Request::new(Method::GET, Url::parse("http://fake.com")?); let mut req = AuthRequest::new(req); session.authenticate(&mut req).await?; Ok(req.headers().get(AUTHORIZATION)) }

@DerGut You're right — removed. HttpClient::token() is now a #[cfg(test)] helper that authenticates a throwaway request and reads the bearer back.

Awesome, thanks!

DerGut · 2026-07-24T19:54:18Z

+        // Release the init-phase session before deriving the catalog session,
+        // so a manager whose init session guards a one-shot resource (released
+        // on drop) can build its catalog session without deadlocking.
+        drop(init_session);


I wonder whether we can instantiate the init_session in the scope of its use (the first /v1/config request) so that we don't have to deal with explicit drops.

This could be another signal that the AuthManager should rather live in the catalog because the HttpClient is not aware of which request is being made, and so it can't tell which session is the appropriate one to use (or to build).
In that sense, it's implicitly temporally coupled to what the session field has been set to, and has to assume that the first request being made is a /v1/config request.

@DerGut Deliberate: an earlier review found holding the init session across catalog_session() breaks managers whose init session guards a Drop-released resource (there's a test). Open to a tidier form that keeps the ordering.

I think the same argument holds here - we can still do it later.

Also thanks for writing the test in a behavioral way that allows to test other approaches. I was able to construct the init_session once in the get_or_try_init RestContext construction (on a test branch based on yours) and directly passed it to the RestCatalog::load_config call. Its lifetime is then constrained to only that constructor only and still passes the test.
I then kept a reference to the catalog_session on the RestContext and put a helper to always use that session on other query_catalog calls.

One thing I noticed while playing around with it a little more: we could tighten the AuthManager trait to return a Box if we did the change now. In my understanding, an init session is only meant to be used once and a public API that locks this in might better express an init_session's intent.

-async fn init_session(&self) -> Result<Arc<dyn AuthSession>> +async fn init_session(&self) -> Result<Box<dyn AuthSession>>

A catalog_session on the other hand is meant to be re-used (and shared by concurrent requests). The current API (if made public) wouldn't convey that difference.

@DerGut Thanks for trying it on a branch — that shape looks like the natural target when the manager moves into the catalog later. And done on the Box suggestion: init_session now returns Box, catalog_session keeps Arc, docs spell out the distinction.

DerGut · 2026-07-24T20:12:43Z

+        self.props
+            .get(REST_CATALOG_PROP_AUTH_TYPE)
+            .cloned()
+            .unwrap_or_else(|| AUTH_TYPE_OAUTH2.to_string())


Just flagging that this diverges from Java's default to none.

Even though the OAuth2Manager behaves similarly without a token, it doesn't match the NoopAuthManager's behavior exactly. For example:

a configured NoopAuthManager on client initialization will always noop - an OAuth2Manager can start authenticating if the /v1/config endpoint returns a token in the properties (this is arguably the better default behavior)

a call to NoopSession::refresh() will always succeed but a call on OAuth2Session::refresh() will fail if no token is backing it

@DerGut Keeps pre-refactor behavior — oauth2 was already the effective default, so none would be the breaking change. It's noop-equivalent when unconfigured (authenticate returns early without token/credential). You're right that refresh() differs.

Oh my bad! Thanks for clarifying!

I'm actually leaning toward using none as default here. Users should be aware of what auth type they are using when they absolutely need to use an auth manager

@CTTY Fair point — and digging into Java, this is exactly what AuthManagers.loadAuthManager does: default none, but infer oauth2 when a legacy token/credential is present, with a warning asking users to set rest.auth.type explicitly. The latest push mirrors both the inference and the warning. One deliberate delta: I also treat an explicit oauth2-server-uri as OAuth intent (Java only checks token/credential) — happy to drop that for strict parity if you prefer, WDYT?

DerGut · 2026-07-26T14:47:04Z

+struct OAuth2Params {
+    extra_headers: HeaderMap,
+    token_endpoint: String,
+    credential: Option<(Option<String>, String)>,


nit: I wonder whether its not a good opportunity to introduce an explicit type for sensitive information akin to the SensitiveBytes type in the Iceberg encryption crate (given the pub with_credential()):

Suggested change

credential: Option<(Option<String>, String)>,

credential: Option<(Option<String>, Credential)>,

that could add some features like redacted logging and zeroization:

pub struct Credential(Zeroizing<String>); impl Credential { pub new(value: String) -> Self; pub fn expose(&self) -> &str; } impl From<String> for Credential { // ... } // Something that explicitly redacts from logging impl fmt::Debug for Credential { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("Credential([REDACTED])") } }

Looks like a good idea to me

@DerGut Thanks — done: added SensitiveString (zeroize-on-drop, Debug prints [REDACTED]) for the token cache and credential; config/client Debug output redacts secrets too.

DerGut · 2026-07-26T19:47:37Z

@plusplusjiajia If you find the time, I would be very happy about your feedback on #2836 which builds the foundation for AuthManager::contextual_session 🙏

ublubu

Thank you! I haven't looked over the whole thing yet, but I like the part I've read.

A few comments inline.

(and tagging as a prerequisite for #1236)

ublubu · 2026-07-29T18:07:22Z

+    /// The in-memory request body, or `None` for an empty or streaming body.
+    pub fn body(&self) -> Option<&[u8]> {
+        self.inner.body().and_then(|body| body.as_bytes())
+    }


For AWS SigV4, we should differentiate between an empty body and a streaming body.

We can generate a signature for an empty body, but we can't do that for a streaming body.

@ublubu Thanks — done: body() now returns AuthRequestBody (Empty/Buffered/Streaming), with a test covering all three.

ublubu · 2026-07-29T18:28:41Z

+    /// 1. **No authentication** - Skip when both `credential` and `token` are missing.
+    /// 2. **Token authentication** - Use the provided `token` directly.
+    /// 3. **OAuth authentication** - Exchange `credential` for a token, cache it, then use it.


I think these three modes should each be their own AuthSession implementation, returned by the AuthManager depending on the configuration:

NoopSession

StaticTokenSession

OAuthSession

We can dedupe the shared logic in the "static preconfigured token" and the "OAuth credential exchange" variants with a wrapper type like this: https://github.com/apache/iceberg-rust/pull/2924/changes#r3677185416

+1. I'm a bit confused by the "modes" here, I think we have NoopSession in this PR already

@ublubu Thanks — done: split into StaticTokenSession / ClientCredentialsSession (token+credential pre-seeds the cache). Deduped via a shared helper for now; open to the TokenProvider wrapper once #2924 lands.

@CTTY Right — NoopSession stays the separate rest.auth.type=none implementation. The confusing "modes" doc was from before the split; the latest push has StaticTokenSession / ClientCredentialsSession as their own types, and the noop-like case is just a static session with no token configured (it attaches nothing)

ublubu · 2026-07-29T18:32:07Z

+        // Clone the token from lock without holding the lock for entire function.
+        let token = self.token.lock().await.clone();


If multiple clients hit token = None at the same time, all of them will attempt the credential exchange.

If we hold the lock instead, only one client makes the credential exchange. Yes, the other clients have to wait for that credential exchange to complete, but they would have to wait anyway (i.e. they'd otherwise be making their own credential exchanges).

example impl: https://github.com/apache/iceberg-rust/pull/2924/changes#r3677209863

+1, the lock should be held at least until the token is exchanged

@ublubu Thanks — done: the lock is held across the exchange

ublubu · 2026-07-29T18:33:49Z

+}
+
+impl<'a> AuthRequest<'a> {
+    pub(crate) fn new(inner: &'a mut Request) -> Self {


If this is pub, external AuthSession|AuthManager implementers can write unit tests.

@ublubu Thanks — done, AuthRequest::new is now pub.

- hold the token lock across the OAuth2 exchange (single flight) - three-state AuthRequestBody: Empty/Buffered/Streaming; pub AuthRequest::new - zeroize OAuth2 secrets via a redacting SensitiveString - split the OAuth2 session into static-token and client-credentials types - init_session returns Box<dyn AuthSession>; catalog_session stays Arc

CTTY

Thanks for this great work! I think the direction is correct, and have left some comments

CTTY · 2026-07-29T23:31:42Z

+struct OAuth2Params {
+    extra_headers: HeaderMap,
+    token_endpoint: String,
+    credential: Option<(Option<String>, String)>,


Looks like a good idea to me

CTTY · 2026-07-29T23:33:45Z

+    /// 1. **No authentication** - Skip when both `credential` and `token` are missing.
+    /// 2. **Token authentication** - Use the provided `token` directly.
+    /// 3. **OAuth authentication** - Exchange `credential` for a token, cache it, then use it.


+1. I'm a bit confused by the "modes" here, I think we have NoopSession in this PR already

CTTY · 2026-07-29T23:51:09Z

+        // Clone the token from lock without holding the lock for entire function.
+        let token = self.token.lock().await.clone();


+1, the lock should be held at least until the token is exchanged

CTTY · 2026-07-30T00:03:19Z

+    pub(crate) fn client(&self) -> Client {
+        self.client
+            .clone()
+            .unwrap_or_else(|| self.default_client.get_or_init(Client::default).clone())


does unwrap_or_default work here? why do we need an extra default_client?

@CTTY Good question — unwrap_or_default builds a new connection pool per call; the OnceLock shares one client across config clones (OAuth + catalog traffic, same pool as before the refactor).

CTTY · 2026-07-30T00:05:52Z

+        self.props
+            .get(REST_CATALOG_PROP_AUTH_TYPE)
+            .cloned()
+            .unwrap_or_else(|| AUTH_TYPE_OAUTH2.to_string())


I'm actually leaning toward using none as default here. Users should be aware of what auth type they are using when they absolutely need to use an auth manager

CTTY · 2026-07-30T00:06:29Z

+            AUTH_TYPE_OAUTH2 => Ok(Arc::new(OAuth2Manager::from_config(self)?)),
+            other => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("unknown '{REST_CATALOG_PROP_AUTH_TYPE}': {other}"),


we should give hint to users and ask them to use with_auth_manager in CatalogBuilder to inject custom auth manager

@CTTY Good point — done in the latest push.

CTTY · 2026-07-30T22:54:11Z

+    /// Drops any cached credentials so the next request re-authenticates.
+    async fn invalidate(&self) -> Result<()> {
+        Ok(())
+    }
+
+    /// Proactively refreshes cached credentials (e.g. re-exchanges an OAuth2
+    /// client credential for a new token), leaving them intact on failure.
+    async fn refresh(&self) -> Result<()> {
+        Ok(())
+    }


RestCatalog::invalidate_token/regenerate_token were implemented in the first place as a workaround, because we had no mechanism to allow users configure token expiry and regeneration, and we still don't have that now :). So I'm guessing existing users are adding custom code to build their own invalidation/refreshing logic to use it. (the original discussions of adding them can be found in #437)

I think we should drop these two APIs based on the following thoughts:

With AuthManager, users can implement/inject their own AuthManager to refresh/invalidate the token

These APIs won't make sense to non-oauth2 authenticators

It will be somewhat a breaking change, but it's more like users will need a different custom code to work with it and the change won't block users from doing what they do with a bit more code

With above said, I do think refreshing token is a basic feature that should come out of the box, and we should use #301 to track that work separately

Would love to hear other perspectives here!

CTTY · 2026-07-30T23:47:58Z

+    /// The auth manager living for the lifetime of the catalog.
+    auth_manager: Arc<dyn AuthManager>,
+    /// The session authenticating requests in the current phase.
+    session: Arc<dyn AuthSession>,


+1 I don't see us keeping auth session and manager in the long term. I'm happy if we could address this in the follow up PR

CTTY · 2026-07-30T23:53:07Z

+    ///
+    /// Returns a [`Box`]: an init session is used once and released, unlike
+    /// the shared [`AuthManager::catalog_session`].
+    async fn init_session(&self) -> Result<Box<dyn AuthSession>>;


If we are planning to move auth manager and session out side of client, how do we ping /v1/config? we just pass all configs from catalog to Oauth2Manager::new()?

I tried this out on an experimental branch. One possible way of doing it would be to continue to call /v1/config during the RestContext setup, and create the init_session just prior to it.
Because the RestContext is wrapped in a OnceCell, we have the guarantee that the init session is only called once (per /v1/config call)

@CTTY Good question — what Jannik sketched matches the intent: the catalog creates the init session just before its /v1/config call; the manager only consumes properties, never issues the request.

DerGut · 2026-07-31T00:10:10Z

+    ///
+    /// The auth manager is kept; it derives a new session from the merged
+    /// properties (carrying over state such as a cached token).
+    pub async fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> {


I noticed another nit: since this method is pub, changing it to async can be considered a breaking API change.
This feels like another signal that the HttpClient abstraction isn't designed to deal with an AuthManager. This time, because it's not expected to do long-running calls.

@DerGut Good catch, but HttpClient is pub(crate) — update_with isn't public API (not in public-api.txt), so the async change breaks nothing. Agreed HttpClient shouldn't own the AuthManager though; that move to the catalog is planned as a follow-up PR, which CTTY already approved.

Aaaah, thanks! I was already confused why this fn was pub at all 🤦

- unset rest.auth.type resolves to oauth2 only when a token, credential or oauth2-server-uri is configured, none otherwise - drop RestCatalog::invalidate_token/regenerate_token and AuthSession::invalidate/refresh (out-of-box refresh tracked separately) - hint at with_auth_manager for unknown auth types

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch 6 times, most recently from 4730ec1 to 6f4aad8 Compare July 22, 2026 10:31

plusplusjiajia marked this pull request as ready for review July 22, 2026 10:32

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch from 6f4aad8 to e61121f Compare July 22, 2026 10:37

DerGut reviewed Jul 22, 2026

View reviewed changes

plusplusjiajia mentioned this pull request Jul 24, 2026

feat(catalog-rest): introduce AuthManager/AuthSession, rework SigV4 as a wrapping auth manager #2815

Closed

DerGut reviewed Jul 24, 2026

View reviewed changes

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch from e61121f to 061b58c Compare July 25, 2026 15:10

plusplusjiajia added 2 commits July 25, 2026 12:46

feat(rest): introduce AuthManager/AuthSession and migrate OAuth2

f20f331

docs(rest): tighten auth comments

645a72f

DerGut reviewed Jul 26, 2026

View reviewed changes

ublubu reviewed Jul 29, 2026

View reviewed changes

ublubu mentioned this pull request Jul 29, 2026

DRAFT - support custom RequestAuthenticator for REST catalog client #2924

Draft

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch from 061b58c to f5628ee Compare July 30, 2026 07:01

chore: sort rest catalog dependencies for taplo

3151273

This was referenced Jul 30, 2026

Add SessionCatalog trait #2836

Open

feat(rest): support AWS SigV4 request signing for the REST catalog #2660

Open

CTTY reviewed Jul 30, 2026

View reviewed changes

DerGut reviewed Jul 31, 2026

View reviewed changes

feat(rest): warn when inferring oauth2 from legacy auth properties

4b2fa37

	credential: Option<(Option<String>, String)>,
	credential: Option<(Option<String>, Credential)>,

		// Clone the token from lock without holding the lock for entire function.
		let token = self.token.lock().await.clone();

Uh oh!

Conversation

plusplusjiajia commented Jul 17, 2026

What it does

Deviations from Java

Uh oh!

plusplusjiajia commented Jul 22, 2026

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!

DerGut Jul 24, 2026 • 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!

DerGut Jul 24, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

DerGut Jul 26, 2026 • 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!

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!

DerGut commented Jul 26, 2026

Uh oh!

ublubu left a comment • 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!

DerGut Jul 24, 2026 •

edited

Loading

DerGut Jul 24, 2026 •

edited

Loading

DerGut Jul 26, 2026 •

edited

Loading

ublubu left a comment •

edited

Loading

ublubu Jul 29, 2026 •

edited

Loading