-
Notifications
You must be signed in to change notification settings - Fork 532
feat(rest): Support refreshing vended storage credentials #2932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,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"; | ||
| /// Identifier for a server-side scan plan associated with credential requests. | ||
| pub const REST_CATALOG_PROP_SCAN_PLAN_ID: &str = "rest.scan.plan-id"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Server-side planning isn't supported yet so this will always be empty anyway. I don't think it's a problem to keep it until it eventually is supported |
||
|
|
||
| const ICEBERG_REST_SPEC_VERSION: &str = "0.14.1"; | ||
| const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
|
|
@@ -164,7 +166,7 @@ impl RestCatalogBuilder { | |
| } | ||
|
|
||
| /// Rest catalog configuration. | ||
| #[derive(Clone, Debug, TypedBuilder)] | ||
| #[derive(Clone, TypedBuilder)] | ||
| pub(crate) struct RestCatalogConfig { | ||
| #[builder(default, setter(strip_option))] | ||
| name: Option<String>, | ||
|
|
@@ -181,6 +183,19 @@ pub(crate) struct RestCatalogConfig { | |
| client: Option<Client>, | ||
| } | ||
|
|
||
| impl std::fmt::Debug for RestCatalogConfig { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| // Catalog and table properties may contain OAuth credentials, delegated | ||
| // storage credentials, or arbitrary secret-bearing `header.*` values. | ||
| f.debug_struct("RestCatalogConfig") | ||
| .field("name", &self.name) | ||
| .field("uri", &self.uri) | ||
| .field("warehouse", &self.warehouse) | ||
| .field("property_keys", &self.props.keys().collect::<Vec<_>>()) | ||
| .finish_non_exhaustive() | ||
| } | ||
| } | ||
|
|
||
| impl RestCatalogConfig { | ||
| fn url_prefixed(&self, parts: &[&str]) -> String { | ||
| [&self.uri, PATH_V1] | ||
|
|
@@ -358,7 +373,7 @@ impl RestCatalogConfig { | |
|
|
||
| #[derive(Debug)] | ||
| struct RestContext { | ||
| client: HttpClient, | ||
| client: Arc<HttpClient>, | ||
| /// Runtime config is fetched from rest server and stored here. | ||
| /// | ||
| /// It's could be different from the user config. | ||
|
|
@@ -447,7 +462,7 @@ impl RestCatalog { | |
|
|
||
| Ok(RestContext { | ||
| config, | ||
| client, | ||
| client: Arc::new(client), | ||
| endpoints, | ||
| }) | ||
| }) | ||
|
|
@@ -510,17 +525,17 @@ impl RestCatalog { | |
| metadata_location: Option<&str>, | ||
| extra_config: Option<HashMap<String, String>>, | ||
| ) -> Result<FileIO> { | ||
| let mut props = self.context().await?.config.props.clone(); | ||
| let context = self.context().await?; | ||
| let mut props = context.config.props.clone(); | ||
| if let Some(config) = extra_config { | ||
| props.extend(config); | ||
| } | ||
|
|
||
| // If the warehouse is a logical identifier instead of a URL we don't want | ||
| // to raise an exception | ||
| let warehouse_path = match self.context().await?.config.warehouse.as_deref() { | ||
| let warehouse_path = match context.config.warehouse.as_deref() { | ||
| Some(url) if Url::parse(url).is_ok() => Some(url), | ||
| Some(_) => None, | ||
| None => None, | ||
| _ => None, | ||
| }; | ||
|
|
||
| if metadata_location.or(warehouse_path).is_none() { | ||
|
|
@@ -541,9 +556,20 @@ impl RestCatalog { | |
| ) | ||
| })?; | ||
|
|
||
| let file_io = FileIOBuilder::new(factory).with_props(props).build(); | ||
| // If the catalog vends refreshable credentials for this table's storage, | ||
| // attach a provider so the backend re-fetches them before they expire. | ||
| let credential_provider = crate::credential::build_vended_credential_provider( | ||
| context.client.clone(), | ||
| &context.config.uri, | ||
| &props, | ||
| )?; | ||
|
|
||
| let mut builder = FileIOBuilder::new(factory).with_props(props); | ||
| if let Some(provider) = credential_provider { | ||
| builder = builder.with_credential_provider(provider); | ||
| } | ||
|
|
||
| Ok(file_io) | ||
| Ok(builder.build()) | ||
| } | ||
|
|
||
| /// Invalidate the current token without generating a new one. On the next request, the client | ||
|
|
@@ -1048,7 +1074,14 @@ impl Catalog for RestCatalog { | |
| "Metadata location missing in `register_table` response!", | ||
| ))?; | ||
|
|
||
| let file_io = self.load_file_io(Some(metadata_location), None).await?; | ||
| let config = response | ||
| .config | ||
| .into_iter() | ||
| .chain(self.user_config.props.clone()) | ||
| .collect(); | ||
| let file_io = self | ||
| .load_file_io(Some(metadata_location), Some(config)) | ||
| .await?; | ||
|
Comment on lines
+1077
to
+1084
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by fix since this is inconsistent with |
||
|
|
||
| let mut table_builder = Table::builder() | ||
| .identifier(table_ident.clone()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,9 +49,13 @@ pub(crate) struct HttpClient { | |
|
|
||
| impl Debug for HttpClient { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| // Omit the reqwest client: injected clients may carry secret default | ||
| // headers. Explicit headers use the same redaction policy as errors. | ||
| f.debug_struct("HttpClient") | ||
| .field("client", &self.client) | ||
| .field("extra_headers", &self.extra_headers) | ||
| .field( | ||
| "extra_headers", | ||
| &format_headers_redacted(&self.extra_headers, self.disable_header_redaction), | ||
| ) | ||
| .finish_non_exhaustive() | ||
| } | ||
| } | ||
|
|
@@ -71,6 +75,24 @@ impl HttpClient { | |
| }) | ||
| } | ||
|
|
||
| /// Create a client for table-scoped resources while reusing this client's | ||
| /// underlying connection pool. | ||
| /// | ||
| /// A load-table response may supply a table token or `header.*` values that | ||
| /// must be used for subsequent table requests such as credential refresh. | ||
| pub(crate) fn for_table( | ||
| &self, | ||
| catalog_uri: &str, | ||
| props: HashMap<String, String>, | ||
| ) -> Result<Self> { | ||
| let cfg = RestCatalogConfig::builder() | ||
| .uri(catalog_uri.to_string()) | ||
| .props(props) | ||
| .client(Some(self.client.clone())) | ||
| .build(); | ||
| Self::new(&cfg) | ||
| } | ||
|
|
||
| /// Update the http client with new configuration. | ||
| /// | ||
| /// If cfg carries new value, we will use cfg instead. | ||
|
|
@@ -156,7 +178,6 @@ impl HttpClient { | |
| ) | ||
| .with_context("operation", "auth") | ||
| .with_context("url", auth_url.to_string()) | ||
| .with_context("json", String::from_utf8_lossy(&text)) | ||
| .with_source(e) | ||
| })?) | ||
| } else { | ||
|
|
@@ -170,7 +191,6 @@ impl HttpClient { | |
| .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)) | ||
|
|
@@ -278,29 +298,30 @@ pub(crate) async fn deserialize_catalog_response<R: DeserializeOwned>( | |
| let bytes = response.bytes().await?; | ||
|
|
||
| serde_json::from_slice::<R>(&bytes).map_err(|e| { | ||
| // Successful REST responses can contain OAuth tokens and delegated | ||
| // storage credentials. Never copy an unparsable response into an error. | ||
| Error::new( | ||
| ErrorKind::Unexpected, | ||
| "Failed to parse response from rest catalog server", | ||
| ) | ||
| .with_context("json", String::from_utf8_lossy(&bytes)) | ||
| .with_source(e) | ||
| }) | ||
| } | ||
|
|
||
| /// 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. | ||
| 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", | ||
| ] | ||
|
Comment on lines
+314
to
+322
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if we want to do it like this since I guess there could technically be other headers that would be made sensitive. The main change I wanted to do was also make |
||
| .iter() | ||
| .any(|pattern| name_lower.contains(pattern)) | ||
| } | ||
|
|
||
| /// Redacts sensitive headers and returns a debug-formatted string. | ||
|
|
@@ -386,6 +407,8 @@ mod tests { | |
| fn test_format_headers_redacted_filters_sensitive() { | ||
| let mut headers = HeaderMap::new(); | ||
| headers.insert("authorization", "Bearer secret-token".parse().unwrap()); | ||
| headers.insert("x-client-secret", "private-value".parse().unwrap()); | ||
| headers.insert("x-client-credential", "credential-value".parse().unwrap()); | ||
| headers.insert("content-type", "application/json".parse().unwrap()); | ||
|
|
||
| let result = format_headers_redacted(&headers, false); | ||
|
|
@@ -395,6 +418,8 @@ mod tests { | |
| assert!(result.contains("[REDACTED]")); | ||
| // Sensitive value should NOT be present | ||
| assert!(!result.contains("secret-token")); | ||
| assert!(!result.contains("private-value")); | ||
| assert!(!result.contains("credential-value")); | ||
| // Non-sensitive header should be present with actual value | ||
| assert!(result.contains("content-type")); | ||
| assert!(result.contains("application/json")); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we're okay with adding a new dep. It's only added for the jittering functionality when retrying