diff --git a/crates/kms/src/backends/local.rs b/crates/kms/src/backends/local.rs index bf5a2f20a..1509f89e7 100644 --- a/crates/kms/src/backends/local.rs +++ b/crates/kms/src/backends/local.rs @@ -3298,6 +3298,13 @@ mod tests { } } + fn filtered_page_request(limit: u32, marker: Option<&str>, status: KeyStatus) -> ListKeysRequest { + ListKeysRequest { + status_filter: Some(status), + ..page_request(limit, marker) + } + } + async fn create_keys(client: &LocalKmsClient, key_ids: &[String]) { for key_id in key_ids { client @@ -3398,6 +3405,69 @@ mod tests { assert!(response.next_marker.is_none()); } + /// A filter narrows a page after it has been cut, so a page can come back + /// empty while keys still remain. The traversal has to continue on + /// `truncated` rather than on a short page, and the cursor has to advance + /// across the keys the filter removed — otherwise a filtered listing ends + /// at the first run of non-matching keys and reports a partial key set as + /// the whole one. + #[tokio::test] + async fn filtered_paging_crosses_a_page_that_matches_nothing() { + let (client, _temp_dir) = create_test_client().await; + let key_ids: Vec = (0..12).map(|index| format!("filter-key-{index:02}")).collect(); + create_keys(&client, &key_ids).await; + // A page worth of adjacent keys is excluded, so one page of the + // traversal matches nothing at all. + for key_id in &key_ids[3..6] { + client.disable_key(key_id, None).await.expect("key should be disabled"); + } + let still_active: Vec = key_ids + .iter() + .enumerate() + .filter(|(index, _)| !(3..6).contains(index)) + .map(|(_, key_id)| key_id.clone()) + .collect(); + + let mut seen = Vec::new(); + let mut pages_without_a_match = 0; + let mut marker: Option = None; + // Bounded so a listing that cannot advance fails the assertions below + // instead of hanging the test run. + for _ in 0..key_ids.len() + 1 { + let response = client + .list_keys(&filtered_page_request(3, marker.as_deref(), KeyStatus::Active), None) + .await + .expect("list should succeed"); + assert!(response.keys.len() <= 3, "a page must not exceed the requested limit"); + assert!( + response.keys.iter().all(|key| key.status == KeyStatus::Active), + "a filtered page must carry matches only" + ); + if response.keys.is_empty() { + pages_without_a_match += 1; + } + seen.extend(response.keys.iter().map(|key| key.key_id.clone())); + if !response.truncated { + assert!(response.next_marker.is_none(), "a final page must not offer a cursor"); + break; + } + marker = Some(response.next_marker.expect("a truncated page must offer a cursor")); + } + + assert_eq!(seen, still_active, "filtered paging must visit every match exactly once"); + assert_eq!(pages_without_a_match, 1, "the excluded run must produce a page with no match"); + + // The keys the first filter excluded are exactly the ones the opposite + // filter lists, so nothing fell out of the key set on the way. + let response = client + .list_keys(&filtered_page_request(key_ids.len() as u32, None, KeyStatus::Disabled), None) + .await + .expect("list should succeed"); + let listed: Vec<&str> = response.keys.iter().map(|key| key.key_id.as_str()).collect(); + assert_eq!(listed, key_ids[3..6].iter().map(String::as_str).collect::>()); + assert!(!response.truncated, "a page covering the whole key set is complete"); + } + /// A limit past the end of the key set returns everything, once. #[tokio::test] async fn list_keys_limit_beyond_the_key_set_returns_one_complete_page() { diff --git a/crates/kms/src/backends/mod.rs b/crates/kms/src/backends/mod.rs index 1a3db7d0f..a0378a39d 100644 --- a/crates/kms/src/backends/mod.rs +++ b/crates/kms/src/backends/mod.rs @@ -267,7 +267,18 @@ pub trait KmsBackend: Send + Sync { /// Describe a key async fn describe_key(&self, request: DescribeKeyRequest) -> Result; - /// List keys + /// List keys. + /// + /// `status_filter` and `usage_filter` narrow a page after it has been cut, + /// so a filtered page can be shorter than the requested limit — and even + /// empty — while keys remain: a caller must page until `truncated` is false + /// rather than until a page comes back short. Every backend applies both + /// filters; a backend that cannot answer a filter must fail rather than + /// return the unfiltered set. + /// + /// Backends that slice their own key set do so with [`paginate_keys`], + /// which fixes the ordering and the marker semantics; a backend paging + /// through a remote API passes that API's own cursor through instead. async fn list_keys(&self, request: ListKeysRequest) -> Result; /// Delete a key diff --git a/crates/kms/src/backends/static_kms.rs b/crates/kms/src/backends/static_kms.rs index e9ba4027a..d28d1fe3f 100644 --- a/crates/kms/src/backends/static_kms.rs +++ b/crates/kms/src/backends/static_kms.rs @@ -263,7 +263,8 @@ impl StaticKmsBackend { }) } - /// List the single configured key, honouring the pagination marker. + /// List the single configured key, honouring the pagination marker and the + /// status and usage filters. pub(crate) fn list_configured_key(&self, request: &ListKeysRequest) -> Result { // A caller asking for no keys gets none, even from a backend whose // whole key set is one key. @@ -285,15 +286,25 @@ impl StaticKmsBackend { created_by: None, }; - // Apply prefix filter if provided + // The marker is an exclusive lower bound on the identifier, as it is + // for every other backend. if let Some(ref marker) = request.marker && self.key_id <= *marker { - return Ok(ListKeysResponse { - keys: vec![], - next_marker: None, - truncated: false, - }); + return Ok(empty_key_page()); + } + + // The configured key is filtered like any other: a caller narrowing the + // listing to disabled or signing keys must get an empty page rather + // than this active encryption key, which it would otherwise have to + // recognise as a non-match on its own. + if request + .status_filter + .as_ref() + .is_some_and(|status| status != &key_info.status) + || request.usage_filter.as_ref().is_some_and(|usage| usage != &key_info.usage) + { + return Ok(empty_key_page()); } Ok(ListKeysResponse { @@ -683,6 +694,42 @@ mod tests { assert!(response.next_marker.is_none()); } + /// A filter that excludes the configured key must empty the page. Handing + /// the key back regardless would answer "list the disabled keys" with an + /// active one, and the response says nothing about the filter having been + /// dropped. + #[tokio::test] + async fn a_filter_the_configured_key_does_not_match_empties_the_page() { + let (backend, key_id, _key) = create_test_backend().await; + + for request in [ + ListKeysRequest { + status_filter: Some(KeyStatus::Disabled), + ..Default::default() + }, + ListKeysRequest { + usage_filter: Some(KeyUsage::SignVerify), + ..Default::default() + }, + ] { + let response = backend.list_configured_key(&request).expect("a filtered list must succeed"); + assert!(response.keys.is_empty(), "excluded key was listed for {request:?}"); + assert!(!response.truncated); + assert!(response.next_marker.is_none()); + } + + // The filters the key does match still list it. + let response = backend + .list_configured_key(&ListKeysRequest { + status_filter: Some(KeyStatus::Active), + usage_filter: Some(KeyUsage::EncryptDecrypt), + ..Default::default() + }) + .expect("a matching list must succeed"); + assert_eq!(response.keys.len(), 1); + assert_eq!(response.keys[0].key_id, key_id); + } + #[tokio::test] async fn lifecycle_mutations_are_unsupported_at_the_product_surface() { let (backend, key_id, _key) = create_test_backend().await; diff --git a/rustfs/src/admin/handlers/kms_keys.rs b/rustfs/src/admin/handlers/kms_keys.rs index 765d027e0..29fd16ec7 100644 --- a/rustfs/src/admin/handlers/kms_keys.rs +++ b/rustfs/src/admin/handlers/kms_keys.rs @@ -33,7 +33,6 @@ use serde::{Deserialize, Serialize}; use serde_json; use std::collections::HashMap; use tracing::{error, info}; -use urlencoding; const LOG_COMPONENT_ADMIN: &str = "admin"; const LOG_SUBSYSTEM_KMS_KEYS: &str = "kms_keys"; @@ -96,21 +95,91 @@ pub struct GenerateDataKeyApiResponse { pub ciphertext_blob: String, // Base64 encoded } +/// The query parameters of an admin KMS request. +/// +/// Parsed with `form_urlencoded`, as the rest of the admin surface does, so a +/// parameter written without a value (`?status`) arrives as an empty value +/// rather than disappearing: a validated parameter must be able to tell "not +/// asked for" from "asked for, unreadable". pub(super) fn extract_query_params(uri: &hyper::Uri) -> HashMap { let mut params = HashMap::new(); if let Some(query) = uri.query() { - query.split('&').for_each(|pair| { - if let Some((key, value)) = pair.split_once('=') { - params.insert( - urlencoding::decode(key).unwrap_or_default().into_owned(), - urlencoding::decode(value).unwrap_or_default().into_owned(), - ); - } - }); + for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { + params.insert(key.into_owned(), value.into_owned()); + } } params } +/// Status values a `status` filter may name, spelled as the response spells +/// them. +const KEY_STATUS_FILTERS: &[(&str, KeyStatus)] = &[ + ("Active", KeyStatus::Active), + ("Disabled", KeyStatus::Disabled), + ("PendingDeletion", KeyStatus::PendingDeletion), + ("Deleted", KeyStatus::Deleted), +]; + +/// Usage values a `usage` filter may name, spelled as the response spells them. +const KEY_USAGE_FILTERS: &[(&str, KeyUsage)] = &[ + ("EncryptDecrypt", KeyUsage::EncryptDecrypt), + ("SignVerify", KeyUsage::SignVerify), +]; + +/// The status and usage filters of a key listing. +struct KeyListFilters { + status: Option, + usage: Option, +} + +/// A filter value with casing and word separators folded away, so the spelling +/// the response uses (`PendingDeletion`) and the AWS-shaped one +/// (`PENDING_DELETION`) name the same filter. +fn canonical_filter_value(value: &str) -> String { + value + .chars() + .filter(|character| !matches!(character, '_' | '-')) + .map(|character| character.to_ascii_lowercase()) + .collect() +} + +fn parse_key_filter(name: &str, value: &str, accepted: &[(&str, T)]) -> Result { + let wanted = canonical_filter_value(value); + accepted + .iter() + .find(|(spelling, _)| canonical_filter_value(spelling) == wanted) + .map(|(_, filter)| filter.clone()) + .ok_or_else(|| { + let spellings: Vec<&str> = accepted.iter().map(|(spelling, _)| *spelling).collect(); + format!("invalid value for '{name}': expected one of {}, got '{value}'", spellings.join(", ")) + }) +} + +/// The `status` and `usage` filters a key listing was asked to apply. +/// +/// An absent parameter means "no filter". A parameter that is present but names +/// no known status or usage — an empty value included — is refused rather than +/// dropped: a listing that ignored the filter would answer "show me the keys +/// pending deletion" with every key there is, and nothing in the response would +/// tell the caller that the narrowing never happened. +/// +/// The filters narrow a page after the backend has cut it, so a filtered page +/// can be shorter than `limit` — even empty — while `truncated` is still true. +/// Callers must page until `truncated` is false rather than until a page comes +/// back short. +fn key_list_filters(query_params: &HashMap) -> Result { + Ok(KeyListFilters { + status: query_params + .get("status") + .map(|value| parse_key_filter("status", value, KEY_STATUS_FILTERS)) + .transpose()?, + usage: query_params + .get("usage") + .map(|value| parse_key_filter("usage", value, KEY_USAGE_FILTERS)) + .transpose()?, + }) +} + fn extract_key_id(uri: &hyper::Uri) -> Option { let query_params = extract_query_params(uri); ["keyId", "key-id", "key"] @@ -374,12 +443,12 @@ mod tests { use super::{ CancelKmsKeyDeletionRequest, CreateKeyApiRequest, CreateKmsKeyRequest, DeleteKmsKeyRequest, DeleteKmsKeyResponse, DescribeKmsKeyResponse, GenerateDataKeyApiRequest, delete_key_error_status, delete_request_from_query, extract_key_id, - key_impact_if_requested, 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, 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, KmsError, ReferenceScope}; + use rustfs_kms::{KeyImpactReport, KeyReference, KeyReferenceKind, KeyStatus, KeyUsage, KmsError, ReferenceScope}; use rustfs_policy::policy::action::{Action, AdminAction, KmsAction}; use rustfs_policy::policy::{Args, Policy}; use std::collections::HashMap; @@ -922,6 +991,71 @@ mod tests { } } + fn list_filters(query: &str) -> Result<(Option, Option), String> { + let uri: Uri = format!("/rustfs/admin/v3/kms/keys{query}").parse().expect("uri should parse"); + key_list_filters(&extract_query_params(&uri)).map(|filters| (filters.status, filters.usage)) + } + + /// 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 + /// `status=PendingDeletion` never reached the backend. + #[test] + fn a_key_listing_filter_is_either_applied_or_refused() { + assert_eq!(list_filters(""), Ok((None, None)), "an unfiltered listing must stay unfiltered"); + assert_eq!( + list_filters("?status=PendingDeletion&usage=EncryptDecrypt"), + Ok((Some(KeyStatus::PendingDeletion), Some(KeyUsage::EncryptDecrypt))) + ); + + // The spelling the response carries and the AWS-shaped one name the + // same filter, so a value read off a listing can be sent straight back. + for query in [ + "?status=pending_deletion", + "?status=PENDING-DELETION", + "?status=pendingdeletion", + ] { + assert_eq!(list_filters(query), Ok((Some(KeyStatus::PendingDeletion), None)), "{query}"); + } + + // Present but unreadable — including a value-less or empty parameter — + // is refused instead of read as "no filter". + for (query, name) in [ + ("?status=enabled", "status"), + ("?status=", "status"), + ("?status", "status"), + ("?usage=encrypt", "usage"), + ("?usage=", "usage"), + ("?status=Active&usage=sign", "usage"), + ] { + let error = list_filters(query).expect_err("an unreadable filter must be refused"); + assert!( + error.contains(&format!("invalid value for '{name}'")), + "unhelpful message for {query}: {error}" + ); + } + } + + /// Both list endpoints must hand the caller's filters to the KMS. Pinning + /// them to `None` — as both did before they were wired up — answers a + /// narrowed listing with every key there is. + #[test] + fn both_list_handlers_pass_the_requested_filters_to_the_kms() { + let src = include_str!("kms_keys.rs"); + + for handler in ["ListKeysHandler", "ListKmsKeysHandler"] { + let block = operation_block(src, handler); + assert!( + block.contains("key_list_filters(&query_params)"), + "{handler} must read the requested filters" + ); + assert!( + block.contains("status_filter: filters.status") && block.contains("usage_filter: filters.usage"), + "{handler} must pass the requested filters to the KMS" + ); + } + } + fn describe_uri(query: &str) -> Uri { format!("/rustfs/admin/v3/kms/keys/key-a{query}") .parse() @@ -1006,6 +1140,10 @@ 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 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 + // key set as if it had been narrowed. + let filters = key_list_filters(&query_params).map_err(|message| s3_error!(InvalidArgument, "{}", message))?; let Some(service) = kms_encryption_service_from_context().await else { return Err(s3_error!(InternalError, "kms service is not initialized")); @@ -1014,8 +1152,8 @@ impl Operation for ListKeysHandler { let request = ListKeysRequest { limit: Some(limit), marker, - status_filter: None, - usage_filter: None, + status_filter: filters.status, + usage_filter: filters.usage, }; match service.list_keys_with_context(request, audit.context()).await { @@ -1742,6 +1880,26 @@ 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 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 + // key set as if it had been narrowed. + let filters = match key_list_filters(&query_params) { + Ok(filters) => filters, + 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 Some(service_manager) = kms_service_manager_from_context() else { let response = ListKmsKeysResponse { @@ -1776,8 +1934,8 @@ impl Operation for ListKmsKeysHandler { let kms_request = ListKeysRequest { limit: Some(limit), marker, - status_filter: None, - usage_filter: None, + status_filter: filters.status, + usage_filter: filters.usage, }; match manager.list_keys_with_context(kms_request, audit.context()).await {