CTTY commented on code in PR #2815: URL: https://github.com/apache/iceberg-rust/pull/2815#discussion_r3582522323
########## crates/catalog/rest/src/signing.rs: ########## @@ -0,0 +1,577 @@ +// 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 iceberg::{Error, ErrorKind, Result}; +use reqsign_core::hash::{base64_encode, hex_hmac_sha256, hex_sha256, hmac_sha256}; +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 { Review Comment: these should live in sigv4.rs? ########## crates/catalog/rest/src/catalog.rs: ########## @@ -76,6 +98,7 @@ impl Default for RestCatalogBuilder { warehouse: None, props: HashMap::new(), client: None, + custom_auth_manager: None, Review Comment: let's just name it `auth_manager` ########## crates/catalog/rest/src/client.rs: ########## @@ -58,189 +51,58 @@ impl Debug for HttpClient { impl HttpClient { /// Create a new http client. - pub fn new(cfg: &RestCatalogConfig) -> Result<Self> { - let extra_headers = cfg.extra_headers()?; + pub async fn new(cfg: &RestCatalogConfig) -> Result<Self> { + 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(), + extra_headers: cfg.extra_headers()?, disable_header_redaction: cfg.disable_header_redaction(), + auth_manager, + session, }) } /// Update the http client with new configuration. /// /// 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<Self> { + /// + /// The auth manager is kept; it derives a new session from the merged + /// properties (carrying over state such as a cached token). + pub async fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> { let extra_headers = (!cfg.extra_headers()?.is_empty()) .then(|| cfg.extra_headers()) .transpose()? .unwrap_or(self.extra_headers); + let session = self.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), 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: self.auth_manager, + session, }) } - /// This API is testing only to assert the token. + /// The session authenticating requests in the current phase. + pub(crate) fn session(&self) -> &Arc<dyn AuthSession> { + &self.session + } + + /// This API is testing only to assert the bearer token the session uses. #[cfg(test)] pub(crate) async fn token(&self) -> Option<String> { let mut req = self - .request(Method::GET, &self.token_endpoint) + .request(Method::GET, "http://localhost/unused";) Review Comment: why is this needed? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
