From 77b712dcb61b8bd214add69b527f6ab9bd108cfb Mon Sep 17 00:00:00 2001 From: overtrue Date: Sun, 2 Aug 2026 18:36:07 +0800 Subject: [PATCH] fix(kms): harden key list contract --- crates/kms/src/backends/local.rs | 20 +++++++ crates/kms/src/backends/mod.rs | 19 ++++++- crates/kms/src/backends/static_kms.rs | 28 ++++++++- crates/kms/src/backends/vault.rs | 35 +++++++++++- crates/kms/src/types.rs | 12 +++- rustfs/src/admin/handlers/kms_keys.rs | 81 +++++++++++++++++++++++++-- 6 files changed, 182 insertions(+), 13 deletions(-) diff --git a/crates/kms/src/backends/local.rs b/crates/kms/src/backends/local.rs index 1509f89e7..c957a2e38 100644 --- a/crates/kms/src/backends/local.rs +++ b/crates/kms/src/backends/local.rs @@ -3101,6 +3101,26 @@ mod tests { ); } + #[tokio::test] + async fn list_keys_fails_closed_on_a_corrupt_record() { + let (client, _temp_dir) = create_test_client().await; + client.create_key("corrupt", "AES_256", None).await.expect("create key"); + + let key_path = client.master_key_path("corrupt").expect("valid key id"); + fs::write(&key_path, b"{\"key_id\":\"corrupt\"") + .await + .expect("write corrupt record"); + + let error = client + .list_keys(&ListKeysRequest::default(), None) + .await + .expect_err("a corrupt record must not disappear from a listing"); + assert!( + matches!(error, KmsError::MaterialCorrupt { key_id, .. } if key_id == "corrupt"), + "got {error:?}" + ); + } + #[tokio::test] async fn missing_salt_with_pre_marker_key_records_still_initializes() { let (dev_client, temp_dir) = create_dev_mode_client().await; diff --git a/crates/kms/src/backends/mod.rs b/crates/kms/src/backends/mod.rs index a0378a39d..92d568cf7 100644 --- a/crates/kms/src/backends/mod.rs +++ b/crates/kms/src/backends/mod.rs @@ -160,7 +160,7 @@ pub(crate) fn ensure_rewrap_context_matches( } /// Page size used when a [`ListKeysRequest`] does not ask for one. -pub(crate) const DEFAULT_LIST_KEYS_PAGE_SIZE: u32 = 100; +pub(crate) const DEFAULT_LIST_KEYS_PAGE_SIZE: u32 = DEFAULT_LIST_KEYS_LIMIT; /// One page of a key set the backend has to slice itself. pub(crate) struct KeyPage<'a, T> { @@ -231,7 +231,7 @@ pub(crate) fn paginate_keys<'a, T>(sorted: &'a [T], request: &ListKeysRequest, k pub(crate) fn list_keys_page_size(limit: Option) -> Option { match limit.unwrap_or(DEFAULT_LIST_KEYS_PAGE_SIZE) { 0 => None, - size => Some(size as usize), + size => Some(size.min(MAX_LIST_KEYS_LIMIT) as usize), } } @@ -860,6 +860,21 @@ mod tests { assert_eq!(page_of(&keys, Some(u32::MAX), Some("key-01")), (vec![keys[2].clone()], None, false)); } + #[test] + fn list_limit_is_bounded_at_the_contract_maximum() { + assert_eq!(list_keys_page_size(Some(MAX_LIST_KEYS_LIMIT)), Some(MAX_LIST_KEYS_LIMIT as usize)); + assert_eq!( + list_keys_page_size(Some(MAX_LIST_KEYS_LIMIT + 1)), + Some(MAX_LIST_KEYS_LIMIT as usize), + "one past the maximum must not expand a backend page" + ); + assert_eq!( + list_keys_page_size(Some(u32::MAX)), + Some(MAX_LIST_KEYS_LIMIT as usize), + "the largest wire value must use the same bounded page" + ); + } + /// The cursor is an identifier, so a marker naming a key that no longer /// exists resumes after where it would have been instead of restarting. #[test] diff --git a/crates/kms/src/backends/static_kms.rs b/crates/kms/src/backends/static_kms.rs index d28d1fe3f..2df1b39a0 100644 --- a/crates/kms/src/backends/static_kms.rs +++ b/crates/kms/src/backends/static_kms.rs @@ -59,6 +59,12 @@ pub struct StaticKmsBackend { key_id: String, /// The raw 32-byte AES-256 key material (zeroed on drop). key: Zeroizing<[u8; KEY_SIZE]>, + /// The time this configured key became visible to the backend. + /// + /// Static KMS has no persisted key record, so this startup timestamp is + /// the only stable creation date it can report across list and describe + /// calls. It must not be regenerated for every response. + created_at: Zoned, } impl StaticKmsBackend { @@ -82,6 +88,7 @@ impl StaticKmsBackend { Ok(Self { key_id: static_config.key_id.clone(), key: key.into(), + created_at: Zoned::now(), }) } @@ -92,7 +99,7 @@ impl StaticKmsBackend { key_state: KeyState::Enabled, key_usage: KeyUsage::EncryptDecrypt, description: Some("Static single-key KMS backend".to_string()), - creation_date: Zoned::now(), + creation_date: self.created_at.clone(), deletion_date: None, origin: "EXTERNAL".to_string(), key_manager: "STATIC".to_string(), @@ -281,7 +288,7 @@ impl StaticKmsBackend { version: 1, metadata: HashMap::new(), tags: HashMap::new(), - created_at: Zoned::now(), + created_at: self.created_at.clone(), rotated_at: None, created_by: None, }; @@ -677,6 +684,23 @@ mod tests { assert!(!response.truncated); } + #[tokio::test] + async fn list_key_creation_date_is_stable_for_the_backend_lifetime() { + let (backend, key_id, _key) = create_test_backend().await; + + let first = backend + .list_configured_key(&ListKeysRequest::default()) + .expect("first list should succeed"); + tokio::task::yield_now().await; + let second = backend + .list_configured_key(&ListKeysRequest::default()) + .expect("second list should succeed"); + let described = backend.configured_key_info(&key_id).expect("describe should succeed"); + + assert_eq!(first.keys[0].created_at, second.keys[0].created_at); + assert_eq!(first.keys[0].created_at, described.created_at); + } + /// A zero limit means zero keys, even for a backend whose whole key set is /// a single configured key. #[tokio::test] diff --git a/crates/kms/src/backends/vault.rs b/crates/kms/src/backends/vault.rs index abfe2682a..fd556d21a 100644 --- a/crates/kms/src/backends/vault.rs +++ b/crates/kms/src/backends/vault.rs @@ -1213,8 +1213,13 @@ impl VaultKmsClient { // A key that disappeared between the listing and the read is // dropped from the page rather than failing it; the cursor comes // from the identifier list, so the listing still advances past it. - let Ok(key_info) = self.describe_key(key_id, None).await else { - continue; + // Any other read error means the record is present but this build + // cannot interpret it. Hiding that entry would make the listing + // claim a complete key set while silently omitting corrupt data. + let key_info = match self.describe_key(key_id, None).await { + Ok(key_info) => key_info, + Err(KmsError::KeyNotFound { .. }) => continue, + Err(error) => return Err(error), }; if request .status_filter @@ -1955,6 +1960,32 @@ mod tests { ); } + #[tokio::test] + async fn list_keys_fails_closed_on_an_unreadable_kv2_record() { + let malformed_record = serde_json::json!({ + "data": {"algorithm": "AES_256"}, + "metadata": { + "created_time": "2026-01-01T00:00:00Z", + "deletion_time": "", + "custom_metadata": null, + "destroyed": false, + "version": 1 + } + }); + let (vault, client) = scripted_client(vec![ + ScriptedResponse::ok(serde_json::json!({ "keys": ["poisoned"] })), + ScriptedResponse::ok(malformed_record), + ]) + .await; + + let error = client + .list_keys(&ListKeysRequest::default(), None) + .await + .expect_err("a malformed KV2 record must not disappear from a listing"); + assert!(matches!(error, KmsError::BackendError { .. }), "got {error:?}"); + assert_eq!(vault.requests().len(), 2, "listing must read the listed record before failing"); + } + #[tokio::test] async fn wired_read_retries_transient_status_then_succeeds() { let (vault, client) = scripted_client(vec![ diff --git a/crates/kms/src/types.rs b/crates/kms/src/types.rs index a19eae996..53eef32bf 100644 --- a/crates/kms/src/types.rs +++ b/crates/kms/src/types.rs @@ -415,7 +415,15 @@ impl DecryptRequest { } } -/// Request to list keys +/// Default page size used when a request omits `limit`. +pub const DEFAULT_LIST_KEYS_LIMIT: u32 = 100; +/// Maximum page size accepted by KMS list implementations. +pub const MAX_LIST_KEYS_LIMIT: u32 = 1000; + +/// Request to list keys. +/// +/// List pages default to [`DEFAULT_LIST_KEYS_LIMIT`] entries and are capped at +/// [`MAX_LIST_KEYS_LIMIT`]. A zero limit is a valid request for an empty page. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ListKeysRequest { /// Maximum number of keys to return @@ -431,7 +439,7 @@ pub struct ListKeysRequest { impl Default for ListKeysRequest { fn default() -> Self { Self { - limit: Some(100), + limit: Some(DEFAULT_LIST_KEYS_LIMIT), marker: None, usage_filter: None, status_filter: None, diff --git a/rustfs/src/admin/handlers/kms_keys.rs b/rustfs/src/admin/handlers/kms_keys.rs index 29fd16ec7..6c08e6fb4 100644 --- a/rustfs/src/admin/handlers/kms_keys.rs +++ b/rustfs/src/admin/handlers/kms_keys.rs @@ -180,6 +180,21 @@ fn key_list_filters(query_params: &HashMap) -> Result) -> Result { + let Some(value) = query_params.get("limit") else { + return Ok(DEFAULT_LIST_KEYS_LIMIT); + }; + + value + .parse::() + .map(|limit| limit.min(MAX_LIST_KEYS_LIMIT)) + .map_err(|_| format!("invalid value for 'limit': expected an unsigned integer, got '{value}'")) +} + fn extract_key_id(uri: &hyper::Uri) -> Option { let query_params = extract_query_params(uri); ["keyId", "key-id", "key"] @@ -443,12 +458,16 @@ mod tests { use super::{ CancelKmsKeyDeletionRequest, CreateKeyApiRequest, CreateKmsKeyRequest, DeleteKmsKeyRequest, DeleteKmsKeyResponse, DescribeKmsKeyResponse, GenerateDataKeyApiRequest, delete_key_error_status, delete_request_from_query, extract_key_id, - extract_query_params, key_impact_if_requested, key_list_filters, kms_create_key_actions, kms_delete_key_actions, - kms_describe_key_actions, kms_generate_data_key_actions, kms_list_keys_actions, scoped_key_id, wants_key_impact, + extract_query_params, key_impact_if_requested, key_list_filters, key_list_limit, kms_create_key_actions, + kms_delete_key_actions, kms_describe_key_actions, kms_generate_data_key_actions, kms_list_keys_actions, scoped_key_id, + wants_key_impact, }; use http::Uri; use hyper::StatusCode; - use rustfs_kms::{KeyImpactReport, KeyReference, KeyReferenceKind, KeyStatus, KeyUsage, KmsError, ReferenceScope}; + use rustfs_kms::{ + DEFAULT_LIST_KEYS_LIMIT, KeyImpactReport, KeyReference, KeyReferenceKind, KeyStatus, KeyUsage, KmsError, + MAX_LIST_KEYS_LIMIT, ReferenceScope, + }; use rustfs_policy::policy::action::{Action, AdminAction, KmsAction}; use rustfs_policy::policy::{Args, Policy}; use std::collections::HashMap; @@ -996,6 +1015,42 @@ mod tests { key_list_filters(&extract_query_params(&uri)).map(|filters| (filters.status, filters.usage)) } + fn list_limit(query: &str) -> Result { + let uri: Uri = format!("/rustfs/admin/v3/kms/keys{query}").parse().expect("uri should parse"); + key_list_limit(&extract_query_params(&uri)) + } + + #[test] + fn a_key_listing_limit_is_bounded_and_malformed_values_are_refused() { + assert_eq!(list_limit(""), Ok(DEFAULT_LIST_KEYS_LIMIT)); + assert_eq!(list_limit("?limit=0"), Ok(0)); + assert_eq!(list_limit(&format!("?limit={MAX_LIST_KEYS_LIMIT}")), Ok(MAX_LIST_KEYS_LIMIT)); + assert_eq!( + list_limit(&format!("?limit={}", MAX_LIST_KEYS_LIMIT + 1)), + Ok(MAX_LIST_KEYS_LIMIT), + "one past the maximum must be clamped" + ); + assert_eq!(list_limit("?limit=4294967295"), Ok(MAX_LIST_KEYS_LIMIT)); + + for query in ["?limit=abc", "?limit=-1", "?limit="] { + let error = list_limit(query).expect_err("malformed limits must not fall back to the default"); + assert!(error.contains("invalid value for 'limit'"), "unhelpful error for {query}: {error}"); + } + } + + #[test] + fn both_list_handlers_validate_the_requested_limit() { + let src = include_str!("kms_keys.rs"); + + for handler in ["ListKeysHandler", "ListKmsKeysHandler"] { + let block = operation_block(src, handler); + assert!( + block.contains("key_list_limit(&query_params)"), + "{handler} must reject malformed limits before reaching the KMS" + ); + } + } + /// A filter is either applied or refused. Answering a narrowed listing with /// the unfiltered key set looks, from the response alone, exactly like a key /// set in which everything matches — the caller cannot tell that its @@ -1138,7 +1193,7 @@ impl Operation for ListKeysHandler { )?; let query_params = extract_query_params(&req.uri); - let limit = query_params.get("limit").and_then(|s| s.parse::().ok()).unwrap_or(100); + let limit = key_list_limit(&query_params).map_err(|message| s3_error!(InvalidArgument, "{}", message))?; let marker = query_params.get("marker").cloned(); // Validated before the listing runs, so a filter the service cannot // apply fails as the input error it is instead of returning the whole @@ -1878,7 +1933,23 @@ impl Operation for ListKmsKeysHandler { )?; let query_params = extract_query_params(&req.uri); - let limit = query_params.get("limit").and_then(|s| s.parse::().ok()).unwrap_or(100); + let limit = match key_list_limit(&query_params) { + Ok(limit) => limit, + Err(message) => { + let response = ListKmsKeysResponse { + success: false, + message, + keys: vec![], + truncated: false, + next_marker: 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)); + } + }; let marker = query_params.get("marker").cloned(); // Validated before the listing runs, so a filter the service cannot // apply fails as the input error it is instead of returning the whole