diff --git a/crates/web-bot-auth/src/keyring.rs b/crates/web-bot-auth/src/keyring.rs index 249b271..913515e 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; @@ -218,8 +227,41 @@ 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`. + /// + /// # Errors + /// + /// Returns [`OperationError::KeyNotPresent`] if the old key is absent, or + /// [`OperationError::KeyOccupied`] if the new identifier is already present. + 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 self.ring.contains_key(&new_identifier) { + return Err(OperationError::KeyOccupied); + } + if old_identifier == new_identifier { + return Ok(()); + } + + if let Some(value) = self.ring.remove(&old_identifier) { + self.ring.insert(new_identifier, value); + } + 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( + 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(), @@ -270,6 +312,77 @@ impl KeyRing { mod tests { use super::*; + #[test] + 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]; + 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!(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)) + ); + assert_eq!( + keyring.get(&"destination".to_string()), + Some(&(Algorithm::Ed25519, destination_key)) + ); + } + + #[test] + 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()), + Err(OperationError::KeyNotPresent) + )); + + let public_key = vec![1; 32]; + assert!(keyring.import_raw( + "existing".to_string(), + Algorithm::Ed25519, + public_key.clone(), + )); + + 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)) + ); + } + + #[test] + fn try_rename_key_with_missing_source_takes_precedence() { + let mut keyring = KeyRing::default(); + let public_key = vec![1; 32]; + assert!(keyring.import_raw( + "existing".to_string(), + Algorithm::Ed25519, + public_key.clone(), + )); + + 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)) + ); + } + #[test] fn test_importing_ed25519_key_from_jwks() { let mut keyring = KeyRing::default(); @@ -283,10 +396,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();