feat(table-catalog): refine table catalog permissions (#3283)

* feat(table-catalog): refine table catalog permissions

* fix(policy): scope table admin resources

---------

Co-authored-by: Henry Guo <marshawcoco@users.noreply.github.com>
Co-authored-by: cxymds <Cxymds@qq.com>
This commit is contained in:
Henry Guo
2026-06-08 22:23:52 +08:00
committed by GitHub
parent f7724d223b
commit 8c3e52efb8
4 changed files with 262 additions and 23 deletions
+25
View File
@@ -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!(
+155
View File
@@ -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#"
+14 -4
View File
@@ -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;
}
+68 -19
View File
@@ -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<Body>, action: AdminAct
.await
}
async fn authorize_table_catalog_warehouse_request(req: &S3Request<Body>, 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::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0)),
warehouse,
)
.await
}
async fn read_json_body<T: DeserializeOwned>(mut input: Body) -> S3Result<T> {
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::GetTableNamespaceAction).await?;
let warehouse = warehouse_from_params(&params)?;
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::SetTableNamespaceAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::SetTableNamespaceAction).await?;
let request = read_json_body::<CreateNamespaceRequest>(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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::GetTableNamespaceAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableNamespaceAction).await?;
let namespace = namespace_from_params(&params)?;
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::DeleteTableNamespaceAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::DeleteTableNamespaceAction).await?;
let namespace = namespace_from_params(&params)?;
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::GetTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableAction).await?;
let namespace = namespace_from_params(&params)?;
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::CreateTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::CreateTableAction).await?;
let namespace = namespace_from_params(&params)?;
let request = read_json_body::<CreateTableRequest>(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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::RegisterTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::RegisterTableAction).await?;
let namespace = namespace_from_params(&params)?;
let request = read_json_body::<RegisterTableRequest>(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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::GetTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::GetTableMetadataAction).await?;
let namespace = namespace_from_params(&params)?;
let table = table_name_from_params(&params)?;
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<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::CommitTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::CommitTableAction).await?;
let namespace = namespace_from_params(&params)?;
let table = table_name_from_params(&params)?;
let request = read_json_body::<RestCommitTableRequest>(req.input).await?;
@@ -821,8 +841,8 @@ pub struct RestDropTableHandler {}
#[async_trait::async_trait]
impl Operation for RestDropTableHandler {
async fn call(&self, req: S3Request<Body>, params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
authorize_table_catalog_request(&req, AdminAction::DeleteTableAction).await?;
let warehouse = warehouse_from_params(&params)?;
authorize_table_catalog_warehouse_request(&req, &warehouse, AdminAction::DeleteTableAction).await?;
let namespace = namespace_from_params(&params)?;
let table = table_name_from_params(&params)?;
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]