From 7710f70fda979bf87a732ab0c814474fae64e5cb Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 13 Aug 2026 23:21:37 +0800 Subject: [PATCH] feat(kms): report a key as due for rotation once its wrap budget is spent (#6059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(kms): construct wrap_budget_reserved in the VaultKeyData deserializer main does not compile: #6019 added VaultKeyData.wrap_budget_reserved on a base that predated #6003's hand-written Deserialize, so the visitor's struct literal never learned about the field. Each PR was green on its own base; the breakage only exists in their merge. The field joins the other three lists the hand-written impl maintains (Field enum, match arm, struct literal, FIELDS) and defaults to 0 when absent — the value a record written before wrap accounting, or rewritten by an older build, carries; zero restarts the reservation rather than blocking a wrap. vault_key_data_deserializer_covers_every_serialized_field turns this class of mistake into a test failure instead of a merge-order accident: it serializes a fully populated record and asserts the deserializer recognizes every emitted key (unknown-field counter stays zero) and reads every value back. Mutation-verified by dropping the new match arm. * feat(kms): report a key as due for rotation once its wrap budget is spent The rotation readiness verdict only knew about age; the wrap accounting landed by #6019 counted wraps and published an aggregate gauge but never fed the per-key verdict, leaving the criterion backlog#1636 asks for unimplemented. RUSTFS_KMS_ROTATION_MAX_WRAPS adds the second, independent threshold, parsed with the same discipline as the age one: unset or unparsable leaves the verdict unreported rather than inventing a policy, and values below one million are raised to it because wraps are reserved in blocks of that size and a smaller threshold would trip on the first reservation regardless of how many wraps happened. The wrap check runs before the age check so that a key crossing both reports 'wraps': the AES-GCM random-nonce ceiling is a cryptographic bound an operator cannot negotiate, while the age period is a policy they chose. Backends that report no count — Transit and AWS wrap externally, and pre-accounting records carry nothing — leave the wrap half silent instead of guessing, and a backend that cannot rotate is still never told to. Refs rustfs/backlog#1636 (PR-3 acceptance criterion), rustfs/backlog#1562. --------- Co-authored-by: houseme --- crates/kms/src/config.rs | 1 + crates/kms/src/manager.rs | 142 ++++++++++++++++++- crates/kms/src/types.rs | 6 + docs/operations/kms-backend-security.md | 2 + docs/operations/kms-observability-runbook.md | 2 +- 5 files changed, 146 insertions(+), 7 deletions(-) diff --git a/crates/kms/src/config.rs b/crates/kms/src/config.rs index d80cdeb78..c33d1ae0a 100644 --- a/crates/kms/src/config.rs +++ b/crates/kms/src/config.rs @@ -41,6 +41,7 @@ pub const ENV_KMS_AWS_ENDPOINT_URL: &str = "RUSTFS_KMS_AWS_ENDPOINT_URL"; /// unset leaves rotation readiness unreported. Read once when the manager is /// built, by [`crate::manager::KmsManager`]. pub const ENV_KMS_ROTATION_MAX_AGE_SECS: &str = "RUSTFS_KMS_ROTATION_MAX_AGE_SECS"; +pub const ENV_KMS_ROTATION_MAX_WRAPS: &str = "RUSTFS_KMS_ROTATION_MAX_WRAPS"; pub const DEFAULT_VAULT_TRANSIT_METADATA_KV_MOUNT: &str = "secret"; pub const DEFAULT_VAULT_TRANSIT_METADATA_KEY_PREFIX: &str = "rustfs/kms/transit-metadata"; pub const DEFAULT_VAULT_APPROLE_MOUNT: &str = "approle"; diff --git a/crates/kms/src/manager.rs b/crates/kms/src/manager.rs index c9ba1d6e3..f2cc936c5 100644 --- a/crates/kms/src/manager.rs +++ b/crates/kms/src/manager.rs @@ -17,7 +17,7 @@ use crate::audit::{KmsAuditOperation, KmsAuditRecord, KmsAuditSink}; use crate::backends::KmsBackend; use crate::cache::{KmsCache, KmsCacheStats}; -use crate::config::{ENV_KMS_ALLOW_IMMEDIATE_DELETION, ENV_KMS_ROTATION_MAX_AGE_SECS, KmsConfig}; +use crate::config::{ENV_KMS_ALLOW_IMMEDIATE_DELETION, ENV_KMS_ROTATION_MAX_AGE_SECS, ENV_KMS_ROTATION_MAX_WRAPS, KmsConfig}; use crate::deletion_worker::DeletionReferenceChecker; use crate::error::{KmsError, Result}; use crate::types::{ @@ -42,6 +42,13 @@ use tracing::warn; /// after it was rotated, which trains operators to ignore the signal. const MIN_ROTATION_MAX_AGE: Duration = Duration::from_secs(3600); +/// Smallest wrap budget that can be configured. +/// +/// Wraps are accounted in reserved blocks, so any threshold below one block +/// would be crossed by a single reservation and report a key that has barely +/// wrapped anything as overdue. +const MIN_ROTATION_MAX_WRAPS: u64 = 1_000_000; + /// Rotation age from the environment, or `None` when the signal is off. /// /// Unset leaves it off rather than guessing a policy: how often a deployment @@ -68,6 +75,33 @@ fn parse_rotation_max_age(value: Option<&str>) -> Option { Some(Duration::from_secs(seconds).max(MIN_ROTATION_MAX_AGE)) } +/// Wrap budget from the environment, or `None` when the signal is off. +/// +/// Same discipline as the age threshold: unset means unreported rather than a +/// guessed policy, and an unparsable value is refused loudly instead of +/// falling back to a number the operator did not write. Clamped to +/// [`MIN_ROTATION_MAX_WRAPS`] because the backend accounts for wraps in +/// reserved blocks, so a threshold below one block would trip on the first +/// reservation regardless of how many wraps actually happened. +fn configured_rotation_max_wraps() -> Option { + parse_rotation_max_wraps(std::env::var(ENV_KMS_ROTATION_MAX_WRAPS).ok().as_deref()) +} + +fn parse_rotation_max_wraps(value: Option<&str>) -> Option { + let value = value?; + let Ok(wraps) = value.trim().parse::() else { + warn!( + variable = ENV_KMS_ROTATION_MAX_WRAPS, + "ignoring unparsable KMS rotation wrap budget; rotation readiness stays unreported" + ); + return None; + }; + if wraps == 0 { + return None; + } + Some(wraps.max(MIN_ROTATION_MAX_WRAPS)) +} + #[derive(Clone)] pub struct KmsManager { backend: Arc, @@ -82,6 +116,7 @@ pub struct KmsManager { /// the verdict unreported. Read once at construction so a listing cannot /// change its answer halfway through. rotation_max_age: Option, + rotation_max_wraps: Option, } impl KmsManager { @@ -103,6 +138,7 @@ impl KmsManager { allow_immediate_deletion: config.allow_immediate_deletion, reference_checker: None, rotation_max_age: configured_rotation_max_age(), + rotation_max_wraps: configured_rotation_max_wraps(), } } @@ -314,9 +350,22 @@ impl KmsManager { key.rotation_due_reason = Some(RotationDueReason::Unsupported); return; } + key.rotation_due = false; + key.rotation_due_reason = None; + + // The wrap budget is checked first: it is the cryptographic bound (the + // AES-GCM random-nonce ceiling), whereas the age threshold is a policy + // choice, so when both are crossed the reason an operator most needs to + // see is the one they cannot negotiate. + if let (Some(max_wraps), Some(wraps)) = (self.rotation_max_wraps, key.wrap_budget_reserved) + && wraps >= max_wraps + { + key.rotation_due = true; + key.rotation_due_reason = Some(RotationDueReason::Wraps); + return; + } + let Some(max_age) = self.rotation_max_age else { - key.rotation_due = false; - key.rotation_due_reason = None; return; }; @@ -333,9 +382,6 @@ impl KmsManager { if age >= max_age { key.rotation_due = true; key.rotation_due_reason = Some(reason); - } else { - key.rotation_due = false; - key.rotation_due_reason = None; } } @@ -1685,10 +1731,15 @@ mod tests { } fn readiness_manager(rotation_max_age: Option) -> KmsManager { + readiness_manager_with(rotation_max_age, None) + } + + fn readiness_manager_with(rotation_max_age: Option, rotation_max_wraps: Option) -> KmsManager { let temp_dir = tempfile::tempdir().expect("temp dir"); let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults(); let mut manager = KmsManager::new(Arc::new(ScriptedBackend::succeeding()), config); manager.rotation_max_age = rotation_max_age; + manager.rotation_max_wraps = rotation_max_wraps; manager } @@ -1765,6 +1816,85 @@ mod tests { assert!(!key.rotation_due, "clock skew must not manufacture an overdue key"); } + /// The wrap-budget half of the verdict: the cryptographic bound, checked + /// independently of the age policy and reported under its own reason. + #[test] + fn rotation_readiness_reports_an_exhausted_wrap_budget() { + let now = Zoned::now(); + let recently = &now - jiff::Span::new().hours(1); + let long_ago = &now - jiff::Span::new().days(400); + let day = Duration::from_secs(86_400); + let budget = 2_000_000; + + let with_wraps = |manager: &KmsManager, wraps: Option, rotated_at: Option| { + let mut key = aged_key(rotated_at, recently.clone()); + key.wrap_budget_reserved = wraps; + manager.apply_rotation_readiness(&mut key, true, &now); + (key.rotation_due, key.rotation_due_reason) + }; + + // Budget configured and exceeded on a freshly rotated key: due, and the + // reason names the wrap budget rather than an age nobody crossed. + let manager = readiness_manager_with(Some(day), Some(budget)); + assert_eq!( + with_wraps(&manager, Some(budget), Some(recently.clone())), + (true, Some(RotationDueReason::Wraps)) + ); + // At the threshold exactly, not only past it: the bound is a ceiling. + assert_eq!( + with_wraps(&manager, Some(budget + 1), Some(recently.clone())), + (true, Some(RotationDueReason::Wraps)) + ); + // Under the threshold: no verdict from the wrap half. + assert_eq!(with_wraps(&manager, Some(budget - 1), Some(recently.clone())), (false, None)); + + // The cryptographic bound outranks the policy one when both are crossed. + let mut key = aged_key(Some(long_ago.clone()), long_ago); + key.wrap_budget_reserved = Some(budget); + manager.apply_rotation_readiness(&mut key, true, &now); + assert_eq!(key.rotation_due_reason, Some(RotationDueReason::Wraps)); + + // No wrap threshold configured: an enormous count reports nothing, the + // same way an unset age threshold does. + let age_only = readiness_manager_with(Some(day), None); + assert_eq!(with_wraps(&age_only, Some(u64::MAX), Some(recently.clone())), (false, None)); + + // Backend reports no count (Transit, AWS, or a pre-accounting record): + // the wrap half stays silent instead of guessing, and the age half + // still decides. + let wraps_only = readiness_manager_with(None, Some(budget)); + assert_eq!(with_wraps(&wraps_only, None, Some(recently.clone())), (false, None)); + assert_eq!( + with_wraps(&wraps_only, Some(budget), Some(recently.clone())), + (true, Some(RotationDueReason::Wraps)) + ); + + // A backend that cannot rotate is never told to, whatever it wrapped. + let mut key = aged_key(None, recently); + key.wrap_budget_reserved = Some(u64::MAX); + wraps_only.apply_rotation_readiness(&mut key, false, &now); + assert!(!key.rotation_due); + assert_eq!(key.rotation_due_reason, Some(RotationDueReason::Unsupported)); + } + + /// Threshold parsing matches the age threshold's discipline: unset and + /// unparsable both disable the signal rather than inventing a policy. + #[test] + fn rotation_wrap_threshold_parsing_refuses_to_guess() { + assert_eq!(parse_rotation_max_wraps(None), None); + assert_eq!(parse_rotation_max_wraps(Some("not-a-number")), None); + assert_eq!(parse_rotation_max_wraps(Some("")), None); + assert_eq!(parse_rotation_max_wraps(Some("-1")), None); + assert_eq!(parse_rotation_max_wraps(Some("0")), None); + // Clamped: below one reservation block the first reservation would trip it. + assert_eq!(parse_rotation_max_wraps(Some("1")), Some(MIN_ROTATION_MAX_WRAPS)); + assert_eq!( + parse_rotation_max_wraps(Some(" 5000000 ")), + Some(5_000_000), + "a configured budget above the floor is honored verbatim" + ); + } + /// The two fields are additive on the wire: a payload written before they /// existed still deserializes, and a key with no verdict serializes exactly /// as it did before. diff --git a/crates/kms/src/types.rs b/crates/kms/src/types.rs index ac41d0c98..b87d26549 100644 --- a/crates/kms/src/types.rs +++ b/crates/kms/src/types.rs @@ -217,6 +217,12 @@ pub enum RotationDueReason { /// The key has never been rotated and has existed longer than the /// configured maximum age. NeverRotated, + /// The key has wrapped more data keys than the configured maximum. + /// + /// Counted per key-material version, so a rotation restarts the budget. + /// The count is an over-estimate by construction (see the backend's + /// reservation accounting), so this verdict errs toward rotating early. + Wraps, /// The backend cannot rotate keys at all, so no age makes one due. Unsupported, } diff --git a/docs/operations/kms-backend-security.md b/docs/operations/kms-backend-security.md index 2ab33f02a..4fe4aad07 100644 --- a/docs/operations/kms-backend-security.md +++ b/docs/operations/kms-backend-security.md @@ -112,6 +112,8 @@ RustFS does not rotate keys on a schedule. There is no built-in rotation worker, Set `RUSTFS_KMS_ROTATION_MAX_AGE_SECS` to that period in whole seconds. Unset — the default — leaves the verdict unreported rather than assuming a policy: how often keys must be rotated is a compliance decision, and a built-in default would report keys as overdue against a rule nobody wrote. An unparsable value is treated the same way, with a warning, instead of silently falling back to a number the operator did not choose. Values below one hour are raised to one hour, because a threshold of seconds reports every key as overdue moments after it was rotated and teaches operators to ignore the signal. +A second, independent threshold covers the cryptographic bound rather than the policy one. `RUSTFS_KMS_ROTATION_MAX_WRAPS` is the number of data keys one key's material may wrap before the verdict reports `rotation_due` with reason `wraps`. It follows the same discipline — unset or unparsable leaves the verdict unreported, and values below one million are raised to one million because wraps are accounted in reserved blocks of that size, so a smaller threshold would trip on the first reservation. Only backends where RustFS wraps locally and can rotate report a count (Vault KV2 today); Transit and AWS wrap externally and report none, so the wrap half stays silent there rather than guessing. When both thresholds are crossed the reported reason is `wraps`: the AES-GCM random-nonce ceiling is not negotiable, while the age period is a policy an operator chose. + `GET /rustfs/admin/v3/kms/keys` then carries two additional fields per key: - `rotation_due` — whether the key has outlived the configured period. diff --git a/docs/operations/kms-observability-runbook.md b/docs/operations/kms-observability-runbook.md index a9c75e88b..4a76fdbd4 100644 --- a/docs/operations/kms-observability-runbook.md +++ b/docs/operations/kms-observability-runbook.md @@ -204,7 +204,7 @@ Meaning: `rustfs_kms_oldest_key_rotation_age_seconds` — seconds since the leas Investigation: -1. Find which keys are due. The gauge deliberately names no key — a per-key label would carry key identifiers into the metric stream — so read the per-key verdict from the listing: `GET /rustfs/admin/v3/kms/keys` carries `rotation_due` and `rotation_due_reason` (`age`, `never_rotated`, or `unsupported`) per key, computed against `RUSTFS_KMS_ROTATION_MAX_AGE_SECS`. The verdict appears only on the listing, not on single-key describe. If `RUSTFS_KMS_ROTATION_MAX_AGE_SECS` is unset, set it to your policy's rotation period so the per-key verdict and this alert agree on what "overdue" means. +1. Find which keys are due. The gauge deliberately names no key — a per-key label would carry key identifiers into the metric stream — so read the per-key verdict from the listing: `GET /rustfs/admin/v3/kms/keys` carries `rotation_due` and `rotation_due_reason` (`age`, `never_rotated`, `wraps`, or `unsupported`) per key, computed against `RUSTFS_KMS_ROTATION_MAX_AGE_SECS` and `RUSTFS_KMS_ROTATION_MAX_WRAPS`. A `wraps` reason means the key's material has wrapped more data keys than the configured budget — the AES-GCM random-nonce ceiling rather than an age policy, so it is not satisfied by relaxing the age threshold. The verdict appears only on the listing, not on single-key describe. If `RUSTFS_KMS_ROTATION_MAX_AGE_SECS` is unset, set it to your policy's rotation period so the per-key verdict and this alert agree on what "overdue" means. 2. If the reason is `unsupported`, the backend cannot rotate at all (Local, Static). There is no key-level response; the decision is a backend migration, and the wrap ceiling above is the reason it cannot be deferred forever. See the [rotation drivers and scheduling matrix](kms-backend-security.md#rotation-drivers-and-scheduling-per-backend). 3. On a backend that can rotate, act per the driver matrix: on **Vault KV2**, check why your external rotation scheduler did not run (or set one up — RustFS deliberately ships none) and satisfy the [pre-rotation checklist](kms-backend-security.md#rotation-drivers-and-scheduling-per-backend) before rotating, above all the [upgrade-ordering hard constraint](kms-backend-security.md#upgrade-before-first-rotation-hard-constraint) — never respond to this alert by rotating in the middle of a rolling upgrade. On **Vault Transit**, check `auto_rotate_period` on the key in Vault. On **AWS KMS**, check the key's automatic rotation status in AWS — and do not schedule rotation through the RustFS endpoint, which maps to quota-limited `RotateKeyOnDemand`. 4. Know the gauge's blind spot on Transit and AWS before chasing a rotation that already happened: only KV2 persists a rotation timestamp, so Transit and AWS keys age from creation permanently and this alert will not clear after a rotation there. Confirm the real cadence at the owning system — the Transit key's version history in Vault, or the key's rotation status in AWS — and treat a confirmed-healthy cadence as a known overstatement of this gauge rather than an overdue key.