Harden admin and RPC security checks (#2773)

Signed-off-by: 安正超 <anzhengchao@gmail.com> Co-authored-by: loverustfs <hello@rustfs.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-08-07 05:43:14 +00:00 · 2026-05-03 19:55:09 +08:00
parent eb23710d2e
commit 66c38b629d
10 changed files with 490 additions and 64 deletions
@@ -12,17 +12,41 @@
 //  See the License for the specific language governing permissions and
 //  limitations under the License.

-use crate::admin::router::Operation;
+use crate::admin::{auth::validate_admin_request, router::Operation};
+use crate::auth::{check_key_valid, get_session_token};
+use crate::server::RemoteAddr;
 use http::header::CONTENT_TYPE;
 use http::{HeaderMap, StatusCode};
 use matchit::Params;
-use s3s::{Body, S3Request, S3Response, S3Result};
+use rustfs_policy::policy::action::{Action, AdminAction};
+use s3s::{Body, S3Request, S3Response, S3Result, s3_error};
 use tracing::info;

+pub(super) async fn authorize_profile_request(req: &S3Request<Body>) -> S3Result<()> {
+    let Some(input_cred) = req.credentials.as_ref() else {
+        return Err(s3_error!(AccessDenied, "Signature is required"));
+    };
+
+    let (cred, owner) =
+        check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?;
+    let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
+
+    validate_admin_request(
+        &req.headers,
+        &cred,
+        owner,
+        false,
+        vec![Action::AdminAction(AdminAction::ProfilingAdminAction)],
+        remote_addr,
+    )
+    .await
+}
+
 pub struct TriggerProfileCPU {}
 #[async_trait::async_trait]
 impl Operation for TriggerProfileCPU {
-    async fn call(&self, _req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+    async fn call(&self, req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+        authorize_profile_request(&req).await?;
        info!("Triggering CPU profile dump via S3 request...");

        let dur = std::time::Duration::from_secs(60);
@@ -40,7 +64,8 @@ impl Operation for TriggerProfileCPU {
 pub struct TriggerProfileMemory {}
 #[async_trait::async_trait]
 impl Operation for TriggerProfileMemory {
-    async fn call(&self, _req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+    async fn call(&self, req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+        authorize_profile_request(&req).await?;
        info!("Triggering Memory profile dump via S3 request...");

        match crate::profiling::dump_memory_pprof_now().await {
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+use super::profile::authorize_profile_request;
 use crate::admin::router::{AdminOperation, Operation, S3Router};
 use crate::server::ADMIN_PREFIX;
 use http::{HeaderMap, HeaderValue, Uri};
@@ -56,6 +57,8 @@ pub struct ProfileHandler {}
 #[async_trait::async_trait]
 impl Operation for ProfileHandler {
    async fn call(&self, req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+        authorize_profile_request(&req).await?;
+
        #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))]
        {
            let requested_url = req.uri.to_string();
@@ -151,7 +154,9 @@ pub struct ProfileStatusHandler {}

 #[async_trait::async_trait]
 impl Operation for ProfileStatusHandler {
-    async fn call(&self, _req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+    async fn call(&self, req: S3Request<Body>, _params: Params<'_, '_>) -> S3Result<S3Response<(StatusCode, Body)>> {
+        authorize_profile_request(&req).await?;
+
        #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))]
        let message = format!("CPU profiling is not supported on {} platform", std::env::consts::OS);
        #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))]
@@ -204,8 +209,26 @@ impl Operation for ProfileStatusHandler {

 #[cfg(test)]
 mod tests {
-    use super::extract_query_params;
-    use http::Uri;
+    use super::{ProfileHandler, ProfileStatusHandler, extract_query_params};
+    use crate::admin::router::Operation;
+    use http::{Extensions, HeaderMap, Uri};
+    use hyper::Method;
+    use matchit::Params;
+    use s3s::{Body, S3ErrorCode, S3Request};
+
+    fn build_profile_request(uri: &'static str) -> S3Request<Body> {
+        S3Request {
+            input: Body::empty(),
+            method: Method::GET,
+            uri: Uri::from_static(uri),
+            headers: HeaderMap::new(),
+            extensions: Extensions::new(),
+            credentials: None,
+            region: None,
+            service: None,
+            trailing_headers: None,
+        }
+    }

    #[test]
    fn test_extract_query_params_decodes_percent_encoded_values() {
@@ -217,4 +240,32 @@ mod tests {
        assert_eq!(params.get("format"), Some(&"flamegraph".to_string()));
        assert_eq!(params.get("note"), Some(&"a+b value".to_string()));
    }
+
+    #[tokio::test]
+    async fn profile_handler_rejects_missing_credentials() {
+        let result = ProfileHandler {}
+            .call(build_profile_request("/rustfs/admin/debug/pprof/profile?format=protobuf"), Params::new())
+            .await;
+        let err = match result {
+            Ok(_) => panic!("profile handler must reject unauthenticated requests"),
+            Err(err) => err,
+        };
+
+        assert_eq!(err.code(), &S3ErrorCode::AccessDenied);
+        assert_eq!(err.message(), Some("Signature is required"));
+    }
+
+    #[tokio::test]
+    async fn profile_status_handler_rejects_missing_credentials() {
+        let result = ProfileStatusHandler {}
+            .call(build_profile_request("/rustfs/admin/debug/pprof/status"), Params::new())
+            .await;
+        let err = match result {
+            Ok(_) => panic!("profile status handler must reject unauthenticated requests"),
+            Err(err) => err,
+        };
+
+        assert_eq!(err.code(), &S3ErrorCode::AccessDenied);
+        assert_eq!(err.message(), Some("Signature is required"));
+    }
 }
@@ -250,7 +250,12 @@ async fn handle_assume_role(

    new_cred.parent_user = cred.access_key.clone();

-    debug!("AssumeRole get new_cred {:?}", &new_cred);
+    debug!(
+        access_key = %new_cred.access_key,
+        parent_user = %new_cred.parent_user,
+        expiration = ?new_cred.expiration,
+        "AssumeRole generated temporary credentials"
+    );

    let updated_at = iam_store
        .set_temp_user(&new_cred.access_key, &new_cred, None)
@@ -139,6 +139,36 @@ fn imported_service_account_status(status: &str) -> Option<String> {
    None
 }

+const SERVICE_ACCOUNT_PARENT_SCOPE_ERROR: &str = "service account parent is outside requester scope";
+
+fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials, owner: bool) -> bool {
+    if parent.is_empty() {
+        return false;
+    }
+
+    if owner {
+        return true;
+    }
+
+    if requester.is_temp() || requester.is_service_account() {
+        return temp_identity_parent(requester).is_some_and(|requester_parent| requester_parent == parent);
+    }
+
+    requester.parent_user.is_empty() && requester.access_key == parent
+}
+
+fn imported_service_account_parent_scope_failure(
+    access_key: &str,
+    parent: &str,
+    requester: &Credentials,
+    owner: bool,
+) -> Option<IAMErrEntity> {
+    (!imported_service_account_parent_allowed(parent, requester, owner)).then(|| IAMErrEntity {
+        name: access_key.to_string(),
+        error: SERVICE_ACCOUNT_PARENT_SCOPE_ERROR.to_string(),
+    })
+}
+
 pub struct AddUser {}
 #[async_trait::async_trait]
 impl Operation for AddUser {
@@ -984,6 +1014,11 @@ impl Operation for ImportIam {
                        return Err(s3_error!(InvalidArgument, "has space be {ak}"));
                    }

+                    if let Some(err) = imported_service_account_parent_scope_failure(&ak, &req.parent, &cred, owner) {
+                        failed.service_accounts.push(err);
+                        continue;
+                    }
+
                    let mut update = true;

                    if let Err(e) = iam_store.get_service_account(&req.access_key).await {
@@ -1216,8 +1251,9 @@ impl Operation for ImportIam {
 #[cfg(test)]
 mod tests {
    use super::{
-        GROUP_POLICY_MAPPING_USER_TYPE, imported_service_account_status, should_check_deny_only, should_reject_group_import_name,
-        should_restore_group_as_disabled,
+        GROUP_POLICY_MAPPING_USER_TYPE, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR, imported_service_account_parent_allowed,
+        imported_service_account_parent_scope_failure, imported_service_account_status, should_check_deny_only,
+        should_reject_group_import_name, should_restore_group_as_disabled,
    };
    use rustfs_credentials::{Credentials, IAM_POLICY_CLAIM_NAME_SA};
    use rustfs_iam::error::Error as IamError;
@@ -1337,6 +1373,65 @@ mod tests {
        assert!(imported_service_account_status("unknown").is_none());
    }

+    #[test]
+    fn test_import_service_account_parent_rejects_other_parent_for_non_owner() {
+        let requester = Credentials {
+            access_key: "delegated-importer".to_string(),
+            ..Default::default()
+        };
+
+        assert!(!imported_service_account_parent_allowed("root-access-key", &requester, false));
+    }
+
+    #[test]
+    fn test_service_account_parent_scope_failure_records_import_error() {
+        let requester = Credentials {
+            access_key: "delegated-importer".to_string(),
+            ..Default::default()
+        };
+        let err = imported_service_account_parent_scope_failure("svc-access-key", "root-access-key", &requester, false)
+            .expect("non-owner must not import a service account for another parent");
+
+        assert_eq!(err.name, "svc-access-key");
+        assert_eq!(err.error, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR);
+        assert!(
+            imported_service_account_parent_scope_failure("svc-access-key", "delegated-importer", &requester, false).is_none()
+        );
+    }
+
+    #[test]
+    fn test_import_service_account_parent_allows_owner_restore() {
+        let requester = Credentials {
+            access_key: "root-access-key".to_string(),
+            ..Default::default()
+        };
+
+        assert!(imported_service_account_parent_allowed("any-imported-parent", &requester, true));
+    }
+
+    #[test]
+    fn test_import_service_account_parent_allows_requester_self_parent() {
+        let requester = Credentials {
+            access_key: "delegated-importer".to_string(),
+            ..Default::default()
+        };
+
+        assert!(imported_service_account_parent_allowed("delegated-importer", &requester, false));
+    }
+
+    #[test]
+    fn test_import_service_account_parent_allows_derived_requester_parent() {
+        let requester = Credentials {
+            access_key: "derived-access-key".to_string(),
+            parent_user: "parent-user".to_string(),
+            session_token: "session-token".to_string(),
+            ..Default::default()
+        };
+
+        assert!(imported_service_account_parent_allowed("parent-user", &requester, false));
+        assert!(!imported_service_account_parent_allowed("other-parent", &requester, false));
+    }
+
    #[test]
    fn test_service_account_import_accepts_null_groups_and_epoch_expiration() {
        let payload = r#"{
@@ -2331,11 +2331,6 @@ where
        // Allow unauthenticated access to health check
        let path = req.uri.path();

-        // Profiling endpoints
-        if req.method == Method::GET && (path == PROFILE_CPU_PATH || path == PROFILE_MEMORY_PATH) {
-            return Ok(());
-        }
-
        // Health check
        if (req.method == Method::HEAD || req.method == Method::GET) && is_public_health_path(path) {
            return Ok(());
@@ -3747,6 +3742,28 @@ mod tests {
        assert_eq!(err.code(), &S3ErrorCode::AccessDenied);
    }

+    #[tokio::test]
+    async fn check_access_rejects_anonymous_profile_request() {
+        let router: S3Router<AdminOperation> = S3Router::new(false);
+        let mut req = S3Request {
+            input: Body::from(String::new()),
+            method: Method::GET,
+            uri: PROFILE_CPU_PATH.parse().expect("uri should parse"),
+            headers: HeaderMap::new(),
+            extensions: http::Extensions::new(),
+            credentials: None,
+            region: None,
+            service: None,
+            trailing_headers: None,
+        };
+
+        let err = router
+            .check_access(&mut req)
+            .await
+            .expect_err("anonymous profile request must be denied");
+        assert_eq!(err.code(), &S3ErrorCode::AccessDenied);
+    }
+
    #[test]
    fn listen_notification_keepalive_plan_defaults_to_space_keepalive() {
        let uri: Uri = "/demo-bucket?events=s3:ObjectCreated:Put".parse().expect("uri should parse");