From cef2eb6abe9cdef8632201f1801b2fd7455a5e76 Mon Sep 17 00:00:00 2001 From: Eray Date: Fri, 4 Sep 2026 12:04:35 +0300 Subject: [PATCH 1/3] fix: preserve keys on rename collision --- crates/web-bot-auth/src/keyring.rs | 72 +++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/crates/web-bot-auth/src/keyring.rs b/crates/web-bot-auth/src/keyring.rs index 249b271..80b79c7 100644 --- a/crates/web-bot-auth/src/keyring.rs +++ b/crates/web-bot-auth/src/keyring.rs @@ -218,9 +218,17 @@ impl KeyRing { .is_none() } - /// Rename a public key from `old_identifier` to `new_identifier`. Returns `false` if the old - /// key was not present. + /// Rename a public key from `old_identifier` to `new_identifier`. + /// + /// Returns `false` without modifying the keyring if the old key is absent or the new identifier + /// is already assigned to another key. pub fn rename_key(&mut self, old_identifier: String, new_identifier: String) -> bool { + if old_identifier == new_identifier { + return self.ring.contains_key(&old_identifier); + } + if self.ring.contains_key(&new_identifier) { + return false; + } match self.ring.remove(&old_identifier) { Some(value) => self.ring.insert(new_identifier, value).is_none(), None => false, @@ -270,6 +278,66 @@ impl KeyRing { mod tests { use super::*; + #[test] + fn rename_key_does_not_replace_existing_destination() { + let mut keyring = KeyRing::default(); + let source_key = vec![1; 32]; + let destination_key = vec![2; 32]; + assert!(keyring.import_raw("source".to_string(), Algorithm::Ed25519, source_key.clone())); + assert!(keyring.import_raw( + "destination".to_string(), + Algorithm::Ed25519, + destination_key.clone(), + )); + + assert!(!keyring.rename_key("source".to_string(), "destination".to_string())); + assert_eq!( + keyring.get(&"source".to_string()), + Some(&(Algorithm::Ed25519, source_key)) + ); + assert_eq!( + keyring.get(&"destination".to_string()), + Some(&(Algorithm::Ed25519, destination_key)) + ); + } + + #[test] + fn rename_key_to_same_identifier_reports_whether_key_exists() { + let mut keyring = KeyRing::default(); + assert!(!keyring.rename_key("missing".to_string(), "missing".to_string())); + + let public_key = vec![1; 32]; + assert!(keyring.import_raw( + "existing".to_string(), + Algorithm::Ed25519, + public_key.clone(), + )); + + assert!(keyring.rename_key("existing".to_string(), "existing".to_string())); + assert_eq!( + keyring.get(&"existing".to_string()), + Some(&(Algorithm::Ed25519, public_key)) + ); + } + + #[test] + fn rename_key_with_missing_source_does_not_modify_keyring() { + let mut keyring = KeyRing::default(); + let public_key = vec![1; 32]; + assert!(keyring.import_raw( + "existing".to_string(), + Algorithm::Ed25519, + public_key.clone(), + )); + + assert!(!keyring.rename_key("missing".to_string(), "new".to_string())); + assert_eq!( + keyring.get(&"existing".to_string()), + Some(&(Algorithm::Ed25519, public_key)) + ); + assert!(keyring.get(&"new".to_string()).is_none()); + } + #[test] fn test_importing_ed25519_key_from_jwks() { let mut keyring = KeyRing::default(); From 73146316487d11a7840b750393a2488a5da7ffd2 Mon Sep 17 00:00:00 2001 From: Eray Date: Fri, 4 Sep 2026 18:52:06 +0300 Subject: [PATCH 2/3] fix: add fallible key rename API --- crates/web-bot-auth/src/keyring.rs | 84 +++++++++++++++---- crates/web-bot-auth/src/message_signatures.rs | 6 +- 2 files changed, 72 insertions(+), 18 deletions(-) diff --git a/crates/web-bot-auth/src/keyring.rs b/crates/web-bot-auth/src/keyring.rs index 80b79c7..ae1062e 100644 --- a/crates/web-bot-auth/src/keyring.rs +++ b/crates/web-bot-auth/src/keyring.rs @@ -21,6 +21,15 @@ pub enum KeyringError { KeyAlreadyExists, } +/// Errors that may occur when modifying a keyring. +#[derive(Debug)] +pub enum OperationError { + /// The old key is not present. + KeyNotPresent, + /// The new key identifier is already occupied. + KeyOccupied, +} + /// Represents a public key to be consumed during the verification. pub type PublicKey = Vec; @@ -220,15 +229,40 @@ impl KeyRing { /// Rename a public key from `old_identifier` to `new_identifier`. /// - /// Returns `false` without modifying the keyring if the old key is absent or the new identifier - /// is already assigned to another key. - pub fn rename_key(&mut self, old_identifier: String, new_identifier: String) -> bool { + /// # Errors + /// + /// Returns [`OperationError::KeyNotPresent`] if the old key is absent, or + /// [`OperationError::KeyOccupied`] if the new identifier belongs to another key. + pub fn try_rename_key( + &mut self, + old_identifier: String, + new_identifier: String, + ) -> Result<(), OperationError> { + if !self.ring.contains_key(&old_identifier) { + return Err(OperationError::KeyNotPresent); + } if old_identifier == new_identifier { - return self.ring.contains_key(&old_identifier); + return Ok(()); } if self.ring.contains_key(&new_identifier) { - return false; + return Err(OperationError::KeyOccupied); } + + let value = self + .ring + .remove(&old_identifier) + .ok_or(OperationError::KeyNotPresent)?; + let replaced = self.ring.insert(new_identifier, value); + debug_assert!(replaced.is_none()); + Ok(()) + } + + /// Rename a public key from `old_identifier` to `new_identifier`. + /// + /// This method does not safely handle destination conflicts. Use [`Self::try_rename_key`] + /// instead. + #[deprecated(note = "does not safely handle destination conflicts; use `try_rename_key`")] + pub fn rename_key(&mut self, old_identifier: String, new_identifier: String) -> bool { match self.ring.remove(&old_identifier) { Some(value) => self.ring.insert(new_identifier, value).is_none(), None => false, @@ -279,7 +313,7 @@ mod tests { use super::*; #[test] - fn rename_key_does_not_replace_existing_destination() { + fn try_rename_key_does_not_replace_existing_destination() { let mut keyring = KeyRing::default(); let source_key = vec![1; 32]; let destination_key = vec![2; 32]; @@ -290,7 +324,10 @@ mod tests { destination_key.clone(), )); - assert!(!keyring.rename_key("source".to_string(), "destination".to_string())); + assert!(matches!( + keyring.try_rename_key("source".to_string(), "destination".to_string()), + Err(OperationError::KeyOccupied) + )); assert_eq!( keyring.get(&"source".to_string()), Some(&(Algorithm::Ed25519, source_key)) @@ -302,9 +339,12 @@ mod tests { } #[test] - fn rename_key_to_same_identifier_reports_whether_key_exists() { + fn try_rename_key_to_same_identifier_reports_whether_key_exists() { let mut keyring = KeyRing::default(); - assert!(!keyring.rename_key("missing".to_string(), "missing".to_string())); + assert!(matches!( + keyring.try_rename_key("missing".to_string(), "missing".to_string()), + Err(OperationError::KeyNotPresent) + )); let public_key = vec![1; 32]; assert!(keyring.import_raw( @@ -313,7 +353,11 @@ mod tests { public_key.clone(), )); - assert!(keyring.rename_key("existing".to_string(), "existing".to_string())); + assert!( + keyring + .try_rename_key("existing".to_string(), "existing".to_string()) + .is_ok() + ); assert_eq!( keyring.get(&"existing".to_string()), Some(&(Algorithm::Ed25519, public_key)) @@ -321,7 +365,7 @@ mod tests { } #[test] - fn rename_key_with_missing_source_does_not_modify_keyring() { + fn try_rename_key_with_missing_source_takes_precedence() { let mut keyring = KeyRing::default(); let public_key = vec![1; 32]; assert!(keyring.import_raw( @@ -330,12 +374,14 @@ mod tests { public_key.clone(), )); - assert!(!keyring.rename_key("missing".to_string(), "new".to_string())); + assert!(matches!( + keyring.try_rename_key("missing".to_string(), "existing".to_string()), + Err(OperationError::KeyNotPresent) + )); assert_eq!( keyring.get(&"existing".to_string()), Some(&(Algorithm::Ed25519, public_key)) ); - assert!(keyring.get(&"new".to_string()).is_none()); } #[test] @@ -351,10 +397,14 @@ mod tests { .get(&String::from("poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U")) .is_some() ); - assert!(keyring.rename_key( - String::from("poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U"), - String::from("test-key-ed25519") - )); + assert!( + keyring + .try_rename_key( + String::from("poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U"), + String::from("test-key-ed25519") + ) + .is_ok() + ); assert!(keyring.get(&String::from("test-key-ed25519")).is_some()); } } diff --git a/crates/web-bot-auth/src/message_signatures.rs b/crates/web-bot-auth/src/message_signatures.rs index 67525bd..7a67a94 100644 --- a/crates/web-bot-auth/src/message_signatures.rs +++ b/crates/web-bot-auth/src/message_signatures.rs @@ -785,7 +785,11 @@ mod tests { #[test] fn test_verifying_renamed_key() { let mut keyring = keyring_with_test_key(); - assert!(keyring.rename_key(TEST_KEY_ID.to_string(), "renamed".to_string())); + assert!( + keyring + .try_rename_key(TEST_KEY_ID.to_string(), "renamed".to_string()) + .is_ok() + ); // The old identifier no longer resolves. let verifier = MessageVerifier::parse(&StandardTestVector {}, |(_, _)| true).unwrap(); let err = verifier.verify(&keyring, None).unwrap_err(); From fa962b557047b6c50b40c8dc92f09ba067dd103f Mon Sep 17 00:00:00 2001 From: Eray Date: Mon, 7 Sep 2026 17:34:12 +0300 Subject: [PATCH 3/3] fix: address key rename review --- crates/web-bot-auth/src/keyring.rs | 33 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/crates/web-bot-auth/src/keyring.rs b/crates/web-bot-auth/src/keyring.rs index ae1062e..913515e 100644 --- a/crates/web-bot-auth/src/keyring.rs +++ b/crates/web-bot-auth/src/keyring.rs @@ -232,7 +232,7 @@ impl KeyRing { /// # Errors /// /// Returns [`OperationError::KeyNotPresent`] if the old key is absent, or - /// [`OperationError::KeyOccupied`] if the new identifier belongs to another key. + /// [`OperationError::KeyOccupied`] if the new identifier is already present. pub fn try_rename_key( &mut self, old_identifier: String, @@ -241,19 +241,16 @@ impl KeyRing { if !self.ring.contains_key(&old_identifier) { return Err(OperationError::KeyNotPresent); } - if old_identifier == new_identifier { - return Ok(()); - } if self.ring.contains_key(&new_identifier) { return Err(OperationError::KeyOccupied); } + if old_identifier == new_identifier { + return Ok(()); + } - let value = self - .ring - .remove(&old_identifier) - .ok_or(OperationError::KeyNotPresent)?; - let replaced = self.ring.insert(new_identifier, value); - debug_assert!(replaced.is_none()); + if let Some(value) = self.ring.remove(&old_identifier) { + self.ring.insert(new_identifier, value); + } Ok(()) } @@ -261,7 +258,10 @@ impl KeyRing { /// /// This method does not safely handle destination conflicts. Use [`Self::try_rename_key`] /// instead. - #[deprecated(note = "does not safely handle destination conflicts; use `try_rename_key`")] + #[deprecated( + since = "0.7.1", + note = "does not safely handle destination conflicts; use `try_rename_key`" + )] pub fn rename_key(&mut self, old_identifier: String, new_identifier: String) -> bool { match self.ring.remove(&old_identifier) { Some(value) => self.ring.insert(new_identifier, value).is_none(), @@ -339,7 +339,7 @@ mod tests { } #[test] - fn try_rename_key_to_same_identifier_reports_whether_key_exists() { + fn try_rename_key_applies_error_precedence_to_same_identifier() { let mut keyring = KeyRing::default(); assert!(matches!( keyring.try_rename_key("missing".to_string(), "missing".to_string()), @@ -353,11 +353,10 @@ mod tests { public_key.clone(), )); - assert!( - keyring - .try_rename_key("existing".to_string(), "existing".to_string()) - .is_ok() - ); + assert!(matches!( + keyring.try_rename_key("existing".to_string(), "existing".to_string()), + Err(OperationError::KeyOccupied) + )); assert_eq!( keyring.get(&"existing".to_string()), Some(&(Algorithm::Ed25519, public_key))