diff --git a/crates/kms/src/backends/vault.rs b/crates/kms/src/backends/vault.rs index 3800ccd61..5b56b50e4 100644 --- a/crates/kms/src/backends/vault.rs +++ b/crates/kms/src/backends/vault.rs @@ -80,6 +80,19 @@ struct VaultKeyData { /// persistence landed, so it must stay optional for backward compatibility. #[serde(default)] deletion_date: Option, + /// When the key's material last became current through a rotation. + /// + /// Written by [`VaultKmsClient::rotate_key`] as part of the same + /// check-and-set that switches the current version, so it can only be set on + /// a rotation that actually committed. `None` means "no rotation time on + /// record", which covers two cases that are deliberately not distinguished + /// here: a key that was never rotated, and a key rotated by a build that + /// predates this field. Nothing is back-filled — inventing a timestamp for + /// the second case would report a rotation that this node never observed. + /// See [`crate::deletion_worker`] for why collapsing the two is safe for the + /// rotation-age gauge. + #[serde(default)] + rotated_at: Option, /// Encrypted key material (base64 encoded) encrypted_key_material: String, /// Version that pre-versioning envelopes (no `master_key_version`) resolve to. @@ -966,7 +979,7 @@ impl VaultKmsClient { description: existing.description, metadata: existing.metadata, created_at: existing.created_at, - rotated_at: None, + rotated_at: existing.rotated_at, created_by: None, deletion_date: existing.deletion_date, }) @@ -993,6 +1006,7 @@ impl VaultKmsClient { metadata: HashMap::new(), tags: HashMap::new(), deletion_date: None, + rotated_at: None, encrypted_key_material: encrypted_material, baseline_version: None, }; @@ -1039,7 +1053,7 @@ impl VaultKmsClient { metadata: key_data.metadata, tags: key_data.tags, created_at: key_data.created_at, - rotated_at: None, + rotated_at: key_data.rotated_at, created_by: None, }) } @@ -1268,8 +1282,15 @@ impl VaultKmsClient { // Step 3: switch the current pointer. The top-level copy of the material is // the fast path for new encryptions and must always match `version`. + // + // The rotation timestamp rides along on this same write: it marks the + // moment the new material became current, and persisting it here means it + // commits if and only if the rotation does. A rotation that fails after + // freezing the version record leaves the key unrotated and unstamped, so + // the recorded time never runs ahead of the current version. key_data.version = new_version; key_data.encrypted_key_material = new_material; + key_data.rotated_at = Some(Zoned::now()); self.cas_store_key_data(key_id, &key_data, cas).await?; info!(key_id, version = new_version, "Vault KMS master key rotated"); @@ -1283,7 +1304,9 @@ impl VaultKmsClient { description: key_data.description.clone(), metadata: key_data.metadata.clone(), created_at: key_data.created_at.clone(), - rotated_at: Some(Zoned::now()), + // The persisted value, not a fresh `now()`: what the caller is told + // must be what a later describe of the same key reports. + rotated_at: key_data.rotated_at.clone(), created_by: None, deletion_date: key_data.deletion_date.clone(), }) @@ -1737,6 +1760,7 @@ mod tests { metadata: HashMap::new(), tags: HashMap::new(), deletion_date: None, + rotated_at: None, encrypted_key_material: general_purpose::STANDARD.encode([0x42u8; 32]), baseline_version: None, } @@ -2082,6 +2106,7 @@ mod tests { encrypted_key_material: general_purpose::STANDARD.encode([0x42u8; 32]), baseline_version: Some(1), deletion_date: None, + rotated_at: None, }; let mut value = serde_json::to_value(&key_data).expect("serialize key data"); @@ -2445,6 +2470,7 @@ mod tests { metadata: HashMap::new(), tags: HashMap::new(), deletion_date: Some(deadline.clone()), + rotated_at: None, encrypted_key_material: "material".to_string(), baseline_version: None, }; @@ -3450,6 +3476,141 @@ mod tests { ); } + /// The rotation-age gauge ages a key from `rotated_at`, falling back to + /// `created_at`. A rotation that commits without recording its time makes a + /// key rotated many times read exactly like one that was never rotated, so + /// the commit must carry the timestamp and a later describe must report it. + /// Reverting either half turns this test red. + #[tokio::test] + async fn wired_rotate_persists_rotation_time_and_describe_reports_it() { + let created_at = Zoned::now() - Duration::from_secs(365 * 86400); + let mut key_data = healthy_key_data(); + key_data.created_at = created_at.clone(); + key_data.version = 2; + key_data.baseline_version = Some(1); + key_data.description = Some("payload key".to_string()); + key_data.metadata.insert("owner".to_string(), "platform".to_string()); + key_data.tags.insert("env".to_string(), "prod".to_string()); + + let (vault, client) = scripted_client(vec![ + ScriptedResponse::ok(kv2_metadata_read_data(3)), + ScriptedResponse::ok(kv2_read_data(&key_data)), + ScriptedResponse::ok(serde_json::json!({ "keys": ["1", "2"] })), + // Version 3's material record, then the pointer switch. + ScriptedResponse::ok(kv2_write_ack()), + ScriptedResponse::ok(kv2_write_ack()), + ]) + .await; + + let rotated = client.rotate_key("wired-key", None).await.expect("rotate a healthy key"); + let reported = rotated.rotated_at.clone().expect("a committed rotation must report its time"); + + let bodies = vault.request_bodies(); + let committed = parse_write_body(&bodies[4]); + let persisted: VaultKeyData = + serde_json::from_value(committed["data"].clone()).expect("the committed record must deserialize"); + assert_eq!( + persisted.rotated_at.as_ref().map(Zoned::timestamp), + Some(reported.timestamp()), + "the rotation must persist the time it reports: {committed}" + ); + + // The rest of the record rides through the read-modify-write untouched; + // a rotation that dropped any of it would corrupt the key. + assert_eq!(persisted.version, 3, "{committed}"); + assert_eq!(persisted.created_at.timestamp(), created_at.timestamp(), "{committed}"); + assert_eq!(persisted.baseline_version, Some(1), "{committed}"); + assert_eq!(persisted.description.as_deref(), Some("payload key"), "{committed}"); + assert_eq!(persisted.metadata.get("owner").map(String::as_str), Some("platform"), "{committed}"); + assert_eq!(persisted.tags.get("env").map(String::as_str), Some("prod"), "{committed}"); + + // Describing the committed record must report the rotation, not the + // creation a year earlier that the gauge would otherwise fall back to. + let (_vault, client) = scripted_client(vec![ScriptedResponse::ok(kv2_read_data(&persisted))]).await; + let described = client + .describe_key("wired-key", None) + .await + .expect("describe the rotated key"); + assert_eq!( + described.rotated_at.as_ref().map(Zoned::timestamp), + Some(reported.timestamp()), + "describe must report the persisted rotation time" + ); + assert_ne!( + described.rotated_at.as_ref().map(Zoned::timestamp), + Some(created_at.timestamp()), + "a rotated key must not be aged from its creation" + ); + } + + /// A record written before rotation timestamps were persisted carries no + /// rotation time. Reporting one anyway — the current time, the read time — + /// would tell the rotation-age gauge the key was just rotated and silence a + /// genuinely overdue key, so the absence has to travel as `None`. + #[tokio::test] + async fn wired_describe_key_invents_no_rotation_time_for_legacy_records() { + let mut key_data = healthy_key_data(); + key_data.created_at = Zoned::now() - Duration::from_secs(365 * 86400); + key_data.version = 4; + key_data.baseline_version = Some(1); + + let mut record = kv2_read_data(&key_data); + record["data"] + .as_object_mut() + .expect("key record must be a JSON object") + .remove("rotated_at") + .expect("current records must carry the field"); + + let (_vault, client) = scripted_client(vec![ScriptedResponse::ok(record)]).await; + let described = client + .describe_key("wired-key", None) + .await + .expect("a record without the field must still describe"); + assert!( + described.rotated_at.is_none(), + "an unstamped record must not be reported as freshly rotated, got {:?}", + described.rotated_at + ); + assert_eq!( + described.created_at.timestamp(), + key_data.created_at.timestamp(), + "the rest of the legacy record must survive the read" + ); + assert_eq!(described.version, 4); + } + + /// The persisted KV2 record round-trips its rotation time, and records + /// written before the field existed keep deserializing (as None) with the + /// rest of their contents intact. + #[test] + fn vault_key_data_rotated_at_round_trips_and_stays_backward_compatible() { + let rotated_at = Zoned::now(); + let mut key_data = healthy_key_data(); + key_data.rotated_at = Some(rotated_at.clone()); + key_data.baseline_version = Some(1); + key_data.version = 2; + key_data.tags.insert("env".to_string(), "prod".to_string()); + + let mut value = serde_json::to_value(&key_data).expect("serialize"); + let restored: VaultKeyData = serde_json::from_value(value.clone()).expect("round trip"); + assert_eq!( + restored.rotated_at.as_ref().map(Zoned::timestamp), + Some(rotated_at.timestamp()), + "the rotation time must survive the KV2 round trip" + ); + + value + .as_object_mut() + .expect("record must be a JSON object") + .remove("rotated_at") + .expect("current records must carry the field"); + let legacy: VaultKeyData = serde_json::from_value(value).expect("legacy record must deserialize"); + assert!(legacy.rotated_at.is_none()); + assert_eq!(legacy.version, 2); + assert_eq!(legacy.baseline_version, Some(1)); + assert_eq!(legacy.tags.get("env").map(String::as_str), Some("prod")); + } + /// Reading a pre-versioning envelope against a key whose baseline was erased /// resolves to the current version, whose material never wrapped it. The /// unwrap therefore fails (AES-GCM cannot yield plaintext under the wrong diff --git a/crates/kms/src/deletion_worker.rs b/crates/kms/src/deletion_worker.rs index c03a17b6e..eacc76265 100644 --- a/crates/kms/src/deletion_worker.rs +++ b/crates/kms/src/deletion_worker.rs @@ -107,6 +107,17 @@ impl KeyCensus { KeyStatus::PendingDeletion => self.pending_deletion += 1, KeyStatus::Deleted => self.tombstones += 1, KeyStatus::Active | KeyStatus::Disabled => { + // A missing rotation time means either "never rotated" or "the + // build that rotated it did not record when". Both fall back to + // creation, and the two are not worth separate series: for the + // first, creation *is* the correct baseline; for the second it + // is an upper bound on the true age, so the gauge can only + // overstate how overdue a key is, never hide an overdue one. + // The overstatement is self-healing — the next rotation stamps + // the record — and erring loud is the right direction for a + // rotation-readiness alert. Backends never fabricate a + // timestamp to close the gap, so nothing here can turn an + // unstamped key into a freshly rotated one. let rotated_at = key.rotated_at.as_ref().unwrap_or(&key.created_at); self.oldest_rotation_age_seconds = self.oldest_rotation_age_seconds.max(seconds_between(rotated_at, now)); } diff --git a/docs/operations/kms-observability-runbook.md b/docs/operations/kms-observability-runbook.md index f5358c492..30bf9019a 100644 --- a/docs/operations/kms-observability-runbook.md +++ b/docs/operations/kms-observability-runbook.md @@ -48,13 +48,15 @@ Published by the background deletion worker (`crates/kms/src/deletion_worker.rs` | --- | --- | --- | --- | | `rustfs_kms_pending_deletion_keys` | gauge | — | Keys scheduled for deletion whose deadline has not passed | | `rustfs_kms_deletion_tombstone_keys` | gauge | — | Keys left tombstoned by an interrupted removal, still awaiting the sweep | -| `rustfs_kms_oldest_key_rotation_age_seconds` | gauge | — | Seconds since the least recently rotated usable key was rotated, counting from creation for keys never rotated; `0` when there are none | +| `rustfs_kms_oldest_key_rotation_age_seconds` | gauge | — | Seconds since the least recently rotated usable key was rotated, counting from creation for keys with no recorded rotation; `0` when there are none | | `rustfs_kms_deletion_sweep_keys_total` | counter | `outcome` | Keys the sweep acted on, by outcome | `outcome` is `removed`, `blocked` (live configuration — the default key, or a reference reported by the injected checker — still points at the key, so the sweep refuses to remove it), `skipped` (pending but not yet due, or the state changed between inspection and removal), or `failed` (the removal attempt failed and is retried next sweep). Every series is emitted at zero from the first sweep on, so a `rate()` over it is defined immediately. The three gauges are republished only by a sweep that saw the whole key set; a sweep that could not finish listing leaves the previous, complete values standing rather than understating them. Keys already on their way out are excluded from the rotation-age gauge, so it does not stay pinned high by a key that will never be rotated again. +The rotation age comes from whatever the backend reports as the last rotation, and backends only report a rotation they recorded themselves. A key rotated before its backend persisted rotation timestamps therefore ages from creation until its next rotation stamps the record: the gauge overstates that key's age rather than inventing a rotation it cannot vouch for, so an alert on it fires early rather than late. Backends that cannot rotate at all (Local, Static) age every key from creation by construction. + ### Vault credential metrics Published by the Vault credential provider (`crates/kms/src/backends/vault_credentials.rs`), so they exist only on Vault-backed backends. Both are label-less: there is exactly one credential generation to describe, and the Vault address, mount, auth path and token are all off limits as label values.