feat(admin): apply the requested key listing filters (#5608)

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<String> = (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<String> = 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<String> = 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::<Vec<_>>());
+        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() {

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<DescribeKeyResponse>;
 
-    /// 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<ListKeysResponse>;
 
     /// Delete a key

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<ListKeysResponse> {
         // 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 {
+            return Ok(empty_key_page());
-                keys: vec![],
+        }
-                next_marker: None,
+
-                truncated: false,
+        // 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;

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<String, String> {
     let mut params = HashMap::new();
     if let Some(query) = uri.query() {
-        query.split('&').for_each(|pair| {
+        for (key, value) in url::form_urlencoded::parse(query.as_bytes()) {
-            if let Some((key, value)) = pair.split_once('=') {
+            params.insert(key.into_owned(), value.into_owned());
-                params.insert(
+        }
-                    urlencoding::decode(key).unwrap_or_default().into_owned(),
-                    urlencoding::decode(value).unwrap_or_default().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<KeyStatus>,
+    usage: Option<KeyUsage>,
+}
+
+/// 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<T: Clone>(name: &str, value: &str, accepted: &[(&str, T)]) -> Result<T, String> {
+    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<String, String>) -> Result<KeyListFilters, String> {
+    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<String> {
     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,
+        extract_query_params, key_impact_if_requested, key_list_filters, kms_create_key_actions, kms_delete_key_actions,
-        kms_generate_data_key_actions, kms_list_keys_actions, scoped_key_id, wants_key_impact,
+        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<KeyStatus>, Option<KeyUsage>), 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::<u32>().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,
+            status_filter: filters.status,
-            usage_filter: None,
+            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::<u32>().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,
+            status_filter: filters.status,
-            usage_filter: None,
+            usage_filter: filters.usage,
         };
 
         match manager.list_keys_with_context(kms_request, audit.context()).await {