diff --git a/crates/policy/src/policy/action.rs b/crates/policy/src/policy/action.rs index 9a237f7ab..83a393b3e 100644 --- a/crates/policy/src/policy/action.rs +++ b/crates/policy/src/policy/action.rs @@ -563,6 +563,31 @@ pub enum AdminAction { } impl AdminAction { + pub(crate) fn is_table_resource_scoped(&self) -> bool { + matches!( + self, + AdminAction::GetTableBucketAction + | AdminAction::SetTableBucketAction + | AdminAction::GetTableNamespaceAction + | AdminAction::SetTableNamespaceAction + | AdminAction::UpdateTableNamespacePropertiesAction + | AdminAction::DeleteTableNamespaceAction + | AdminAction::GetTableAction + | AdminAction::SetTableAction + | AdminAction::CreateTableAction + | AdminAction::RegisterTableAction + | AdminAction::CommitTableAction + | AdminAction::DeleteTableAction + | AdminAction::GetTableLifecycleAction + | AdminAction::SetTableLifecycleAction + | AdminAction::RunTableMaintenanceAction + | AdminAction::GetTableMetadataLocationAction + | AdminAction::SetTableMetadataLocationAction + | AdminAction::GetTableMetadataAction + | AdminAction::SetTableMetadataAction + ) + } + // IsValid - checks if action is valid or not. pub fn is_valid(&self) -> bool { matches!( diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 56a9c816c..05f8d49d9 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -1530,6 +1530,161 @@ mod test { ); } + #[tokio::test] + async fn test_table_admin_action_with_resource_is_limited_to_bucket() -> Result<()> { + use crate::policy::action::{Action, AdminAction}; + + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:GetTableMetadata"], + "Resource": ["arn:aws:s3:::warehouse-a"] + } + ] +} +"#; + + let policy = Policy::parse_config(data.as_bytes())?; + let conditions = HashMap::new(); + let claims = HashMap::new(); + let groups = None; + + let matching_args = Args { + account: "testuser", + groups: &groups, + action: Action::AdminAction(AdminAction::GetTableMetadataAction), + bucket: "warehouse-a", + conditions: &conditions, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + assert!( + policy.is_allowed(&matching_args).await, + "table admin action should allow the explicitly granted warehouse bucket" + ); + + let mismatched_args = Args { + account: "testuser", + groups: &groups, + action: Action::AdminAction(AdminAction::GetTableMetadataAction), + bucket: "warehouse-b", + conditions: &conditions, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + assert!( + !policy.is_allowed(&mismatched_args).await, + "table admin action must not ignore Resource when the request targets a different warehouse bucket" + ); + + Ok(()) + } + + #[tokio::test] + async fn test_table_admin_action_with_not_resource_excludes_bucket() -> Result<()> { + use crate::policy::action::{Action, AdminAction}; + + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:GetTableMetadata"], + "NotResource": ["arn:aws:s3:::warehouse-b"] + } + ] +} +"#; + + let policy = Policy::parse_config(data.as_bytes())?; + let conditions = HashMap::new(); + let claims = HashMap::new(); + let groups = None; + + let allowed_args = Args { + account: "testuser", + groups: &groups, + action: Action::AdminAction(AdminAction::GetTableMetadataAction), + bucket: "warehouse-a", + conditions: &conditions, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + assert!( + policy.is_allowed(&allowed_args).await, + "table admin NotResource should allow a warehouse outside the excluded bucket" + ); + + let excluded_args = Args { + account: "testuser", + groups: &groups, + action: Action::AdminAction(AdminAction::GetTableMetadataAction), + bucket: "warehouse-b", + conditions: &conditions, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + assert!( + !policy.is_allowed(&excluded_args).await, + "table admin NotResource should deny the excluded warehouse bucket" + ); + + Ok(()) + } + + #[tokio::test] + async fn test_non_table_admin_action_keeps_unscoped_resource_behavior() -> Result<()> { + use crate::policy::action::{Action, AdminAction}; + + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:ServerInfo"], + "Resource": ["arn:aws:s3:::warehouse-a"] + } + ] +} +"#; + + let policy = Policy::parse_config(data.as_bytes())?; + let conditions = HashMap::new(); + let claims = HashMap::new(); + let groups = None; + + let args = Args { + account: "testuser", + groups: &groups, + action: Action::AdminAction(AdminAction::ServerInfoAdminAction), + bucket: "warehouse-b", + conditions: &conditions, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + assert!( + policy.is_allowed(&args).await, + "existing non-table admin actions should preserve resource-independent evaluation" + ); + + Ok(()) + } + #[test] fn test_sts_statement_without_resource_is_valid() { let data = r#" diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index c5c52f3a3..c3f4220a1 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -89,6 +89,18 @@ enum ActionFamily { } impl Statement { + fn skips_resource_match_for_args(&self, args: &Args<'_>) -> bool { + if self.is_sts() { + return true; + } + + if !self.is_admin() { + return false; + } + + !matches!(args.action, Action::AdminAction(action) if action.is_table_resource_scoped()) + } + fn is_kms(&self) -> bool { for act in self.actions.iter() { if matches!(act, Action::KmsAction(_)) { @@ -188,8 +200,7 @@ impl Statement { .resources .is_match_with_resolver(&resource, args.conditions, Some(resolver)) .await - && !self.is_admin() - && !self.is_sts() + && !self.skips_resource_match_for_args(args) { return false; } @@ -199,8 +210,7 @@ impl Statement { .not_resources .is_match_with_resolver(&resource, args.conditions, Some(resolver)) .await - && !self.is_admin() - && !self.is_sts() + && !self.skips_resource_match_for_args(args) { return false; } diff --git a/rustfs/src/admin/handlers/table_catalog.rs b/rustfs/src/admin/handlers/table_catalog.rs index 1854910ad..25fe00560 100644 --- a/rustfs/src/admin/handlers/table_catalog.rs +++ b/rustfs/src/admin/handlers/table_catalog.rs @@ -13,7 +13,7 @@ // limitations under the License. use crate::admin::{ - auth::validate_admin_request, + auth::{validate_admin_request, validate_admin_request_with_bucket}, router::{AdminOperation, Operation, S3Router}, }; use crate::auth::{check_key_valid, get_session_token}; @@ -245,6 +245,26 @@ async fn authorize_table_catalog_request(req: &S3Request, action: AdminAct .await } +async fn authorize_table_catalog_warehouse_request(req: &S3Request, warehouse: &str, action: AdminAction) -> S3Result<()> { + let Some(input_cred) = &req.credentials else { + return Err(s3_error!(InvalidRequest, "authentication required")); + }; + + let (cred, owner) = + check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?; + + validate_admin_request_with_bucket( + &req.headers, + &cred, + owner, + false, + vec![Action::AdminAction(action)], + req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)), + warehouse, + ) + .await +} + async fn read_json_body(mut input: Body) -> S3Result { let body = input .store_all_limited(MAX_ADMIN_REQUEST_BODY_SIZE) @@ -683,8 +703,8 @@ pub struct RestListNamespacesHandler {} #[async_trait::async_trait] impl Operation for RestListNamespacesHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::GetTableNamespaceAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableNamespaceAction).await?; let store = table_catalog_store()?; let response = list_namespaces_response(&store, &warehouse).await?; build_json_response(StatusCode::OK, &response) @@ -696,8 +716,8 @@ pub struct RestCreateNamespaceHandler {} #[async_trait::async_trait] impl Operation for RestCreateNamespaceHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::SetTableNamespaceAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::SetTableNamespaceAction).await?; let request = read_json_body::(req.input).await?; let store = table_catalog_store()?; let table_bucket_enabled = table_bucket_enabled_from_metadata(&warehouse).await?; @@ -711,8 +731,8 @@ pub struct RestGetNamespaceHandler {} #[async_trait::async_trait] impl Operation for RestGetNamespaceHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::GetTableNamespaceAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableNamespaceAction).await?; let namespace = namespace_from_params(¶ms)?; let store = table_catalog_store()?; let response = get_namespace_response(&store, &warehouse, &namespace).await?; @@ -725,8 +745,8 @@ pub struct RestDropNamespaceHandler {} #[async_trait::async_trait] impl Operation for RestDropNamespaceHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::DeleteTableNamespaceAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::DeleteTableNamespaceAction).await?; let namespace = namespace_from_params(¶ms)?; let store = table_catalog_store()?; drop_namespace_in_store(&store, &warehouse, &namespace.public_name()).await?; @@ -739,8 +759,8 @@ pub struct RestListTablesHandler {} #[async_trait::async_trait] impl Operation for RestListTablesHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::GetTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableAction).await?; let namespace = namespace_from_params(¶ms)?; let store = table_catalog_store()?; let response = list_tables_response(&store, &warehouse, &namespace).await?; @@ -753,8 +773,8 @@ pub struct RestCreateTableHandler {} #[async_trait::async_trait] impl Operation for RestCreateTableHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::CreateTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::CreateTableAction).await?; let namespace = namespace_from_params(¶ms)?; let request = read_json_body::(req.input).await?; let metadata_backend = table_catalog_backend()?; @@ -771,8 +791,8 @@ pub struct RestRegisterTableHandler {} #[async_trait::async_trait] impl Operation for RestRegisterTableHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::RegisterTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::RegisterTableAction).await?; let namespace = namespace_from_params(¶ms)?; let request = read_json_body::(req.input).await?; let metadata_backend = table_catalog_backend()?; @@ -789,8 +809,8 @@ pub struct RestLoadTableHandler {} #[async_trait::async_trait] impl Operation for RestLoadTableHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::GetTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableMetadataAction).await?; let namespace = namespace_from_params(¶ms)?; let table = table_name_from_params(¶ms)?; let metadata_backend = table_catalog_backend()?; @@ -805,8 +825,8 @@ pub struct RestCommitTableHandler {} #[async_trait::async_trait] impl Operation for RestCommitTableHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::CommitTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::CommitTableAction).await?; let namespace = namespace_from_params(¶ms)?; let table = table_name_from_params(¶ms)?; let request = read_json_body::(req.input).await?; @@ -821,8 +841,8 @@ pub struct RestDropTableHandler {} #[async_trait::async_trait] impl Operation for RestDropTableHandler { async fn call(&self, req: S3Request, params: Params<'_, '_>) -> S3Result> { - authorize_table_catalog_request(&req, AdminAction::DeleteTableAction).await?; let warehouse = warehouse_from_params(¶ms)?; + authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::DeleteTableAction).await?; let namespace = namespace_from_params(¶ms)?; let table = table_name_from_params(¶ms)?; let store = table_catalog_store()?; @@ -871,14 +891,43 @@ mod tests { fn table_catalog_handlers_require_table_admin_actions() { let src = include_str!("table_catalog.rs"); - assert!(src.contains("AdminAction::GetTableCatalogAction")); - assert!(src.contains("AdminAction::GetTableNamespaceAction")); - assert!(src.contains("AdminAction::SetTableNamespaceAction")); - assert!(src.contains("AdminAction::DeleteTableNamespaceAction")); - assert!(src.contains("AdminAction::CreateTableAction")); - assert!(src.contains("AdminAction::RegisterTableAction")); - assert!(src.contains("AdminAction::CommitTableAction")); - assert!(src.contains("AdminAction::DeleteTableAction")); + assert!( + operation_block(src, "GetCatalogConfigHandler") + .contains("authorize_table_catalog_request(&req, AdminAction::GetTableCatalogAction).await?;") + ); + + for (handler, action) in [ + ("RestListNamespacesHandler", "AdminAction::GetTableNamespaceAction"), + ("RestCreateNamespaceHandler", "AdminAction::SetTableNamespaceAction"), + ("RestGetNamespaceHandler", "AdminAction::GetTableNamespaceAction"), + ("RestDropNamespaceHandler", "AdminAction::DeleteTableNamespaceAction"), + ("RestListTablesHandler", "AdminAction::GetTableAction"), + ("RestCreateTableHandler", "AdminAction::CreateTableAction"), + ("RestRegisterTableHandler", "AdminAction::RegisterTableAction"), + ("RestLoadTableHandler", "AdminAction::GetTableMetadataAction"), + ("RestCommitTableHandler", "AdminAction::CommitTableAction"), + ("RestDropTableHandler", "AdminAction::DeleteTableAction"), + ] { + let block = operation_block(src, handler); + assert!( + block.contains(&format!("authorize_table_catalog_warehouse_request(&req, &warehouse, {action}).await?;")), + "{handler} should require {action} with warehouse-scoped auth" + ); + assert!( + !block.contains("authorize_table_catalog_request(&req,"), + "{handler} must not use unscoped table catalog authorization" + ); + } + } + + fn operation_block<'a>(src: &'a str, handler: &str) -> &'a str { + let marker = format!("impl Operation for {handler}"); + let block = src.split_once(&marker).expect("handler impl should exist").1; + let end = block + .find("\npub struct ") + .or_else(|| block.find("\n#[cfg(test)]")) + .unwrap_or(block.len()); + &block[..end] } #[test]