From 17f0bd26376281996a478e920c7b27720a4e7caa Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Tue, 21 Jul 2026 16:14:06 +0800 Subject: [PATCH] fix(iam): report duplicate access keys clearly (#5066) * fix(iam): report duplicate access keys clearly * fix(iam): narrow duplicate access key handling --- crates/iam/src/error.rs | 7 ++ crates/iam/src/manager.rs | 4 +- crates/iam/src/sys.rs | 76 +++++++++++++++++++- rustfs/src/admin/handlers/iam_error.rs | 12 +++- rustfs/src/admin/handlers/service_account.rs | 10 +-- rustfs/src/admin/handlers/user.rs | 40 +++++++++-- 6 files changed, 134 insertions(+), 15 deletions(-) diff --git a/crates/iam/src/error.rs b/crates/iam/src/error.rs index d434eb28a..6f652590f 100644 --- a/crates/iam/src/error.rs +++ b/crates/iam/src/error.rs @@ -90,6 +90,10 @@ pub enum Error { #[error("invalid access_key")] InvalidAccessKey, + + #[error("access key is already in use")] + AccessKeyAlreadyExists, + #[error("action not allowed")] IAMActionNotAllowed, @@ -159,6 +163,7 @@ impl Clone for Error { Error::NoAccessKey => Error::NoAccessKey, Error::InvalidToken => Error::InvalidToken, Error::InvalidAccessKey => Error::InvalidAccessKey, + Error::AccessKeyAlreadyExists => Error::AccessKeyAlreadyExists, Error::IAMActionNotAllowed => Error::IAMActionNotAllowed, Error::InvalidExpiration => Error::InvalidExpiration, Error::NoSecretKeyWithAccessKey => Error::NoSecretKeyWithAccessKey, @@ -297,6 +302,7 @@ mod tests { Error::NoSuchUser("testuser".to_string()), Error::NoSuchAccount("testaccount".to_string()), Error::InvalidArgument, + Error::AccessKeyAlreadyExists, Error::IAMActionNotAllowed, Error::PolicyTooLarge, Error::ConfigNotFound, @@ -415,6 +421,7 @@ mod tests { (Error::NoSuchUser("testuser".to_string()), "user 'testuser' does not exist"), (Error::NoSuchAccount("testaccount".to_string()), "account 'testaccount' does not exist"), (Error::InvalidArgument, "invalid arguments specified"), + (Error::AccessKeyAlreadyExists, "access key is already in use"), (Error::IAMActionNotAllowed, "action not allowed"), (Error::ConfigNotFound, "config not found"), ]; diff --git a/crates/iam/src/manager.rs b/crates/iam/src/manager.rs index 7589643be..dd8f6ac6f 100644 --- a/crates/iam/src/manager.rs +++ b/crates/iam/src/manager.rs @@ -787,7 +787,7 @@ where if let Some(x) = users.get(&cred.access_key) && x.credentials.is_service_account() { - return Err(Error::IAMActionNotAllowed); + return Err(Error::AccessKeyAlreadyExists); } drop(cache); drop(users); @@ -1336,7 +1336,7 @@ where if let Some(x) = users.get(access_key) { warn!(error = ?x, "IAM user already exists"); if x.credentials.is_temp() { - return Err(Error::IAMActionNotAllowed); + return Err(Error::AccessKeyAlreadyExists); } } drop(cache); diff --git a/crates/iam/src/sys.rs b/crates/iam/src/sys.rs index 7895abe87..34b866997 100644 --- a/crates/iam/src/sys.rs +++ b/crates/iam/src/sys.rs @@ -456,7 +456,7 @@ impl IamSys { } if parent_user == opts.access_key { - return Err(IamError::IAMActionNotAllowed); + return Err(IamError::AccessKeyAlreadyExists); } if opts.access_key == SITE_REPLICATOR_SERVICE_ACCOUNT && !opts.allow_site_replicator_account { @@ -1686,6 +1686,80 @@ mod tests { } } + async fn test_iam_sys() -> IamSys { + let store = StsTestMockStore::new(false); + let cache = IamCache::new(store).await.expect("IAM cache should initialize"); + IamSys::new(cache) + } + + fn service_account_opts(access_key: &str, secret_key: &str) -> NewServiceAccountOpts { + NewServiceAccountOpts { + access_key: access_key.to_string(), + secret_key: secret_key.to_string(), + ..Default::default() + } + } + + #[tokio::test] + async fn new_service_account_rejects_parent_access_key() { + ensure_test_global_credentials(); + + let iam_sys = test_iam_sys().await; + let access_key = "PARENTACCESSKEY123"; + + let err = iam_sys + .new_service_account(access_key, None, service_account_opts(access_key, "parentAccessKeySecret123")) + .await + .expect_err("a service account must not reuse its parent access key"); + + assert_eq!(err, Error::AccessKeyAlreadyExists); + } + + #[tokio::test] + async fn duplicate_service_account_returns_access_key_already_exists() { + ensure_test_global_credentials(); + + let iam_sys = test_iam_sys().await; + let access_key = "DUPLICATESERVICEKEY1"; + + iam_sys + .new_service_account("svc-parent-user", None, service_account_opts(access_key, "firstServiceSecret123")) + .await + .expect("initial service account should be created"); + + let err = iam_sys + .new_service_account("svc-parent-user", None, service_account_opts(access_key, "secondServiceSecret123")) + .await + .expect_err("duplicate service account should fail"); + + assert_eq!(err, Error::AccessKeyAlreadyExists); + } + + #[tokio::test] + async fn user_creation_rejects_service_account_access_key() { + ensure_test_global_credentials(); + + let iam_sys = test_iam_sys().await; + let access_key = "SERVICEACCOUNTKEY123"; + + iam_sys + .new_service_account("svc-parent-user", None, service_account_opts(access_key, "serviceAccountSecret123")) + .await + .expect("service account should be created"); + + let user = AddOrUpdateUserReq { + secret_key: "regularUserSecret123".to_string(), + policy: None, + status: rustfs_madmin::AccountStatus::Enabled, + }; + let err = iam_sys + .create_user(access_key, &user) + .await + .expect_err("user creation must reject a service-account access key"); + + assert_eq!(err, Error::AccessKeyAlreadyExists); + } + #[tokio::test] async fn test_new_service_account_without_expiration_omits_exp_claim() { ensure_test_global_credentials(); diff --git a/rustfs/src/admin/handlers/iam_error.rs b/rustfs/src/admin/handlers/iam_error.rs index 0ec3e3244..3fe3b4394 100644 --- a/rustfs/src/admin/handlers/iam_error.rs +++ b/rustfs/src/admin/handlers/iam_error.rs @@ -23,7 +23,9 @@ pub(crate) fn iam_error_to_s3_error(err: IamError) -> S3Error { | IamError::NoSuchTempAccount(_) | IamError::NoSuchGroup(_) | IamError::NoSuchPolicy => S3ErrorCode::NoSuchResource, - IamError::InvalidAccessKeyLength | IamError::InvalidSecretKeyLength => S3ErrorCode::InvalidArgument, + IamError::InvalidAccessKeyLength | IamError::InvalidSecretKeyLength | IamError::AccessKeyAlreadyExists => { + S3ErrorCode::InvalidArgument + } _ => S3ErrorCode::InternalError, }; @@ -64,6 +66,14 @@ mod tests { } } + #[test] + fn duplicate_access_key_maps_to_invalid_argument() { + let s3_error = iam_error_to_s3_error(IamError::AccessKeyAlreadyExists); + + assert_eq!(s3_error.code(), &S3ErrorCode::InvalidArgument); + assert_eq!(s3_error.message(), Some("access key is already in use")); + } + #[test] fn non_validation_iam_errors_remain_internal_errors() { let s3_error = iam_error_to_s3_error(IamError::IamSysNotInitialized); diff --git a/rustfs/src/admin/handlers/service_account.rs b/rustfs/src/admin/handlers/service_account.rs index 11029ba3f..1b27192f2 100644 --- a/rustfs/src/admin/handlers/service_account.rs +++ b/rustfs/src/admin/handlers/service_account.rs @@ -398,9 +398,9 @@ impl Operation for AddServiceAccount { "admin service account state" ); match e { - rustfs_iam::error::Error::InvalidAccessKeyLength | rustfs_iam::error::Error::InvalidSecretKeyLength => { - iam_error_to_s3_error(e) - } + rustfs_iam::error::Error::InvalidAccessKeyLength + | rustfs_iam::error::Error::InvalidSecretKeyLength + | rustfs_iam::error::Error::AccessKeyAlreadyExists => iam_error_to_s3_error(e), err => s3_error!(InternalError, "create service account failed, e: {:?}", err), } })?; @@ -1580,8 +1580,8 @@ mod tests { let create_block = &create_block[..create_end]; assert!( - create_block.contains("iam_error_to_s3_error(e)"), - "service-account create validation errors must map to client S3 errors" + create_block.contains("AccessKeyAlreadyExists") && create_block.contains("iam_error_to_s3_error(e)"), + "duplicate access keys must map to client S3 errors" ); } diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 3a576ef7a..ff88602d5 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -183,6 +183,14 @@ fn imported_service_account_access_key_failure(entry_access_key: &str, payload_a } pub struct AddUser {} + +fn map_add_user_create_error(err: rustfs_iam::error::Error) -> S3Error { + match err { + rustfs_iam::error::Error::AccessKeyAlreadyExists => iam_error_to_s3_error(err), + err => S3Error::with_message(S3ErrorCode::InternalError, format!("failed to create user: {err}")), + } +} + #[async_trait::async_trait] impl Operation for AddUser { async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { @@ -274,10 +282,7 @@ impl Operation for AddUser { ) .await?; - let updated_at = iam_store - .create_user(ak, &args) - .await - .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to create user: {e}")))?; + let updated_at = iam_store.create_user(ak, &args).await.map_err(map_add_user_create_error)?; if let Err(err) = site_replication_iam_change_hook(SRIAMItem { r#type: "iam-user".to_string(), @@ -1335,15 +1340,38 @@ mod tests { use super::{ GROUP_POLICY_MAPPING_USER_TYPE, SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR, imported_service_account_access_key_failure, 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, + imported_service_account_parent_scope_failure, imported_service_account_status, map_add_user_create_error, + 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; use rustfs_madmin::user::SRSvcAccCreate; + use s3s::S3ErrorCode; use serde_json::Value; use std::collections::HashMap; + #[test] + fn add_user_maps_duplicate_access_keys_to_s3_errors() { + let err = map_add_user_create_error(IamError::AccessKeyAlreadyExists); + + assert_eq!(err.code(), &S3ErrorCode::InvalidArgument); + assert_eq!(err.message(), Some("access key is already in use")); + } + + #[test] + fn add_user_preserves_internal_error_fallback() { + let err = map_add_user_create_error(IamError::IamSysNotInitialized); + + assert_eq!(err.code(), &S3ErrorCode::InternalError); + assert_eq!(err.message(), Some("failed to create user: not initialized")); + } + + #[test] + fn add_user_operation_maps_create_errors() { + let mapper_call = concat!("map_err(map_add_user_", "create_error)"); + assert!(include_str!("user.rs").contains(mapper_call)); + } + #[test] fn test_should_check_deny_only_for_regular_self_request() { let cred = Credentials {