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.

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.

I think Jannik's direction above is correct, eventually we will need to move auth manager outside of client. Should we just do it in this PR?

My concern is mainly around custom auth manager. currently init_session doesn't take any properties, and a custom auth manager may need to rely on props from RestCatalogConfig to get token or credential to even initialize. Outh2Manager in this PR uses from_config but I don't think it's generalizable

Took another look at Java's API again, initSession and catalogSession both take in a client and a properties map.

The main use case from what I understand is AuthManager can inherit the existing client from the catalog and modify necessary fields like AuthSession carried by the client, so this client can be used to do something else like refreshing token.

I think supporting refreshing token can be a different PR, but can we investigate this more to make sure the existing design does not block that?

@CTTY Thanks for digging into the Java signatures — both parameters are in the latest push. The client is actually used: OAuth2Manager no longer captures its own, so a manager reuses the catalog's pool, with with_client as an override. I also moved the auth out of HttpClient rather than deferring it: manager and session live on the catalog, HttpClient is plain transport again, and the init session is scoped to the handshake block. Thanks @DerGut for the prototype. Note this puts reqwest::Client in the public trait — already exposed via with_client, but say the word if you'd rather have an abstraction.

Awesome, thanks for addressing both! 🙇
IMO we should be consistent with abstractions: either use reqwest::Client and reqwest::Response directly, or introduce an abstraction for both of them. If we do it for one only, we'd get the worst of both worlds (public API tied to dependency and possibly worse ergonomics of wrapper).

I have't yet looked into it too deeply, but we could maybe just reuse the HttpClient that we already have.

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 🤦

- 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

- 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

Replaces the crate-private SensitiveString with the equivalent iceberg::Credential introduced in apache#2836.

plusplusjiajia · 2026-08-05T10:11:05Z

@CTTY All comments addressed, could you take another look when you get a chance?

CTTY

Thanks for continue working on this! I'm still not sure how we can schedule the token refreshing service with the existing API

CTTY · 2026-08-06T23:42:10Z

+    /// otherwise `oauth2` when a `token`, `credential` or `oauth2-server-uri`
+    /// is configured (preserving pre-`rest.auth.type` setups), `none` when
+    /// none is.
+    fn auth_type(&self) -> String {


we should make this case-insensitive

@CTTY Good catch, thanks — fixed: the configured type is now matched case-insensitively.

CTTY · 2026-08-07T00:04:54Z

+    ///
+    /// 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>>;


I think Jannik's direction above is correct, eventually we will need to move auth manager outside of client. Should we just do it in this PR?

My concern is mainly around custom auth manager. currently init_session doesn't take any properties, and a custom auth manager may need to rely on props from RestCatalogConfig to get token or credential to even initialize. Outh2Manager in this PR uses from_config but I don't think it's generalizable

CTTY · 2026-08-07T21:09:23Z

+    ///
+    /// 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>>;


Took another look at Java's API again, initSession and catalogSession both take in a client and a properties map.

The main use case from what I understand is AuthManager can inherit the existing client from the catalog and modify necessary fields like AuthSession carried by the client, so this client can be used to do something else like refreshing token.

I think supporting refreshing token can be a different PR, but can we investigate this more to make sure the existing design does not block that?

CTTY · 2026-08-07T21:53:04Z

+    ///
+    /// Both share the manager's token cell, so a cached token survives the
+    /// config handshake.
+    fn build_session(&self, params: OAuth2Params) -> Box<dyn AuthSession> {


This can be inlined to init_session if init_session takes in a prop map

@CTTY Good call — done: both paths share one session_from(client, props) now.

CTTY · 2026-08-07T22:01:41Z

+/// [`AuthSession`] for a pre-configured bearer token: attaches it as-is and
+/// cannot obtain a new one (there is no credential to exchange).
+#[derive(Debug)]
+struct StaticTokenSession {


I think the only difference between StaticTokenSession and ClientCredentialsSession is how they behave when the existing token is None. Can we just have one Oauth2Session?

Some thing like below

struct OAuth2Session { token: Arc<Mutex<Option<Credential>>>, token_source: TokenSource, } enum TokenSource { StaticToken ClientCredentials(ClientCredentialsConfig), } struct ClientCredentialsConfig { client: Client, credential: (Option<String>, Credential), token_endpoint: String, extra_headers: HeaderMap, extra_oauth_params: HashMap<String, String>, }

TokenSource can be refactored to TokenProvider if needed in the future to further reduce duplicate code

@CTTY Nice simplification, thanks — done.

CTTY · 2026-08-07T22:05:52Z

    }

-    /// Executes the given `Request` and returns a `Response`.
-    pub async fn execute(&self, mut request: Request) -> Result<Response> {


I think we should change the signature to take in AuthRequest/HttpRequest, HttpClient should be interacting with the wrapper HttpRequest. and HttpClient will use the underlying Client to work with the nested Request

@CTTY Good point, thanks — done: it takes the session now.

CTTY · 2026-08-07T22:07:37Z

+/// Wraps the request so authentication implementations depend only on the
+/// stable `http` crate and standard types, not on the concrete HTTP client the
+/// REST catalog uses internally.
+pub struct AuthRequest<'a> {


nit: I think HttpRequest is a better name. it will be more consistent to HttpClient. and HttpClient will only work with HttpRequest not directly with the underlying Request

@CTTY Thanks, agreed — renamed.

CTTY · 2026-08-07T22:07:59Z

+
+/// The body of an [`AuthRequest`], as seen by authentication.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum AuthRequestBody<'a> {


nit: same here, I think HttpRequestBody will be a better name

@CTTY Renamed too, thanks.

… and props

CTTY

thanks for continue working on this! I think we still have some gaps/fuzzy abstraction layers, but we are getting close!

AuthManager should live under RestCatalog directly
HttpClient should be binded with a session, and the child client should be able to just clone the parent client and override the session
Ideally I'd like to see no plain Request in catalog.rs, and RestCatalog should only work with HttpRequest directly

CTTY · 2026-08-10T23:17:51Z

+    /// the shared [`AuthManager::catalog_session`].
+    async fn init_session(
+        &self,
+        client: &Client,


AuthManager should be dealing with HttpClient and ideally only HttpClient will work with Client directly

CTTY · 2026-08-10T23:18:03Z

+    /// (e.g. a cached token) over from the init session.
+    async fn catalog_session(
+        &self,
+        client: &Client,


Same here, I think this should be HttpClient

CTTY · 2026-08-10T23:38:15Z

+
+    /// Resolves the auth manager: a `with_auth_manager` override wins,
+    /// otherwise one is built from the `rest.auth.type` configuration.
+    pub(crate) fn resolve_auth_manager(&self) -> Result<Arc<dyn AuthManager>> {


AuthManager should live within RestCatalog directly as Option<Arc<dyn AuthManager>>, and it should be resolved along with context initialization(You are already doing this now :) but I think the main point is that auth manager and related functions should not live in RestCatalogConfig.)

CTTY · 2026-08-10T23:38:54Z

+    default_client: Arc<OnceLock<Client>>,
+
+    #[builder(default)]
+    auth_manager: Option<Arc<dyn AuthManager>>,


This should be moved to RestCatalog directly

CTTY · 2026-08-10T23:51:22Z

-        self.execute(request).await
+    pub async fn query_catalog(
+        &self,
+        mut request: Request,


This should be HttpRequest

CTTY · 2026-08-10T23:56:29Z

+/// Wraps the request so an [`AuthSession`] mutates it through the stable
+/// `http` crate types rather than the concrete request type the REST catalog
+/// uses internally.
+pub struct HttpRequest<'a> {


Im leaning toward moving this to a different module rest/src/request.rs. And we move to use this most of the places. This way we will have a cleaner abstraction layer

CTTY · 2026-08-11T00:01:34Z

-    /// Extra oauth parameters to be added to each authentication request.
-    extra_oauth_params: HashMap<String, String>,
    /// Whether to disable header redaction in error logs (defaults to false for security).
    disable_header_redaction: bool,


I thought the HttpClient would be binded with a AuthSession?

CTTY · 2026-08-11T00:04:42Z

    client: HttpClient,
+    /// The session the catalog's auth manager derived from the merged
+    /// configuration; it authenticates every request below.
+    session: Arc<dyn AuthSession>,


I think the Session needs to be binded with the session directly. My understanding is that each client will have a session. and if a child session wants to use the parent client, it will need to clone the parent client and remove the parent session from the client

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

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

This was referenced Jul 30, 2026

Add SessionCatalog trait #2836

Merged

feat(rest): add SigV4 auth manager for the REST catalog #2660

Open

CTTY reviewed Jul 30, 2026

View reviewed changes

DerGut reviewed Jul 31, 2026

View reviewed changes

plusplusjiajia added 5 commits August 5, 2026 03:30

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

8b11282

docs(rest): tighten auth comments

5aaf209

chore: sort rest catalog dependencies for taplo

61eb02e

plusplusjiajia added 2 commits August 5, 2026 03:31

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

57dc2cf

refactor(rest): use iceberg::Credential for OAuth2 secrets

510dacd

Replaces the crate-private SensitiveString with the equivalent iceberg::Credential introduced in apache#2836.

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch from 4b2fa37 to 510dacd Compare August 5, 2026 08:09

plusplusjiajia mentioned this pull request Aug 5, 2026

fix: redact secret property values in SessionContext Debug #2960

Open

plusplusjiajia requested review from CTTY, DerGut and ublubu August 5, 2026 10:08

This was referenced Aug 5, 2026

Session/ Request/ Auth Context Propagation #2774

Open

refactor: Move SensitiveBytes and Credential into shared module #2971

Open

Credential refactor #2972

Open

CTTY reviewed Aug 7, 2026

View reviewed changes

refactor(rest): move auth out of HttpClient, hand managers the client…

450dbfc

… and props

plusplusjiajia force-pushed the feat/rest-auth-manager-core branch from 63b819d to 450dbfc Compare August 8, 2026 09:50

CTTY reviewed Aug 11, 2026

View reviewed changes

	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