fix: Use registration token to prevent double target id use

On storage target registration, the node sends a "node string id" /
alias which is generated locally on the node. This commit uses that as a
registration token to "ensure" that a once registered target id is not
reused with a different target - if the registration token is already
known, the RegisterNodeMsg will fail. This restores old managements
behavior.

Note that the RegisterNodeMsg actually doesn't do anything
on an already known target (and didn't before this commit) besides
logging. It is up to the server nodes to abort on a failing
RegisterNodeMsg.

Author: rustybee42

mgmtd/src/bee_msg/node.rs

@@ -246,7 +246,7 @@ client version < 8.0)"
                         );
                     };
 
-                    db::target::insert_meta(tx, target_id, None)?;
+                    db::target::insert_meta(tx, target_id)?;
                 }
 
                 (node, true)

mgmtd/src/bee_msg/target.rs

@@ -123,17 +123,38 @@ impl HandleWithResponse for RegisterTarget {
         fail_on_pre_shutdown(ctx)?;
 
         let ctx2 = ctx.clone();
+        log::debug!("AAA {}", str::from_utf8(&self.reg_token)?);
 
         let (id, is_new) = ctx
             .db
             .write_tx(move |tx| {
+                let reg_token = str::from_utf8(&self.reg_token)?;
+
                 // Do not do anything if the target already exists
                 if let Some(id) = try_resolve_num_id(
                     tx,
                     EntityType::Target,
                     NodeType::Storage,
                     self.target_id.into(),
                 )? {
+                    let stored_reg_token: Option<String> = tx.query_row(
+                        sql!(
+                            "SELECT reg_token FROM targets
+                            WHERE target_id = ?1 AND node_type = ?2"
+                        ),
+                        rusqlite::params![id.num_id(), NodeType::Storage.sql_variant()],
+                        |row| row.get(0),
+                    )?;
+
+                    if let Some(st) = stored_reg_token
+                        && st != reg_token
+                    {
+                        bail!(
+                            "Storage target with Id {id} has already been registered and its
+registration token ({reg_token}) does not match the stored token ({st})"
+                        );
+                    }
+
                     return Ok((id.num_id().try_into()?, false));
                 }
 
@@ -142,11 +163,7 @@ impl HandleWithResponse for RegisterTarget {
                 }
 
                 Ok((
-                    db::target::insert_storage(
-                        tx,
-                        self.target_id,
-                        Some(format!("target_{}", std::str::from_utf8(&self.alias)?).try_into()?),
-                    )?,
+                    db::target::insert_storage(tx, self.target_id, Some(reg_token))?,
                     true,
                 ))
             })
@@ -155,7 +172,7 @@ impl HandleWithResponse for RegisterTarget {
         if is_new {
             log::info!("Registered new storage target with Id {id}");
         } else {
-            log::debug!("Re-registered existing storage target with Id {id}");
+            log::debug!("Re-registered existing storage target with Id {id}",);
         }
 
         Ok(RegisterTargetResp { id })

mgmtd/src/db/import_v7.rs

@@ -143,13 +143,8 @@ fn meta_nodes(tx: &Transaction, f: &Path) -> Result<(NodeId, bool)> {
                 "{num_id} is not a valid numeric meta node/target id (must be between 1 and 65535)",
             );
         };
-        target::insert(
-            tx,
-            target_id,
-            None,
-            NodeTypeServer::Meta,
-            Some(target_id.into()),
-        )?;
+
+        target::insert_meta(tx, target_id)?;
     }
 
     if root_id == 0 {

mgmtd/src/db/schema/4.sql

@@ -0,0 +1 @@
+ALTER TABLE targets ADD COLUMN reg_token TEXT;

mgmtd/src/db/target.rs

@@ -37,25 +37,17 @@ pub(crate) fn validate_ids(
 ///
 /// BeeGFS doesn't really support meta targets at the moment, so there always must be exactly one
 /// meta target per meta node with their IDs being the same.
-pub(crate) fn insert_meta(
-    tx: &Transaction,
-    target_id: TargetId,
-    alias: Option<Alias>,
-) -> Result<()> {
+pub(crate) fn insert_meta(tx: &Transaction, target_id: TargetId) -> Result<()> {
     let target_id = if target_id == 0 {
         misc::find_new_id(tx, "targets", "target_id", NodeType::Meta, 1..=0xFFFF)?
-    } else if try_resolve_num_id(tx, EntityType::Target, NodeType::Meta, target_id.into())?
-        .is_some()
-    {
-        bail!(TypedError::value_exists("numeric target id", target_id));
     } else {
         target_id
     };
 
     insert(
         tx,
         target_id,
-        alias,
+        None,
         NodeTypeServer::Meta,
         Some(target_id.into()),
     )?;
@@ -78,45 +70,36 @@ pub(crate) fn insert_meta(
 pub(crate) fn insert_storage(
     tx: &Transaction,
     target_id: TargetId,
-    alias: Option<Alias>,
+    reg_token: Option<&str>,
 ) -> Result<TargetId> {
     let target_id = if target_id == 0 {
         misc::find_new_id(tx, "targets", "target_id", NodeType::Storage, 1..=0xFFFF)?
-    } else if try_resolve_num_id(tx, EntityType::Target, NodeType::Storage, target_id.into())?
-        .is_some()
-    {
-        return Ok(target_id);
     } else {
         target_id
     };
 
-    insert(tx, target_id, alias, NodeTypeServer::Storage, None)?;
+    insert(tx, target_id, reg_token, NodeTypeServer::Storage, None)?;
 
     Ok(target_id)
 }
 
-pub(crate) fn insert(
+fn insert(
     tx: &Transaction,
     target_id: TargetId,
-    alias: Option<Alias>,
+    reg_token: Option<&str>,
     node_type: NodeTypeServer,
     // This is optional because storage targets come "unmapped"
     node_id: Option<NodeId>,
 ) -> Result<()> {
     anyhow::ensure!(target_id > 0, "A target id must be > 0");
 
-    let alias = if let Some(alias) = alias {
-        alias
-    } else {
-        format!("target_{}_{}", node_type.user_str(), target_id).try_into()?
-    };
-
+    let alias = format!("target_{}_{target_id}", node_type.user_str()).try_into()?;
     let new_uid = entity::insert(tx, EntityType::Target, &alias)?;
 
     tx.execute(
         sql!(
-            "INSERT INTO targets (target_uid, node_type, target_id, node_id, pool_id)
-            VALUES (?1, ?2, ?3, ?4, ?5)"
+            "INSERT INTO targets (target_uid, node_type, target_id, node_id, pool_id, reg_token)
+            VALUES (?1, ?2, ?3, ?4, ?5, ?6)"
         ),
         params![
             new_uid,
@@ -127,7 +110,8 @@ pub(crate) fn insert(
                 Some(1)
             } else {
                 None
-            }
+            },
+            reg_token
         ],
     )?;
 
@@ -287,11 +271,10 @@ mod test {
     #[test]
     fn set_get_meta() {
         with_test_data(|tx| {
-            super::insert_meta(tx, 1, Some("existing_meta_target".try_into().unwrap()))
-                .unwrap_err();
-            super::insert_meta(tx, 99, Some("new_meta_target".try_into().unwrap())).unwrap();
-            // existing alias
-            super::insert_meta(tx, 99, Some("new_meta_target".try_into().unwrap())).unwrap_err();
+            super::insert_meta(tx, 1).unwrap_err();
+            super::insert_meta(tx, 99).unwrap();
+            // existing id
+            super::insert_meta(tx, 99).unwrap_err();
 
             let targets: i64 = tx
                 .query_row(sql!("SELECT COUNT(*) FROM meta_targets"), [], |row| {
@@ -306,15 +289,11 @@ mod test {
     #[test]
     fn set_get_storage_and_map() {
         with_test_data(|tx| {
-            let new_target_id =
-                super::insert_storage(tx, 0, Some("new_storage_target".try_into().unwrap()))
-                    .unwrap();
-            super::insert_storage(tx, 1000, Some("new_storage_target_2".try_into().unwrap()))
-                .unwrap();
+            let new_target_id = super::insert_storage(tx, 0, Some("new_storage_target")).unwrap();
+            super::insert_storage(tx, 1000, Some("new_storage_target_2")).unwrap();
 
-            // existing alias
-            super::insert_storage(tx, 0, Some("new_storage_target".try_into().unwrap()))
-                .unwrap_err();
+            // existing id
+            super::insert_storage(tx, 1000, Some("new_storage_target")).unwrap_err();
 
             super::update_storage_node_mappings(tx, &[new_target_id, 1000], 1).unwrap();
 

shared/src/bee_msg/target.rs

@@ -94,7 +94,7 @@ impl Deserializable for TargetReachabilityState {
 #[derive(Clone, Debug, Default, PartialEq, Eq, BeeSerde)]
 pub struct RegisterTarget {
     #[bee_serde(as = CStr<0>)]
-    pub alias: Vec<u8>,
+    pub reg_token: Vec<u8>,
     pub target_id: TargetId,
 }