fix(auth): restrict root-derived accounts and credential logs (#8674)

This commit is contained in:
Chris
2026-10-19 08:57:31 +09:00
committed by GitHub
parent fca83c72e4
commit ff70c06cef
5 changed files with 262 additions and 55 deletions
+65 -1
View File
@@ -56,6 +56,10 @@ use tokio::{
use tracing::warn;
use tracing::{debug, error};
const EVENT_USER_UPDATE_EXISTING: &str = "iam.user.update_existing";
const LOG_COMPONENT_IAM: &str = "iam";
const LOG_SUBSYSTEM_USERS: &str = "users";
const IAM_FORMAT_FILE: &str = "format.json";
const IAM_FORMAT_VERSION_1: i32 = 1;
#[cfg(not(test))]
@@ -1485,7 +1489,13 @@ where
let cache = self.cache.snapshot();
let users = Arc::clone(&cache.users);
if let Some(x) = users.get(access_key) {
warn!(error = ?x, "IAM user already exists");
debug!(
event = EVENT_USER_UPDATE_EXISTING,
component = LOG_COMPONENT_IAM,
subsystem = LOG_SUBSYSTEM_USERS,
state = "existing",
"Updating existing IAM user"
);
if x.credentials.is_temp() {
return Err(Error::AccessKeyAlreadyExists);
}
@@ -2921,6 +2931,60 @@ mod tests {
}
}
#[tokio::test]
async fn credential_boundary_existing_user_logs_omit_credentials() {
use tracing::instrument::WithSubscriber;
let cache = build_test_iam_cache(FailingInitialLoadStore);
let identity = UserIdentity::new(Credentials {
access_key: "LOGACCESS-PRIVATE-TAIL".to_string(),
secret_key: "Qstored-secret-valueZ".to_string(),
status: STATUS_ENABLED.to_string(),
..Default::default()
});
cache
.cache
.add_or_update_user(&identity.credentials.access_key, &identity, OffsetDateTime::now_utc());
let log = tempfile::NamedTempFile::new().expect("capture file");
let writer = log.reopen().expect("capture writer");
let subscriber = tracing_subscriber::fmt()
.with_max_level(tracing::Level::DEBUG)
.with_ansi(false)
.without_time()
.with_writer(move || writer.try_clone().expect("clone capture writer"))
.finish();
let result = cache
.add_user(
&identity.credentials.access_key,
&AddOrUpdateUserReq {
secret_key: "replacement-secret".to_string(),
policy: None,
status: AccountStatus::Enabled,
},
)
.with_subscriber(subscriber)
.await;
assert!(
matches!(result, Err(Error::InvalidArgument)),
"the test store rejects persistence after the existing-user path"
);
let captured = std::fs::read_to_string(log.path()).expect("read captured logs");
assert!(
captured.contains(EVENT_USER_UPDATE_EXISTING) || captured.contains("IAM user already exists"),
"the existing-user diagnostic must be captured"
);
for forbidden in [
&identity.credentials.access_key,
&identity.credentials.secret_key,
"Q***Z|20",
"secret_key",
"session_token",
"credentials",
] {
assert!(!captured.contains(forbidden), "credential material must not appear in logs: {forbidden}");
}
}
#[tokio::test]
async fn sts_policy_lookup_ignores_colliding_regular_user_mapping() {
let iam = build_test_iam_cache(FailingInitialLoadStore);
+1 -1
View File
@@ -12,7 +12,7 @@ Every fixed RustFS GitHub Security Advisory maps to at least one named regressio
| [GHSA-3p3x-734c-h5vx](https://github.com/rustfs/rustfs/security/advisories/GHSA-3p3x-734c-h5vx) | Constant-time secret comparison on WebDAV/FTPS password login | rustfs/rustfs#4403 | `assert_ftps_ghsa_3p3x_wrong_credentials_rejected` (`crates/e2e_test/src/protocols/ftps_core.rs`); `GHSA-3p3x` auth-failure block in `test_webdav_core_operations` (`crates/e2e_test/src/protocols/webdav_core.rs`) | e2e (`e2e-protocols`) |
| [GHSA-r5qv-rc46-hv8q](https://github.com/rustfs/rustfs/security/advisories/GHSA-r5qv-rc46-hv8q) | Internode RPC authentication must fail closed | rustfs/rustfs#4402 | `ghsa_r5qv_resolve_shared_secret_rejects_default_fallback`, `ghsa_r5qv_verify_rpc_signature_fails_closed_on_missing_or_invalid_auth` (`crates/ecstore/src/cluster/rpc/http_auth.rs`) | unit |
| [GHSA-m77q-r63m-pj89](https://github.com/rustfs/rustfs/security/advisories/GHSA-m77q-r63m-pj89) | STS JWTs signed with the shared root secret (intentionally unfixed) | n/a; tests pin the by-design behaviour and must flip red -> green when m77q is fixed | `test_ghsa_m77q_sts_session_token_signed_with_root_secret`, `test_created_sts_credentials_authorize_with_session_token_claims` (`crates/iam/src/sys.rs`); `token_signing_key` doc (`crates/iam/src/root_credentials.rs`) | unit |
| [GHSA-5354-r3w2-34m8](https://github.com/rustfs/rustfs/security/advisories/GHSA-5354-r3w2-34m8) | Service-account parent must stay within caller scope; a non-owner with `CreateServiceAccountAdminAction` could parent a service account to root | rustfs/rustfs#5141 | `ghsa_5354_non_owner_service_account_parent_confined_to_scope`, `ghsa_5354_scope_guard_matches_owner_or_self_scope_for_derived_credentials`, and the `add_service_account_parent_within_scope` invariant they pin (`rustfs/src/admin/handlers/service_account.rs`) | unit |
| [GHSA-5354-r3w2-34m8](https://github.com/rustfs/rustfs/security/advisories/GHSA-5354-r3w2-34m8) | Service-account parent must stay within caller scope; a non-owner with `CreateServiceAccountAdminAction` could parent a service account to root | rustfs/rustfs#5141 | `ghsa_5354_non_owner_service_account_parent_confined_to_scope`, `ghsa_5354_scope_guard_preserves_non_root_self_scope`, and the `add_service_account_parent_within_scope` invariant they pin (`rustfs/src/admin/handlers/service_account.rs`) | unit |
| [GHSA-3ppv-fx5m-m749](https://github.com/rustfs/rustfs/security/advisories/GHSA-3ppv-fx5m-m749) | Versioned reads (`get_object`, CopyObject source, UploadPartCopy source) authorize against `s3:GetObjectVersion`, not `s3:GetObject` | rustfs/rustfs#5142 | `ghsa_3ppv_versioned_read_selects_get_object_version_action` and the `versioned_read_action` helper it pins (`rustfs/src/storage/access.rs`) | unit |
| [GHSA-v9cp-qfw9-9pfp](https://github.com/rustfs/rustfs/security/advisories/GHSA-v9cp-qfw9-9pfp) | `ForAllValues:`/`ForAnyValue:` negated string operators applied negation to the aggregate instead of the per-value predicate | fixed, GHSA private-fork merge | `ghsa_v9cp_for_all_values_not_equals_partial_overlap`, `ghsa_v9cp_for_any_value_not_equals_partial_overlap` and the absent-key/positive-quantifier cases beside them (`crates/policy/tests/quantified_negation.rs`); the value set must partially overlap the policy set, since contained or disjoint sets cannot tell the quantifiers apart | crate test |
| [GHSA-6r96-hmgc-726c](https://github.com/rustfs/rustfs/security/advisories/GHSA-6r96-hmgc-726c) | Request headers must not populate server-derived IAM condition keys (`userid`, `groups`, `jwt:`/`ldap:` claims) | fixed, GHSA private-fork merge | `ghsa_6r96_identity_condition_keys_ignore_spoofed_headers`, `ghsa_6r96_claim_condition_keys_ignore_spoofed_headers`, and `test_request_headers_still_reach_conditions`, which keeps the reserved set from growing too broad (`rustfs/src/auth.rs`) | unit |
+61 -39
View File
@@ -125,14 +125,25 @@ fn is_service_account_owner_of(caller: &StoredCredentials, target_parent_user: &
///
/// The `CreateServiceAccountAdminAction` permission gates *whether* a caller may
/// create service accounts, not *for whom*. A non-owner is therefore confined to
/// its own scope; only an owner may target another user — including the root
/// its own non-root scope; only an owner may target another user — including the root
/// credential, whose existence check is deliberately lenient. Without this a
/// caller holding only that action could pass `target_user = <root access key>`
/// and mint a root-parented service account that authenticates as owner via
/// `prepare_service_account_auth`, a full-takeover privilege escalation. Mirrors
/// `imported_service_account_parent_allowed` on the ImportIam path (GHSA-566f).
fn add_service_account_parent_within_scope(target_user: &str, req_user: &str, req_parent_user: &str, owner: bool) -> bool {
owner || target_user == req_user || target_user == req_parent_user
fn add_service_account_parent_within_scope(
root_access_key: &str,
target_user: &str,
req_user: &str,
req_parent_user: &str,
owner: bool,
) -> bool {
let effective_parent = if target_user == req_user {
req_parent_user
} else {
target_user
};
owner || (effective_parent != root_access_key && (target_user == req_user || target_user == req_parent_user))
}
/// Fail-closed ownership check used by DeleteServiceAccount for a caller that
@@ -401,9 +412,8 @@ impl Operation for AddServiceAccount {
// a root-parented (owner) service account. Evaluated on the original
// `target_user` the caller submitted, before the derived-credential rewrite
// to `req_parent_user` below, so the guard is bound to the parent actually
// requested. Its allow set is exactly `owner || is_svc_acc`. See the helper's
// doc comment.
if !add_service_account_parent_within_scope(&target_user, &req_user, &req_parent_user, owner) {
// requested and to the effective parent after that rewrite.
if !add_service_account_parent_within_scope(&sys_cred.access_key, &target_user, &req_user, &req_parent_user, owner) {
return Err(s3_error!(AccessDenied, "service account parent is outside requester scope"));
}
@@ -1470,76 +1480,88 @@ mod tests {
// Attack: non-owner "alice" targets root as the parent -> DENY.
assert!(
!add_service_account_parent_within_scope(root, "alice", "alice", false),
!add_service_account_parent_within_scope(root, root, "alice", "alice", false),
"non-owner must not parent a service account to root"
);
// Non-owner targeting an unrelated user -> DENY.
assert!(
!add_service_account_parent_within_scope("bob", "alice", "alice", false),
!add_service_account_parent_within_scope(root, "bob", "alice", "alice", false),
"non-owner must not parent a service account to another user"
);
// Self-scoped creation (default target == own key) -> ALLOW.
assert!(
add_service_account_parent_within_scope("alice", "alice", "alice", false),
add_service_account_parent_within_scope(root, "alice", "alice", "alice", false),
"non-owner may create a service account for itself"
);
// Derived credential targeting its own parent -> ALLOW.
assert!(
add_service_account_parent_within_scope("parent-user", "svc-key", "parent-user", false),
add_service_account_parent_within_scope(root, "parent-user", "svc-key", "parent-user", false),
"derived credential may create a service account under its own parent"
);
// Owner may target any parent, including root (cross-user creation intact).
assert!(
add_service_account_parent_within_scope(root, root, root, true),
add_service_account_parent_within_scope(root, root, root, root, true),
"owner retains cross-user / root-parent creation"
);
assert!(
add_service_account_parent_within_scope("bob", root, root, true),
add_service_account_parent_within_scope(root, "bob", root, root, true),
"owner may create a service account for another user"
);
}
#[test]
fn ghsa_5354_scope_guard_matches_owner_or_self_scope_for_derived_credentials() {
// The handler evaluates `add_service_account_parent_within_scope` on the
// original `target_user`, before the `target_user = req_parent_user` rewrite
// taken inside the `is_svc_acc` branch. The guard's allow set must therefore
// stay exactly `owner || is_svc_acc`, where
// `is_svc_acc == (target_user == req_user || target_user == req_parent_user)`.
//
// The named-boundary test above only exercises the self-scoped case with
// `req_user == req_parent_user`. A *derived* credential has `req_user`
// (its own key) distinct from `req_parent_user` (its parent), which is the
// realistic attacker shape: a service account holding
// CreateServiceAccountAdminAction. Pin the equivalence across that shape so a
// later change to either the guard or the rewrite cannot silently let a
// derived non-owner credential escape its own parent's scope.
fn ghsa_5354_scope_guard_preserves_non_root_self_scope() {
let root = rustfs_credentials::DEFAULT_ACCESS_KEY;
let cases: &[(&str, &str, &str, bool)] = &[
let cases: &[(&str, &str, &str, bool, bool)] = &[
// Derived non-owner (own key != parent) aiming at root -> deny.
(root, "attacker-sa", "alice", false),
(root, "attacker-sa", "alice", false, false),
// Derived non-owner aiming at an unrelated user -> deny.
("bob", "attacker-sa", "alice", false),
("bob", "attacker-sa", "alice", false, false),
// Derived non-owner aiming at its own parent -> allow.
("alice", "attacker-sa", "alice", false),
("alice", "attacker-sa", "alice", false, true),
// Derived non-owner aiming at its own key -> allow.
("attacker-sa", "attacker-sa", "alice", false),
("attacker-sa", "attacker-sa", "alice", false, true),
// Top-level non-owner aiming at itself -> allow.
("alice", "alice", "alice", false),
("alice", "alice", "alice", false, true),
// Owner is unrestricted, including root and a derived shape.
(root, "attacker-sa", "alice", true),
("bob", root, root, true),
(root, "attacker-sa", "alice", true, true),
("bob", root, root, true, true),
];
for &(target_user, req_user, req_parent_user, owner) in cases {
let is_svc_acc = target_user == req_user || target_user == req_parent_user;
for &(target_user, req_user, req_parent_user, owner, expected) in cases {
assert_eq!(
add_service_account_parent_within_scope(target_user, req_user, req_parent_user, owner),
owner || is_svc_acc,
"scope guard must equal `owner || is_svc_acc` for (target={target_user}, req_user={req_user}, req_parent={req_parent_user}, owner={owner})"
add_service_account_parent_within_scope(root, target_user, req_user, req_parent_user, owner),
expected,
"scope guard mismatch for (target={target_user}, req_user={req_user}, req_parent={req_parent_user}, owner={owner})"
);
}
}
#[test]
fn credential_boundary_root_derived_non_owner_cannot_create_owner_service_account() {
let root = "deployment-root";
for target in [root, "restricted-service-account", "other-user"] {
assert!(
!add_service_account_parent_within_scope(root, target, "restricted-service-account", root, false),
"a root-derived non-owner must be denied even when targeting its own key: {target}"
);
}
assert!(add_service_account_parent_within_scope(root, root, root, root, true));
assert!(add_service_account_parent_within_scope(
root,
"alice",
"alice-service-account",
"alice",
false
));
assert!(add_service_account_parent_within_scope(
root,
"alice-service-account",
"alice-service-account",
"alice",
false
));
}
#[test]
fn guess_user_provider_detects_builtin_accounts() {
let credentials = StoredCredentials {
+83 -11
View File
@@ -163,7 +163,7 @@ fn imported_service_account_status(status: &str) -> Option<String> {
const SERVICE_ACCOUNT_PARENT_SCOPE_ERROR: &str = "service account parent is outside requester scope";
const SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR: &str = "service account access key does not match import entry";
fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials, owner: bool) -> bool {
fn imported_service_account_parent_allowed(root_access_key: &str, parent: &str, requester: &Credentials, owner: bool) -> bool {
if parent.is_empty() {
return false;
}
@@ -172,6 +172,10 @@ fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials
return true;
}
if parent == root_access_key {
return false;
}
if requester.is_temp() || requester.is_service_account() {
return temp_identity_parent(requester).is_some_and(|requester_parent| requester_parent == parent);
}
@@ -180,12 +184,13 @@ fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials
}
fn imported_service_account_parent_scope_failure(
root_access_key: &str,
access_key: &str,
parent: &str,
requester: &Credentials,
owner: bool,
) -> Option<IAMErrEntity> {
(!imported_service_account_parent_allowed(parent, requester, owner)).then(|| IAMErrEntity {
(!imported_service_account_parent_allowed(root_access_key, parent, requester, owner)).then(|| IAMErrEntity {
name: access_key.to_string(),
error: SERVICE_ACCOUNT_PARENT_SCOPE_ERROR.to_string(),
})
@@ -1089,7 +1094,9 @@ impl Operation for ImportIam {
continue;
}
if let Some(err) = imported_service_account_parent_scope_failure(&ak, &req.parent, &cred, owner) {
if let Some(err) =
imported_service_account_parent_scope_failure(&sys_cred.access_key, &ak, &req.parent, &cred, owner)
{
failed.service_accounts.push(err);
continue;
}
@@ -1657,7 +1664,12 @@ mod tests {
..Default::default()
};
assert!(!imported_service_account_parent_allowed("root-access-key", &requester, false));
assert!(!imported_service_account_parent_allowed(
"root-access-key",
"root-access-key",
&requester,
false
));
}
#[test]
@@ -1666,13 +1678,26 @@ mod tests {
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");
let err = imported_service_account_parent_scope_failure(
"root-access-key",
"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()
imported_service_account_parent_scope_failure(
"root-access-key",
"svc-access-key",
"delegated-importer",
&requester,
false
)
.is_none()
);
}
@@ -1703,6 +1728,33 @@ mod tests {
assert!(imported_service_account_access_key_failure("svcalpha", "svcalpha").is_none());
}
#[test]
fn credential_boundary_root_derived_non_owner_import_rejects_owner_service_account() {
let root = "deployment-root";
for temporary in [false, true] {
let requester = Credentials {
access_key: "restricted-account".to_string(),
parent_user: root.to_string(),
session_token: "session-token".to_string(),
expiration: temporary.then(|| time::OffsetDateTime::now_utc() + time::Duration::hours(1)),
claims: (!temporary).then(|| {
HashMap::from([(
rustfs_credentials::IAM_POLICY_CLAIM_NAME_SA.to_string(),
serde_json::json!("inherited-policy"),
)])
}),
..Default::default()
};
assert_eq!(requester.is_service_account(), !temporary);
assert!(requester.is_temp());
let error = imported_service_account_parent_scope_failure(root, "backdoor", root, &requester, false)
.expect("a restricted root-derived caller must not import an owner account");
assert_eq!(error.name, "backdoor");
assert_eq!(error.error, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR);
assert!(imported_service_account_parent_allowed(root, root, &requester, true));
}
}
#[test]
fn test_import_service_account_parent_allows_owner_restore() {
let requester = Credentials {
@@ -1710,7 +1762,12 @@ mod tests {
..Default::default()
};
assert!(imported_service_account_parent_allowed("any-imported-parent", &requester, true));
assert!(imported_service_account_parent_allowed(
"root-access-key",
"any-imported-parent",
&requester,
true
));
}
#[test]
@@ -1720,7 +1777,12 @@ mod tests {
..Default::default()
};
assert!(imported_service_account_parent_allowed("delegated-importer", &requester, false));
assert!(imported_service_account_parent_allowed(
"root-access-key",
"delegated-importer",
&requester,
false
));
}
#[test]
@@ -1732,8 +1794,18 @@ mod tests {
..Default::default()
};
assert!(imported_service_account_parent_allowed("parent-user", &requester, false));
assert!(!imported_service_account_parent_allowed("other-parent", &requester, false));
assert!(imported_service_account_parent_allowed(
"root-access-key",
"parent-user",
&requester,
false
));
assert!(!imported_service_account_parent_allowed(
"root-access-key",
"other-parent",
&requester,
false
));
}
#[test]
+52 -3
View File
@@ -240,7 +240,6 @@ impl S3Auth for IAMAuth {
event = EVENT_SECRET_KEY_LOOKUP_FAILED,
component = LOG_COMPONENT_AUTH,
subsystem = LOG_SUBSYSTEM_CREDENTIALS,
access_key = %MaskedAccessKey(access_key),
reason = "no_such_user",
"Secret key lookup rejected"
);
@@ -250,7 +249,6 @@ impl S3Auth for IAMAuth {
event = EVENT_SECRET_KEY_LOOKUP_FAILED,
component = LOG_COMPONENT_AUTH,
subsystem = LOG_SUBSYSTEM_CREDENTIALS,
access_key = %MaskedAccessKey(access_key),
error = ?e,
reason = "check_key_error",
"Secret key lookup errored"
@@ -263,7 +261,6 @@ impl S3Auth for IAMAuth {
event = EVENT_SECRET_KEY_LOOKUP_FAILED,
component = LOG_COMPONENT_AUTH,
subsystem = LOG_SUBSYSTEM_CREDENTIALS,
access_key = %MaskedAccessKey(access_key),
reason = "iam_not_initialized",
"Secret key lookup skipped"
);
@@ -1587,6 +1584,58 @@ mod tests {
assert!(error.message().unwrap_or("").contains("Your account is not signed up"));
}
#[tokio::test]
async fn credential_boundary_rejected_access_key_logs_omit_key_fragments() {
use rustfs_iam::{cache::Cache, manager::IamCache};
use tracing::instrument::WithSubscriber;
let (_temp_dir, _disk_paths, store) = crate::app::gating_test_env::isolated_multi_pool_ecstore().await;
let (send_chan, _receiver) = tokio::sync::mpsc::channel(1);
let cache = Arc::new(IamCache {
cache: Cache::default(),
api: ObjectStore::new(store.clone()),
state: Default::default(),
loading: Default::default(),
roles: HashMap::new(),
send_chan,
last_timestamp: Default::default(),
sync_failures: Default::default(),
sync_successes: Default::default(),
last_sync_duration_millis: Default::default(),
});
let slot = ServerContextSlot::new();
let context = AppContext::new(
store,
Arc::new(ContextIam {
handle: Arc::new(IamSys::new(cache)),
}),
Arc::new(TestKms),
);
assert!(slot.install(Arc::new(context)));
for (slot, reason) in [(slot, "no_such_user"), (ServerContextSlot::new(), "iam_not_initialized")] {
let auth = IAMAuth::with_server_context("bootstrap-key", SecretKey::from("bootstrap-secret"), slot);
let log = tempfile::NamedTempFile::new().expect("capture file");
let writer = log.reopen().expect("capture writer");
let subscriber = tracing_subscriber::fmt()
.with_max_level(tracing::Level::WARN)
.with_ansi(false)
.without_time()
.with_writer(move || writer.try_clone().expect("clone capture writer"))
.finish();
let error = auth
.get_secret_key("LEAK-private-access-key-TAIL")
.with_subscriber(subscriber)
.await
.expect_err("unknown credentials must be rejected");
assert_eq!(error.code(), &S3ErrorCode::InvalidAccessKeyId);
let captured = std::fs::read_to_string(log.path()).expect("read captured logs");
assert!(captured.contains(reason), "capture the expected rejection: {captured}");
for forbidden in ["LEAK", "TAIL", "private-access-key", "access_key="] {
assert!(!captured.contains(forbidden), "rejected access-key material must not be logged");
}
}
}
#[test]
fn test_iam_lookup_error_maps_to_internal_error() {
let result = iam_lookup_error_to_s3_error(&IamError::Io(std::io::Error::other("load user failed")));