DRAFT - support custom RequestAuthenticator for REST catalog client - #2924
Draft
ublubu wants to merge 1 commit into
Draft
DRAFT - support custom RequestAuthenticator for REST catalog client#2924ublubu wants to merge 1 commit into
ublubu wants to merge 1 commit into
Conversation
* support custom RequestAuthenticator for REST catalog client * safer Debug impls and error ctx * mutex-guard the entire OAuth2 token-fetch process
ublubu
commented
Jul 29, 2026
Comment on lines
+47
to
+61
| pub trait RequestAuthenticator: Send + Sync + Debug { | ||
| /// Apply authentication to the outgoing request. | ||
| async fn authenticate_request(&self, req: &mut Request) -> Result<()>; | ||
|
|
||
| /// Discard any cached auth state (tokens, signatures, credentials, etc). | ||
| async fn invalidate_cache(&self) -> Result<()>; | ||
|
|
||
| /// Refresh the cached auth state (e.g. a bearer token). | ||
| /// | ||
| /// Expected flow: | ||
| /// 1. Attempt to regenerate the auth state (e.g. fetch a new bearer token). | ||
| /// 2a. If regeneration succeeded, use the new state and discard the old state. (e.g. replace the token) | ||
| /// 2b. If regeneration failed, keep the old state but surface the error. (e.g. keep the old token) | ||
| async fn regenerate_cache(&self) -> Result<()>; | ||
| } |
ublubu
commented
Jul 29, 2026
Comment on lines
+173
to
+187
| pub struct AuthenticatorConfig { | ||
| /// Option 1: Custom request authenticator. | ||
| pub custom_authenticator: Option<Arc<dyn RequestAuthenticator>>, | ||
|
|
||
| /// Option 2: Default OAuth flow. | ||
| /// OAuth endpoint. | ||
| pub token_endpoint: String, | ||
| /// OAuth credentials. | ||
| pub credential: Option<(Option<String>, String)>, | ||
| /// OAuth params. | ||
| pub extra_oauth_params: HashMap<String, String>, | ||
|
|
||
| /// Option 3: Static auth token. | ||
| pub token: Option<String>, | ||
| } |
ublubu
commented
Jul 29, 2026
Comment on lines
+80
to
+82
| pub struct BearerTokenAuthenticator { | ||
| provider: Arc<dyn TokenProvider>, | ||
| } |
Author
There was a problem hiding this comment.
This wrapper type lets us share code between the StaticTokenSession and OAuthSession variants I suggested for #2838.
ublubu
commented
Jul 29, 2026
Comment on lines
+283
to
+293
| async fn token(&self) -> Result<String> { | ||
| let mut cached = self.cached_token.lock().await; | ||
|
|
||
| if let Some(token) = cached.clone() { | ||
| return Ok(token); | ||
| } | ||
|
|
||
| let token = self.exchange_credential_for_token().await?; | ||
| *cached = Some(token.clone()); | ||
| Ok(token) | ||
| } |
Author
There was a problem hiding this comment.
Since every client has to wait for a credential exchange (when there's token), they might as well all wait for the same credential exchange (instead of each making their own).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
prerequisite to #1236
I'm introducing this PR draft so I can point at a couple things from my #2838 review comments.
Changes included here:
support custom RequestAuthenticator for REST catalog client
mutex-guard the entire OAuth2 token-fetch process
duplicating my notes from #1236 below:
Hey, I had a look at the prior art catalogued in #2311, and I really like the
Authenticatortrait idea from #2088.Over at MaterializeInc/materialize, we've already been using
iceberg-rustwith AWS SigV4 for S3 Tables (andgcp_authfor Google's BigLake) using that technique.The main difference is that, while the trait lives in our
iceberg-rustfork, the AWS/GCP-specific authenticators are implemented in our home repo,materialize.SigV4 Authenticator:
GCP Authenticator (Bearer Token Authenticator wrapper + GCP Token Provider):
Proposal for an authenticator trait to upstream into
apache/iceberg-rustRequestAuthenticator/AuthenticatorAlmost exactly the same as #2088. Most importantly,
mutaccess to the entire request (needed for SigV4):iceberg-rust/crates/catalog/rest/src/token.rs
Lines 48 to 49 in a9ad7e2
Helper types and traits:
BearerTokenAuthenticator+trait TokenProviderOAuth2 and
gcp_auth(and the static token config option) all provide bearer tokens the authenticator can put in a request header.A
BearerTokenAuthenticatorholds aimpl TokenProviderso it can act asimpl RequestAuthenticator.Similar to #2088's
OAuth2Authenticator, but split into two parts.Draft diff to add just these new traits without breaking anything:
Here's a full diff illustrating what I have in mind. I did my best to preserve the existing auth behavior and config APIs.
main...MaterializeInc:iceberg-rust:to-upstream/requestauthenticator
Next steps
I'm actively working on Materialize's support for Iceberg. I'd like to get our
iceberg-rustfork closer to upstream, so I'm happy to drive this if it needs a driver.