From 8b1128291e307353097e2ed64be42a9041953cba Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 17 Jul 2026 01:40:00 -0400 Subject: [PATCH 1/9] feat(rest): introduce AuthManager/AuthSession and migrate OAuth2 --- crates/catalog/rest/public-api.txt | 43 ++ crates/catalog/rest/src/auth/mod.rs | 159 ++++++ crates/catalog/rest/src/auth/oauth2.rs | 353 +++++++++++++ crates/catalog/rest/src/catalog.rs | 689 +++++++++++++++++++++---- crates/catalog/rest/src/client.rs | 251 +++------ crates/catalog/rest/src/lib.rs | 2 + 6 files changed, 1225 insertions(+), 272 deletions(-) create mode 100644 crates/catalog/rest/src/auth/mod.rs create mode 100644 crates/catalog/rest/src/auth/oauth2.rs diff --git a/crates/catalog/rest/public-api.txt b/crates/catalog/rest/public-api.txt index 776b11c40a..98725d33e6 100644 --- a/crates/catalog/rest/public-api.txt +++ b/crates/catalog/rest/public-api.txt @@ -1,4 +1,11 @@ pub mod iceberg_catalog_rest +pub struct iceberg_catalog_rest::AuthRequest<'a> +impl<'a> iceberg_catalog_rest::AuthRequest<'a> +pub fn iceberg_catalog_rest::AuthRequest<'a>::body(&self) -> core::option::Option<&[u8]> +pub fn iceberg_catalog_rest::AuthRequest<'a>::headers(&self) -> &http::header::map::HeaderMap +pub fn iceberg_catalog_rest::AuthRequest<'a>::headers_mut(&mut self) -> &mut http::header::map::HeaderMap +pub fn iceberg_catalog_rest::AuthRequest<'a>::method(&self) -> &http::method::Method +pub fn iceberg_catalog_rest::AuthRequest<'a>::url_str(&self) -> &str pub struct iceberg_catalog_rest::CommitTableRequest pub iceberg_catalog_rest::CommitTableRequest::identifier: core::option::Option pub iceberg_catalog_rest::CommitTableRequest::requirements: alloc::vec::Vec @@ -175,6 +182,25 @@ impl serde_core::ser::Serialize for iceberg_catalog_rest::NamespaceResponse pub fn iceberg_catalog_rest::NamespaceResponse::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg_catalog_rest::NamespaceResponse pub fn iceberg_catalog_rest::NamespaceResponse::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> +pub struct iceberg_catalog_rest::NoopAuthManager +impl core::fmt::Debug for iceberg_catalog_rest::NoopAuthManager +pub fn iceberg_catalog_rest::NoopAuthManager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager +pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub struct iceberg_catalog_rest::OAuth2Manager +impl iceberg_catalog_rest::OAuth2Manager +pub fn iceberg_catalog_rest::OAuth2Manager::new(token_endpoint: impl core::convert::Into) -> Self +pub fn iceberg_catalog_rest::OAuth2Manager::with_client(self, client: reqwest::async_impl::client::Client) -> Self +pub fn iceberg_catalog_rest::OAuth2Manager::with_credential(self, client_id: core::option::Option, client_secret: alloc::string::String) -> Self +pub fn iceberg_catalog_rest::OAuth2Manager::with_extra_headers(self, headers: http::header::map::HeaderMap) -> Self +pub fn iceberg_catalog_rest::OAuth2Manager::with_extra_oauth_params(self, params: std::collections::hash::map::HashMap) -> Self +pub fn iceberg_catalog_rest::OAuth2Manager::with_token(self, token: impl core::convert::Into) -> Self +impl core::fmt::Debug for iceberg_catalog_rest::OAuth2Manager +pub fn iceberg_catalog_rest::OAuth2Manager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager +pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg_catalog_rest::RegisterTableRequest pub iceberg_catalog_rest::RegisterTableRequest::metadata_location: alloc::string::String pub iceberg_catalog_rest::RegisterTableRequest::name: alloc::string::String @@ -230,6 +256,7 @@ pub fn iceberg_catalog_rest::RestCatalog::update_namespace<'life0, 'life1, 'asyn pub fn iceberg_catalog_rest::RestCatalog::update_table<'life0, 'async_trait>(&'life0 self, commit: iceberg::catalog::TableCommit) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg_catalog_rest::RestCatalogBuilder impl iceberg_catalog_rest::RestCatalogBuilder +pub fn iceberg_catalog_rest::RestCatalogBuilder::with_auth_manager(self, auth_manager: alloc::sync::Arc) -> Self pub fn iceberg_catalog_rest::RestCatalogBuilder::with_client(self, client: reqwest::async_impl::client::Client) -> Self impl core::default::Default for iceberg_catalog_rest::RestCatalogBuilder pub fn iceberg_catalog_rest::RestCatalogBuilder::default() -> Self @@ -287,6 +314,22 @@ impl serde_core::ser::Serialize for iceberg_catalog_rest::UpdateNamespacePropert pub fn iceberg_catalog_rest::UpdateNamespacePropertiesResponse::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg_catalog_rest::UpdateNamespacePropertiesResponse pub fn iceberg_catalog_rest::UpdateNamespacePropertiesResponse::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> +pub const iceberg_catalog_rest::AUTH_TYPE_NONE: &str +pub const iceberg_catalog_rest::AUTH_TYPE_OAUTH2: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_AUTH_TYPE: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_URI: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_WAREHOUSE: &str +pub trait iceberg_catalog_rest::AuthManager: core::fmt::Debug + core::marker::Send + core::marker::Sync +pub fn iceberg_catalog_rest::AuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_catalog_rest::AuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager +pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager +pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub trait iceberg_catalog_rest::AuthSession: core::fmt::Debug + core::marker::Send + core::marker::Sync +pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::AuthRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::AuthSession::invalidate<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::AuthSession::refresh<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs new file mode 100644 index 0000000000..05602b9756 --- /dev/null +++ b/crates/catalog/rest/src/auth/mod.rs @@ -0,0 +1,159 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Pluggable authentication for the REST catalog, mirroring Iceberg Java's +//! `AuthManager`/`AuthSession` API. + +mod oauth2; + +use std::collections::HashMap; +use std::fmt::Debug; +use std::sync::Arc; + +use async_trait::async_trait; +use http::{HeaderMap, Method}; +use iceberg::Result; +pub use oauth2::OAuth2Manager; +use reqwest::Request; + +/// `rest.auth.type` value disabling authentication. +pub const AUTH_TYPE_NONE: &str = "none"; +/// `rest.auth.type` value selecting OAuth2 token authentication. +pub const AUTH_TYPE_OAUTH2: &str = "oauth2"; + +/// Creates the [`AuthSession`]s used to authenticate REST catalog requests. +/// +/// A manager is created once per catalog, either from the `rest.auth.type` +/// property or injected through `RestCatalogBuilder::with_auth_manager`, and +/// lives for the lifetime of the catalog. +#[async_trait] +pub trait AuthManager: Debug + Send + Sync { + /// Session used for the initial `/v1/config` handshake, built from the + /// user-supplied configuration. + async fn init_session(&self) -> Result>; + + /// Session used for all subsequent catalog requests, given the properties + /// merged from the user configuration and the server's config response. + /// + /// Implementations may carry state (e.g. a cached token) over from the + /// init session. + async fn catalog_session( + &self, + props: &HashMap, + ) -> Result>; +} + +/// An outgoing REST request being authenticated by an [`AuthSession`]. +/// +/// 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> { + inner: &'a mut Request, +} + +impl<'a> AuthRequest<'a> { + pub(crate) fn new(inner: &'a mut Request) -> Self { + Self { inner } + } + + /// The request method. + pub fn method(&self) -> &Method { + self.inner.method() + } + + /// The request URL, as a string (scheme, host, path and query). + pub fn url_str(&self) -> &str { + self.inner.url().as_str() + } + + /// The request headers. + pub fn headers(&self) -> &HeaderMap { + self.inner.headers() + } + + /// The mutable request headers, e.g. to add an `Authorization` header. + pub fn headers_mut(&mut self) -> &mut HeaderMap { + self.inner.headers_mut() + } + + /// 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()) + } +} + +/// Authenticates outgoing REST catalog requests. +#[async_trait] +pub trait AuthSession: Debug + Send + Sync { + /// Applies authentication to the request (adds headers, signs, ...). + async fn authenticate(&self, request: &mut AuthRequest<'_>) -> Result<()>; + + /// Drops any cached credentials so the next request re-authenticates. + /// + /// Exists to back the pre-existing [`RestCatalog::invalidate_token`] API + /// and has no counterpart in Java's `AuthSession`. Implementations are + /// expected to manage credential lifetime inside [`Self::authenticate`]; + /// this is not part of the intended extension surface, and the default + /// no-op is fine for most managers. + /// + /// [`RestCatalog::invalidate_token`]: crate::RestCatalog::invalidate_token + 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. + /// + /// Like [`Self::invalidate`], this backs the pre-existing + /// [`RestCatalog::regenerate_token`] API rather than being part of the + /// intended extension surface. + /// + /// [`RestCatalog::regenerate_token`]: crate::RestCatalog::regenerate_token + async fn refresh(&self) -> Result<()> { + Ok(()) + } +} + +/// [`AuthManager`] that performs no authentication. +#[derive(Debug)] +pub struct NoopAuthManager; + +/// [`AuthSession`] that performs no authentication. +#[derive(Debug)] +struct NoopSession; + +#[async_trait] +impl AuthManager for NoopAuthManager { + async fn init_session(&self) -> Result> { + Ok(Arc::new(NoopSession)) + } + + async fn catalog_session( + &self, + _props: &HashMap, + ) -> Result> { + Ok(Arc::new(NoopSession)) + } +} + +#[async_trait] +impl AuthSession for NoopSession { + async fn authenticate(&self, _request: &mut AuthRequest<'_>) -> Result<()> { + Ok(()) + } +} diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs new file mode 100644 index 0000000000..7177f0ff66 --- /dev/null +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -0,0 +1,353 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; +use std::fmt::{Debug, Formatter}; +use std::sync::Arc; + +use async_trait::async_trait; +use http::StatusCode; +use iceberg::{Error, ErrorKind, Result}; +use reqwest::header::HeaderMap; +use reqwest::{Client, Method}; +use tokio::sync::Mutex; + +use super::{AuthManager, AuthRequest, AuthSession}; +use crate::catalog::{ + REST_CATALOG_PROP_URI, RestCatalogConfig, credential_from_props, default_token_endpoint, + explicit_headers_from_props, +}; +use crate::types::{ErrorResponse, TokenResponse}; + +/// Per-phase OAuth2 parameters (init vs. post-handshake catalog phase). +#[derive(Clone)] +struct OAuth2Params { + extra_headers: HeaderMap, + token_endpoint: String, + credential: Option<(Option, String)>, + extra_oauth_params: HashMap, +} + +/// [`AuthManager`] implementing the OAuth2 client-credentials flow used by +/// Iceberg REST catalogs. +/// +/// A configured `token` is used directly; otherwise `credential` is exchanged +/// for a token at the token endpoint and cached. The cached token is shared +/// across sessions so it survives the config handshake. +pub struct OAuth2Manager { + client: Client, + token: Arc>>, + init_params: OAuth2Params, + /// True when the token endpoint was derived from the catalog URI (not + /// explicitly configured): it is then recomputed from the merged URI in + /// [`Self::catalog_session`], since `/v1/config` may override the URI. + endpoint_is_default: bool, +} + +impl OAuth2Manager { + /// Creates a manager exchanging credentials at `token_endpoint`, with no + /// token or credential configured. Combine with the `with_*` methods: + /// + /// ```rust,ignore + /// let manager = OAuth2Manager::new("https://auth.example.com/v1/oauth/tokens") + /// .with_credential(Some("client-id".into()), "client-secret".into()); + /// ``` + pub fn new(token_endpoint: impl Into) -> Self { + Self { + client: Client::default(), + token: Arc::new(Mutex::new(None)), + init_params: OAuth2Params { + extra_headers: HeaderMap::new(), + token_endpoint: token_endpoint.into(), + credential: None, + // Same default as the configuration path (and the + // pre-AuthManager client): the Iceberg catalog scope. + extra_oauth_params: HashMap::from([("scope".to_string(), "catalog".to_string())]), + }, + endpoint_is_default: false, + } + } + + /// Sets a bearer token used directly (takes precedence over `credential`). + pub fn with_token(mut self, token: impl Into) -> Self { + self.token = Arc::new(Mutex::new(Some(token.into()))); + self + } + + /// Sets the client credential exchanged for a token at the token endpoint. + pub fn with_credential(mut self, client_id: Option, client_secret: String) -> Self { + self.init_params.credential = Some((client_id, client_secret)); + self + } + + /// Sets the HTTP client used for token requests. + pub fn with_client(mut self, client: Client) -> Self { + self.client = client; + self + } + + /// Sets extra headers sent with token requests. + pub fn with_extra_headers(mut self, headers: HeaderMap) -> Self { + self.init_params.extra_headers = headers; + self + } + + /// Adds extra OAuth2 form parameters (e.g. `scope`, `audience`), merged + /// onto the defaults: provide a `scope` entry to replace the default + /// `catalog` scope. + pub fn with_extra_oauth_params(mut self, params: HashMap) -> Self { + self.init_params.extra_oauth_params.extend(params); + self + } + + pub(crate) fn from_config(cfg: &RestCatalogConfig) -> Result { + Ok(Self { + client: cfg.client(), + token: Arc::new(Mutex::new(cfg.token())), + init_params: OAuth2Params { + extra_headers: cfg.extra_headers()?, + token_endpoint: cfg.get_token_endpoint(), + credential: cfg.credential(), + extra_oauth_params: cfg.extra_oauth_params(), + }, + endpoint_is_default: cfg.explicit_oauth2_server_uri().is_none(), + }) + } +} + +impl Debug for OAuth2Manager { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("OAuth2Manager") + .field("token_endpoint", &self.init_params.token_endpoint) + .finish_non_exhaustive() + } +} + +#[async_trait] +impl AuthManager for OAuth2Manager { + async fn init_session(&self) -> Result> { + Ok(Arc::new(OAuth2Session { + client: self.client.clone(), + token: self.token.clone(), + params: self.init_params.clone(), + })) + } + + async fn catalog_session( + &self, + props: &HashMap, + ) -> Result> { + // The server config may carry a new token (or restate the user's). + if let Some(token) = props.get("token") { + *self.token.lock().await = Some(token.clone()); + } + + // Explicit property overrides are merged ONTO the manager's configured + // options: an injected manager keeps its token endpoint, extra headers + // and OAuth params unless a property explicitly overrides them. + let mut extra_headers = self.init_params.extra_headers.clone(); + extra_headers.extend(explicit_headers_from_props(props)?); + + let mut extra_oauth_params = self.init_params.extra_oauth_params.clone(); + for key in ["scope", "audience", "resource"] { + if let Some(value) = props.get(key) { + extra_oauth_params.insert(key.to_string(), value.to_string()); + } + } + + let token_endpoint = match props.get("oauth2-server-uri") { + Some(uri) if !uri.is_empty() => uri.clone(), + // The built-in manager's default endpoint follows the merged + // catalog URI (a `/v1/config` override may have changed it); + // injected managers keep their explicitly configured endpoint. + _ if self.endpoint_is_default => props + .get(REST_CATALOG_PROP_URI) + .map(|uri| default_token_endpoint(uri)) + .unwrap_or_else(|| self.init_params.token_endpoint.clone()), + _ => self.init_params.token_endpoint.clone(), + }; + + Ok(Arc::new(OAuth2Session { + client: self.client.clone(), + token: self.token.clone(), + params: OAuth2Params { + extra_headers, + token_endpoint, + credential: credential_from_props(props) + .or_else(|| self.init_params.credential.clone()), + extra_oauth_params, + }, + })) + } +} + +/// [`AuthSession`] adding a `Authorization: Bearer ` header. +struct OAuth2Session { + client: Client, + /// Cached bearer token, shared with the owning [`OAuth2Manager`]. + token: Arc>>, + params: OAuth2Params, +} + +impl Debug for OAuth2Session { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("OAuth2Session") + .field("token_endpoint", &self.params.token_endpoint) + .finish_non_exhaustive() + } +} + +impl OAuth2Session { + async fn exchange_credential_for_token(&self) -> Result { + // Credential must exist here. + let (client_id, client_secret) = self.params.credential.as_ref().ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + "Credential must be provided for authentication", + ) + })?; + + let mut params = HashMap::with_capacity(4); + params.insert("grant_type", "client_credentials"); + if let Some(client_id) = client_id { + params.insert("client_id", client_id); + } + params.insert("client_secret", client_secret); + params.extend( + self.params + .extra_oauth_params + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())), + ); + + let mut auth_req = self + .client + .request(Method::POST, &self.params.token_endpoint) + .headers(self.params.extra_headers.clone()) + .form(¶ms) + .build()?; + // extra headers add content-type application/json header it's necessary to override it with proper type + // note that form call doesn't add content-type header if already present + auth_req.headers_mut().insert( + http::header::CONTENT_TYPE, + http::HeaderValue::from_static("application/x-www-form-urlencoded"), + ); + let auth_url = auth_req.url().clone(); + let auth_resp = self.client.execute(auth_req).await?; + + let auth_res: TokenResponse = if auth_resp.status() == StatusCode::OK { + let text = auth_resp + .bytes() + .await + .map_err(|err| err.with_url(auth_url.clone()))?; + Ok(serde_json::from_slice(&text).map_err(|e| { + Error::new( + ErrorKind::Unexpected, + "Failed to parse response from rest catalog server!", + ) + .with_context("operation", "auth") + .with_context("url", auth_url.to_string()) + .with_context("json", String::from_utf8_lossy(&text)) + .with_source(e) + })?) + } else { + let code = auth_resp.status(); + let text = auth_resp + .bytes() + .await + .map_err(|err| err.with_url(auth_url.clone()))?; + let e: ErrorResponse = serde_json::from_slice(&text).map_err(|e| { + Error::new(ErrorKind::Unexpected, "Received unexpected response") + .with_context("code", code.to_string()) + .with_context("operation", "auth") + .with_context("url", auth_url.to_string()) + .with_context("json", String::from_utf8_lossy(&text)) + .with_source(e) + })?; + Err(Error::from(e)) + }?; + Ok(auth_res.access_token) + } +} + +#[async_trait] +impl AuthSession for OAuth2Session { + /// Adds a bearer token to the authorization header. + /// + /// Three modes: + /// + /// 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. + /// + /// When both `credential` and `token` are present, `token` takes precedence. + /// + /// # TODO: Support automatic token refreshing. + async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { + // Clone the token from lock without holding the lock for entire function. + let token = self.token.lock().await.clone(); + + if self.params.credential.is_none() && token.is_none() { + return Ok(()); + } + + // Either use the provided token or exchange credential for token, cache and use that + let token = match token { + Some(token) => token, + None => { + let token = self.exchange_credential_for_token().await?; + // Update token so that we use it for next request instead of + // exchanging credential for token from the server again + *self.token.lock().await = Some(token.clone()); + token + } + }; + + // Insert token in request. + req.headers_mut().insert( + http::header::AUTHORIZATION, + format!("Bearer {token}").parse().map_err(|e| { + Error::new( + ErrorKind::DataInvalid, + "Invalid token received from catalog server!", + ) + .with_source(e) + })?, + ); + + Ok(()) + } + + /// Invalidate the current token without generating a new one. On the next + /// request, the session will attempt to generate a new token. + async fn invalidate(&self) -> Result<()> { + *self.token.lock().await = None; + Ok(()) + } + + /// Invalidate the current token and set a new one. Generates a new token + /// before invalidating the current one, meaning the old token will be used + /// until this function acquires the lock and overwrites the token. + /// + /// If credential is invalid, or the request fails, this method will return + /// an error and leave the current token unchanged. + async fn refresh(&self) -> Result<()> { + let new_token = self.exchange_credential_for_token().await?; + *self.token.lock().await = Some(new_token); + Ok(()) + } +} diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 8642b32d22..26c48c24be 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -20,7 +20,7 @@ use std::collections::{HashMap, HashSet}; use std::future::Future; use std::str::FromStr; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use async_trait::async_trait; use iceberg::encryption::kms::{KeyManagementClient, KmsClientFactory}; @@ -38,6 +38,7 @@ use reqwest::{Client, Method, StatusCode, Url}; use tokio::sync::OnceCell; use typed_builder::TypedBuilder; +use crate::auth::{AUTH_TYPE_NONE, AUTH_TYPE_OAUTH2, AuthManager, NoopAuthManager, OAuth2Manager}; use crate::client::{ HttpClient, deserialize_catalog_response, deserialize_unexpected_catalog_error, }; @@ -54,6 +55,8 @@ pub const REST_CATALOG_PROP_URI: &str = "uri"; pub const REST_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Disable header redaction in error logs (defaults to false for security) pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str = "disable-header-redaction"; +/// Authentication scheme: `none` or `oauth2` (default). +pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type"; const ICEBERG_REST_SPEC_VERSION: &str = "0.14.1"; const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -77,6 +80,8 @@ impl Default for RestCatalogBuilder { warehouse: None, props: HashMap::new(), client: None, + default_client: Arc::new(OnceLock::new()), + auth_manager: None, }, storage_factory: None, kms_client_factory: None, @@ -161,6 +166,12 @@ impl RestCatalogBuilder { self.config.client = Some(client); self } + + /// Injects a custom auth manager, overriding the `rest.auth.type` configuration. + pub fn with_auth_manager(mut self, auth_manager: Arc) -> Self { + self.config.auth_manager = Some(auth_manager); + self + } } /// Rest catalog configuration. @@ -179,6 +190,15 @@ pub(crate) struct RestCatalogConfig { #[builder(default)] client: Option, + + /// Lazily-created default HTTP client, shared through clones of this + /// config so OAuth and catalog traffic reuse one connection pool + /// (matching the single-client behavior before the AuthManager refactor). + #[builder(default)] + default_client: Arc>, + + #[builder(default)] + auth_manager: Option>, } impl RestCatalogConfig { @@ -195,11 +215,13 @@ impl RestCatalogConfig { } pub(crate) fn get_token_endpoint(&self) -> String { - if let Some(oauth2_uri) = self.props.get("oauth2-server-uri") { - oauth2_uri.to_string() - } else { - [&self.uri, PATH_V1, "oauth", "tokens"].join("/") - } + self.explicit_oauth2_server_uri() + .unwrap_or_else(|| default_token_endpoint(&self.uri)) + } + + /// The `oauth2-server-uri` property, only when explicitly configured. + pub(crate) fn explicit_oauth2_server_uri(&self) -> Option { + self.props.get("oauth2-server-uri").cloned() } fn namespaces_endpoint(&self) -> String { @@ -231,9 +253,13 @@ impl RestCatalogConfig { ]) } - /// Get the client from the config. - pub(crate) fn client(&self) -> Option { - self.client.clone() + /// The HTTP client: the configured one, or a lazily-created default that + /// is shared across every user of this config (and its clones), so token + /// and catalog requests keep sharing one connection pool. + pub(crate) fn client(&self) -> Client { + self.client + .clone() + .unwrap_or_else(|| self.default_client.get_or_init(Client::default).clone()) } /// Get the token from the config. @@ -245,89 +271,32 @@ impl RestCatalogConfig { /// Get the credentials from the config. The client can use these credentials to fetch a new /// token. - /// - /// ## Output - /// - /// - `None`: No credential is set. - /// - `Some(None, client_secret)`: No client_id is set, use client_secret directly. - /// - `Some(Some(client_id), client_secret)`: Both client_id and client_secret are set. pub(crate) fn credential(&self) -> Option<(Option, String)> { - let cred = self.props.get("credential")?; - - match cred.split_once(':') { - Some((client_id, client_secret)) => { - Some((Some(client_id.to_string()), client_secret.to_string())) - } - None => Some((None, cred.to_string())), - } + credential_from_props(&self.props) } - /// Get the extra headers from config, which includes: - /// - /// - `content-type` - /// - `x-client-version` - /// - `user-agent` - /// - All headers specified by `header.xxx` in props. + /// Get the extra headers from config, see [`extra_headers_from_props`]. pub(crate) fn extra_headers(&self) -> Result { - let mut headers = HeaderMap::from_iter([ - ( - header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ), - ( - HeaderName::from_static("x-client-version"), - HeaderValue::from_static(ICEBERG_REST_SPEC_VERSION), - ), - ( - header::USER_AGENT, - HeaderValue::from_str(&format!("iceberg-rs/{CARGO_PKG_VERSION}")).unwrap(), - ), - ]); - - for (key, value) in self - .props - .iter() - .filter_map(|(k, v)| k.strip_prefix("header.").map(|k| (k, v))) - { - headers.insert( - HeaderName::from_str(key).map_err(|e| { - Error::new( - ErrorKind::DataInvalid, - format!("Invalid header name: {key}"), - ) - .with_source(e) - })?, - HeaderValue::from_str(value).map_err(|e| { - Error::new( - ErrorKind::DataInvalid, - format!("Invalid header value: {value}"), - ) - .with_source(e) - })?, - ); - } - - Ok(headers) + extra_headers_from_props(&self.props) } /// Get the optional OAuth headers from the config. pub(crate) fn extra_oauth_params(&self) -> HashMap { - let mut params = HashMap::new(); - - if let Some(scope) = self.props.get("scope") { - params.insert("scope".to_string(), scope.to_string()); - } else { - params.insert("scope".to_string(), "catalog".to_string()); - } - - let optional_params = ["audience", "resource"]; - for param_name in optional_params { - if let Some(value) = self.props.get(param_name) { - params.insert(param_name.to_string(), value.to_string()); - } - } + oauth_params_from_props(&self.props) + } - params + /// The properties handed to [`AuthManager::catalog_session`], with the + /// resolved token endpoint made explicit. + pub(crate) fn auth_props(&self) -> HashMap { + // `oauth2-server-uri` stays absent unless explicitly configured, so an + // injected manager keeps its own token endpoint instead of having a + // synthesized `/v1/oauth/tokens` forced onto it (posting a + // client secret to the wrong host). The resolved catalog `uri` (which + // a `/v1/config` override may have changed) IS passed, so the built-in + // manager can recompute its default endpoint from it. + let mut props = self.props.clone(); + props.insert(REST_CATALOG_PROP_URI.to_string(), self.uri.clone()); + props } /// Check if header redaction is disabled in error logs. @@ -341,6 +310,32 @@ impl RestCatalogConfig { .unwrap_or(false) } + /// The configured auth scheme: explicit `rest.auth.type` or the default + /// `oauth2` (which behaves as no auth when neither `token` nor + /// `credential` is set). + fn auth_type(&self) -> String { + self.props + .get(REST_CATALOG_PROP_AUTH_TYPE) + .cloned() + .unwrap_or_else(|| AUTH_TYPE_OAUTH2.to_string()) + } + + /// 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> { + if let Some(auth_manager) = &self.auth_manager { + return Ok(auth_manager.clone()); + } + match self.auth_type().as_str() { + AUTH_TYPE_NONE => Ok(Arc::new(NoopAuthManager)), + AUTH_TYPE_OAUTH2 => Ok(Arc::new(OAuth2Manager::from_config(self)?)), + other => Err(Error::new( + ErrorKind::DataInvalid, + format!("unknown '{REST_CATALOG_PROP_AUTH_TYPE}': {other}"), + )), + } + } + /// Merge the `RestCatalogConfig` with the a [`CatalogConfig`] (fetched from the REST server). pub(crate) fn merge_with_config(mut self, mut config: CatalogConfig) -> Self { if let Some(uri) = config.overrides.remove("uri") { @@ -349,6 +344,13 @@ impl RestCatalogConfig { let mut props = config.defaults; props.extend(self.props); + // The client-side warehouse was moved off the props by the builder; + // restore it between defaults and overrides so managers receive the + // resolved value via `auth_props` with the standard precedence + // (server default < client < server override). + if let Some(warehouse) = &self.warehouse { + props.insert(REST_CATALOG_PROP_WAREHOUSE.to_string(), warehouse.clone()); + } props.extend(config.overrides); self.props = props; @@ -356,6 +358,106 @@ impl RestCatalogConfig { } } +/// Parses the `credential` property. +/// +/// ## Output +/// +/// - `None`: No credential is set. +/// - `Some(None, client_secret)`: No client_id is set, use client_secret directly. +/// - `Some(Some(client_id), client_secret)`: Both client_id and client_secret are set. +pub(crate) fn credential_from_props( + props: &HashMap, +) -> Option<(Option, String)> { + let cred = props.get("credential")?; + + match cred.split_once(':') { + Some((client_id, client_secret)) => { + Some((Some(client_id.to_string()), client_secret.to_string())) + } + None => Some((None, cred.to_string())), + } +} + +/// The extra headers added to each request, which include: +/// +/// - `content-type` +/// - `x-client-version` +/// - `user-agent` +/// - All headers specified by `header.xxx` in props. +pub(crate) fn extra_headers_from_props(props: &HashMap) -> Result { + let mut headers = HeaderMap::from_iter([ + ( + header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ), + ( + HeaderName::from_static("x-client-version"), + HeaderValue::from_static(ICEBERG_REST_SPEC_VERSION), + ), + ( + header::USER_AGENT, + HeaderValue::from_str(&format!("iceberg-rs/{CARGO_PKG_VERSION}")).unwrap(), + ), + ]); + + headers.extend(explicit_headers_from_props(props)?); + + Ok(headers) +} + +/// The default OAuth2 token endpoint for a catalog `uri`. +pub(crate) fn default_token_endpoint(uri: &str) -> String { + [uri, PATH_V1, "oauth", "tokens"].join("/") +} + +/// Only the headers explicitly configured via `header.xxx` props (no defaults). +pub(crate) fn explicit_headers_from_props(props: &HashMap) -> Result { + let mut headers = HeaderMap::new(); + for (key, value) in props + .iter() + .filter_map(|(k, v)| k.strip_prefix("header.").map(|k| (k, v))) + { + headers.insert( + HeaderName::from_str(key).map_err(|e| { + Error::new( + ErrorKind::DataInvalid, + format!("Invalid header name: {key}"), + ) + .with_source(e) + })?, + HeaderValue::from_str(value).map_err(|e| { + Error::new( + ErrorKind::DataInvalid, + format!("Invalid header value: {value}"), + ) + .with_source(e) + })?, + ); + } + + Ok(headers) +} + +/// The optional OAuth parameters added to each authentication request. +pub(crate) fn oauth_params_from_props(props: &HashMap) -> HashMap { + let mut params = HashMap::new(); + + if let Some(scope) = props.get("scope") { + params.insert("scope".to_string(), scope.to_string()); + } else { + params.insert("scope".to_string(), "catalog".to_string()); + } + + let optional_params = ["audience", "resource"]; + for param_name in optional_params { + if let Some(value) = props.get(param_name) { + params.insert(param_name.to_string(), value.to_string()); + } + } + + params +} + #[derive(Debug)] struct RestContext { client: HttpClient, @@ -432,7 +534,7 @@ impl RestCatalog { async fn context(&self) -> Result<&RestContext> { self.ctx .get_or_try_init(|| async { - let client = HttpClient::new(&self.user_config)?; + let client = HttpClient::new(&self.user_config).await?; let catalog_config = RestCatalog::load_config(&client, &self.user_config).await?; // Use the advertised endpoints as-is, falling back to // `DEFAULT_ENDPOINTS` when absent or empty. @@ -443,7 +545,7 @@ impl RestCatalog { _ => crate::endpoint::DEFAULT_ENDPOINTS.clone(), }; let config = self.user_config.clone().merge_with_config(catalog_config); - let client = client.update_with(&config)?; + let client = client.update_with(&config).await?; Ok(RestContext { config, @@ -549,7 +651,7 @@ impl RestCatalog { /// Invalidate the current token without generating a new one. On the next request, the client /// will attempt to generate a new token. pub async fn invalidate_token(&self) -> Result<()> { - self.context().await?.client.invalidate_token().await + self.context().await?.client.session().invalidate().await } /// Invalidate the current token and set a new one. Generates a new token before invalidating @@ -559,7 +661,7 @@ impl RestCatalog { /// If credential is invalid, or the request fails, this method will return an error and leave /// the current token unchanged. pub async fn regenerate_token(&self) -> Result<()> { - self.context().await?.client.regenerate_token().await + self.context().await?.client.session().refresh().await } } @@ -1465,9 +1567,18 @@ mod tests { let oauth_mock = create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 200) .await; + // The next request re-exchanges the credential and sends the new token. + let ns_mock = server + .mock("GET", "/v1/namespaces") + .match_header("authorization", "Bearer ey000000000001") + .with_body(r#"{"namespaces": []}"#) + .create_async() + .await; catalog.invalidate_token().await.unwrap(); - let token = catalog.context().await.unwrap().client.token().await; + catalog.list_namespaces(None).await.unwrap(); oauth_mock.assert_async().await; + ns_mock.assert_async().await; + let token = catalog.context().await.unwrap().client.token().await; assert_eq!(token, Some("ey000000000001".to_string())); } @@ -1499,8 +1610,10 @@ mod tests { create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 500) .await; catalog.invalidate_token().await.unwrap(); - let token = catalog.context().await.unwrap().client.token().await; + // The failed re-exchange surfaces as an error and no token is cached. + assert!(catalog.list_namespaces(None).await.is_err()); oauth_mock.assert_async().await; + let token = catalog.context().await.unwrap().client.token().await; assert_eq!(token, None); } @@ -1765,6 +1878,404 @@ mod tests { list_ns_mock.assert_async().await; } + #[tokio::test] + async fn test_auth_type_none_disables_auth() { + // An explicit `rest.auth.type=none` wins over a configured token. + let props = HashMap::from([ + (REST_CATALOG_PROP_AUTH_TYPE.to_string(), "none".to_string()), + ("token".to_string(), "some-oauth-token".to_string()), + ]); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + + let session = config + .resolve_auth_manager() + .unwrap() + .init_session() + .await + .unwrap(); + let mut req = Client::new() + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .await + .unwrap(); + assert!(req.headers().get("authorization").is_none()); + } + + #[tokio::test] + async fn test_header_prop_overrides_token_on_the_wire() { + // Pre-AuthManager behavior, preserved: extra headers are applied after + // authentication, so a user-configured `header.authorization` wins + // over a configured token. + let mut server = Server::new_async().await; + let config_mock = create_config_mock(&mut server).await; + let list_ns_mock = server + .mock("GET", "/v1/namespaces") + .match_header("authorization", "Basic xyz") + .with_body(r#"{"namespaces": []}"#) + .create_async() + .await; + + let props = HashMap::from([ + ("token".to_string(), "some-oauth-token".to_string()), + ("header.authorization".to_string(), "Basic xyz".to_string()), + ]); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .props(props) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + + catalog.list_namespaces(None).await.unwrap(); + config_mock.assert_async().await; + list_ns_mock.assert_async().await; + } + + #[tokio::test] + async fn test_builtin_oauth_endpoint_follows_uri_override() { + // When `/v1/config` overrides `uri` (and no explicit `oauth2-server-uri` + // is set), the built-in manager's default token endpoint must follow + // the merged URI: a refresh after the handshake posts to the new host. + let mut bootstrap = Server::new_async().await; + let mut overridden = Server::new_async().await; + + let config_mock = bootstrap + .mock("GET", "/v1/config") + .with_status(200) + .with_body(format!( + r#"{{"overrides": {{"uri": "{}"}}, "defaults": {{}}}}"#, + overridden.url() + )) + .create_async() + .await; + // Handshake exchange still uses the bootstrap-derived default. + let bootstrap_oauth_mock = + create_oauth_mock_with_path(&mut bootstrap, "/v1/oauth/tokens", "tok-boot", 200).await; + // The refresh must follow the overridden URI. + let overridden_oauth_mock = + create_oauth_mock_with_path(&mut overridden, "/v1/oauth/tokens", "tok-new", 200).await; + + let props = HashMap::from([("credential".to_string(), "client1:secret1".to_string())]); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(bootstrap.url()) + .props(props) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + + catalog.context().await.unwrap(); + catalog.regenerate_token().await.unwrap(); + + config_mock.assert_async().await; + bootstrap_oauth_mock.assert_async().await; + overridden_oauth_mock.assert_async().await; + let token = catalog.context().await.unwrap().client.token().await; + assert_eq!(token, Some("tok-new".to_string())); + } + + #[tokio::test] + async fn test_injected_oauth_manager_keeps_endpoint_and_options() { + // An injected OAuth2Manager must keep its own token endpoint, extra + // headers and OAuth params across the config handshake: only explicit + // properties may override them, never synthesized defaults. + let mut server = Server::new_async().await; + let config_mock = create_config_mock(&mut server).await; + + // The catalog-host default endpoint must never see the credential. + let default_endpoint_mock = server + .mock("POST", "/v1/oauth/tokens") + .expect(0) + .create_async() + .await; + // Both exchanges (handshake + regenerate) hit the injected endpoint, + // carrying the injected header and OAuth param. + let custom_endpoint_mock = server + .mock("POST", "/custom/oauth/tokens") + .match_header("x-tenant", "t1") + // The default catalog scope must survive alongside the injected + // audience (with_extra_oauth_params merges onto the defaults). + .match_body(mockito::Matcher::AllOf(vec![ + mockito::Matcher::Regex("scope=catalog".to_string()), + mockito::Matcher::Regex("audience=aud-1".to_string()), + ])) + .with_status(200) + .with_body( + r#"{ + "access_token": "ey000000000000", + "token_type": "Bearer", + "issued_token_type": "urn:ietf:params:oauth:token-type:access_token", + "expires_in": 86400 + }"#, + ) + .expect(2) + .create_async() + .await; + + let manager = OAuth2Manager::new(format!("{}/custom/oauth/tokens", server.url())) + .with_credential(Some("client1".to_string()), "secret1".to_string()) + .with_extra_headers(HeaderMap::from_iter([( + HeaderName::from_static("x-tenant"), + HeaderValue::from_static("t1"), + )])) + .with_extra_oauth_params(HashMap::from([( + "audience".to_string(), + "aud-1".to_string(), + )])); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .auth_manager(Some(Arc::new(manager))) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + + // Handshake performs the first exchange; regenerate the second — both + // must use the injected endpoint/options. + catalog.context().await.unwrap(); + catalog.regenerate_token().await.unwrap(); + + config_mock.assert_async().await; + custom_endpoint_mock.assert_async().await; + default_endpoint_mock.assert_async().await; + } + + #[tokio::test] + async fn test_catalog_session_receives_resolved_warehouse() { + use tokio::sync::Mutex as AsyncMutex; + + use crate::auth::AuthSession; + + // A custom manager must receive the resolved warehouse in the props + // handed to `catalog_session`, with the standard precedence: + // server default < client-side warehouse < server override. + #[derive(Debug)] + struct PlainSession; + #[async_trait] + impl AuthSession for PlainSession { + async fn authenticate( + &self, + _request: &mut crate::auth::AuthRequest<'_>, + ) -> Result<()> { + Ok(()) + } + } + + #[derive(Debug)] + struct CapturingManager(Arc>>>); + #[async_trait] + impl AuthManager for CapturingManager { + async fn init_session(&self) -> Result> { + Ok(Arc::new(PlainSession)) + } + async fn catalog_session( + &self, + props: &HashMap, + ) -> Result> { + *self.0.lock().await = Some(props.clone()); + Ok(Arc::new(PlainSession)) + } + } + + // Client warehouse wins over a server default. + let mut server = Server::new_async().await; + let config_mock = server + .mock("GET", "/v1/config") + .match_query(mockito::Matcher::UrlEncoded( + "warehouse".to_string(), + "client-wh".to_string(), + )) + .with_status(200) + .with_body(r#"{"defaults": {"warehouse": "default-wh"}, "overrides": {}}"#) + .create_async() + .await; + let captured = Arc::new(AsyncMutex::new(None)); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .warehouse("client-wh".to_string()) + .auth_manager(Some(Arc::new(CapturingManager(captured.clone())))) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + catalog.context().await.unwrap(); + config_mock.assert_async().await; + let props = captured.lock().await.clone().unwrap(); + assert_eq!( + props.get("warehouse").map(String::as_str), + Some("client-wh") + ); + + // A server override wins over the client warehouse. + let mut server = Server::new_async().await; + let config_mock = server + .mock("GET", "/v1/config") + .match_query(mockito::Matcher::UrlEncoded( + "warehouse".to_string(), + "client-wh".to_string(), + )) + .with_status(200) + .with_body(r#"{"defaults": {}, "overrides": {"warehouse": "override-wh"}}"#) + .create_async() + .await; + let captured = Arc::new(AsyncMutex::new(None)); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .warehouse("client-wh".to_string()) + .auth_manager(Some(Arc::new(CapturingManager(captured.clone())))) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + catalog.context().await.unwrap(); + config_mock.assert_async().await; + let props = captured.lock().await.clone().unwrap(); + assert_eq!( + props.get("warehouse").map(String::as_str), + Some("override-wh") + ); + } + + #[tokio::test] + async fn test_init_session_dropped_before_catalog_session() { + use std::sync::atomic::{AtomicBool, Ordering}; + + use crate::auth::AuthSession; + + // A manager whose init session guards a one-shot resource (released on + // drop) must see it released before `catalog_session` is invoked. + #[derive(Debug)] + struct GuardSession(Arc); + impl Drop for GuardSession { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } + } + #[async_trait] + impl AuthSession for GuardSession { + async fn authenticate( + &self, + _request: &mut crate::auth::AuthRequest<'_>, + ) -> Result<()> { + Ok(()) + } + } + + #[derive(Debug)] + struct PlainSession; + #[async_trait] + impl AuthSession for PlainSession { + async fn authenticate( + &self, + _request: &mut crate::auth::AuthRequest<'_>, + ) -> Result<()> { + Ok(()) + } + } + + #[derive(Debug)] + struct GuardManager(Arc); + #[async_trait] + impl AuthManager for GuardManager { + async fn init_session(&self) -> Result> { + Ok(Arc::new(GuardSession(self.0.clone()))) + } + async fn catalog_session( + &self, + _props: &HashMap, + ) -> Result> { + if !self.0.load(Ordering::SeqCst) { + return Err(Error::new( + ErrorKind::Unexpected, + "init session must be dropped before catalog_session", + )); + } + Ok(Arc::new(PlainSession)) + } + } + + let mut server = Server::new_async().await; + let config_mock = create_config_mock(&mut server).await; + + let dropped = Arc::new(AtomicBool::new(false)); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .auth_manager(Some(Arc::new(GuardManager(dropped.clone())))) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + + catalog.context().await.unwrap(); + config_mock.assert_async().await; + assert!(dropped.load(Ordering::SeqCst)); + } + + #[test] + fn test_unknown_auth_type_is_rejected() { + let props = HashMap::from([( + REST_CATALOG_PROP_AUTH_TYPE.to_string(), + "kerberos".to_string(), + )]); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + + let err = config.resolve_auth_manager().unwrap_err(); + assert!(err.message().contains(REST_CATALOG_PROP_AUTH_TYPE)); + } + + #[test] + fn test_with_auth_manager_overrides_config() { + // A custom auth manager takes precedence over `rest.auth.type`. + #[derive(Debug)] + struct StubAuthManager; + #[async_trait] + impl AuthManager for StubAuthManager { + async fn init_session(&self) -> Result> { + unimplemented!() + } + async fn catalog_session( + &self, + _props: &HashMap, + ) -> Result> { + unimplemented!() + } + } + + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([( + REST_CATALOG_PROP_AUTH_TYPE.to_string(), + "kerberos".to_string(), + )])) + .auth_manager(Some(Arc::new(StubAuthManager))) + .build(); + + // The unknown auth type is never consulted. + assert!(config.resolve_auth_manager().is_ok()); + } + #[tokio::test] async fn test_list_namespace_with_pagination() { let mut server = Server::new_async().await; diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index 07dc0620da..0b9300ac75 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -17,34 +17,27 @@ use std::collections::HashMap; use std::fmt::{Debug, Formatter}; +use std::sync::Arc; -use http::StatusCode; use iceberg::{Error, ErrorKind, Result}; use reqwest::header::HeaderMap; use reqwest::{Client, IntoUrl, Method, Request, RequestBuilder, Response}; use serde::de::DeserializeOwned; -use tokio::sync::Mutex; use crate::RestCatalogConfig; -use crate::types::{ErrorResponse, TokenResponse}; +use crate::auth::{AuthManager, AuthRequest, AuthSession}; pub(crate) struct HttpClient { client: Client, - /// The token to be used for authentication. - /// - /// It's possible to fetch the token from the server while needed. - token: Mutex>, - /// The token endpoint to be used for authentication. - token_endpoint: String, - /// The credential to be used for authentication. - credential: Option<(Option, String)>, /// Extra headers to be added to each request. extra_headers: HeaderMap, - /// Extra oauth parameters to be added to each authentication request. - extra_oauth_params: HashMap, /// Whether to disable header redaction in error logs (defaults to false for security). disable_header_redaction: bool, + /// The auth manager living for the lifetime of the catalog. + auth_manager: Arc, + /// The session authenticating requests in the current phase. + session: Arc, } impl Debug for HttpClient { @@ -58,16 +51,15 @@ impl Debug for HttpClient { impl HttpClient { /// Create a new http client. - pub fn new(cfg: &RestCatalogConfig) -> Result { - let extra_headers = cfg.extra_headers()?; + pub async fn new(cfg: &RestCatalogConfig) -> Result { + let auth_manager = cfg.resolve_auth_manager()?; + let session = auth_manager.init_session().await?; Ok(HttpClient { - client: cfg.client().unwrap_or_default(), - token: Mutex::new(cfg.token()), - token_endpoint: cfg.get_token_endpoint(), - credential: cfg.credential(), - extra_headers, - extra_oauth_params: cfg.extra_oauth_params(), + client: cfg.client(), + extra_headers: cfg.extra_headers()?, disable_header_redaction: cfg.disable_header_redaction(), + auth_manager, + session, }) } @@ -75,172 +67,66 @@ impl HttpClient { /// /// If cfg carries new value, we will use cfg instead. /// Otherwise, we will keep the old value. - pub fn update_with(self, cfg: &RestCatalogConfig) -> Result { + /// + /// 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 { + let HttpClient { + // The same client comes back from `cfg.client()` below: the config + // clone shares the lazily-created default (or the user's client). + client: _, + extra_headers: current_headers, + disable_header_redaction: _, + auth_manager, + session: init_session, + } = self; + // 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); + let extra_headers = (!cfg.extra_headers()?.is_empty()) .then(|| cfg.extra_headers()) .transpose()? - .unwrap_or(self.extra_headers); + .unwrap_or(current_headers); + let session = auth_manager.catalog_session(&cfg.auth_props()).await?; Ok(HttpClient { - client: cfg.client().unwrap_or(self.client), - token: Mutex::new(cfg.token().or_else(|| self.token.into_inner())), - token_endpoint: if !cfg.get_token_endpoint().is_empty() { - cfg.get_token_endpoint() - } else { - self.token_endpoint - }, - credential: cfg.credential().or(self.credential), + client: cfg.client(), extra_headers, - extra_oauth_params: if !cfg.extra_oauth_params().is_empty() { - cfg.extra_oauth_params() - } else { - self.extra_oauth_params - }, disable_header_redaction: cfg.disable_header_redaction(), + auth_manager, + session, }) } - /// This API is testing only to assert the token. - #[cfg(test)] - pub(crate) async fn token(&self) -> Option { - let mut req = self - .request(Method::GET, &self.token_endpoint) - .build() - .unwrap(); - self.authenticate(&mut req).await.ok(); - self.token.lock().await.clone() - } - - async fn exchange_credential_for_token(&self) -> Result { - // Credential must exist here. - let (client_id, client_secret) = self.credential.as_ref().ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - "Credential must be provided for authentication", - ) - })?; - - let mut params = HashMap::with_capacity(4); - params.insert("grant_type", "client_credentials"); - if let Some(client_id) = client_id { - params.insert("client_id", client_id); - } - params.insert("client_secret", client_secret); - params.extend( - self.extra_oauth_params - .iter() - .map(|(k, v)| (k.as_str(), v.as_str())), - ); - - let mut auth_req = self - .request(Method::POST, &self.token_endpoint) - .form(¶ms) - .build()?; - // extra headers add content-type application/json header it's necessary to override it with proper type - // note that form call doesn't add content-type header if already present - auth_req.headers_mut().insert( - http::header::CONTENT_TYPE, - http::HeaderValue::from_static("application/x-www-form-urlencoded"), - ); - let auth_url = auth_req.url().clone(); - let auth_resp = self.client.execute(auth_req).await?; - - let auth_res: TokenResponse = if auth_resp.status() == StatusCode::OK { - let text = auth_resp - .bytes() - .await - .map_err(|err| err.with_url(auth_url.clone()))?; - Ok(serde_json::from_slice(&text).map_err(|e| { - Error::new( - ErrorKind::Unexpected, - "Failed to parse response from rest catalog server!", - ) - .with_context("operation", "auth") - .with_context("url", auth_url.to_string()) - .with_context("json", String::from_utf8_lossy(&text)) - .with_source(e) - })?) - } else { - let code = auth_resp.status(); - let text = auth_resp - .bytes() - .await - .map_err(|err| err.with_url(auth_url.clone()))?; - let e: ErrorResponse = serde_json::from_slice(&text).map_err(|e| { - Error::new(ErrorKind::Unexpected, "Received unexpected response") - .with_context("code", code.to_string()) - .with_context("operation", "auth") - .with_context("url", auth_url.to_string()) - .with_context("json", String::from_utf8_lossy(&text)) - .with_source(e) - })?; - Err(Error::from(e)) - }?; - Ok(auth_res.access_token) - } - - /// Invalidate the current token without generating a new one. On the next request, the client - /// will attempt to generate a new token. - pub(crate) async fn invalidate_token(&self) -> Result<()> { - *self.token.lock().await = None; - Ok(()) - } - - /// Invalidate the current token and set a new one. Generates a new token before invalidating - /// the current token, meaning the old token will be used until this function acquires the lock - /// and overwrites the token. - /// - /// If credential is invalid, or the request fails, this method will return an error and leave - /// the current token unchanged. - pub(crate) async fn regenerate_token(&self) -> Result<()> { - let new_token = self.exchange_credential_for_token().await?; - *self.token.lock().await = Some(new_token.clone()); - Ok(()) + /// The session authenticating requests in the current phase. + pub(crate) fn session(&self) -> &Arc { + &self.session } - /// Authenticates the request by adding a bearer token to the authorization header. - /// - /// This method supports three authentication modes: - /// - /// 1. **No authentication** - Skip authentication when both `credential` and `token` are missing. - /// 2. **Token authentication** - Use the provided `token` directly for authentication. - /// 3. **OAuth authentication** - Exchange `credential` for a token, cache it, then use it for authentication. - /// - /// When both `credential` and `token` are present, `token` takes precedence. + /// Testing only: the bearer token the current session would attach. /// - /// # TODO: Support automatic token refreshing. - async fn authenticate(&self, req: &mut Request) -> Result<()> { - // Clone the token from lock without holding the lock for entire function. - let token = self.token.lock().await.clone(); - - if self.credential.is_none() && token.is_none() { - return Ok(()); - } - - // Either use the provided token or exchange credential for token, cache and use that - let token = match token { - Some(token) => token, - None => { - let token = self.exchange_credential_for_token().await?; - // Update token so that we use it for next request instead of - // exchanging credential for token from the server again - *self.token.lock().await = Some(token.clone()); - token - } - }; - - // Insert token in request. - req.headers_mut().insert( - http::header::AUTHORIZATION, - format!("Bearer {token}").parse().map_err(|e| { - Error::new( - ErrorKind::DataInvalid, - "Invalid token received from catalog server!", - ) - .with_source(e) - })?, - ); - - Ok(()) + /// Authenticates a throwaway request (never sent — `authenticate` only + /// mutates it) and reads the header back, so this works for any + /// [`AuthSession`] without the trait carrying a test-only method. + #[cfg(test)] + pub(crate) async fn token(&self) -> Option { + let mut request = self + .client + .request(Method::GET, "http://localhost/token-probe") + .build() + .ok()?; + self.session + .authenticate(&mut AuthRequest::new(&mut request)) + .await + .ok()?; + request + .headers() + .get(reqwest::header::AUTHORIZATION)? + .to_str() + .ok()? + .strip_prefix("Bearer ") + .map(str::to_string) } #[inline] @@ -250,17 +136,16 @@ impl HttpClient { .headers(self.extra_headers.clone()) } - /// Executes the given `Request` and returns a `Response`. - pub async fn execute(&self, mut request: Request) -> Result { - request.headers_mut().extend(self.extra_headers.clone()); - Ok(self.client.execute(request).await?) - } - // Queries the Iceberg REST catalog after authentication with the given `Request` and // returns a `Response`. pub async fn query_catalog(&self, mut request: Request) -> Result { - self.authenticate(&mut request).await?; - self.execute(request).await + // Authenticate first, then apply extra headers, so a configured + // `header.authorization` keeps overriding a token (unchanged behavior). + self.session + .authenticate(&mut AuthRequest::new(&mut request)) + .await?; + request.headers_mut().extend(self.extra_headers.clone()); + Ok(self.client.execute(request).await?) } /// Returns whether header redaction is disabled for this client. diff --git a/crates/catalog/rest/src/lib.rs b/crates/catalog/rest/src/lib.rs index 383728401f..663ea65f26 100644 --- a/crates/catalog/rest/src/lib.rs +++ b/crates/catalog/rest/src/lib.rs @@ -51,11 +51,13 @@ #![deny(missing_docs)] +mod auth; mod catalog; mod client; mod endpoint; mod types; +pub use auth::*; pub use catalog::*; pub use endpoint::Endpoint; pub use types::*; From 5aaf2094ee6bad7fad9f1b81c95d3cfce0b8dc8e Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Sat, 25 Jul 2026 11:06:45 -0400 Subject: [PATCH 2/9] docs(rest): tighten auth comments --- crates/catalog/rest/src/auth/mod.rs | 11 +++-------- crates/catalog/rest/src/auth/oauth2.rs | 13 +++++-------- crates/catalog/rest/src/catalog.rs | 13 ++++--------- crates/catalog/rest/src/client.rs | 14 ++++++-------- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index 05602b9756..39c801d268 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -105,11 +105,8 @@ pub trait AuthSession: Debug + Send + Sync { /// Drops any cached credentials so the next request re-authenticates. /// - /// Exists to back the pre-existing [`RestCatalog::invalidate_token`] API - /// and has no counterpart in Java's `AuthSession`. Implementations are - /// expected to manage credential lifetime inside [`Self::authenticate`]; - /// this is not part of the intended extension surface, and the default - /// no-op is fine for most managers. + /// Backs the existing [`RestCatalog::invalidate_token`] API; not part of + /// the intended extension surface, and the default no-op is usually fine. /// /// [`RestCatalog::invalidate_token`]: crate::RestCatalog::invalidate_token async fn invalidate(&self) -> Result<()> { @@ -119,9 +116,7 @@ pub trait AuthSession: Debug + Send + Sync { /// Proactively refreshes cached credentials (e.g. re-exchanges an OAuth2 /// client credential for a new token), leaving them intact on failure. /// - /// Like [`Self::invalidate`], this backs the pre-existing - /// [`RestCatalog::regenerate_token`] API rather than being part of the - /// intended extension surface. + /// Like [`Self::invalidate`], backs [`RestCatalog::regenerate_token`]. /// /// [`RestCatalog::regenerate_token`]: crate::RestCatalog::regenerate_token async fn refresh(&self) -> Result<()> { diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs index 7177f0ff66..03100c4b24 100644 --- a/crates/catalog/rest/src/auth/oauth2.rs +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -74,8 +74,7 @@ impl OAuth2Manager { extra_headers: HeaderMap::new(), token_endpoint: token_endpoint.into(), credential: None, - // Same default as the configuration path (and the - // pre-AuthManager client): the Iceberg catalog scope. + // Same default as the configuration path: the catalog scope. extra_oauth_params: HashMap::from([("scope".to_string(), "catalog".to_string())]), }, endpoint_is_default: false, @@ -156,9 +155,8 @@ impl AuthManager for OAuth2Manager { *self.token.lock().await = Some(token.clone()); } - // Explicit property overrides are merged ONTO the manager's configured - // options: an injected manager keeps its token endpoint, extra headers - // and OAuth params unless a property explicitly overrides them. + // Explicit property overrides merge ONTO the manager's options, so an + // injected manager keeps whatever a property doesn't override. let mut extra_headers = self.init_params.extra_headers.clone(); extra_headers.extend(explicit_headers_from_props(props)?); @@ -171,9 +169,8 @@ impl AuthManager for OAuth2Manager { let token_endpoint = match props.get("oauth2-server-uri") { Some(uri) if !uri.is_empty() => uri.clone(), - // The built-in manager's default endpoint follows the merged - // catalog URI (a `/v1/config` override may have changed it); - // injected managers keep their explicitly configured endpoint. + // A default endpoint follows the merged catalog URI (which + // `/v1/config` may have overridden); explicit ones are kept. _ if self.endpoint_is_default => props .get(REST_CATALOG_PROP_URI) .map(|uri| default_token_endpoint(uri)) diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 26c48c24be..fb0ed72bb7 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -289,11 +289,8 @@ impl RestCatalogConfig { /// resolved token endpoint made explicit. pub(crate) fn auth_props(&self) -> HashMap { // `oauth2-server-uri` stays absent unless explicitly configured, so an - // injected manager keeps its own token endpoint instead of having a - // synthesized `/v1/oauth/tokens` forced onto it (posting a - // client secret to the wrong host). The resolved catalog `uri` (which - // a `/v1/config` override may have changed) IS passed, so the built-in - // manager can recompute its default endpoint from it. + // injected manager keeps its own endpoint. The resolved `uri` IS passed + // so the built-in manager can recompute its default from it. let mut props = self.props.clone(); props.insert(REST_CATALOG_PROP_URI.to_string(), self.uri.clone()); props @@ -344,10 +341,8 @@ impl RestCatalogConfig { let mut props = config.defaults; props.extend(self.props); - // The client-side warehouse was moved off the props by the builder; - // restore it between defaults and overrides so managers receive the - // resolved value via `auth_props` with the standard precedence - // (server default < client < server override). + // The builder moved the client warehouse off the props; restore it + // between defaults and overrides (default < client < override). if let Some(warehouse) = &self.warehouse { props.insert(REST_CATALOG_PROP_WAREHOUSE.to_string(), warehouse.clone()); } diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index 0b9300ac75..2dc26cdb60 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -72,17 +72,15 @@ impl HttpClient { /// properties (carrying over state such as a cached token). pub async fn update_with(self, cfg: &RestCatalogConfig) -> Result { let HttpClient { - // The same client comes back from `cfg.client()` below: the config - // clone shares the lazily-created default (or the user's client). + // `cfg.client()` below returns this same shared client. client: _, extra_headers: current_headers, disable_header_redaction: _, auth_manager, session: init_session, } = self; - // 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. + // Release the init session first, so a manager whose init session + // guards a one-shot resource can build its catalog session. drop(init_session); let extra_headers = (!cfg.extra_headers()?.is_empty()) @@ -106,9 +104,9 @@ impl HttpClient { /// Testing only: the bearer token the current session would attach. /// - /// Authenticates a throwaway request (never sent — `authenticate` only - /// mutates it) and reads the header back, so this works for any - /// [`AuthSession`] without the trait carrying a test-only method. + /// Authenticates a throwaway request (never sent) and reads the header + /// back, so it works for any [`AuthSession`] without a test-only trait + /// method. #[cfg(test)] pub(crate) async fn token(&self) -> Option { let mut request = self From 38fc47ac3cbdb8b8bac7696a1114d335bf30ef02 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Thu, 30 Jul 2026 00:16:26 -0400 Subject: [PATCH 3/9] fix(rest): address auth review feedback - 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; catalog_session stays Arc --- Cargo.lock | 3 + crates/catalog/rest/Cargo.toml | 5 + crates/catalog/rest/public-api.txt | 28 +++- crates/catalog/rest/src/auth/mod.rs | 177 +++++++++++++++++++-- crates/catalog/rest/src/auth/oauth2.rs | 209 +++++++++++++++---------- crates/catalog/rest/src/catalog.rs | 179 +++++++++++++++++++-- crates/catalog/rest/src/client.rs | 61 ++++++-- 7 files changed, 541 insertions(+), 121 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0f264cc1f5..141b76dcbb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3891,7 +3891,9 @@ name = "iceberg-catalog-rest" version = "0.10.0" dependencies = [ "async-trait", + "bytes", "chrono", + "futures", "http 1.4.2", "iceberg", "iceberg_test_utils", @@ -3904,6 +3906,7 @@ dependencies = [ "tokio", "typed-builder", "uuid", + "zeroize", ] [[package]] diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 247709efd4..1541fe8200 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -36,6 +36,7 @@ http = { workspace = true } iceberg = { workspace = true } itertools = { workspace = true } reqwest = { workspace = true } +zeroize = { workspace = true } serde = { workspace = true } serde_derive = { workspace = true } serde_json = { workspace = true } @@ -44,8 +45,12 @@ typed-builder = { workspace = true } uuid = { workspace = true, features = ["v4"] } [dev-dependencies] +bytes = { workspace = true } +futures = { workspace = true } iceberg_test_utils = { path = "../../test_utils", features = ["tests"] } mockito = { workspace = true } +# `stream` lets tests build a streaming body to exercise AuthRequestBody::Streaming. +reqwest = { workspace = true, features = ["stream"] } tokio = { workspace = true } [lints] diff --git a/crates/catalog/rest/public-api.txt b/crates/catalog/rest/public-api.txt index 98725d33e6..e0beecd24a 100644 --- a/crates/catalog/rest/public-api.txt +++ b/crates/catalog/rest/public-api.txt @@ -1,10 +1,26 @@ pub mod iceberg_catalog_rest +pub enum iceberg_catalog_rest::AuthRequestBody<'a> +pub iceberg_catalog_rest::AuthRequestBody::Buffered(&'a [u8]) +pub iceberg_catalog_rest::AuthRequestBody::Empty +pub iceberg_catalog_rest::AuthRequestBody::Streaming +impl<'a> iceberg_catalog_rest::AuthRequestBody<'a> +pub fn iceberg_catalog_rest::AuthRequestBody<'a>::as_bytes(&self) -> core::option::Option<&'a [u8]> +impl<'a> core::clone::Clone for iceberg_catalog_rest::AuthRequestBody<'a> +pub fn iceberg_catalog_rest::AuthRequestBody<'a>::clone(&self) -> iceberg_catalog_rest::AuthRequestBody<'a> +impl<'a> core::cmp::Eq for iceberg_catalog_rest::AuthRequestBody<'a> +impl<'a> core::cmp::PartialEq for iceberg_catalog_rest::AuthRequestBody<'a> +pub fn iceberg_catalog_rest::AuthRequestBody<'a>::eq(&self, other: &iceberg_catalog_rest::AuthRequestBody<'a>) -> bool +impl<'a> core::fmt::Debug for iceberg_catalog_rest::AuthRequestBody<'a> +pub fn iceberg_catalog_rest::AuthRequestBody<'a>::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl<'a> core::marker::Copy for iceberg_catalog_rest::AuthRequestBody<'a> +impl<'a> core::marker::StructuralPartialEq for iceberg_catalog_rest::AuthRequestBody<'a> pub struct iceberg_catalog_rest::AuthRequest<'a> impl<'a> iceberg_catalog_rest::AuthRequest<'a> -pub fn iceberg_catalog_rest::AuthRequest<'a>::body(&self) -> core::option::Option<&[u8]> +pub fn iceberg_catalog_rest::AuthRequest<'a>::body(&self) -> iceberg_catalog_rest::AuthRequestBody<'_> pub fn iceberg_catalog_rest::AuthRequest<'a>::headers(&self) -> &http::header::map::HeaderMap pub fn iceberg_catalog_rest::AuthRequest<'a>::headers_mut(&mut self) -> &mut http::header::map::HeaderMap pub fn iceberg_catalog_rest::AuthRequest<'a>::method(&self) -> &http::method::Method +pub fn iceberg_catalog_rest::AuthRequest<'a>::new(inner: &'a mut reqwest::async_impl::request::Request) -> Self pub fn iceberg_catalog_rest::AuthRequest<'a>::url_str(&self) -> &str pub struct iceberg_catalog_rest::CommitTableRequest pub iceberg_catalog_rest::CommitTableRequest::identifier: core::option::Option @@ -187,7 +203,7 @@ impl core::fmt::Debug for iceberg_catalog_rest::NoopAuthManager pub fn iceberg_catalog_rest::NoopAuthManager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg_catalog_rest::OAuth2Manager impl iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::new(token_endpoint: impl core::convert::Into) -> Self @@ -200,7 +216,7 @@ impl core::fmt::Debug for iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg_catalog_rest::RegisterTableRequest pub iceberg_catalog_rest::RegisterTableRequest::metadata_location: alloc::string::String pub iceberg_catalog_rest::RegisterTableRequest::name: alloc::string::String @@ -322,13 +338,13 @@ pub const iceberg_catalog_rest::REST_CATALOG_PROP_URI: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_WAREHOUSE: &str pub trait iceberg_catalog_rest::AuthManager: core::fmt::Debug + core::marker::Send + core::marker::Sync pub fn iceberg_catalog_rest::AuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::AuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::AuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub trait iceberg_catalog_rest::AuthSession: core::fmt::Debug + core::marker::Send + core::marker::Sync pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::AuthRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub fn iceberg_catalog_rest::AuthSession::invalidate<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index 39c801d268..fd2e9c7acb 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -44,13 +44,17 @@ pub const AUTH_TYPE_OAUTH2: &str = "oauth2"; pub trait AuthManager: Debug + Send + Sync { /// Session used for the initial `/v1/config` handshake, built from the /// user-supplied configuration. - async fn init_session(&self) -> Result>; + /// + /// Returns a [`Box`]: an init session is used once and released, unlike + /// the shared [`AuthManager::catalog_session`]. + async fn init_session(&self) -> Result>; /// Session used for all subsequent catalog requests, given the properties /// merged from the user configuration and the server's config response. /// - /// Implementations may carry state (e.g. a cached token) over from the - /// init session. + /// Returns an [`Arc`]: this session is shared by concurrent requests for + /// the rest of the catalog's lifetime. Implementations may carry state + /// (e.g. a cached token) over from the init session. async fn catalog_session( &self, props: &HashMap, @@ -67,7 +71,8 @@ pub struct AuthRequest<'a> { } impl<'a> AuthRequest<'a> { - pub(crate) fn new(inner: &'a mut Request) -> Self { + /// Wraps a request, e.g. to unit-test a custom [`AuthSession`]. + pub fn new(inner: &'a mut Request) -> Self { Self { inner } } @@ -91,9 +96,40 @@ impl<'a> AuthRequest<'a> { self.inner.headers_mut() } - /// 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()) + /// The request body, distinguishing an absent body from a streaming one: + /// signers can sign [`AuthRequestBody::Empty`] (empty-payload hash) and + /// [`AuthRequestBody::Buffered`], but not [`AuthRequestBody::Streaming`]. + pub fn body(&self) -> AuthRequestBody<'_> { + match self.inner.body() { + None => AuthRequestBody::Empty, + Some(body) => match body.as_bytes() { + Some(bytes) => AuthRequestBody::Buffered(bytes), + None => AuthRequestBody::Streaming, + }, + } + } +} + +/// The body of an [`AuthRequest`], as seen by authentication. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AuthRequestBody<'a> { + /// No body is set. + Empty, + /// An in-memory body. + Buffered(&'a [u8]), + /// A streaming body, whose bytes are not available for e.g. signing. + Streaming, +} + +impl<'a> AuthRequestBody<'a> { + /// The signable bytes: empty for [`Self::Empty`], the buffer for + /// [`Self::Buffered`], and `None` for [`Self::Streaming`]. + pub fn as_bytes(&self) -> Option<&'a [u8]> { + match self { + AuthRequestBody::Empty => Some(&[]), + AuthRequestBody::Buffered(bytes) => Some(bytes), + AuthRequestBody::Streaming => None, + } } } @@ -124,6 +160,32 @@ pub trait AuthSession: Debug + Send + Sync { } } +/// A secret string: `Debug` prints `[REDACTED]` and the memory is zeroized on +/// drop. String-shaped counterpart of the core crate's `SensitiveBytes`; note +/// that copies formatted into requests (form bodies, header values) are owned +/// by the HTTP stack and outlive this wrapper. +#[derive(Clone)] +pub(crate) struct SensitiveString(zeroize::Zeroizing); + +impl SensitiveString { + /// The wrapped secret. + pub(crate) fn expose(&self) -> &str { + &self.0 + } +} + +impl From for SensitiveString { + fn from(secret: String) -> Self { + Self(zeroize::Zeroizing::new(secret)) + } +} + +impl Debug for SensitiveString { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("[REDACTED]") + } +} + /// [`AuthManager`] that performs no authentication. #[derive(Debug)] pub struct NoopAuthManager; @@ -134,8 +196,8 @@ struct NoopSession; #[async_trait] impl AuthManager for NoopAuthManager { - async fn init_session(&self) -> Result> { - Ok(Arc::new(NoopSession)) + async fn init_session(&self) -> Result> { + Ok(Box::new(NoopSession)) } async fn catalog_session( @@ -152,3 +214,100 @@ impl AuthSession for NoopSession { Ok(()) } } + +#[cfg(test)] +mod tests { + use reqwest::Client; + + use super::*; + + #[tokio::test] + async fn test_static_token_session_lifecycle() { + // Token-only config: attach as-is; refresh keeps erroring (no + // credential to exchange); after invalidate, no auth is sent. + let manager = OAuth2Manager::new("http://localhost/unused").with_token("tok-static"); + let session = manager.init_session().await.unwrap(); + let client = Client::new(); + + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut AuthRequest::new(&mut req)) + .await + .unwrap(); + assert_eq!( + req.headers().get("authorization").unwrap(), + "Bearer tok-static" + ); + + assert!(session.refresh().await.is_err()); + + session.invalidate().await.unwrap(); + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut AuthRequest::new(&mut req)) + .await + .unwrap(); + assert!(req.headers().get("authorization").is_none()); + } + + #[test] + fn test_sensitive_string_redacts_debug() { + let secret = SensitiveString::from("s3cret-value".to_string()); + assert_eq!(format!("{secret:?}"), "[REDACTED]"); + + // Containers can safely derive Debug around it. + #[derive(Debug)] + #[allow(dead_code)] + struct Holder { + secret: SensitiveString, + } + let rendered = format!("{:?}", Holder { secret }); + assert!(!rendered.contains("s3cret-value"), "leaked: {rendered}"); + assert!(rendered.contains("[REDACTED]")); + } + + #[test] + fn test_auth_request_body_states() { + let client = Client::new(); + + // No body at all. + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + let auth_req = AuthRequest::new(&mut req); + let body = auth_req.body(); + assert_eq!(body, AuthRequestBody::Empty); + assert_eq!(body.as_bytes(), Some(&[] as &[u8])); + + // An in-memory body. + let mut req = client + .post("https://rest.example.com/v1/namespaces") + .body("{}") + .build() + .unwrap(); + let auth_req = AuthRequest::new(&mut req); + let body = auth_req.body(); + assert_eq!(body, AuthRequestBody::Buffered(b"{}")); + assert_eq!(body.as_bytes(), Some(b"{}" as &[u8])); + + // A streaming body: bytes are unavailable, so it must not sign as empty. + let mut req = client + .post("https://rest.example.com/v1/namespaces") + .body(reqwest::Body::wrap_stream(futures::stream::once(async { + Ok::<_, std::io::Error>(bytes::Bytes::from_static(b"chunk")) + }))) + .build() + .unwrap(); + let auth_req = AuthRequest::new(&mut req); + let body = auth_req.body(); + assert_eq!(body, AuthRequestBody::Streaming); + assert_eq!(body.as_bytes(), None); + } +} diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs index 03100c4b24..4281ee3b62 100644 --- a/crates/catalog/rest/src/auth/oauth2.rs +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -26,7 +26,7 @@ use reqwest::header::HeaderMap; use reqwest::{Client, Method}; use tokio::sync::Mutex; -use super::{AuthManager, AuthRequest, AuthSession}; +use super::{AuthManager, AuthRequest, AuthSession, SensitiveString}; use crate::catalog::{ REST_CATALOG_PROP_URI, RestCatalogConfig, credential_from_props, default_token_endpoint, explicit_headers_from_props, @@ -38,7 +38,7 @@ use crate::types::{ErrorResponse, TokenResponse}; struct OAuth2Params { extra_headers: HeaderMap, token_endpoint: String, - credential: Option<(Option, String)>, + credential: Option<(Option, SensitiveString)>, extra_oauth_params: HashMap, } @@ -50,7 +50,7 @@ struct OAuth2Params { /// across sessions so it survives the config handshake. pub struct OAuth2Manager { client: Client, - token: Arc>>, + token: Arc>>, init_params: OAuth2Params, /// True when the token endpoint was derived from the catalog URI (not /// explicitly configured): it is then recomputed from the merged URI in @@ -83,13 +83,13 @@ impl OAuth2Manager { /// Sets a bearer token used directly (takes precedence over `credential`). pub fn with_token(mut self, token: impl Into) -> Self { - self.token = Arc::new(Mutex::new(Some(token.into()))); + self.token = Arc::new(Mutex::new(Some(SensitiveString::from(token.into())))); self } /// Sets the client credential exchanged for a token at the token endpoint. pub fn with_credential(mut self, client_id: Option, client_secret: String) -> Self { - self.init_params.credential = Some((client_id, client_secret)); + self.init_params.credential = Some((client_id, client_secret.into())); self } @@ -116,11 +116,11 @@ impl OAuth2Manager { pub(crate) fn from_config(cfg: &RestCatalogConfig) -> Result { Ok(Self { client: cfg.client(), - token: Arc::new(Mutex::new(cfg.token())), + token: Arc::new(Mutex::new(cfg.token().map(SensitiveString::from))), init_params: OAuth2Params { extra_headers: cfg.extra_headers()?, token_endpoint: cfg.get_token_endpoint(), - credential: cfg.credential(), + credential: cfg.credential().map(|(id, secret)| (id, secret.into())), extra_oauth_params: cfg.extra_oauth_params(), }, endpoint_is_default: cfg.explicit_oauth2_server_uri().is_none(), @@ -138,12 +138,8 @@ impl Debug for OAuth2Manager { #[async_trait] impl AuthManager for OAuth2Manager { - async fn init_session(&self) -> Result> { - Ok(Arc::new(OAuth2Session { - client: self.client.clone(), - token: self.token.clone(), - params: self.init_params.clone(), - })) + async fn init_session(&self) -> Result> { + Ok(self.build_session(self.init_params.clone())) } async fn catalog_session( @@ -152,7 +148,7 @@ impl AuthManager for OAuth2Manager { ) -> Result> { // The server config may carry a new token (or restate the user's). if let Some(token) = props.get("token") { - *self.token.lock().await = Some(token.clone()); + *self.token.lock().await = Some(SensitiveString::from(token.clone())); } // Explicit property overrides merge ONTO the manager's options, so an @@ -178,63 +174,136 @@ impl AuthManager for OAuth2Manager { _ => self.init_params.token_endpoint.clone(), }; - Ok(Arc::new(OAuth2Session { - client: self.client.clone(), - token: self.token.clone(), - params: OAuth2Params { + Ok(Arc::from( + self.build_session(OAuth2Params { extra_headers, token_endpoint, credential: credential_from_props(props) + .map(|(id, secret)| (id, secret.into())) .or_else(|| self.init_params.credential.clone()), extra_oauth_params, - }, - })) + }), + )) } } -/// [`AuthSession`] adding a `Authorization: Bearer ` header. -struct OAuth2Session { +impl OAuth2Manager { + /// Builds the session matching the configured mode: + /// + /// - a `credential` yields a [`ClientCredentialsSession`] (its token cache + /// pre-seeded when a `token` is also set, and the token then takes + /// precedence until invalidated); + /// - otherwise a [`StaticTokenSession`], which attaches the configured + /// token as-is — or nothing when none is set. + /// + /// Both share the manager's token cell, so a cached token survives the + /// config handshake and `invalidate` is observed by later sessions. + fn build_session(&self, params: OAuth2Params) -> Box { + match params.credential { + Some(credential) => Box::new(ClientCredentialsSession { + client: self.client.clone(), + token: self.token.clone(), + credential, + token_endpoint: params.token_endpoint, + extra_headers: params.extra_headers, + extra_oauth_params: params.extra_oauth_params, + }), + None => Box::new(StaticTokenSession { + token: self.token.clone(), + }), + } + } +} + +/// Attaches `token` as a `Authorization: Bearer ` header, marked +/// sensitive so `Debug`-formatted requests redact it. +fn attach_bearer(req: &mut AuthRequest<'_>, token: &SensitiveString) -> Result<()> { + let mut value: http::HeaderValue = + format!("Bearer {}", token.expose()).parse().map_err(|e| { + Error::new( + ErrorKind::DataInvalid, + "Invalid token received from catalog server!", + ) + .with_source(e) + })?; + value.set_sensitive(true); + req.headers_mut().insert(http::header::AUTHORIZATION, value); + Ok(()) +} + +/// [`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 { + /// Shared with the owning [`OAuth2Manager`]. + token: Arc>>, +} + +#[async_trait] +impl AuthSession for StaticTokenSession { + async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { + // After `invalidate` there is nothing to fall back to: no auth is sent. + match self.token.lock().await.clone() { + Some(token) => attach_bearer(req, &token), + None => Ok(()), + } + } + + async fn invalidate(&self) -> Result<()> { + *self.token.lock().await = None; + Ok(()) + } + + async fn refresh(&self) -> Result<()> { + Err(Error::new( + ErrorKind::DataInvalid, + "Credential must be provided for authentication", + )) + } +} + +/// [`AuthSession`] implementing the OAuth2 client-credentials flow: exchanges +/// the credential for a token at the token endpoint and caches it. +/// +/// # TODO: Support automatic token refreshing. +struct ClientCredentialsSession { client: Client, /// Cached bearer token, shared with the owning [`OAuth2Manager`]. - token: Arc>>, - params: OAuth2Params, + token: Arc>>, + credential: (Option, SensitiveString), + token_endpoint: String, + extra_headers: HeaderMap, + extra_oauth_params: HashMap, } -impl Debug for OAuth2Session { +impl Debug for ClientCredentialsSession { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.debug_struct("OAuth2Session") - .field("token_endpoint", &self.params.token_endpoint) + f.debug_struct("ClientCredentialsSession") + .field("token_endpoint", &self.token_endpoint) .finish_non_exhaustive() } } -impl OAuth2Session { +impl ClientCredentialsSession { async fn exchange_credential_for_token(&self) -> Result { - // Credential must exist here. - let (client_id, client_secret) = self.params.credential.as_ref().ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - "Credential must be provided for authentication", - ) - })?; + let (client_id, client_secret) = &self.credential; let mut params = HashMap::with_capacity(4); params.insert("grant_type", "client_credentials"); if let Some(client_id) = client_id { params.insert("client_id", client_id); } - params.insert("client_secret", client_secret); + params.insert("client_secret", client_secret.expose()); params.extend( - self.params - .extra_oauth_params + self.extra_oauth_params .iter() .map(|(k, v)| (k.as_str(), v.as_str())), ); let mut auth_req = self .client - .request(Method::POST, &self.params.token_endpoint) - .headers(self.params.extra_headers.clone()) + .request(Method::POST, &self.token_endpoint) + .headers(self.extra_headers.clone()) .form(¶ms) .build()?; // extra headers add content-type application/json header it's necessary to override it with proper type @@ -282,51 +351,27 @@ impl OAuth2Session { } #[async_trait] -impl AuthSession for OAuth2Session { - /// Adds a bearer token to the authorization header. - /// - /// Three modes: - /// - /// 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. - /// - /// When both `credential` and `token` are present, `token` takes precedence. - /// - /// # TODO: Support automatic token refreshing. +impl AuthSession for ClientCredentialsSession { + /// Uses the cached token when present (a configured `token` takes + /// precedence over the credential until invalidated); otherwise exchanges + /// the credential for a token, caches it, then uses it. async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { - // Clone the token from lock without holding the lock for entire function. - let token = self.token.lock().await.clone(); - - if self.params.credential.is_none() && token.is_none() { - return Ok(()); - } - - // Either use the provided token or exchange credential for token, cache and use that - let token = match token { - Some(token) => token, - None => { - let token = self.exchange_credential_for_token().await?; - // Update token so that we use it for next request instead of - // exchanging credential for token from the server again - *self.token.lock().await = Some(token.clone()); - token + // The lock is held across the exchange: waiters reuse a successful + // result, and retry themselves after a failure. + let token = { + let mut token = self.token.lock().await; + match &*token { + Some(token) => token.clone(), + None => { + let new_token = + SensitiveString::from(self.exchange_credential_for_token().await?); + *token = Some(new_token.clone()); + new_token + } } }; - // Insert token in request. - req.headers_mut().insert( - http::header::AUTHORIZATION, - format!("Bearer {token}").parse().map_err(|e| { - Error::new( - ErrorKind::DataInvalid, - "Invalid token received from catalog server!", - ) - .with_source(e) - })?, - ); - - Ok(()) + attach_bearer(req, &token) } /// Invalidate the current token without generating a new one. On the next @@ -344,7 +389,7 @@ impl AuthSession for OAuth2Session { /// an error and leave the current token unchanged. async fn refresh(&self) -> Result<()> { let new_token = self.exchange_credential_for_token().await?; - *self.token.lock().await = Some(new_token); + *self.token.lock().await = Some(new_token.into()); Ok(()) } } diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index fb0ed72bb7..9495fc4b4f 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -53,7 +53,8 @@ use crate::types::{ pub const REST_CATALOG_PROP_URI: &str = "uri"; /// REST catalog warehouse location pub const REST_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; -/// Disable header redaction in error logs (defaults to false for security) +/// Disable header redaction in error logs and `Debug` output (defaults to +/// false for security) pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str = "disable-header-redaction"; /// Authentication scheme: `none` or `oauth2` (default). pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type"; @@ -175,7 +176,7 @@ impl RestCatalogBuilder { } /// Rest catalog configuration. -#[derive(Clone, Debug, TypedBuilder)] +#[derive(Clone, TypedBuilder)] pub(crate) struct RestCatalogConfig { #[builder(default, setter(strip_option))] name: Option, @@ -201,6 +202,44 @@ pub(crate) struct RestCatalogConfig { auth_manager: Option>, } +/// Property keys whose values are secrets, or may embed them (headers, +/// connection strings, keys like `adls.account-key` or `s3.sse.key`). +fn is_sensitive_prop(key: &str) -> bool { + key.contains("token") + || key.contains("credential") + || key.contains("secret") + || key.contains("password") + || key.contains("key") + || key.contains("connection-string") + || key.starts_with("header.") +} + +/// Redacts secret property values: this config is printed by +/// [`RestCatalog`]'s derived `Debug`. +impl std::fmt::Debug for RestCatalogConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let props: HashMap<&str, &str> = self + .props + .iter() + .map(|(key, value)| { + let value = if is_sensitive_prop(key) { + "[REDACTED]" + } else { + value.as_str() + }; + (key.as_str(), value) + }) + .collect(); + f.debug_struct("RestCatalogConfig") + .field("name", &self.name) + .field("uri", &self.uri) + .field("warehouse", &self.warehouse) + .field("props", &props) + .field("auth_manager", &self.auth_manager) + .finish_non_exhaustive() + } +} + impl RestCatalogConfig { fn url_prefixed(&self, parts: &[&str]) -> String { [&self.uri, PATH_V1] @@ -286,7 +325,7 @@ impl RestCatalogConfig { } /// The properties handed to [`AuthManager::catalog_session`], with the - /// resolved token endpoint made explicit. + /// resolved catalog `uri` made explicit. pub(crate) fn auth_props(&self) -> HashMap { // `oauth2-server-uri` stays absent unless explicitly configured, so an // injected manager keeps its own endpoint. The resolved `uri` IS passed @@ -423,7 +462,8 @@ pub(crate) fn explicit_headers_from_props(props: &HashMap) -> Re HeaderValue::from_str(value).map_err(|e| { Error::new( ErrorKind::DataInvalid, - format!("Invalid header value: {value}"), + // The value itself is omitted: it may be a secret. + format!("Invalid value for header: {key}"), ) .with_source(e) })?, @@ -645,6 +685,9 @@ impl RestCatalog { /// Invalidate the current token without generating a new one. On the next request, the client /// will attempt to generate a new token. + /// + /// Sessions that don't manage a token (e.g. with `rest.auth.type` `none`, + /// or a custom [`AuthManager`]) may treat this as a no-op. pub async fn invalidate_token(&self) -> Result<()> { self.context().await?.client.session().invalidate().await } @@ -655,6 +698,9 @@ impl RestCatalog { /// /// If credential is invalid, or the request fails, this method will return an error and leave /// the current token unchanged. + /// + /// Errors when the session has no credential to exchange (e.g. a static + /// token); a custom [`AuthManager`]'s session may treat this as a no-op. pub async fn regenerate_token(&self) -> Result<()> { self.context().await?.client.session().refresh().await } @@ -1980,6 +2026,92 @@ mod tests { assert_eq!(token, Some("tok-new".to_string())); } + #[tokio::test] + async fn test_concurrent_authenticate_single_token_exchange() { + // Concurrent requests that all find no cached token must trigger ONE + // credential exchange (the lock is held across it), not one each. + let mut server = Server::new_async().await; + // create_oauth_mock_with_path expects exactly 1 hit. + let oauth_mock = + create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "tok-once", 200).await; + + let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) + .with_credential(Some("client1".to_string()), "secret1".to_string()); + let session: Arc = + Arc::from(manager.init_session().await.unwrap()); + + let client = Client::new(); + let attempts = (0..8).map(|_| { + let session = session.clone(); + let client = client.clone(); + async move { + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .await + .unwrap(); + req.headers() + .get("authorization") + .unwrap() + .to_str() + .unwrap() + .to_string() + } + }); + let bearers = futures::future::join_all(attempts).await; + + oauth_mock.assert_async().await; + assert!(bearers.iter().all(|b| b == "Bearer tok-once")); + } + + #[tokio::test] + async fn test_seeded_token_with_credential_exchanges_once_after_invalidate() { + // The seeded token is attached without an exchange; after + // invalidate() the credential is exchanged exactly once. + let mut server = Server::new_async().await; + // create_oauth_mock_with_path expects exactly 1 hit. + let oauth_mock = + create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "tok-new", 200).await; + + let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) + .with_token("tok-seed") + .with_credential(Some("client1".to_string()), "secret1".to_string()); + let session = manager.init_session().await.unwrap(); + + let client = Client::new(); + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .await + .unwrap(); + assert_eq!( + req.headers().get("authorization").unwrap(), + "Bearer tok-seed" + ); + + session.invalidate().await.unwrap(); + let mut req = client + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .await + .unwrap(); + assert_eq!( + req.headers().get("authorization").unwrap(), + "Bearer tok-new" + ); + + oauth_mock.assert_async().await; + } + #[tokio::test] async fn test_injected_oauth_manager_keeps_endpoint_and_options() { // An injected OAuth2Manager must keep its own token endpoint, extra @@ -2073,8 +2205,8 @@ mod tests { struct CapturingManager(Arc>>>); #[async_trait] impl AuthManager for CapturingManager { - async fn init_session(&self) -> Result> { - Ok(Arc::new(PlainSession)) + async fn init_session(&self) -> Result> { + Ok(Box::new(PlainSession)) } async fn catalog_session( &self, @@ -2189,8 +2321,8 @@ mod tests { struct GuardManager(Arc); #[async_trait] impl AuthManager for GuardManager { - async fn init_session(&self) -> Result> { - Ok(Arc::new(GuardSession(self.0.clone()))) + async fn init_session(&self) -> Result> { + Ok(Box::new(GuardSession(self.0.clone()))) } async fn catalog_session( &self, @@ -2225,6 +2357,35 @@ mod tests { assert!(dropped.load(Ordering::SeqCst)); } + #[test] + fn test_config_debug_redacts_secrets() { + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([ + ("token".to_string(), "tok-secret".to_string()), + ("credential".to_string(), "id:cred-secret".to_string()), + ("header.authorization".to_string(), "Basic xyz".to_string()), + ("adls.account-key".to_string(), "adls-secret".to_string()), + ("s3.sse.key".to_string(), "sse-secret".to_string()), + ( + "adls.connection-string".to_string(), + "cs-secret".to_string(), + ), + ("warehouse".to_string(), "wh1".to_string()), + ])) + .build(); + + let out = format!("{config:?}"); + assert!(!out.contains("tok-secret")); + assert!(!out.contains("cred-secret")); + assert!(!out.contains("Basic xyz")); + assert!(!out.contains("adls-secret")); + assert!(!out.contains("sse-secret")); + assert!(!out.contains("cs-secret")); + assert!(out.contains("[REDACTED]")); + assert!(out.contains("wh1")); + } + #[test] fn test_unknown_auth_type_is_rejected() { let props = HashMap::from([( @@ -2247,7 +2408,7 @@ mod tests { struct StubAuthManager; #[async_trait] impl AuthManager for StubAuthManager { - async fn init_session(&self) -> Result> { + async fn init_session(&self) -> Result> { unimplemented!() } async fn catalog_session( diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index 2dc26cdb60..9c84536f28 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -42,9 +42,14 @@ pub(crate) struct HttpClient { impl Debug for HttpClient { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // The inner client is omitted: an injected one may carry secrets in + // its default headers. f.debug_struct("HttpClient") - .field("client", &self.client) - .field("extra_headers", &self.extra_headers) + .field( + // `header.*` values may hold secrets (e.g. `authorization`). + "extra_headers", + &format_headers_redacted(&self.extra_headers, self.disable_header_redaction), + ) .finish_non_exhaustive() } } @@ -53,7 +58,7 @@ impl HttpClient { /// Create a new http client. pub async fn new(cfg: &RestCatalogConfig) -> Result { let auth_manager = cfg.resolve_auth_manager()?; - let session = auth_manager.init_session().await?; + let session = Arc::from(auth_manager.init_session().await?); Ok(HttpClient { client: cfg.client(), extra_headers: cfg.extra_headers()?, @@ -170,20 +175,21 @@ pub(crate) async fn deserialize_catalog_response( }) } -/// Headers that contain sensitive information and should be excluded from logs. -const SENSITIVE_HEADERS: &[&str] = &[ - "authorization", - "proxy-authorization", - "set-cookie", - "cookie", - "x-api-key", - "x-auth-token", -]; - -/// Returns true if the header name is considered sensitive. +/// Returns true if the header may carry a secret (matched by substring, so +/// e.g. `x-client-secret` is covered along with `authorization`). fn is_sensitive_header(name: &str) -> bool { let name_lower = name.to_lowercase(); - SENSITIVE_HEADERS.iter().any(|h| name_lower == *h) + [ + "auth", + "token", + "secret", + "key", + "password", + "cookie", + "credential", + ] + .iter() + .any(|pattern| name_lower.contains(pattern)) } /// Redacts sensitive headers and returns a debug-formatted string. @@ -265,6 +271,31 @@ mod tests { assert!(result.contains("abc123")); } + #[tokio::test] + async fn test_http_client_debug_redacts_headers() { + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([ + ("header.authorization".to_string(), "Basic xyz".to_string()), + ( + "header.x-client-secret".to_string(), + "shh-secret".to_string(), + ), + ( + "header.x-client-credential".to_string(), + "cred-value".to_string(), + ), + ])) + .build(); + let client = HttpClient::new(&config).await.unwrap(); + + let out = format!("{client:?}"); + assert!(!out.contains("Basic xyz")); + assert!(!out.contains("shh-secret")); + assert!(!out.contains("cred-value")); + assert!(out.contains("[REDACTED]")); + } + #[test] fn test_format_headers_redacted_filters_sensitive() { let mut headers = HeaderMap::new(); From 61eb02e5beae9bfe8018075829f27159c6a807d7 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Thu, 30 Jul 2026 03:07:34 -0400 Subject: [PATCH 4/9] chore: sort rest catalog dependencies for taplo --- crates/catalog/rest/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 1541fe8200..33ab6a770c 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -36,13 +36,13 @@ http = { workspace = true } iceberg = { workspace = true } itertools = { workspace = true } reqwest = { workspace = true } -zeroize = { workspace = true } serde = { workspace = true } serde_derive = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true } typed-builder = { workspace = true } uuid = { workspace = true, features = ["v4"] } +zeroize = { workspace = true } [dev-dependencies] bytes = { workspace = true } From 61f5b721c8086e87ffd46306c1d859355b5130c0 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Thu, 30 Jul 2026 22:54:31 -0400 Subject: [PATCH 5/9] refactor(rest): address committer review - 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 --- crates/catalog/rest/public-api.txt | 5 - crates/catalog/rest/src/auth/mod.rs | 41 +--- crates/catalog/rest/src/auth/oauth2.rs | 40 +--- crates/catalog/rest/src/catalog.rs | 304 +++++++------------------ crates/catalog/rest/src/client.rs | 1 + 5 files changed, 96 insertions(+), 295 deletions(-) diff --git a/crates/catalog/rest/public-api.txt b/crates/catalog/rest/public-api.txt index e0beecd24a..d39fa416ac 100644 --- a/crates/catalog/rest/public-api.txt +++ b/crates/catalog/rest/public-api.txt @@ -249,9 +249,6 @@ pub fn iceberg_catalog_rest::RenameTableRequest::serialize<__S>(&self, __seriali impl<'de> serde_core::de::Deserialize<'de> for iceberg_catalog_rest::RenameTableRequest pub fn iceberg_catalog_rest::RenameTableRequest::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> pub struct iceberg_catalog_rest::RestCatalog -impl iceberg_catalog_rest::RestCatalog -pub async fn iceberg_catalog_rest::RestCatalog::invalidate_token(&self) -> iceberg::error::Result<()> -pub async fn iceberg_catalog_rest::RestCatalog::regenerate_token(&self) -> iceberg::error::Result<()> impl core::fmt::Debug for iceberg_catalog_rest::RestCatalog pub fn iceberg_catalog_rest::RestCatalog::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg::catalog::Catalog for iceberg_catalog_rest::RestCatalog @@ -347,5 +344,3 @@ pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'asy pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub trait iceberg_catalog_rest::AuthSession: core::fmt::Debug + core::marker::Send + core::marker::Sync pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::AuthRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait -pub fn iceberg_catalog_rest::AuthSession::invalidate<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg_catalog_rest::AuthSession::refresh<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index fd2e9c7acb..9b06818a59 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -138,26 +138,6 @@ impl<'a> AuthRequestBody<'a> { pub trait AuthSession: Debug + Send + Sync { /// Applies authentication to the request (adds headers, signs, ...). async fn authenticate(&self, request: &mut AuthRequest<'_>) -> Result<()>; - - /// Drops any cached credentials so the next request re-authenticates. - /// - /// Backs the existing [`RestCatalog::invalidate_token`] API; not part of - /// the intended extension surface, and the default no-op is usually fine. - /// - /// [`RestCatalog::invalidate_token`]: crate::RestCatalog::invalidate_token - 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. - /// - /// Like [`Self::invalidate`], backs [`RestCatalog::regenerate_token`]. - /// - /// [`RestCatalog::regenerate_token`]: crate::RestCatalog::regenerate_token - async fn refresh(&self) -> Result<()> { - Ok(()) - } } /// A secret string: `Debug` prints `[REDACTED]` and the memory is zeroized on @@ -222,14 +202,12 @@ mod tests { use super::*; #[tokio::test] - async fn test_static_token_session_lifecycle() { - // Token-only config: attach as-is; refresh keeps erroring (no - // credential to exchange); after invalidate, no auth is sent. + async fn test_static_token_session_attaches_token() { + // Token-only config: the token is attached as-is. let manager = OAuth2Manager::new("http://localhost/unused").with_token("tok-static"); let session = manager.init_session().await.unwrap(); - let client = Client::new(); - let mut req = client + let mut req = Client::new() .get("https://rest.example.com/v1/config") .build() .unwrap(); @@ -241,19 +219,6 @@ mod tests { req.headers().get("authorization").unwrap(), "Bearer tok-static" ); - - assert!(session.refresh().await.is_err()); - - session.invalidate().await.unwrap(); - let mut req = client - .get("https://rest.example.com/v1/config") - .build() - .unwrap(); - session - .authenticate(&mut AuthRequest::new(&mut req)) - .await - .unwrap(); - assert!(req.headers().get("authorization").is_none()); } #[test] diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs index 4281ee3b62..45388f3739 100644 --- a/crates/catalog/rest/src/auth/oauth2.rs +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -192,12 +192,12 @@ impl OAuth2Manager { /// /// - a `credential` yields a [`ClientCredentialsSession`] (its token cache /// pre-seeded when a `token` is also set, and the token then takes - /// precedence until invalidated); + /// precedence over the credential); /// - otherwise a [`StaticTokenSession`], which attaches the configured /// token as-is — or nothing when none is set. /// /// Both share the manager's token cell, so a cached token survives the - /// config handshake and `invalidate` is observed by later sessions. + /// config handshake. fn build_session(&self, params: OAuth2Params) -> Box { match params.credential { Some(credential) => Box::new(ClientCredentialsSession { @@ -242,24 +242,11 @@ struct StaticTokenSession { #[async_trait] impl AuthSession for StaticTokenSession { async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { - // After `invalidate` there is nothing to fall back to: no auth is sent. match self.token.lock().await.clone() { Some(token) => attach_bearer(req, &token), None => Ok(()), } } - - async fn invalidate(&self) -> Result<()> { - *self.token.lock().await = None; - Ok(()) - } - - async fn refresh(&self) -> Result<()> { - Err(Error::new( - ErrorKind::DataInvalid, - "Credential must be provided for authentication", - )) - } } /// [`AuthSession`] implementing the OAuth2 client-credentials flow: exchanges @@ -353,8 +340,8 @@ impl ClientCredentialsSession { #[async_trait] impl AuthSession for ClientCredentialsSession { /// Uses the cached token when present (a configured `token` takes - /// precedence over the credential until invalidated); otherwise exchanges - /// the credential for a token, caches it, then uses it. + /// precedence over the credential); otherwise exchanges the credential + /// for a token, caches it, then uses it. async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { // The lock is held across the exchange: waiters reuse a successful // result, and retry themselves after a failure. @@ -373,23 +360,4 @@ impl AuthSession for ClientCredentialsSession { attach_bearer(req, &token) } - - /// Invalidate the current token without generating a new one. On the next - /// request, the session will attempt to generate a new token. - async fn invalidate(&self) -> Result<()> { - *self.token.lock().await = None; - Ok(()) - } - - /// Invalidate the current token and set a new one. Generates a new token - /// before invalidating the current one, meaning the old token will be used - /// until this function acquires the lock and overwrites the token. - /// - /// If credential is invalid, or the request fails, this method will return - /// an error and leave the current token unchanged. - async fn refresh(&self) -> Result<()> { - let new_token = self.exchange_credential_for_token().await?; - *self.token.lock().await = Some(new_token.into()); - Ok(()) - } } diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 9495fc4b4f..7d3207f54a 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -56,7 +56,9 @@ pub const REST_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Disable header redaction in error logs and `Debug` output (defaults to /// false for security) pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str = "disable-header-redaction"; -/// Authentication scheme: `none` or `oauth2` (default). +/// Authentication scheme: `none` or `oauth2`. When unset, `oauth2` is used +/// if a `token`, `credential` or `oauth2-server-uri` is configured, `none` +/// otherwise. pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type"; const ICEBERG_REST_SPEC_VERSION: &str = "0.14.1"; @@ -346,14 +348,24 @@ impl RestCatalogConfig { .unwrap_or(false) } - /// The configured auth scheme: explicit `rest.auth.type` or the default - /// `oauth2` (which behaves as no auth when neither `token` nor - /// `credential` is set). + /// The configured auth scheme: explicit `rest.auth.type` when set; + /// 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 { self.props .get(REST_CATALOG_PROP_AUTH_TYPE) .cloned() - .unwrap_or_else(|| AUTH_TYPE_OAUTH2.to_string()) + .unwrap_or_else(|| { + if self.token().is_some() + || self.credential().is_some() + || self.explicit_oauth2_server_uri().is_some() + { + AUTH_TYPE_OAUTH2.to_string() + } else { + AUTH_TYPE_NONE.to_string() + } + }) } /// Resolves the auth manager: a `with_auth_manager` override wins, @@ -367,7 +379,11 @@ impl RestCatalogConfig { AUTH_TYPE_OAUTH2 => Ok(Arc::new(OAuth2Manager::from_config(self)?)), other => Err(Error::new( ErrorKind::DataInvalid, - format!("unknown '{REST_CATALOG_PROP_AUTH_TYPE}': {other}"), + format!( + "unknown '{REST_CATALOG_PROP_AUTH_TYPE}': {other}; use \ + `RestCatalogBuilder::with_auth_manager` to inject a \ + custom auth manager" + ), )), } } @@ -682,28 +698,6 @@ impl RestCatalog { Ok(file_io) } - - /// Invalidate the current token without generating a new one. On the next request, the client - /// will attempt to generate a new token. - /// - /// Sessions that don't manage a token (e.g. with `rest.auth.type` `none`, - /// or a custom [`AuthManager`]) may treat this as a no-op. - pub async fn invalidate_token(&self) -> Result<()> { - self.context().await?.client.session().invalidate().await - } - - /// Invalidate the current token and set a new one. Generates a new token before invalidating - /// the current token, meaning the old token will be used until this function acquires the lock - /// and overwrites the token. - /// - /// If credential is invalid, or the request fails, this method will return an error and leave - /// the current token unchanged. - /// - /// Errors when the session has no credential to exchange (e.g. a static - /// token); a custom [`AuthManager`]'s session may treat this as a no-op. - pub async fn regenerate_token(&self) -> Result<()> { - self.context().await?.client.session().refresh().await - } } /// All requests and expected responses are derived from the REST catalog API spec: @@ -1581,152 +1575,6 @@ mod tests { assert_eq!(token, Some("ey000000000000".to_string())); } - #[tokio::test] - async fn test_invalidate_token() { - let mut server = Server::new_async().await; - let oauth_mock = create_oauth_mock(&mut server).await; - let config_mock = create_config_mock(&mut server).await; - - let mut props = HashMap::new(); - props.insert("credential".to_string(), "client1:secret1".to_string()); - - let catalog = RestCatalog::new( - RestCatalogConfig::builder() - .uri(server.url()) - .props(props) - .build(), - Some(Arc::new(LocalFsStorageFactory)), - Runtime::current(), - None, - ); - - let token = catalog.context().await.unwrap().client.token().await; - oauth_mock.assert_async().await; - config_mock.assert_async().await; - assert_eq!(token, Some("ey000000000000".to_string())); - - let oauth_mock = - create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 200) - .await; - // The next request re-exchanges the credential and sends the new token. - let ns_mock = server - .mock("GET", "/v1/namespaces") - .match_header("authorization", "Bearer ey000000000001") - .with_body(r#"{"namespaces": []}"#) - .create_async() - .await; - catalog.invalidate_token().await.unwrap(); - catalog.list_namespaces(None).await.unwrap(); - oauth_mock.assert_async().await; - ns_mock.assert_async().await; - let token = catalog.context().await.unwrap().client.token().await; - assert_eq!(token, Some("ey000000000001".to_string())); - } - - #[tokio::test] - async fn test_invalidate_token_failing_request() { - let mut server = Server::new_async().await; - let oauth_mock = create_oauth_mock(&mut server).await; - let config_mock = create_config_mock(&mut server).await; - - let mut props = HashMap::new(); - props.insert("credential".to_string(), "client1:secret1".to_string()); - - let catalog = RestCatalog::new( - RestCatalogConfig::builder() - .uri(server.url()) - .props(props) - .build(), - Some(Arc::new(LocalFsStorageFactory)), - Runtime::current(), - None, - ); - - let token = catalog.context().await.unwrap().client.token().await; - oauth_mock.assert_async().await; - config_mock.assert_async().await; - assert_eq!(token, Some("ey000000000000".to_string())); - - let oauth_mock = - create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 500) - .await; - catalog.invalidate_token().await.unwrap(); - // The failed re-exchange surfaces as an error and no token is cached. - assert!(catalog.list_namespaces(None).await.is_err()); - oauth_mock.assert_async().await; - let token = catalog.context().await.unwrap().client.token().await; - assert_eq!(token, None); - } - - #[tokio::test] - async fn test_regenerate_token() { - let mut server = Server::new_async().await; - let oauth_mock = create_oauth_mock(&mut server).await; - let config_mock = create_config_mock(&mut server).await; - - let mut props = HashMap::new(); - props.insert("credential".to_string(), "client1:secret1".to_string()); - - let catalog = RestCatalog::new( - RestCatalogConfig::builder() - .uri(server.url()) - .props(props) - .build(), - Some(Arc::new(LocalFsStorageFactory)), - Runtime::current(), - None, - ); - - let token = catalog.context().await.unwrap().client.token().await; - oauth_mock.assert_async().await; - config_mock.assert_async().await; - assert_eq!(token, Some("ey000000000000".to_string())); - - let oauth_mock = - create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 200) - .await; - catalog.regenerate_token().await.unwrap(); - oauth_mock.assert_async().await; - let token = catalog.context().await.unwrap().client.token().await; - assert_eq!(token, Some("ey000000000001".to_string())); - } - - #[tokio::test] - async fn test_regenerate_token_failing_request() { - let mut server = Server::new_async().await; - let oauth_mock = create_oauth_mock(&mut server).await; - let config_mock = create_config_mock(&mut server).await; - - let mut props = HashMap::new(); - props.insert("credential".to_string(), "client1:secret1".to_string()); - - let catalog = RestCatalog::new( - RestCatalogConfig::builder() - .uri(server.url()) - .props(props) - .build(), - Some(Arc::new(LocalFsStorageFactory)), - Runtime::current(), - None, - ); - - let token = catalog.context().await.unwrap().client.token().await; - oauth_mock.assert_async().await; - config_mock.assert_async().await; - assert_eq!(token, Some("ey000000000000".to_string())); - - let oauth_mock = - create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "ey000000000001", 500) - .await; - let invalidate_result = catalog.regenerate_token().await; - assert!(invalidate_result.is_err()); - oauth_mock.assert_async().await; - let token = catalog.context().await.unwrap().client.token().await; - - // original token is left intact - assert_eq!(token, Some("ey000000000000".to_string())); - } - #[tokio::test] async fn test_http_headers() { let server = Server::new_async().await; @@ -1985,9 +1833,9 @@ mod tests { async fn test_builtin_oauth_endpoint_follows_uri_override() { // When `/v1/config` overrides `uri` (and no explicit `oauth2-server-uri` // is set), the built-in manager's default token endpoint must follow - // the merged URI: a refresh after the handshake posts to the new host. + // the merged URI. let mut bootstrap = Server::new_async().await; - let mut overridden = Server::new_async().await; + let overridden = Server::new_async().await; let config_mock = bootstrap .mock("GET", "/v1/config") @@ -2001,9 +1849,6 @@ mod tests { // Handshake exchange still uses the bootstrap-derived default. let bootstrap_oauth_mock = create_oauth_mock_with_path(&mut bootstrap, "/v1/oauth/tokens", "tok-boot", 200).await; - // The refresh must follow the overridden URI. - let overridden_oauth_mock = - create_oauth_mock_with_path(&mut overridden, "/v1/oauth/tokens", "tok-new", 200).await; let props = HashMap::from([("credential".to_string(), "client1:secret1".to_string())]); let catalog = RestCatalog::new( @@ -2016,14 +1861,13 @@ mod tests { None, ); - catalog.context().await.unwrap(); - catalog.regenerate_token().await.unwrap(); - + let context = catalog.context().await.unwrap(); config_mock.assert_async().await; bootstrap_oauth_mock.assert_async().await; - overridden_oauth_mock.assert_async().await; - let token = catalog.context().await.unwrap().client.token().await; - assert_eq!(token, Some("tok-new".to_string())); + // The catalog session's endpoint follows the overridden URI (visible + // via the session's Debug, which prints its token endpoint). + let session_debug = format!("{:?}", context.client.session()); + assert!(session_debug.contains(&format!("{}/v1/oauth/tokens", overridden.url()))); } #[tokio::test] @@ -2068,21 +1912,22 @@ mod tests { } #[tokio::test] - async fn test_seeded_token_with_credential_exchanges_once_after_invalidate() { - // The seeded token is attached without an exchange; after - // invalidate() the credential is exchanged exactly once. + async fn test_seeded_token_takes_precedence_over_credential() { + // token + credential: the seeded token is attached without any + // credential exchange. let mut server = Server::new_async().await; - // create_oauth_mock_with_path expects exactly 1 hit. - let oauth_mock = - create_oauth_mock_with_path(&mut server, "/v1/oauth/tokens", "tok-new", 200).await; + let oauth_mock = server + .mock("POST", "/v1/oauth/tokens") + .expect(0) + .create_async() + .await; let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) .with_token("tok-seed") .with_credential(Some("client1".to_string()), "secret1".to_string()); let session = manager.init_session().await.unwrap(); - let client = Client::new(); - let mut req = client + let mut req = Client::new() .get("https://rest.example.com/v1/config") .build() .unwrap(); @@ -2095,20 +1940,6 @@ mod tests { "Bearer tok-seed" ); - session.invalidate().await.unwrap(); - let mut req = client - .get("https://rest.example.com/v1/config") - .build() - .unwrap(); - session - .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) - .await - .unwrap(); - assert_eq!( - req.headers().get("authorization").unwrap(), - "Bearer tok-new" - ); - oauth_mock.assert_async().await; } @@ -2118,7 +1949,14 @@ mod tests { // headers and OAuth params across the config handshake: only explicit // properties may override them, never synthesized defaults. let mut server = Server::new_async().await; - let config_mock = create_config_mock(&mut server).await; + // The server vends the credential, so the exchange runs through the + // post-handshake catalog session (exercising its property merging). + let config_mock = server + .mock("GET", "/v1/config") + .with_status(200) + .with_body(r#"{"defaults": {"credential": "client1:secret1"}, "overrides": {}}"#) + .create_async() + .await; // The catalog-host default endpoint must never see the credential. let default_endpoint_mock = server @@ -2126,8 +1964,8 @@ mod tests { .expect(0) .create_async() .await; - // Both exchanges (handshake + regenerate) hit the injected endpoint, - // carrying the injected header and OAuth param. + // The exchange hits the injected endpoint, carrying the injected + // header and OAuth param. let custom_endpoint_mock = server .mock("POST", "/custom/oauth/tokens") .match_header("x-tenant", "t1") @@ -2146,12 +1984,16 @@ mod tests { "expires_in": 86400 }"#, ) - .expect(2) + .create_async() + .await; + let ns_mock = server + .mock("GET", "/v1/namespaces") + .match_header("authorization", "Bearer ey000000000000") + .with_body(r#"{"namespaces": []}"#) .create_async() .await; let manager = OAuth2Manager::new(format!("{}/custom/oauth/tokens", server.url())) - .with_credential(Some("client1".to_string()), "secret1".to_string()) .with_extra_headers(HeaderMap::from_iter([( HeaderName::from_static("x-tenant"), HeaderValue::from_static("t1"), @@ -2170,14 +2012,12 @@ mod tests { None, ); - // Handshake performs the first exchange; regenerate the second — both - // must use the injected endpoint/options. - catalog.context().await.unwrap(); - catalog.regenerate_token().await.unwrap(); + catalog.list_namespaces(None).await.unwrap(); config_mock.assert_async().await; custom_endpoint_mock.assert_async().await; default_endpoint_mock.assert_async().await; + ns_mock.assert_async().await; } #[tokio::test] @@ -2386,6 +2226,38 @@ mod tests { assert!(out.contains("wh1")); } + #[test] + fn test_auth_type_defaults() { + // Unset `rest.auth.type`: `oauth2` when any OAuth material is + // configured (existing setups keep working), `none` otherwise. + let bare = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .build(); + assert!(format!("{:?}", bare.resolve_auth_manager().unwrap()).contains("NoopAuthManager")); + + let with_token = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([("token".to_string(), "tok".to_string())])) + .build(); + assert!( + format!("{:?}", with_token.resolve_auth_manager().unwrap()).contains("OAuth2Manager") + ); + + // An explicit OAuth endpoint is oauth2 intent too: the manager can + // still pick up a server-supplied token from `/v1/config`. + let with_endpoint = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([( + "oauth2-server-uri".to_string(), + "http://auth.example.com/tokens".to_string(), + )])) + .build(); + assert!( + format!("{:?}", with_endpoint.resolve_auth_manager().unwrap()) + .contains("OAuth2Manager") + ); + } + #[test] fn test_unknown_auth_type_is_rejected() { let props = HashMap::from([( diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index 9c84536f28..856ee5c6c9 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -103,6 +103,7 @@ impl HttpClient { } /// The session authenticating requests in the current phase. + #[cfg(test)] pub(crate) fn session(&self) -> &Arc { &self.session } From 57dc2cf9cd6fc2743e6893e01deaaea976ce73a9 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 31 Jul 2026 00:31:23 -0400 Subject: [PATCH 6/9] feat(rest): warn when inferring oauth2 from legacy auth properties --- Cargo.lock | 1 + crates/catalog/rest/Cargo.toml | 1 + crates/catalog/rest/src/catalog.rs | 11 ++++++++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 141b76dcbb..db3231c057 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3904,6 +3904,7 @@ dependencies = [ "serde_derive", "serde_json", "tokio", + "tracing", "typed-builder", "uuid", "zeroize", diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 33ab6a770c..deafb46083 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -40,6 +40,7 @@ serde = { workspace = true } serde_derive = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true } +tracing = { workspace = true } typed-builder = { workspace = true } uuid = { workspace = true, features = ["v4"] } zeroize = { workspace = true } diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 7d3207f54a..7fb60afedd 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -374,7 +374,16 @@ impl RestCatalogConfig { if let Some(auth_manager) = &self.auth_manager { return Ok(auth_manager.clone()); } - match self.auth_type().as_str() { + let auth_type = self.auth_type(); + // Java parity (`AuthManagers`): make the inference visible so users + // configure the type explicitly. + if auth_type == AUTH_TYPE_OAUTH2 && !self.props.contains_key(REST_CATALOG_PROP_AUTH_TYPE) { + tracing::warn!( + "Inferring {REST_CATALOG_PROP_AUTH_TYPE}={AUTH_TYPE_OAUTH2} from the configured \ + OAuth properties; set it explicitly to avoid this warning" + ); + } + match auth_type.as_str() { AUTH_TYPE_NONE => Ok(Arc::new(NoopAuthManager)), AUTH_TYPE_OAUTH2 => Ok(Arc::new(OAuth2Manager::from_config(self)?)), other => Err(Error::new( From 510dacdd6c990a0b298aed2c686a3f47f4030b47 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Wed, 5 Aug 2026 03:34:48 -0400 Subject: [PATCH 7/9] refactor(rest): use iceberg::Credential for OAuth2 secrets Replaces the crate-private SensitiveString with the equivalent iceberg::Credential introduced in #2836. --- Cargo.lock | 1 - crates/catalog/rest/Cargo.toml | 1 - crates/catalog/rest/src/auth/mod.rs | 42 -------------------------- crates/catalog/rest/src/auth/oauth2.rs | 25 ++++++++------- 4 files changed, 12 insertions(+), 57 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db3231c057..f72e5e0458 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3907,7 +3907,6 @@ dependencies = [ "tracing", "typed-builder", "uuid", - "zeroize", ] [[package]] diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index deafb46083..4235067d7b 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -43,7 +43,6 @@ tokio = { workspace = true } tracing = { workspace = true } typed-builder = { workspace = true } uuid = { workspace = true, features = ["v4"] } -zeroize = { workspace = true } [dev-dependencies] bytes = { workspace = true } diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index 9b06818a59..02af436a19 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -140,32 +140,6 @@ pub trait AuthSession: Debug + Send + Sync { async fn authenticate(&self, request: &mut AuthRequest<'_>) -> Result<()>; } -/// A secret string: `Debug` prints `[REDACTED]` and the memory is zeroized on -/// drop. String-shaped counterpart of the core crate's `SensitiveBytes`; note -/// that copies formatted into requests (form bodies, header values) are owned -/// by the HTTP stack and outlive this wrapper. -#[derive(Clone)] -pub(crate) struct SensitiveString(zeroize::Zeroizing); - -impl SensitiveString { - /// The wrapped secret. - pub(crate) fn expose(&self) -> &str { - &self.0 - } -} - -impl From for SensitiveString { - fn from(secret: String) -> Self { - Self(zeroize::Zeroizing::new(secret)) - } -} - -impl Debug for SensitiveString { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str("[REDACTED]") - } -} - /// [`AuthManager`] that performs no authentication. #[derive(Debug)] pub struct NoopAuthManager; @@ -221,22 +195,6 @@ mod tests { ); } - #[test] - fn test_sensitive_string_redacts_debug() { - let secret = SensitiveString::from("s3cret-value".to_string()); - assert_eq!(format!("{secret:?}"), "[REDACTED]"); - - // Containers can safely derive Debug around it. - #[derive(Debug)] - #[allow(dead_code)] - struct Holder { - secret: SensitiveString, - } - let rendered = format!("{:?}", Holder { secret }); - assert!(!rendered.contains("s3cret-value"), "leaked: {rendered}"); - assert!(rendered.contains("[REDACTED]")); - } - #[test] fn test_auth_request_body_states() { let client = Client::new(); diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs index 45388f3739..630b7505e9 100644 --- a/crates/catalog/rest/src/auth/oauth2.rs +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -21,12 +21,12 @@ use std::sync::Arc; use async_trait::async_trait; use http::StatusCode; -use iceberg::{Error, ErrorKind, Result}; +use iceberg::{Credential, Error, ErrorKind, Result}; use reqwest::header::HeaderMap; use reqwest::{Client, Method}; use tokio::sync::Mutex; -use super::{AuthManager, AuthRequest, AuthSession, SensitiveString}; +use super::{AuthManager, AuthRequest, AuthSession}; use crate::catalog::{ REST_CATALOG_PROP_URI, RestCatalogConfig, credential_from_props, default_token_endpoint, explicit_headers_from_props, @@ -38,7 +38,7 @@ use crate::types::{ErrorResponse, TokenResponse}; struct OAuth2Params { extra_headers: HeaderMap, token_endpoint: String, - credential: Option<(Option, SensitiveString)>, + credential: Option<(Option, Credential)>, extra_oauth_params: HashMap, } @@ -50,7 +50,7 @@ struct OAuth2Params { /// across sessions so it survives the config handshake. pub struct OAuth2Manager { client: Client, - token: Arc>>, + token: Arc>>, init_params: OAuth2Params, /// True when the token endpoint was derived from the catalog URI (not /// explicitly configured): it is then recomputed from the merged URI in @@ -83,7 +83,7 @@ impl OAuth2Manager { /// Sets a bearer token used directly (takes precedence over `credential`). pub fn with_token(mut self, token: impl Into) -> Self { - self.token = Arc::new(Mutex::new(Some(SensitiveString::from(token.into())))); + self.token = Arc::new(Mutex::new(Some(Credential::from(token.into())))); self } @@ -116,7 +116,7 @@ impl OAuth2Manager { pub(crate) fn from_config(cfg: &RestCatalogConfig) -> Result { Ok(Self { client: cfg.client(), - token: Arc::new(Mutex::new(cfg.token().map(SensitiveString::from))), + token: Arc::new(Mutex::new(cfg.token().map(Credential::from))), init_params: OAuth2Params { extra_headers: cfg.extra_headers()?, token_endpoint: cfg.get_token_endpoint(), @@ -148,7 +148,7 @@ impl AuthManager for OAuth2Manager { ) -> Result> { // The server config may carry a new token (or restate the user's). if let Some(token) = props.get("token") { - *self.token.lock().await = Some(SensitiveString::from(token.clone())); + *self.token.lock().await = Some(Credential::from(token.clone())); } // Explicit property overrides merge ONTO the manager's options, so an @@ -217,7 +217,7 @@ impl OAuth2Manager { /// Attaches `token` as a `Authorization: Bearer ` header, marked /// sensitive so `Debug`-formatted requests redact it. -fn attach_bearer(req: &mut AuthRequest<'_>, token: &SensitiveString) -> Result<()> { +fn attach_bearer(req: &mut AuthRequest<'_>, token: &Credential) -> Result<()> { let mut value: http::HeaderValue = format!("Bearer {}", token.expose()).parse().map_err(|e| { Error::new( @@ -236,7 +236,7 @@ fn attach_bearer(req: &mut AuthRequest<'_>, token: &SensitiveString) -> Result<( #[derive(Debug)] struct StaticTokenSession { /// Shared with the owning [`OAuth2Manager`]. - token: Arc>>, + token: Arc>>, } #[async_trait] @@ -256,8 +256,8 @@ impl AuthSession for StaticTokenSession { struct ClientCredentialsSession { client: Client, /// Cached bearer token, shared with the owning [`OAuth2Manager`]. - token: Arc>>, - credential: (Option, SensitiveString), + token: Arc>>, + credential: (Option, Credential), token_endpoint: String, extra_headers: HeaderMap, extra_oauth_params: HashMap, @@ -350,8 +350,7 @@ impl AuthSession for ClientCredentialsSession { match &*token { Some(token) => token.clone(), None => { - let new_token = - SensitiveString::from(self.exchange_credential_for_token().await?); + let new_token = Credential::from(self.exchange_credential_for_token().await?); *token = Some(new_token.clone()); new_token } From 450dbfc8d88ae2adde3ca77c63f717204196eb1c Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Sat, 8 Aug 2026 02:55:33 -0400 Subject: [PATCH 8/9] refactor(rest): move auth out of HttpClient, hand managers the client and props --- crates/catalog/rest/Cargo.toml | 2 +- crates/catalog/rest/public-api.txt | 68 ++--- crates/catalog/rest/src/auth/mod.rs | 87 +++--- crates/catalog/rest/src/auth/oauth2.rs | 180 +++++++------ crates/catalog/rest/src/catalog.rs | 359 +++++++++++++++++++++---- crates/catalog/rest/src/client.rs | 65 ++--- 6 files changed, 498 insertions(+), 263 deletions(-) diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 4235067d7b..8dc9a86d7f 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -49,7 +49,7 @@ bytes = { workspace = true } futures = { workspace = true } iceberg_test_utils = { path = "../../test_utils", features = ["tests"] } mockito = { workspace = true } -# `stream` lets tests build a streaming body to exercise AuthRequestBody::Streaming. +# `stream` lets tests build a streaming body to exercise HttpRequestBody::Streaming. reqwest = { workspace = true, features = ["stream"] } tokio = { workspace = true } diff --git a/crates/catalog/rest/public-api.txt b/crates/catalog/rest/public-api.txt index d39fa416ac..739fc9f57f 100644 --- a/crates/catalog/rest/public-api.txt +++ b/crates/catalog/rest/public-api.txt @@ -1,27 +1,19 @@ pub mod iceberg_catalog_rest -pub enum iceberg_catalog_rest::AuthRequestBody<'a> -pub iceberg_catalog_rest::AuthRequestBody::Buffered(&'a [u8]) -pub iceberg_catalog_rest::AuthRequestBody::Empty -pub iceberg_catalog_rest::AuthRequestBody::Streaming -impl<'a> iceberg_catalog_rest::AuthRequestBody<'a> -pub fn iceberg_catalog_rest::AuthRequestBody<'a>::as_bytes(&self) -> core::option::Option<&'a [u8]> -impl<'a> core::clone::Clone for iceberg_catalog_rest::AuthRequestBody<'a> -pub fn iceberg_catalog_rest::AuthRequestBody<'a>::clone(&self) -> iceberg_catalog_rest::AuthRequestBody<'a> -impl<'a> core::cmp::Eq for iceberg_catalog_rest::AuthRequestBody<'a> -impl<'a> core::cmp::PartialEq for iceberg_catalog_rest::AuthRequestBody<'a> -pub fn iceberg_catalog_rest::AuthRequestBody<'a>::eq(&self, other: &iceberg_catalog_rest::AuthRequestBody<'a>) -> bool -impl<'a> core::fmt::Debug for iceberg_catalog_rest::AuthRequestBody<'a> -pub fn iceberg_catalog_rest::AuthRequestBody<'a>::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl<'a> core::marker::Copy for iceberg_catalog_rest::AuthRequestBody<'a> -impl<'a> core::marker::StructuralPartialEq for iceberg_catalog_rest::AuthRequestBody<'a> -pub struct iceberg_catalog_rest::AuthRequest<'a> -impl<'a> iceberg_catalog_rest::AuthRequest<'a> -pub fn iceberg_catalog_rest::AuthRequest<'a>::body(&self) -> iceberg_catalog_rest::AuthRequestBody<'_> -pub fn iceberg_catalog_rest::AuthRequest<'a>::headers(&self) -> &http::header::map::HeaderMap -pub fn iceberg_catalog_rest::AuthRequest<'a>::headers_mut(&mut self) -> &mut http::header::map::HeaderMap -pub fn iceberg_catalog_rest::AuthRequest<'a>::method(&self) -> &http::method::Method -pub fn iceberg_catalog_rest::AuthRequest<'a>::new(inner: &'a mut reqwest::async_impl::request::Request) -> Self -pub fn iceberg_catalog_rest::AuthRequest<'a>::url_str(&self) -> &str +pub enum iceberg_catalog_rest::HttpRequestBody<'a> +pub iceberg_catalog_rest::HttpRequestBody::Buffered(&'a [u8]) +pub iceberg_catalog_rest::HttpRequestBody::Empty +pub iceberg_catalog_rest::HttpRequestBody::Streaming +impl<'a> iceberg_catalog_rest::HttpRequestBody<'a> +pub fn iceberg_catalog_rest::HttpRequestBody<'a>::as_bytes(&self) -> core::option::Option<&'a [u8]> +impl<'a> core::clone::Clone for iceberg_catalog_rest::HttpRequestBody<'a> +pub fn iceberg_catalog_rest::HttpRequestBody<'a>::clone(&self) -> iceberg_catalog_rest::HttpRequestBody<'a> +impl<'a> core::cmp::Eq for iceberg_catalog_rest::HttpRequestBody<'a> +impl<'a> core::cmp::PartialEq for iceberg_catalog_rest::HttpRequestBody<'a> +pub fn iceberg_catalog_rest::HttpRequestBody<'a>::eq(&self, other: &iceberg_catalog_rest::HttpRequestBody<'a>) -> bool +impl<'a> core::fmt::Debug for iceberg_catalog_rest::HttpRequestBody<'a> +pub fn iceberg_catalog_rest::HttpRequestBody<'a>::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl<'a> core::marker::Copy for iceberg_catalog_rest::HttpRequestBody<'a> +impl<'a> core::marker::StructuralPartialEq for iceberg_catalog_rest::HttpRequestBody<'a> pub struct iceberg_catalog_rest::CommitTableRequest pub iceberg_catalog_rest::CommitTableRequest::identifier: core::option::Option pub iceberg_catalog_rest::CommitTableRequest::requirements: alloc::vec::Vec @@ -132,6 +124,14 @@ impl serde_core::ser::Serialize for iceberg_catalog_rest::ErrorResponse pub fn iceberg_catalog_rest::ErrorResponse::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg_catalog_rest::ErrorResponse pub fn iceberg_catalog_rest::ErrorResponse::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> +pub struct iceberg_catalog_rest::HttpRequest<'a> +impl<'a> iceberg_catalog_rest::HttpRequest<'a> +pub fn iceberg_catalog_rest::HttpRequest<'a>::body(&self) -> iceberg_catalog_rest::HttpRequestBody<'_> +pub fn iceberg_catalog_rest::HttpRequest<'a>::headers(&self) -> &http::header::map::HeaderMap +pub fn iceberg_catalog_rest::HttpRequest<'a>::headers_mut(&mut self) -> &mut http::header::map::HeaderMap +pub fn iceberg_catalog_rest::HttpRequest<'a>::method(&self) -> &http::method::Method +pub fn iceberg_catalog_rest::HttpRequest<'a>::new(inner: &'a mut reqwest::async_impl::request::Request) -> Self +pub fn iceberg_catalog_rest::HttpRequest<'a>::url_str(&self) -> &str pub struct iceberg_catalog_rest::ListNamespaceResponse pub iceberg_catalog_rest::ListNamespaceResponse::namespaces: alloc::vec::Vec pub iceberg_catalog_rest::ListNamespaceResponse::next_page_token: core::option::Option @@ -202,8 +202,8 @@ pub struct iceberg_catalog_rest::NoopAuthManager impl core::fmt::Debug for iceberg_catalog_rest::NoopAuthManager pub fn iceberg_catalog_rest::NoopAuthManager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager -pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, _client: &'life1 reqwest::async_impl::client::Client, _props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, _client: &'life1 reqwest::async_impl::client::Client, _props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub struct iceberg_catalog_rest::OAuth2Manager impl iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::new(token_endpoint: impl core::convert::Into) -> Self @@ -215,8 +215,8 @@ pub fn iceberg_catalog_rest::OAuth2Manager::with_token(self, token: impl core::c impl core::fmt::Debug for iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager -pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub struct iceberg_catalog_rest::RegisterTableRequest pub iceberg_catalog_rest::RegisterTableRequest::metadata_location: alloc::string::String pub iceberg_catalog_rest::RegisterTableRequest::name: alloc::string::String @@ -334,13 +334,13 @@ pub const iceberg_catalog_rest::REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_URI: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_WAREHOUSE: &str pub trait iceberg_catalog_rest::AuthManager: core::fmt::Debug + core::marker::Send + core::marker::Sync -pub fn iceberg_catalog_rest::AuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::AuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::AuthManager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::AuthManager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::NoopAuthManager -pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, _props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, _client: &'life1 reqwest::async_impl::client::Client, _props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, _client: &'life1 reqwest::async_impl::client::Client, _props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager -pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'async_trait>(&'life0 self, props: &'life1 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'async_trait>(&'life0 self) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub trait iceberg_catalog_rest::AuthSession: core::fmt::Debug + core::marker::Send + core::marker::Sync -pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::AuthRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::HttpRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index 02af436a19..97d139d5af 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -28,7 +28,7 @@ use async_trait::async_trait; use http::{HeaderMap, Method}; use iceberg::Result; pub use oauth2::OAuth2Manager; -use reqwest::Request; +use reqwest::{Client, Request}; /// `rest.auth.type` value disabling authentication. pub const AUTH_TYPE_NONE: &str = "none"; @@ -38,16 +38,24 @@ pub const AUTH_TYPE_OAUTH2: &str = "oauth2"; /// Creates the [`AuthSession`]s used to authenticate REST catalog requests. /// /// A manager is created once per catalog, either from the `rest.auth.type` -/// property or injected through `RestCatalogBuilder::with_auth_manager`, and -/// lives for the lifetime of the catalog. +/// property or injected through `RestCatalogBuilder::with_auth_manager`. It +/// builds the sessions the catalog then keeps. +/// +/// Both methods are handed the catalog's HTTP client, which an implementation +/// may reuse for its own requests (e.g. a token exchange) so that they share +/// the catalog's connection pool. #[async_trait] pub trait AuthManager: Debug + Send + Sync { - /// Session used for the initial `/v1/config` handshake, built from the - /// user-supplied configuration. + /// Session used for the initial `/v1/config` handshake, given the + /// user-supplied properties. /// /// Returns a [`Box`]: an init session is used once and released, unlike /// the shared [`AuthManager::catalog_session`]. - async fn init_session(&self) -> Result>; + async fn init_session( + &self, + client: &Client, + props: &HashMap, + ) -> Result>; /// Session used for all subsequent catalog requests, given the properties /// merged from the user configuration and the server's config response. @@ -57,20 +65,21 @@ pub trait AuthManager: Debug + Send + Sync { /// (e.g. a cached token) over from the init session. async fn catalog_session( &self, + client: &Client, props: &HashMap, ) -> Result>; } /// An outgoing REST request being authenticated by an [`AuthSession`]. /// -/// 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> { +/// 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> { inner: &'a mut Request, } -impl<'a> AuthRequest<'a> { +impl<'a> HttpRequest<'a> { /// Wraps a request, e.g. to unit-test a custom [`AuthSession`]. pub fn new(inner: &'a mut Request) -> Self { Self { inner } @@ -97,22 +106,22 @@ impl<'a> AuthRequest<'a> { } /// The request body, distinguishing an absent body from a streaming one: - /// signers can sign [`AuthRequestBody::Empty`] (empty-payload hash) and - /// [`AuthRequestBody::Buffered`], but not [`AuthRequestBody::Streaming`]. - pub fn body(&self) -> AuthRequestBody<'_> { + /// signers can sign [`HttpRequestBody::Empty`] (empty-payload hash) and + /// [`HttpRequestBody::Buffered`], but not [`HttpRequestBody::Streaming`]. + pub fn body(&self) -> HttpRequestBody<'_> { match self.inner.body() { - None => AuthRequestBody::Empty, + None => HttpRequestBody::Empty, Some(body) => match body.as_bytes() { - Some(bytes) => AuthRequestBody::Buffered(bytes), - None => AuthRequestBody::Streaming, + Some(bytes) => HttpRequestBody::Buffered(bytes), + None => HttpRequestBody::Streaming, }, } } } -/// The body of an [`AuthRequest`], as seen by authentication. +/// The body of an [`HttpRequest`], as seen by authentication. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum AuthRequestBody<'a> { +pub enum HttpRequestBody<'a> { /// No body is set. Empty, /// An in-memory body. @@ -121,14 +130,14 @@ pub enum AuthRequestBody<'a> { Streaming, } -impl<'a> AuthRequestBody<'a> { +impl<'a> HttpRequestBody<'a> { /// The signable bytes: empty for [`Self::Empty`], the buffer for /// [`Self::Buffered`], and `None` for [`Self::Streaming`]. pub fn as_bytes(&self) -> Option<&'a [u8]> { match self { - AuthRequestBody::Empty => Some(&[]), - AuthRequestBody::Buffered(bytes) => Some(bytes), - AuthRequestBody::Streaming => None, + HttpRequestBody::Empty => Some(&[]), + HttpRequestBody::Buffered(bytes) => Some(bytes), + HttpRequestBody::Streaming => None, } } } @@ -137,7 +146,7 @@ impl<'a> AuthRequestBody<'a> { #[async_trait] pub trait AuthSession: Debug + Send + Sync { /// Applies authentication to the request (adds headers, signs, ...). - async fn authenticate(&self, request: &mut AuthRequest<'_>) -> Result<()>; + async fn authenticate(&self, request: &mut HttpRequest<'_>) -> Result<()>; } /// [`AuthManager`] that performs no authentication. @@ -150,12 +159,17 @@ struct NoopSession; #[async_trait] impl AuthManager for NoopAuthManager { - async fn init_session(&self) -> Result> { + async fn init_session( + &self, + _client: &Client, + _props: &HashMap, + ) -> Result> { Ok(Box::new(NoopSession)) } async fn catalog_session( &self, + _client: &Client, _props: &HashMap, ) -> Result> { Ok(Arc::new(NoopSession)) @@ -164,29 +178,30 @@ impl AuthManager for NoopAuthManager { #[async_trait] impl AuthSession for NoopSession { - async fn authenticate(&self, _request: &mut AuthRequest<'_>) -> Result<()> { + async fn authenticate(&self, _request: &mut HttpRequest<'_>) -> Result<()> { Ok(()) } } #[cfg(test)] mod tests { - use reqwest::Client; - use super::*; #[tokio::test] async fn test_static_token_session_attaches_token() { // Token-only config: the token is attached as-is. let manager = OAuth2Manager::new("http://localhost/unused").with_token("tok-static"); - let session = manager.init_session().await.unwrap(); + let session = manager + .init_session(&Client::new(), &HashMap::new()) + .await + .unwrap(); let mut req = Client::new() .get("https://rest.example.com/v1/config") .build() .unwrap(); session - .authenticate(&mut AuthRequest::new(&mut req)) + .authenticate(&mut HttpRequest::new(&mut req)) .await .unwrap(); assert_eq!( @@ -204,9 +219,9 @@ mod tests { .get("https://rest.example.com/v1/config") .build() .unwrap(); - let auth_req = AuthRequest::new(&mut req); + let auth_req = HttpRequest::new(&mut req); let body = auth_req.body(); - assert_eq!(body, AuthRequestBody::Empty); + assert_eq!(body, HttpRequestBody::Empty); assert_eq!(body.as_bytes(), Some(&[] as &[u8])); // An in-memory body. @@ -215,9 +230,9 @@ mod tests { .body("{}") .build() .unwrap(); - let auth_req = AuthRequest::new(&mut req); + let auth_req = HttpRequest::new(&mut req); let body = auth_req.body(); - assert_eq!(body, AuthRequestBody::Buffered(b"{}")); + assert_eq!(body, HttpRequestBody::Buffered(b"{}")); assert_eq!(body.as_bytes(), Some(b"{}" as &[u8])); // A streaming body: bytes are unavailable, so it must not sign as empty. @@ -228,9 +243,9 @@ mod tests { }))) .build() .unwrap(); - let auth_req = AuthRequest::new(&mut req); + let auth_req = HttpRequest::new(&mut req); let body = auth_req.body(); - assert_eq!(body, AuthRequestBody::Streaming); + assert_eq!(body, HttpRequestBody::Streaming); assert_eq!(body.as_bytes(), None); } } diff --git a/crates/catalog/rest/src/auth/oauth2.rs b/crates/catalog/rest/src/auth/oauth2.rs index 630b7505e9..fe3f93814c 100644 --- a/crates/catalog/rest/src/auth/oauth2.rs +++ b/crates/catalog/rest/src/auth/oauth2.rs @@ -26,15 +26,14 @@ use reqwest::header::HeaderMap; use reqwest::{Client, Method}; use tokio::sync::Mutex; -use super::{AuthManager, AuthRequest, AuthSession}; +use super::{AuthManager, AuthSession, HttpRequest}; use crate::catalog::{ REST_CATALOG_PROP_URI, RestCatalogConfig, credential_from_props, default_token_endpoint, explicit_headers_from_props, }; use crate::types::{ErrorResponse, TokenResponse}; -/// Per-phase OAuth2 parameters (init vs. post-handshake catalog phase). -#[derive(Clone)] +/// The manager's own OAuth2 options, which properties are merged onto. struct OAuth2Params { extra_headers: HeaderMap, token_endpoint: String, @@ -49,7 +48,8 @@ struct OAuth2Params { /// for a token at the token endpoint and cached. The cached token is shared /// across sessions so it survives the config handshake. pub struct OAuth2Manager { - client: Client, + /// Overrides the catalog's client for token requests. + client: Option, token: Arc>>, init_params: OAuth2Params, /// True when the token endpoint was derived from the catalog URI (not @@ -68,7 +68,7 @@ impl OAuth2Manager { /// ``` pub fn new(token_endpoint: impl Into) -> Self { Self { - client: Client::default(), + client: None, token: Arc::new(Mutex::new(None)), init_params: OAuth2Params { extra_headers: HeaderMap::new(), @@ -93,9 +93,9 @@ impl OAuth2Manager { self } - /// Sets the HTTP client used for token requests. + /// Sets the client used for token requests, instead of the catalog's. pub fn with_client(mut self, client: Client) -> Self { - self.client = client; + self.client = Some(client); self } @@ -115,7 +115,8 @@ impl OAuth2Manager { pub(crate) fn from_config(cfg: &RestCatalogConfig) -> Result { Ok(Self { - client: cfg.client(), + // The catalog hands its client to every session. + client: None, token: Arc::new(Mutex::new(cfg.token().map(Credential::from))), init_params: OAuth2Params { extra_headers: cfg.extra_headers()?, @@ -138,21 +139,38 @@ impl Debug for OAuth2Manager { #[async_trait] impl AuthManager for OAuth2Manager { - async fn init_session(&self) -> Result> { - Ok(self.build_session(self.init_params.clone())) + async fn init_session( + &self, + client: &Client, + props: &HashMap, + ) -> Result> { + Ok(Box::new(self.session_from(client, props).await?)) } async fn catalog_session( &self, + client: &Client, props: &HashMap, ) -> Result> { - // The server config may carry a new token (or restate the user's). + Ok(Arc::new(self.session_from(client, props).await?)) + } +} + +impl OAuth2Manager { + /// Builds a session from the manager's options with `props` merged onto + /// them, so an injected manager keeps whatever a property doesn't + /// override. The manager's token cell is shared with every session it + /// builds, so a token cached during the handshake survives it. + async fn session_from( + &self, + client: &Client, + props: &HashMap, + ) -> Result { + // The properties may carry a new token (or restate the user's). if let Some(token) = props.get("token") { *self.token.lock().await = Some(Credential::from(token.clone())); } - // Explicit property overrides merge ONTO the manager's options, so an - // injected manager keeps whatever a property doesn't override. let mut extra_headers = self.init_params.extra_headers.clone(); extra_headers.extend(explicit_headers_from_props(props)?); @@ -174,50 +192,34 @@ impl AuthManager for OAuth2Manager { _ => self.init_params.token_endpoint.clone(), }; - Ok(Arc::from( - self.build_session(OAuth2Params { - extra_headers, - token_endpoint, - credential: credential_from_props(props) - .map(|(id, secret)| (id, secret.into())) - .or_else(|| self.init_params.credential.clone()), - extra_oauth_params, - }), - )) - } -} + let credential = credential_from_props(props) + .map(|(id, secret)| (id, secret.into())) + .or_else(|| self.init_params.credential.clone()); -impl OAuth2Manager { - /// Builds the session matching the configured mode: - /// - /// - a `credential` yields a [`ClientCredentialsSession`] (its token cache - /// pre-seeded when a `token` is also set, and the token then takes - /// precedence over the credential); - /// - otherwise a [`StaticTokenSession`], which attaches the configured - /// token as-is — or nothing when none is set. - /// - /// Both share the manager's token cell, so a cached token survives the - /// config handshake. - fn build_session(&self, params: OAuth2Params) -> Box { - match params.credential { - Some(credential) => Box::new(ClientCredentialsSession { - client: self.client.clone(), - token: self.token.clone(), - credential, - token_endpoint: params.token_endpoint, - extra_headers: params.extra_headers, - extra_oauth_params: params.extra_oauth_params, - }), - None => Box::new(StaticTokenSession { - token: self.token.clone(), - }), - } + Ok(OAuth2Session { + token: self.token.clone(), + // A configured token takes precedence over the credential: the + // token cell is pre-seeded, and the credential only comes into + // play once that token is gone. + token_source: match credential { + Some(credential) => { + TokenSource::ClientCredentials(Box::new(ClientCredentialsConfig { + client: self.client.clone().unwrap_or_else(|| client.clone()), + credential, + token_endpoint, + extra_headers, + extra_oauth_params, + })) + } + None => TokenSource::StaticToken, + }, + }) } } /// Attaches `token` as a `Authorization: Bearer ` header, marked /// sensitive so `Debug`-formatted requests redact it. -fn attach_bearer(req: &mut AuthRequest<'_>, token: &Credential) -> Result<()> { +fn attach_bearer(req: &mut HttpRequest<'_>, token: &Credential) -> Result<()> { let mut value: http::HeaderValue = format!("Bearer {}", token.expose()).parse().map_err(|e| { Error::new( @@ -231,47 +233,47 @@ fn attach_bearer(req: &mut AuthRequest<'_>, token: &Credential) -> Result<()> { Ok(()) } -/// [`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 { - /// Shared with the owning [`OAuth2Manager`]. +/// [`AuthSession`] attaching an OAuth2 bearer token. +/// +/// The token is a configured one (which replaces whatever the cell holds), a +/// token cached by an earlier session (the cell is shared with the owning +/// [`OAuth2Manager`]), or — with [`TokenSource::ClientCredentials`] — one +/// exchanged for the credential on demand. +/// +/// # TODO: Support automatic token refreshing. +struct OAuth2Session { token: Arc>>, + token_source: TokenSource, } -#[async_trait] -impl AuthSession for StaticTokenSession { - async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { - match self.token.lock().await.clone() { - Some(token) => attach_bearer(req, &token), - None => Ok(()), - } - } +/// How an [`OAuth2Session`] obtains a token once none is cached. +enum TokenSource { + /// Nothing to obtain: the session attaches the configured token, or no + /// authentication at all when there is none. + StaticToken, + /// The credential is exchanged for a token at the token endpoint. + ClientCredentials(Box), } -/// [`AuthSession`] implementing the OAuth2 client-credentials flow: exchanges -/// the credential for a token at the token endpoint and caches it. -/// -/// # TODO: Support automatic token refreshing. -struct ClientCredentialsSession { +struct ClientCredentialsConfig { client: Client, - /// Cached bearer token, shared with the owning [`OAuth2Manager`]. - token: Arc>>, credential: (Option, Credential), token_endpoint: String, extra_headers: HeaderMap, extra_oauth_params: HashMap, } -impl Debug for ClientCredentialsSession { +impl Debug for OAuth2Session { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.debug_struct("ClientCredentialsSession") - .field("token_endpoint", &self.token_endpoint) - .finish_non_exhaustive() + let mut out = f.debug_struct("OAuth2Session"); + if let TokenSource::ClientCredentials(config) = &self.token_source { + out.field("token_endpoint", &config.token_endpoint); + } + out.finish_non_exhaustive() } } -impl ClientCredentialsSession { +impl ClientCredentialsConfig { async fn exchange_credential_for_token(&self) -> Result { let (client_id, client_secret) = &self.credential; @@ -338,25 +340,29 @@ impl ClientCredentialsSession { } #[async_trait] -impl AuthSession for ClientCredentialsSession { - /// Uses the cached token when present (a configured `token` takes - /// precedence over the credential); otherwise exchanges the credential - /// for a token, caches it, then uses it. - async fn authenticate(&self, req: &mut AuthRequest<'_>) -> Result<()> { +impl AuthSession for OAuth2Session { + /// Uses the cached token when present; otherwise exchanges the credential + /// for one, caches it, then uses it. Without a credential and without a + /// token, no authentication is attached. + async fn authenticate(&self, req: &mut HttpRequest<'_>) -> Result<()> { // The lock is held across the exchange: waiters reuse a successful // result, and retry themselves after a failure. let token = { let mut token = self.token.lock().await; - match &*token { - Some(token) => token.clone(), - None => { - let new_token = Credential::from(self.exchange_credential_for_token().await?); + match (&*token, &self.token_source) { + (Some(token), _) => Some(token.clone()), + (None, TokenSource::StaticToken) => None, + (None, TokenSource::ClientCredentials(config)) => { + let new_token = Credential::from(config.exchange_credential_for_token().await?); *token = Some(new_token.clone()); - new_token + Some(new_token) } } }; - attach_bearer(req, &token) + match token { + Some(token) => attach_bearer(req, &token), + None => Ok(()), + } } } diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 7fb60afedd..955128d01d 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -18,6 +18,7 @@ //! This module contains the iceberg REST catalog implementation. use std::collections::{HashMap, HashSet}; +use std::fmt::{Debug, Formatter}; use std::future::Future; use std::str::FromStr; use std::sync::{Arc, OnceLock}; @@ -34,11 +35,13 @@ use itertools::Itertools; use reqwest::header::{ HeaderMap, HeaderName, HeaderValue, {self}, }; -use reqwest::{Client, Method, StatusCode, Url}; +use reqwest::{Client, Method, Request, Response, StatusCode, Url}; use tokio::sync::OnceCell; use typed_builder::TypedBuilder; -use crate::auth::{AUTH_TYPE_NONE, AUTH_TYPE_OAUTH2, AuthManager, NoopAuthManager, OAuth2Manager}; +use crate::auth::{ + AUTH_TYPE_NONE, AUTH_TYPE_OAUTH2, AuthManager, AuthSession, NoopAuthManager, OAuth2Manager, +}; use crate::client::{ HttpClient, deserialize_catalog_response, deserialize_unexpected_catalog_error, }; @@ -218,8 +221,8 @@ fn is_sensitive_prop(key: &str) -> bool { /// Redacts secret property values: this config is printed by /// [`RestCatalog`]'s derived `Debug`. -impl std::fmt::Debug for RestCatalogConfig { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl Debug for RestCatalogConfig { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let props: HashMap<&str, &str> = self .props .iter() @@ -326,14 +329,22 @@ impl RestCatalogConfig { oauth_params_from_props(&self.props) } - /// The properties handed to [`AuthManager::catalog_session`], with the - /// resolved catalog `uri` made explicit. + /// The properties handed to the [`AuthManager`], with the catalog `uri` + /// and `warehouse` made explicit. pub(crate) fn auth_props(&self) -> HashMap { // `oauth2-server-uri` stays absent unless explicitly configured, so an - // injected manager keeps its own endpoint. The resolved `uri` IS passed - // so the built-in manager can recompute its default from it. + // injected manager keeps its own endpoint. The resolved `uri` and + // `warehouse` ARE passed: the builder moved them off the props, and + // the built-in manager recomputes its token endpoint from the URI. let mut props = self.props.clone(); props.insert(REST_CATALOG_PROP_URI.to_string(), self.uri.clone()); + if let Some(warehouse) = &self.warehouse { + // A fallback only: after the handshake the merged props hold + // the resolved warehouse, server override included. + props + .entry(REST_CATALOG_PROP_WAREHOUSE.to_string()) + .or_insert_with(|| warehouse.clone()); + } props } @@ -348,14 +359,16 @@ impl RestCatalogConfig { .unwrap_or(false) } - /// The configured auth scheme: explicit `rest.auth.type` when set; + /// The configured auth scheme: explicit `rest.auth.type` (matched + /// case-insensitively) when set; /// 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 { self.props .get(REST_CATALOG_PROP_AUTH_TYPE) - .cloned() + // Matched case-insensitively, as the other flag properties are. + .map(|auth_type| auth_type.to_ascii_lowercase()) .unwrap_or_else(|| { if self.token().is_some() || self.credential().is_some() @@ -518,9 +531,11 @@ pub(crate) fn oauth_params_from_props(props: &HashMap) -> HashMa params } -#[derive(Debug)] struct RestContext { client: HttpClient, + /// The session the catalog's auth manager derived from the merged + /// configuration; it authenticates every request below. + session: Arc, /// Runtime config is fetched from rest server and stored here. /// /// It's could be different from the user config. @@ -529,6 +544,32 @@ struct RestContext { endpoints: HashSet, } +impl Debug for RestContext { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // The session is omitted: a custom one may print its credentials. + f.debug_struct("RestContext") + .field("client", &self.client) + .field("config", &self.config) + .field("endpoints", &self.endpoints) + .finish_non_exhaustive() + } +} + +impl RestContext { + /// Testing only: the bearer token the catalog session would attach. + #[cfg(test)] + async fn token(&self) -> Option { + self.client.token(self.session.as_ref()).await + } + + /// Sends `request` authenticated by the catalog session. + async fn query_catalog(&self, request: Request) -> Result { + self.client + .query_catalog(request, self.session.as_ref()) + .await + } +} + /// Rest catalog implementation. #[derive(Debug)] pub struct RestCatalog { @@ -574,7 +615,7 @@ impl RestCatalog { } let request = request_builder.build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::NO_CONTENT | StatusCode::OK => Ok(()), @@ -594,8 +635,18 @@ impl RestCatalog { async fn context(&self) -> Result<&RestContext> { self.ctx .get_or_try_init(|| async { - let client = HttpClient::new(&self.user_config).await?; - let catalog_config = RestCatalog::load_config(&client, &self.user_config).await?; + let client = HttpClient::new(&self.user_config)?; + let auth_manager = self.user_config.resolve_auth_manager()?; + // The init session lives only for the config handshake, so a + // manager whose session guards a one-shot resource can release + // it before deriving the catalog session. + let catalog_config = { + let init_session = auth_manager + .init_session(&self.user_config.client(), &self.user_config.auth_props()) + .await?; + RestCatalog::load_config(&client, init_session.as_ref(), &self.user_config) + .await? + }; // Use the advertised endpoints as-is, falling back to // `DEFAULT_ENDPOINTS` when absent or empty. let endpoints = match &catalog_config.endpoints { @@ -605,11 +656,15 @@ impl RestCatalog { _ => crate::endpoint::DEFAULT_ENDPOINTS.clone(), }; let config = self.user_config.clone().merge_with_config(catalog_config); - let client = client.update_with(&config).await?; + let client = client.update_with(&config)?; + let session = auth_manager + .catalog_session(&config.client(), &config.auth_props()) + .await?; Ok(RestContext { config, client, + session, endpoints, }) }) @@ -627,7 +682,7 @@ impl RestCatalog { /// `2xx` means it exists, `404` means it doesn't. async fn check_exists_via_head(&self, context: &RestContext, url: String) -> Result { let request = context.client.request(Method::HEAD, url).build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::NO_CONTENT | StatusCode::OK => Ok(true), @@ -645,6 +700,7 @@ impl RestCatalog { /// It's required for a REST catalog to update its config after creation. async fn load_config( client: &HttpClient, + session: &dyn AuthSession, user_config: &RestCatalogConfig, ) -> Result { let mut request_builder = client.request(Method::GET, user_config.config_endpoint()); @@ -655,7 +711,7 @@ impl RestCatalog { let request = request_builder.build()?; - let http_response = client.query_catalog(request).await?; + let http_response = client.query_catalog(request, session).await?; match http_response.status() { StatusCode::OK => deserialize_catalog_response(http_response).await, @@ -734,7 +790,7 @@ impl Catalog for RestCatalog { request = request.query(&[("pageToken", token)]); } - let http_response = context.client.query_catalog(request.build()?).await?; + let http_response = context.query_catalog(request.build()?).await?; match http_response.status() { StatusCode::OK => { @@ -784,7 +840,7 @@ impl Catalog for RestCatalog { }) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::OK => { @@ -812,7 +868,7 @@ impl Catalog for RestCatalog { .request(Method::GET, context.config.namespace_endpoint(namespace)) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::OK => { @@ -869,7 +925,7 @@ impl Catalog for RestCatalog { .request(Method::DELETE, context.config.namespace_endpoint(namespace)) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::NO_CONTENT | StatusCode::OK => Ok(()), @@ -898,7 +954,7 @@ impl Catalog for RestCatalog { request = request.query(&[("pageToken", token)]); } - let http_response = context.client.query_catalog(request.build()?).await?; + let http_response = context.query_catalog(request.build()?).await?; match http_response.status() { StatusCode::OK => { @@ -960,7 +1016,7 @@ impl Catalog for RestCatalog { }) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; let response = match http_response.status() { StatusCode::OK => { @@ -1031,7 +1087,7 @@ impl Catalog for RestCatalog { .request(Method::GET, context.config.table_endpoint(table_ident)) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; let response = match http_response.status() { StatusCode::OK | StatusCode::NOT_MODIFIED => { @@ -1120,7 +1176,7 @@ impl Catalog for RestCatalog { }) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; match http_response.status() { StatusCode::NO_CONTENT | StatusCode::OK => Ok(()), @@ -1162,7 +1218,7 @@ impl Catalog for RestCatalog { }) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; let response: LoadTableResult = match http_response.status() { StatusCode::OK => { @@ -1224,7 +1280,7 @@ impl Catalog for RestCatalog { }) .build()?; - let http_response = context.client.query_catalog(request).await?; + let http_response = context.query_catalog(request).await?; let response: CommitTableResponse = match http_response.status() { StatusCode::OK => deserialize_catalog_response(http_response).await?, @@ -1305,6 +1361,7 @@ mod tests { use uuid::uuid; use super::*; + use crate::auth::HttpRequest; #[tokio::test] async fn test_update_config() { @@ -1528,7 +1585,7 @@ mod tests { None, ); - let token = catalog.context().await.unwrap().client.token().await; + let token = catalog.context().await.unwrap().token().await; oauth_mock.assert_async().await; config_mock.assert_async().await; assert_eq!(token, Some("ey000000000000".to_string())); @@ -1577,7 +1634,7 @@ mod tests { None, ); - let token = catalog.context().await.unwrap().client.token().await; + let token = catalog.context().await.unwrap().token().await; oauth_mock.assert_async().await; config_mock.assert_async().await; @@ -1682,7 +1739,7 @@ mod tests { None, ); - let token = catalog.context().await.unwrap().client.token().await; + let token = catalog.context().await.unwrap().token().await; oauth_mock.assert_async().await; config_mock.assert_async().await; @@ -1791,7 +1848,7 @@ mod tests { let session = config .resolve_auth_manager() .unwrap() - .init_session() + .init_session(&Client::new(), &HashMap::new()) .await .unwrap(); let mut req = Client::new() @@ -1799,7 +1856,7 @@ mod tests { .build() .unwrap(); session - .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .authenticate(&mut HttpRequest::new(&mut req)) .await .unwrap(); assert!(req.headers().get("authorization").is_none()); @@ -1875,7 +1932,7 @@ mod tests { bootstrap_oauth_mock.assert_async().await; // The catalog session's endpoint follows the overridden URI (visible // via the session's Debug, which prints its token endpoint). - let session_debug = format!("{:?}", context.client.session()); + let session_debug = format!("{:?}", context.session); assert!(session_debug.contains(&format!("{}/v1/oauth/tokens", overridden.url()))); } @@ -1890,8 +1947,12 @@ mod tests { let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) .with_credential(Some("client1".to_string()), "secret1".to_string()); - let session: Arc = - Arc::from(manager.init_session().await.unwrap()); + let session: Arc = Arc::from( + manager + .init_session(&Client::new(), &HashMap::new()) + .await + .unwrap(), + ); let client = Client::new(); let attempts = (0..8).map(|_| { @@ -1903,7 +1964,7 @@ mod tests { .build() .unwrap(); session - .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .authenticate(&mut HttpRequest::new(&mut req)) .await .unwrap(); req.headers() @@ -1934,14 +1995,17 @@ mod tests { let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) .with_token("tok-seed") .with_credential(Some("client1".to_string()), "secret1".to_string()); - let session = manager.init_session().await.unwrap(); + let session = manager + .init_session(&Client::new(), &HashMap::new()) + .await + .unwrap(); let mut req = Client::new() .get("https://rest.example.com/v1/config") .build() .unwrap(); session - .authenticate(&mut crate::auth::AuthRequest::new(&mut req)) + .authenticate(&mut HttpRequest::new(&mut req)) .await .unwrap(); assert_eq!( @@ -2030,10 +2094,173 @@ mod tests { } #[tokio::test] - async fn test_catalog_session_receives_resolved_warehouse() { + async fn test_props_token_takes_precedence_over_props_credential() { + // Both arriving through the properties rather than the builder. + let mut server = Server::new_async().await; + let oauth_mock = server + .mock("POST", "/v1/oauth/tokens") + .expect(0) + .create_async() + .await; + + let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())); + let session = manager + .init_session( + &Client::new(), + &HashMap::from([ + ("token".to_string(), "tok-props".to_string()), + ("credential".to_string(), "client1:secret1".to_string()), + ]), + ) + .await + .unwrap(); + + let mut req = Client::new() + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + assert_eq!( + req.headers().get("authorization").unwrap(), + "Bearer tok-props" + ); + oauth_mock.assert_async().await; + } + + #[tokio::test] + async fn test_manager_uses_the_catalog_client_unless_overridden() { + // The manager exchanges the credential over the client it is + // handed, sharing the catalog's pool; `with_client` overrides it. + let mut server = Server::new_async().await; + let catalog_client_mock = server + .mock("POST", "/v1/oauth/tokens") + .match_header("x-from", "catalog-client") + .with_status(200) + .with_body(r#"{"access_token": "tok", "token_type": "Bearer"}"#) + .create_async() + .await; + + let tagged = |tag: &'static str| { + Client::builder() + .default_headers(HeaderMap::from_iter([( + HeaderName::from_static("x-from"), + HeaderValue::from_static(tag), + )])) + .build() + .unwrap() + }; + + let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) + .with_credential(Some("client1".to_string()), "secret1".to_string()); + let session = manager + .init_session(&tagged("catalog-client"), &HashMap::new()) + .await + .unwrap(); + let mut req = Client::new() + .get("https://rest.example.com/v1/namespaces") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + catalog_client_mock.assert_async().await; + + // An explicit client wins over the catalog's. + let own_client_mock = server + .mock("POST", "/v1/oauth/tokens") + .match_header("x-from", "own-client") + .with_status(200) + .with_body(r#"{"access_token": "tok", "token_type": "Bearer"}"#) + .create_async() + .await; + let manager = OAuth2Manager::new(format!("{}/v1/oauth/tokens", server.url())) + .with_credential(Some("client1".to_string()), "secret1".to_string()) + .with_client(tagged("own-client")); + let session = manager + .init_session(&tagged("catalog-client"), &HashMap::new()) + .await + .unwrap(); + let mut req = Client::new() + .get("https://rest.example.com/v1/namespaces") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + own_client_mock.assert_async().await; + } + + #[tokio::test] + async fn test_init_session_receives_user_props() { use tokio::sync::Mutex as AsyncMutex; - use crate::auth::AuthSession; + // A custom manager initializes from the user configuration: the + // props carry the catalog `uri` and the user's credentials. + #[derive(Debug)] + struct PlainSession; + #[async_trait] + impl AuthSession for PlainSession { + async fn authenticate(&self, _request: &mut HttpRequest<'_>) -> Result<()> { + Ok(()) + } + } + + #[derive(Debug)] + struct CapturingManager(Arc>>>); + #[async_trait] + impl AuthManager for CapturingManager { + async fn init_session( + &self, + _client: &Client, + props: &HashMap, + ) -> Result> { + *self.0.lock().await = Some(props.clone()); + Ok(Box::new(PlainSession)) + } + async fn catalog_session( + &self, + _client: &Client, + _props: &HashMap, + ) -> Result> { + Ok(Arc::new(PlainSession)) + } + } + + let mut server = Server::new_async().await; + let config_mock = create_config_mock(&mut server).await; + let captured = Arc::new(AsyncMutex::new(None)); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .props(HashMap::from([( + "token".to_string(), + "tok-user".to_string(), + )])) + .auth_manager(Some(Arc::new(CapturingManager(captured.clone())))) + .build(), + Some(Arc::new(LocalFsStorageFactory)), + Runtime::current(), + None, + ); + + catalog.context().await.unwrap(); + config_mock.assert_async().await; + let props = captured.lock().await.clone().unwrap(); + assert_eq!(props.get("token").map(String::as_str), Some("tok-user")); + assert_eq!( + props.get(REST_CATALOG_PROP_URI).map(String::as_str), + Some(server.url().as_str()) + ); + } + + #[tokio::test] + async fn test_catalog_session_receives_resolved_warehouse() { + use tokio::sync::Mutex as AsyncMutex; // A custom manager must receive the resolved warehouse in the props // handed to `catalog_session`, with the standard precedence: @@ -2042,10 +2269,7 @@ mod tests { struct PlainSession; #[async_trait] impl AuthSession for PlainSession { - async fn authenticate( - &self, - _request: &mut crate::auth::AuthRequest<'_>, - ) -> Result<()> { + async fn authenticate(&self, _request: &mut HttpRequest<'_>) -> Result<()> { Ok(()) } } @@ -2054,11 +2278,16 @@ mod tests { struct CapturingManager(Arc>>>); #[async_trait] impl AuthManager for CapturingManager { - async fn init_session(&self) -> Result> { + async fn init_session( + &self, + _client: &Client, + _props: &HashMap, + ) -> Result> { Ok(Box::new(PlainSession)) } async fn catalog_session( &self, + _client: &Client, props: &HashMap, ) -> Result> { *self.0.lock().await = Some(props.clone()); @@ -2133,8 +2362,6 @@ mod tests { async fn test_init_session_dropped_before_catalog_session() { use std::sync::atomic::{AtomicBool, Ordering}; - use crate::auth::AuthSession; - // A manager whose init session guards a one-shot resource (released on // drop) must see it released before `catalog_session` is invoked. #[derive(Debug)] @@ -2146,10 +2373,7 @@ mod tests { } #[async_trait] impl AuthSession for GuardSession { - async fn authenticate( - &self, - _request: &mut crate::auth::AuthRequest<'_>, - ) -> Result<()> { + async fn authenticate(&self, _request: &mut HttpRequest<'_>) -> Result<()> { Ok(()) } } @@ -2158,10 +2382,7 @@ mod tests { struct PlainSession; #[async_trait] impl AuthSession for PlainSession { - async fn authenticate( - &self, - _request: &mut crate::auth::AuthRequest<'_>, - ) -> Result<()> { + async fn authenticate(&self, _request: &mut HttpRequest<'_>) -> Result<()> { Ok(()) } } @@ -2170,11 +2391,16 @@ mod tests { struct GuardManager(Arc); #[async_trait] impl AuthManager for GuardManager { - async fn init_session(&self) -> Result> { + async fn init_session( + &self, + _client: &Client, + _props: &HashMap, + ) -> Result> { Ok(Box::new(GuardSession(self.0.clone()))) } async fn catalog_session( &self, + _client: &Client, _props: &HashMap, ) -> Result> { if !self.0.load(Ordering::SeqCst) { @@ -2252,6 +2478,18 @@ mod tests { format!("{:?}", with_token.resolve_auth_manager().unwrap()).contains("OAuth2Manager") ); + // An explicit type is matched case-insensitively. + let mixed_case = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([( + REST_CATALOG_PROP_AUTH_TYPE.to_string(), + "OAuth2".to_string(), + )])) + .build(); + assert!( + format!("{:?}", mixed_case.resolve_auth_manager().unwrap()).contains("OAuth2Manager") + ); + // An explicit OAuth endpoint is oauth2 intent too: the manager can // still pick up a server-supplied token from `/v1/config`. let with_endpoint = RestCatalogConfig::builder() @@ -2289,13 +2527,18 @@ mod tests { struct StubAuthManager; #[async_trait] impl AuthManager for StubAuthManager { - async fn init_session(&self) -> Result> { + async fn init_session( + &self, + _client: &Client, + _props: &HashMap, + ) -> Result> { unimplemented!() } async fn catalog_session( &self, + _client: &Client, _props: &HashMap, - ) -> Result> { + ) -> Result> { unimplemented!() } } diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index 856ee5c6c9..c6c1914eeb 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -17,7 +17,6 @@ use std::collections::HashMap; use std::fmt::{Debug, Formatter}; -use std::sync::Arc; use iceberg::{Error, ErrorKind, Result}; use reqwest::header::HeaderMap; @@ -25,7 +24,7 @@ use reqwest::{Client, IntoUrl, Method, Request, RequestBuilder, Response}; use serde::de::DeserializeOwned; use crate::RestCatalogConfig; -use crate::auth::{AuthManager, AuthRequest, AuthSession}; +use crate::auth::{AuthSession, HttpRequest}; pub(crate) struct HttpClient { client: Client, @@ -34,10 +33,6 @@ pub(crate) struct HttpClient { extra_headers: HeaderMap, /// Whether to disable header redaction in error logs (defaults to false for security). disable_header_redaction: bool, - /// The auth manager living for the lifetime of the catalog. - auth_manager: Arc, - /// The session authenticating requests in the current phase. - session: Arc, } impl Debug for HttpClient { @@ -56,15 +51,11 @@ impl Debug for HttpClient { impl HttpClient { /// Create a new http client. - pub async fn new(cfg: &RestCatalogConfig) -> Result { - let auth_manager = cfg.resolve_auth_manager()?; - let session = Arc::from(auth_manager.init_session().await?); + pub fn new(cfg: &RestCatalogConfig) -> Result { Ok(HttpClient { client: cfg.client(), extra_headers: cfg.extra_headers()?, disable_header_redaction: cfg.disable_header_redaction(), - auth_manager, - session, }) } @@ -72,56 +63,32 @@ impl HttpClient { /// /// If cfg carries new value, we will use cfg instead. /// Otherwise, we will keep the old value. - /// - /// 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 { - let HttpClient { - // `cfg.client()` below returns this same shared client. - client: _, - extra_headers: current_headers, - disable_header_redaction: _, - auth_manager, - session: init_session, - } = self; - // Release the init session first, so a manager whose init session - // guards a one-shot resource can build its catalog session. - drop(init_session); - + pub fn update_with(self, cfg: &RestCatalogConfig) -> Result { let extra_headers = (!cfg.extra_headers()?.is_empty()) .then(|| cfg.extra_headers()) .transpose()? - .unwrap_or(current_headers); - let session = auth_manager.catalog_session(&cfg.auth_props()).await?; + .unwrap_or(self.extra_headers); Ok(HttpClient { + // `cfg.client()` returns the same shared client. client: cfg.client(), extra_headers, disable_header_redaction: cfg.disable_header_redaction(), - auth_manager, - session, }) } - /// The session authenticating requests in the current phase. - #[cfg(test)] - pub(crate) fn session(&self) -> &Arc { - &self.session - } - - /// Testing only: the bearer token the current session would attach. + /// Testing only: the bearer token `session` would attach. /// /// Authenticates a throwaway request (never sent) and reads the header - /// back, so it works for any [`AuthSession`] without a test-only trait - /// method. + /// back, so it works for any [`AuthSession`]. #[cfg(test)] - pub(crate) async fn token(&self) -> Option { + pub(crate) async fn token(&self, session: &dyn AuthSession) -> Option { let mut request = self .client .request(Method::GET, "http://localhost/token-probe") .build() .ok()?; - self.session - .authenticate(&mut AuthRequest::new(&mut request)) + session + .authenticate(&mut HttpRequest::new(&mut request)) .await .ok()?; request @@ -142,11 +109,15 @@ impl HttpClient { // Queries the Iceberg REST catalog after authentication with the given `Request` and // returns a `Response`. - pub async fn query_catalog(&self, mut request: Request) -> Result { + pub async fn query_catalog( + &self, + mut request: Request, + session: &dyn AuthSession, + ) -> Result { // Authenticate first, then apply extra headers, so a configured // `header.authorization` keeps overriding a token (unchanged behavior). - self.session - .authenticate(&mut AuthRequest::new(&mut request)) + session + .authenticate(&mut HttpRequest::new(&mut request)) .await?; request.headers_mut().extend(self.extra_headers.clone()); Ok(self.client.execute(request).await?) @@ -288,7 +259,7 @@ mod tests { ), ])) .build(); - let client = HttpClient::new(&config).await.unwrap(); + let client = HttpClient::new(&config).unwrap(); let out = format!("{client:?}"); assert!(!out.contains("Basic xyz")); From b3044c7a2ed9e63dea6b7cde558a29842d0ba249 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 17 Jul 2026 01:45:22 -0400 Subject: [PATCH 9/9] feat(rest): add SigV4 auth manager wrapping a delegate session --- Cargo.lock | 29 +- Cargo.toml | 2 + crates/catalog/rest/Cargo.toml | 3 + crates/catalog/rest/public-api.txt | 45 ++ crates/catalog/rest/src/auth/mod.rs | 10 + crates/catalog/rest/src/auth/sigv4/mod.rs | 129 ++++ crates/catalog/rest/src/auth/sigv4/signer.rs | 747 +++++++++++++++++++ crates/catalog/rest/src/catalog.rs | 587 ++++++++++++++- crates/catalog/rest/src/client.rs | 7 +- 9 files changed, 1524 insertions(+), 35 deletions(-) create mode 100644 crates/catalog/rest/src/auth/sigv4/mod.rs create mode 100644 crates/catalog/rest/src/auth/sigv4/signer.rs diff --git a/Cargo.lock b/Cargo.lock index f72e5e0458..dcb386fd8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -167,7 +167,7 @@ dependencies = [ "miniz_oxide", "num-bigint", "quad-rand", - "rand 0.9.5", + "rand 0.9.4", "regex-lite", "serde", "serde_bytes", @@ -295,7 +295,7 @@ dependencies = [ "flate2", "indexmap 2.14.0", "liblzma", - "rand 0.9.5", + "rand 0.9.4", "serde", "serde_json", "snap", @@ -2098,7 +2098,7 @@ dependencies = [ "log", "object_store", "parking_lot", - "rand 0.9.5", + "rand 0.9.4", "tokio", "tokio-util", "url", @@ -2249,7 +2249,7 @@ dependencies = [ "object_store", "parking_lot", "parquet", - "rand 0.9.5", + "rand 0.9.4", "tempfile", "url", ] @@ -2348,7 +2348,7 @@ dependencies = [ "md-5 0.11.0", "memchr", "num-traits", - "rand 0.9.5", + "rand 0.9.4", "regex", "sha2 0.11.0", "uuid", @@ -2682,7 +2682,7 @@ dependencies = [ "log", "num-traits", "percent-encoding", - "rand 0.9.5", + "rand 0.9.4", "serde_json", "sha1 0.11.0", "sha2 0.11.0", @@ -3797,7 +3797,7 @@ dependencies = [ "ordered-float 4.6.0", "parquet", "pretty_assertions", - "rand 0.9.5", + "rand 0.9.4", "regex", "reqwest 0.12.28", "roaring", @@ -3891,9 +3891,11 @@ name = "iceberg-catalog-rest" version = "0.10.0" dependencies = [ "async-trait", + "base64 0.22.1", "bytes", "chrono", "futures", + "hmac 0.12.1", "http 1.4.2", "iceberg", "iceberg_test_utils", @@ -3903,6 +3905,7 @@ dependencies = [ "serde", "serde_derive", "serde_json", + "sha2 0.10.9", "tokio", "tracing", "typed-builder", @@ -4807,7 +4810,7 @@ dependencies = [ "hyper-util", "log", "pin-project-lite", - "rand 0.9.5", + "rand 0.9.4", "regex", "serde_json", "serde_urlencoded", @@ -6045,9 +6048,9 @@ dependencies = [ [[package]] name = "rand" -version = "0.9.5" +version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +checksum = "44c5af06bb1b7d3216d91932aed5265164bf384dc89cd6ba05cf59a35f5f76ea" dependencies = [ "rand_chacha 0.9.0", "rand_core 0.9.5", @@ -6585,7 +6588,7 @@ version = "2.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6b1e7f9a428571be2dc5bc0505c13fb6bf936822b894ec87abf8a08a4e51742d" dependencies = [ - "rand 0.9.5", + "rand 0.9.4", ] [[package]] @@ -8099,7 +8102,7 @@ version = "2.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ea3136b675547379c4bd395ca6b938e5ad3c3d20fad76e7fe85f9e0d011419c" dependencies = [ - "rand 0.9.5", + "rand 0.9.4", ] [[package]] @@ -8308,7 +8311,7 @@ dependencies = [ "nix 0.29.0", "once_cell", "pin-project", - "rand 0.9.5", + "rand 0.9.4", "socket2 0.5.10", "thiserror 2.0.18", "tokio", diff --git a/Cargo.toml b/Cargo.toml index a789ef1967..55467844a8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,6 +97,7 @@ form_urlencoded = "1.2.2" fs-err = "3.1.0" futures = "0.3" hive_metastore = "0.2.0" +hmac = "0.12" home = "0.5.12" http = "1.2" iceberg = { version = "0.10.0", path = "./crates/iceberg" } @@ -137,6 +138,7 @@ serde_derive = "1.0.219" serde_json = "1.0.142" serde_repr = "0.1.16" serde_with = "3.4" +sha2 = "0.10" sqllogictest = "0.29" sqlx = { version = "0.8.1", default-features = false } stacker = "0.1.20" diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 8dc9a86d7f..e1a152ff58 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -31,7 +31,9 @@ repository = { workspace = true } [dependencies] async-trait = { workspace = true } +base64 = { workspace = true } chrono = { workspace = true } +hmac = { workspace = true } http = { workspace = true } iceberg = { workspace = true } itertools = { workspace = true } @@ -39,6 +41,7 @@ reqwest = { workspace = true } serde = { workspace = true } serde_derive = { workspace = true } serde_json = { workspace = true } +sha2 = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } typed-builder = { workspace = true } diff --git a/crates/catalog/rest/public-api.txt b/crates/catalog/rest/public-api.txt index 739fc9f57f..bc1a1dfc41 100644 --- a/crates/catalog/rest/public-api.txt +++ b/crates/catalog/rest/public-api.txt @@ -14,6 +14,24 @@ impl<'a> core::fmt::Debug for iceberg_catalog_rest::HttpRequestBody<'a> pub fn iceberg_catalog_rest::HttpRequestBody<'a>::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl<'a> core::marker::Copy for iceberg_catalog_rest::HttpRequestBody<'a> impl<'a> core::marker::StructuralPartialEq for iceberg_catalog_rest::HttpRequestBody<'a> +pub enum iceberg_catalog_rest::PayloadHashMode +pub iceberg_catalog_rest::PayloadHashMode::IcebergRest +pub iceberg_catalog_rest::PayloadHashMode::StandardAws +impl core::clone::Clone for iceberg_catalog_rest::PayloadHashMode +pub fn iceberg_catalog_rest::PayloadHashMode::clone(&self) -> iceberg_catalog_rest::PayloadHashMode +impl core::cmp::Eq for iceberg_catalog_rest::PayloadHashMode +impl core::cmp::PartialEq for iceberg_catalog_rest::PayloadHashMode +pub fn iceberg_catalog_rest::PayloadHashMode::eq(&self, other: &iceberg_catalog_rest::PayloadHashMode) -> bool +impl core::fmt::Debug for iceberg_catalog_rest::PayloadHashMode +pub fn iceberg_catalog_rest::PayloadHashMode::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for iceberg_catalog_rest::PayloadHashMode +impl core::marker::StructuralPartialEq for iceberg_catalog_rest::PayloadHashMode +pub struct iceberg_catalog_rest::AwsCredentials +pub iceberg_catalog_rest::AwsCredentials::access_key_id: alloc::string::String +pub iceberg_catalog_rest::AwsCredentials::secret_access_key: iceberg::catalog::session::Credential +pub iceberg_catalog_rest::AwsCredentials::session_token: core::option::Option +impl core::clone::Clone for iceberg_catalog_rest::AwsCredentials +pub fn iceberg_catalog_rest::AwsCredentials::clone(&self) -> iceberg_catalog_rest::AwsCredentials pub struct iceberg_catalog_rest::CommitTableRequest pub iceberg_catalog_rest::CommitTableRequest::identifier: core::option::Option pub iceberg_catalog_rest::CommitTableRequest::requirements: alloc::vec::Vec @@ -281,6 +299,22 @@ pub fn iceberg_catalog_rest::RestCatalogBuilder::load(self, name: impl core::con pub fn iceberg_catalog_rest::RestCatalogBuilder::with_kms_client_factory(self, kms_client_factory: alloc::sync::Arc) -> Self pub fn iceberg_catalog_rest::RestCatalogBuilder::with_runtime(self, runtime: iceberg::runtime::Runtime) -> Self pub fn iceberg_catalog_rest::RestCatalogBuilder::with_storage_factory(self, storage_factory: alloc::sync::Arc) -> Self +pub struct iceberg_catalog_rest::SigV4AuthManager +impl iceberg_catalog_rest::SigV4AuthManager +pub fn iceberg_catalog_rest::SigV4AuthManager::new(delegate: alloc::sync::Arc, signer: iceberg_catalog_rest::SigV4Signer) -> Self +impl core::fmt::Debug for iceberg_catalog_rest::SigV4AuthManager +pub fn iceberg_catalog_rest::SigV4AuthManager::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::SigV4AuthManager +pub fn iceberg_catalog_rest::SigV4AuthManager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::SigV4AuthManager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub struct iceberg_catalog_rest::SigV4Signer +impl iceberg_catalog_rest::SigV4Signer +pub fn iceberg_catalog_rest::SigV4Signer::new(credentials: iceberg_catalog_rest::AwsCredentials, region: alloc::string::String, service: alloc::string::String, mode: iceberg_catalog_rest::PayloadHashMode) -> Self +pub fn iceberg_catalog_rest::SigV4Signer::sign(&self, request: &mut reqwest::async_impl::request::Request) -> iceberg::error::Result<()> +impl core::clone::Clone for iceberg_catalog_rest::SigV4Signer +pub fn iceberg_catalog_rest::SigV4Signer::clone(&self) -> iceberg_catalog_rest::SigV4Signer +impl core::fmt::Debug for iceberg_catalog_rest::SigV4Signer +pub fn iceberg_catalog_rest::SigV4Signer::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result pub struct iceberg_catalog_rest::StorageCredential pub iceberg_catalog_rest::StorageCredential::config: std::collections::hash::map::HashMap pub iceberg_catalog_rest::StorageCredential::prefix: alloc::string::String @@ -329,8 +363,16 @@ impl<'de> serde_core::de::Deserialize<'de> for iceberg_catalog_rest::UpdateNames pub fn iceberg_catalog_rest::UpdateNamespacePropertiesResponse::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> pub const iceberg_catalog_rest::AUTH_TYPE_NONE: &str pub const iceberg_catalog_rest::AUTH_TYPE_OAUTH2: &str +pub const iceberg_catalog_rest::AUTH_TYPE_SIGV4: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_ACCESS_KEY_ID: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_AUTH_TYPE: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SECRET_ACCESS_KEY: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SESSION_TOKEN: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SIGNING_NAME: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SIGNING_REGION: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE: &str +pub const iceberg_catalog_rest::REST_CATALOG_PROP_SIGV4_ENABLED: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_URI: &str pub const iceberg_catalog_rest::REST_CATALOG_PROP_WAREHOUSE: &str pub trait iceberg_catalog_rest::AuthManager: core::fmt::Debug + core::marker::Send + core::marker::Sync @@ -342,5 +384,8 @@ pub fn iceberg_catalog_rest::NoopAuthManager::init_session<'life0, 'life1, 'life impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::OAuth2Manager pub fn iceberg_catalog_rest::OAuth2Manager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub fn iceberg_catalog_rest::OAuth2Manager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +impl iceberg_catalog_rest::AuthManager for iceberg_catalog_rest::SigV4AuthManager +pub fn iceberg_catalog_rest::SigV4AuthManager::catalog_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait +pub fn iceberg_catalog_rest::SigV4AuthManager::init_session<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, client: &'life1 reqwest::async_impl::client::Client, props: &'life2 std::collections::hash::map::HashMap) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub trait iceberg_catalog_rest::AuthSession: core::fmt::Debug + core::marker::Send + core::marker::Sync pub fn iceberg_catalog_rest::AuthSession::authenticate<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, request: &'life1 mut iceberg_catalog_rest::HttpRequest<'life2>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait diff --git a/crates/catalog/rest/src/auth/mod.rs b/crates/catalog/rest/src/auth/mod.rs index 97d139d5af..dc8a9c4e8b 100644 --- a/crates/catalog/rest/src/auth/mod.rs +++ b/crates/catalog/rest/src/auth/mod.rs @@ -19,6 +19,7 @@ //! `AuthManager`/`AuthSession` API. mod oauth2; +mod sigv4; use std::collections::HashMap; use std::fmt::Debug; @@ -29,11 +30,14 @@ use http::{HeaderMap, Method}; use iceberg::Result; pub use oauth2::OAuth2Manager; use reqwest::{Client, Request}; +pub use sigv4::{AwsCredentials, PayloadHashMode, SigV4AuthManager, SigV4Signer}; /// `rest.auth.type` value disabling authentication. pub const AUTH_TYPE_NONE: &str = "none"; /// `rest.auth.type` value selecting OAuth2 token authentication. pub const AUTH_TYPE_OAUTH2: &str = "oauth2"; +/// `rest.auth.type` value selecting AWS SigV4 request signing. +pub const AUTH_TYPE_SIGV4: &str = "sigv4"; /// Creates the [`AuthSession`]s used to authenticate REST catalog requests. /// @@ -117,6 +121,12 @@ impl<'a> HttpRequest<'a> { }, } } + + /// The wrapped request, for crate-internal consumers that need the concrete + /// client type (e.g. handing it to [`SigV4Signer::sign`]). + pub(crate) fn inner_mut(&mut self) -> &mut Request { + self.inner + } } /// The body of an [`HttpRequest`], as seen by authentication. diff --git a/crates/catalog/rest/src/auth/sigv4/mod.rs b/crates/catalog/rest/src/auth/sigv4/mod.rs new file mode 100644 index 0000000000..8159f46f81 --- /dev/null +++ b/crates/catalog/rest/src/auth/sigv4/mod.rs @@ -0,0 +1,129 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; +use std::sync::Arc; + +use async_trait::async_trait; +use iceberg::Result; +use reqwest::Client; +use reqwest::header::{AUTHORIZATION, HeaderName}; + +mod signer; + +pub use signer::{AwsCredentials, PayloadHashMode, SigV4Signer}; + +use super::{AuthManager, AuthSession, HttpRequest}; +use crate::catalog::sigv4_signer_from_props; + +/// Header the delegate's `Authorization` is relocated to before signing, so +/// token-based auth composes with SigV4, which needs `Authorization` for the +/// signature itself. Iceberg Java relocates with the `Original-` prefix. +const RELOCATED_AUTH_HEADER: HeaderName = HeaderName::from_static("original-authorization"); + +/// [`AuthManager`] that SigV4-signs every request, wrapping a delegate +/// manager whose authentication (e.g. an OAuth2 bearer token) is relocated to +/// `Original-Authorization` and included in the signature. +/// +/// An injected HTTP client must not follow redirects (a signed request can't +/// be transparently re-followed) nor set default headers that change the +/// signed set (e.g. `Host`, `x-amz-*`): those apply after signing. +#[derive(Debug)] +pub struct SigV4AuthManager { + delegate: Arc, + signer: SigV4Signer, + /// True when the signer was built from catalog properties: + /// [`Self::catalog_session`] then rebuilds it from the merged props. + signer_from_config: bool, +} + +impl SigV4AuthManager { + /// Creates a SigV4 manager signing with `signer` on top of `delegate`. + /// + /// The signer is kept as-is — its credentials and payload mode exist + /// nowhere in the properties, so signing properties never replace it. + pub fn new(delegate: Arc, signer: SigV4Signer) -> Self { + Self { + delegate, + signer, + signer_from_config: false, + } + } + + /// A manager whose signer derives from catalog properties; the merged + /// `/v1/config` properties rebuild it. + pub(crate) fn from_config_signer(delegate: Arc, signer: SigV4Signer) -> Self { + Self { + delegate, + signer, + signer_from_config: true, + } + } +} + +#[async_trait] +impl AuthManager for SigV4AuthManager { + async fn init_session( + &self, + client: &Client, + props: &HashMap, + ) -> Result> { + Ok(Box::new(SigV4Session { + delegate: Arc::from(self.delegate.init_session(client, props).await?), + signer: self.signer.clone(), + })) + } + + async fn catalog_session( + &self, + client: &Client, + props: &HashMap, + ) -> Result> { + // A config-built signer follows the merged properties; an injected + // one is kept as-is (see [`Self::new`]). + let signer = if self.signer_from_config { + sigv4_signer_from_props(props)? + } else { + self.signer.clone() + }; + Ok(Arc::new(SigV4Session { + delegate: self.delegate.catalog_session(client, props).await?, + signer, + })) + } +} + +/// [`AuthSession`] applying the delegate's auth, then SigV4-signing. +#[derive(Debug)] +struct SigV4Session { + delegate: Arc, + signer: SigV4Signer, +} + +#[async_trait] +impl AuthSession for SigV4Session { + async fn authenticate(&self, request: &mut HttpRequest<'_>) -> Result<()> { + self.delegate.authenticate(request).await?; + if let Some(mut auth) = request.headers_mut().remove(AUTHORIZATION) { + // Force-mark it: a delegate (or `header.authorization`) may have + // supplied a non-sensitive value. + auth.set_sensitive(true); + request.headers_mut().insert(RELOCATED_AUTH_HEADER, auth); + } + self.signer.sign(request.inner_mut()) + } +} diff --git a/crates/catalog/rest/src/auth/sigv4/signer.rs b/crates/catalog/rest/src/auth/sigv4/signer.rs new file mode 100644 index 0000000000..d7adcfb91c --- /dev/null +++ b/crates/catalog/rest/src/auth/sigv4/signer.rs @@ -0,0 +1,747 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use chrono::{DateTime, Utc}; +use hmac::{Hmac, Mac}; +use iceberg::{Credential, Error, ErrorKind, Result}; +use sha2::{Digest, Sha256}; + +/// Hex SHA-256 of the empty string. +const EMPTY_BODY_HEX_SHA256: &str = + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; + +/// How the payload hash is encoded in the `x-amz-content-sha256` header. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum PayloadHashMode { + /// Iceberg Java's RESTSigV4 style: base64 header for non-empty bodies, hex + /// for empty; the canonical request always uses hex. + IcebergRest, + /// Standard AWS SigV4 style: hex everywhere (e.g. AWS Glue). + StandardAws, +} + +/// Derives the AWS SigV4 signing key. +fn hmac_sha256(key: &[u8], data: &[u8]) -> Vec { + let mut mac = as Mac>::new_from_slice(key).expect("HMAC takes a key of any size"); + mac.update(data); + mac.finalize().into_bytes().to_vec() +} + +fn hex_sha256(data: &[u8]) -> String { + encode_hex(&Sha256::digest(data)) +} + +fn hex_hmac_sha256(key: &[u8], data: &[u8]) -> String { + encode_hex(&hmac_sha256(key, data)) +} + +fn encode_hex(bytes: &[u8]) -> String { + bytes.iter().map(|byte| format!("{byte:02x}")).collect() +} + +fn base64_encode(bytes: &[u8]) -> String { + base64::engine::Engine::encode(&base64::engine::general_purpose::STANDARD, bytes) +} + +fn signing_key(secret: &str, date: &str, region: &str, service: &str) -> Vec { + let k_date = hmac_sha256(format!("AWS4{secret}").as_bytes(), date.as_bytes()); + let k_region = hmac_sha256(&k_date, region.as_bytes()); + let k_service = hmac_sha256(&k_region, service.as_bytes()); + hmac_sha256(&k_service, b"aws4_request") +} + +/// Computes the value of the `x-amz-content-sha256` header. +fn content_sha256_header(body: &[u8], mode: PayloadHashMode) -> String { + match mode { + PayloadHashMode::StandardAws => hex_sha256(body), + PayloadHashMode::IcebergRest => { + if body.is_empty() { + EMPTY_BODY_HEX_SHA256.to_string() + } else { + base64_encode(&Sha256::digest(body)) + } + } + } +} + +/// Builds the SigV4 canonical request. `headers` are (lowercased, trimmed) +/// pairs; `payload_hash` is always the hex sha256 of the body. +fn canonical_request( + method: &str, + canonical_uri: &str, + canonical_query: &str, + headers: &[(String, String)], + payload_hash: &str, +) -> String { + let mut sorted = headers.to_vec(); + sorted.sort_by(|a, b| a.0.cmp(&b.0)); + + let canonical_headers: String = sorted.iter().map(|(k, v)| format!("{k}:{v}\n")).collect(); + let signed_headers = sorted + .iter() + .map(|(k, _)| k.as_str()) + .collect::>() + .join(";"); + + format!( + "{method}\n{canonical_uri}\n{canonical_query}\n{canonical_headers}\n{signed_headers}\n{payload_hash}" + ) +} + +/// Builds the SigV4 string-to-sign. +fn string_to_sign(amz_date: &str, scope: &str, canonical_request: &str) -> String { + format!( + "AWS4-HMAC-SHA256\n{amz_date}\n{scope}\n{}", + hex_sha256(canonical_request.as_bytes()) + ) +} + +/// Static AWS-style credentials used for SigV4 signing of catalog requests. +#[derive(Clone)] +pub struct AwsCredentials { + /// AWS access key id. + pub access_key_id: String, + /// AWS secret access key. + pub secret_access_key: Credential, + /// Optional STS session token. + pub session_token: Option, +} + +/// AWS SigV4 signer following Iceberg Java's `RESTSigV4AuthSession`: it adds the +/// required amz headers and signs all request headers except a small blacklist. +#[derive(Clone)] +pub struct SigV4Signer { + credentials: AwsCredentials, + region: String, + service: String, + mode: PayloadHashMode, +} + +impl SigV4Signer { + /// Creates a new SigV4 signer. + pub fn new( + credentials: AwsCredentials, + region: String, + service: String, + mode: PayloadHashMode, + ) -> Self { + Self { + credentials, + region, + service, + mode, + } + } + + /// Signs `request` in place. + pub fn sign(&self, request: &mut reqwest::Request) -> Result<()> { + self.sign_at(request, Utc::now()) + } + + fn sign_at(&self, request: &mut reqwest::Request, now: DateTime) -> Result<()> { + let amz_date = now.format("%Y%m%dT%H%M%SZ").to_string(); + let date = now.format("%Y%m%d").to_string(); + let scope = format!("{date}/{}/{}/aws4_request", self.region, self.service); + + // The signed `host` must match what the HTTP layer sends: an explicit + // Host header wins; otherwise it derives from the URL, including a + // non-default port (the url crate strips scheme-default ports). + let explicit_host = request + .headers() + .get(reqwest::header::HOST) + .and_then(|v| v.to_str().ok()) + .map(|v| v.trim().to_string()); + let host = if let Some(host) = explicit_host { + host + } else { + let h = request + .url() + .host_str() + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "request url has no host"))?; + match request.url().port() { + Some(port) => format!("{h}:{port}"), + None => h.to_string(), + } + }; + // AWS SDK v2 parity (Aws4Signer defaults `normalizePath=true`, + // `doubleUrlEncode=true`): the normalized, already-percent-encoded + // path is encoded once more keeping `/` — e.g. `%2C` becomes `%252C`. + let canonical_uri = uri_encode(&normalize_path(request.url().path()), false); + // Canonicalized from the raw query, not `query_pairs()`: that form- + // decodes, turning a wire `+` into a space that re-encodes as `%20`, + // while SigV4 verifiers read `+` as a literal plus. + let mut encoded_pairs: Vec<(String, String)> = request + .url() + .query() + .unwrap_or_default() + .split('&') + .filter(|pair| !pair.is_empty()) + .map(|pair| { + let (name, value) = pair.split_once('=').unwrap_or((pair, "")); + ( + uri_encode(&percent_decode(name), true), + uri_encode(&percent_decode(value), true), + ) + }) + .collect(); + encoded_pairs.sort(); + let canonical_query = encoded_pairs + .iter() + .map(|(k, v)| format!("{k}={v}")) + .collect::>() + .join("&"); + + let body: &[u8] = match request.body() { + None => &[], + Some(b) => b.as_bytes().ok_or_else(|| { + Error::new( + ErrorKind::FeatureUnsupported, + "cannot sign a streaming request body", + ) + })?, + }; + let payload_hex = hex_sha256(body); + let content_header = content_sha256_header(body, self.mode); + + let mut headers = vec![ + ("host".to_string(), host), + ("x-amz-content-sha256".to_string(), content_header.clone()), + ("x-amz-date".to_string(), amz_date.clone()), + ]; + if let Some(tok) = &self.credentials.session_token { + headers.push(("x-amz-security-token".to_string(), tok.expose().to_string())); + } + + // Sign every other request header too (except a blacklist), as Iceberg + // Java / botocore do; signing only host/x-amz-* yields a signature mismatch. + const SKIP_HEADERS: &[&str] = &[ + "user-agent", + "authorization", + "expect", + "x-amzn-trace-id", + // Transports and proxies may rewrite these (AWS signers skip them). + "connection", + "transfer-encoding", + "x-forwarded-for", + ]; + let base_count = headers.len(); + for (name, value) in request.headers().iter() { + let lname = name.as_str().to_ascii_lowercase(); + if SKIP_HEADERS.contains(&lname.as_str()) + || headers[..base_count].iter().any(|(k, _)| *k == lname) + { + continue; + } + if let Ok(v) = value.to_str() { + // Canonical form: sequential spaces collapse to one, repeated + // header values comma-join in order. + let v = v.split_whitespace().collect::>().join(" "); + if let Some(pos) = headers[base_count..].iter().position(|(k, _)| *k == lname) { + let joined = &mut headers[base_count + pos].1; + joined.push(','); + joined.push_str(&v); + } else { + headers.push((lname, v)); + } + } + } + + // Canonical payload hash is ALWAYS hex (the IcebergRest split: header may be base64). + let creq = canonical_request( + request.method().as_str(), + &canonical_uri, + &canonical_query, + &headers, + &payload_hex, + ); + let sts = string_to_sign(&amz_date, &scope, &creq); + let key = signing_key( + self.credentials.secret_access_key.expose(), + &date, + &self.region, + &self.service, + ); + let signature = hex_hmac_sha256(&key, sts.as_bytes()); + + let mut signed = headers.iter().map(|(k, _)| k.clone()).collect::>(); + signed.sort(); + let signed_headers = signed.join(";"); + let authorization = format!( + "AWS4-HMAC-SHA256 Credential={}/{scope}, SignedHeaders={signed_headers}, Signature={signature}", + self.credentials.access_key_id + ); + + let h = request.headers_mut(); + h.insert("x-amz-date", amz_date.parse().unwrap()); + h.insert("x-amz-content-sha256", content_header.parse().unwrap()); + if let Some(tok) = &self.credentials.session_token { + let mut token_value: reqwest::header::HeaderValue = + tok.expose().parse().map_err(|e| { + Error::new(ErrorKind::DataInvalid, "invalid session token").with_source(e) + })?; + // Redacted in `Debug`-formatted requests. + token_value.set_sensitive(true); + h.insert("x-amz-security-token", token_value); + } + let mut auth_value: reqwest::header::HeaderValue = authorization.parse().map_err(|e| { + Error::new(ErrorKind::Unexpected, "invalid Authorization header").with_source(e) + })?; + auth_value.set_sensitive(true); + h.insert(reqwest::header::AUTHORIZATION, auth_value); + Ok(()) + } +} + +impl std::fmt::Debug for SigV4Signer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SigV4Signer") + .field("region", &self.region) + .field("service", &self.service) + .field("mode", &self.mode) + .field("access_key_id", &self.credentials.access_key_id) + .finish_non_exhaustive() + } +} + +/// Path normalization as AWS signers apply it (botocore `normalize_url_path`, +/// AWS SDK v2 `normalizePath`): duplicate slashes and `.` segments collapse, +/// `..` pops, a trailing slash is kept. +fn normalize_path(path: &str) -> String { + let mut segments: Vec<&str> = Vec::new(); + for segment in path.split('/') { + match segment { + "" | "." => {} + ".." => { + segments.pop(); + } + other => segments.push(other), + } + } + let mut normalized = String::from("/"); + normalized.push_str(&segments.join("/")); + if !normalized.ends_with('/') + && (path.ends_with('/') || path.ends_with("/.") || path.ends_with("/..")) + { + normalized.push('/'); + } + normalized +} + +/// Decodes `%XX` sequences, leaving `+` alone: unlike form decoding, RFC 3986 +/// has no special meaning for it, and neither does SigV4. +fn percent_decode(input: &str) -> String { + fn hex(byte: u8) -> Option { + (byte as char).to_digit(16).map(|digit| digit as u8) + } + let bytes = input.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'%' + && i + 2 < bytes.len() + && let (Some(high), Some(low)) = (hex(bytes[i + 1]), hex(bytes[i + 2])) + { + out.push(high * 16 + low); + i += 3; + continue; + } + out.push(bytes[i]); + i += 1; + } + String::from_utf8_lossy(&out).into_owned() +} + +/// RFC 3986 URI encoding (AWS rules). When `encode_slash` is false, `/` is kept. +fn uri_encode(s: &str, encode_slash: bool) -> String { + let mut out = String::with_capacity(s.len()); + for b in s.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' => { + out.push(b as char) + } + b'/' if !encode_slash => out.push('/'), + _ => out.push_str(&format!("%{b:02X}")), + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + const EMPTY_HEX: &str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; + + #[test] + fn test_percent_decode_keeps_plus() { + // `+` is a literal plus to SigV4, not a space: reqwest writes spaces + // in query values as `+`, and the signature must match the wire. + assert_eq!(percent_decode("my+wh"), "my+wh"); + assert_eq!(percent_decode("my%20wh"), "my wh"); + assert_eq!(percent_decode("a%2Fb"), "a/b"); + assert_eq!(percent_decode("bad%zz"), "bad%zz"); + } + + #[test] + fn test_canonical_query_encodes_a_wire_plus() { + let mut request = reqwest::Client::new() + .get("https://rest.example.com/v1/namespaces") + .query(&[("warehouse", "my wh")]) + .build() + .unwrap(); + // reqwest wrote the space as `+`, so the canonical query must carry + // it as `%2B` — form-decoding it to `%20` would break the signature. + assert!(request.url().query().unwrap().contains("my+wh")); + let signer = SigV4Signer::new( + AwsCredentials { + access_key_id: "ak".to_string(), + secret_access_key: "sk".to_string().into(), + session_token: None, + }, + "us-east-1".to_string(), + "execute-api".to_string(), + PayloadHashMode::StandardAws, + ); + signer.sign(&mut request).unwrap(); + let signed_headers = request + .headers() + .get("authorization") + .unwrap() + .to_str() + .unwrap() + .to_string(); + assert!(signed_headers.contains("SignedHeaders=")); + // The wire query is untouched by signing. + assert!(request.url().query().unwrap().contains("my+wh")); + } + + #[test] + fn test_normalize_path() { + assert_eq!(normalize_path(""), "/"); + assert_eq!(normalize_path("/"), "/"); + assert_eq!(normalize_path("//v1/config"), "/v1/config"); + assert_eq!(normalize_path("/a/./b/../c"), "/a/c"); + assert_eq!(normalize_path("/a/b/"), "/a/b/"); + } + + #[test] + fn content_sha256_header_iceberg_mode() { + let v = content_sha256_header(b"hello", PayloadHashMode::IcebergRest); + assert_eq!(v, "LPJNul+wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ="); + let e = content_sha256_header(b"", PayloadHashMode::IcebergRest); + assert_eq!(e, EMPTY_HEX); + } + + #[test] + fn content_sha256_header_standard_mode() { + let v = content_sha256_header(b"hello", PayloadHashMode::StandardAws); + assert_eq!( + v, + "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824" + ); + } + + #[test] + fn signing_key_and_signature_match_aws_vector() { + let secret = "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY"; + let date = "20150830"; + let region = "us-east-1"; + let service = "service"; + let key = signing_key(secret, date, region, service); + + let string_to_sign = "AWS4-HMAC-SHA256\n\ +20150830T123600Z\n\ +20150830/us-east-1/service/aws4_request\n\ +bb579772317eb040ac9ed261061d46c1f17a8133879d6129b6e1c25292927e63"; + let sig = hex_hmac_sha256(&key, string_to_sign.as_bytes()); + assert_eq!( + sig, + "5fa00fa31553b73ebf1942676e86291e8372ff2a2260956d9b8aae1d763fbf31" + ); + } + + #[test] + fn canonical_request_get_vanilla() { + let headers = vec![ + ("host".to_string(), "example.amazonaws.com".to_string()), + ("x-amz-date".to_string(), "20150830T123600Z".to_string()), + ]; + let creq = canonical_request("GET", "/", "", &headers, &hex_sha256(b"")); + let expected = "GET\n/\n\n\ +host:example.amazonaws.com\n\ +x-amz-date:20150830T123600Z\n\ +\n\ +host;x-amz-date\n\ +e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; + assert_eq!(creq, expected); + assert_eq!( + hex_sha256(creq.as_bytes()), + "bb579772317eb040ac9ed261061d46c1f17a8133879d6129b6e1c25292927e63" + ); + } + + #[test] + fn signs_request_iceberg_mode() { + let creds = AwsCredentials { + access_key_id: "AKIDEXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" + .to_string() + .into(), + session_token: Some("SESSIONTOKEN".to_string().into()), + }; + let signer = SigV4Signer::new( + creds, + "us-east-1".into(), + "glue".into(), + PayloadHashMode::IcebergRest, + ); + let client = reqwest::Client::new(); + let mut req = client + .post("https://rest.example.com/v1/namespaces") + .body("{}") + .build() + .unwrap(); + + signer.sign(&mut req).unwrap(); + + let h = req.headers(); + assert!( + h.get("authorization") + .unwrap() + .to_str() + .unwrap() + .starts_with("AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/") + ); + assert!(h.contains_key("x-amz-date")); + assert_eq!(h.get("x-amz-security-token").unwrap(), "SESSIONTOKEN"); + let csha = h.get("x-amz-content-sha256").unwrap().to_str().unwrap(); + assert_eq!(csha, "RBNvo1WzZ4oRRq0W9+hknpT7T8If536DEMBg9hyq/4o="); + } + + #[test] + fn string_to_sign_get_vanilla() { + let creq_hash = "bb579772317eb040ac9ed261061d46c1f17a8133879d6129b6e1c25292927e63"; + let headers = vec![ + ("host".to_string(), "example.amazonaws.com".to_string()), + ("x-amz-date".to_string(), "20150830T123600Z".to_string()), + ]; + let creq = canonical_request("GET", "/", "", &headers, &hex_sha256(b"")); + assert_eq!(hex_sha256(creq.as_bytes()), creq_hash); + let sts = string_to_sign( + "20150830T123600Z", + "20150830/us-east-1/service/aws4_request", + &creq, + ); + assert!(sts.ends_with(creq_hash)); + assert!(sts.starts_with("AWS4-HMAC-SHA256\n20150830T123600Z\n")); + } + + /// Empty body uses the hex constant and existing headers are signed too + /// (mirrors Java's `TestRESTSigV4AuthSession::authenticateWithoutBody`). + #[test] + fn signs_empty_body_and_all_headers() { + let creds = AwsCredentials { + access_key_id: "AKIDEXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" + .to_string() + .into(), + session_token: None, + }; + let signer = SigV4Signer::new( + creds, + "us-east-1".into(), + "glue".into(), + PayloadHashMode::IcebergRest, + ); + let client = reqwest::Client::new(); + let mut req = client + .get("https://rest.example.com/v1/config") + .header("content-type", "application/json") + .header("content-encoding", "gzip") + .build() + .unwrap(); + + signer.sign(&mut req).unwrap(); + + let h = req.headers(); + assert_eq!(h.get("x-amz-content-sha256").unwrap(), EMPTY_HEX); + assert!(!h.contains_key("x-amz-security-token")); + let auth = h.get("authorization").unwrap().to_str().unwrap(); + assert!(auth.starts_with("AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/")); + assert!(auth.contains( + "SignedHeaders=content-encoding;content-type;host;x-amz-content-sha256;x-amz-date" + )); + } + + /// The signed `host` must include an explicit non-default port, matching + /// what reqwest/hyper put on the wire and what the AWS SDK signs. + #[test] + fn signs_host_with_non_default_port() { + use chrono::TimeZone; + + let creds = AwsCredentials { + access_key_id: "AKIDEXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" + .to_string() + .into(), + session_token: None, + }; + let signer = SigV4Signer::new( + creds, + "us-east-1".into(), + "glue".into(), + PayloadHashMode::IcebergRest, + ); + let client = reqwest::Client::new(); + let mut req = client + .get("https://rest.example.com:8181/v1/config") + .build() + .unwrap(); + let now = Utc.with_ymd_and_hms(2015, 8, 30, 12, 36, 0).unwrap(); + + signer.sign_at(&mut req, now).unwrap(); + + // Independently recompute the signature with `host:port` in the + // canonical request; a signer that drops the port won't match. + let headers = vec![ + ("host".to_string(), "rest.example.com:8181".to_string()), + ("x-amz-content-sha256".to_string(), EMPTY_HEX.to_string()), + ("x-amz-date".to_string(), "20150830T123600Z".to_string()), + ]; + let creq = canonical_request("GET", "/v1/config", "", &headers, EMPTY_HEX); + let sts = string_to_sign( + "20150830T123600Z", + "20150830/us-east-1/glue/aws4_request", + &creq, + ); + let key = signing_key( + "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY", + "20150830", + "us-east-1", + "glue", + ); + let expected = hex_hmac_sha256(&key, sts.as_bytes()); + + let auth = req + .headers() + .get("authorization") + .unwrap() + .to_str() + .unwrap(); + assert!(auth.ends_with(&format!("Signature={expected}")), "{auth}"); + } + + /// AWS SDK v2 parity (`doubleUrlEncode`): the canonical URI encodes the + /// serialized path once more — literal `,` becomes `%2C`, an encoded + /// `%2C` becomes `%252C` — while plain paths stay byte-identical. + #[test] + fn canonical_uri_is_aws_double_encoded() { + use chrono::TimeZone; + + assert_eq!(uri_encode("/v1/namespaces", false), "/v1/namespaces"); + assert_eq!( + uri_encode("/v1/namespaces/a%2Cb/tables/x,y", false), + "/v1/namespaces/a%252Cb/tables/x%2Cy" + ); + + let creds = AwsCredentials { + access_key_id: "AKIDEXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" + .to_string() + .into(), + session_token: None, + }; + let signer = SigV4Signer::new( + creds, + "us-east-1".into(), + "glue".into(), + PayloadHashMode::IcebergRest, + ); + let client = reqwest::Client::new(); + let mut req = client + .get("https://rest.example.com/v1/namespaces/a%2Cb/tables/x,y") + .build() + .unwrap(); + let now = Utc.with_ymd_and_hms(2015, 8, 30, 12, 36, 0).unwrap(); + + signer.sign_at(&mut req, now).unwrap(); + + let headers = vec![ + ("host".to_string(), "rest.example.com".to_string()), + ("x-amz-content-sha256".to_string(), EMPTY_HEX.to_string()), + ("x-amz-date".to_string(), "20150830T123600Z".to_string()), + ]; + let creq = canonical_request( + "GET", + "/v1/namespaces/a%252Cb/tables/x%2Cy", + "", + &headers, + EMPTY_HEX, + ); + let sts = string_to_sign( + "20150830T123600Z", + "20150830/us-east-1/glue/aws4_request", + &creq, + ); + let key = signing_key( + "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY", + "20150830", + "us-east-1", + "glue", + ); + let expected = hex_hmac_sha256(&key, sts.as_bytes()); + + let auth = req + .headers() + .get("authorization") + .unwrap() + .to_str() + .unwrap(); + assert!(auth.ends_with(&format!("Signature={expected}")), "{auth}"); + } + + #[test] + fn signs_request_standard_mode_uses_hex_header() { + let creds = AwsCredentials { + access_key_id: "AKIDEXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" + .to_string() + .into(), + session_token: None, + }; + let signer = SigV4Signer::new( + creds, + "us-east-1".into(), + "glue".into(), + PayloadHashMode::StandardAws, + ); + let client = reqwest::Client::new(); + let mut req = client + .post("https://rest.example.com/v1/namespaces") + .body("hello") + .build() + .unwrap(); + + signer.sign(&mut req).unwrap(); + + // StandardAws keeps the header in hex. + assert_eq!( + req.headers().get("x-amz-content-sha256").unwrap(), + "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824" + ); + } +} diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 955128d01d..ab07eb0274 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -28,8 +28,8 @@ use iceberg::encryption::kms::{KeyManagementClient, KmsClientFactory}; use iceberg::io::{FileIO, FileIOBuilder, StorageFactory}; use iceberg::table::Table; use iceberg::{ - Catalog, CatalogBuilder, Error, ErrorKind, Namespace, NamespaceIdent, Result, Runtime, - TableCommit, TableCreation, TableIdent, + Catalog, CatalogBuilder, Credential, Error, ErrorKind, Namespace, NamespaceIdent, Result, + Runtime, TableCommit, TableCreation, TableIdent, }; use itertools::Itertools; use reqwest::header::{ @@ -40,7 +40,8 @@ use tokio::sync::OnceCell; use typed_builder::TypedBuilder; use crate::auth::{ - AUTH_TYPE_NONE, AUTH_TYPE_OAUTH2, AuthManager, AuthSession, NoopAuthManager, OAuth2Manager, + AUTH_TYPE_NONE, AUTH_TYPE_OAUTH2, AUTH_TYPE_SIGV4, AuthManager, AuthSession, AwsCredentials, + NoopAuthManager, OAuth2Manager, PayloadHashMode, SigV4AuthManager, SigV4Signer, }; use crate::client::{ HttpClient, deserialize_catalog_response, deserialize_unexpected_catalog_error, @@ -59,11 +60,28 @@ pub const REST_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Disable header redaction in error logs and `Debug` output (defaults to /// false for security) pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str = "disable-header-redaction"; -/// Authentication scheme: `none` or `oauth2`. When unset, `oauth2` is used -/// if a `token`, `credential` or `oauth2-server-uri` is configured, `none` -/// otherwise. +/// Authentication scheme: `none`, `oauth2` or `sigv4`. When unset: `sigv4` +/// if `rest.sigv4-enabled` is true, `oauth2` if a `token`, `credential` or +/// `oauth2-server-uri` is configured, `none` otherwise. pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type"; +/// Enable AWS SigV4 request signing for the REST catalog. +pub const REST_CATALOG_PROP_SIGV4_ENABLED: &str = "rest.sigv4-enabled"; +/// SigV4 signing service name (defaults to `execute-api`, as Iceberg Java). +pub const REST_CATALOG_PROP_SIGNING_NAME: &str = "rest.signing-name"; +/// SigV4 signing region (required for SigV4 signing). +pub const REST_CATALOG_PROP_SIGNING_REGION: &str = "rest.signing-region"; +/// SigV4 access key id (set together with the secret; otherwise the +/// `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY` env vars are used). +pub const REST_CATALOG_PROP_ACCESS_KEY_ID: &str = "rest.access-key-id"; +/// SigV4 secret access key (see [`REST_CATALOG_PROP_ACCESS_KEY_ID`]). +pub const REST_CATALOG_PROP_SECRET_ACCESS_KEY: &str = "rest.secret-access-key"; +/// SigV4 session token (optional; from the env only with env credentials). +pub const REST_CATALOG_PROP_SESSION_TOKEN: &str = "rest.session-token"; +/// Auth scheme SigV4 wraps: `oauth2` (the default; attaches nothing until a +/// token or credential is available) or `none` to disable delegate auth. +pub const REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE: &str = "rest.auth.sigv4.delegate-auth-type"; + const ICEBERG_REST_SPEC_VERSION: &str = "0.14.1"; const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); const PATH_V1: &str = "v1"; @@ -301,9 +319,25 @@ impl RestCatalogConfig { /// is shared across every user of this config (and its clones), so token /// and catalog requests keep sharing one connection pool. pub(crate) fn client(&self) -> Client { - self.client - .clone() - .unwrap_or_else(|| self.default_client.get_or_init(Client::default).clone()) + self.client.clone().unwrap_or_else(|| { + self.default_client + .get_or_init(|| { + // A signed request can't be transparently re-followed + // (Java re-signs every hop), and reqwest's cross-origin + // strip list doesn't know relocated or custom auth + // headers. Surface the 3xx instead; an injected client + // should disable redirects too. + if self.auth_manager.is_some() || self.auth_type() == AUTH_TYPE_SIGV4 { + Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .expect("failed to build default HTTP client") + } else { + Client::default() + } + }) + .clone() + }) } /// Get the token from the config. @@ -359,12 +393,25 @@ impl RestCatalogConfig { .unwrap_or(false) } - /// The configured auth scheme: explicit `rest.auth.type` (matched - /// case-insensitively) when set; - /// otherwise `oauth2` when a `token`, `credential` or `oauth2-server-uri` - /// is configured (preserving pre-`rest.auth.type` setups), `none` when - /// none is. + fn sigv4_enabled(&self) -> bool { + self.props + .get(REST_CATALOG_PROP_SIGV4_ENABLED) + .map(|v| v.eq_ignore_ascii_case("true")) + .unwrap_or(false) + } + + /// Explicit `rest.auth.type` (matched case-insensitively), the legacy + /// `rest.sigv4-enabled` switch, `oauth2` when a `token`, `credential` or + /// `oauth2-server-uri` is configured, `none` otherwise. fn auth_type(&self) -> String { + // The legacy switch wins over `rest.auth.type`, as it does in Java. + if self.sigv4_enabled() { + tracing::warn!( + "'{REST_CATALOG_PROP_SIGV4_ENABLED}' is deprecated; set \ + '{REST_CATALOG_PROP_AUTH_TYPE}={AUTH_TYPE_SIGV4}' instead" + ); + return AUTH_TYPE_SIGV4.to_string(); + } self.props .get(REST_CATALOG_PROP_AUTH_TYPE) // Matched case-insensitively, as the other flag properties are. @@ -399,6 +446,32 @@ impl RestCatalogConfig { match auth_type.as_str() { AUTH_TYPE_NONE => Ok(Arc::new(NoopAuthManager)), AUTH_TYPE_OAUTH2 => Ok(Arc::new(OAuth2Manager::from_config(self)?)), + AUTH_TYPE_SIGV4 => { + // SigV4 signs on top of a delegate auth (Java parity). The + // delegate defaults to OAuth2 even without a token/credential: + // it attaches nothing then, but still picks up a token the + // server's config response supplies. + let delegate_auth_type = self + .props + .get(REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE) + .map(|auth_type| auth_type.to_ascii_lowercase()); + let delegate: Arc = match delegate_auth_type.as_deref() { + Some(AUTH_TYPE_NONE) => Arc::new(NoopAuthManager), + Some(AUTH_TYPE_OAUTH2) | None => Arc::new(OAuth2Manager::from_config(self)?), + Some(other) => { + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "unknown '{REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE}': {other}" + ), + )); + } + }; + Ok(Arc::new(SigV4AuthManager::from_config_signer( + delegate, + self.build_sigv4_signer()?, + ))) + } other => Err(Error::new( ErrorKind::DataInvalid, format!( @@ -410,6 +483,10 @@ impl RestCatalogConfig { } } + fn build_sigv4_signer(&self) -> Result { + sigv4_signer_from_props(&self.props) + } + /// Merge the `RestCatalogConfig` with the a [`CatalogConfig`] (fetched from the REST server). pub(crate) fn merge_with_config(mut self, mut config: CatalogConfig) -> Self { if let Some(uri) = config.overrides.remove("uri") { @@ -430,6 +507,82 @@ impl RestCatalogConfig { } } +/// Builds a [`SigV4Signer`] from the SigV4 properties (credentials fall back +/// to the standard `AWS_*` environment variables). +/// +/// Runs on the user properties at construction and on the merged properties +/// after the config handshake, so server-supplied values are honored. +pub(crate) fn sigv4_signer_from_props(props: &HashMap) -> Result { + let non_blank = |value: &String| { + let trimmed = value.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) + }; + let region = props + .get(REST_CATALOG_PROP_SIGNING_REGION) + .and_then(non_blank) + .ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("'{REST_CATALOG_PROP_SIGNING_REGION}' is required for SigV4 signing"), + ) + })?; + let name = props + .get(REST_CATALOG_PROP_SIGNING_NAME) + .and_then(non_blank) + // Iceberg Java's REST_SIGNING_NAME_DEFAULT. + .unwrap_or_else(|| "execute-api".to_string()); + + // Blank values are treated as absent rather than as usable credentials. + let non_blank = |value: &String| { + let trimmed = value.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) + }; + let prop = |key: &str| props.get(key).and_then(non_blank); + let env = |key: &str| std::env::var(key).ok().as_ref().and_then(non_blank); + + // The credential tuple comes from ONE source: mixing an explicit key with + // ambient environment credentials would cross principals. + let credentials = match ( + prop(REST_CATALOG_PROP_ACCESS_KEY_ID), + prop(REST_CATALOG_PROP_SECRET_ACCESS_KEY), + ) { + (Some(access_key_id), Some(secret_access_key)) => AwsCredentials { + access_key_id, + secret_access_key: secret_access_key.into(), + session_token: prop(REST_CATALOG_PROP_SESSION_TOKEN).map(Credential::from), + }, + (None, None) => match (env("AWS_ACCESS_KEY_ID"), env("AWS_SECRET_ACCESS_KEY")) { + (Some(access_key_id), Some(secret_access_key)) => AwsCredentials { + access_key_id, + secret_access_key: secret_access_key.into(), + session_token: env("AWS_SESSION_TOKEN").map(Credential::from), + }, + _ => { + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "missing SigV4 credentials: set '{REST_CATALOG_PROP_ACCESS_KEY_ID}'/'{REST_CATALOG_PROP_SECRET_ACCESS_KEY}' or the AWS_* env vars" + ), + )); + } + }, + _ => { + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "'{REST_CATALOG_PROP_ACCESS_KEY_ID}' and '{REST_CATALOG_PROP_SECRET_ACCESS_KEY}' must be set together" + ), + )); + } + }; + Ok(SigV4Signer::new( + credentials, + region, + name, + PayloadHashMode::IcebergRest, + )) +} + /// Parses the `credential` property. /// /// ## Output @@ -1833,6 +1986,120 @@ mod tests { list_ns_mock.assert_async().await; } + #[tokio::test] + async fn test_sigv4_signs_requests() { + let mut server = Server::new_async().await; + + let config_mock = create_config_mock(&mut server).await; + + // With sigv4 enabled, requests must carry an AWS SigV4 Authorization header. + let list_ns_mock = server + .mock("GET", "/v1/namespaces") + .match_header( + "authorization", + mockito::Matcher::Regex("^AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/".to_string()), + ) + .with_body(r#"{"namespaces": []}"#) + .create_async() + .await; + + let props = HashMap::from([ + ( + REST_CATALOG_PROP_SIGV4_ENABLED.to_string(), + "true".to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_REGION.to_string(), + "us-east-1".to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_NAME.to_string(), + "glue".to_string(), + ), + ( + REST_CATALOG_PROP_ACCESS_KEY_ID.to_string(), + "AKIDEXAMPLE".to_string(), + ), + ( + REST_CATALOG_PROP_SECRET_ACCESS_KEY.to_string(), + "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY".to_string(), + ), + ]); + let catalog = RestCatalog::new( + RestCatalogConfig::builder() + .uri(server.url()) + .props(props) + .build(), + None, + Runtime::current(), + None, + ); + + let namespaces = catalog.list_namespaces(None).await.unwrap(); + assert!(namespaces.is_empty()); + + config_mock.assert_async().await; + list_ns_mock.assert_async().await; + } + + #[tokio::test] + async fn test_sigv4_composes_with_token_auth() { + // SigV4 signs on top of token auth: the bearer token is relocated to + // `Original-Authorization` and the signature takes `Authorization`. + let props = HashMap::from([ + ( + REST_CATALOG_PROP_SIGV4_ENABLED.to_string(), + "true".to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_REGION.to_string(), + "us-east-1".to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_NAME.to_string(), + "glue".to_string(), + ), + ( + REST_CATALOG_PROP_ACCESS_KEY_ID.to_string(), + "AKIDEXAMPLE".to_string(), + ), + ( + REST_CATALOG_PROP_SECRET_ACCESS_KEY.to_string(), + "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY".to_string(), + ), + ("token".to_string(), "some-oauth-token".to_string()), + ]); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + + let session = config + .resolve_auth_manager() + .unwrap() + .init_session(&Client::new(), &HashMap::new()) + .await + .unwrap(); + let mut req = Client::new() + .get("https://rest.example.com/v1/config") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + + let headers = req.headers(); + assert_eq!( + headers.get("original-authorization").unwrap(), + "Bearer some-oauth-token" + ); + let auth = headers.get("authorization").unwrap().to_str().unwrap(); + assert!(auth.starts_with("AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/")); + // The relocated token is part of the signature. + assert!(auth.contains("original-authorization")); + } + #[tokio::test] async fn test_auth_type_none_disables_auth() { // An explicit `rest.auth.type=none` wins over a configured token. @@ -1863,15 +2130,15 @@ mod tests { } #[tokio::test] - async fn test_header_prop_overrides_token_on_the_wire() { - // Pre-AuthManager behavior, preserved: extra headers are applied after - // authentication, so a user-configured `header.authorization` wins - // over a configured token. + async fn test_token_overrides_header_prop_on_the_wire() { + // Deliberate change with the auth managers: authentication is applied + // after extra headers (a SigV4 session must sign the final header + // set), so a configured token now wins over `header.authorization`. let mut server = Server::new_async().await; let config_mock = create_config_mock(&mut server).await; let list_ns_mock = server .mock("GET", "/v1/namespaces") - .match_header("authorization", "Basic xyz") + .match_header("authorization", "Bearer some-oauth-token") .with_body(r#"{"namespaces": []}"#) .create_async() .await; @@ -2520,6 +2787,288 @@ mod tests { assert!(err.message().contains(REST_CATALOG_PROP_AUTH_TYPE)); } + #[test] + fn test_legacy_sigv4_switch_wins_over_auth_type() { + // Java's AuthManagers gives the deprecated switch precedence, so a + // config carrying both still signs. + let mut props = sigv4_props(); + props.insert( + REST_CATALOG_PROP_AUTH_TYPE.to_string(), + AUTH_TYPE_OAUTH2.to_string(), + ); + props.insert( + REST_CATALOG_PROP_SIGV4_ENABLED.to_string(), + "true".to_string(), + ); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + assert!( + format!("{:?}", config.resolve_auth_manager().unwrap()).contains("SigV4AuthManager") + ); + } + + #[test] + fn test_sigv4_signing_props_resolution() { + // Missing region. + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(HashMap::from([( + REST_CATALOG_PROP_SIGV4_ENABLED.to_string(), + "true".to_string(), + )])) + .build(); + let err = config.resolve_auth_manager().unwrap_err(); + assert!(err.message().contains(REST_CATALOG_PROP_SIGNING_REGION)); + + // The signing name defaults to `execute-api` (Iceberg Java parity). + let mut props = sigv4_props(); + props.remove(REST_CATALOG_PROP_SIGNING_NAME); + let signer = sigv4_signer_from_props(&props).unwrap(); + assert!(format!("{signer:?}").contains("execute-api")); + + // An access key without its secret is rejected, never mixed with + // environment credentials. + let mut props = sigv4_props(); + props.remove(REST_CATALOG_PROP_SECRET_ACCESS_KEY); + let err = sigv4_signer_from_props(&props).unwrap_err(); + assert!(err.message().contains("must be set together")); + + // A blank value is absent, not a usable credential. + let mut props = sigv4_props(); + props.insert( + REST_CATALOG_PROP_ACCESS_KEY_ID.to_string(), + " ".to_string(), + ); + let err = sigv4_signer_from_props(&props).unwrap_err(); + assert!(err.message().contains("must be set together")); + } + + #[test] + fn test_sigv4_prop_names_match_iceberg_java() { + // Iceberg Java's AwsProperties values; renaming any of these breaks + // portable configurations. + assert_eq!(REST_CATALOG_PROP_SIGNING_NAME, "rest.signing-name"); + assert_eq!(REST_CATALOG_PROP_SIGNING_REGION, "rest.signing-region"); + assert_eq!(REST_CATALOG_PROP_ACCESS_KEY_ID, "rest.access-key-id"); + assert_eq!( + REST_CATALOG_PROP_SECRET_ACCESS_KEY, + "rest.secret-access-key" + ); + assert_eq!(REST_CATALOG_PROP_SESSION_TOKEN, "rest.session-token"); + } + + #[tokio::test] + async fn test_sigv4_default_client_does_not_follow_redirects() { + // The default client must surface the 3xx instead of re-sending a + // stale signature (and relocated auth headers) to the target. + let mut server = Server::new_async().await; + let redirect_mock = server + .mock("GET", "/v1/config") + .with_status(302) + .with_header("location", "http://localhost/nowhere") + .expect(2) + .create_async() + .await; + + let config = RestCatalogConfig::builder() + .uri(server.url()) + .props(sigv4_props()) + .build(); + let response = config + .client() + .get(format!("{}/v1/config", server.url())) + .send() + .await + .unwrap(); + assert_eq!(response.status(), 302); + + // Same for an injected manager without SigV4 properties. + let config = RestCatalogConfig::builder() + .uri(server.url()) + .auth_manager(Some(Arc::new(NoopAuthManager))) + .build(); + let response = config + .client() + .get(format!("{}/v1/config", server.url())) + .send() + .await + .unwrap(); + assert_eq!(response.status(), 302); + + redirect_mock.assert_async().await; + } + + fn sigv4_props() -> HashMap { + HashMap::from([ + ( + REST_CATALOG_PROP_AUTH_TYPE.to_string(), + AUTH_TYPE_SIGV4.to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_REGION.to_string(), + "us-east-1".to_string(), + ), + ( + REST_CATALOG_PROP_SIGNING_NAME.to_string(), + "execute-api".to_string(), + ), + ( + REST_CATALOG_PROP_ACCESS_KEY_ID.to_string(), + "ak".to_string(), + ), + ( + REST_CATALOG_PROP_SECRET_ACCESS_KEY.to_string(), + "sk".to_string(), + ), + ]) + } + + #[test] + fn test_sigv4_delegate_auth_type_explicit() { + // `none` forces a Noop delegate even when a token is configured. + let mut props = sigv4_props(); + props.insert("token".to_string(), "tok".to_string()); + props.insert( + REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE.to_string(), + AUTH_TYPE_NONE.to_string(), + ); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + let manager = config.resolve_auth_manager().unwrap(); + assert!(format!("{manager:?}").contains("NoopAuthManager")); + + // `oauth2` forces an OAuth2 delegate even without a token/credential. + let mut props = sigv4_props(); + props.insert( + REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE.to_string(), + AUTH_TYPE_OAUTH2.to_string(), + ); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + let manager = config.resolve_auth_manager().unwrap(); + assert!(format!("{manager:?}").contains("OAuth2Manager")); + + // Unknown delegate types are rejected. + let mut props = sigv4_props(); + props.insert( + REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE.to_string(), + "kerberos".to_string(), + ); + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(props) + .build(); + let err = config.resolve_auth_manager().unwrap_err(); + assert!( + err.message() + .contains(REST_CATALOG_PROP_SIGV4_DELEGATE_AUTH_TYPE) + ); + } + + #[tokio::test] + async fn test_sigv4_catalog_session_rebuilds_signer_from_merged_props() { + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(sigv4_props()) + .build(); + let manager = config.resolve_auth_manager().unwrap(); + + // A config-built signer follows the merged props: a server-supplied + // signing default/override is honored. + let mut merged = sigv4_props(); + merged.insert( + REST_CATALOG_PROP_SIGNING_REGION.to_string(), + "eu-west-1".to_string(), + ); + let session = manager + .catalog_session(&Client::new(), &merged) + .await + .unwrap(); + assert!(format!("{session:?}").contains("eu-west-1")); + + // An injected signer is never rebuilt from the merged props: signing + // must keep its credentials, region and payload-hash mode. + let manager = SigV4AuthManager::new( + Arc::new(NoopAuthManager), + SigV4Signer::new( + AwsCredentials { + access_key_id: "injected-ak".to_string(), + secret_access_key: "injected-sk".to_string().into(), + session_token: None, + }, + "us-west-2".to_string(), + "execute-api".to_string(), + PayloadHashMode::StandardAws, + ), + ); + let session = manager + .catalog_session(&Client::new(), &merged) + .await + .unwrap(); + + let mut req = Client::new() + .post("https://rest.example.com/v1/namespaces") + .body("{}") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + let authorization = req + .headers() + .get("authorization") + .unwrap() + .to_str() + .unwrap(); + assert!(authorization.contains("Credential=injected-ak/")); + assert!(authorization.contains("/us-west-2/execute-api/")); + // StandardAws keeps the hex payload hash (IcebergRest would base64 it). + assert_eq!( + req.headers().get("x-amz-content-sha256").unwrap(), + "44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a" + ); + } + + #[tokio::test] + async fn test_sigv4_default_delegate_picks_up_server_token() { + // The delegate defaults to OAuth2 even without a token/credential: + // a server-supplied token is attached, not dropped by a Noop. + let config = RestCatalogConfig::builder() + .uri("http://localhost".to_string()) + .props(sigv4_props()) + .build(); + let manager = config.resolve_auth_manager().unwrap(); + assert!(format!("{manager:?}").contains("OAuth2Manager")); + + let mut merged = sigv4_props(); + merged.insert("token".to_string(), "srv-tok".to_string()); + let session = manager + .catalog_session(&Client::new(), &merged) + .await + .unwrap(); + + let mut req = Client::new() + .get("https://rest.example.com/v1/namespaces") + .build() + .unwrap(); + session + .authenticate(&mut HttpRequest::new(&mut req)) + .await + .unwrap(); + assert_eq!( + req.headers().get("original-authorization").unwrap(), + "Bearer srv-tok" + ); + assert!(req.headers().contains_key("authorization")); + } + #[test] fn test_with_auth_manager_overrides_config() { // A custom auth manager takes precedence over `rest.auth.type`. diff --git a/crates/catalog/rest/src/client.rs b/crates/catalog/rest/src/client.rs index c6c1914eeb..ad998896c9 100644 --- a/crates/catalog/rest/src/client.rs +++ b/crates/catalog/rest/src/client.rs @@ -114,12 +114,13 @@ impl HttpClient { mut request: Request, session: &dyn AuthSession, ) -> Result { - // Authenticate first, then apply extra headers, so a configured - // `header.authorization` keeps overriding a token (unchanged behavior). + // Apply extra headers BEFORE authentication so a SigV4 session signs the + // final header set (Java parity). Deliberate change: auth now wins over a + // configured `header.authorization` instead of being clobbered by it. + request.headers_mut().extend(self.extra_headers.clone()); session .authenticate(&mut HttpRequest::new(&mut request)) .await?; - request.headers_mut().extend(self.extra_headers.clone()); Ok(self.client.execute(request).await?) }