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