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)

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,

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",

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)]

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)

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<DeleteKmsKeyRequest, DeleteKmsKeyResponse> {
+        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::<DeleteKmsKeyRequest>(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<String>,
 }
 
+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<DeleteKmsKeyRequest, DeleteKmsKeyResponse> {
+    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::<u32>().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::<u32>().ok());
-            let force_immediate = query_params.get("force_immediate").and_then(|s| s.parse::<bool>().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}"),

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::<BTreeSet<_>>();
+
+        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()