diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1b0afb3..3f389450 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,6 +69,8 @@ jobs: run: bash testing/check-image-scoping.sh - name: Check every action is pinned to a commit SHA run: bash testing/check-action-pins.sh + - name: Check every source comment resolves in-repo + run: bash testing/check-comment-refs.sh # Hermetic: synthetic ping functions, no containers, ~45s. Lives beside # the other two so both runners gate on it identically — putting it in # only one would create exactly the drift check-ci-parity.sh exists to @@ -515,8 +517,9 @@ jobs: # NM+dnsmasq, dns-delegate, no-resolver) across five distros, # plus end-to-end scenarios that boot a real fips daemon with a # real TUN and assert `dig @127.0.0.53 AAAA .fips` - # returns AAAA. Pins the production DNS bind path that - # ISSUE-2026-0002 lived in. Single matrix entry runs all 13 + # returns AAAA. Pins the production DNS bind path where a + # loopback-delivered query was once misattributed to the mesh + # interface and dropped. Single matrix entry runs all 13 # scenarios sequentially; ~7-12 min warm, ~12-15 min cold. - suite: dns-resolver type: dns-resolver diff --git a/Cargo.lock b/Cargo.lock index 05ace93c..4c222b5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1127,6 +1127,7 @@ dependencies = [ "tun", "windows-service", "wintun", + "zeroize", ] [[package]] @@ -4215,6 +4216,20 @@ name = "zeroize" version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c50655cbb0fe3fc43170059e702f1ce5e19b84cec58dc87b037a09935c2f328" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] [[package]] name = "zerotrie" diff --git a/Cargo.toml b/Cargo.toml index bf8602b7..cf9a7586 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,7 @@ sha2 = "0.10" hkdf = "0.12" ring = "0.17" libm = "0.2" +zeroize = { version = "1.9", features = ["zeroize_derive"] } rand = "0.10.1" crossbeam-channel = "0.5" thiserror = "2.0" diff --git a/docs/design/fips-session-layer.md b/docs/design/fips-session-layer.md index faa37ee9..93becd62 100644 --- a/docs/design/fips-session-layer.md +++ b/docs/design/fips-session-layer.md @@ -223,7 +223,10 @@ than network addresses. A session survives: FSP uses Noise XK for session encryption, distinct from the Noise IK pattern used at the link layer. The full Noise descriptor is -`Noise_XK_secp256k1_ChaChaPoly_SHA256`. +`Noise_XK_secp256k1_ChaChaPoly_SHA256`, with one deviation recorded in +[the security reference](../reference/security.md): the handshake AEAD +uses an empty associated-data field where standard Noise +`EncryptAndHash` uses the handshake hash `h`. The XK pattern (pre-message: `← s`): diff --git a/docs/reference/security.md b/docs/reference/security.md index 4a207a1b..12c64bdc 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -58,16 +58,34 @@ idempotent). | Curve | secp256k1 | FMP IK, FSP XK, Schnorr signatures | | Diffie-Hellman | ECDH on secp256k1 (x-only normalized) | Noise IK, Noise XK | | AEAD | ChaCha20-Poly1305 | FMP link encryption, FSP session encryption | -| Hash | SHA-256 | NodeAddr derivation, Noise transcript | +| Hash | SHA-256 | NodeAddr derivation, Noise key schedule | | Key derivation | HKDF-SHA256 | Noise key schedule | | Signatures | secp256k1 Schnorr | TreeAnnounce, LookupResponse proof, Nostr adverts | -| Noise pattern (link) | `Noise_IK_secp256k1_ChaChaPoly_SHA256` | FMP link layer (IK with epoch payload) | -| Noise pattern (session) | `Noise_XK_secp256k1_ChaChaPoly_SHA256` | FSP session layer (XK with epoch payload) | +| Noise pattern (link) | `Noise_IK_secp256k1_ChaChaPoly_SHA256`, with the deviation below | FMP link layer (IK with epoch payload) | +| Noise pattern (session) | `Noise_XK_secp256k1_ChaChaPoly_SHA256`, with the deviation below | FSP session layer (XK with epoch payload) | These choices align with the Nostr cryptographic stack (secp256k1 + ChaCha20-Poly1305 + SHA-256) and the NIP-44 encrypted messaging standard. +### Deviation: Empty Associated Data in the Handshake AEAD + +Both Noise patterns above deviate from the standard construction in one +respect. The handshake AEAD uses an empty associated-data field where +standard Noise `EncryptAndHash` uses the handshake hash `h`. + +The choice was deliberate. Using secp256k1 rather than 25519 already put the +construction outside standard Noise, so no standard-Noise peer could be +confused with it, and the transcript hash bought no distinguishing value. + +That argument is about domain separation, and on those grounds it holds. It +does not cover transcript binding, which is the property actually absent. +Domain separation and DH binding survive through the chaining key `ck`, which +`mix_key` chains from `ck = h`, seeded from the protocol name in +`SymmetricState::initialize` (`src/noise/handshake.rs`). The handshake hash +`h` is maintained at every step and is never fed to the AEAD, so it binds +nothing. + ## Rekey Defaults Both link-layer and session-layer Noise sessions rekey under one of diff --git a/packaging/common/fips-dns-setup b/packaging/common/fips-dns-setup index 2a6678b2..6617060f 100755 --- a/packaging/common/fips-dns-setup +++ b/packaging/common/fips-dns-setup @@ -117,8 +117,7 @@ EOF # - The IPV6_PKTINFO ifindex attribution behaviour where Linux reports # a packet to fips0's own IPv6 address as arriving on fips0 (despite # loopback delivery), which would otherwise be silently dropped by -# the daemon's mesh-interface filter — see ISSUE-2026-0002 in the -# project tracker. +# the daemon's mesh-interface filter. # # This backend is the recommended path on every systemd-resolved host # that doesn't have native dns-delegate support (systemd >= 258). diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index 02bfbf0e..ad4ea187 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -299,7 +299,7 @@ async fn main() { } }; - // Install inbound port-forward rules (TASK-2026-0061). + // Install inbound port-forward rules. if let Err(e) = nat_mgr.set_port_forwards(&gw_config.port_forwards) { error!(error = %e, "Failed to install port-forward rules"); let _ = nat_mgr.cleanup(); diff --git a/src/bin/fips.rs b/src/bin/fips.rs index eaada81d..1fb474be 100644 --- a/src/bin/fips.rs +++ b/src/bin/fips.rs @@ -10,6 +10,7 @@ use fips::{Config, Node}; use std::path::PathBuf; use tracing::{debug, error, info}; use tracing_subscriber::{EnvFilter, fmt}; +use zeroize::Zeroize; /// FIPS mesh network daemon #[derive(Parser, Debug)] @@ -130,7 +131,7 @@ async fn run_daemon( fips::node::warn_on_legacy_config_paths(); // Identity provisioning: config nsec > key file > generate ephemeral - let resolved = match resolve_identity(&config, &loaded_paths) { + let mut resolved = match resolve_identity(&config, &loaded_paths) { Ok(r) => r, Err(e) => { error!("Failed to resolve identity: {}", e); @@ -150,7 +151,15 @@ async fn run_daemon( // Create node with resolved identity let mut config = config; - config.node.identity.nsec = Some(resolved.nsec); + // Take the nsec rather than move it: `ResolvedIdentity` clears its copy + // on drop, so it cannot be left partially moved. Clear whatever the field + // already held first — assigning over it drops the old `String` in place, + // which does not run `Drop for IdentityConfig`, so a key that came from + // the config file would be freed uncleared. + if let Some(mut old) = config.node.identity.nsec.take() { + old.zeroize(); + } + config.node.identity.nsec = Some(std::mem::take(&mut resolved.nsec)); debug!("Creating node"); let mut node = match Node::new(config) { Ok(node) => node, diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index a64f564a..15cbe6fe 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -15,6 +15,7 @@ use std::io::{BufRead, BufReader, Write}; use std::net::{Ipv6Addr, SocketAddrV6}; use std::path::{Path, PathBuf}; use std::time::Duration; +use zeroize::Zeroizing; /// FIPS control client #[derive(Parser, Debug)] @@ -413,11 +414,18 @@ fn main() { // Commands that don't require a running daemon if let Commands::Keygen { dir, force, stdout } = &cli.command { let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. The + // nsec is that same key in another encoding, so it is guarded too. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = Zeroizing::new(encode_nsec(&secret_key)); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if *stdout { - println!("{nsec}"); + println!("{}", nsec.as_str()); println!("{npub}"); return; } diff --git a/src/config/gateway.rs b/src/config/gateway.rs index d524176d..b9a96330 100644 --- a/src/config/gateway.rs +++ b/src/config/gateway.rs @@ -76,7 +76,7 @@ pub struct GatewayConfig { #[serde(default)] pub conntrack: ConntrackConfig, - /// Inbound mesh port forwarding rules. See TASK-2026-0061. + /// Inbound mesh port forwarding rules. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub port_forwards: Vec, } diff --git a/src/config/mod.rs b/src/config/mod.rs index 5a3a775b..f136d218 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -32,6 +32,7 @@ use crate::{Identity, IdentityError}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use thiserror::Error; +use zeroize::{Zeroize, Zeroizing}; #[cfg(target_os = "linux")] pub use gateway::{ConntrackConfig, GatewayConfig, GatewayDnsConfig, PortForward, Proto}; @@ -69,8 +70,7 @@ const PUB_FILENAME: &str = "fips.pub"; /// Recognizes IPv4 `127.x.x.x`, IPv6 `::1` (with or without brackets), and /// the literal string `localhost`. Hostnames are conservatively assumed to /// be non-loopback. Used by `Config::validate()` to reject misconfigured -/// loopback UDP binds combined with non-loopback peer addresses (see -/// ISSUE-2026-0005). +/// loopback UDP binds combined with non-loopback peer addresses. fn is_loopback_addr_str(addr: &str) -> bool { // Bracketed IPv6: `[::1]:port` if let Some(rest) = addr.strip_prefix('[') @@ -310,11 +310,16 @@ pub fn default_gateway_path() -> PathBuf { } /// Read a bare bech32 nsec from a key file. +/// +/// The file contents are the private key, and trimming copies it into a +/// second string, so the read buffer is cleared on every exit path rather +/// than dropped as it stands. The returned nsec is the caller's. pub fn read_key_file(path: &Path) -> Result { let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { path: path.to_path_buf(), source: e, })?; + let contents = Zeroizing::new(contents); let nsec = contents.trim().to_string(); if nsec.is_empty() { return Err(ConfigError::EmptyKeyFile { @@ -535,7 +540,9 @@ pub fn resolve_identity( if config.node.identity.persistent { // Persistent mode: load existing key file or generate-and-persist if key_path.exists() { - let nsec = read_key_file(&key_path)?; + // Held in a guard, not a bare `String`: if the parse below fails, + // the `?` returns and a bare local would be freed uncleared. + let nsec = Zeroizing::new(read_key_file(&key_path)?); let identity = Identity::from_secret_str(&nsec)?; warn_unmanaged_key_file(&key_path); if let Err(e) = write_pub_file(&pub_path, &identity.npub()) { @@ -546,7 +553,7 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(key_path), }); } @@ -560,7 +567,8 @@ pub fn resolve_identity( Path::new(SYSTEM_CONFIG_DIR), Path::new(LEGACY_SYSTEM_CONFIG_DIR), ) { - let nsec = read_key_file(&legacy)?; + // Guarded for the same reason as the current-path read above. + let nsec = Zeroizing::new(read_key_file(&legacy)?); let identity = Identity::from_secret_str(&nsec)?; tracing::warn!( legacy = %legacy.display(), @@ -577,14 +585,20 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(legacy), }); } // No key file anywhere — generate and persist let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -622,7 +636,13 @@ pub fn resolve_identity( // Ephemeral mode (default): fresh keypair every start, write key files // for operator visibility let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -664,6 +684,14 @@ pub fn resolve_identity( } /// Result of identity resolution. +/// +/// `nsec` is the node's private key in plaintext. Every local that carries it +/// through [`resolve_identity`] either moves into this struct or is held in a +/// guard that clears it, so this is where the surviving string lives and where +/// clearing it belongs. +/// A caller that wants the value out should take it with [`Option::take`] or +/// `std::mem::take` rather than moving the field, which the `Drop` below +/// forbids. pub struct ResolvedIdentity { /// The nsec string (bech32 or hex) for creating an Identity. pub nsec: String, @@ -671,6 +699,14 @@ pub struct ResolvedIdentity { pub source: IdentitySource, } +impl Drop for ResolvedIdentity { + /// Clear the plaintext private key rather than dropping the allocation + /// with the key still in it. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Where a resolved identity originated. pub enum IdentitySource { /// From explicit nsec in config file. @@ -732,6 +768,20 @@ pub struct IdentityConfig { pub persistent: bool, } +impl Drop for IdentityConfig { + /// Clear the plaintext private key. + /// + /// This field holds the node's private key for the whole process + /// lifetime, which is the longest any secret lives in this crate, so + /// leaving the allocation to be freed with the key still in it is the + /// largest residue the crate can reach. A caller that needs the value out + /// should take it with [`Option::take`]; moving the field is what the + /// `Drop` forbids. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Root configuration structure. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Config { @@ -802,10 +852,16 @@ impl Config { /// Load configuration from a single file. pub fn load_file(path: &Path) -> Result { - let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { - path: path.to_path_buf(), - source: e, - })?; + // The config file is the highest-priority home of a plaintext key: + // `node.identity.nsec` is read straight out of it, so the whole file + // text is treated as secret for as long as it is held. + let contents = + Zeroizing::new( + std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { + path: path.to_path_buf(), + source: e, + })?, + ); let mut config: Config = serde_yaml::from_str(&contents).map_err(|e| ConfigError::ParseYaml { @@ -897,10 +953,19 @@ impl Config { /// Merge another configuration into this one. /// /// Values from `other` override values in `self` when present. - pub fn merge(&mut self, other: Config) { - // Merge node.identity section + pub fn merge(&mut self, mut other: Config) { + // Merge node.identity section. The nsec is taken rather than moved + // out of `other.node.identity`, which clears its private key on drop + // and so cannot be left partially moved. if other.node.identity.nsec.is_some() { - self.node.identity.nsec = other.node.identity.nsec; + // Clear whatever this field already held before overwriting it. + // Assigning over the field drops the old `String` in place, which + // does not run `Drop for IdentityConfig` and would free a + // plaintext key uncleared when two config files both carry one. + if let Some(mut old) = self.node.identity.nsec.take() { + old.zeroize(); + } + self.node.identity.nsec = other.node.identity.nsec.take(); } if other.node.identity.persistent { self.node.identity.persistent = true; @@ -1037,9 +1102,9 @@ impl Config { // Reject loopback UDP bind combined with non-loopback peer addresses. // Linux pins the source IP to a loopback-bound socket, so packets // sent from such a socket to external peers are dropped at the - // routing layer with no clear error in the daemon log. See - // ISSUE-2026-0005. Outbound-only mode is exempt because it - // overrides bind_addr to 0.0.0.0:0 (kernel-picked source). + // routing layer with no clear error in the daemon log. + // Outbound-only mode is exempt because it overrides bind_addr to + // 0.0.0.0:0 (kernel-picked source). for (name, cfg) in self.transports.udp.iter() { if cfg.outbound_only() { continue; diff --git a/src/config/transport.rs b/src/config/transport.rs index 61651d71..dcb00459 100644 --- a/src/config/transport.rs +++ b/src/config/transport.rs @@ -103,7 +103,7 @@ pub struct UdpConfig { /// unfamiliar addresses. The Node-level gate at /// `src/node/handlers/handshake.rs` carves out msg1 from peers /// already established on this transport (so rekey continues to - /// work) — see ISSUE-2026-0004. + /// work). #[serde(default, skip_serializing_if = "Option::is_none")] pub accept_connections: Option, } diff --git a/src/control/mod.rs b/src/control/mod.rs index ddbc384f..0f436f54 100644 --- a/src/control/mod.rs +++ b/src/control/mod.rs @@ -78,8 +78,8 @@ where match serde_json::from_str::(line.trim()) { Ok(request) => { // First try to serve the request entirely off-loop from the - // read handle. In R0 this always returns None (no query is - // cut over yet); R1+ adds the per-command snapshot branches. + // read handle. It returns None for any command with no snapshot + // branch, and those fall through to the rx_loop path below. match snapshot_dispatch(&request, &read_handle) { Some(resp) => resp, None => { diff --git a/src/control/queries.rs b/src/control/queries.rs index 24036739..6040a041 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -2687,8 +2687,8 @@ mod tests { assert_snapshot("show_stats_history_all_peers", &render_response(resp)); } - /// The five Category-D queries cut over to off-loop serving in R3. Served - /// via `snapshot_dispatch`; coverage asserted in + /// The five derived/routing/cache queries served off-loop via + /// `snapshot_dispatch`; coverage asserted in /// `snapshot_dispatch_serves_category_d_queries` below. const OFF_LOOP_CATEGORY_D: &[&str] = &[ "show_tree", @@ -2698,7 +2698,7 @@ mod tests { "show_identity_cache", ]; - /// The six Category-E queries cut over to off-loop serving in R4. Served via + /// The six per-entity table queries served off-loop via /// `snapshot_dispatch`; coverage asserted in /// `snapshot_dispatch_serves_category_e_queries`. const OFF_LOOP_CATEGORY_E: &[&str] = &[ @@ -2710,7 +2710,7 @@ mod tests { "show_mmp", ]; - /// Milestone-completion contract: every pure-read `show_*` query is served + /// Contract: every pure-read `show_*` query is served /// off-loop via `snapshot_dispatch`, and the rx_loop control path carries no /// `show_*` arm at all — only the mutating COMMAND handlers (`connect` / /// `disconnect`) reach it. This test enumerates the full read surface and @@ -2784,8 +2784,8 @@ mod tests { /// the rx_loop source carries no `queries::dispatch` call and no /// `starts_with("show_")` routing branch. Reads the committed source of /// `src/node/dataplane/rx_loop.rs` and asserts both markers are absent. This - /// is the milestone's "remove `show_*` from the data-plane dispatch path" - /// invariant, guarded against regression. + /// guards the "no `show_*` on the data-plane dispatch path" invariant + /// against regression. #[test] fn rx_loop_has_no_show_dispatch() { let src = include_str!("../node/dataplane/rx_loop.rs"); @@ -2850,10 +2850,10 @@ mod tests { } } - /// The R1/R2 scalar-and-series queries are served off-loop via + /// The scalar-and-series queries are served off-loop via /// `snapshot_dispatch`; mutations return `None` and take the rx_loop COMMAND /// path. (`show_stats_peers` / `show_stats_history_all_peers`, formerly - /// asserted on-loop here, were cut over in R5 — see + /// asserted on-loop here, are now served off-loop too — see /// `snapshot_dispatch_serves_every_read_query` for the full read surface.) #[test] fn snapshot_dispatch_serves_scalar_and_series_queries() { @@ -2888,7 +2888,7 @@ mod tests { "show_stats_all_history", Some(json!({ "window": "10s", "granularity": "1s" })), ), - // R3 Category-D cutover. + // Derived/routing/cache queries, served off-loop. ("show_tree", None), ("show_bloom", None), ("show_cache", None), @@ -2910,7 +2910,7 @@ mod tests { } } - /// R5 cutover + byte-identity: after a `record_stats_history()` tick the + /// Byte-identity: after a `record_stats_history()` tick the /// off-loop `show_acl` / `show_stats_peers` / `show_stats_history_all_peers` /// renders each equal their on-loop oracle byte-for-byte, and all three are /// served off-loop via `snapshot_dispatch`. @@ -2996,7 +2996,7 @@ mod tests { assert_eq!(snap.connection_count, node.connection_count()); assert_eq!(snap.estimated_mesh_size, node.estimated_mesh_size()); assert_eq!(snap.effective_ipv6_mtu, node.effective_ipv6_mtu()); - // R5: the ACL status projection matches the node's live ACL status. + // The ACL status projection matches the node's live ACL status. assert_eq!(snap.acl_status, node.peer_acl_status()); // Off-loop render must equal the on-loop render byte-for-byte. @@ -3008,9 +3008,9 @@ mod tests { ); } - /// The five Category-D queries are served off-loop via `snapshot_dispatch` - /// (return `Some` with status ok); everything not cut over stays on the - /// rx_loop path (`None`). + /// The five derived/routing/cache queries are served off-loop via + /// `snapshot_dispatch` (return `Some` with status ok); everything not cut + /// over stays on the rx_loop path (`None`). #[test] fn snapshot_dispatch_serves_category_d_queries() { use super::super::protocol::Request; @@ -3033,7 +3033,7 @@ mod tests { } // Mutations take the rx_loop COMMAND path. (Every read query, including - // the per-peer stats-series queries, is served off-loop as of R5.) + // the per-peer stats-series queries, is served off-loop.) for cmd in ["connect", "disconnect"] { assert!( snapshot_dispatch(&req(cmd), &handle).is_none(), @@ -3043,7 +3043,7 @@ mod tests { } /// The tick-published `RoutingSnapshot` reflects node state, and each - /// off-loop Category-D render equals its on-loop render byte-for-byte + /// off-loop routing render equals its on-loop render byte-for-byte /// (modulo the volatile-key redaction the wire-schema tests already apply). #[test] fn routing_snapshot_matches_on_loop_after_tick() { @@ -3102,10 +3102,11 @@ mod tests { ); } - // ---- R4 Category-E coverage ------------------------------------------ + // ---- per-entity table coverage --------------------------------------- - /// The six Category-E queries are served off-loop via `snapshot_dispatch` - /// (return `Some` with status ok); mutations take the rx_loop COMMAND path. + /// The six per-entity table queries are served off-loop via + /// `snapshot_dispatch` (return `Some` with status ok); mutations take the + /// rx_loop COMMAND path. #[test] fn snapshot_dispatch_serves_category_e_queries() { use super::super::protocol::Request; @@ -3128,7 +3129,7 @@ mod tests { } // Mutations take the rx_loop COMMAND path. (Every read query is served - // off-loop as of R5.) + // off-loop.) for cmd in ["connect", "disconnect"] { assert!( snapshot_dispatch(&req(cmd), &handle).is_none(), @@ -3138,7 +3139,7 @@ mod tests { } /// Freshness + fidelity: after a `record_stats_history()` tick (the entity - /// publisher site) each off-loop Category-E render equals its on-loop render + /// publisher site) each off-loop per-entity render equals its on-loop render /// byte-for-byte, and the seeded snapshot is empty before the first tick. #[test] fn entity_snapshot_matches_on_loop_after_tick() { @@ -3188,7 +3189,7 @@ mod tests { ); } - /// Structural sharing (the R4 umbrella mandate): a republish in which only + /// Structural sharing: a republish in which only /// one row changed re-allocates only that one `Arc` — every unchanged /// row is reused by pointer (`Arc::ptr_eq`). Exercises /// [`reconcile_rows`](super::super::snapshot::reconcile_rows), the diff --git a/src/control/read_handle.rs b/src/control/read_handle.rs index a2f66fa1..30a187bc 100644 --- a/src/control/read_handle.rs +++ b/src/control/read_handle.rs @@ -2,27 +2,33 @@ //! queries can render off the rx_loop hot path instead of round-tripping the //! mpsc → rx_loop oneshot. //! -//! This is the stable seam of the control read-isolation milestone -//! (TASK-2026-0152, phase R0). The handle bundles the state that is already -//! independently shareable, and grows one `ArcSwap` snapshot cell per phase as -//! each subsystem's read state is published from its natural mutator: +//! The handle bundles the node state that is independently shareable, plus one +//! `ArcSwap` snapshot cell per read subsystem: //! -//! - `context` / `metrics` — already `Arc`-shared (refactor steps B/C). -//! - `stats` (R2) — `ArcSwap`: stats_history dual-ring + the -//! scalar gauges `show_status` needs, published from the tick. -//! - `routing` (R3) — `ArcSwap`: tree / bloom / coord / -//! identity, published from their announce / discovery mutators. -//! - `entities` (R4) — `ArcSwap`: peers / sessions / links / -//! connections / transports, published per-entity with `Vec>` +//! - `context` / `metrics` — already `Arc`-shared. +//! - `stats` — `ArcSwap`: stats_history dual-ring + the scalar +//! gauges `show_status` needs, published from the tick. +//! - `routing` — `ArcSwap`: tree / bloom / coord / identity, +//! published from the tick. +//! - `entities` — `ArcSwap`: peers / sessions / links / +//! connections / transports, published from the tick with `Vec>` //! structural sharing. //! -//! Publisher placement follows the Q1 rules in -//! `design/fast-path-refactoring-r0-read-handle.md`: every snapshot is -//! published at its state's natural mutation site (on-change), never by the -//! contended rx_loop task it is meant to bypass. +//! Publisher placement: all three snapshot cells are published from the +//! periodic tick, which runs as one arm of the rx_loop's `select!`. Publishing +//! therefore costs the rx_loop; what the handle removes is the read-side round +//! trip out to the rx_loop and back, not the cost of publishing. The +//! `publish_routing_snapshot` and `publish_entities_snapshot` doc comments on +//! `Node` carry the reasoning for the two projections that need coherent +//! `&Node` access across subsystems. //! -//! R0 ships only the type and the dispatch seam ([`snapshot_dispatch`]); no -//! query reads the handle yet. Cutover begins in R1. +//! A projection is a point-in-time copy, not a live view. The entity tables in +//! particular are mutated on the packet path between ticks, so a reader sees +//! the state as of the last publish. +//! +//! [`snapshot_dispatch`] is the seam: it serves the commands in its match arms +//! directly from the handle and returns `None` for everything else, so the +//! caller falls back to the mpsc → rx_loop path. use std::sync::Arc; @@ -37,9 +43,9 @@ use super::snapshot::{EntitySnapshot, RoutingSnapshot, StatsSnapshot}; /// Cloneable read-only view of node state for off-loop control serving. /// /// All fields are `Arc` / `ArcSwap` handles, so cloning is cheap and a clone -/// can be held by every accepted control connection. Fields are consumed -/// starting R1 as `show_*` queries cut over to off-loop rendering; until then -/// they are wired but unread. +/// can be held by every accepted control connection. The snapshot cells are +/// read by the `*_from_handle` query functions that [`snapshot_dispatch`] +/// routes to. #[derive(Clone)] pub(crate) struct ControlReadHandle { /// Effectively-immutable node context (config, identity, limits). @@ -47,14 +53,14 @@ pub(crate) struct ControlReadHandle { /// Metrics registry (counters / gauges) for `show_stats_*`. metrics: Arc, /// stats_history dual-ring read copy + the scalar gauges/counts - /// `show_status` needs, published from the tick (R2, Q1-b). + /// `show_status` needs, published from the tick. stats: Arc>, - /// Category-D derived/routing/cache read view (tree / bloom / coord / - /// identity + F-queue scalars), published from the tick (R3). + /// Derived/routing/cache read view (tree / bloom / coord / + /// identity + F-queue scalars), published from the tick. routing: Arc>, - /// Category-E per-entity table read view (peers / sessions / links / + /// Per-entity table read view (peers / sessions / links / /// connections / transports + mmp), published from the tick with - /// `Vec>` structural sharing (R4). + /// `Vec>` structural sharing. entities: Arc>, } @@ -90,19 +96,19 @@ impl ControlReadHandle { } /// Load the latest published stats snapshot (the freshest available by - /// construction; no IO_TIMEOUT staleness gate, per Q1-e). + /// construction; no IO_TIMEOUT staleness gate). pub(crate) fn stats(&self) -> arc_swap::Guard> { self.stats.load() } - /// Load the latest published Category-D routing snapshot (freshest - /// available by construction; no staleness gate, per Q1-e). + /// Load the latest published routing snapshot (freshest + /// available by construction; no staleness gate). pub(crate) fn routing(&self) -> arc_swap::Guard> { self.routing.load() } - /// Load the latest published Category-E entity snapshot (freshest available - /// by construction; no staleness gate, per Q1-e). + /// Load the latest published entity snapshot (freshest available + /// by construction; no staleness gate). pub(crate) fn entities(&self) -> arc_swap::Guard> { self.entities.load() } @@ -110,14 +116,16 @@ impl ControlReadHandle { /// Attempt to serve a request entirely from the read handle, off the rx_loop. /// -/// Returns `Some(response)` when the command is a pure-snapshot query that has -/// been cut over to off-loop rendering, or `None` when it must take the -/// mpsc → rx_loop path (parameterized queries, mutations, and any query not -/// yet cut over). +/// Returns `Some(response)` when the command can be rendered from the bundled +/// snapshot cells, or `None` when it must take the mpsc → rx_loop path. /// -/// Cutover queries (R1) read only `NodeContext` / `MetricsRegistry` (the state -/// the read handle already bundles) plus host-OS facts (`/proc`, nftables), so -/// they render entirely in the control task without touching `Node`. +/// The queries served here read any of the cells the handle bundles — +/// `context`, `metrics`, `stats`, `routing`, `entities` — plus host-OS facts +/// gathered in [`super::listening`] and [`super::firewall_state`], so they +/// render in the control task without touching `Node`. Taking a parameter is +/// not what decides it: `show_stats_history` is parameterized and is served +/// here. What falls back is a query needing live `Node` state the snapshot does +/// not carry, and every mutation. /// /// **It now also carries mutating commands**, namely the `profile_tick_*` /// family under the `profiling` feature. They are served here rather than on @@ -158,18 +166,18 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - )), "show_stats_list" => Some(Response::ok(queries::show_stats_list())), "show_metrics" => Some(Response::ok(queries::show_metrics_from_handle(handle))), - // R5: peer-ACL status, served from the tick-published `StatsSnapshot`. + // Peer-ACL status, served from the tick-published `StatsSnapshot`. // The ACL is an `arc_swap::ArcSwap` reloaded only on the tick; // its status projection is captured at the same tick. "show_acl" => Some(Response::ok(queries::show_acl_from_handle(handle))), - // R2: served from the tick-published `StatsSnapshot` (rings + scalar + // Served from the tick-published `StatsSnapshot` (rings + scalar // gauges/counts). `show_status` and the two node-level/per-peer series // queries carry enough data in the snapshot to render faithfully // off-loop, including the parameterized series selectors (the snapshot // holds the full rings, so any metric / window / granularity is // satisfiable). // - // R5 closes out the per-peer stats queries: `show_stats_peers` and + // The per-peer stats queries: `show_stats_peers` and // `show_stats_history_all_peers` now read the snapshot's per-peer // `peer_meta` (live `is_active`, resolved npub / display name, captured // at publish time) joined against the `history` rings, so they no longer @@ -188,7 +196,7 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - handle, request.params.as_ref(), )), - // R3: served from the tick-published `RoutingSnapshot` (tree / bloom / + // Served from the tick-published `RoutingSnapshot` (tree / bloom / // coord cache / identity cache + F-queue scalars). Display names are // resolved at publish time, so these render entirely off-loop. The // counter-family `stats` blocks come from the `MetricsRegistry` (also @@ -200,7 +208,7 @@ pub(crate) fn snapshot_dispatch(request: &Request, handle: &ControlReadHandle) - "show_identity_cache" => Some(Response::ok(queries::show_identity_cache_from_handle( handle, ))), - // R4: served from the tick-published `EntitySnapshot` (per-entity + // Served from the tick-published `EntitySnapshot` (per-entity // `Vec>` tables with structural sharing). Display names, // tree-relationship flags, and Nostr-traversal state are resolved at // publish time, so these render entirely off-loop. All six are diff --git a/src/control/snapshot.rs b/src/control/snapshot.rs index 8f5f4378..042f79b3 100644 --- a/src/control/snapshot.rs +++ b/src/control/snapshot.rs @@ -1,8 +1,7 @@ //! Read-side state snapshots published from the node's natural mutators so //! pure-snapshot `show_*` queries render off the rx_loop hot path. //! -//! [`StatsSnapshot`] is the R2 reference implementation of the canonical -//! snapshot pattern (see `design/fast-path-refactoring-r0-read-handle.md`): a +//! [`StatsSnapshot`] is the reference implementation of the canonical snapshot pattern: a //! read-only data bundle published via `ArcSwap` from the tick after //! `StatsHistory::tick()`. It carries //! @@ -13,10 +12,10 @@ //! peer / session / link / connection / transport counts), plus //! `peer_aliases` (effectively immutable after construction). //! -//! The snapshot holds *data*, not rendered `Response` envelopes (Q1-d): +//! The snapshot holds *data*, not rendered `Response` envelopes: //! rendering happens in the control task off the rx_loop. Staleness is bounded //! by the tick interval and is never staler than the underlying data, which -//! also advances only on the tick (Q1-b). +//! also advances only on the tick. use std::collections::HashMap; use std::sync::Arc; @@ -28,7 +27,7 @@ use crate::node::stats_history::StatsHistory; use crate::upper::tun::TunState; /// Read-only snapshot of the stats-history rings plus the scalar gauges and -/// counts `show_status` reports. Published from the tick (Q1-b). +/// counts `show_status` reports. Published from the tick. #[derive(Clone)] pub(crate) struct StatsSnapshot { /// Cloned read copy of the history rings (the dual-ring read side). @@ -66,14 +65,14 @@ pub(crate) struct StatsSnapshot { pub peer_aliases: Arc>, /// Loaded peer-ACL status (`show_acl`). The ACL itself is an /// `arc_swap::ArcSwap` mutated only by the tick's `reload_peer_acl`; - /// the human-readable status is a cheap projection of it (R5). + /// the human-readable status is a cheap projection of it. pub acl_status: PeerAclStatus, /// Per-stats-history-peer metadata resolved against the live peer/session /// tables and host map at publish time (`show_stats_peers` / /// `show_stats_history_all_peers`), keyed by `NodeAddr`. The lifecycle /// timestamps and per-peer metric rings stay in `history`; this map carries /// only the cross-subsystem fields a renderer can't derive from the rings - /// alone (`is_active`, resolved `npub`, resolved `display_name`) (R5). + /// alone (`is_active`, resolved `npub`, resolved `display_name`). pub peer_meta: Arc>, } @@ -138,34 +137,34 @@ fn empty_acl_status() -> PeerAclStatus { } // ===================================================================== -// RoutingSnapshot (R3 — Category-D derived/routing/cache read view) +// RoutingSnapshot (derived/routing/cache read view) // ===================================================================== -/// Read-only snapshot of the Category-D derived/routing/cache subsystems that +/// Read-only snapshot of the derived/routing/cache subsystems that /// the pure-snapshot `show_tree` / `show_bloom` / `show_cache` / `show_routing` /// / `show_identity_cache` queries render. Published via `ArcSwap`. /// -/// The R0 stub (`design/fast-path-refactoring-r0-read-handle.md`) names a -/// single combined `ArcSwap` for R3. This is that cell: one +/// This is the single combined `ArcSwap` cell for the routing +/// subsystems: one /// cohesive routing view holding the four subsystems (tree / bloom / coord /// cache / identity cache) plus the F-queue summary scalars. /// -/// **Publisher placement (Q1).** The four subsystems mutate at many scattered +/// **Publisher placement.** The four subsystems mutate at many scattered /// handler sites (28 `coord_cache_mut` call sites, 16 `tree_state_mut`, ~32 /// identity-cache touches), and every projected row needs a *display name* -/// resolved against the live peer/session tables and host map — Category-E +/// resolved against the live peer/session tables and host map — per-entity /// state reachable only with `&Node`. Wiring an on-change `publish_*` at each /// mutation site would be large, error-prone surgery, and each call would still /// need `&Node` to resolve names across subsystem boundaries. So this snapshot -/// is published from the **tick** (Q1-b acceptable-at-mutator / the documented -/// interim the spec permits, mirroring R2's stats publish): the tick is the one -/// site with coherent `&Node` access to resolve all display names together. A +/// is published from the **tick**, the same placement the stats snapshot +/// above uses: the tick is the one site with coherent `&Node` access to +/// resolve all display names together. A /// single combined cell is the natural shape because there is exactly one /// publisher — the multi-mutator "rebuild the whole snapshot N times" hazard -/// that Q1-c warns against does not arise. +/// does not arise. /// /// The snapshot holds *data* (typed rows + scalars), not rendered `Response` -/// envelopes (Q1-d); rendering happens off the rx_loop in the control task. The +/// envelopes; rendering happens off the rx_loop in the control task. The /// counter-family `stats` blocks the queries also emit come from the /// `MetricsRegistry` (already `Arc`-shared in the handle) at render time, not /// from this snapshot. @@ -174,9 +173,9 @@ fn empty_acl_status() -> PeerAclStatus { /// the captured absolute timestamps, so the rendered age stays fresh relative /// to the read, exactly as the on-loop queries computed it. /// -/// Forward-compat: when step 5 structurally extracts the Category-D subsystems -/// into typed types, these projections become thin views over them without -/// changing the read-handle interface or this publisher placement. +/// Forward-compat: if the derived/routing/cache subsystems are later +/// extracted into typed types, these projections become thin views over them +/// without changing the read-handle interface or this publisher placement. #[derive(Clone)] pub(crate) struct RoutingSnapshot { /// Spanning-tree read view (`show_tree`). @@ -397,20 +396,18 @@ pub(crate) struct IdentityRow { } // ===================================================================== -// EntitySnapshot (R4 — Category-E per-entity table read views) +// EntitySnapshot (per-entity table read views) // ===================================================================== -/// Read-only snapshot of the Category-E per-entity tables that the +/// Read-only snapshot of the per-entity tables that the /// pure-snapshot `show_peers` / `show_sessions` / `show_links` / /// `show_connections` / `show_transports` / `show_mmp` queries render. /// Published via `ArcSwap`. /// -/// The R0 stub (`design/fast-path-refactoring-r0-read-handle.md`) pre-scopes -/// R4 as `entities — ArcSwap: peers / sessions / links / -/// connections / transports, published per-entity with `Vec>` -/// structural sharing`. This is that cell. +/// This is the `entities` cell: peers / sessions / links / connections / +/// transports, published per-entity with `Vec>` structural sharing. /// -/// **Structural sharing (the umbrella mandate).** Every entity table is a +/// **Structural sharing.** Every entity table is a /// `Vec>`, so a republish in which only one row changed re-allocates /// only that one `Arc` — the unchanged rows are reused by pointer from the /// previous snapshot (`Arc::ptr_eq`-stable). The publisher diffs each freshly @@ -418,10 +415,11 @@ pub(crate) struct IdentityRow { /// keeps the old `Arc` when they are equal. A clone of the snapshot for each /// accepted control connection is then a vector of cheap pointer clones, not a /// deep table copy. This is what keeps the per-tick publish cost off the hot -/// path at scale, as the umbrella requires for R4. +/// path at scale. /// -/// **Publisher placement (Q1).** Like R3, this is published from the **tick**, -/// not per-mutator. Two reasons, both stronger than for R3: +/// **Publisher placement.** Like the routing snapshot, this is published from +/// the **tick**, not per-mutator. Two reasons, both stronger here than for the +/// routing snapshot: /// /// 1. Every projected row needs a *display name* resolved against the live /// peer/session tables and host map (`&Node`), and `show_peers` additionally @@ -432,23 +430,22 @@ pub(crate) struct IdentityRow { /// metrics, `last_seen`, noise counters, replay/decrypt counters) are /// mutated continuously on the **data plane / rx_loop**, not at the discrete /// peer/session/link lifecycle mutators. Per-lifecycle-mutator publication -/// (Q1-a) would therefore not even capture freshness for those fields; the +/// would therefore not even capture freshness for those fields; the /// tick is the natural cadence at which this read view advances. /// -/// The diff-and-reuse therefore satisfies the structural-sharing goal the -/// umbrella mandates (only changed rows re-allocate) while keeping a single -/// coherent `&Node` publisher — the "no monolithic per-tick *re-allocation* of -/// every row" warning is honored because unchanged rows are reused, not rebuilt. -/// This is the documented acceptable interim (the spec's tick-publish-with- -/// Arc-reuse fallback), consistent with R3. +/// The diff-and-reuse therefore satisfies the structural-sharing goal (only +/// changed rows re-allocate) while keeping a single coherent `&Node` +/// publisher: there is no monolithic per-tick *re-allocation* of every row, +/// because unchanged rows are reused rather than rebuilt. This is the same +/// tick publish placement the routing snapshot uses, for the same reason. /// -/// The snapshot holds typed rows (Q1-d data, not rendered `Response` +/// The snapshot holds typed rows (data, not rendered `Response` /// envelopes). Time-relative fields (`idle_ms`) are derived at render time from /// captured absolute timestamps, so the rendered age stays fresh relative to /// the read, exactly as the on-loop queries computed it. /// -/// Forward-compat: step 10 later extracts the session table into a typed -/// `(transport_id, our_index)`-indexed type; these projections then become thin +/// Forward-compat: if the session table is later extracted into a typed +/// `(transport_id, our_index)`-indexed type, these projections become thin /// views over it without changing the read-handle interface or this publisher /// placement. #[derive(Clone)] @@ -736,7 +733,7 @@ pub(crate) struct MmpSessionRow { /// one, preserving structural sharing: an `Arc` from `prev` is reused /// (kept by pointer) whenever a new row matches an old row by identity `key` /// **and** compares equal by value, so only changed/new rows allocate a fresh -/// `Arc`. This is the `Vec>` discipline the R4 umbrella mandates — a +/// `Arc`. This is the `Vec>` structural-sharing discipline — a /// single-row change re-allocates one row, not the whole table, keeping the /// per-tick publish cost off the hot path at scale. /// diff --git a/src/gateway/nat.rs b/src/gateway/nat.rs index b908e3cc..b1859f61 100644 --- a/src/gateway/nat.rs +++ b/src/gateway/nat.rs @@ -66,7 +66,7 @@ pub struct NatManager { lan_interface: String, /// Active mappings keyed by virtual IP. mappings: HashMap, - /// Inbound port-forward rules (TASK-2026-0061). + /// Inbound port-forward rules. port_forwards: Vec, } @@ -235,7 +235,7 @@ impl NatManager { batch.add(&snat_rule, MsgType::Add); } - // Inbound port-forward rules (TASK-2026-0061). Each forward is + // Inbound port-forward rules. Each forward is // one DNAT rule in prerouting keyed on (iif fips0, nfproto ipv6, // l4proto, th dport). When any forwards are configured, emit a // single LAN-side masquerade in postrouting so the LAN target diff --git a/src/identity/encoding.rs b/src/identity/encoding.rs index d81f9aa6..772a635f 100644 --- a/src/identity/encoding.rs +++ b/src/identity/encoding.rs @@ -2,6 +2,7 @@ use bech32::{Bech32, Hrp}; use secp256k1::{SecretKey, XOnlyPublicKey}; +use zeroize::{Zeroize, Zeroizing}; use super::IdentityError; @@ -33,14 +34,25 @@ pub fn decode_npub(npub: &str) -> Result { } /// Encode a secret key as a bech32 nsec string (NIP-19). +/// +/// The returned string is the private key in another encoding, so it is the +/// caller's to clear. What this function clears is the raw byte copy +/// `secret_bytes` hands back, which would otherwise sit in an unnamed +/// temporary until the end of the statement. pub fn encode_nsec(secret_key: &SecretKey) -> String { - bech32::encode::(NSEC_HRP, &secret_key.secret_bytes()) - .expect("nsec encoding cannot fail") + let mut secret_bytes = secret_key.secret_bytes(); + let nsec = + bech32::encode::(NSEC_HRP, &secret_bytes).expect("nsec encoding cannot fail"); + secret_bytes.zeroize(); + nsec } /// Decode an nsec string to a secret key. pub fn decode_nsec(nsec: &str) -> Result { let (hrp, data) = bech32::decode(nsec)?; + // `data` is the raw private key. The guard clears it on every exit path, + // including the two length and prefix rejections below. + let data = Zeroizing::new(data); if hrp != NSEC_HRP { return Err(IdentityError::InvalidNsecPrefix(hrp.to_string())); @@ -59,7 +71,9 @@ pub fn decode_secret(s: &str) -> Result { if s.starts_with("nsec1") { decode_nsec(s) } else { - let bytes = hex::decode(s)?; + // `bytes` is the raw private key; the guard clears it on both the + // length rejection and the normal return. + let bytes = Zeroizing::new(hex::decode(s)?); if bytes.len() != 32 { return Err(IdentityError::InvalidNsecLength(bytes.len())); } diff --git a/src/identity/local.rs b/src/identity/local.rs index 26db8e17..7a494369 100644 --- a/src/identity/local.rs +++ b/src/identity/local.rs @@ -2,6 +2,7 @@ use secp256k1::{Keypair, PublicKey, SecretKey, XOnlyPublicKey}; use std::fmt; +use zeroize::Zeroize; use super::auth::{AuthResponse, auth_challenge_digest}; use super::encoding::{decode_secret, encode_npub}; @@ -11,6 +12,13 @@ use super::{FipsAddress, IdentityError, NodeAddr, sha256}; /// /// The identity holds the secp256k1 keypair and provides methods for signing /// and verifying protocol messages. +/// +/// The keypair is the node's long-term private key. It is erased when the +/// identity is dropped, and every constructor below erases the intermediate +/// secret it built the identity from. All of that clears the copies this +/// crate owns, not every copy that ever existed: `secp256k1` names its erase +/// non-secure because the compiler may duplicate or move the bytes to places +/// no code here can name. #[derive(Clone)] pub struct Identity { keypair: Keypair, @@ -23,39 +31,51 @@ impl Identity { pub fn generate() -> Self { let mut secret_bytes = [0u8; 32]; rand::Rng::fill_bytes(&mut rand::rng(), &mut secret_bytes); - let secret_key = + let mut secret_key = SecretKey::from_slice(&secret_bytes).expect("32 random bytes is a valid secret key"); - Self::from_secret_key(secret_key) + let identity = Self::from_secret_key(secret_key); + secret_bytes.zeroize(); + secret_key.non_secure_erase(); + identity } /// Create an identity from an existing keypair. - pub fn from_keypair(keypair: Keypair) -> Self { + pub fn from_keypair(mut keypair: Keypair) -> Self { let (pubkey, _parity) = keypair.x_only_public_key(); let node_addr = NodeAddr::from_pubkey(&pubkey); let address = FipsAddress::from_node_addr(&node_addr); - Self { + let identity = Self { keypair, node_addr, address, - } + }; + keypair.non_secure_erase(); + identity } /// Create an identity from a secret key. - pub fn from_secret_key(secret_key: SecretKey) -> Self { - let keypair = Keypair::from_secret_key(&super::SECP, &secret_key); - Self::from_keypair(keypair) + pub fn from_secret_key(mut secret_key: SecretKey) -> Self { + let mut keypair = Keypair::from_secret_key(&super::SECP, &secret_key); + let identity = Self::from_keypair(keypair); + keypair.non_secure_erase(); + secret_key.non_secure_erase(); + identity } /// Create an identity from secret key bytes. pub fn from_secret_bytes(bytes: &[u8; 32]) -> Result { - let secret_key = SecretKey::from_slice(bytes)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = SecretKey::from_slice(bytes)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Create an identity from an nsec string (bech32) or hex-encoded secret. pub fn from_secret_str(s: &str) -> Result { - let secret_key = decode_secret(s)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = decode_secret(s)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Return the underlying keypair. @@ -110,6 +130,51 @@ impl Identity { } } +impl Drop for Identity { + /// Erase the long-term private key this identity owns. + /// + /// `Keypair` is `Copy` and so cannot clear itself on drop; `Identity` is + /// not, so it does it for the copy it holds. See the type's own + /// documentation for what that does and does not reach. + fn drop(&mut self) { + self.keypair.non_secure_erase(); + } +} + +/// A `Keypair` copy that is erased when it goes out of scope. +/// +/// `Keypair` is `Copy` and so cannot clear itself on drop. A frame that holds +/// a copy of the node's long-term private key across several exit paths — +/// early error returns, `?`, a normal return — would otherwise need an erase +/// written at each one, and a missed path is invisible. Holding the copy here +/// instead makes the clearing structural. +/// +/// This clears the copy this guard owns, not every copy that ever existed: +/// `secp256k1` names its erase non-secure because the compiler may duplicate +/// or move the bytes to places no code here can name. +pub(crate) struct ErasingKeypair(Keypair); + +impl ErasingKeypair { + /// Take a copy of `source` into the guard and erase `source` in place, so + /// the caller's own binding does not outlive the move. + pub(crate) fn take(source: &mut Keypair) -> Self { + let guarded = Self(*source); + source.non_secure_erase(); + guarded + } + + /// Borrow the guarded keypair. + pub(crate) fn get(&self) -> &Keypair { + &self.0 + } +} + +impl Drop for ErasingKeypair { + fn drop(&mut self) { + self.0.non_secure_erase(); + } +} + impl fmt::Debug for Identity { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Identity") diff --git a/src/identity/mod.rs b/src/identity/mod.rs index 6093427d..b260f8ce 100644 --- a/src/identity/mod.rs +++ b/src/identity/mod.rs @@ -20,6 +20,7 @@ use thiserror::Error; pub use address::FipsAddress; pub use auth::{AuthChallenge, AuthResponse}; pub use encoding::{decode_npub, decode_nsec, decode_secret, encode_npub, encode_nsec}; +pub(crate) use local::ErasingKeypair; pub use local::Identity; pub use node_addr::NodeAddr; pub use peer::PeerIdentity; diff --git a/src/node/dataplane/rx_loop.rs b/src/node/dataplane/rx_loop.rs index 78bf3e8c..086f1860 100644 --- a/src/node/dataplane/rx_loop.rs +++ b/src/node/dataplane/rx_loop.rs @@ -1,9 +1,11 @@ //! RX event loop and packet dispatch. use crate::control::{ControlSocket, commands}; +use crate::node::reject::{RejectReason, TransportReject}; use crate::node::{Node, NodeError}; use crate::proto::fmp::wire::{ COMMON_PREFIX_SIZE, CommonPrefix, FMP_VERSION, PHASE_ESTABLISHED, PHASE_MSG1, PHASE_MSG2, + expected_payload_len, }; use crate::transport::ReceivedPacket; use std::time::Duration; @@ -448,7 +450,11 @@ impl Node { /// Process a single received packet. /// /// Dispatches based on the phase field in the 4-byte common prefix. - async fn process_packet(&mut self, packet: ReceivedPacket) { + /// + /// Visible to the rest of `crate::node` so tests can drive a single + /// packet through the dispatch, the same reach `handle_msg1` and + /// `handle_msg2` already have. + pub(in crate::node) async fn process_packet(&mut self, packet: ReceivedPacket) { if packet.data.len() < COMMON_PREFIX_SIZE { return; // Drop packets too short for common prefix } @@ -499,6 +505,39 @@ impl Node { return; } + // Drop a frame whose declared payload length disagrees with the + // frame that arrived, before that field can be used as a parsing + // input. + // + // Every transport's packets converge here, but the two families + // reach this line differently. TCP, Tor and Nym read their frame + // boundary out of this same field, so for them the comparison holds + // by construction and never fires. UDP, Ethernet and BLE deliver one + // whole frame per packet, where the arrived length is known exactly + // and nothing compares the two today. A short read on those + // transports is a truncated frame, which fails the AEAD tag or the + // exact-size handshake parse already; this changes which reason it + // is dropped for, not whether it is dropped. + // + // A `None` means the phase carries no fixed relationship and the + // frame is left alone rather than rejected, so an unrecognised phase + // still reaches the dispatch below and is handled there. + if let Some(expected) = expected_payload_len(prefix.phase, packet.data.len()) + && prefix.payload_len != expected + { + debug!( + phase = prefix.phase, + declared = prefix.payload_len, + expected, + len = packet.data.len(), + transport_id = %packet.transport_id, + "FMP payload_len disagrees with frame length, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Transport(TransportReject::PayloadLenMismatch)); + return; + } + match prefix.phase { PHASE_ESTABLISHED => { self.handle_encrypted_frame(packet).await; diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index 7dec1d7f..e29dde73 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -93,8 +93,10 @@ use tracing::{debug, trace, warn}; /// inside the worker. That second alloc + ~1.5 KB memcpy per packet at /// line rate cost ~150 MB/sec of memory bandwidth on the hot worker.) pub(crate) struct FmpSendJob { - /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`) - /// — the clone is just a refcount bump on the inner key material. + /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`), but + /// the clone is not a refcount bump: `ring` stores the ChaCha20 key + /// inline as `[u32; 8]`, so cloning copies the key material outright and + /// leaves a second copy that nothing outside `ring` can clear. pub cipher: LessSafeKey, /// Pre-reserved monotonic counter (via `take_send_counter`). pub counter: u64, diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index edd89617..f3cb8f52 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -22,6 +22,28 @@ use crate::utils::index::SessionIndex; use std::time::Duration; use tracing::{debug, info, warn}; +/// Why an inbound msg1 got past the `accept_connections` gate, and against +/// what identity the post-DH confirmation must check it. +/// +/// Three outcomes, not two: an `Option` would conflate "no waiver was needed" +/// with "the waiver was used and nobody owns the matched address", and the +/// second of those is the case that must reject. +#[derive(Debug, PartialEq, Eq)] +pub(in crate::node) enum Msg1Waiver { + /// The transport accepts fresh inbound handshakes (or no transport is + /// registered), so the address carve-out did not admit this msg1 and + /// there is nothing to confirm. + NotNeeded, + /// The carve-out is what admitted this msg1, and the matched address + /// belongs to this identity: either a promoted peer, or a handshake + /// already in flight on the matched link whose identity is expected + /// (outbound dial) or already learned (inbound msg1). + Expect(NodeAddr), + /// The carve-out is what admitted this msg1, and no identity can be + /// attributed to the matched address. Fail closed: reject after the DH. + Unattributed, +} + impl EstablishView for Node { fn establish_snapshot(&self, peer_addr: &NodeAddr) -> EstablishSnapshot { let existing = self.peers.get(peer_addr); @@ -169,6 +191,80 @@ impl Node { false } + /// Classify the msg1 waiver for a source that `should_admit_msg1` + /// admitted, so the post-DH confirmation knows whether it has an + /// identity to check against and what to do when it has none. + /// + /// `established` is the caller's already-computed + /// `is_established_link_msg1(...)`, so the O(peers) scan is not repeated + /// on the refusal path. + /// + /// The two attribution limbs are composed the same way + /// `is_established_link_msg1` composes its own: as an OR, not as an + /// if/else. An `addr_to_link` entry that yields no identity must not + /// short-circuit the address scan, because the two keys can be different + /// forms of the same peer's address (the hostname-vs-numeric case that + /// predicate 2 exists for) and the entry can outlive the link it named. + pub(in crate::node) fn msg1_waiver( + &self, + established: bool, + transport_id: crate::transport::TransportId, + remote_addr: &crate::transport::TransportAddr, + ) -> Msg1Waiver { + // The carve-out only admits anything when the gate would otherwise + // refuse, so an accepting transport has nothing to confirm. + if self + .transports + .get(&transport_id) + .is_none_or(|t| t.accept_connections()) + { + return Msg1Waiver::NotNeeded; + } + if !established { + // `should_admit_msg1` refused this msg1 and the caller returned, + // so this arm is unreachable from the one call site. Fail closed + // rather than skipping the check, so a second caller cannot + // reintroduce the hole this classifier exists to close. + return Msg1Waiver::Unattributed; + } + + // Predicate 1: the reverse-address lookup. + if let Some(&link_id) = self.addr_to_link.get(&(transport_id, remote_addr.clone())) { + if let Some(peer) = self.peers.values().find(|p| p.link_id() == link_id) { + return Msg1Waiver::Expect(*peer.node_addr()); + } + // A link with no promoted peer: a dial in progress or an inbound + // handshake in flight. Both register a connection carrying the + // expected (outbound) or learned (inbound) identity. + if let Some(id) = self + .connections() + .find(|(id, _)| **id == link_id) + .and_then(|(_, machine)| machine.conn_expected_identity()) + { + return Msg1Waiver::Expect(*id.node_addr()); + } + // Deliberately fall through instead of returning. The entry can + // name a link that no longer exists — `remove_link` clears the + // reverse lookup only under the key it rebuilds from the link's + // own remote address, so an entry inserted under a second + // address form for that link survives its removal. Rejecting + // here would refuse a peer predicate 2 can still attribute, and + // would refuse it permanently: this classifier's caller returns + // above the insert that overwrites the stale entry, so nothing + // downstream would ever repair the map. + } + + // Predicate 2: the address scan over promoted peers, which always + // yields an identity when it matches. + self.peers + .values() + .find(|p| { + p.transport_id() == Some(transport_id) && p.current_addr() == Some(remote_addr) + }) + .map(|p| Msg1Waiver::Expect(*p.node_addr())) + .unwrap_or(Msg1Waiver::Unattributed) + } + /// Returns true if an inbound msg1 should be admitted past the /// `accept_connections` gate. /// @@ -250,6 +346,12 @@ impl Node { return; } + // Snapshot which identity, if any, the address carve-out attributed + // this source to. Taken here rather than after the DH so the answer + // is the one the gate acted on. On an accepting transport this is one + // map lookup and a return. + let waiver = self.msg1_waiver(established, packet.transport_id, &packet.remote_addr); + // Parse header let header = match Msg1Header::parse(&packet.data) { Some(h) => h, @@ -334,14 +436,18 @@ impl Node { machine.set_conn_source_addr(packet.remote_addr.clone()); machine.set_leg(HandshakeCrypto::new()); - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let noise_msg1 = &packet.data[header.noise_msg1_offset..]; - let msg2_response = match machine.receive_handshake_init( + let init_result = machine.receive_handshake_init( our_keypair, self.startup_epoch(), noise_msg1, packet.timestamp_ms, - ) { + ); + our_keypair.non_secure_erase(); + let msg2_response = match init_result { Ok(m) => m, Err(e) => { debug!( @@ -368,6 +474,39 @@ impl Node { let peer_node_addr = *peer_identity.node_addr(); + // The address carve-out admitted this msg1 past a refusing gate on + // the strength of the source address alone. Now that the DH has + // revealed the initiator's static, confirm it belongs to the party + // that address is attributed to; an off-path party sourcing from an + // established peer's address gets no further than here. Cheap + // rejection is unchanged: a stranger under accept_connections=false + // is still refused above, having paid nothing. + match waiver { + Msg1Waiver::NotNeeded => {} + Msg1Waiver::Expect(expected) if expected == peer_node_addr => {} + Msg1Waiver::Expect(expected) => { + warn!( + expected = %self.peer_display_name(&expected), + actual = %self.peer_display_name(&peer_node_addr), + transport_id = %packet.transport_id, + "Msg1 admitted by the established-address waiver carries a different identity, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + return; + } + Msg1Waiver::Unattributed => { + warn!( + actual = %self.peer_display_name(&peer_node_addr), + transport_id = %packet.transport_id, + "Msg1 admitted by the established-address waiver, but no identity owns that address, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + return; + } + } + // === PHASE B result === // Bundle the Noise wire-step outputs (identity, remote epoch, sender // index, opaque msg2 payload). The wire step touched no `Node` registry diff --git a/src/node/handlers/mod.rs b/src/node/handlers/mod.rs index 3affc11b..d702e6d9 100644 --- a/src/node/handlers/mod.rs +++ b/src/node/handlers/mod.rs @@ -1,6 +1,6 @@ //! Message handlers: per-message-type behavior on `impl Node`. -mod handshake; +pub(in crate::node) mod handshake; pub(crate) mod lookup; mod mmp; mod rekey; diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index fb5488c9..ac1246ac 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -298,8 +298,11 @@ impl Node { }; // Create IK initiator handshake directly (no PeerConnection) - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut hs = HandshakeState::new_initiator(our_keypair, peer_pubkey); + our_keypair.non_secure_erase(); hs.set_local_epoch(self.startup_epoch()); let noise_msg1 = match hs.write_message_1() { @@ -680,8 +683,11 @@ impl Node { let dest_pubkey = *entry.remote_pubkey(); // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = match handshake.write_xk_message_1() { diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 3b7155ef..cf1662c6 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -645,8 +645,11 @@ impl Node { .record_reject(RejectReason::Session(SessionReject::RekeyPending)); return; } - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; + // the handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -700,8 +703,11 @@ impl Node { } // Create XK responder handshake and process msg1 - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -739,7 +745,11 @@ impl Node { // Store session entry in AwaitingMsg3 state with ack payload for potential resend. // Use a dummy pubkey since we don't know the initiator's identity yet. // We use our own pubkey as placeholder; it will be replaced in handle_session_msg3. - let placeholder_pubkey = self.identity().keypair().public_key(); + // `keypair()` hands back a copy of the long-term private key, so the + // temporary is bound and erased rather than left to the statement end. + let mut our_keypair = self.identity().keypair(); + let placeholder_pubkey = our_keypair.public_key(); + our_keypair.non_secure_erase(); let now_ms = Self::now_ms(); let resend_interval = self.config().node.rate_limit.handshake_resend_interval_ms; let mut entry = SessionEntry::new( @@ -2016,8 +2026,11 @@ impl Node { } // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = handshake .write_xk_message_1() diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 831bef32..8e1765be 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -610,14 +610,17 @@ impl Node { }; // Start the Noise handshake and get message 1 - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let startup_epoch = self.startup_epoch(); - let noise_msg1 = match self + let start_result = self .peer_machines .get_mut(&link_id) .expect("dial-time machine carries the connection") - .start_handshake(our_keypair, startup_epoch, current_time_ms) - { + .start_handshake(our_keypair, startup_epoch, current_time_ms); + our_keypair.non_secure_erase(); + let noise_msg1 = match start_result { Ok(msg) => msg, Err(e) => { // Clean up the index, link, and dial-time machine diff --git a/src/node/reject.rs b/src/node/reject.rs index 382323a4..5dd06b2a 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -298,10 +298,10 @@ pub enum ForwardingReject { /// Transport-layer rejection reasons. /// -/// Currently covers the admission cap-hit path at the TCP and Tor -/// accept loops. Additional transport-side rejection variants -/// (framing errors, connection failures wired through to the node -/// stats path) can be added incrementally. +/// Covers the admission cap-hit path at the TCP and Tor accept loops +/// and the frame-length check at the receive dispatch point. Additional +/// transport-side rejection variants (connection failures wired through +/// to the node stats path) can be added incrementally. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum TransportReject { @@ -310,6 +310,13 @@ pub enum TransportReject { /// (`max_inbound_connections`) was already reached. Tracked via /// [`TransportStats::inbound_cap_exceeded`](crate::node::stats::TransportStats). InboundCapExceeded, + /// Inbound FMP frame dropped because the payload length its header + /// declares does not match the frame the transport delivered. This + /// is a framing rejection, not an admission one: it applies to + /// established data frames as well as to handshake frames, and it + /// is decided before the phase dispatch. Tracked via + /// [`TransportStats::payload_len_mismatch`](crate::node::stats::TransportStats). + PayloadLenMismatch, } #[cfg(test)] @@ -403,4 +410,18 @@ mod tests { RejectReason::Transport(TransportReject::InboundCapExceeded) )); } + + #[test] + fn transport_reject_payload_len_mismatch_round_trips() { + let r = RejectReason::Transport(TransportReject::PayloadLenMismatch); + assert!(matches!( + r, + RejectReason::Transport(TransportReject::PayloadLenMismatch) + )); + assert_ne!( + r, + RejectReason::Transport(TransportReject::InboundCapExceeded), + "a framing drop must not compare equal to an admission rejection" + ); + } } diff --git a/src/node/stats.rs b/src/node/stats.rs index 96503f43..f72639ea 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -202,6 +202,10 @@ impl MmpStats { /// typed-rejection enum stays the canonical entry point and so a /// future transport-to-node bridge (event or sampling) has a /// well-known destination. +/// +/// `payload_len_mismatch` is different in kind: it counts a framing +/// drop the node itself performs at the receive dispatch point, so it +/// has a live writer and can be non-zero on a running node. #[derive(Default)] pub struct TransportStats { /// Reserved for node-side inbound-cap-exceeded admission rejection @@ -209,18 +213,24 @@ pub struct TransportStats { /// on the transport-level stats (`TcpStats::connections_rejected`, /// `TorStats::connections_rejected`) directly. pub inbound_cap_exceeded: u64, + /// Inbound FMP frames dropped because the payload length declared + /// in the common prefix did not match the frame the transport + /// delivered. + pub payload_len_mismatch: u64, } impl TransportStats { pub fn snapshot(&self) -> TransportStatsSnapshot { TransportStatsSnapshot { inbound_cap_exceeded: self.inbound_cap_exceeded, + payload_len_mismatch: self.payload_len_mismatch, } } pub(super) fn record_reject(&mut self, reason: TransportReject) { match reason { TransportReject::InboundCapExceeded => self.inbound_cap_exceeded += 1, + TransportReject::PayloadLenMismatch => self.payload_len_mismatch += 1, } } } @@ -396,6 +406,7 @@ pub struct MmpStatsSnapshot { #[derive(Clone, Debug, Default, Serialize)] pub struct TransportStatsSnapshot { pub inbound_cap_exceeded: u64, + pub payload_len_mismatch: u64, } #[derive(Clone, Debug, Default, Serialize)] @@ -577,4 +588,25 @@ mod tests { stats.record_reject(RejectReason::Transport(TransportReject::InboundCapExceeded)); assert_eq!(stats.transport.inbound_cap_exceeded, 1); } + + /// Records both transport reasons so a swapped or shared arm shows up + /// as a mis-attributed counter rather than as a plausible total. + #[test] + fn transport_stats_record_reject_keeps_the_two_reasons_on_separate_counters() { + let mut s = TransportStats::default(); + s.record_reject(TransportReject::PayloadLenMismatch); + s.record_reject(TransportReject::PayloadLenMismatch); + s.record_reject(TransportReject::InboundCapExceeded); + assert_eq!(s.payload_len_mismatch, 2); + assert_eq!(s.inbound_cap_exceeded, 1); + } + + #[test] + fn node_stats_record_reject_dispatches_payload_len_mismatch_to_transport() { + let mut stats = NodeStats::new(); + stats.record_reject(RejectReason::Transport(TransportReject::PayloadLenMismatch)); + assert_eq!(stats.transport.payload_len_mismatch, 1); + assert_eq!(stats.transport.inbound_cap_exceeded, 0); + assert_eq!(stats.handshake.bad_state, 0); + } } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index cd3de0fc..736d0e98 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -1129,7 +1129,7 @@ async fn test_should_admit_msg1_rejects_fresh_when_accept_off() { assert!(!node.should_admit_msg1(transport_id, &addr)); } -/// ISSUE-2026-0004 regression test: `should_admit_msg1` admits rekey/restart +/// Regression test: `should_admit_msg1` admits rekey/restart /// msg1 from a peer with an existing link even when the transport has /// accept_connections=false. Without this, the dual-init tie-breaker /// deadlocks (the larger-NodeAddr side drops the winner's rekey msg1). @@ -1201,7 +1201,8 @@ async fn test_should_admit_msg1_admits_rekey_when_udp_accept_off() { } /// Regression test for the udp.outbound_only rekey loop observed in -/// production 2026-04-30 (parallel to ISSUE-2026-0004). +/// production 2026-04-30 (parallel to the rekey/restart admission case +/// above). /// /// Production scenario: nomad runs `udp.outbound_only=true` with peer /// core-vm configured by hostname (`core-vm.tail65015.ts.net:2121`). @@ -1282,3 +1283,660 @@ async fn test_should_admit_msg1_admits_rekey_when_addr_form_differs() { assert!(node.is_established_link_msg1(transport_id, &numeric_addr)); assert!(!node.is_established_link_msg1(transport_id, &stranger_addr)); } + +// ============================================================================ +// Frame-length validation at the dispatch point +// ============================================================================ + +/// Build a promoted peer and return the node, the peer's address, and the +/// session index inbound frames must name to reach it. +/// +/// `handle_encrypted_frame` looks a frame up by `(transport_id, receiver_idx)` +/// in `peers_by_index`, so a frame carrying this index reaches the decrypt and +/// bumps the peer's failure counter. That counter is how the tests below tell +/// "the frame reached its handler" apart from "the frame was dropped before +/// the dispatch": an unknown session is dropped silently and counts nothing. +fn node_with_promoted_peer(transport_id: TransportId) -> (Node, NodeAddr, SessionIndex) { + let mut node = make_node(); + let link_id = LinkId::new(1); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1_000); + let node_addr = *identity.node_addr(); + node.promote_connection(link_id, identity, 2_000).unwrap(); + let our_index = node + .get_peer(&node_addr) + .and_then(|p| p.our_index()) + .expect("promoted peer must have our_index"); + (node, node_addr, our_index) +} + +/// A well-formed established frame carrying 40 bytes of inner plaintext. +/// +/// 16-byte header + 40 + 16-byte tag = 72 bytes on the wire, declaring 40. +/// The ciphertext is filler: these tests are about the length check, and +/// every one of them stops before or at the AEAD. +fn established_frame_declaring_40(receiver_idx: SessionIndex) -> Vec { + use crate::noise::TAG_SIZE; + use crate::proto::fmp::wire::{build_encrypted, build_established_header}; + + let header = build_established_header(receiver_idx, 0, 0, 40); + build_encrypted(&header, &[0u8; 40 + TAG_SIZE]) +} + +#[tokio::test] +async fn an_established_frame_whose_declared_payload_len_disagrees_with_its_length_is_dropped() { + let transport_id = TransportId::new(1); + let (mut node, node_addr, our_index) = node_with_promoted_peer(transport_id); + + let mut frame = established_frame_declaring_40(our_index); + // Bytes 2-3 are the little-endian payload_len. 68 is what a validator + // written to the field's looser description would compute for this frame + // (72 on the wire minus the 4-byte common prefix), so it is both a wrong + // value and the specific wrong value worth naming. + frame[2..4].copy_from_slice(&68u16.to_le_bytes()); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 1, + "the frame must be counted as a framing drop" + ); + assert_eq!( + node.get_peer(&node_addr) + .expect("peer must survive a dropped frame") + .consecutive_decrypt_failures(), + 0, + "the frame must be dropped before the phase dispatch, so the \ + encrypted-frame handler never sees it" + ); +} + +#[tokio::test] +async fn an_established_frame_with_a_correct_payload_len_is_not_dropped() { + let transport_id = TransportId::new(1); + let (mut node, node_addr, our_index) = node_with_promoted_peer(transport_id); + + let frame = established_frame_declaring_40(our_index); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 0, + "a frame whose header agrees with its length must not be dropped" + ); + assert_eq!( + node.get_peer(&node_addr) + .expect("peer must survive a failed decrypt below the threshold") + .consecutive_decrypt_failures(), + 1, + "the frame must reach the encrypted-frame handler, where the filler \ + ciphertext fails the AEAD tag" + ); +} + +#[tokio::test] +async fn a_msg1_with_a_correct_payload_len_is_not_dropped() { + use crate::proto::fmp::wire::build_msg1; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + // A real-shaped msg1 with filler Noise bytes: `build_msg1` writes the + // only payload_len the msg1 arm accepts, and the handshake fails one + // step later at the DH, which is the observable that it got there. + let frame = build_msg1(SessionIndex::new(1), &[0u8; 106]); + + node.process_packet(ReceivedPacket::new( + transport_id, + TransportAddr::from_string("127.0.0.1:2121"), + frame, + )) + .await; + + assert_eq!( + node.stats().transport.payload_len_mismatch, + 0, + "a msg1 built by the encoder must not be dropped by the length check" + ); + assert_eq!( + node.stats().handshake.bad_state, + 1, + "the msg1 must reach handle_msg1, which rejects the filler Noise \ + payload at the DH" + ); +} + +// ============================================================================ +// Identity confirmation after the DH (the address-keyed msg1 carve-out) +// ============================================================================ + +/// A node whose only transport refuses fresh inbound handshakes, paired with +/// a second started UDP socket standing in for the far end. +/// +/// Returns the node, the far end's address, the far end's receive channel, +/// and the far end's transport, which the caller must keep alive for its +/// receive loop to go on running. +/// +/// Both transports are started on purpose. A msg2 the node decides to send +/// then really leaves it and really arrives on the returned channel, which is +/// what makes "no msg2 was sent" an observation about the confirmation rather +/// than a property of the fixture: on an unstarted transport every send +/// fails, and the send-failure arm records the same reject the confirmation +/// records, so the two worlds would be indistinguishable. +async fn node_refusing_inbound( + transport_id: TransportId, +) -> ( + Node, + TransportAddr, + crate::transport::PacketRx, + crate::transport::udp::UdpTransport, +) { + use crate::config::UdpConfig; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + + let refusing = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let mut udp = UdpTransport::new(transport_id, None, refusing, tx); + udp.start_async().await.expect("node transport must bind"); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + let far_end_config = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }; + let (far_tx, far_rx) = packet_channel(64); + let mut far_end = UdpTransport::new(TransportId::new(200), None, far_end_config, far_tx); + far_end.start_async().await.expect("far end must bind"); + let far_addr = TransportAddr::from_string( + &far_end + .local_addr() + .expect("a started transport has a local address") + .to_string(), + ); + + (node, far_addr, far_rx, far_end) +} + +/// A real, cryptographically valid msg1 from `initiator` addressed to +/// `responder`'s static key. +/// +/// It has to be valid under our static, or `receive_handshake_init` refuses +/// it for the wrong reason and the test passes without ever reaching the +/// confirmation. +fn genuine_msg1(initiator: &Node, responder: &Node) -> Vec { + use crate::proto::fmp::wire::build_msg1; + + let responder_identity = PeerIdentity::from_pubkey_full(responder.identity().pubkey_full()); + let mut machine = outbound_leg(LinkId::new(9_999), responder_identity, 1_000); + let noise_msg1 = machine + .start_handshake( + initiator.identity().keypair(), + initiator.startup_epoch(), + 1_000, + ) + .expect("the initiator side of a real msg1 must build"); + build_msg1(SessionIndex::new(7), &noise_msg1) +} + +/// The node address a `Node`'s own identity presents to its peers. +fn node_addr_of(node: &Node) -> NodeAddr { + *PeerIdentity::from_pubkey_full(node.identity().pubkey_full()).node_addr() +} + +/// The FMP phase byte of a packet, for telling a msg1 from a msg2 on the wire. +fn wire_phase(data: &[u8]) -> Option { + crate::proto::fmp::wire::CommonPrefix::parse(data).map(|p| p.phase) +} + +/// Assert that nothing arrives on `rx` within a window long enough for a +/// localhost datagram to have been delivered had one been sent. +async fn assert_nothing_sent(rx: &mut crate::transport::PacketRx, why: &str) { + let arrival = tokio::time::timeout(std::time::Duration::from_millis(250), rx.recv()).await; + assert!(arrival.is_err(), "{}", why); +} + +#[tokio::test] +async fn a_msg1_spoofed_from_an_established_peers_address_is_dropped_after_the_dh_reveals_a_different_identity() + { + use crate::peer::ActivePeer; + + let transport_id = TransportId::new(1); + let (mut node, victim_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // The victim: a promoted peer at the address the spoofed msg1 will be + // sourced from, so the carve-out admits the msg1 past the refusing gate. + let victim = make_node(); + let victim_identity = PeerIdentity::from_pubkey_full(victim.identity().pubkey_full()); + let victim_node_addr = *victim_identity.node_addr(); + let victim_link = node.allocate_link_id(); + let mut victim_peer = ActivePeer::new(victim_identity, victim_link, 1_000); + victim_peer.set_current_addr(transport_id, victim_addr.clone()); + node.peers.insert(victim_node_addr, victim_peer); + node.addr_to_link + .insert((transport_id, victim_addr.clone()), victim_link); + + // The off-path party: a genuine msg1 under our static, built with a + // different identity's keypair, sourced from the victim's address. + let attacker = make_node(); + let attacker_node_addr = node_addr_of(&attacker); + let wire_msg1 = genuine_msg1(&attacker, &node); + + let bad_state_before = node.stats().handshake.bad_state; + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + victim_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + assert!( + node.get_peer(&attacker_node_addr).is_none(), + "the identity the DH revealed does not own the address the waiver \ + matched, so it must not become a peer" + ); + assert_eq!( + node.peer_count(), + 1, + "only the victim may remain a peer after the spoofed msg1" + ); + assert_eq!( + node.connection_count(), + 0, + "the rejected msg1 must leave no connection behind" + ); + assert_eq!( + node.link_count(), + 0, + "the rejected msg1 must leave no link behind" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); + assert_nothing_sent( + &mut far_rx, + "no msg2 may reach the victim's address: the responder answers only \ + after the confirmation, and this msg1 must not get that far", + ) + .await; +} + +#[tokio::test] +async fn a_msg1_admitted_by_a_link_no_identity_owns_is_dropped_after_the_dh() { + let transport_id = TransportId::new(1); + let (mut node, source_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // The fixture `test_should_admit_msg1_admits_rekey_when_accept_off` uses: + // a reverse-address entry with no peer and no connection behind it. It is + // enough to waive the refusing gate, and it attributes the address to + // nobody. + let link_id = node.allocate_link_id(); + node.addr_to_link + .insert((transport_id, source_addr.clone()), link_id); + + assert!( + node.should_admit_msg1(transport_id, &source_addr), + "the fixture must exercise the carve-out, not the gate" + ); + + let initiator = make_node(); + let initiator_node_addr = node_addr_of(&initiator); + let wire_msg1 = genuine_msg1(&initiator, &node); + + let bad_state_before = node.stats().handshake.bad_state; + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + source_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + assert!( + node.get_peer(&initiator_node_addr).is_none(), + "a msg1 the waiver could attribute to no identity must not promote \ + the identity the DH revealed" + ); + assert_eq!(node.peer_count(), 0, "no peer may be created"); + assert_eq!( + node.connection_count(), + 0, + "the rejected msg1 must leave no connection behind" + ); + assert_eq!( + node.link_count(), + 0, + "the rejected msg1 must leave no link behind" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); + assert_nothing_sent( + &mut far_rx, + "no msg2 may leave the node for an address no identity owns", + ) + .await; +} + +#[tokio::test] +async fn a_msg1_from_a_link_whose_dial_expects_this_identity_is_admitted() { + let transport_id = TransportId::new(1); + let (mut node, peer_addr, mut far_rx, _far_end) = node_refusing_inbound(transport_id).await; + + // UDP is connectionless, so `initiate_connection` runs `start_handshake` + // in the same synchronous stretch: the reverse-address entry and the + // connection carrying the dialled identity land together, and the + // classifier can attribute the address. Do not substitute a + // connection-oriented transport here; that arm defers `start_handshake` + // and is the window the next test is about. + let peer = make_node(); + let peer_identity = PeerIdentity::from_pubkey_full(peer.identity().pubkey_full()); + node.initiate_connection(transport_id, peer_addr.clone(), peer_identity) + .await + .expect("the dial must register the link and the connection"); + + // Our own dial's msg1 went out first; drain it so the assertion below is + // about the answer to the crossing msg1. + let ours = tokio::time::timeout(std::time::Duration::from_secs(1), far_rx.recv()) + .await + .expect("our dial's msg1 must arrive") + .expect("the far end's channel must be open"); + assert_eq!( + wire_phase(&ours.data), + Some(crate::proto::fmp::wire::PHASE_MSG1), + "the dial's own packet is a msg1" + ); + + let bad_state_before = node.stats().handshake.bad_state; + let wire_msg1 = genuine_msg1(&peer, &node); + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + peer_addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + let answer = tokio::time::timeout(std::time::Duration::from_secs(1), far_rx.recv()) + .await + .expect("the crossing msg1 must be answered") + .expect("the far end's channel must be open"); + assert_eq!( + wire_phase(&answer.data), + Some(crate::proto::fmp::wire::PHASE_MSG2), + "a simultaneous open with the peer we dialled must still be answered" + ); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 0, + "a crossing msg1 from the identity the dial expects must not be \ + rejected" + ); +} + +#[tokio::test] +async fn a_crossing_msg1_in_the_connection_oriented_dial_window_is_rejected() { + use crate::config::TcpConfig; + use crate::transport::tcp::TcpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + // bind_addr=None makes accept_connections() false, the idiom + // `test_should_admit_msg1_rejects_fresh_when_accept_off` uses. + let cfg = TcpConfig { + bind_addr: None, + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let tcp = TcpTransport::new(transport_id, None, cfg, tx); + node.transports + .insert(transport_id, TransportHandle::Tcp(tcp)); + + let peer = make_node(); + let peer_identity = PeerIdentity::from_pubkey_full(peer.identity().pubkey_full()); + let addr = TransportAddr::from_string("10.0.0.2:2121"); + + // The state `initiate_connection`'s connection-oriented arm leaves behind + // while the transport connect is outstanding: a link, a reverse-address + // entry, a pending connect, and no connection yet. Built directly rather + // than by driving a connect, which would need a reachable peer. + let link_id = node.allocate_link_id(); + let link = Link::new( + link_id, + transport_id, + addr.clone(), + LinkDirection::Outbound, + Duration::from_millis(100), + ); + node.links.insert(link_id, link); + node.addr_to_link + .insert((transport_id, addr.clone()), link_id); + node.peering.pending_connects.push(PendingConnect { + link_id, + transport_id, + remote_addr: addr.clone(), + peer_identity, + }); + + let bad_state_before = node.stats().handshake.bad_state; + let wire_msg1 = genuine_msg1(&peer, &node); + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + addr.clone(), + wire_msg1, + 2_000, + )) + .await; + + // This is the assertion that separates the two worlds, and it is the one + // to read first when the test reds. After the change the reject returns + // above every registry mutation, so the dial's own entry is untouched. + // Without it, `handle_msg1` allocates a fresh link id, writes it over + // this entry, and then removes the entry outright when the msg2 send + // fails on the unstarted transport. + assert_eq!( + node.addr_to_link.get(&(transport_id, addr.clone())), + Some(&link_id), + "the dial's reverse-address entry must still name the dial's own link" + ); + // The remaining three state the shape of the outcome. They hold either + // way for this fixture, whose unstarted TCP transport cannot send, so + // they are not what makes this test able to fail. + assert_eq!(node.peer_count(), 0, "no peer may be created"); + assert_eq!(node.connection_count(), 0, "no connection may be created"); + assert_eq!( + node.stats().handshake.bad_state - bad_state_before, + 1, + "the drop must be counted" + ); +} + +#[tokio::test] +async fn msg1_waiver_classifies_all_three_outcomes() { + use crate::config::UdpConfig; + use crate::node::handlers::handshake::Msg1Waiver; + use crate::peer::ActivePeer; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + + // A registered accepting transport, not an unregistered id, so the + // NotNeeded assertion exercises the `accept_connections()` limb of the + // guard rather than its absent-transport fallback. + let accepting_id = TransportId::new(1); + let (accept_tx, _accept_rx) = packet_channel(64); + let accepting = UdpTransport::new( + accepting_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(true), + ..Default::default() + }, + accept_tx, + ); + node.transports + .insert(accepting_id, TransportHandle::Udp(accepting)); + + let refusing_id = TransportId::new(2); + let (refuse_tx, _refuse_rx) = packet_channel(64); + let refusing = UdpTransport::new( + refusing_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }, + refuse_tx, + ); + node.transports + .insert(refusing_id, TransportHandle::Udp(refusing)); + + let classify = |node: &Node, transport_id: TransportId, addr: &TransportAddr| { + node.msg1_waiver( + node.is_established_link_msg1(transport_id, addr), + transport_id, + addr, + ) + }; + + // NotNeeded: the gate would have admitted this msg1 anyway, so nothing + // was waived and there is nothing to confirm. + let fresh = TransportAddr::from_string("10.0.0.2:2121"); + assert_eq!( + classify(&node, accepting_id, &fresh), + Msg1Waiver::NotNeeded, + "an accepting transport waives nothing" + ); + + // Unattributed: the carve-out admits on a bare reverse-address entry + // that names neither a promoted peer nor a carrier. + let bare = TransportAddr::from_string("10.0.0.3:2121"); + let bare_link = node.allocate_link_id(); + node.addr_to_link + .insert((refusing_id, bare.clone()), bare_link); + assert_eq!( + classify(&node, refusing_id, &bare), + Msg1Waiver::Unattributed, + "a link no identity owns must fail closed" + ); + + // Expect, by promoted peer. `ActivePeer::new` sets neither transport_id + // nor current_addr, so this peer is reached through the reverse-address + // entry that names its link. + let promoted_addr = TransportAddr::from_string("10.0.0.4:2121"); + let promoted_link = node.allocate_link_id(); + let promoted = make_peer_identity(); + let promoted_node_addr = *promoted.node_addr(); + node.peers.insert( + promoted_node_addr, + ActivePeer::new(promoted, promoted_link, 1_000), + ); + node.addr_to_link + .insert((refusing_id, promoted_addr.clone()), promoted_link); + assert_eq!( + classify(&node, refusing_id, &promoted_addr), + Msg1Waiver::Expect(promoted_node_addr), + "an address whose link a promoted peer owns is attributed to that peer" + ); + + // Expect, by carrier: a link with no promoted peer yet, whose connection + // carries the identity the dial expects. + let carrier_addr = TransportAddr::from_string("10.0.0.5:2121"); + let carrier_link = node.allocate_link_id(); + let dialled = make_peer_identity(); + let dialled_node_addr = *dialled.node_addr(); + node.seed_handshake_machine(HandshakeSeed::outbound(carrier_link, dialled, 1_000)) + .unwrap(); + node.addr_to_link + .insert((refusing_id, carrier_addr.clone()), carrier_link); + assert_eq!( + classify(&node, refusing_id, &carrier_addr), + Msg1Waiver::Expect(dialled_node_addr), + "an address whose link carries a dialled identity is attributed to it" + ); +} + +#[tokio::test] +async fn a_stale_reverse_address_entry_does_not_hide_a_peer_reachable_by_address() { + use crate::config::UdpConfig; + use crate::node::handlers::handshake::Msg1Waiver; + use crate::peer::ActivePeer; + use crate::transport::udp::UdpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + let (tx, _rx) = packet_channel(64); + let udp = UdpTransport::new( + transport_id, + None, + UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + accept_connections: Some(false), + ..Default::default() + }, + tx, + ); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + // A live peer whose current_addr is the numeric form inbound packets + // carry, which is the second predicate's whole reason for existing. + let numeric = TransportAddr::from_string("100.64.0.5:2121"); + let peer_link = node.allocate_link_id(); + let peer_identity = make_peer_identity(); + let peer_node_addr = *peer_identity.node_addr(); + let mut peer = ActivePeer::new(peer_identity, peer_link, 1_000); + peer.set_current_addr(transport_id, numeric.clone()); + node.peers.insert(peer_node_addr, peer); + + // A reverse-address entry at that same numeric address naming a link + // that no longer exists. `remove_link` clears the reverse lookup only + // under the key it rebuilds from the removed link's own remote address, + // so an entry inserted for that link under a second address form outlives + // it, and link ids are never reused. + let dead_link = node.allocate_link_id(); + assert_ne!( + dead_link, peer_link, + "the stale entry names a different link" + ); + node.addr_to_link + .insert((transport_id, numeric.clone()), dead_link); + + assert_eq!( + node.msg1_waiver( + node.is_established_link_msg1(transport_id, &numeric), + transport_id, + &numeric + ), + Msg1Waiver::Expect(peer_node_addr), + "the stale entry must not hide the peer the address scan finds: \ + classifying this Unattributed would reject the peer's rekey msg1 \ + after the DH, and would go on rejecting it, because the reject \ + returns above the insert that would overwrite the stale entry" + ); +} diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 63c25f1d..256f820e 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1682,7 +1682,7 @@ async fn test_initiate_peer_connections_schedules_retry_on_no_transport() { } // ============================================================================ -// transport_mtu() — ISSUE-2026-0011 regression coverage +// transport_mtu() — minimum-across-transports regression coverage // ============================================================================ /// Helper: spawn a UdpTransport with the given mtu, started and operational. @@ -1707,7 +1707,7 @@ async fn make_udp_transport_with_mtu(id: u32, mtu: u16) -> TransportHandle { async fn test_transport_mtu_returns_min_across_operational() { // Multiple operational transports with varied MTUs. The picker must // return the smallest, deterministically, regardless of HashMap - // iteration order. This is the core ISSUE-2026-0011 regression test. + // iteration order. This is the core regression test for that. let mut node = make_node(); let (packet_tx, packet_rx) = packet_channel(64); node.supervisor.packet_tx = Some(packet_tx); diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index a7de40bc..5116707f 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -9,6 +9,7 @@ use rand::Rng; use secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey, ecdh::shared_secret_point}; use sha2::{Digest, Sha256}; use std::fmt; +use zeroize::{Zeroize, ZeroizeOnDrop}; /// Symmetric state during handshake. /// @@ -17,13 +18,24 @@ use std::fmt; /// `Clone` exists for [`HandshakeState::try_read_xk_message_2`], which has to /// put the pre-read state back after a message that mixed material in before /// failing to authenticate. -#[derive(Clone)] +/// +/// `ck` and `h` are cleared on drop, including on the clone above once it +/// goes out of scope. `cipher` is skipped because [`CipherState`] clears its +/// own retained key. +#[derive(Clone, Zeroize, ZeroizeOnDrop)] struct SymmetricState { /// Chaining key for key derivation. ck: [u8; 32], - /// Handshake hash for transcript binding. + /// Running SHA-256 accumulator over the handshake transcript. + /// + /// Maintained by `mix_hash` at every step, but never fed to the AEAD: + /// `encrypt_and_hash` passes an empty associated-data field. Nothing in + /// production reads it, so it provides no transcript binding today, and + /// anything built on `handshake_hash()` (channel binding, an exporter, + /// cookie binding) will silently not work until the AAD carries `h`. h: [u8; 32], /// Current cipher state for encrypting handshake payloads. + #[zeroize(skip)] cipher: CipherState, } @@ -70,6 +82,9 @@ impl SymmetricState { let mut key = [0u8; 32]; key.copy_from_slice(&output[32..64]); self.cipher.initialize_key(key); + key.zeroize(); + + output.zeroize(); } /// Encrypt and mix into hash. @@ -98,10 +113,16 @@ impl SymmetricState { k1.copy_from_slice(&output[..32]); k2.copy_from_slice(&output[32..64]); - (CipherState::new(k1), CipherState::new(k2)) + let ciphers = (CipherState::new(k1), CipherState::new(k2)); + + output.zeroize(); + k1.zeroize(); + k2.zeroize(); + + ciphers } - /// Get the handshake hash (for channel binding). + /// Get the handshake hash. fn handshake_hash(&self) -> [u8; 32] { self.h } @@ -158,7 +179,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key and will send first. /// Used by FMP (link layer). - pub fn new_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -180,6 +201,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -187,7 +212,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 1. Used by FMP (link layer). - pub fn new_responder(static_keypair: Keypair) -> Self { + pub fn new_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -208,6 +233,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -215,7 +244,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key. XK defers the /// initiator's static key reveal to msg3. Used by FSP (session layer). - pub fn new_xk_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_xk_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -235,6 +264,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -242,7 +275,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 3. Used by FSP (session layer). - pub fn new_xk_responder(static_keypair: Keypair) -> Self { + pub fn new_xk_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -262,6 +295,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -301,9 +338,15 @@ impl HandshakeState { let mut secret_bytes = [0u8; 32]; rng.fill_bytes(&mut secret_bytes); - let secret_key = + let mut secret_key = SecretKey::from_slice(&secret_bytes).expect("32 random bytes is valid secret key"); self.ephemeral_keypair = Some(Keypair::from_secret_key(&self.secp, &secret_key)); + secret_bytes.zeroize(); + // The erase is called non-secure because the private key may sit in + // further copies this function cannot name. It still clears the copy + // this frame owns; the keypair the key was just stored in is cleared + // by `Drop for HandshakeState`. + secret_key.non_secure_erase(); } /// Perform ECDH between our secret and their public key. @@ -314,15 +357,26 @@ impl HandshakeState { /// may have the wrong parity for the responder's static key. Since P and /// -P produce ECDH result points with the same x-coordinate, hashing /// only x ensures both sides derive the same shared secret. + /// + /// `our_secret` is borrowed, so this frame makes no copy of it. Every + /// caller below binds the value `Keypair::secret_key` hands back and + /// erases that binding once the DH is done, because the returned + /// `SecretKey` is a whole private key rather than a handle to one. Those + /// erases clear the copies this crate owns; `secp256k1` names its erase + /// non-secure because the compiler may hold further copies that no code + /// here can name. fn ecdh(&self, our_secret: &SecretKey, their_public: &PublicKey) -> [u8; 32] { // Get raw (x, y) coordinates (64 bytes) without any hashing - let point = shared_secret_point(their_public, our_secret); + let mut point = shared_secret_point(their_public, our_secret); // Hash only the x-coordinate (first 32 bytes), ignoring y/parity let mut hasher = Sha256::new(); hasher.update(&point[..32]); - let hash = hasher.finalize(); + let mut hash = hasher.finalize(); let mut result = [0u8; 32]; result.copy_from_slice(&hash); + hash.as_mut_slice().zeroize(); + point.zeroize(); + // `result` is moved out, so clearing it belongs to the caller. result } @@ -367,8 +421,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: encrypt our static and send let our_static = self.static_keypair.public_key().serialize(); @@ -376,8 +433,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &remote_static); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -420,8 +480,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: decrypt initiator's static let encrypted_static_end = PUBKEY_SIZE + PUBKEY_SIZE + super::TAG_SIZE; @@ -432,8 +495,11 @@ impl HandshakeState { self.remote_static = Some(rs); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &rs); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &rs); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -487,12 +553,18 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -535,14 +607,20 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(e, rs), mix into key // (initiator uses their ephemeral with responder's static) let rs = self.remote_static.expect("initiator has remote static"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -599,8 +677,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -639,8 +720,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -686,8 +770,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -731,8 +818,11 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -818,8 +908,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -869,8 +962,11 @@ impl HandshakeState { .ephemeral_keypair .as_ref() .expect("should have ephemeral after msg2"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -916,12 +1012,32 @@ impl HandshakeState { )) } - /// Get the handshake hash (for channel binding, available after complete). + /// Get the handshake hash (available after complete). pub fn handshake_hash(&self) -> [u8; 32] { self.symmetric.handshake_hash() } } +impl Drop for HandshakeState { + /// Erase the two private keys this state holds. + /// + /// `static_keypair` is the node's long-term private key and + /// `ephemeral_keypair` is the handshake's own. Both live here for the + /// whole handshake, which is longer than in any other frame, so this is + /// where clearing them matters most. `Keypair` is `Copy` and so cannot + /// clear itself on drop; `HandshakeState` is not, so it does it for both. + /// + /// This clears the copies this crate owns, not every copy that ever + /// existed: `secp256k1` names its erase non-secure because the compiler + /// may duplicate or move the bytes to places no code here can name. + fn drop(&mut self) { + self.static_keypair.non_secure_erase(); + if let Some(ephemeral) = self.ephemeral_keypair.as_mut() { + ephemeral.non_secure_erase(); + } + } +} + impl fmt::Debug for HandshakeState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("HandshakeState") diff --git a/src/noise/mod.rs b/src/noise/mod.rs index a3d656c4..85366e4f 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -42,6 +42,7 @@ mod session; use ring::aead::{Aad, CHACHA20_POLY1305, LessSafeKey, Nonce, UnboundKey}; use std::fmt; use thiserror::Error; +use zeroize::{Zeroize, ZeroizeOnDrop}; pub use handshake::HandshakeState; pub use replay::ReplayWindow; @@ -181,21 +182,32 @@ impl fmt::Display for HandshakeProgress { /// AEAD is `ring`'s ChaCha20-Poly1305 (BoringSSL backend), which dispatches /// to NEON on aarch64 and AVX2/AVX-512 on x86_64. The 32-byte key is /// retained alongside a cached `LessSafeKey` so the per-packet AEAD skips -/// the keyed-cipher construction (key copy + Poly1305 key derivation). -/// `LessSafeKey` itself doesn't implement `Clone` (deliberate, for safety), -/// so `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from the -/// retained key bytes — cheap for ChaCha20-Poly1305 since the construction -/// is essentially a key copy plus a constant-time check. +/// rebuilding the keyed cipher. (It does not skip Poly1305 key derivation, +/// which is nonce-dependent and happens per message inside seal and open.) +/// `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from those +/// retained bytes, and so does `cipher_clone` for each off-task AEAD worker; +/// those two are why the bytes are kept. The rebuild is cheap for +/// ChaCha20-Poly1305: a length check and a conversion of the 32 key bytes +/// into little-endian words. +/// +/// Because the key bytes are retained, every clone leaves a second copy in +/// memory; each copy is cleared when it goes out of scope. Only `key` is +/// zeroized. `cipher` is skipped because `LessSafeKey` +/// holds its material behind `ring`'s own API and cannot be cleared from +/// here; `nonce` and `has_key` are skipped because they are not secret. +#[derive(Zeroize, ZeroizeOnDrop)] pub struct CipherState { - /// Encryption key (32 bytes). Retained so we can rebuild the keyed - /// AEAD on `Clone` and on `initialize_key` (ring's `UnboundKey` / - /// `LessSafeKey` do not implement `Clone`). + /// Encryption key (32 bytes). Retained because `Clone` and + /// `cipher_clone` rebuild the keyed AEAD from these bytes. key: [u8; 32], /// Cached keyed AEAD, valid iff `has_key`. None for an un-keyed state. + #[zeroize(skip)] cipher: Option, /// Nonce counter (8 bytes used, 4 bytes zero prefix). + #[zeroize(skip)] pub(super) nonce: u64, /// Whether this cipher has a valid key. + #[zeroize(skip)] has_key: bool, } @@ -217,14 +229,19 @@ impl Clone for CipherState { impl CipherState { /// Create a new cipher state with the given key. - pub(crate) fn new(key: [u8; 32]) -> Self { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(crate) fn new(mut key: [u8; 32]) -> Self { let cipher = Self::build_cipher(&key); - Self { + let state = Self { key, cipher, nonce: 0, has_key: true, - } + }; + key.zeroize(); + state } /// Create an empty cipher state (no key yet). @@ -238,11 +255,15 @@ impl CipherState { } /// Initialize with a key. - pub(super) fn initialize_key(&mut self, key: [u8; 32]) { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(super) fn initialize_key(&mut self, mut key: [u8; 32]) { self.key = key; self.cipher = Self::build_cipher(&key); self.nonce = 0; self.has_key = true; + key.zeroize(); } /// Build a ring `LessSafeKey` from raw key bytes. Centralized so the @@ -395,6 +416,13 @@ impl CipherState { self.has_key } + /// Copy out the retained key bytes, so a test can observe zeroization + /// without reading freed memory. + #[cfg(test)] + fn key_bytes(&self) -> [u8; 32] { + self.key + } + /// Clone the underlying keyed AEAD, for off-task AEAD workers. /// /// Returns `None` if no key. The cloned `LessSafeKey` pairs with diff --git a/src/noise/session.rs b/src/noise/session.rs index 0df0aaef..92a8a059 100644 --- a/src/noise/session.rs +++ b/src/noise/session.rs @@ -14,7 +14,7 @@ pub struct NoiseSession { send_cipher: CipherState, /// Cipher for receiving. recv_cipher: CipherState, - /// Handshake hash for channel binding. + /// Handshake hash. handshake_hash: [u8; 32], /// Remote peer's static public key. remote_static: PublicKey, @@ -190,7 +190,7 @@ impl NoiseSession { self.replay_window.reset(); } - /// Get the handshake hash for channel binding. + /// Get the handshake hash. pub fn handshake_hash(&self) -> &[u8; 32] { &self.handshake_hash } diff --git a/src/noise/tests.rs b/src/noise/tests.rs index f452f8bc..82cd7a93 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -201,6 +201,30 @@ fn test_cipher_state_nonce_sequence() { assert_eq!(cipher.nonce(), 2); } +#[test] +fn cipher_state_drop_impl_clears_the_retained_key() { + // Reading the bytes back out of a dropped `CipherState` would mean + // reading freed memory, so the drop behaviour is asserted through two + // observable proxies instead: that `CipherState` implements + // `ZeroizeOnDrop`, and that an explicit `zeroize()` leaves `key` + // all-zero while `has_key` is untouched. + fn assert_zeroize_on_drop() {} + assert_zeroize_on_drop::(); + + let mut cipher = CipherState::new([7u8; 32]); + // `key_bytes` hands back a copy of live key material, so the observation + // is bound and cleared rather than left in an unnamed temporary. + let mut observed = cipher.key_bytes(); + assert_eq!(observed, [7u8; 32]); + observed.zeroize(); + assert!(cipher.has_key()); + + cipher.zeroize(); + + assert_eq!(cipher.key_bytes(), [0u8; 32]); + assert!(cipher.has_key()); +} + #[test] fn test_session_remote_static() { let keypair1 = generate_keypair(); diff --git a/src/nostr/runtime.rs b/src/nostr/runtime.rs index cfa0f0c6..76b505fa 100644 --- a/src/nostr/runtime.rs +++ b/src/nostr/runtime.rs @@ -15,6 +15,7 @@ use serde::Serialize; use tokio::sync::{Mutex, Notify, RwLock, broadcast, mpsc, oneshot}; use tokio::task::JoinHandle; use tracing::{debug, info, trace, warn}; +use zeroize::{Zeroize, Zeroizing}; use super::advert::{AdvertMachine, PublishPlan}; use super::failure_state::FailureState; @@ -286,7 +287,15 @@ impl NostrRendezvous { return Err(BootstrapError::Disabled); } - let keys = nostr::Keys::parse(&hex::encode(identity.keypair().secret_bytes())) + // Three copies of the private key are made to reach `Keys::parse`: + // the keypair, its raw bytes, and the hex string. Each is bound and + // cleared here; `nostr::Keys` clears its own on drop. + let mut our_keypair = identity.keypair(); + let mut secret_bytes = our_keypair.secret_bytes(); + let secret_hex = Zeroizing::new(hex::encode(secret_bytes)); + secret_bytes.zeroize(); + our_keypair.non_secure_erase(); + let keys = nostr::Keys::parse(secret_hex.as_str()) .map_err(|e| BootstrapError::Nostr(e.to_string()))?; let client = Client::builder() .signer(keys.clone()) diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 316af9d6..d50f78df 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -56,6 +56,7 @@ #![allow(dead_code)] +use crate::identity::ErasingKeypair; use crate::noise::{self, NoiseError, NoiseSession}; use crate::proto::fmp::{ ConnAction, ConnSnapshot, ConnectionState, EstablishSnapshot, Fmp, InboundDecision, @@ -617,10 +618,15 @@ impl PeerMachine { /// The epoch is our startup epoch, encrypted into msg1 for restart detection. pub(crate) fn start_handshake( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], current_time_ms: u64, ) -> Result, NoiseError> { + // The parameter is this frame's own copy of the node's long-term + // private key, and the state checks below return before it is used. + // The guard clears it on every exit path. + let our_keypair = ErasingKeypair::take(&mut our_keypair); + let msg1 = { let direction = self.conn.direction(); let expected_identity = self.conn.expected_identity().copied(); @@ -637,7 +643,9 @@ impl PeerMachine { .expect("outbound must have expected identity") .pubkey_full(); - let mut hs = noise::HandshakeState::new_initiator(our_keypair, remote_static); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_initiator(kp, remote_static); + kp.non_secure_erase(); hs.set_local_epoch(epoch); let msg1 = hs.write_message_1()?; @@ -656,11 +664,15 @@ impl PeerMachine { /// The epoch is our startup epoch, encrypted into msg2 for restart detection. pub(crate) fn receive_handshake_init( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], message: &[u8], current_time_ms: u64, ) -> Result, NoiseError> { + // Same as `start_handshake`: the parameter copy outlives two early + // returns, so the guard owns it rather than an erase per exit path. + let our_keypair = ErasingKeypair::take(&mut our_keypair); + let (msg2, learned_identity, remote_epoch) = { let direction = self.conn.direction(); let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; @@ -672,7 +684,9 @@ impl PeerMachine { }); } - let mut hs = noise::HandshakeState::new_responder(our_keypair); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_responder(kp); + kp.non_secure_erase(); hs.set_local_epoch(epoch); // Process message 1 (this reveals the initiator's identity and epoch) diff --git a/src/proto/fmp/wire.rs b/src/proto/fmp/wire.rs index fc7cc615..3e5b87aa 100644 --- a/src/proto/fmp/wire.rs +++ b/src/proto/fmp/wire.rs @@ -234,6 +234,38 @@ pub const FLAG_CE: u8 = 0x02; /// Spin bit for RTT measurement. pub const FLAG_SP: u8 = 0x04; +// ============================================================================ +// Wire Length Validation +// ============================================================================ + +/// Expected `payload_len` for a packet of `phase` and total wire length +/// `total`, or `None` if the phase carries no fixed relationship. +/// +/// Each frame kind states its own relationship; there is no single shared +/// convention. The handshake frames count everything after the 4-byte common +/// prefix. The established frame counts only the inner plaintext, excluding +/// both the 16-byte header and the AEAD tag, which is what +/// [`CommonPrefix::payload_len`] documents. +/// +/// The handshake arms are built from the same constants the encoders use; the +/// established arm is not, since no encoder reads `ENCRYPTED_MIN_SIZE`. Neither +/// shape stops a one-sided edit from making the two disagree, which is what the +/// tests below exist to catch. +/// +/// A `None` from the established arm means "no fixed relationship", so a caller +/// skips such a frame rather than rejecting it. That is the safe direction: a +/// truncating cast here would produce a false rejection instead. +pub fn expected_payload_len(phase: u8, total: usize) -> Option { + match phase { + PHASE_MSG1 => Some((MSG1_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16), + PHASE_MSG2 => Some((MSG2_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16), + PHASE_ESTABLISHED => total + .checked_sub(ENCRYPTED_MIN_SIZE) + .and_then(|n| u16::try_from(n).ok()), + _ => None, + } +} + // ============================================================================ // Common Prefix // ============================================================================ @@ -791,4 +823,40 @@ mod tests { // payload_len = sender_idx(4) + receiver_idx(4) + noise_msg2(57) = 65 assert_eq!(prefix.payload_len, 65); } + + #[test] + fn expected_payload_len_for_an_established_frame_excludes_header_and_tag() { + let header = build_established_header(SessionIndex::new(7), 0, 0, 40); + let frame = build_encrypted(&header, &[0u8; 40 + TAG_SIZE]); + + // 16 header + 40 plaintext + 16 tag = 72 on the wire, declaring 40. + assert_eq!(frame.len(), ESTABLISHED_HEADER_SIZE + 40 + TAG_SIZE); + assert_eq!( + expected_payload_len(PHASE_ESTABLISHED, frame.len()), + Some(40) + ); + } + + #[test] + fn expected_payload_len_matches_what_build_msg1_and_build_msg2_actually_emit() { + // The Noise buffers are sized with literals rather than + // HANDSHAKE_MSG1_SIZE / HANDSHAKE_MSG2_SIZE, unlike the two tests + // above. With the constant on both sides, changing it would move the + // encoder's payload_len and the validator's expectation together and + // leave this test green. Pinning the input keeps the two sides + // independent so a one-sided change is caught. + let msg1 = build_msg1(SessionIndex::new(1), &[0u8; 106]); + let prefix = CommonPrefix::parse(&msg1).unwrap(); + assert_eq!( + expected_payload_len(PHASE_MSG1, msg1.len()), + Some(prefix.payload_len) + ); + + let msg2 = build_msg2(SessionIndex::new(1), SessionIndex::new(2), &[0u8; 57]); + let prefix = CommonPrefix::parse(&msg2).unwrap(); + assert_eq!( + expected_payload_len(PHASE_MSG2, msg2.len()), + Some(prefix.payload_len) + ); + } } diff --git a/src/proto/link.rs b/src/proto/link.rs index 93b4b7f5..ebc4d26b 100644 --- a/src/proto/link.rs +++ b/src/proto/link.rs @@ -21,10 +21,10 @@ pub enum LinkMessageType { /// Payload is opaque to intermediate nodes (end-to-end encrypted). SessionDatagram = 0x00, - // MMP reports (0x01-0x02) — content defined in TASK-2026-0006 - /// Sender-side MMP report (stub). + // MMP reports (0x01-0x02) — payload is an encoded SenderReport or ReceiverReport + /// Sender-side MMP report. SenderReport = 0x01, - /// Receiver-side MMP report (stub). + /// Receiver-side MMP report. ReceiverReport = 0x02, // Tree protocol (0x10-0x1F) diff --git a/src/transport/ble/io.rs b/src/transport/ble/io.rs index 178a8a2f..6b053492 100644 --- a/src/transport/ble/io.rs +++ b/src/transport/ble/io.rs @@ -23,6 +23,15 @@ pub trait BleStream: Send + Sync { /// Receive data from the L2CAP connection. /// /// Returns the number of bytes read into `buf`. + /// + /// A single call must never return bytes drawn from more than one SDU. + /// The receive loop emits what one call returns as one FMP frame. A + /// concatenation is caught by the frame-length check in the node's + /// dispatch only when the leading frame is an established one; where it + /// is a handshake frame, that check passes and the buffer is dropped one + /// step later by that frame kind's exact-size parse. Returning less than + /// a whole SDU is allowed: a truncated frame fails the AEAD tag or the + /// exact-size parse regardless. fn recv( &self, buf: &mut [u8], diff --git a/src/transport/framing.rs b/src/transport/framing.rs index b2edb822..6e317b5e 100644 --- a/src/transport/framing.rs +++ b/src/transport/framing.rs @@ -229,6 +229,28 @@ mod tests { frame } + /// The wire sizes above are written as literals, independently of the FMP + /// wire module, which derives the same values from the Noise message sizes. + /// Keeping them independent is deliberate: this module takes no dependency + /// on `crate::proto::fmp`, and the import below exists only under + /// `cfg(test)`. + /// The cost is that the two can drift. A wrong literal on this side also + /// breaks the TCP integration tests, since they move real frames through + /// this reader; a wrong value on the wire-module side does not reach here + /// at all, and this test is what catches that direction. + #[test] + fn stream_reader_constants_agree_with_the_fmp_wire_module() { + use crate::proto::fmp::wire; + + assert_eq!(MSG1_WIRE_SIZE, wire::MSG1_WIRE_SIZE); + assert_eq!(MSG2_WIRE_SIZE, wire::MSG2_WIRE_SIZE); + assert_eq!(PREFIX_SIZE, wire::COMMON_PREFIX_SIZE); + assert_eq!( + ESTABLISHED_REMAINING_HEADER + PREFIX_SIZE, + wire::ESTABLISHED_HEADER_SIZE + ); + } + #[tokio::test] async fn test_read_established_frame() { let payload_len = 64u16; diff --git a/src/transport/udp/mod.rs b/src/transport/udp/mod.rs index e24c535d..2e8732d1 100644 --- a/src/transport/udp/mod.rs +++ b/src/transport/udp/mod.rs @@ -415,8 +415,8 @@ impl Transport for UdpTransport { /// Whether the transport accepts inbound handshake initiations. /// `outbound_only` mode forces this to false; otherwise reflects the /// `accept_connections` config field (default: true). Note that the - /// hard gate is at the Node level (see ISSUE-2026-0004 fix in - /// `src/node/handlers/handshake.rs`); this method is what that gate + /// hard gate is at the Node level (in `src/node/handlers/handshake.rs`); + /// this method is what that gate /// consults for transports that lack runtime-state-based filtering. fn accept_connections(&self) -> bool { if self.config.outbound_only() { diff --git a/testing/chaos/scenarios/bloom-storm.yaml b/testing/chaos/scenarios/bloom-storm.yaml index 9a017512..ca94eba9 100644 --- a/testing/chaos/scenarios/bloom-storm.yaml +++ b/testing/chaos/scenarios/bloom-storm.yaml @@ -72,7 +72,7 @@ netem: # `interval_secs`, the policies on the two listed edges are swapped. # This drives n04 to alternate parents between n02 and n03 each # round. The 4s cadence and 5ms-vs-100ms delta come from the -# original ISSUE-2026-0019 reproduction harness — they are +# original reproduction harness for this flap — they are # calibrated to produce a parent switch per round under the # zero-hysteresis FIPS overrides below. link_swap: diff --git a/testing/check-comment-refs.sh b/testing/check-comment-refs.sh new file mode 100755 index 00000000..118e6560 --- /dev/null +++ b/testing/check-comment-refs.sh @@ -0,0 +1,181 @@ +#!/bin/bash +# ── Source comment reference guard ────────────────────────────────────────── +# Every reference a source comment makes must resolve for a reader who has +# only this repository. +# +# A comment that names a private planning artifact — by identifier, by file +# path, or in programme vocabulary that has no in-repo referent — is dead text +# to everyone outside the workspace that holds the artifact, and it publishes +# the existence and shape of that workspace to everyone else. A hand-run grep +# closes the population that exists on the day it runs; this closes it for +# every commit after. +# +# Three checks, each a `git grep` at HEAD rather than over the working tree, +# because the thing being gated is what is committed: +# +# 1. identifiers, repo-wide. A local item identifier anywhere in the tree. +# Repo-wide because packaging/, testing/ and the workflow files are as +# public as src/ — more so in the packaging case, which ships to users. +# 2. document paths, scoped to src/. A comment under src/ citing an *.md +# path that does not exist in the tree at the checked commit. This is a +# resolvability rule rather than a denylist, so it catches private +# documents nobody has thought of. Scoped to src/ because resolving the +# whole tree's relative documentation links against the repository root +# is a different checker with its own population to triage first. +# 3. programme vocabulary, scoped to src/. Phase, rung, milestone and step +# labels that name a plan a reader cannot open. The pattern is shared +# verbatim with the sweep that produced today's clean tree, so the two +# cannot drift apart. +# +# Known coverage gaps, recorded rather than discovered: +# - a bare "milestone" in some other phrasing (the narrow alternatives keep +# the Tor bootstrap loop in src/transport/tor/mod.rs out of check 3); +# - the CATEGORY_D / CATEGORY_E code identifiers and test names, which +# check 3's case-sensitive Category-[A-Z] deliberately does not match; +# - programme vocabulary, or a document path, outside src/; +# - a reference to a private artifact made in free prose with no marker at +# all, at any scope: no check here matches it; +# - a pathspec typo introduced after this file lands. git grep returns 1 +# with empty output for a pathspec that matches nothing, which is +# indistinguishable from health. The non-empty-population assertion below +# narrows that for the src/-scoped checks and closes nothing for check 1. +# +# Exit 0 = every reference resolves. Exit 1 = one or more do not; every hit is +# printed, not just the first. Exit 2 = the check could not look (not in a work +# tree, wrong directory, empty pathspec, or a git-level failure); never treated +# as a pass. +# ───────────────────────────────────────────────────────────────────────────── +set -uo pipefail + +# A pathspec is resolved against the current directory even when a rev is +# supplied, and a run from the wrong directory returns rc 1 with empty output +# on every check — the same shape as a clean tree. Assert the directory, then +# assert the post-condition rather than trusting the cd: `cd ""` succeeds and +# does not move, so a one-line form silently leaves the script where it began +# whenever the command substitution comes back empty. +ROOT=$(git rev-parse --show-toplevel 2>/dev/null) \ + || { echo "check-comment-refs: not inside a git work tree" >&2; exit 2; } +[[ -n "$ROOT" ]] \ + || { echo "check-comment-refs: empty work-tree root" >&2; exit 2; } +cd "$ROOT" || exit 2 +[[ -z "$(git rev-parse --show-prefix)" ]] \ + || { echo "check-comment-refs: not at the work-tree root" >&2; exit 2; } + +# The src/ pathspec must select something. wc -l is deliberate where the rest +# of this script preserves exit statuses: it always exits 0, so the decision is +# made on n and never on a status, and a failing git ls-tree yields n=0 and +# fires the assertion. This covers checks 2 and 3 only; check 1's pathspec is +# "." and stays non-empty from anywhere, so its wrong-directory case is covered +# by the --show-prefix assertion above and its mistyped-pathspec case by +# nothing. +n=$(git ls-tree -r --name-only HEAD -- src/ | wc -l) +(( n > 0 )) || { echo "check-comment-refs: pathspec src/ matched no files" >&2; exit 2; } + +# Deliberately a superset of the sweep's own acceptance regex: the year group +# is optional, so the three-digit form is caught as well, and the separator is +# optional so underscores and spaces are caught alongside hyphens. Narrowing +# this is how a whole class goes unguarded while every break-check still +# passes. +ID_PAT='(TASK|ISSUE|IDEA|QUICK|RECUR)[-_ ]?(20[0-9]{2}[-_ ])?[0-9]{3,4}' + +# Verbatim from the sweep's enumerating pattern. Do not edit one without the +# other. If check 3's scope is ever widened beyond src/, this text matches +# itself through six of its alternatives and the widening must add +# ':(exclude)testing/check-comment-refs.sh' — never an exclusion of testing/, +# which holds real check-1 hits a directory-wide exclusion would drop. +VOCAB_PAT='\b[RQ][0-9]\b|Category-[A-Z]|\bumbrella\b|refactor steps?|\bpre-scopes\b|R0 stub|read-isolation|cut over yet|Cutover begins|in Step [0-9]|(the|this) milestone|Milestone-' + +MD_PAT='[A-Za-z0-9_./-]+\.md\b' + +FAILED=0 + +# ── Check 1: local item identifiers, repo-wide ────────────────────────────── +# The capture preserves git grep's status instead of flattening it with +# `|| true`: a legitimate no-hit run exits 1, but so would a malformed +# pathspec magic, an unresolvable rev or a bad regex exit 128, and collapsing +# both into "clean" is the single likeliest way this script reports green +# while checking nothing. The hit decision is made on the output; the +# could-not-look decision is made on the status. +rc=0 +out=$(git grep -nEI "$ID_PAT" HEAD -- .) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 1 grep failed (rc=$rc)" >&2 + exit 2 +fi +if [[ -n "$out" ]]; then + printf '%s\n' "$out" \ + | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a local work item: |' + FAILED=1 +fi + +# ── Check 2: document paths under src/ must resolve in-repo ───────────────── +rc=0 +md_lines=$(git grep -nI '\.md' HEAD -- src/) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 2 grep failed (rc=$rc)" >&2 + exit 2 +fi + +# Drop any line carrying a URL before extracting a token from it. Keying the +# skip on the extracted token cannot work: ':' is outside the token character +# class, so the scheme is stripped first and what survives does not begin with +# "http". Inert on today's tree, and kept so a future URL citation does not red. +cited=$(printf '%s\n' "$md_lines" | grep -vE 'https?://') + +declare -A CITES=() +while IFS= read -r line; do + [[ -n "$line" ]] || continue + loc=${line#HEAD:} + loc=$(printf '%s' "$loc" | sed -E 's|^([^:]*):([0-9]+):.*|\1:\2|') + content=$(printf '%s' "$line" | sed -E 's|^HEAD:[^:]*:[0-9]+:||') + while IFS= read -r tok; do + [[ -n "$tok" ]] || continue + CITES["$tok"]+="$loc " + done < <(printf '%s\n' "$content" | grep -oE "$MD_PAT") +done <<< "$cited" + +if (( ${#CITES[@]} > 0 )); then + while IFS= read -r tok; do + [[ -n "$tok" ]] || continue + lrc=0 + lsout=$(git ls-tree HEAD -- "$tok" 2>/dev/null) || lrc=$? + # A '../'-prefixed or absolute token makes git ls-tree exit 128 with + # "is outside repository". That is neither a hit nor clean: the check + # could not look, and reporting it as a hit would red a legitimate + # citation while reporting it as clean would hide one. + if (( lrc != 0 )); then + echo "check-comment-refs: could not resolve '$tok' (git ls-tree rc=$lrc)" >&2 + exit 2 + fi + if [[ -z "$lsout" ]]; then + for loc in ${CITES["$tok"]}; do + printf '%s: cites %s, which does not exist in this repository\n' \ + "$loc" "$tok" + done + FAILED=1 + fi + done < <(printf '%s\n' "${!CITES[@]}" | sort) +fi + +# ── Check 3: programme vocabulary under src/ ──────────────────────────────── +rc=0 +out=$(git grep -nEI "$VOCAB_PAT" HEAD -- src/) || rc=$? +if (( rc > 1 )); then + echo "check-comment-refs: check 3 grep failed (rc=$rc)" >&2 + exit 2 +fi +if [[ -n "$out" ]]; then + printf '%s\n' "$out" \ + | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a plan this repository does not carry: |' + FAILED=1 +fi + +if (( FAILED != 0 )); then + echo "" >&2 + echo "check-comment-refs: a source comment references something a reader holding" >&2 + echo "only this repository cannot resolve. Rewrite the comment to say what the" >&2 + echo "code does, citing nothing outside the tree." >&2 + exit 1 +fi + +exit 0 diff --git a/testing/ci-local.sh b/testing/ci-local.sh index 7a4c1e3a..1dc06645 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -1267,6 +1267,18 @@ run_action_pins() { record "action-pins" $rc } +# Every reference a source comment makes must resolve for a reader who has only +# this repository. A comment naming a private planning artifact is dead text to +# everyone outside the workspace holding it, and publishes that workspace's +# shape to everyone else. A hand-run grep closes the population that exists on +# the day it runs; this closes it for every commit after. +run_comment_refs() { + local rc=0 + info "[comment-refs] Checking that every source comment resolves in-repo" + "$SCRIPT_DIR/check-comment-refs.sh" || rc=$? + record "comment-refs" $rc +} + # Every daemon log string a test matches on must still be emitted by src/. # A stale one does not fail — it stops observing, and an expect-zero assertion # built on it then passes for the wrong reason. @@ -1322,6 +1334,7 @@ main() { run_trailing_log run_image_scoping run_action_pins + run_comment_refs run_wait_converge if [[ "$TEST_ONLY" == true ]]; then diff --git a/testing/interop/interop-test.sh b/testing/interop/interop-test.sh index ab27b3c1..83c1ac9f 100755 --- a/testing/interop/interop-test.sh +++ b/testing/interop/interop-test.sh @@ -949,7 +949,7 @@ echo "" # versions. A mixed-version bloom/tree-encoding divergence shows up as a # node that never produces an in-band estimate (or returns null). Strict # band = [0.75N, 1.25N]; polled up to MESH_SIZE_TIMEOUT (the estimate -# converges over minutes — see ISSUE-2026-0046 on its transient jitter). +# converges over minutes and is transiently jittery). echo "Phase 7: Mesh-size estimate convergence (strict ±25% of true N=$NUM_NODES)" PASSED=0; FAILED=0 ms_lo="$(awk -v n="$NUM_NODES" 'BEGIN{printf "%.2f", 0.75*n}')" diff --git a/testing/mesh-lab/compose-trace-nat.yml b/testing/mesh-lab/compose-trace-nat.yml index 4da307f8..70c6c700 100644 --- a/testing/mesh-lab/compose-trace-nat.yml +++ b/testing/mesh-lab/compose-trace-nat.yml @@ -1,6 +1,6 @@ # Compose override that bumps RUST_LOG to trace level on the modules # relevant to NAT-traversal handshake-completion flake evidence -# collection (ISSUE-2026-0027): +# collection: # # - fips::nostr — overlay advert publish/consume # (where the cross-init race begins) diff --git a/testing/static/scripts/rekey-test.sh b/testing/static/scripts/rekey-test.sh index bc78113b..4a4d39c2 100755 --- a/testing/static/scripts/rekey-test.sh +++ b/testing/static/scripts/rekey-test.sh @@ -176,7 +176,7 @@ RECONVERGE_STALL=10 TIMEOUT=5 CONVERGENCE_PING_TIMEOUT=1 # Strict-ping retry policy for the per-phase ping_all asserts. Under 1% -# i.i.d. packet loss (the lab condition surfaced by ISSUE-2026-0028) a +# i.i.d. packet loss (the lab condition this retry policy is sized for) a # single-shot ping fails at roughly 2% per directed pair, so a 20-pair # strict assert misses with probability ~1 - (0.98)^20 ≈ 33%, which is # below the per-pair loss-math floor but well above the routing-state