From 04befaef88820e35d5ecace2a3604f34b29b07d3 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sat, 1 Aug 2026 23:02:02 +0800 Subject: [PATCH] fix(admin): retire the query-string form of immediate KMS key deletion Immediate deletion destroys master key material outright, and every object encrypted under that key becomes permanently unreadable. The delete endpoint accepted that request as a query parameter, which is the form most easily issued by accident and the one that made the waiting window bypassable. The query string can now only schedule a deletion: `force_immediate` with any value other than `false`, or a `confirm_key_id` parameter, is refused with 400 rather than downgraded to a scheduled deletion, so a caller cannot read the answer as "destroyed". The JSON body form is unchanged and remains the single way to reach the service gate that enforces the server opt-in and the echoed confirmation. Classify the route accordingly: `RouteRiskLevel` gains `Critical` for routes whose worst case is permanent loss of user data, and the KMS key deletion route is the only member, pinned in both directions by a matrix test. Endpoint-level coverage for the 7-30 day window bound is added for every configured backend. Refs rustfs/backlog#1585 (part of rustfs/backlog#1562) --- .../src/kms/configured_roundtrip_test.rs | 47 +++- crates/e2e_test/src/kms/kms_vault_test.rs | 23 +- .../security-governance/src/admin_matrix.rs | 8 + docs/operations/kms-backend-security.md | 2 +- rustfs/src/admin/handlers/kms_keys.rs | 207 ++++++++++++++---- rustfs/src/admin/route_policy.rs | 48 +++- 6 files changed, 289 insertions(+), 46 deletions(-) diff --git a/crates/e2e_test/src/kms/configured_roundtrip_test.rs b/crates/e2e_test/src/kms/configured_roundtrip_test.rs index 13e96ef0b..a666ef8d7 100644 --- a/crates/e2e_test/src/kms/configured_roundtrip_test.rs +++ b/crates/e2e_test/src/kms/configured_roundtrip_test.rs @@ -126,6 +126,51 @@ async fn assert_key_deletion_lifecycle(base_url: &str, access_key: &str, secret_ assert_eq!(cancelled["success"], true); assert_eq!(cancelled["key_metadata"]["key_state"], "Enabled"); + // A window outside 7-30 days is refused at the endpoint, whatever the + // backend: the bound is enforced once in the service, so no backend can + // stretch or skip it (rustfs/backlog#1585). + for days in [6, 31] { + let refused = kms_admin_request( + base_url, + http::Method::DELETE, + "/rustfs/admin/v3/kms/keys/delete", + Some( + &serde_json::json!({ + "key_id": key_id, + "pending_window_in_days": days + }) + .to_string(), + ), + access_key, + secret_key, + ) + .await + .err() + .ok_or_else(|| format!("a {days}-day deletion window must be refused"))?; + assert!( + refused.to_string().contains("400 Bad Request"), + "a {days}-day deletion window must report a client error: {refused}" + ); + } + + // Immediate deletion is no longer reachable through the query string, so it + // fails before the service gate is even consulted. + let refused = kms_admin_request( + base_url, + http::Method::DELETE, + &format!("/rustfs/admin/v3/kms/keys/delete?keyId={key_id}&force_immediate=true"), + None, + access_key, + secret_key, + ) + .await + .err() + .ok_or("immediate KMS key deletion must not be reachable through the query string")?; + assert!( + refused.to_string().contains("400 Bad Request"), + "a query-string immediate deletion must report a client error: {refused}" + ); + // A default server refuses to skip the waiting window (rustfs/backlog#1585): // immediate deletion is unrecoverable and takes every object encrypted under // the key with it, so the endpoint must reject it rather than honour it. @@ -151,7 +196,7 @@ async fn assert_key_deletion_lifecycle(base_url: &str, access_key: &str, secret_ "refused immediate deletion must report a client error: {refused}" ); - // The refused request left the key alone, so the window-bounded path still + // The refused requests left the key alone, so the window-bounded path still // has something to schedule. let described = kms_admin_request( base_url, diff --git a/crates/e2e_test/src/kms/kms_vault_test.rs b/crates/e2e_test/src/kms/kms_vault_test.rs index 1ca370815..d360ea557 100644 --- a/crates/e2e_test/src/kms/kms_vault_test.rs +++ b/crates/e2e_test/src/kms/kms_vault_test.rs @@ -417,6 +417,22 @@ async fn test_vault_kms_key_crud( info!("✅ Read: Successfully listed keys, found test key"); + // A waiting window outside 7-30 days is refused at the endpoint for this + // backend too: the bound is enforced once in the service (rustfs/backlog#1585). + for days in [6, 31] { + let window_error = crate::common::execute_awscurl( + &format!("{base_url}/rustfs/admin/v3/kms/keys/delete?keyId={key_id}&pending_window_in_days={days}"), + "DELETE", + None, + access_key, + secret_key, + ) + .await + .err() + .ok_or_else(|| format!("A {days}-day deletion window must be refused"))?; + info!("✅ Delete window {} correctly refused: {}", days, window_error); + } + // Delete let delete_response = crate::common::execute_awscurl( &format!("{base_url}/rustfs/admin/v3/kms/keys/delete?keyId={key_id}"), @@ -449,9 +465,10 @@ async fn test_vault_kms_key_crud( info!("✅ Delete verification: Key state correctly changed to: {}", key_state); - // Force Delete - a default server refuses to skip the waiting window - // (rustfs/backlog#1585): destroying the key material immediately would take - // every object encrypted under the key with it. + // Force Delete - the query string can no longer ask for immediate deletion, + // and a default server refuses it in any case (rustfs/backlog#1585): + // destroying the key material immediately would take every object encrypted + // under the key with it. let force_delete_error = crate::common::execute_awscurl( &format!("{base_url}/rustfs/admin/v3/kms/keys/delete?keyId={key_id}&force_immediate=true"), "DELETE", diff --git a/crates/security-governance/src/admin_matrix.rs b/crates/security-governance/src/admin_matrix.rs index 029d72f1e..eeff42513 100644 --- a/crates/security-governance/src/admin_matrix.rs +++ b/crates/security-governance/src/admin_matrix.rs @@ -63,6 +63,14 @@ pub enum RouteRiskLevel { Normal, Sensitive, High, + /// A single authorized request can destroy stored data beyond every + /// recovery path the server offers — no undo, no waiting window, no + /// backup taken on the caller's behalf. + /// + /// This is deliberately narrower than [`Self::High`], which covers routes + /// that change state an operator can put back. Reserve it for routes whose + /// worst case is permanent loss of user data. + Critical, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] diff --git a/docs/operations/kms-backend-security.md b/docs/operations/kms-backend-security.md index 5f590bf44..99348c03c 100644 --- a/docs/operations/kms-backend-security.md +++ b/docs/operations/kms-backend-security.md @@ -63,7 +63,7 @@ Decryption loads exactly the version recorded in the envelope and fails closed w - Every version record that any stored DEK envelope references must remain readable. Until an object rewrap/migration capability exists, assume **every** version of a rotated key is referenced: destroying a version record permanently orphans all objects whose DEKs it wrapped. - Version records are ordinary KV v2 secrets under the key subtree. Never run `kv metadata delete` or `kv destroy` against `{prefix}/{key_id}/versions/*`, and do not apply `delete-version-after` or retention tooling to that subtree. RustFS-managed retention does not rely on KV2's own secret versioning (each version record has a single KV revision), so KV `max-versions` settings do not protect or endanger history — but metadata deletion always removes a record entirely. -- Permanent key deletion through RustFS (`force_immediate` after `PendingDeletion`) purges the key's version records together with the key record; that is the only supported way to remove them. It is refused by default: the server must set `RUSTFS_KMS_ALLOW_IMMEDIATE_DELETION=true`, and the request must echo the key id back as `confirm_key_id`. Leave the gate off unless you are actively destroying keys, and turn it off again afterwards — the pending-deletion window plus `CancelKeyDeletion` is the only recovery path for objects encrypted under the key. +- Permanent key deletion through RustFS (`force_immediate` after `PendingDeletion`) purges the key's version records together with the key record; that is the only supported way to remove them. It is refused by default: the server must set `RUSTFS_KMS_ALLOW_IMMEDIATE_DELETION=true`, and the request must be a `DELETE` with a JSON body that sets `force_immediate` and echoes the key id back as `confirm_key_id` — the query-parameter form (`?force_immediate=true`) is refused outright, whatever the gate is set to. Leave the gate off unless you are actively destroying keys, and turn it off again afterwards — the pending-deletion window plus `CancelKeyDeletion` is the only recovery path for objects encrypted under the key. - For Vault Transit, retention is governed by the Transit key's `min_decryption_version`: never raise it above the oldest version that may still protect live ciphertext. ### Upgrade before first rotation (hard constraint) diff --git a/rustfs/src/admin/handlers/kms_keys.rs b/rustfs/src/admin/handlers/kms_keys.rs index be4f412ea..f9d475202 100644 --- a/rustfs/src/admin/handlers/kms_keys.rs +++ b/rustfs/src/admin/handlers/kms_keys.rs @@ -371,11 +371,13 @@ impl Operation for DescribeKeyHandler { #[cfg(test)] mod tests { use super::{ - CancelKmsKeyDeletionRequest, CreateKeyApiRequest, CreateKmsKeyRequest, DeleteKmsKeyRequest, GenerateDataKeyApiRequest, - extract_key_id, kms_create_key_actions, kms_delete_key_actions, kms_describe_key_actions, kms_generate_data_key_actions, - kms_list_keys_actions, scoped_key_id, + CancelKmsKeyDeletionRequest, CreateKeyApiRequest, CreateKmsKeyRequest, DeleteKmsKeyRequest, DeleteKmsKeyResponse, + GenerateDataKeyApiRequest, delete_key_error_status, delete_request_from_query, extract_key_id, kms_create_key_actions, + kms_delete_key_actions, kms_describe_key_actions, kms_generate_data_key_actions, kms_list_keys_actions, scoped_key_id, }; use http::Uri; + use hyper::StatusCode; + use rustfs_kms::KmsError; use rustfs_policy::policy::action::{Action, AdminAction, KmsAction}; use rustfs_policy::policy::{Args, Policy}; use std::collections::HashMap; @@ -463,6 +465,98 @@ mod tests { } } + fn delete_query(query: &str) -> Result { + let uri: Uri = format!("/rustfs/admin/v3/kms/keys/delete?{query}") + .parse() + .expect("uri should parse"); + delete_request_from_query(&uri) + } + + /// The query string can no longer ask for immediate deletion in any form + /// (rustfs/backlog#1585). Refusing beats downgrading to a scheduled + /// deletion: a caller who asked for destruction must not read the answer as + /// "destroyed". + #[test] + fn delete_query_cannot_request_immediate_deletion() { + for query in [ + "keyId=key-a&force_immediate=true", + "keyId=key-a&force_immediate=True", + "keyId=key-a&force_immediate=1", + "keyId=key-a&force_immediate=", + "keyId=key-a&confirm_key_id=key-a", + "keyId=key-a&force_immediate=false&confirm_key_id=key-a", + ] { + let Err(refused) = delete_query(query) else { + panic!("{query} must be refused"); + }; + assert!(!refused.success, "{query} must not report success"); + assert_eq!(refused.key_id, "key-a"); + assert!(refused.deletion_date.is_none(), "{query} must not report a deletion date"); + assert!( + refused.message.contains("JSON body"), + "{query} must point the caller at the body form: {}", + refused.message + ); + } + } + + /// The scheduled path keeps its query form, including the explicit + /// `force_immediate=false` some clients send. + #[test] + fn delete_query_still_schedules_a_deletion() { + for (query, expected_window) in [ + ("keyId=key-a", None), + ("keyId=key-a&force_immediate=false", None), + ("keyId=key-a&pending_window_in_days=7", Some(7)), + ] { + let request = delete_query(query).expect("a scheduled deletion must still parse from the query"); + assert_eq!(request.key_id, "key-a"); + assert_eq!(request.pending_window_in_days, expected_window); + assert_eq!(request.force_immediate, None, "{query} must reach the service as scheduled"); + assert_eq!(request.confirm_key_id, None, "{query} must not carry a confirmation"); + } + } + + #[test] + fn delete_query_without_a_key_id_is_refused() { + let uri: Uri = "/rustfs/admin/v3/kms/keys/delete".parse().expect("uri should parse"); + let refused = delete_request_from_query(&uri).expect_err("a delete without a key must be refused"); + assert!(refused.message.contains("keyId")); + assert!(refused.key_id.is_empty()); + } + + /// The body form still carries both immediate-deletion fields; the service + /// gate, not the transport, decides whether they are honoured. + #[test] + fn delete_body_still_carries_the_confirmation_fields() { + let request = + serde_json::from_str::(r#"{"key_id":"key-a","force_immediate":true,"confirm_key_id":"key-a"}"#) + .expect("delete body should parse"); + + assert_eq!(request.force_immediate, Some(true)); + assert_eq!(request.confirm_key_id.as_deref(), Some("key-a")); + } + + /// A refused waiting window and a refused immediate deletion are the + /// caller's input to fix, so the endpoint answers 400 rather than blaming + /// the backend. + #[test] + fn refused_deletions_report_a_client_error() { + for error in [ + KmsError::invalid_parameter("pending_window_in_days must be between 7 and 30"), + KmsError::invalid_operation("immediate deletion of key key-a is not allowed"), + KmsError::validation_error("bad input"), + ] { + assert_eq!(delete_key_error_status(&error), StatusCode::BAD_REQUEST, "{error} must be a 400"); + } + + assert_eq!(delete_key_error_status(&KmsError::key_not_found("key-a")), StatusCode::NOT_FOUND); + assert_eq!( + delete_key_error_status(&KmsError::backend_error("vault is down")), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + #[test] fn scoped_key_id_reads_the_body_first_and_falls_back_to_the_query() { let uri: Uri = "/rustfs/admin/v3/kms/keys/delete?keyId=query-key" @@ -1043,6 +1137,66 @@ pub struct DeleteKmsKeyResponse { pub deletion_date: Option, } +const IMMEDIATE_DELETION_QUERY_RETIRED: &str = "immediate deletion is no longer accepted as a query parameter; send it as a JSON body with force_immediate and confirm_key_id set to the key id"; + +/// Delete request carried entirely by the query string, which can only ever +/// schedule a deletion. +/// +/// Immediate deletion destroys key material outright and takes every object +/// encrypted under the key with it, so it is reachable only through the JSON +/// body form (rustfs/backlog#1585): a URL travels through shell history, proxy +/// logs and browser bars, and asking for it there is far too easy to do by +/// accident. An immediate-deletion attempt made this way is refused rather +/// than downgraded to a scheduled deletion, so the caller cannot mistake one +/// outcome for the other. +fn delete_request_from_query(uri: &hyper::Uri) -> Result { + let query_params = extract_query_params(uri); + let refuse = |key_id: &str, message: &str| DeleteKmsKeyResponse { + success: false, + message: message.to_string(), + key_id: key_id.to_string(), + deletion_date: None, + }; + + let Some(key_id) = query_params.get("keyId") else { + return Err(refuse("", "missing required parameter: 'keyId'")); + }; + + // `force_immediate=false` is the scheduled path spelled out, so it stays + // accepted; anything else in that parameter is an immediate-deletion + // attempt, including a value that does not parse as a boolean. + if query_params.get("force_immediate").is_some_and(|value| value != "false") || query_params.contains_key("confirm_key_id") { + return Err(refuse(key_id, IMMEDIATE_DELETION_QUERY_RETIRED)); + } + + Ok(DeleteKmsKeyRequest { + key_id: key_id.clone(), + pending_window_in_days: query_params.get("pending_window_in_days").and_then(|s| s.parse::().ok()), + force_immediate: None, + confirm_key_id: None, + }) +} + +/// Status for a deletion the KMS refused. +/// +/// A rejected waiting window and a refused immediate deletion both arrive as +/// [`KmsError::InvalidOperation`], and both are the caller's input to fix, so +/// they must surface as 400 rather than as a server fault. +fn delete_key_error_status(error: &KmsError) -> StatusCode { + match error { + KmsError::KeyNotFound { .. } => StatusCode::NOT_FOUND, + KmsError::InvalidOperation { .. } | KmsError::ValidationError { .. } => StatusCode::BAD_REQUEST, + // Damaged or missing key material is an integrity fault of an existing + // key: it must surface as a server error, never as NOT_FOUND (the key + // exists) and never as a retryable backend outage. + KmsError::MaterialMissing { .. } + | KmsError::MaterialCorrupt { .. } + | KmsError::MaterialAuthenticationFailed { .. } + | KmsError::UnsupportedFormatVersion { .. } => StatusCode::INTERNAL_SERVER_ERROR, + _ => StatusCode::INTERNAL_SERVER_ERROR, + } +} + /// Delete a KMS key pub struct DeleteKmsKeyHandler; @@ -1081,31 +1235,15 @@ impl Operation for DeleteKmsKeyHandler { let body = body.map_err(|e| s3_error!(InvalidRequest, "failed to read request body: {}", e))?; let request: DeleteKmsKeyRequest = if body.is_empty() { - let query_params = extract_query_params(&req.uri); - let Some(key_id) = query_params.get("keyId") else { - let response = DeleteKmsKeyResponse { - success: false, - message: "missing required parameter: 'keyId'".to_string(), - key_id: "".to_string(), - deletion_date: None, - }; - let data = - serde_json::to_vec(&response).map_err(|e| s3_error!(InternalError, "failed to serialize response: {}", e))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, "application/json".parse().expect("operation should succeed")); - return Ok(S3Response::with_headers((StatusCode::BAD_REQUEST, Body::from(data)), headers)); - }; - - // Extract pending_window_in_days and force_immediate from query parameters - let pending_window_in_days = query_params.get("pending_window_in_days").and_then(|s| s.parse::().ok()); - let force_immediate = query_params.get("force_immediate").and_then(|s| s.parse::().ok()); - let confirm_key_id = query_params.get("confirm_key_id").map(|s| s.to_string()); - - DeleteKmsKeyRequest { - key_id: key_id.clone(), - pending_window_in_days, - force_immediate, - confirm_key_id, + match delete_request_from_query(&req.uri) { + Ok(request) => request, + Err(response) => { + let data = serde_json::to_vec(&response) + .map_err(|e| s3_error!(InternalError, "failed to serialize response: {}", e))?; + let mut headers = HeaderMap::new(); + headers.insert(CONTENT_TYPE, "application/json".parse().expect("operation should succeed")); + return Ok(S3Response::with_headers((StatusCode::BAD_REQUEST, Body::from(data)), headers)); + } } } else { serde_json::from_slice(&body).map_err(|e| s3_error!(InvalidRequest, "invalid JSON: {}", e))? @@ -1183,18 +1321,7 @@ impl Operation for DeleteKmsKeyHandler { error = %e, "admin kms keys state" ); - let status = match &e { - KmsError::KeyNotFound { .. } => StatusCode::NOT_FOUND, - KmsError::InvalidOperation { .. } | KmsError::ValidationError { .. } => StatusCode::BAD_REQUEST, - // Damaged or missing key material is an integrity fault of an existing - // key: it must surface as a server error, never as NOT_FOUND (the key - // exists) and never as a retryable backend outage. - KmsError::MaterialMissing { .. } - | KmsError::MaterialCorrupt { .. } - | KmsError::MaterialAuthenticationFailed { .. } - | KmsError::UnsupportedFormatVersion { .. } => StatusCode::INTERNAL_SERVER_ERROR, - _ => StatusCode::INTERNAL_SERVER_ERROR, - }; + let status = delete_key_error_status(&e); let response = DeleteKmsKeyResponse { success: false, message: format!("Failed to delete key: {e}"), diff --git a/rustfs/src/admin/route_policy.rs b/rustfs/src/admin/route_policy.rs index 0f7c78115..568e5d348 100644 --- a/rustfs/src/admin/route_policy.rs +++ b/rustfs/src/admin/route_policy.rs @@ -766,11 +766,16 @@ pub const ADMIN_ROUTE_POLICY_SPECS: &[AdminRouteSpec] = &[ ), admin(HttpMethod::Post, "/rustfs/admin/v3/kms/reconfigure", KMS_CONFIGURE, RouteRiskLevel::High), admin(HttpMethod::Post, "/rustfs/admin/v3/kms/keys", KMS_CONFIGURE, RouteRiskLevel::High), + // Critical rather than High: destroying a master key makes every object + // encrypted under it permanently unreadable, and nothing on the server can + // bring those objects back (rustfs/backlog#1585). The waiting window and + // cancel-deletion are the only recovery path, which is why the route below + // that reopens it stays merely High. admin( HttpMethod::Delete, "/rustfs/admin/v3/kms/keys/delete", KMS_DELETE_KEY, - RouteRiskLevel::High, + RouteRiskLevel::Critical, ), admin( HttpMethod::Post, @@ -1880,6 +1885,39 @@ mod tests { } } + /// `Critical` marks the routes whose worst case is permanent loss of user + /// data, so the inventory is pinned in both directions: the KMS key + /// deletion route may not be quietly downgraded, and no other route may be + /// promoted without the same review. + #[test] + fn route_policy_reserves_critical_risk_for_unrecoverable_destruction() { + let critical = ADMIN_ROUTE_POLICY_SPECS + .iter() + .filter(|spec| spec.risk_level() == RouteRiskLevel::Critical) + .map(|spec| route_key(spec.method(), spec.path())) + .collect::>(); + + assert_eq!( + critical, + BTreeSet::from([route_key(HttpMethod::Delete, "/rustfs/admin/v3/kms/keys/delete")]), + "the Critical set changed; classify the new route deliberately or restore the old one" + ); + } + + /// The routes that recover from, or merely describe, a pending deletion are + /// not Critical: refusing them is what leaves an operator without a way + /// back. + #[test] + fn route_policy_keeps_kms_deletion_recovery_below_critical() { + for (method, path) in [ + (HttpMethod::Post, "/rustfs/admin/v3/kms/keys/cancel-deletion"), + (HttpMethod::Get, "/rustfs/admin/v3/kms/keys/{key_id}"), + (HttpMethod::Get, "/rustfs/admin/v3/kms/keys"), + ] { + assert_risk_below_critical(method, path); + } + } + /// Backup and restore expose the whole key inventory at once, so no other /// KMS action may reach them: holding `kms:Configure` or a per-key action /// must not be enough. @@ -2039,6 +2077,14 @@ mod tests { assert_ne!(spec.access().admin_action(), Some(action)); } + fn assert_risk_below_critical(method: HttpMethod, path: &str) { + let spec = ADMIN_ROUTE_POLICY_SPECS + .iter() + .find(|spec| spec.method() == method && spec.path() == path) + .expect("expected direct route policy"); + assert_ne!(spec.risk_level(), RouteRiskLevel::Critical, "{path} must not be Critical"); + } + fn assert_public(method: HttpMethod, path: &str, kind: PublicRouteKind) { let spec = ADMIN_ROUTE_POLICY_SPECS .iter()