make peer info creation fallible

CHANGELOG.md

@@ -1,6 +1,9 @@
 **UNRELEASED**
 
-- Add private route example
+- veilid-core:
+  - Add private route example
+  - Add `require_inbound_relay` option in VeilidConfig. Default is false, but if enabled, forces OutboundOnly/InboundRelay mode. Can be used as an extra layer of IP address obscurity for some threat models. (@neequ57)
+  - Fix crash when peer info has missing or unsupported node ids
 
 **Changed in Veilid 0.4.7**
 

veilid-core/src/network_manager/bootstrap/direct_bootstrap/mod.rs

@@ -135,12 +135,19 @@ impl NetworkManager {
                             bsrec.dial_info_details().to_vec(), // Dial info is as specified in the bootstrap list
                         ),
                     ));
-
-                    Some(Arc::new(PeerInfo::new(
+                    let bspi = match PeerInfo::new(
                         RoutingDomain::PublicInternet,
                         bsrec.node_ids().clone(),
                         sni,
-                    )))
+                    ) {
+                        Ok(v) => v,
+                        Err(e) => {
+                            veilid_log!(self error "Bootstrap has invalid peer info: {}", e);
+                            return None;
+                        }
+                    };
+
+                    Some(Arc::new(bspi))
                 }
             })
             .collect();

veilid-core/src/network_manager/bootstrap/txt_bootstrap/mod.rs

@@ -144,11 +144,19 @@ impl NetworkManager {
                         ),
                     ));
 
-                    Some(Arc::new(PeerInfo::new(
+                    let bspi = match PeerInfo::new(
                         RoutingDomain::PublicInternet,
                         bsrec.node_ids().clone(),
                         sni,
-                    )))
+                    ) {
+                        Ok(v) => v,
+                        Err(e) => {
+                            veilid_log!(self error "Bootstrap has invalid peer info: {}", e);
+                            return None;
+                        }
+                    };
+
+                    Some(Arc::new(bspi))
                 }
             })
             .collect();

veilid-core/src/routing_table/bucket_entry.rs

@@ -588,9 +588,13 @@ impl BucketEntryInner {
         };
         // Peer info includes all node ids, even unvalidated ones
         let node_ids = self.node_ids();
-        let opt_pi = opt_current_sni
-            .as_ref()
-            .map(|s| Arc::new(PeerInfo::new(routing_domain, node_ids, *s.clone())));
+        let opt_pi = match opt_current_sni {
+            Some(s) => match PeerInfo::new(routing_domain, node_ids, *s.clone()) {
+                Ok(v) => Some(Arc::new(v)),
+                Err(_) => None,
+            },
+            None => None,
+        };
 
         // Cache the peerinfo
         pi_cache.insert(routing_domain, opt_pi.clone());

veilid-core/src/routing_table/routing_table_inner/mod.rs

@@ -752,6 +752,9 @@ impl RoutingTableInner {
         F: FnOnce(&mut RoutingTableInner, &mut BucketEntryInner),
     {
         let routing_table = self.routing_table();
+        if node_ids.is_empty() {
+            bail!("Can't create node with no node id");
+        }
 
         // Ensure someone isn't trying register this node itself
         if routing_table.matches_own_node_id(node_ids) {
@@ -761,11 +764,14 @@ impl RoutingTableInner {
         // Look up all bucket entries and make sure we only have zero or one
         // If we have more than one, pick the one with the best cryptokind to add node ids to
         let mut best_entry: Option<Arc<BucketEntry>> = None;
+        let mut supported_node_ids = TypedNodeIdGroup::new();
         for node_id in node_ids.iter() {
             // Ignore node ids we don't support
             if !VALID_CRYPTO_KINDS.contains(&node_id.kind) {
                 continue;
             }
+            supported_node_ids.add(*node_id);
+
             // Find the first in crypto sort order
             let bucket_index = routing_table.calculate_bucket_index(node_id);
             let bucket = self.get_bucket(bucket_index);
@@ -798,8 +804,13 @@ impl RoutingTableInner {
             return Ok(nr);
         }
 
+        // Fail out if we can't handle this node
+        if supported_node_ids.is_empty() {
+            bail!("Not registering node with no supported node ids");
+        }
+
         // If no entry exists yet, add the first entry to a bucket, possibly evicting a bucket member
-        let first_node_id = node_ids[0];
+        let first_node_id = supported_node_ids[0];
         let bucket_entry = routing_table.calculate_bucket_index(&first_node_id);
         let bucket = self.get_bucket_mut(bucket_entry);
         let new_entry = bucket.add_new_entry(first_node_id.value);

veilid-core/src/routing_table/routing_table_inner/routing_domains/mod.rs

@@ -419,6 +419,7 @@ impl RoutingDomainDetailCommon {
             routing_table.node_ids(),
             signed_node_info,
         )
+        .expect("our own peerinfo should never fail")
     }
 
     fn clear_cache(&self) {

veilid-core/src/routing_table/tests/test_serialize_routing_table.rs

@@ -1,74 +1,31 @@
 use super::*;
 use crate::{routing_table::*, RegisteredComponents};
 
-pub async fn test_routingtable_buckets_round_trip() {
-    let original_registry = mock_registry::init("a").await;
-    let copy_registry = mock_registry::init("b").await;
-
-    // Wrap to close lifetime of 'inner' which is borrowed here so terminate() can succeed
-    // (it also .write() locks routing table inner)
-    {
-        let original = original_registry.routing_table();
-        let copy = copy_registry.routing_table();
-
-        // Add lots of routes to `original` here to exercise all various types.
-
-        let (serialized_bucket_map, all_entry_bytes) = original.serialized_buckets();
-
-        RoutingTable::populate_routing_table_inner(
-            &mut copy.inner.write(),
-            serialized_bucket_map,
-            all_entry_bytes,
-        )
-        .unwrap();
-
-        let original_inner = &*original.inner.read();
-        let copy_inner = &*copy.inner.read();
-
-        let routing_table_keys: Vec<_> = original_inner.buckets.keys().clone().collect();
-        let copy_keys: Vec<_> = copy_inner.buckets.keys().clone().collect();
-
-        assert_eq!(routing_table_keys.len(), copy_keys.len());
-
-        for crypto in routing_table_keys {
-            // The same keys are present in the original and copy RoutingTables.
-            let original_buckets = original_inner.buckets.get(crypto).unwrap();
-            let copy_buckets = copy_inner.buckets.get(crypto).unwrap();
-
-            // Recurse into RoutingTable.inner.buckets
-            for (left_buckets, right_buckets) in original_buckets.iter().zip(copy_buckets.iter()) {
-                // Recurse into RoutingTable.inner.buckets.entries
-                for ((left_crypto, left_entries), (right_crypto, right_entries)) in
-                    left_buckets.entries().zip(right_buckets.entries())
-                {
-                    assert_eq!(left_crypto, right_crypto);
-
-                    assert_eq!(
-                        format!("{:?}", left_entries),
-                        format!("{:?}", right_entries)
-                    );
-                }
-            }
-        }
-    }
-
-    // Even if these are mocks, we should still practice good hygiene.
-    mock_registry::terminate(original_registry).await;
-    mock_registry::terminate(copy_registry).await;
+fn make_mock_typed_node_id(kind: CryptoKind, idx: u8) -> TypedNodeId {
+    TypedNodeId::new(
+        kind,
+        NodeId::new([
+            idx, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+            0, 0, 0, 0,
+        ]),
+    )
 }
 
-pub fn test_round_trip_peerinfo() {
-    let mut tks = TypedPublicKeyGroup::new();
-    tks.add(TypedPublicKey::new(
-        CRYPTO_KIND_VLD0,
-        PublicKey::new([
-            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-            0, 0, 0,
-        ]),
-    ));
-    let pi: PeerInfo = PeerInfo::new(
+fn make_mock_typed_node_id_group(vld0: bool, unknown: bool) -> TypedNodeIdGroup {
+    let mut tks = TypedNodeIdGroup::new();
+    if vld0 {
+        tks.add(make_mock_typed_node_id(CRYPTO_KIND_VLD0, 0));
+    }
+    if unknown {
+        tks.add(make_mock_typed_node_id(CryptoKind([1, 2, 3, 4]), 0));
+    }
+    tks
+}
+
+fn make_mock_peer_info(node_ids: TypedNodeIdGroup) -> EyreResult<PeerInfo> {
+    PeerInfo::new(
         RoutingDomain::PublicInternet,
-        tks.into(),
+        node_ids,
         SignedNodeInfo::Direct(SignedDirectNodeInfo::new(
             NodeInfo::new(
                 NetworkClass::OutboundOnly,
@@ -82,7 +39,91 @@ pub fn test_round_trip_peerinfo() {
             Timestamp::new(0),
             Vec::new(),
         )),
-    );
+    )
+}
+
+fn add_mock_data(routing_table: &VeilidComponentGuard<'_, RoutingTable>) {
+    let pi =
+        make_mock_peer_info(make_mock_typed_node_id_group(true, false)).expect("should be valid");
+    routing_table
+        .register_node_with_peer_info(Arc::new(pi), true)
+        .expect("should register");
+    let pi2 =
+        make_mock_peer_info(make_mock_typed_node_id_group(true, true)).expect("should be valid");
+    routing_table
+        .register_node_with_peer_info(Arc::new(pi2), true)
+        .expect("should register");
+
+    let _ = make_mock_peer_info(make_mock_typed_node_id_group(false, false))
+        .expect_err("should fail with no node ids");
+
+    let pi3 =
+        make_mock_peer_info(make_mock_typed_node_id_group(false, true)).expect("should be valid");
+
+    let _ = routing_table
+        .register_node_with_peer_info(Arc::new(pi3), true)
+        .expect_err("should fail with only unsupported node ids");
+}
+pub async fn test_routingtable_buckets_round_trip() {
+    let original_registry = mock_registry::init("a").await;
+    let copy_registry = mock_registry::init("b").await;
+
+    // Wrap to close lifetime of 'inner' which is borrowed here so terminate() can succeed
+    // (it also .write() locks routing table inner)
+    {
+        let original = original_registry.routing_table();
+        let copy = copy_registry.routing_table();
+
+        add_mock_data(&original);
+
+        let (serialized_bucket_map, all_entry_bytes) = original.serialized_buckets();
+
+        RoutingTable::populate_routing_table_inner(
+            &mut copy.inner.write(),
+            serialized_bucket_map,
+            all_entry_bytes,
+        )
+        .unwrap();
+
+        let original_inner = &*original.inner.read();
+        let copy_inner = &*copy.inner.read();
+
+        let original_crypto_kinds: Vec<_> = original_inner.buckets.keys().clone().collect();
+        let copy_crypto_kinds: Vec<_> = copy_inner.buckets.keys().clone().collect();
+
+        assert_eq!(original_crypto_kinds.len(), copy_crypto_kinds.len());
+
+        for crypto in original_crypto_kinds {
+            // The same keys are present in the original and copy RoutingTables.
+            let original_buckets = original_inner.buckets.get(crypto).unwrap();
+            let copy_buckets = copy_inner.buckets.get(crypto).unwrap();
+
+            // Recurse into RoutingTable.inner.buckets
+            for (left_bucket, right_bucket) in original_buckets.iter().zip(copy_buckets.iter()) {
+                // Recurse into RoutingTable.inner.buckets.entries
+                for ((left_node_id, left_entry), (right_node_id, right_entry)) in
+                    left_bucket.entries().zip(right_bucket.entries())
+                {
+                    assert_eq!(left_node_id, right_node_id);
+
+                    let s = left_entry.with(original_inner, |_rti, e| serialize_json(e));
+                    let s2 = right_entry.with(copy_inner, |_rti, e| serialize_json(e));
+
+                    assert_eq!(s, s2);
+                }
+            }
+        }
+    }
+
+    // Even if these are mocks, we should still practice good hygiene.
+    mock_registry::terminate(original_registry).await;
+    mock_registry::terminate(copy_registry).await;
+}
+
+pub fn test_round_trip_peerinfo() {
+    let pi =
+        make_mock_peer_info(make_mock_typed_node_id_group(true, true)).expect("should be valid");
+
     let s = serialize_json(&pi);
     let pi2 = deserialize_json(&s).expect("Should deserialize");
     let s2 = serialize_json(&pi2);

veilid-core/src/routing_table/types/peer_info.rs

@@ -34,13 +34,26 @@ impl PeerInfo {
         routing_domain: RoutingDomain,
         node_ids: TypedNodeIdGroup,
         signed_node_info: SignedNodeInfo,
-    ) -> Self {
-        assert!(!node_ids.is_empty() && node_ids.len() <= MAX_CRYPTO_KINDS);
-        Self {
+    ) -> EyreResult<Self> {
+        if node_ids.is_empty() {
+            bail!(
+                "no node ids for peer info ({:?})\n{:#?}",
+                routing_domain,
+                signed_node_info
+            );
+        } else if node_ids.len() > MAX_CRYPTO_KINDS {
+            bail!(
+                "too many node ids for peer info ({:?}): {:?}\n{:#?}",
+                routing_domain,
+                node_ids,
+                signed_node_info
+            );
+        }
+        Ok(Self {
             routing_domain,
             node_ids,
             signed_node_info,
-        }
+        })
     }
 
     pub fn validate(&self, crypto: &Crypto) -> VeilidAPIResult<()> {

veilid-core/src/routing_table/types/signed_node_info.rs

@@ -69,11 +69,14 @@ impl SignedNodeInfo {
     pub fn relay_peer_info(&self, routing_domain: RoutingDomain) -> Option<Arc<PeerInfo>> {
         match self {
             SignedNodeInfo::Direct(_) => None,
-            SignedNodeInfo::Relayed(r) => Some(Arc::new(PeerInfo::new(
-                routing_domain,
-                r.relay_ids().clone(),
-                SignedNodeInfo::Direct(r.relay_info().clone()),
-            ))),
+            SignedNodeInfo::Relayed(r) => Some(Arc::new(
+                PeerInfo::new(
+                    routing_domain,
+                    r.relay_ids().clone(),
+                    SignedNodeInfo::Direct(r.relay_info().clone()),
+                )
+                .unwrap(), // validate() above, should have ensured this succeeds
+            )),
         }
     }
     pub fn has_any_dial_info(&self) -> bool {

veilid-core/src/rpc_processor/coders/peer_info.rs

@@ -47,9 +47,6 @@ pub fn decode_peer_info(
     if node_ids.is_empty() {
         return Err(RPCError::protocol("no verified node ids"));
     }
-    Ok(PeerInfo::new(
-        decode_context.routing_domain,
-        node_ids,
-        signed_node_info,
-    ))
+    PeerInfo::new(decode_context.routing_domain, node_ids, signed_node_info)
+        .map_err(RPCError::map_invalid_format("invalid peer info"))
 }