Cache the destination's identity from its session before a PathBroken lookup

The lookup a PathBroken starts now runs whether or not the destination's
identity is cached, but its answer is verified against the cached key. With
the identity evicted, the answer was discarded unverified, the lookup ran to
its timeout, and packets queued for the destination were dropped as
unreachable.

An admitted PathBroken names a destination this node holds a session with,
and the session holds its key: the one this node initiated to, or the one
the responder handshake authenticated. Cache it from there before the lookup
when the identity cache has none.

docs/design/fips-mesh-operation.md

@@ -384,8 +384,8 @@ source.
    whose signals all arrive over one link never demotes this way; its
    verified coordinates last until discovery replaces them or their
    verification ages out after 300 seconds.
-3. Initiate discovery for the destination, whether or not its identity is
-   cached
+3. Initiate discovery for the destination. If its identity is not cached,
+   cache it first from the session's key, so the response can be verified
 4. Reset CP warmup counter
 
 The source also counts, without refusing anything, a PathBroken that

src/node/handlers/session.rs

@@ -2024,6 +2024,7 @@ impl Node {
                         "PathBroken quorum reached; demoted verified coordinates to a hint");
                 }
                 FspAction::InitiateLookup { dest } => {
+                    self.cache_session_identity(&dest);
                     self.maybe_initiate_lookup(&dest).await;
                 }
                 _ => {}
@@ -2071,6 +2072,23 @@ impl Node {
         }
     }
 
+    /// Cache the identity of `dest` from its session entry if the identity
+    /// cache has none.
+    ///
+    /// A lookup's answer is verified against the target's cached key, so a
+    /// lookup for a destination whose identity has been evicted runs to its
+    /// timeout and drops the packets queued for it as unreachable. A session
+    /// already holds the key: the one this node initiated to, or the one the
+    /// responder handshake authenticated.
+    fn cache_session_identity(&mut self, dest: &NodeAddr) {
+        if self.has_cached_identity(dest) {
+            return;
+        }
+        if let Some(pubkey) = self.sessions.get(dest).map(|e| *e.remote_pubkey()) {
+            self.register_identity(*dest, pubkey);
+        }
+    }
+
     /// Count, without acting on, the two ways an admitted PathBroken can
     /// disagree with this node's own view of the path.
     ///

src/node/tests/coord_forgery.rs

@@ -542,6 +542,106 @@ async fn an_admitted_path_broken_starts_a_lookup_without_a_cached_identity() {
     cleanup_nodes(&mut fx.nodes).await;
 }
 
+/// The lookup a PathBroken starts must be answerable when the destination's
+/// identity is not cached. The session already holds the destination's key,
+/// so the lookup's answer can be verified with it: the position is learned,
+/// the lookup does not run to its timeout, and packets queued for the
+/// destination are not dropped as unreachable when it would have.
+#[tokio::test]
+async fn a_path_broken_without_a_cached_identity_starts_a_lookup_this_node_can_verify() {
+    // V - P1 - D, with D a real node V has a session with but whose identity
+    // V has not cached.
+    let mut nodes = run_tree_test(3, &[(0, 1), (1, 2)], false).await;
+    let p1 = *nodes[1].node.node_addr();
+    let dest = *nodes[2].node.node_addr();
+    let dest_pubkey = nodes[2].node.identity().pubkey_full();
+    let real: Vec<NodeAddr> = nodes[2]
+        .node
+        .tree_state()
+        .my_coords()
+        .node_addrs()
+        .copied()
+        .collect();
+    let handshake = crate::noise::HandshakeState::new_xk_initiator(
+        nodes[0].node.identity().keypair(),
+        dest_pubkey,
+    );
+    nodes[0].node.sessions.insert(
+        dest,
+        crate::node::session::SessionEntry::new(
+            dest,
+            dest_pubkey,
+            EndToEndState::Initiating(handshake),
+            1000,
+            true,
+        ),
+    );
+    assert!(
+        !nodes[0].node.has_cached_identity(&dest),
+        "precondition: the destination's identity is not cached"
+    );
+    nodes[0]
+        .node
+        .queue_pending_tun_packet_for_test(dest, vec![0x60; 40]);
+
+    let lookup = &nodes[0].node.metrics().lookup;
+    let (accepted, miss, timed_out) = (
+        lookup.resp_accepted.get(),
+        lookup.resp_identity_miss.get(),
+        lookup.resp_timed_out.get(),
+    );
+
+    let victim = *nodes[0].node.node_addr();
+    let payload = PathBroken::new(dest, p1).encode();
+    let encoded = SessionDatagram::new(p1, victim, payload).encode();
+    let start = wall_ms();
+    nodes[0]
+        .node
+        .handle_session_datagram(&p1, &encoded[1..], false)
+        .await;
+    for _ in 0..10 {
+        tokio::time::sleep(std::time::Duration::from_millis(50)).await;
+        crate::node::tests::spanning_tree::process_available_packets(&mut nodes).await;
+    }
+
+    let lookup = &nodes[0].node.metrics().lookup;
+    assert_eq!(
+        lookup.resp_identity_miss.get(),
+        miss,
+        "the lookup's answer could not be verified for want of an identity"
+    );
+    assert!(
+        lookup.resp_accepted.get() > accepted,
+        "the lookup's answer must be verified and accepted"
+    );
+    let entry = nodes[0].node.coord_cache().get_entry(&dest).unwrap();
+    assert_eq!(
+        entry.coords().node_addrs().copied().collect::<Vec<_>>(),
+        real
+    );
+    assert!(entry.is_verified(wall_ms()));
+
+    // Run the lookup schedule well past its end: an answered lookup has
+    // nothing left to time out, so the queued packet survives.
+    for step in 1..=6 {
+        nodes[0]
+            .node
+            .check_pending_lookups(start + step * 20_000)
+            .await;
+    }
+    assert_eq!(
+        nodes[0].node.metrics().lookup.resp_timed_out.get(),
+        timed_out,
+        "the lookup ran to its timeout"
+    );
+    assert_eq!(
+        nodes[0].node.pending_tun_total_packets(),
+        1,
+        "the packet queued for the destination was dropped"
+    );
+    cleanup_nodes(&mut nodes).await;
+}
+
 /// The healthy path for keeping a verified entry below quorum: a genuine
 /// failure reported once is still recovered, because the lookup the report
 /// starts replaces the stale value with the destination's real position.