Add collision-safe KeyRing rename API

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<u8>;
 
@@ -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());
     }
 }

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();