From d556d4dc94a2c98257dd715b27220885fc855fe2 Mon Sep 17 00:00:00 2001 From: yi111 <153097222+Yi-111-a@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:40:10 +0800 Subject: [PATCH] fix(admin): report a missing policy or user as 404 NoSuchResource, not 500 InternalError Merge the approved fix from pull request #8127. --- CHANGELOG.md | 1 + rustfs/src/admin/handlers/policies.rs | 20 ++++++++++++++++++ rustfs/src/admin/handlers/user.rs | 30 ++++++++++++++++++++------- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a9b7f292..db20aff54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Presigned URLs honour only signed headers** (GHSA-g8w9-qw9q-fghr): a SigV4 presigned request that carries an `x-amz-*` request header not listed in `X-Amz-SignedHeaders` is now rejected with `403 AccessDenied` ("There were headers present in the request which were not signed"), matching AWS S3. Previously the holder of a presigned `PutObject` URL could add unsigned `x-amz-tagging`, `x-amz-storage-class`, `x-amz-website-redirect-location`, ACL, metadata, Object Lock or SSE headers and have them applied. Presigners that intend a property must set it before signing so the SDK lists the header in `SignedHeaders`; `x-amz-cf-id` (CloudFront) remains tolerated unsigned. Header-signed SigV4 and SigV2 requests are unchanged. ### Fixed +- **Two admin handlers reported a missing policy or user as `500 InternalError`**: `GET /rustfs/admin/v3/info-canned-policy` and `DELETE /rustfs/admin/v3/remove-user` wrapped every IAM error as an internal fault instead of classifying it, so a lookup for something that does not exist answered `500 InternalError` where the rest of the admin API answers `404 NoSuchResource` — `GET /rustfs/admin/v3/user-info` already did, and `iam_error_to_s3_error` already mapped `NoSuchPolicy`, `NoSuchUser`, `NoSuchServiceAccount` and `NoSuchTempAccount` to `NoSuchResource`. A client could not tell "does not exist" from a real server failure, and a controller that checks existence before creating (a Kubernetes operator reconciling policies or users) could never create the object: the missing-policy lookup failed with a `500` and it retried forever. Both handlers now map their IAM errors through the shared `iam_error_to_s3_error` helper, so a missing policy or user is `404 NoSuchResource` and the `error` is preserved as the S3 error's source; the three IAM calls in the removal path (temporary-user lookup, service-account lookup, delete) are all reclassified, and every other IAM error stays `500 InternalError`. Fixes #8117. - **Vault backends accepted key names that escape the key prefix**: only the Local backend refused a key identifier containing a path separator, so on Vault KV2 `POST /rustfs/admin/v3/kms/keys` with a name such as `bad/name` succeeded and created a nested KV2 path that the key listing then reported as a directory rather than a key, and an identifier containing `..` addressed a record outside the configured key prefix once the HTTP client normalised the URL. Both Vault backends now apply the Local backend's containment rule at the point where the identifier becomes a path or a Transit key name: an empty identifier, one containing `/`, `\\` or NUL, or the dot segments `.` and `..` is refused with `400` (`InvalidKey`) before any request reaches Vault. The AWS backend is unaffected, since it addresses keys by ARN and alias. A Vault deployment that created such a key on an earlier build must re-create it under a plain name; objects encrypted under the old name stay readable only until that key is refused, so migrate them first. Refs rustfs/backlog#2474. - **Fresh multi-pool bootstrap with distinct format creators**: a new deployment whose pools have their first endpoint on different nodes (for example two single-node pools) could never publish its initial `pool.bin`: each node held fresh-bootstrap proof only for the pool it formatted, the deployment-wide proof collapsed to none, and every node died with `pool metadata recovery required: no durable bootstrap identity or pool.bin replica is available` after the startup retry budget. The first pool's creator now mints the pending cluster identity on its own pool, every other creator copies that nonce-bound identity onto the pool it formatted first-hand, and the elected writer publishes `pool.bin` once every pool replica carries the same pending identity. Corrupt or disagreeing replicas, pools that merely have a format, expansion pools joining an initialized deployment, and restarts without first-hand proof still fail closed. Non-elected nodes that start before `pool.bin` exists, and the elected writer while it waits for the other creators, no longer latch their pool-metadata write gate for the life of the process. Refs rustfs/backlog#2338, rustfs/backlog#2375. - **Lock RPC timeout storms** (#7363): the remote lock client no longer evicts and re-dials the shared internode HTTP/2 channel on every request deadline. A timeout evicts only when the peer has not completed any lock RPC for two deadlines, evictions and transport-failure re-dials are rate limited per peer (`RUSTFS_OBJECT_LOCK_RPC_EVICTION_COOLDOWN_MS`, default 5 s), and a timed-out request is left running instead of being reset (bounded per peer by `RUSTFS_OBJECT_LOCK_RPC_DETACHED_LIMIT`, default 256), so a slow lock endpoint can no longer drive the `RST_STREAM`/`GOAWAY too_many_resets`/reconnect loop. A lock granted after its caller timed out is released immediately, and unlocks that fail the quick retries continue on a deferred 1/2/4/8/16 s schedule before the server lease reclaims them. New `rustfs_remote_lock_*` metrics cover timeouts, evictions, suppressed evictions, detached streams, late completions and late releases per peer. Operator guide at `docs/operations/lock-rpc-storm-protection.md`. diff --git a/rustfs/src/admin/handlers/policies.rs b/rustfs/src/admin/handlers/policies.rs index 533d5f626..1e0ad0160 100644 --- a/rustfs/src/admin/handlers/policies.rs +++ b/rustfs/src/admin/handlers/policies.rs @@ -1271,4 +1271,24 @@ mod tests { assert!(!production.contains("check_key_valid(get_session_token")); } + + #[test] + fn info_canned_policy_maps_iam_errors_to_s3_errors() { + let production = include_str!("policies.rs") + .split("\n#[cfg(test)]\n") + .next() + .expect("production source must precede tests"); + let body = source_block(production, "impl Operation for InfoCannedPolicy"); + + let lookup = body + .find("info_policy(") + .expect("InfoCannedPolicy should look the policy up through the IAM store"); + let tail = &body[lookup..]; + let end = tail.find("?;").unwrap_or(tail.len()); + + assert!( + tail[..end].contains("iam_error_to_s3_error(e)"), + "a missing policy must map through iam_error_to_s3_error (404 NoSuchResource) instead of 500 InternalError" + ); + } } diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 88a6d4d50..5d58382bf 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -516,17 +516,12 @@ impl Operation for RemoveUser { return Err(s3_error!(InvalidArgument, "cannot remove a temporary user")); } - let (is_service_account, _) = iam_store.is_service_account(ak).await.map_err(|e| { - S3Error::with_message(S3ErrorCode::InternalError, format!("failed to query service account state: {e}")) - })?; + let (is_service_account, _) = iam_store.is_service_account(ak).await.map_err(iam_error_to_s3_error)?; if is_service_account { return Err(s3_error!(InvalidArgument, "cannot remove a service account")); } - iam_store - .delete_user(ak, true) - .await - .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to delete user: {e}")))?; + iam_store.delete_user(ak, true).await.map_err(iam_error_to_s3_error)?; if let Err(err) = site_replication_iam_change_hook(SRIAMItem { r#type: "iam-user".to_string(), @@ -1440,6 +1435,27 @@ mod tests { assert!(include_str!("user.rs").contains(mapper_call)); } + #[test] + fn remove_user_maps_iam_errors_to_s3_errors() { + let production = include_str!("user.rs") + .split("\n#[cfg(test)]\n") + .next() + .expect("production source must precede tests"); + let body = source_block(production, "impl Operation for RemoveUser"); + + for call in ["is_temp_user(ak)", "is_service_account(ak)", "delete_user(ak, true)"] { + let start = body + .find(call) + .unwrap_or_else(|| panic!("RemoveUser should call {call} through the IAM store")); + let tail = &body[start..]; + let end = tail.find("?;").unwrap_or(tail.len()); + assert!( + tail[..end].contains("iam_error_to_s3_error"), + "{call} must map through iam_error_to_s3_error so a missing user is 404 NoSuchResource, not 500 InternalError" + ); + } + } + #[test] fn import_iam_enqueues_a_site_replication_snapshot() { let body = source_block(include_str!("user.rs"), "impl Operation for ImportIam");