From 6537f68c51be26934bc70240529a6b9e94c34cdb Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 11 Aug 2026 15:26:03 +0000 Subject: [PATCH 1/5] Refuse symlinked key paths, enforce the key mode, and stop discarding write errors Every private key in the tree is written by one function, and it opened the path with create+truncate and no O_NOFOLLOW, so a symlink pre-planted at the key path was followed and its target overwritten. The mode was supplied only through open(2)'s mode argument, which the kernel applies when it creates the file and ignores otherwise, so a fips.key that already existed at 0644 stayed 0644 after every rewrite. That half needs no attacker: one chmod, or a restore that did not preserve modes, leaves the key readable forever. Both writers now go through a shared open helper that carries O_NOFOLLOW and, for the private key only, applies the mode to the open descriptor before any secret bytes are written. The public key keeps its create-time mode instead, because forcing it would reopen an operator-tightened fips.pub on every start. A refused open is classified by inspecting the path rather than the errno, since O_NOFOLLOW reports a symlinked final component differently across the Unix variants this module compiles for. Six write results that were discarded now report. The sharpest was in persistent mode: a failed key write fell through to an ephemeral identity with no message at all, so a node could silently change npub on every start while its config asked for a stable one. An ephemeral start over an existing key file now warns before it overwrites, naming the path and the setting that would have preserved the identity, which is the warning the key generation tool has always given and the daemon never did. Existence is tested with symlink_metadata rather than exists, because a dangling symlink reports false from the latter while still being a file the write acts on. The persistent read path warns when it finds a key file whose mode is looser than 0600 or which is a symlink. It does not repair either: the daemon does not own a file it did not create. Windows is a named coverage gap in both writers' docs. There is no mode to enforce and no O_NOFOLLOW; the file inherits the parent directory's ACLs. --- src/bin/fipsctl.rs | 10 +- src/config/mod.rs | 436 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 409 insertions(+), 37 deletions(-) diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index a32cb89..1554304 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -394,7 +394,9 @@ fn main() { let key_path = dir.join("fips.key"); let pub_path = dir.join("fips.pub"); - if key_path.exists() && !force { + // symlink_metadata rather than exists: a dangling symlink at the key + // path reports exists() == false and would slip past the guard. + if key_path.symlink_metadata().is_ok() && !force { eprintln!("error: key file already exists: {}", key_path.display()); eprintln!("Use --force to overwrite."); std::process::exit(1); @@ -410,9 +412,11 @@ fn main() { std::process::exit(1); } + // Non-fatal: the private key is already on disk by this point, so + // failing the whole run here would report failure for a keygen that + // did in fact produce the identity. if let Err(e) = write_pub_file(&pub_path, &npub) { - eprintln!("error: failed to write pub file: {e}"); - std::process::exit(1); + eprintln!("warning: failed to write pub file: {e}"); } eprintln!("{npub}"); diff --git a/src/config/mod.rs b/src/config/mod.rs index 6584da3..890fbe0 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -231,26 +231,128 @@ pub fn read_key_file(path: &Path) -> Result { Ok(nsec) } -/// Write a bare bech32 nsec to a key file with restricted permissions. +/// Open a key or public key file for writing, without following a symlink at +/// the path. /// -/// On Unix, the file is created with mode 0600 (owner read/write only). -/// On Windows, the file inherits default ACLs from the parent directory. -pub fn write_key_file(path: &Path, nsec: &str) -> Result<(), ConfigError> { - use std::io::Write; - +/// On Unix the open carries `O_NOFOLLOW`, so a symlink pre-planted at `path` +/// fails the open instead of having its target truncated. When `enforce_mode` +/// is set, `mode` is then applied to the open descriptor (`fchmod`) before the +/// caller writes anything, so a file that already existed at a looser mode is +/// tightened before any secret bytes reach it. The permission change is made +/// through the descriptor rather than `std::fs::set_permissions`, which +/// re-resolves the name and would reopen the window the `O_NOFOLLOW` closes. +/// +/// `O_NOFOLLOW` covers only the *final* path component. An attacker who can +/// replace an intermediate directory of the key path is unaffected by it. +/// +/// On Windows neither the mode handling nor the symlink protection applies; +/// the file inherits the parent directory's ACLs. +fn open_mode_enforced( + path: &Path, + mode: u32, + enforce_mode: bool, +) -> std::io::Result { let mut opts = std::fs::OpenOptions::new(); opts.write(true).create(true).truncate(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - opts.mode(0o600); + opts.mode(mode).custom_flags(libc::O_NOFOLLOW); } - let mut file = opts.open(path).map_err(|e| ConfigError::WriteKeyFile { + let file = opts.open(path)?; + + #[cfg(unix)] + if enforce_mode { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(std::fs::Permissions::from_mode(mode))?; + } + + #[cfg(not(unix))] + let _ = (mode, enforce_mode); + + Ok(file) +} + +/// Classify a failure to open a key or public key file for writing. +/// +/// A refused open is reported as [`ConfigError::KeyPathIsSymlink`] when the +/// path is in fact a symlink. The check is on the path rather than on the +/// errno because `O_NOFOLLOW` reports a final-component symlink as `ELOOP` on +/// Linux and macOS but `EMLINK` on FreeBSD and `EFTYPE` on NetBSD, and this +/// module's `cfg(unix)` is deliberately broader than Linux. +fn classify_open_error(path: &Path, source: std::io::Error) -> ConfigError { + if path + .symlink_metadata() + .map(|m| m.file_type().is_symlink()) + .unwrap_or(false) + { + return ConfigError::KeyPathIsSymlink { + path: path.to_path_buf(), + }; + } + ConfigError::WriteKeyFile { path: path.to_path_buf(), - source: e, - })?; + source, + } +} + +/// Warn about an existing identity key file the daemon will not rewrite. +/// +/// The persistent path reads an existing key and returns without writing it, +/// so a mode loosened by an operator `chmod` or by a restore that did not +/// preserve modes is otherwise never surfaced anywhere. Repairing the mode is +/// deliberately left to the operator; this only reports it. A symlinked key is +/// reported too, since the daemon does not manage the target's mode. +/// +/// Unix only: on Windows the file's ACLs are inherited from the parent +/// directory and there is no mode to inspect. +fn warn_unmanaged_key_file(path: &Path) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + let Ok(meta) = path.symlink_metadata() else { + return; + }; + + if meta.file_type().is_symlink() { + tracing::warn!( + path = %path.display(), + "Identity key file is a symlink; the daemon does not manage the mode of its target" + ); + return; + } + + if meta.is_file() && meta.permissions().mode() & 0o077 != 0 { + tracing::warn!( + path = %path.display(), + mode = format!("{:04o}", meta.permissions().mode() & 0o7777), + "Identity key file is accessible beyond its owner; expected mode 0600" + ); + } + } + + #[cfg(not(unix))] + let _ = path; +} + +/// Write a bare bech32 nsec to a key file with restricted permissions. +/// +/// On Unix, the file is opened with `O_NOFOLLOW` (a symlink at the path is +/// refused rather than followed) and forced to mode 0600 (owner read/write +/// only) before any key material is written, so an existing file at a looser +/// mode is corrected rather than inherited. +/// +/// Coverage gap: on Windows the file inherits default ACLs from the parent +/// directory, and neither the mode enforcement nor the symlink protection +/// applies. The exclusion is deliberate. +pub fn write_key_file(path: &Path, nsec: &str) -> Result<(), ConfigError> { + use std::io::Write; + + let mut file = + open_mode_enforced(path, 0o600, true).map_err(|e| classify_open_error(path, e))?; file.write_all(nsec.as_bytes()) .map_err(|e| ConfigError::WriteKeyFile { @@ -267,24 +369,20 @@ pub fn write_key_file(path: &Path, nsec: &str) -> Result<(), ConfigError> { /// Write a bare bech32 npub to a public key file. /// -/// On Unix, the file is created with mode 0644 (owner read/write, others read). -/// On Windows, the file inherits default ACLs from the parent directory. +/// On Unix, the file is opened with `O_NOFOLLOW` (a symlink at the path is +/// refused rather than followed) and created with mode 0644 (owner +/// read/write, others read). The mode is applied at creation only: this file +/// is rewritten on every persistent start, and forcing the mode would reopen +/// an operator-tightened `fips.pub` to world-readable each time. +/// +/// Coverage gap: on Windows the file inherits default ACLs from the parent +/// directory, and neither the mode handling nor the symlink protection +/// applies. The exclusion is deliberate. pub fn write_pub_file(path: &Path, npub: &str) -> Result<(), ConfigError> { use std::io::Write; - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create(true).truncate(true); - - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - opts.mode(0o644); - } - - let mut file = opts.open(path).map_err(|e| ConfigError::WriteKeyFile { - path: path.to_path_buf(), - source: e, - })?; + let mut file = + open_mode_enforced(path, 0o644, false).map_err(|e| classify_open_error(path, e))?; file.write_all(npub.as_bytes()) .map_err(|e| ConfigError::WriteKeyFile { @@ -346,7 +444,14 @@ pub fn resolve_identity( if key_path.exists() { let nsec = read_key_file(&key_path)?; let identity = Identity::from_secret_str(&nsec)?; - let _ = write_pub_file(&pub_path, &identity.npub()); + warn_unmanaged_key_file(&key_path); + if let Err(e) = write_pub_file(&pub_path, &identity.npub()) { + tracing::warn!( + path = %pub_path.display(), + error = %e, + "Failed to write the public key file" + ); + } return Ok(ResolvedIdentity { nsec, source: IdentitySource::KeyFile(key_path), @@ -370,7 +475,14 @@ pub fn resolve_identity( "Identity key found at the legacy path but not at the current default; \ using it so the node keeps its identity — move it to the current path" ); - let _ = write_pub_file(&pub_path, &identity.npub()); + warn_unmanaged_key_file(&legacy); + if let Err(e) = write_pub_file(&pub_path, &identity.npub()) { + tracing::warn!( + path = %pub_path.display(), + error = %e, + "Failed to write the public key file" + ); + } return Ok(ResolvedIdentity { nsec, source: IdentitySource::KeyFile(legacy), @@ -388,16 +500,30 @@ pub fn resolve_identity( match write_key_file(&key_path, &nsec) { Ok(()) => { - let _ = write_pub_file(&pub_path, &npub); + if let Err(e) = write_pub_file(&pub_path, &npub) { + tracing::warn!( + path = %pub_path.display(), + error = %e, + "Failed to write the public key file" + ); + } Ok(ResolvedIdentity { nsec, source: IdentitySource::Generated(key_path), }) } - Err(_) => Ok(ResolvedIdentity { - nsec, - source: IdentitySource::Ephemeral, - }), + Err(e) => { + tracing::warn!( + path = %key_path.display(), + error = %e, + "Failed to persist the generated identity key; this node is starting with an \ + ephemeral identity and its npub will change on every start" + ); + Ok(ResolvedIdentity { + nsec, + source: IdentitySource::Ephemeral, + }) + } } } else { // Ephemeral mode (default): fresh keypair every start, write key files @@ -410,8 +536,32 @@ pub fn resolve_identity( let _ = std::fs::create_dir_all(parent); } - let _ = write_key_file(&key_path, &nsec); - let _ = write_pub_file(&pub_path, &npub); + // symlink_metadata rather than exists: a dangling symlink at the key + // path reports exists() == false but is still an existing file the + // write is about to act on. + if key_path.symlink_metadata().is_ok() { + tracing::warn!( + path = %key_path.display(), + config_key = "node.identity.persistent", + "An existing key file at this path is being replaced by a fresh ephemeral \ + identity; set node.identity.persistent: true to keep the existing identity" + ); + } + + if let Err(e) = write_key_file(&key_path, &nsec) { + tracing::warn!( + path = %key_path.display(), + error = %e, + "Failed to write the ephemeral key file" + ); + } + if let Err(e) = write_pub_file(&pub_path, &npub) { + tracing::warn!( + path = %pub_path.display(), + error = %e, + "Failed to write the public key file" + ); + } Ok(ResolvedIdentity { nsec, @@ -464,6 +614,9 @@ pub enum ConfigError { source: std::io::Error, }, + #[error("refusing to write key file through a symlink: {path}")] + KeyPathIsSymlink { path: PathBuf }, + #[error("identity error: {0}")] Identity(#[from] IdentityError), @@ -1167,6 +1320,221 @@ node: assert_eq!(metadata.mode() & 0o777, 0o644); } + /// Collect formatted tracing events on the current thread. + /// + /// `resolve_identity` reports its identity-loss conditions only in the + /// log, so the log is what the tests have to assert on. Installed with + /// `tracing::subscriber::with_default`, which is thread-local, so parallel + /// tests do not see each other's events. + #[derive(Clone, Default)] + struct LogCapture(std::sync::Arc>>); + + impl LogCapture { + fn warnings(&self) -> Vec { + self.0 + .lock() + .unwrap() + .iter() + .filter(|line| line.starts_with("WARN")) + .cloned() + .collect() + } + } + + impl tracing_subscriber::Layer for LogCapture { + fn on_event( + &self, + event: &tracing::Event<'_>, + _ctx: tracing_subscriber::layer::Context<'_, S>, + ) { + struct Fields(String); + impl tracing::field::Visit for Fields { + fn record_debug( + &mut self, + field: &tracing::field::Field, + value: &dyn std::fmt::Debug, + ) { + self.0.push_str(&format!(" {}={:?}", field.name(), value)); + } + } + + let mut fields = Fields(event.metadata().level().to_string()); + event.record(&mut fields); + self.0.lock().unwrap().push(fields.0); + } + } + + fn capture_logs(f: impl FnOnce() -> T) -> (T, LogCapture) { + use tracing_subscriber::layer::SubscriberExt; + + let capture = LogCapture::default(); + let subscriber = tracing_subscriber::registry().with(capture.clone()); + let out = tracing::subscriber::with_default(subscriber, f); + (out, capture) + } + + #[cfg(unix)] + #[test] + fn test_write_key_file_refuses_symlink() { + let temp_dir = TempDir::new().unwrap(); + let victim = temp_dir.path().join("victim"); + let key_path = temp_dir.path().join("fips.key"); + + fs::write(&victim, "victim contents\n").unwrap(); + std::os::unix::fs::symlink(&victim, &key_path).unwrap(); + + let err = write_key_file(&key_path, "nsec1secret").unwrap_err(); + assert!(matches!(err, ConfigError::KeyPathIsSymlink { .. }), "{err}"); + + assert_eq!(fs::read_to_string(&victim).unwrap(), "victim contents\n"); + assert!( + key_path + .symlink_metadata() + .unwrap() + .file_type() + .is_symlink(), + "the symlink itself must be left in place, not replaced" + ); + } + + #[cfg(unix)] + #[test] + fn test_write_key_file_fixes_existing_mode() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let temp_dir = TempDir::new().unwrap(); + let key_path = temp_dir.path().join("fips.key"); + + fs::write(&key_path, "nsec1old\n").unwrap(); + fs::set_permissions(&key_path, fs::Permissions::from_mode(0o644)).unwrap(); + + write_key_file(&key_path, "nsec1new").unwrap(); + + assert_eq!(fs::metadata(&key_path).unwrap().mode() & 0o777, 0o600); + assert_eq!(read_key_file(&key_path).unwrap(), "nsec1new"); + } + + #[cfg(unix)] + #[test] + fn test_write_pub_file_refuses_symlink() { + let temp_dir = TempDir::new().unwrap(); + let victim = temp_dir.path().join("victim"); + let pub_path = temp_dir.path().join("fips.pub"); + + fs::write(&victim, "victim contents\n").unwrap(); + std::os::unix::fs::symlink(&victim, &pub_path).unwrap(); + + let err = write_pub_file(&pub_path, "npub1test").unwrap_err(); + assert!(matches!(err, ConfigError::KeyPathIsSymlink { .. }), "{err}"); + assert_eq!(fs::read_to_string(&victim).unwrap(), "victim contents\n"); + } + + #[test] + fn test_ephemeral_over_existing_key_warns() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + let identity = crate::Identity::generate(); + let existing = crate::encode_nsec(&identity.keypair().secret_key()); + write_key_file(&key_path, &existing).unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let (resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + assert_ne!(resolved.nsec, existing); + let warnings = logs.warnings(); + assert!( + warnings + .iter() + .any(|w| w.contains(&key_path.display().to_string()) + && w.contains("node.identity.persistent")), + "expected a warning naming the key path and the config key, got {warnings:?}" + ); + } + + #[cfg(unix)] + #[test] + fn test_ephemeral_dangling_symlink_detected() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + let target = temp_dir.path().join("absent-target"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + std::os::unix::fs::symlink(&target, &key_path).unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let (_resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + let warnings = logs.warnings(); + assert!( + warnings + .iter() + .any(|w| w.contains(&key_path.display().to_string())), + "expected a warning naming the key path, got {warnings:?}" + ); + assert!( + !target.exists(), + "the write must not have been followed through the dangling symlink" + ); + } + + #[cfg(unix)] + #[test] + fn test_persistent_permissive_key_warns() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + + fs::write(&config_path, "node:\n identity:\n persistent: true\n").unwrap(); + let identity = crate::Identity::generate(); + let nsec = crate::encode_nsec(&identity.keypair().secret_key()); + write_key_file(&key_path, &nsec).unwrap(); + fs::set_permissions(&key_path, fs::Permissions::from_mode(0o644)).unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let (resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + // The key is still read: this warns, it does not refuse or repair. + assert_eq!(resolved.nsec, nsec); + let warnings = logs.warnings(); + assert!( + warnings + .iter() + .any(|w| w.contains(&key_path.display().to_string()) && w.contains("0644")), + "expected a warning naming the key path and its mode, got {warnings:?}" + ); + } + + /// Healthy-path regression guard only. This passes against the pre-fix + /// code vacuously, because that code warns about nothing at all, so it is + /// not evidence that the fix works: it only catches a future change that + /// starts warning on an ordinary first ephemeral start. + #[test] + fn test_ephemeral_first_run_does_not_warn() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let (_resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + assert!( + logs.warnings().is_empty(), + "first ephemeral start must be silent, got {:?}", + logs.warnings() + ); + } + #[test] fn test_key_file_empty_error() { let temp_dir = TempDir::new().unwrap(); From 210debc5832aefabbdca3826b4a80a7452dc8839 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 11 Aug 2026 15:26:19 +0000 Subject: [PATCH 2/5] Validate gateway DNS answers before they become a NAT mapping The gateway's DNS forwarder accepted whatever datagram arrived on its upstream socket as the answer. It reused the client's own transaction ID in the upstream query, bound that socket to a wildcard address, received with a call that discards the sender, never compared the response ID or the question section against what it asked, and never checked that the returned address was inside the mesh prefix. The address it extracted is installed as a DNAT rule that carries no interface constraint, so a forged answer redirected traffic rather than merely poisoning a lookup. Four changes close it. The upstream query now carries a freshly drawn random transaction ID rather than the client's. The upstream socket is connected to the resolver before use, so the kernel drops datagrams from anyone else. A parsed response must be a response, carry the same ID, carry exactly one question matching the qname and qclass that were sent, and be for AAAA; anything else is discarded and the receive continues against the original deadline instead of accepting the first datagram to arrive. The extracted address goes through the validating address parser rather than a comment asserting the prefix byte, and a non-mesh answer is refused before any pool allocation, so no mapping event is emitted and no rule is installed. The validation sits before the rcode check, which changes one behaviour worth naming: an upstream that answers FORMERR or REFUSED with an empty question section no longer has that rcode relayed to the client and gets SERVFAIL instead. Checking after the rcode would let a forged NXDOMAIN through, so the placement is deliberate. Connecting the socket also fixes the dead-upstream half of the availability problem in the same loop, since a connected socket surfaces ECONNREFUSED immediately instead of stalling to the five second timeout. The serve loop still handles one query at a time; that half is untouched here. The tests drive real queries through a fake upstream: a foreign source injecting a well-formed answer, a wrong transaction ID, a wrong question, a non-mesh address, and the healthy path as an over-rejection guard. Each was checked by reverting the corresponding fix and confirming the intended test reds alone. --- src/gateway/dns.rs | 373 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 345 insertions(+), 28 deletions(-) diff --git a/src/gateway/dns.rs b/src/gateway/dns.rs index 7f840aa..cb71e36 100644 --- a/src/gateway/dns.rs +++ b/src/gateway/dns.rs @@ -9,7 +9,7 @@ use simple_dns::{CLASS, Packet, PacketFlag, RCODE, ResourceRecord, rdata}; -use simple_dns::{QTYPE, TYPE}; +use simple_dns::{QCLASS, QTYPE, TYPE}; use std::net::{Ipv6Addr, SocketAddr}; use tokio::net::UdpSocket; use tokio::sync::watch; @@ -57,16 +57,39 @@ fn extract_aaaa(packet: &Packet) -> Option { } /// Derive NodeAddr from a FIPS mesh address (fd00::/8). -/// The NodeAddr is bytes 1-15 of the IPv6 address prepended with the first byte. -fn node_addr_from_mesh(mesh_addr: Ipv6Addr) -> NodeAddr { - let bytes = mesh_addr.octets(); - // NodeAddr = first 16 bytes of SHA-256(pubkey), which maps to - // FipsAddress = fd + NodeAddr[1..16]. So NodeAddr[0] = bytes[1]. - // Actually, FipsAddress = [0xfd, nodeaddr[0..15]] - // So nodeaddr[0..15] = bytes[1..16] +/// Returns None unless the address carries the FIPS prefix. +fn node_addr_from_mesh(mesh_addr: Ipv6Addr) -> Option { + // FipsAddress = [0xfd, node_addr[0..15]], so node_addr[0..15] = bytes[1..16]. + let bytes = *crate::identity::FipsAddress::from_bytes(mesh_addr.octets()) + .ok()? + .as_bytes(); let mut node_bytes = [0u8; 16]; node_bytes[..15].copy_from_slice(&bytes[1..16]); - NodeAddr::from_bytes(node_bytes) + Some(NodeAddr::from_bytes(node_bytes)) +} + +/// Check that an upstream datagram answers the query we actually sent. +/// +/// Guards against off-path forgery: the transaction ID and question must +/// match, and the packet must be a response. Names are compared +/// case-insensitively because DNS names are case-insensitive on the wire +/// while `simple_dns` compares label bytes exactly. +fn upstream_response_matches( + response: &Packet, + upstream_id: u16, + upstream_qname: &str, + upstream_qclass: QCLASS, +) -> bool { + if !response.has_flags(PacketFlag::RESPONSE) || response.id() != upstream_id { + return false; + } + if response.questions.len() != 1 { + return false; + } + let question = &response.questions[0]; + question.qtype == QTYPE::TYPE(TYPE::AAAA) + && question.qclass == upstream_qclass + && question.qname.to_string().to_ascii_lowercase() == upstream_qname } /// Build a REFUSED DNS response. @@ -209,9 +232,16 @@ async fn handle_query( // Build an AAAA query for the daemon regardless of what the client asked // (A, AAAA, ANY, etc.). Mesh addresses are always IPv6, so the daemon // only returns useful answers for AAAA queries. + // The upstream transaction ID is drawn fresh so that an off-path forger + // cannot guess it from the client's query. Client-facing responses keep + // the client's own ID. + let upstream_id: u16 = rand::random(); + let question = query.questions.first()?; + let upstream_qname = question.qname.to_string().to_ascii_lowercase(); + let upstream_qclass = question.qclass; + let upstream_query_bytes = { - let question = query.questions.first()?; - let mut aaaa_query = Packet::new_query(query.id()); + let mut aaaa_query = Packet::new_query(upstream_id); let aaaa_question = simple_dns::Question::new( question.qname.clone(), QTYPE::TYPE(TYPE::AAAA), @@ -241,29 +271,50 @@ async fn handle_query( } }; - if let Err(e) = upstream_socket - .send_to(&upstream_query_bytes, upstream) - .await - { + // Connect the socket so the kernel drops datagrams from any source other + // than the configured upstream. + if let Err(e) = upstream_socket.connect(upstream).await { + warn!(error = %e, upstream = %upstream, "Failed to connect upstream socket"); + return build_servfail(&query); + } + + if let Err(e) = upstream_socket.send(&upstream_query_bytes).await { warn!(error = %e, "Failed to forward query to daemon"); return build_servfail(&query); } + // Keep reading until a datagram matches the query we sent, or the deadline + // passes. Datagrams that do not match are discarded rather than accepted. + let deadline = tokio::time::Instant::now() + UPSTREAM_TIMEOUT; let mut resp_buf = vec![0u8; MAX_DNS_SIZE]; - let resp_len = - match tokio::time::timeout(UPSTREAM_TIMEOUT, upstream_socket.recv(&mut resp_buf)).await { - Ok(Ok(len)) => len, - Ok(Err(e)) => { - warn!(error = %e, "Upstream recv error"); - return build_servfail(&query); + let upstream_response_bytes = loop { + let resp_len = + match tokio::time::timeout_at(deadline, upstream_socket.recv(&mut resp_buf)).await { + Ok(Ok(len)) => len, + Ok(Err(e)) => { + warn!(error = %e, upstream = %upstream, "Upstream recv error"); + return build_servfail(&query); + } + Err(_) => { + warn!(upstream = %upstream, "Upstream DNS timeout"); + return build_servfail(&query); + } + }; + + match Packet::parse(&resp_buf[..resp_len]) { + Ok(p) => { + if upstream_response_matches(&p, upstream_id, &upstream_qname, upstream_qclass) { + break resp_buf[..resp_len].to_vec(); + } + debug!(name = %fips_name, "Discarding unsolicited upstream datagram"); } Err(_) => { - warn!("Upstream DNS timeout"); - return build_servfail(&query); + debug!(name = %fips_name, "Discarding unparseable upstream datagram"); } - }; + } + }; - let upstream_response = match Packet::parse(&resp_buf[..resp_len]) { + let upstream_response = match Packet::parse(&upstream_response_bytes) { Ok(p) => p, Err(_) => return build_servfail(&query), }; @@ -292,8 +343,19 @@ async fn handle_query( } }; - // Derive NodeAddr from mesh address - let node_addr = node_addr_from_mesh(mesh_addr); + // Derive NodeAddr from mesh address. An answer outside fd00::/8 is not a + // mesh address and must never reach the NAT mapping path. + let node_addr = match node_addr_from_mesh(mesh_addr) { + Some(addr) => addr, + None => { + warn!( + name = %fips_name, + mesh_addr = %mesh_addr, + "Upstream AAAA is not a FIPS mesh address, rejecting" + ); + return build_servfail(&query); + } + }; // Allocate virtual IP from pool let mut pool_guard = pool.lock().await; @@ -347,11 +409,86 @@ async fn handle_query( mod tests { use super::*; + use simple_dns::{Name, Question}; + use tokio::sync::mpsc; + + const TEST_TTL: u32 = 60; + + /// Build a client-facing AAAA query. + fn build_query(id: u16, qname: &str) -> Vec { + let mut packet = Packet::new_query(id); + let question = Question::new( + Name::new_unchecked(qname), + QTYPE::TYPE(TYPE::AAAA), + CLASS::IN.into(), + false, + ); + packet.questions.push(question); + packet.build_bytes_vec_compressed().unwrap() + } + + /// Build an upstream NOERROR AAAA answer. + fn build_answer(id: u16, qname: &str, addr: &str) -> Vec { + let mut packet = Packet::new_reply(id); + packet.set_flags(PacketFlag::RESPONSE | PacketFlag::RECURSION_AVAILABLE); + let name = Name::new_unchecked(qname); + packet.questions.push(Question::new( + name.clone(), + QTYPE::TYPE(TYPE::AAAA), + CLASS::IN.into(), + false, + )); + let address: Ipv6Addr = addr.parse().unwrap(); + packet.answers.push(ResourceRecord::new( + name, + CLASS::IN, + TEST_TTL, + rdata::RData::AAAA(rdata::AAAA { + address: address.into(), + }), + )); + packet.build_bytes_vec_compressed().unwrap() + } + + /// A fake upstream that answers one query with a scripted list of + /// datagrams, in order, from its own socket. + fn spawn_upstream(socket: UdpSocket, replies: F) -> tokio::task::JoinHandle<()> + where + F: FnOnce(u16) -> Vec> + Send + 'static, + { + tokio::spawn(async move { + let mut buf = vec![0u8; MAX_DNS_SIZE]; + let (len, src) = socket.recv_from(&mut buf).await.unwrap(); + let observed_id = Packet::parse(&buf[..len]).unwrap().id(); + for reply in replies(observed_id) { + socket.send_to(&reply, src).await.unwrap(); + } + }) + } + + fn test_pool() -> std::sync::Arc> { + std::sync::Arc::new(tokio::sync::Mutex::new( + VirtualIpPool::new("fd01::/112", TEST_TTL as u64, 30).unwrap(), + )) + } + + /// Assert the response is an AAAA answer whose address came from the pool. + fn assert_pool_answer(response: &[u8]) -> Ipv6Addr { + let packet = Packet::parse(response).unwrap(); + assert_eq!(packet.rcode(), RCODE::NoError); + let addr = extract_aaaa(&packet).expect("expected an AAAA answer"); + assert!( + addr.octets()[0] == 0xfd && addr.octets()[1] == 0x01, + "expected a pool virtual IP, got {addr}" + ); + addr + } + #[test] fn test_node_addr_from_mesh() { // fd00::1 → node_addr bytes should be [0, 0, ..., 0, 1] in positions 0..15 let mesh: Ipv6Addr = "fd00::1".parse().unwrap(); - let node = node_addr_from_mesh(mesh); + let node = node_addr_from_mesh(mesh).unwrap(); let bytes = node.as_bytes(); // mesh = [0xfd, 0, 0, ..., 0, 1] // node = bytes[1..16] of mesh = [0, 0, ..., 0, 1] in first 15 bytes @@ -359,6 +496,186 @@ mod tests { assert_eq!(bytes[0], 0); } + #[test] + fn test_node_addr_from_mesh_rejects_non_mesh() { + let addr: Ipv6Addr = "2001:db8::1".parse().unwrap(); + assert!(node_addr_from_mesh(addr).is_none()); + } + + #[tokio::test] + async fn test_foreign_source_answer_not_accepted() { + let upstream_socket = UdpSocket::bind("[::1]:0").await.unwrap(); + let upstream = upstream_socket.local_addr().unwrap(); + let foreign = UdpSocket::bind("[::1]:0").await.unwrap(); + + // The fake upstream learns the gateway's ephemeral port from the query + // it receives, has a third socket forge an answer to that port, then + // sends the genuine answer itself. + let handle = tokio::spawn(async move { + let mut buf = vec![0u8; MAX_DNS_SIZE]; + let (len, src) = upstream_socket.recv_from(&mut buf).await.unwrap(); + let observed_id = Packet::parse(&buf[..len]).unwrap().id(); + let forged = build_answer(observed_id, "test.fips", "2001:db8::1"); + foreign.send_to(&forged, src).await.unwrap(); + let genuine = build_answer(observed_id, "test.fips", "fd00::1"); + upstream_socket.send_to(&genuine, src).await.unwrap(); + }); + + let pool = test_pool(); + let (event_tx, mut event_rx) = mpsc::channel(16); + let response = handle_query( + &build_query(0x1234, "test.fips"), + upstream, + TEST_TTL, + &pool, + &event_tx, + ) + .await + .unwrap(); + handle.await.unwrap(); + + assert_pool_answer(&response); + match event_rx.try_recv().unwrap() { + PoolEvent::MappingCreated { mesh_addr, .. } => { + assert_eq!(mesh_addr, "fd00::1".parse::().unwrap()); + } + other => panic!("unexpected event: {other:?}"), + } + assert!(matches!( + event_rx.try_recv(), + Err(mpsc::error::TryRecvError::Empty) + )); + } + + #[tokio::test] + async fn test_upstream_id_mismatch_discarded() { + let upstream_socket = UdpSocket::bind("[::1]:0").await.unwrap(); + let upstream = upstream_socket.local_addr().unwrap(); + let handle = spawn_upstream(upstream_socket, |id| { + vec![ + build_answer(id.wrapping_add(1), "test.fips", "2001:db8::1"), + build_answer(id, "test.fips", "fd00::1"), + ] + }); + + let pool = test_pool(); + let (event_tx, mut event_rx) = mpsc::channel(16); + let response = handle_query( + &build_query(0x1234, "test.fips"), + upstream, + TEST_TTL, + &pool, + &event_tx, + ) + .await + .unwrap(); + handle.await.unwrap(); + + assert_pool_answer(&response); + match event_rx.try_recv().unwrap() { + PoolEvent::MappingCreated { mesh_addr, .. } => { + assert_eq!(mesh_addr, "fd00::1".parse::().unwrap()); + } + other => panic!("unexpected event: {other:?}"), + } + } + + #[tokio::test] + async fn test_upstream_question_mismatch_discarded() { + let upstream_socket = UdpSocket::bind("[::1]:0").await.unwrap(); + let upstream = upstream_socket.local_addr().unwrap(); + let handle = spawn_upstream(upstream_socket, |id| { + vec![ + build_answer(id, "other.fips", "2001:db8::1"), + build_answer(id, "test.fips", "fd00::1"), + ] + }); + + let pool = test_pool(); + let (event_tx, mut event_rx) = mpsc::channel(16); + let response = handle_query( + &build_query(0x1234, "test.fips"), + upstream, + TEST_TTL, + &pool, + &event_tx, + ) + .await + .unwrap(); + handle.await.unwrap(); + + assert_pool_answer(&response); + match event_rx.try_recv().unwrap() { + PoolEvent::MappingCreated { mesh_addr, .. } => { + assert_eq!(mesh_addr, "fd00::1".parse::().unwrap()); + } + other => panic!("unexpected event: {other:?}"), + } + } + + #[tokio::test] + async fn test_non_mesh_aaaa_rejected() { + let upstream_socket = UdpSocket::bind("[::1]:0").await.unwrap(); + let upstream = upstream_socket.local_addr().unwrap(); + let handle = spawn_upstream(upstream_socket, |id| { + vec![build_answer(id, "test.fips", "2001:db8::1")] + }); + + let pool = test_pool(); + let (event_tx, mut event_rx) = mpsc::channel(16); + let response = handle_query( + &build_query(0x1234, "test.fips"), + upstream, + TEST_TTL, + &pool, + &event_tx, + ) + .await + .unwrap(); + handle.await.unwrap(); + + let packet = Packet::parse(&response).unwrap(); + assert_eq!(packet.rcode(), RCODE::ServerFailure); + assert!(matches!( + event_rx.try_recv(), + Err(mpsc::error::TryRecvError::Empty) + )); + } + + #[tokio::test] + async fn test_healthy_path_resolves() { + let upstream_socket = UdpSocket::bind("[::1]:0").await.unwrap(); + let upstream = upstream_socket.local_addr().unwrap(); + let handle = spawn_upstream(upstream_socket, |id| { + vec![build_answer(id, "test.fips", "fd00::1")] + }); + + let pool = test_pool(); + let (event_tx, mut event_rx) = mpsc::channel(16); + let response = handle_query( + &build_query(0x1234, "test.fips"), + upstream, + TEST_TTL, + &pool, + &event_tx, + ) + .await + .unwrap(); + handle.await.unwrap(); + + assert_pool_answer(&response); + match event_rx.try_recv().unwrap() { + PoolEvent::MappingCreated { mesh_addr, .. } => { + assert_eq!(mesh_addr, "fd00::1".parse::().unwrap()); + } + other => panic!("unexpected event: {other:?}"), + } + assert!(matches!( + event_rx.try_recv(), + Err(mpsc::error::TryRecvError::Empty) + )); + } + #[test] fn test_extract_fips_name() { // Build a simple AAAA query for test.fips From 9f82c4726efac185364bfc086185f29a0f44c516 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 11 Aug 2026 15:26:34 +0000 Subject: [PATCH 3/5] Bound the first inbound frame, and let the reaper close the socket it forgets An accepted TCP socket took an inbound slot before a single byte was read. The cap is tested at accept, the pool insert and the counter bump follow with no read in between, and the frame reader's two read_exact calls carry no deadline. So an unauthenticated remote held a slot by connecting and sending nothing, and since pool keys are ip:port, N sockets from one address are N slots rather than one. At the 256 default that locks out inbound peering for as long as the attacker keeps the sockets open. The first frame on an inbound connection now has a deadline. It is a module constant rather than a config key: this branch takes no new operator-facing surface, and a knob is not needed to fix a missing bound. The onion listener has the identical accept-then-count ordering and gets the same treatment; it was not in the original report. The second half is that nothing reclaimed a slot once taken. The node-layer handshake reaper tore down session state but never closed the transport connection, so a peer that sent a real msg1 and then stalled was forgotten by the node while its socket, its pool entry and its slot lived on. The reaper now closes the transport connection too. Closing twice is safe: every close_connection implementation guards on removing the entry from its pool, and the connectionless ones are no-ops, so the handshake paths that already close and then drop a link are undisturbed. What this does not close, and it should be said plainly rather than discovered later: the deadline covers the first frame only. A peer that sends one well-formed frame and then goes silent still holds its slot, and so does one that completes msg1 and stalls beyond the reaper's reach. Closing those needs a rolling idle deadline, which interacts with heartbeats being per peer rather than per link and is a larger decision than this change. --- src/node/handlers/rx_loop.rs | 2 +- src/node/handlers/timeout.rs | 34 ++- src/node/lifecycle.rs | 2 +- src/node/tests/handshake.rs | 4 +- src/transport/tcp/mod.rs | 488 +++++++++++++++++++++++++++++++++-- src/transport/tcp/stream.rs | 58 +++-- src/transport/tor/mod.rs | 216 ++++++++++++++-- 7 files changed, 722 insertions(+), 82 deletions(-) diff --git a/src/node/handlers/rx_loop.rs b/src/node/handlers/rx_loop.rs index 525550f..518d852 100644 --- a/src/node/handlers/rx_loop.rs +++ b/src/node/handlers/rx_loop.rs @@ -250,7 +250,7 @@ impl Node { let _ = response_tx.send(response); } _ = tick.tick() => { - self.check_timeouts(); + self.check_timeouts().await; let now_ms = Self::now_ms(); self.reload_peer_acl().await; // The host map hot-reloads on the same tick as the ACL. It diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index e59c4e8..27437cf 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -11,7 +11,7 @@ impl Node { /// /// Called periodically by the RX event loop. Removes connections that have /// been idle longer than the configured handshake timeout or are in Failed state. - pub(in crate::node) fn check_timeouts(&mut self) { + pub(in crate::node) async fn check_timeouts(&mut self) { if self.connections.is_empty() { return; } @@ -53,16 +53,21 @@ impl Node { self.schedule_retry(*identity.node_addr(), now_ms); } } - self.cleanup_stale_connection(link_id, now_ms); + self.cleanup_stale_connection(link_id, now_ms).await; } } /// Remove a handshake connection and all associated state. /// - /// Frees the session index, removes pending_outbound entry, and cleans up - /// the link and address mapping. Does not log — callers provide context-appropriate - /// log messages. - pub(in crate::node) fn cleanup_stale_connection(&mut self, link_id: LinkId, _now_ms: u64) { + /// Frees the session index, removes pending_outbound entry, closes the + /// underlying transport connection, and cleans up the link and address + /// mapping. Does not log — callers provide context-appropriate log + /// messages. + pub(in crate::node) async fn cleanup_stale_connection( + &mut self, + link_id: LinkId, + _now_ms: u64, + ) { let conn = match self.connections.remove(&link_id) { Some(c) => c, None => return, @@ -77,6 +82,23 @@ impl Node { let _ = self.index_allocator.free(idx); } + // Tear down the transport connection, not just the node-side state. + // A connection-oriented transport otherwise keeps the socket, its + // pool entry and its inbound-slot accounting alive after the node + // has forgotten the handshake that socket belonged to, so a peer + // that sends msg1 and then stalls holds an inbound slot forever. + // Closing twice is harmless: every `close_connection` implementation + // is `if let Some(conn) = pool.remove(addr)` and the connectionless + // default is a no-op, so the handshake paths that already close and + // then drop a link cannot be disturbed by this. + if let Some(link) = self.links.get(&link_id) { + let tid = link.transport_id(); + let addr = link.remote_addr().clone(); + if let Some(transport) = self.transports.get(&tid) { + transport.close_connection(&addr).await; + } + } + // Remove link and addr_to_link self.remove_link(&link_id); if let Some(transport_id) = transport_id { diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index a4d44b2..e252397 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -754,7 +754,7 @@ impl Node { .map(|(link_id, _)| *link_id) .collect(); for link_id in stale { - self.cleanup_stale_connection(link_id, now_ms); + self.cleanup_stale_connection(link_id, now_ms).await; } } } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 822eb3d..996bdc2 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -700,7 +700,7 @@ async fn test_stale_connection_cleanup() { // Connection was created at time 1000ms. check_timeouts uses SystemTime::now(), // which is far beyond the 30s timeout. The connection should be cleaned up. - node.check_timeouts(); + node.check_timeouts().await; // Verify everything was cleaned up assert_eq!( @@ -770,7 +770,7 @@ async fn test_failed_connection_cleanup() { assert_eq!(node.connection_count(), 1); // Failed connections should be cleaned up immediately regardless of age - node.check_timeouts(); + node.check_timeouts().await; assert_eq!( node.connection_count(), diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index 350c879..beaadb8 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -126,6 +126,9 @@ pub struct TcpTransport { /// fallback when this transport has no explicit `max_inbound_connections`. /// `None` means "not provided" — fall through to the built-in default. node_max_connections: Option, + /// Deadline from accept to the first complete inbound frame. Defaults to + /// `INBOUND_FIRST_FRAME_TIMEOUT`; overridable only from tests. + first_frame_timeout: Duration, /// Transport statistics. stats: Arc, } @@ -149,10 +152,22 @@ impl TcpTransport { accept_task: None, local_addr: None, node_max_connections: None, + first_frame_timeout: INBOUND_FIRST_FRAME_TIMEOUT, stats: Arc::new(TcpStats::new()), } } + /// Override the accept-to-first-frame deadline. + /// + /// Test-only: the accept loop is reachable from the test module only + /// through `start_async()`, which reads this field when it builds the + /// `AcceptConfig`, so there is no other way to drive the deadline at a + /// duration a unit test can wait for. + #[cfg(test)] + pub(crate) fn set_first_frame_timeout(&mut self, d: Duration) { + self.first_frame_timeout = d; + } + /// Set the node-wide `node.limits.max_connections` value. /// /// Used as the inbound-cap fallback when this transport instance has no @@ -241,6 +256,7 @@ impl TcpTransport { keepalive_secs: self.config.keepalive_secs(), recv_buf: self.config.recv_buf_size(), send_buf: self.config.send_buf_size(), + first_frame_timeout: self.first_frame_timeout, }; let accept_task = tokio::spawn(async move { @@ -459,6 +475,10 @@ impl TcpTransport { mtu, recv_stats, Direction::Outbound, + // Outbound connections hold no inbound slot and are not + // gated on an accept-loop insert. + None, + None, ) .await; }); @@ -708,6 +728,10 @@ impl TcpTransport { mss_mtu, recv_stats, Direction::Outbound, + // Outbound connections hold no inbound slot and are not + // gated on an accept-loop insert. + None, + None, ) .await; }); @@ -801,6 +825,20 @@ impl Transport for TcpTransport { // Accept Loop // ============================================================================ +/// Deadline from accept to the first complete inbound FMP frame. +/// +/// An accepted socket takes an inbound pool slot before any byte is read, +/// so without a deadline a remote that connects and stays silent holds +/// that slot for as long as it keeps the socket open. The node-layer +/// reaper cannot see such a socket: no frame means no link and no node +/// state to time out. The value matches the node-layer handshake reaper +/// (`handshake_timeout_secs`, `src/config/node.rs:101`), so a peer that +/// misses this deadline would have been reaped node-side anyway. +/// +/// Deliberately not a config key: `maint` takes no new operator-facing +/// TOML surface. +pub(crate) const INBOUND_FIRST_FRAME_TIMEOUT: Duration = Duration::from_secs(30); + /// Socket configuration parameters passed to the accept loop. struct AcceptConfig { mtu: u16, @@ -809,6 +847,7 @@ struct AcceptConfig { keepalive_secs: u64, recv_buf: usize, send_buf: usize, + first_frame_timeout: Duration, } /// TCP accept loop — runs as a spawned task when bind_addr is configured. @@ -828,6 +867,7 @@ async fn accept_loop( keepalive_secs, recv_buf, send_buf, + first_frame_timeout, } = cfg; debug!(transport_id = %transport_id, "TCP accept loop starting"); @@ -904,6 +944,12 @@ async fn accept_loop( let recv_stats = stats.clone(); let recv_addr = remote_addr.clone(); + // Readiness barrier: the receive task must not reach its + // cleanup path before the pool insert and counter bump below, + // or it would remove nothing and leave an orphaned entry with + // a permanently incremented inbound counter. + let (ready_tx, ready_rx) = tokio::sync::oneshot::channel(); + let recv_task = tokio::spawn(async move { tcp_receive_loop( read_half, @@ -914,6 +960,8 @@ async fn accept_loop( conn_mtu, recv_stats, Direction::Inbound, + Some(first_frame_timeout), + Some(ready_rx), ) .await; }); @@ -928,10 +976,15 @@ async fn accept_loop( let mut pool_guard = pool.lock().await; pool_guard.insert(remote_addr.clone(), conn); + drop(pool_guard); stats.record_connection_accepted(); stats.record_pool_inbound_added(); + // Release the receive task now that both the pool entry and + // the inbound counter are in place. + let _ = ready_tx.send(()); + debug!( transport_id = %transport_id, remote_addr = %remote_addr, @@ -962,6 +1015,12 @@ async fn accept_loop( /// the cleanup path can decrement the correct `pool_inbound` / /// `pool_outbound` counter regardless of whether the matching pool /// entry survived to be removed. +/// +/// `first_frame_timeout` bounds the wait for the *first* complete frame +/// only, and is `Some` for inbound connections (which hold a capped pool +/// slot from accept) and `None` for outbound ones. `ready_rx`, when +/// present, is the accept loop's readiness barrier: the loop must not run +/// its cleanup before the accept loop has inserted the pool entry. #[allow(clippy::too_many_arguments)] async fn tcp_receive_loop( mut reader: tokio::net::tcp::OwnedReadHalf, @@ -972,6 +1031,8 @@ async fn tcp_receive_loop( mtu: u16, stats: Arc, direction: Direction, + first_frame_timeout: Option, + ready_rx: Option>, ) { debug!( transport_id = %transport_id, @@ -979,39 +1040,74 @@ async fn tcp_receive_loop( "TCP receive loop starting" ); - loop { - match read_fmp_packet(&mut reader, mtu).await { - Ok(data) => { - stats.record_recv(data.len()); + // An `Err` here means the accept loop went away between the insert and + // the signal. Fall through to the cleanup below rather than returning, + // so a pooled entry cannot be stranded with the counter incremented. + let admitted = match ready_rx { + Some(rx) => rx.await.is_ok(), + None => true, + }; - trace!( - transport_id = %transport_id, - remote_addr = %remote_addr, - bytes = data.len(), - "TCP packet received" - ); + if admitted { + let mut first = true; + loop { + let read = match first_frame_timeout { + // Bound the first read only. A silent remote otherwise holds + // its inbound slot for as long as it keeps the socket open. + Some(d) if first => { + match tokio::time::timeout(d, read_fmp_packet(&mut reader, mtu)).await { + Ok(result) => result, + Err(_) => { + // Not a recv error: `record_recv_error` means + // framing or I/O failure, and folding deadline + // expiries into it corrupts that counter. + debug!( + transport_id = %transport_id, + remote_addr = %remote_addr, + timeout_secs = d.as_secs_f64(), + "No complete frame within the first-frame deadline, dropping inbound connection" + ); + break; + } + } + } + _ => read_fmp_packet(&mut reader, mtu).await, + }; + first = false; - let packet = ReceivedPacket::new(transport_id, remote_addr.clone(), data); + match read { + Ok(data) => { + stats.record_recv(data.len()); - if packet_tx.send(packet).await.is_err() { + trace!( + transport_id = %transport_id, + remote_addr = %remote_addr, + bytes = data.len(), + "TCP packet received" + ); + + let packet = ReceivedPacket::new(transport_id, remote_addr.clone(), data); + + if packet_tx.send(packet).await.is_err() { + debug!( + transport_id = %transport_id, + "Packet channel closed, stopping TCP receive loop" + ); + break; + } + } + Err(e) => { + stats.record_recv_error(); + // EOF or protocol error — remove connection from pool debug!( transport_id = %transport_id, - "Packet channel closed, stopping TCP receive loop" + remote_addr = %remote_addr, + error = %e, + "TCP receive error, removing connection" ); break; } } - Err(e) => { - stats.record_recv_error(); - // EOF or protocol error — remove connection from pool - debug!( - transport_id = %transport_id, - remote_addr = %remote_addr, - error = %e, - "TCP receive error, removing connection" - ); - break; - } } } @@ -1146,10 +1242,34 @@ fn read_mss_mtu(stream: &std::net::TcpStream, default_mtu: u16) -> u16 { #[cfg(test)] mod tests { + use super::stream::build_msg1_frame; use super::*; use crate::transport::packet_channel; use tokio::time::{Duration, timeout}; + /// Poll `f` every 10ms until it holds or `limit` elapses. + async fn wait_until bool>(mut f: F, limit: Duration) -> bool { + let deadline = Instant::now() + limit; + loop { + if f() { + return true; + } + if Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + fn capped_config(max_inbound: usize) -> TcpConfig { + TcpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + mtu: Some(1400), + max_inbound_connections: Some(max_inbound), + ..Default::default() + } + } + fn make_config() -> TcpConfig { TcpConfig { bind_addr: Some("127.0.0.1:0".to_string()), @@ -1752,4 +1872,324 @@ mod tests { t1.stop_async().await.unwrap(); t2.stop_async().await.unwrap(); } + + // ======================================================================== + // Inbound first-frame deadline + // ======================================================================== + + /// A socket that connects and sends nothing must have its inbound slot + /// released by the first-frame deadline. + /// + /// Break-check: with the `tokio::time::timeout` wrapper removed from the + /// first read, the socket parks on an unbounded `read_exact` and the + /// count stays at 1 for as long as the peer keeps the socket open, so + /// the second assertion fails. + #[tokio::test] + async fn idle_inbound_socket_releases_its_slot() { + let (tx, _rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_millis(200)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + // Connect and say nothing. Held open for the whole test so that any + // slot release is the deadline's doing and not a client disconnect. + let squatter = TcpStream::connect(listen).await.unwrap(); + + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 1, + Duration::from_secs(2) + ) + .await, + "an accepted socket should take an inbound slot" + ); + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(2) + ) + .await, + "a silent inbound socket should lose its slot at the first-frame deadline" + ); + assert!( + transport.pool.lock().await.is_empty(), + "the pool entry should go with the slot" + ); + + drop(squatter); + transport.stop_async().await.unwrap(); + } + + /// With the cap filled by a silent socket, a genuine peer is refused + /// until the deadline frees the slot, and admitted afterwards. + /// + /// Break-check: without the deadline the squatter never releases, so the + /// genuine peer's frame is never delivered and the final receive times + /// out. + #[tokio::test] + async fn inbound_cap_recovers_after_first_frame_deadline() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, capped_config(1), tx); + transport.set_first_frame_timeout(Duration::from_millis(300)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let squatter = TcpStream::connect(listen).await.unwrap(); + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 1, + Duration::from_secs(2) + ) + .await, + "the squatter should fill the cap of one" + ); + + // While the cap is full a genuine peer is rejected outright. + let mut early = TcpStream::connect(listen).await.unwrap(); + let _ = early.write_all(&build_msg1_frame()).await; + assert!( + timeout(Duration::from_millis(200), rx.recv()) + .await + .is_err(), + "a peer arriving while the cap is full must not be admitted" + ); + drop(early); + + // The deadline frees the slot without the squatter disconnecting. + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(2) + ) + .await, + "the deadline should free the slot the squatter took" + ); + + let mut genuine = TcpStream::connect(listen).await.unwrap(); + genuine.write_all(&build_msg1_frame()).await.unwrap(); + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the genuine peer's frame") + .expect("packet channel closed"); + assert_eq!(packet.data, build_msg1_frame()); + + drop(squatter); + drop(genuine); + transport.stop_async().await.unwrap(); + } + + /// Regression guard, not evidence that the fix works. + /// + /// The deadline is scoped to the first iteration, so an established + /// connection that then goes quiet cannot be dropped by it: this test + /// passes by construction under the current design. It is kept so that a + /// future general (every-read) idle deadline cannot silently start + /// reaping quiet links without a test going red. + #[tokio::test] + async fn established_connection_survives_long_idle() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_millis(200)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + peer.write_all(&build_msg1_frame()).await.unwrap(); + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout") + .expect("packet channel closed"); + assert_eq!(packet.data, build_msg1_frame()); + + // Four deadlines' worth of silence after the first frame. + tokio::time::sleep(Duration::from_millis(800)).await; + + assert_eq!( + transport.stats().pool_inbound_count(), + 1, + "an established connection must not be dropped by the first-frame deadline" + ); + assert!(!transport.pool.lock().await.is_empty()); + + drop(peer); + transport.stop_async().await.unwrap(); + } + + /// A genuine peer that is slow to start, but finishes its first frame + /// inside the deadline, is admitted. + #[tokio::test] + async fn slow_first_frame_within_deadline_is_admitted() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_secs(1)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + peer.write_all(&build_msg1_frame()).await.unwrap(); + + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout") + .expect("packet channel closed"); + assert_eq!(packet.data, build_msg1_frame()); + assert_eq!(transport.stats().pool_inbound_count(), 1); + + drop(peer); + transport.stop_async().await.unwrap(); + } + + /// The honest-slow-peer case the wrapper actually kills: a first frame + /// that *starts* inside the deadline but completes after it. The + /// deadline covers the whole frame, not its first byte, so the drip is + /// dropped and its slot released. + #[tokio::test] + async fn byte_dripped_first_frame_past_deadline_is_dropped() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_millis(300)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let frame = build_msg1_frame(); + let mut peer = TcpStream::connect(listen).await.unwrap(); + // Prefix inside the deadline, remainder well past it. + peer.write_all(&frame[..4]).await.unwrap(); + tokio::time::sleep(Duration::from_millis(600)).await; + let _ = peer.write_all(&frame[4..]).await; + + assert!( + timeout(Duration::from_millis(500), rx.recv()) + .await + .is_err(), + "a first frame completing after the deadline must not be delivered" + ); + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(2) + ) + .await, + "the dripped connection should have released its slot" + ); + + drop(peer); + transport.stop_async().await.unwrap(); + } + + /// Break-check for the readiness barrier's error path. + /// + /// Stands in for an accept loop aborted between the pool insert and the + /// `ready_tx.send()`: the sender is dropped, so `ready_rx.await` returns + /// `Err`. The receive loop must still fall through to its cleanup, or + /// the pooled entry and its inbound-counter increment are stranded with + /// no task left to undo them. A bare `return` on the error path fails + /// both assertions below. + #[tokio::test] + async fn receive_loop_cleans_up_when_readiness_signal_is_dropped() { + let (tx, _rx) = packet_channel(10); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let client = TcpStream::connect(listen).await.unwrap(); + let (server, peer_addr) = listener.accept().await.unwrap(); + let remote = TransportAddr::from_string(&peer_addr.to_string()); + let (read_half, write_half) = server.into_split(); + + let pool: ConnectionPool = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(TcpStats::new()); + pool.lock().await.insert( + remote.clone(), + TcpConnection { + writer: Arc::new(Mutex::new(write_half)), + recv_task: tokio::spawn(async {}), + mtu: 1400, + established_at: Instant::now(), + direction: Direction::Inbound, + }, + ); + stats.record_pool_inbound_added(); + assert_eq!(stats.pool_inbound_count(), 1); + + let (ready_tx, ready_rx) = tokio::sync::oneshot::channel::<()>(); + drop(ready_tx); + + tcp_receive_loop( + read_half, + TransportId::new(1), + remote.clone(), + tx, + pool.clone(), + 1400, + stats.clone(), + Direction::Inbound, + Some(Duration::from_millis(50)), + Some(ready_rx), + ) + .await; + + assert!( + pool.lock().await.is_empty(), + "an aborted accept must not strand a pool entry" + ); + assert_eq!( + stats.pool_inbound_count(), + 0, + "an aborted accept must not strand an inbound-counter increment" + ); + drop(client); + } + + /// Invariant guard: a deadline that expires immediately still leaves no + /// orphaned pool entry or counter increment behind. + /// + /// This is not a break-check for the readiness barrier. On the + /// current-thread test runtime the accept loop queues for the pool lock + /// before the spawned receive task can run at all, so the insert wins + /// the race with or without the barrier. The barrier's error path is + /// break-checked in `receive_loop_cleans_up_when_readiness_signal_is_dropped`. + #[tokio::test] + async fn zero_deadline_leaves_no_orphaned_pool_entry() { + let (tx, _rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::ZERO); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + // Hold the pool across the accept so the receive task cannot reach + // its cleanup while the accept loop is mid-insert. + let guard = transport.pool.lock().await; + let client = TcpStream::connect(listen).await.unwrap(); + tokio::time::sleep(Duration::from_millis(100)).await; + drop(guard); + + // Sequence the checks off `connections_accepted`, which the accept + // loop bumps only after its insert. Reading the pool counter first + // would otherwise observe the pre-accept zero and prove nothing. + assert!( + wait_until( + || transport.stats().snapshot().connections_accepted == 1, + Duration::from_secs(2) + ) + .await, + "the accept loop should have admitted the connection" + ); + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0 + && transport + .pool + .try_lock() + .map(|p| p.is_empty()) + .unwrap_or(false), + Duration::from_secs(2) + ) + .await, + "an immediately expired deadline should leave neither a pool entry nor a counter increment" + ); + + drop(client); + transport.stop_async().await.unwrap(); + } } diff --git a/src/transport/tcp/stream.rs b/src/transport/tcp/stream.rs index 4323fdc..4d5e03c 100644 --- a/src/transport/tcp/stream.rs +++ b/src/transport/tcp/stream.rs @@ -178,6 +178,39 @@ pub async fn read_fmp_packet( Ok(packet) } +// ============================================================================ +// Test Frame Builders +// ============================================================================ + +/// Build a minimal established frame with the given payload_len. +/// Layout: [ver+phase:1][flags:1][payload_len:2 LE][12 bytes header][payload_len bytes][16 bytes tag] +/// +/// Lives at module scope so the transport modules that share this reader +/// (tcp, tor) can build wire-shaped frames in their own tests. +#[cfg(test)] +pub(crate) fn build_established_frame(payload_len: u16) -> Vec { + let total = PREFIX_SIZE + ESTABLISHED_REMAINING_HEADER + payload_len as usize + AEAD_TAG_SIZE; + let mut frame = vec![0u8; total]; + frame[0] = 0x00; // ver=0, phase=0 (established) + frame[1] = 0x00; // flags + frame[2..4].copy_from_slice(&payload_len.to_le_bytes()); + // Fill remaining with pattern for verification + for (i, byte) in frame[PREFIX_SIZE..total].iter_mut().enumerate() { + *byte = ((PREFIX_SIZE + i) & 0xFF) as u8; + } + frame +} + +/// Build a msg1 frame (114 bytes total). +#[cfg(test)] +pub(crate) fn build_msg1_frame() -> Vec { + let mut frame = vec![0xAA; MSG1_WIRE_SIZE]; + frame[0] = 0x01; // ver=0, phase=1 + frame[1] = 0x00; // flags + frame[2..4].copy_from_slice(&MSG1_PAYLOAD_LEN.to_le_bytes()); + frame +} + // ============================================================================ // Tests // ============================================================================ @@ -187,31 +220,6 @@ mod tests { use super::*; use std::io::Cursor; - /// Build a minimal established frame with the given payload_len. - /// Layout: [ver+phase:1][flags:1][payload_len:2 LE][12 bytes header][payload_len bytes][16 bytes tag] - fn build_established_frame(payload_len: u16) -> Vec { - let total = - PREFIX_SIZE + ESTABLISHED_REMAINING_HEADER + payload_len as usize + AEAD_TAG_SIZE; - let mut frame = vec![0u8; total]; - frame[0] = 0x00; // ver=0, phase=0 (established) - frame[1] = 0x00; // flags - frame[2..4].copy_from_slice(&payload_len.to_le_bytes()); - // Fill remaining with pattern for verification - for (i, byte) in frame[PREFIX_SIZE..total].iter_mut().enumerate() { - *byte = ((PREFIX_SIZE + i) & 0xFF) as u8; - } - frame - } - - /// Build a msg1 frame (114 bytes total). - fn build_msg1_frame() -> Vec { - let mut frame = vec![0xAA; MSG1_WIRE_SIZE]; - frame[0] = 0x01; // ver=0, phase=1 - frame[1] = 0x00; // flags - frame[2..4].copy_from_slice(&MSG1_PAYLOAD_LEN.to_le_bytes()); - frame - } - /// Build a msg2 frame (69 bytes total). fn build_msg2_frame() -> Vec { let mut frame = vec![0xBB; MSG2_WIRE_SIZE]; diff --git a/src/transport/tor/mod.rs b/src/transport/tor/mod.rs index 5c2b8a9..43527a7 100644 --- a/src/transport/tor/mod.rs +++ b/src/transport/tor/mod.rs @@ -31,6 +31,7 @@ use super::{ TransportError, TransportId, TransportState, TransportType, }; use crate::config::TorConfig; +use crate::transport::tcp::INBOUND_FIRST_FRAME_TIMEOUT; use crate::transport::tcp::stream::read_fmp_packet; use control::{ControlAuth, TorControlClient, TorMonitoringInfo}; use stats::TorStats; @@ -408,6 +409,7 @@ impl TorTransport { pool, mtu, max_inbound, + INBOUND_FIRST_FRAME_TIMEOUT, stats, ) .await; @@ -835,6 +837,10 @@ impl TorTransport { mtu, recv_stats, Direction::Outbound, + // Outbound connections hold no inbound slot and are not + // gated on an accept-loop insert. + None, + None, ) .await; }); @@ -1067,6 +1073,10 @@ impl TorTransport { mtu, recv_stats, Direction::Outbound, + // Outbound connections hold no inbound slot and are not + // gated on an accept-loop insert. + None, + None, ) .await; }); @@ -1178,6 +1188,12 @@ impl Transport for TorTransport { /// connection from the pool and exits. `direction` is captured so the /// cleanup path can decrement the correct `pool_inbound` / /// `pool_outbound` counter. +/// +/// `first_frame_timeout` bounds the wait for the *first* complete frame +/// only, and is `Some` for inbound connections (which hold a capped pool +/// slot from accept) and `None` for outbound ones. `ready_rx`, when +/// present, is the accept loop's readiness barrier: the loop must not run +/// its cleanup before the accept loop has inserted the pool entry. #[allow(clippy::too_many_arguments)] async fn tor_receive_loop( mut reader: tokio::net::tcp::OwnedReadHalf, @@ -1188,6 +1204,8 @@ async fn tor_receive_loop( mtu: u16, stats: Arc, direction: Direction, + first_frame_timeout: Option, + ready_rx: Option>, ) { debug!( transport_id = %transport_id, @@ -1195,38 +1213,73 @@ async fn tor_receive_loop( "Tor receive loop starting" ); - loop { - match read_fmp_packet(&mut reader, mtu).await { - Ok(data) => { - stats.record_recv(data.len()); + // An `Err` here means the accept loop went away between the insert and + // the signal. Fall through to the cleanup below rather than returning, + // so a pooled entry cannot be stranded with the counter incremented. + let admitted = match ready_rx { + Some(rx) => rx.await.is_ok(), + None => true, + }; - trace!( - transport_id = %transport_id, - remote_addr = %remote_addr, - bytes = data.len(), - "Tor packet received" - ); + if admitted { + let mut first = true; + loop { + let read = match first_frame_timeout { + // Bound the first read only. A silent remote otherwise holds + // its inbound slot for as long as it keeps the socket open. + Some(d) if first => { + match tokio::time::timeout(d, read_fmp_packet(&mut reader, mtu)).await { + Ok(result) => result, + Err(_) => { + // Not a recv error: `record_recv_error` means + // framing or I/O failure, and folding deadline + // expiries into it corrupts that counter. + debug!( + transport_id = %transport_id, + remote_addr = %remote_addr, + timeout_secs = d.as_secs_f64(), + "No complete frame within the first-frame deadline, dropping inbound onion connection" + ); + break; + } + } + } + _ => read_fmp_packet(&mut reader, mtu).await, + }; + first = false; - let packet = ReceivedPacket::new(transport_id, remote_addr.clone(), data); + match read { + Ok(data) => { + stats.record_recv(data.len()); - if packet_tx.send(packet).await.is_err() { + trace!( + transport_id = %transport_id, + remote_addr = %remote_addr, + bytes = data.len(), + "Tor packet received" + ); + + let packet = ReceivedPacket::new(transport_id, remote_addr.clone(), data); + + if packet_tx.send(packet).await.is_err() { + debug!( + transport_id = %transport_id, + "Packet channel closed, stopping Tor receive loop" + ); + break; + } + } + Err(e) => { + stats.record_recv_error(); debug!( transport_id = %transport_id, - "Packet channel closed, stopping Tor receive loop" + remote_addr = %remote_addr, + error = %e, + "Tor receive error, removing connection" ); break; } } - Err(e) => { - stats.record_recv_error(); - debug!( - transport_id = %transport_id, - remote_addr = %remote_addr, - error = %e, - "Tor receive error, removing connection" - ); - break; - } } } @@ -1292,6 +1345,7 @@ fn configure_socket( /// connections to a local TCP listener; we accept them, configure /// socket options, split the stream, and spawn a per-connection /// receive task. +#[allow(clippy::too_many_arguments)] async fn tor_accept_loop( listener: TcpListener, transport_id: TransportId, @@ -1299,6 +1353,7 @@ async fn tor_accept_loop( pool: ConnectionPool, mtu: u16, max_inbound: usize, + first_frame_timeout: Duration, stats: Arc, ) { debug!( @@ -1376,6 +1431,12 @@ async fn tor_accept_loop( let recv_addr = remote_addr.clone(); let recv_tx = packet_tx.clone(); + // Readiness barrier: the receive task must not reach its cleanup + // path before the pool insert and counter bump below, or it would + // remove nothing and leave an orphaned entry with a permanently + // incremented inbound counter. + let (ready_tx, ready_rx) = tokio::sync::oneshot::channel(); + let recv_task = tokio::spawn(async move { tor_receive_loop( read_half, @@ -1386,6 +1447,8 @@ async fn tor_accept_loop( mtu, recv_stats, Direction::Inbound, + Some(first_frame_timeout), + Some(ready_rx), ) .await; }); @@ -1406,6 +1469,10 @@ async fn tor_accept_loop( stats.record_connection_accepted(); stats.record_pool_inbound_added(); + // Release the receive task now that both the pool entry and the + // inbound counter are in place. + let _ = ready_tx.send(()); + debug!( transport_id = %transport_id, peer_addr = %peer_addr, @@ -2034,4 +2101,107 @@ mod tests { let err = format!("{}", result.unwrap_err()); assert!(err.contains("directory")); } + + // ======================================================================== + // Inbound first-frame deadline (onion listener) + // ======================================================================== + + /// Poll `f` every 10ms until it holds or `limit` elapses. + async fn wait_until bool>(mut f: F, limit: Duration) -> bool { + let deadline = Instant::now() + limit; + loop { + if f() { + return true; + } + if Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + /// Drives `tor_accept_loop` directly: the only production path to it is + /// `start_directory_mode`, which needs a Tor-managed hostname file and a + /// running daemon, so it is not reachable from a unit test. + fn spawn_onion_accept_loop( + listener: TcpListener, + packet_tx: PacketTx, + first_frame_timeout: Duration, + ) -> (ConnectionPool, Arc, JoinHandle<()>) { + let pool: ConnectionPool = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(TorStats::new()); + let handle = tokio::spawn(tor_accept_loop( + listener, + TransportId::new(1), + packet_tx, + pool.clone(), + 1400, + 64, + first_frame_timeout, + stats.clone(), + )); + (pool, stats, handle) + } + + /// Mirror of the TCP case: a silent onion-side socket must lose its + /// inbound slot at the deadline. Break-check: with the wrapper removed + /// the count stays at 1 and the second assertion fails. + #[tokio::test] + async fn idle_inbound_onion_socket_releases_its_slot() { + let (tx, _rx) = packet_channel(32); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let (pool, stats, accept) = + spawn_onion_accept_loop(listener, tx, Duration::from_millis(200)); + + // Held open for the whole test: any release is the deadline's doing. + let squatter = TcpStream::connect(listen).await.unwrap(); + + assert!( + wait_until(|| stats.pool_inbound_count() == 1, Duration::from_secs(2)).await, + "an accepted onion socket should take an inbound slot" + ); + assert!( + wait_until(|| stats.pool_inbound_count() == 0, Duration::from_secs(2)).await, + "a silent onion socket should lose its slot at the first-frame deadline" + ); + assert!(pool.lock().await.is_empty()); + + drop(squatter); + accept.abort(); + } + + /// Regression guard only, as for TCP: the deadline is scoped to the + /// first iteration, so this passes by construction. It exists so a + /// future general idle deadline cannot start reaping quiet onion links + /// without a test going red. + #[tokio::test] + async fn established_onion_connection_survives_long_idle() { + let (tx, mut rx) = packet_channel(32); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let (pool, stats, accept) = + spawn_onion_accept_loop(listener, tx, Duration::from_millis(200)); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + peer.write_all(&build_msg1_frame()).await.unwrap(); + let packet = tokio::time::timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout") + .expect("packet channel closed"); + assert_eq!(packet.data, build_msg1_frame()); + + // Four deadlines' worth of silence after the first frame. + tokio::time::sleep(Duration::from_millis(800)).await; + + assert_eq!( + stats.pool_inbound_count(), + 1, + "an established onion connection must not be dropped by the first-frame deadline" + ); + assert!(!pool.lock().await.is_empty()); + + drop(peer); + accept.abort(); + } } From d399f8e07d3adc8074583c80cc1ffee60e6abdc6 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 11 Aug 2026 15:26:47 +0000 Subject: [PATCH 4/5] Pin every GitHub Action to a commit SHA, and verify the nak download Not one action reference in this repository was pinned. Every uses: line named a mutable tag, and one named a branch. That includes the jobs holding the AUR deploy key, the jobs with release write scope, and the packaging jobs that run with a signing key in the environment, so whoever controls one of those action repositories could repoint a tag into a job holding our credentials. Sixty-two of the sixty-six references are now full commit SHAs with the original tag kept as a trailing comment, so a reader can still tell which release a pin is. Each SHA was resolved from the upstream peeled tag. Four references are left unpinned and justified in one place rather than silently: two actions select the tool they install from the ref name itself, so a bare SHA hands them a hex string where a toolchain name belongs and the step fails. Pinning those means moving the selection into with:, which changes what resolves, and that is a separate decision from pinning. A guard enforces the form on every sweep, wired into the parity job and the local runner beside the existing checkers. It accepts only owner/repo@40-hex with a mandatory trailing comment, treats an unreadable tree as exit 2 rather than as a pass, and its header names what it does not cover: the actions that pinned actions themselves invoke, the pip and cargo installs that are version pinned at best, and anything fetched at run time. The sharper hole was not the tags. The OpenWrt packaging workflow fetched a helper binary straight from a release URL with no verification, in two jobs that hold a signing key, which is code execution from a third-party host into a credentialed job and needs nobody to retag anything. That download now goes through a shared script with per-architecture pinned SHA-256 constants, modelled on the zig block already in that workflow. Upstream publishes no checksum document, so the provenance comment records the asset URL and the date the hashes were taken by downloading rather than pretending they were verified against a published sum. --- .github/scripts/install-nak.sh | 105 +++++++++++++++++++ .github/workflows/aur-publish-git.yml | 4 +- .github/workflows/aur-publish.yml | 6 +- .github/workflows/ci.yml | 52 ++++----- .github/workflows/package-linux.yml | 12 +-- .github/workflows/package-macos.yml | 14 +-- .github/workflows/package-openwrt.yml | 59 +++++------ .github/workflows/package-windows.yml | 12 +-- testing/check-action-pins.sh | 145 ++++++++++++++++++++++++++ testing/ci-local.sh | 11 ++ 10 files changed, 338 insertions(+), 82 deletions(-) create mode 100755 .github/scripts/install-nak.sh create mode 100755 testing/check-action-pins.sh diff --git a/.github/scripts/install-nak.sh b/.github/scripts/install-nak.sh new file mode 100755 index 0000000..f04e897 --- /dev/null +++ b/.github/scripts/install-nak.sh @@ -0,0 +1,105 @@ +#!/bin/bash +# ── Install nak, checksum-verified ────────────────────────────────────────── +# nak signs the release announcement events, and the jobs that call this script +# hand it the publishing nsec on argv. An unverified download therefore runs +# with the signing key in reach, so the binary is staged, checked against a +# pinned SHA-256, and only then installed. +# +# Called from .github/workflows/package-openwrt.yml by both the .ipk (`build`) +# and .apk (`build-apk`) jobs, which is why it lives here rather than under +# packaging/openwrt-ipk/ — that directory is the .ipk payload tree. +# +# Exit 0 = installed and verified. Any non-zero exit means nothing was +# installed. +# ───────────────────────────────────────────────────────────────────────────── +set -euo pipefail + +NAK_VERSION="0.16.2" +INSTALL_PATH="/usr/local/bin/nak" + +ARCH=$(uname -m) +# Each arch carries the expected SHA-256 of its upstream release asset. +# +# Unlike the zig hashes in the same workflow, which come from ziglang.org's own +# https://ziglang.org/download/index.json, these are NOT upstream-attested: +# fiatjaf/nak publishes no checksum document, sidecar or SHA256SUMS alongside +# its release assets, so the only way to obtain a hash is to download the asset +# and compute it. These were derived that way on 2026-08-11 from +# https://github.com/fiatjaf/nak/releases/download/v0.16.2/nak-v0.16.2-linux- +# What the pin buys is therefore continuity, not authenticity: it detects the +# asset changing under a fixed tag, a corrupted or truncated transfer, and a +# substituted download, but it cannot attest that the bytes captured on that +# date were the bytes upstream intended. Bumping NAK_VERSION means re-deriving +# every hash below, and adding an arch means adding its hash here too. +case "$ARCH" in + x86_64|amd64) + NAK_ARCH="amd64" + NAK_SHA256="495243c070c4533ce96e98b6f34b7e97fd4be2da3353488b400233ed7ed0d4da" + ;; + aarch64|arm64) + NAK_ARCH="arm64" + NAK_SHA256="1fb8868c60ebf77dd86f90d6374ebf8557412baa37026d2844af932776085b88" + ;; + *) + echo "Unsupported architecture: $ARCH" + exit 1 + ;; +esac + +if [ -z "${NAK_SHA256:-}" ]; then + echo "No SHA-256 pinned for nak ${NAK_VERSION} on ${NAK_ARCH}." + echo "Add one to the case above, derived by downloading the asset:" + echo " curl -fsSL | sha256sum" + exit 1 +fi + +NAME="nak-v${NAK_VERSION}-linux-${NAK_ARCH}" +URL="https://github.com/fiatjaf/nak/releases/download/v${NAK_VERSION}/${NAME}" +# Stage outside the checkout so a failed attempt cannot leave a stray binary in +# the working tree, and so nothing lands at $INSTALL_PATH before it verifies. +NAK_TMP="$(mktemp -d)" +trap 'rm -rf "$NAK_TMP"' EXIT +TMP="${NAK_TMP}/${NAME}" + +# Download to a file and check it before installing. curl's own --retry does +# not cover a short read (exit 18), and a checksum mismatch needs a fresh +# download anyway, so the retry is an explicit bounded loop — the same failure +# mode that forced one on the zig step in this workflow. +verified="" +previous="" +for attempt in 1 2 3; do + rm -f "$TMP" + if curl -fsSL -o "$TMP" "$URL" && [ -s "$TMP" ]; then + actual="$(sha256sum < "$TMP" | cut -d' ' -f1)" + if [ "$actual" = "$NAK_SHA256" ]; then + echo "nak binary matches its pinned SHA-256 (${actual})" + verified=yes + break + fi + echo "nak binary failed its checksum on attempt ${attempt}:" + echo " expected ${NAK_SHA256}" + echo " actual ${actual}" + echo " size $(wc -c < "$TMP") bytes" + if [ "$actual" = "$previous" ]; then + echo "Two attempts fetched byte-identical content, so retrying is not" + echo "going to help: the pin is stale, upstream re-published, or the" + echo "source is serving the same bad file every time." + break + fi + previous="$actual" + else + echo "nak binary download failed on attempt ${attempt}" + fi + if [ "$attempt" -lt 3 ]; then + sleep $((attempt * 10)) + fi +done + +if [ -z "$verified" ]; then + echo "nak ${NAK_VERSION} (${NAK_ARCH}) did not download with its pinned" + echo "SHA-256 ${NAK_SHA256} in 3 attempts. Nothing installed at ${INSTALL_PATH}." + exit 1 +fi + +install -m 0755 "$TMP" "$INSTALL_PATH" +echo "Installed nak ${NAK_VERSION} (${NAK_ARCH}) at ${INSTALL_PATH}" diff --git a/.github/workflows/aur-publish-git.yml b/.github/workflows/aur-publish-git.yml index 8d9c47f..28f7ab4 100644 --- a/.github/workflows/aur-publish-git.yml +++ b/.github/workflows/aur-publish-git.yml @@ -17,7 +17,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Patch PKGBUILD-git b2sums for local assets run: | @@ -42,7 +42,7 @@ jobs: awk '/^b2sums=\(/,/\)$/' packaging/aur/PKGBUILD-git - name: Publish to AUR - uses: KSXGitHub/github-actions-deploy-aur@v4.1.2 + uses: KSXGitHub/github-actions-deploy-aur@abe8ac26b51011c88be58c8809fd2ac674068ea5 # v4.1.2 with: pkgname: fips-git pkgbuild: packaging/aur/PKGBUILD-git diff --git a/.github/workflows/aur-publish.yml b/.github/workflows/aur-publish.yml index 286f0d9..50eadd1 100644 --- a/.github/workflows/aur-publish.yml +++ b/.github/workflows/aur-publish.yml @@ -41,7 +41,7 @@ jobs: set -euo pipefail pacman -Sy --noconfirm --needed base-devel namcap git curl - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Resolve package version id: ver @@ -176,7 +176,7 @@ jobs: echo "version=${TAG#v}" >> "$GITHUB_OUTPUT" echo "pkgrel=${PKGREL}" >> "$GITHUB_OUTPUT" - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: ref: ${{ steps.tag.outputs.tag }} @@ -188,7 +188,7 @@ jobs: run: bash packaging/aur/patch-pkgbuild.sh - name: Publish to AUR - uses: KSXGitHub/github-actions-deploy-aur@v4.1.2 + uses: KSXGitHub/github-actions-deploy-aur@abe8ac26b51011c88be58c8809fd2ac674068ea5 # v4.1.2 with: pkgname: fips pkgbuild: packaging/aur/PKGBUILD diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9083806..a271b7e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,7 +56,7 @@ jobs: name: CI parity runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Install Python deps run: pip3 install --quiet pyyaml - name: Check local and GitHub runners cover the same work @@ -67,6 +67,8 @@ jobs: run: python3 testing/check-trailing-log.py - name: Check nothing resolves the shared mutable test image run: bash testing/check-image-scoping.sh + - name: Check every action is pinned to a commit SHA + run: bash testing/check-action-pins.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 @@ -79,8 +81,8 @@ jobs: name: Format check runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 - - uses: actions-rust-lang/setup-rust-toolchain@v1 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + - uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: components: rustfmt cache: false @@ -91,16 +93,16 @@ jobs: name: Clippy runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Install system dependencies run: sudo apt-get update && sudo apt-get install -y libdbus-1-dev - - uses: actions-rust-lang/setup-rust-toolchain@v1 + - uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: components: clippy cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -125,7 +127,7 @@ jobs: - os: windows-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Set SOURCE_DATE_EPOCH from git (Unix) if: runner.os != 'Windows' @@ -147,13 +149,13 @@ jobs: run: sudo nft -c -f packaging/common/fips.nft - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -182,7 +184,7 @@ jobs: # Upload the Linux binary so integration jobs can use it without rebuilding - name: Upload Linux binary if: matrix.os == 'ubuntu-latest' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fips-linux path: | @@ -203,7 +205,7 @@ jobs: runs-on: ubuntu-latest needs: [build] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Set SOURCE_DATE_EPOCH from git run: echo "SOURCE_DATE_EPOCH=$(git log -1 --format=%ct)" >> "$GITHUB_ENV" @@ -212,13 +214,13 @@ jobs: run: sudo apt-get update && sudo apt-get install -y libdbus-1-dev - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -235,7 +237,7 @@ jobs: run: cargo nextest run --all --profile ci - name: Publish test report (Checks tab) - uses: dorny/test-reporter@v2 + uses: dorny/test-reporter@df6247429542221bc30d46a036ee47af1102c451 # v2 if: always() with: name: Unit Tests @@ -244,7 +246,7 @@ jobs: fail-on-error: false - name: Publish test report (run summary) - uses: mikepenz/action-junit-report@v4 + uses: mikepenz/action-junit-report@db71d41eb79864e25ab0337e395c352e84523afe # v4 if: always() with: report_paths: target/nextest/ci/junit.xml @@ -259,19 +261,19 @@ jobs: runs-on: macos-latest needs: [build] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Set SOURCE_DATE_EPOCH from git run: echo "SOURCE_DATE_EPOCH=$(git log -1 --format=%ct)" >> "$GITHUB_ENV" - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -294,16 +296,16 @@ jobs: name: Unit tests (Windows) runs-on: windows-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -332,7 +334,7 @@ jobs: name: PowerShell lint (Windows packaging) runs-on: windows-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Run PSScriptAnalyzer shell: pwsh @@ -471,11 +473,11 @@ jobs: type: dns-resolver steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 # Fetch the pre-built Linux binary from job 1 - name: Download Linux binary - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: fips-linux path: _bin @@ -646,7 +648,7 @@ jobs: - name: Upload sim results on failure (chaos) if: matrix.type == 'chaos' && failure() - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: sim-results-${{ matrix.scenario }} path: testing/chaos/sim-results/ diff --git a/.github/workflows/package-linux.yml b/.github/workflows/package-linux.yml index d7eefa6..b0d5327 100644 --- a/.github/workflows/package-linux.yml +++ b/.github/workflows/package-linux.yml @@ -19,7 +19,7 @@ jobs: outputs: linux_package_version: ${{ steps.linux_version.outputs.linux_package_version }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -61,7 +61,7 @@ jobs: deb_arch: arm64 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -72,14 +72,14 @@ jobs: run: sudo apt-get update && sudo apt-get install -y --no-install-recommends libdbus-1-dev llvm - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build if: ${{ env.ACT != 'true' }} - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -140,7 +140,7 @@ jobs: - name: Upload artifact (GitHub only) if: ${{ env.ACT != 'true' }} - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fips_${{ needs.determine-versioning.outputs.linux_package_version }}_${{ matrix.artifact_arch }}_linux path: | @@ -164,7 +164,7 @@ jobs: steps: - name: Download Linux artifacts - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: path: dist merge-multiple: true diff --git a/.github/workflows/package-macos.yml b/.github/workflows/package-macos.yml index e9b212e..a0ad927 100644 --- a/.github/workflows/package-macos.yml +++ b/.github/workflows/package-macos.yml @@ -19,7 +19,7 @@ jobs: outputs: macos_package_version: ${{ steps.macos_version.outputs.macos_package_version }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -61,7 +61,7 @@ jobs: target: x86_64-apple-darwin steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -69,14 +69,14 @@ jobs: run: echo "SOURCE_DATE_EPOCH=$(git log -1 --format=%ct)" >> "$GITHUB_ENV" - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: target: ${{ matrix.target }} cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -209,7 +209,7 @@ jobs: ( cd "$(dirname "$PKG")" && shasum -a 256 "$(basename "$PKG")" | tee "$(basename "$PKG").sha256" ) - name: Upload artifact - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fips_${{ needs.determine-versioning.outputs.macos_package_version }}_${{ matrix.arch }}_macos path: | @@ -229,7 +229,7 @@ jobs: steps: - name: Download macOS artifacts - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: path: dist merge-multiple: true @@ -283,7 +283,7 @@ jobs: steps: - name: Download macOS artifacts - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: path: dist merge-multiple: true diff --git a/.github/workflows/package-openwrt.yml b/.github/workflows/package-openwrt.yml index 1552a44..c120ada 100644 --- a/.github/workflows/package-openwrt.yml +++ b/.github/workflows/package-openwrt.yml @@ -27,7 +27,7 @@ jobs: apk_version: ${{ steps.version.outputs.apk_version }} release_channel: ${{ steps.channel.outputs.release_channel }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -104,13 +104,13 @@ jobs: # x86 routers / VMs steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 - name: Install Rust toolchain (stable) if: matrix.rust_channel == 'stable' - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: target: ${{ matrix.rust_target }} cache: false @@ -124,7 +124,7 @@ jobs: - name: Cache Cargo registry + build if: ${{ env.ACT != 'true' }} - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -247,7 +247,7 @@ jobs: ls -lh out/ - name: Upload binaries artifact - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fips-bins-${{ matrix.openwrt_arch }} path: out/ @@ -270,7 +270,7 @@ jobs: openwrt_arch: x86_64 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -283,24 +283,17 @@ jobs: echo "PACKAGE_FILENAME=$PACKAGE_FILENAME" >> $GITHUB_ENV - name: Download prebuilt binaries - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: fips-bins-${{ matrix.openwrt_arch }} path: bins + # nak receives the signing nsec on argv further down, so the download is + # staged and checked against a pinned SHA-256 before it is installed. - name: Install nak shell: bash run: | - NAK_VERSION="0.16.2" - ARCH=$(uname -m) - case "$ARCH" in - x86_64|amd64) NAK_ARCH="amd64" ;; - aarch64|arm64) NAK_ARCH="arm64" ;; - *) echo "Unsupported architecture: $ARCH"; exit 1 ;; - esac - curl -fsSL "https://github.com/fiatjaf/nak/releases/download/v${NAK_VERSION}/nak-v${NAK_VERSION}-linux-${NAK_ARCH}" \ - -o /usr/local/bin/nak - chmod +x /usr/local/bin/nak + bash .github/scripts/install-nak.sh nak --version - name: Install jq @@ -346,6 +339,13 @@ jobs: fi shellcheck --version + # Its own step, and its own shell dialect. The shipped-scripts lint below + # runs --shell=sh with the OpenWrt rc.common exclude set, which misfires + # on a bash script; install-nak.sh is also not shipped in the package. + - name: Lint install-nak.sh + shell: bash + run: shellcheck --shell=bash .github/scripts/install-nak.sh + - name: Lint shipped shell scripts shell: bash run: | @@ -529,7 +529,7 @@ jobs: - name: Upload artifact (GitHub only) if: ${{ env.ACT != 'true' }} - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: ${{ env.PACKAGE_FILENAME }} path: dist/${{ env.PACKAGE_FILENAME }} @@ -684,7 +684,7 @@ jobs: APK_TOOLS_COMMIT: "b5a31c0d865342ad80be10d68f1bb3d3ad9b0866" steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -697,7 +697,7 @@ jobs: echo "PACKAGE_FILENAME=$PACKAGE_FILENAME" >> $GITHUB_ENV - name: Download prebuilt binaries - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: fips-bins-${{ matrix.openwrt_arch }} path: bins @@ -821,25 +821,18 @@ jobs: - name: Upload artifact (GitHub only) if: ${{ env.ACT != 'true' }} - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: ${{ env.PACKAGE_FILENAME }} path: dist/${{ env.PACKAGE_FILENAME }} retention-days: 30 + # nak receives the signing nsec on argv further down, so the download is + # staged and checked against a pinned SHA-256 before it is installed. - name: Install nak shell: bash run: | - NAK_VERSION="0.16.2" - ARCH=$(uname -m) - case "$ARCH" in - x86_64|amd64) NAK_ARCH="amd64" ;; - aarch64|arm64) NAK_ARCH="arm64" ;; - *) echo "Unsupported architecture: $ARCH"; exit 1 ;; - esac - curl -fsSL "https://github.com/fiatjaf/nak/releases/download/v${NAK_VERSION}/nak-v${NAK_VERSION}-linux-${NAK_ARCH}" \ - -o /usr/local/bin/nak - chmod +x /usr/local/bin/nak + bash .github/scripts/install-nak.sh nak --version - name: Install jq @@ -983,7 +976,7 @@ jobs: steps: - name: Download package artifacts - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: # Only the .ipk/.apk packages (named fips__.*), not the # fips-bins-* raw-binary artifacts shared between the build jobs. @@ -1000,7 +993,7 @@ jobs: > checksums-openwrt.txt - name: Create release - uses: softprops/action-gh-release@v2 + uses: softprops/action-gh-release@3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 # v2 with: files: | dist/*.ipk diff --git a/.github/workflows/package-windows.yml b/.github/workflows/package-windows.yml index ca13320..286d0ab 100644 --- a/.github/workflows/package-windows.yml +++ b/.github/workflows/package-windows.yml @@ -19,7 +19,7 @@ jobs: outputs: package_version: ${{ steps.version.outputs.package_version }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -52,7 +52,7 @@ jobs: needs: determine-versioning steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: fetch-depth: 0 @@ -63,13 +63,13 @@ jobs: echo "SOURCE_DATE_EPOCH=$epoch" >> $env:GITHUB_ENV - name: Install Rust toolchain - uses: actions-rust-lang/setup-rust-toolchain@v1 + uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1 with: cache: false rustflags: '' - name: Cache Cargo registry + build - uses: actions/cache@v5 + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: | ~/.cargo/registry @@ -146,7 +146,7 @@ jobs: } - name: Upload artifact - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fips_${{ needs.determine-versioning.outputs.package_version }}_x86_64_windows path: deploy/fips-*-windows-*.zip @@ -170,7 +170,7 @@ jobs: steps: - name: Download Windows artifacts - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: path: dist merge-multiple: true diff --git a/testing/check-action-pins.sh b/testing/check-action-pins.sh new file mode 100755 index 0000000..63a9e2c --- /dev/null +++ b/testing/check-action-pins.sh @@ -0,0 +1,145 @@ +#!/bin/bash +# ── GitHub Action pinning guard ───────────────────────────────────────────── +# Every third-party action this repository invokes must be referenced by a +# 40-character commit SHA, with its human-readable tag in a trailing comment. +# +# A tag is a mutable pointer. Whoever controls an action's repository can move +# `v6` to different code at any time, and several of the jobs here are worth +# moving it for: aur-publish.yml and aur-publish-git.yml hand an action +# AUR_SSH_PRIVATE_KEY, and the OpenWrt release jobs run with HIVE_CI_NSEC in +# the environment. A SHA is content-addressed and cannot be repointed. The +# trailing comment is required rather than optional so the pin stays legible: +# a bare 40-hex string tells a reader nothing about which release it is, and a +# pin nobody can read is a pin nobody updates. +# +# What counts as a violation: any `uses:` reference that is not +# * `owner/repo@<40 hex> # ` — the required form, comment mandatory; or +# * a local action, `./path` or `docker://...`; or +# * one of the individually justified references listed below. +# +# WHAT THIS GUARD DOES NOT COVER, so a green run is not read as "the workflows +# fetch nothing unverified": +# * the actions that the pinned actions themselves invoke. Pinning +# KSXGitHub/github-actions-deploy-aur removes the retag vector; it does not +# constrain what that action does with the SSH key it is given by design +# (aur-publish.yml, aur-publish-git.yml). +# * `pip3 install --quiet pyyaml` in ci.yml's ci-parity job, which holds +# `checks: write`. Unpinned entirely, version and hash both. +# * `cargo install cargo-zigbuild --version 0.19.8 --locked` in +# package-openwrt.yml. Version-pinned, not hash-pinned. +# * anything a workflow downloads at run time. The zig tarball and the nak +# binary are SHA-256 checked in their own steps; nothing here enforces that. +# +# Exit 0 = clean. Exit 1 = an unpinned reference. Exit 2 = the guard could not +# run; never treated as a pass. +# ───────────────────────────────────────────────────────────────────────────── +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +REPO_ROOT="$SCRIPT_DIR/.." + +# The one accepted form for a third-party action. The comment is mandatory +# rather than optional: an optional comment would let the checker accept a pin +# it cannot describe, and a pin nobody can read is a pin nobody updates. +PINNED_RE='^[^@]+@[0-9a-f]{40} +#.*$' + +# Individually justified unpinned references. Each entry is the exact ref text. +# +# Both of these actions read the tool they install from the ref name itself +# (`github.action_ref`), so replacing the ref with a SHA hands them a 40-hex +# string where a toolchain or tool name belongs and the step fails outright. +# They are not pinnable without also moving the selection into `with:`, which +# changes which toolchain resolves, and that is a separate decision from +# pinning. Note what stays exposed: both remain repointable by their upstream +# owners. +ALLOWED_REFS=( + 'dtolnay/rust-toolchain@nightly' + 'taiki-e/install-action@nextest' +) + +if ! command -v git >/dev/null 2>&1; then + echo "check-action-pins: git not available, cannot sweep" >&2 + exit 2 +fi +if [[ ! -d "$REPO_ROOT/.github" ]]; then + echo "check-action-pins: $REPO_ROOT/.github missing, refusing to pass" >&2 + exit 2 +fi + +# Tracked files only. Workflows plus any composite/local action definition: +# a future .yaml extension and a future .github/actions/ tree both have to be +# swept, or the guard silently narrows as the repository grows. +if ! tracked="$(git -C "$REPO_ROOT" ls-files -- '.github/workflows/*.yml' '.github/workflows/*.yaml' '.github/actions/*.yml' '.github/actions/*.yaml')"; then + echo "check-action-pins: git ls-files failed, refusing to pass" >&2 + exit 2 +fi +if [[ -z "$tracked" ]]; then + echo "check-action-pins: no tracked workflow or action files, refusing to pass" >&2 + exit 2 +fi +mapfile -t files < <(printf '%s\n' "$tracked") +if [[ ${#files[@]} -eq 0 ]]; then + echo "check-action-pins: empty file list, refusing to pass" >&2 + exit 2 +fi + +# True when this ref is one of the justified references above. +allowed_ref() { + local ref="$1" entry + for entry in "${ALLOWED_REFS[@]}"; do + [[ "$ref" == "$entry" ]] && return 0 + done + return 1 +} + +violations=0 +checked=0 + +for f in "${files[@]}"; do + [[ -f "$REPO_ROOT/$f" ]] || continue + + while IFS= read -r hit; do + n="${hit%%:*}" + text="${hit#*:}" + # A commented-out step is describing a reference, not resolving it. + [[ "$text" =~ ^[[:space:]]*# ]] && continue + + # Everything after `uses:`, with surrounding whitespace and any quoting + # removed. The trailing comment is part of the ref text on purpose: + # the accepted form requires it. + ref="${text#*uses:}" + ref="${ref#"${ref%%[![:space:]]*}"}" + ref="${ref%"${ref##*[![:space:]]}"}" + + checked=$((checked + 1)) + + # A local action or a container image is not a mutable upstream tag. + [[ "$ref" == ./* ]] && continue + [[ "$ref" == docker://* ]] && continue + [[ "$ref" =~ $PINNED_RE ]] && continue + allowed_ref "$ref" && continue + + echo "$f:$n: $ref" + violations=$((violations + 1)) + done < <(grep -nE '^[[:space:]]*(- )?uses:' "$REPO_ROOT/$f" 2>/dev/null) +done + +if [[ $checked -eq 0 ]]; then + echo "check-action-pins: no uses: references found at all, refusing to pass" >&2 + exit 2 +fi + +if [[ $violations -gt 0 ]]; then + echo "" + echo "check-action-pins: $violations action reference(s) are not pinned to a commit SHA." + echo "Required form: uses: owner/repo@<40-hex-commit-sha> # " + echo "Resolve one with:" + echo " git ls-remote https://github.com/owner/repo 'refs/tags/^{}' refs/tags/" + echo "and use the peeled (^{}) SHA when the tag is annotated." + echo "A tag is a mutable pointer its owner can repoint; several of these jobs" + echo "hold a signing key or an SSH deploy key while the action runs." + exit 1 +fi + +echo "check-action-pins: all $checked action reference(s) pinned or justified" +exit 0 diff --git a/testing/ci-local.sh b/testing/ci-local.sh index 6858e5f..f3df3ab 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -1318,6 +1318,16 @@ run_image_scoping() { record "image-scoping" $rc } +# Every third-party action must be referenced by commit SHA. A tag is a mutable +# pointer, and the jobs holding the AUR deploy key and the release signing key +# are exactly the ones worth repointing it for. Static, and it costs nothing. +run_action_pins() { + local rc=0 + info "[action-pins] Checking that every action is pinned to a commit SHA" + "$SCRIPT_DIR/check-action-pins.sh" || rc=$? + record "action-pins" $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. @@ -1372,6 +1382,7 @@ main() { run_log_strings run_trailing_log run_image_scoping + run_action_pins run_wait_converge if [[ "$TEST_ONLY" == true ]]; then From 64aa314e2664ad15af285fa4babbe1f3c4e7b3b4 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 11 Aug 2026 15:44:53 +0000 Subject: [PATCH 5/5] Record the four security fixes in the changelog The dependency refresh landed with its own entry; the gateway DNS validation, the key-write hardening, the inbound TCP slot deadline and the action pinning did not. Each entry states what the defect was, what closed it, and where a behaviour changed: the DNS rcode relay, the fips.pub mode policy, and the fact that the TCP deadline covers the first frame only and leaves a peer that sends one frame then goes silent still holding a slot. --- CHANGELOG.md | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 663b1f7..5185bd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -237,6 +237,73 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 0.16.4 carries an unsoundness advisory, and `nostr-relay-pool` itself is now marked unmaintained. +- The gateway DNS forwarder now validates an upstream answer before it becomes + a NAT mapping. It previously accepted whatever datagram arrived: the upstream + query reused the client's own transaction ID, the upstream socket was + wildcard-bound and never connected, the receive discarded the sender, neither + the response ID nor the question section was compared against what was asked, + and the returned address was not checked against the mesh prefix. Because the + extracted address is installed as a DNAT rule that carries no interface + constraint, a forged answer redirected traffic rather than only poisoning a + lookup. The upstream query now carries a random transaction ID, the socket is + connected so the kernel drops foreign sources, a response must match on ID, + question and type or it is discarded while the receive continues against the + original deadline, and the address goes through the validating parser with a + non-mesh answer refused before any allocation. One deliberate behaviour + change: the validation sits before the rcode check, so an upstream answering + FORMERR or REFUSED with an empty question section now yields SERVFAIL rather + than having its rcode relayed. Checking after the rcode would admit a forged + NXDOMAIN. Connecting the socket also means a dead upstream surfaces + ECONNREFUSED immediately instead of stalling for five seconds. + +- Private key writes no longer follow a symlink, and the key file's mode is + enforced rather than merely requested. The single write path opened with + create and truncate and no `O_NOFOLLOW`, so a symlink planted at the key path + was followed and its target overwritten, and it supplied the mode only + through `open(2)`, which the kernel honours on creation and ignores + otherwise, so a `fips.key` that already existed at 0644 stayed 0644 through + every rewrite. That second half needs no attacker: one `chmod`, or a restore + that did not preserve modes, leaves the key readable indefinitely. Both + writers now share an open helper carrying `O_NOFOLLOW`, and the private key + has its mode applied to the open descriptor before any secret bytes are + written. The public key keeps create-time mode instead, since forcing it + would reopen an operator-tightened `fips.pub` on every start. On Windows + neither protection applies and the file inherits the parent directory's + ACLs; that exclusion is deliberate and recorded at both writers. + +- An accepted inbound TCP connection no longer holds a slot indefinitely + without sending anything. The cap was tested at accept and the pool insert + and counter bump followed with no read in between, while the frame reader's + reads carried no deadline, so an unauthenticated remote held a slot by + connecting and staying silent. Pool keys are `ip:port`, so N sockets from one + address took N slots, and at the 256 default that locked out inbound peering + for as long as the sockets stayed open. The first frame on an inbound + connection now has a deadline, as a module constant rather than a new + configuration key, and the onion listener gets the same treatment for the + same accept-then-count ordering. Separately, the node's handshake reaper tore + down session state without closing the transport connection, so a peer that + sent msg1 and then stalled was forgotten by the node while its socket and + slot survived; the reaper now closes the connection too. **What this does not + close**: the deadline covers the first frame only, so a peer that sends one + well-formed frame and then goes silent still holds its slot. Closing that + needs a rolling idle deadline. + +- Every GitHub Action is pinned to a commit SHA, and the OpenWrt packaging + workflow verifies the helper binary it downloads. No reference in the + repository was pinned before: all sixty-six named a mutable tag and one named + a branch, including the jobs holding the AUR deploy key, the jobs with + release write scope, and the packaging jobs that run with a signing key in + the environment. Sixty-two are now full commit SHAs with the original tag + retained as a trailing comment. Four are left unpinned and justified in one + place: two actions read the tool to install from the ref name itself, so a + SHA would hand them a hex string where a toolchain name belongs. A guard + enforces the form on every sweep, treats an unreadable tree as an error + rather than a pass, and documents what it does not cover. The sharper hole + was not the tags: the OpenWrt workflow fetched a helper binary from a release + URL with no verification at all, in two jobs holding a signing key. That + download now checks a per-architecture pinned SHA-256, with the hash + provenance recorded honestly, upstream publishing no checksum document. + ## [0.4.1] - 2026-07-19 ### Changed