From 15d81c530c3f16720fcbb3611cb86112b4d916fb Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:05:59 -0500 Subject: [PATCH 01/14] feat(node): add a persistent libp2p identity key file Add p2p_key_path (--p2p-key-path / GITLAWB_P2P_KEY, default ~/.gitlawb/p2p.key) with a resolver mirroring resolved_key_path, and load_or_create_p2p_keypair, which generates an Ed25519 keypair on first start, persists it 0600, and loads it thereafter. Mirrors the existing load_or_create_keypair idiom for the node identity PEM. A corrupt or unreadable key file is a hard error naming the path rather than a silent regeneration, so a disk problem cannot quietly rotate the node's network identity. Not yet wired into p2p::start; that follows. --- crates/gitlawb-node/src/config.rs | 14 ++++ crates/gitlawb-node/src/p2p/mod.rs | 104 ++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index fefa063e6..7104054b5 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -104,6 +104,10 @@ pub struct Config { #[arg(long, env = "GITLAWB_P2P_PORT", default_value_t = 7546)] pub p2p_port: u16, + /// Path to the persistent libp2p identity key + #[arg(long, env = "GITLAWB_P2P_KEY", default_value = "~/.gitlawb/p2p.key")] + pub p2p_key_path: String, + /// libp2p bootstrap multiaddrs (comma-separated) /// Example: /ip4/1.2.3.4/udp/7546/quic-v1/p2p/12D3KooW... #[arg(long, env = "GITLAWB_P2P_BOOTSTRAP", value_delimiter = ',')] @@ -556,6 +560,16 @@ impl Config { PathBuf::from(&self.key_path) } + /// Resolve ~ in p2p_key_path + pub fn resolved_p2p_key_path(&self) -> PathBuf { + if self.p2p_key_path.starts_with("~/") { + if let Some(home) = dirs_next::home_dir() { + return home.join(&self.p2p_key_path[2..]); + } + } + PathBuf::from(&self.p2p_key_path) + } + /// DB connections reserved for everything other than held write-locks: auth /// lookups, visibility-rule reads, the post-receive tail's own DB writes, and /// admin tooling. A write pins one pooled connection for its whole duration, so diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 80e28a4aa..14bcee32c 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -9,10 +9,11 @@ use std::collections::{hash_map::DefaultHasher, HashMap}; use std::hash::{Hash, Hasher}; +use std::path::Path; use std::sync::Arc; use std::time::Duration; -use anyhow::Result; +use anyhow::{Context, Result}; use chrono::Utc; use futures::StreamExt; use libp2p_core::{muxing::StreamMuxerBox, Multiaddr, PeerId, Transport}; @@ -164,6 +165,44 @@ struct GitlawbBehaviour { identify: identify::Behaviour, } +/// Load the node's persistent libp2p identity from `key_path`, generating and +/// storing a fresh Ed25519 keypair the first time. +pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { + if key_path.exists() { + let bytes = std::fs::read(key_path) + .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; + let kp = identity::Keypair::from_protobuf_encoding(&bytes) + .with_context(|| format!("invalid p2p key in {}", key_path.display()))?; + info!(path = %key_path.display(), "loaded existing p2p identity"); + Ok(kp) + } else { + let kp = identity::Keypair::generate_ed25519(); + let bytes = kp + .to_protobuf_encoding() + .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?; + + if let Some(parent) = key_path.parent() { + std::fs::create_dir_all(parent)?; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::write(key_path, &bytes)?; + std::fs::set_permissions(key_path, std::fs::Permissions::from_mode(0o600))?; + } + #[cfg(not(unix))] + std::fs::write(key_path, &bytes)?; + + info!( + path = %key_path.display(), + peer_id = %PeerId::from(kp.public()), + "generated new p2p identity" + ); + Ok(kp) + } +} + /// Start the libp2p swarm. Returns a handle for sending commands and the /// listening multiaddrs. Runs the event loop as a background tokio task /// that exits cleanly when `shutdown_rx` flips to `true`. @@ -443,6 +482,69 @@ pub async fn start( mod tests { use super::*; + #[test] + fn p2p_identity_not_derivable_from_did_alone() { + let dir_a = tempfile::tempdir().unwrap(); + let dir_b = tempfile::tempdir().unwrap(); + + let kp_a = load_or_create_p2p_keypair(&dir_a.path().join("p2p.key")).unwrap(); + let kp_b = load_or_create_p2p_keypair(&dir_b.path().join("p2p.key")).unwrap(); + + assert_ne!( + PeerId::from(kp_a.public()), + PeerId::from(kp_b.public()), + "two independent key files must yield different PeerIds" + ); + } + + #[test] + fn p2p_identity_stable_across_restarts() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + + let first = load_or_create_p2p_keypair(&path).unwrap(); + let second = load_or_create_p2p_keypair(&path).unwrap(); + + assert_eq!( + PeerId::from(first.public()), + PeerId::from(second.public()), + "the same key file must yield the same PeerId" + ); + } + + #[cfg(unix)] + #[test] + fn p2p_key_file_is_0600_on_unix() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("keys").join("p2p.key"); + + load_or_create_p2p_keypair(&path).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!( + mode & 0o777, + 0o600, + "key file must be owner-read/write only" + ); + } + + #[test] + fn p2p_corrupt_key_file_is_an_error_not_a_panic() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + std::fs::write(&path, [0xFFu8; 7]).unwrap(); + + let err = + load_or_create_p2p_keypair(&path).expect_err("a corrupt key file must be an error"); + let msg = format!("{err:#}"); + assert!( + msg.contains(&path.display().to_string()), + "error must name the key path, got: {msg}" + ); + } + #[test] fn ref_update_event_round_trip_with_owner_did() { let event = RefUpdateEvent { From f75b0f4afad536c9e042e2dea247ea1a34ec97bf Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:15:54 -0500 Subject: [PATCH 02/14] fix(node): load the libp2p identity from the persistent key file p2p::start now takes the Ed25519 keypair loaded by load_or_create_p2p_keypair instead of computing one from the node DID, so a node's network identity is generated once from the OS RNG and kept on disk rather than recomputed from a public value on every start. The node DID parameter is gone from start; the call site loads the key first and continues without p2p if the key file cannot be read, matching how a swarm-start failure is already handled. The gossipsub message_id_fn is untouched and keeps its own hasher. --- crates/gitlawb-node/src/main.rs | 36 ++++++++++++++++++------------ crates/gitlawb-node/src/p2p/mod.rs | 25 +++++---------------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 10aaca5b9..e5a830355 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -229,22 +229,30 @@ async fn main() -> Result<()> { .filter_map(|s| s.parse().ok()) .collect(); let shutdown_rx = shutdown_tx.subscribe(); - match p2p::start( - &node_did.to_string(), - config.p2p_port, - bootstrap_addrs, - Arc::clone(&db), - config.auto_sync, - shutdown_rx, - ) - .await - { - Ok(handle) => { - info!(port = config.p2p_port, peer_id = %handle.local_peer_id, "libp2p swarm started"); - Some(Arc::new(handle)) + match p2p::load_or_create_p2p_keypair(&config.resolved_p2p_key_path()) { + Ok(local_key) => { + match p2p::start( + local_key, + config.p2p_port, + bootstrap_addrs, + Arc::clone(&db), + config.auto_sync, + shutdown_rx, + ) + .await + { + Ok(handle) => { + info!(port = config.p2p_port, peer_id = %handle.local_peer_id, "libp2p swarm started"); + Some(Arc::new(handle)) + } + Err(e) => { + tracing::warn!(err = %e, "failed to start libp2p swarm — continuing without p2p"); + None + } + } } Err(e) => { - tracing::warn!(err = %e, "failed to start libp2p swarm — continuing without p2p"); + tracing::warn!(err = %e, "failed to load p2p identity key — continuing without p2p"); None } } diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 14bcee32c..49229be3b 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -4,8 +4,8 @@ //! - Peer discovery via Kademlia DHT (DID → multiaddr mapping) //! - Real-time ref-update events via Gossipsub //! -//! The node's PeerId is derived from its Ed25519 identity keypair, -//! so the gitlawb DID and libp2p PeerId share the same key. +//! The node's PeerId comes from an Ed25519 keypair loaded from a persistent +//! key file, so the PeerId is stable across restarts. use std::collections::{hash_map::DefaultHasher, HashMap}; use std::hash::{Hash, Hasher}; @@ -206,31 +206,16 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result /// Start the libp2p swarm. Returns a handle for sending commands and the /// listening multiaddrs. Runs the event loop as a background tokio task /// that exits cleanly when `shutdown_rx` flips to `true`. +/// `local_key` is the node's libp2p identity, loaded from the persistent key +/// file by [`load_or_create_p2p_keypair`]. pub async fn start( - node_did: &str, + local_key: identity::Keypair, listen_port: u16, bootstrap_addrs: Vec, db: Arc, auto_sync: bool, shutdown_rx: tokio::sync::watch::Receiver, ) -> Result { - // Derive a stable libp2p Ed25519 key from a seed based on the node DID. - // In production you'd load/persist this key alongside the identity PEM. - // For now we use the DID string as a deterministic seed. - let seed = { - let mut h = DefaultHasher::new(); - node_did.hash(&mut h); - h.finish() - }; - let mut seed_bytes = [0u8; 32]; - seed_bytes[..8].copy_from_slice(&seed.to_le_bytes()); - // Spread the seed across all bytes for better distribution - for i in 1..4 { - seed_bytes[i * 8..(i + 1) * 8].copy_from_slice(&seed.wrapping_add(i as u64).to_le_bytes()); - } - - let local_key = identity::Keypair::ed25519_from_bytes(seed_bytes) - .map_err(|e| anyhow::anyhow!("failed to create p2p keypair: {e}"))?; let local_peer_id = PeerId::from(local_key.public()); info!(peer_id = %local_peer_id, "libp2p identity"); From 532ffc7202d1c5907916f9bf4ad18e65b1026abc Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:43:56 -0500 Subject: [PATCH 03/14] fix(node): create the p2p key with 0600 at creation and reject a loose one Open the key file with create_new and the mode set at creation, then fsync, instead of writing it and narrowing the mode afterwards. The secret is never on disk under a wider mode, an interrupted start cannot leave it readable, and the exclusive open also refuses a pre-existing entry at the path and makes a concurrent start take the key that landed rather than clobber it. Refuse to load a key file whose mode grants group or other access, and name the observed mode so the operator can fix it. Report an empty key file as empty rather than surfacing a protobuf decode error that blames a missing rsa feature. Pin GITLAWB_P2P_KEY onto the mounted volume in the Docker and fly configs and document it, so the key does not depend on home-directory resolution to land on persistent storage. --- .env.example | 5 + Dockerfile | 1 + README.md | 1 + crates/gitlawb-node/src/p2p/mod.rs | 197 +++++++++++++++++++++++++---- infra/fly/fly.toml | 1 + infra/fly/gitlawb-node-2.fly.toml | 1 + infra/fly/gitlawb-node-3.fly.toml | 1 + 7 files changed, 181 insertions(+), 26 deletions(-) diff --git a/.env.example b/.env.example index b70d11172..77527ed9e 100644 --- a/.env.example +++ b/.env.example @@ -7,6 +7,11 @@ # Generate with: gl identity new GITLAWB_KEY=/data/keys/identity.pem +# Path to the node's persistent libp2p identity key file. Generated on first +# start with owner-only permissions; keep it on a persistent volume so the +# PeerId survives redeploys. Default: ~/.gitlawb/p2p.key +#GITLAWB_P2P_KEY=/data/keys/p2p.key + # Publicly reachable URL of this node (used in peer announcements) GITLAWB_PUBLIC_URL=https://your-node.example.com diff --git a/Dockerfile b/Dockerfile index 3b4669453..f030de508 100644 --- a/Dockerfile +++ b/Dockerfile @@ -75,6 +75,7 @@ WORKDIR /data ENV GITLAWB_REPOS_DIR=/data/repos \ GITLAWB_KEY=/data/keys/identity.pem \ + GITLAWB_P2P_KEY=/data/keys/p2p.key \ GITLAWB_HOST=0.0.0.0 \ GITLAWB_PORT=7545 \ GITLAWB_P2P_PORT=7546 diff --git a/README.md b/README.md index 1588161fb..fd132e5c4 100644 --- a/README.md +++ b/README.md @@ -339,6 +339,7 @@ Important node settings: | `GITLAWB_P2P_PORT` | libp2p QUIC/UDP port. Use `0` to disable. | | `GITLAWB_BOOTSTRAP_PEERS` | Comma-separated HTTP peer URLs. | | `GITLAWB_P2P_BOOTSTRAP` | Comma-separated libp2p multiaddrs. | +| `GITLAWB_P2P_KEY` | Path to the node's persistent libp2p identity key file, which fixes the PeerId across restarts. Created with owner-only permissions on first start. Default `~/.gitlawb/p2p.key`; point it at a persistent volume when running in a container. | | `GITLAWB_BOOTSTRAP_DISABLE_SEEDS` | Disable embedded seed peers for isolated dev/test networks. | | `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. | | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. | diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 49229be3b..00a2f76a8 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -169,38 +169,99 @@ struct GitlawbBehaviour { /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { if key_path.exists() { - let bytes = std::fs::read(key_path) - .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; - let kp = identity::Keypair::from_protobuf_encoding(&bytes) - .with_context(|| format!("invalid p2p key in {}", key_path.display()))?; - info!(path = %key_path.display(), "loaded existing p2p identity"); - Ok(kp) - } else { - let kp = identity::Keypair::generate_ed25519(); - let bytes = kp - .to_protobuf_encoding() - .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?; + return read_p2p_keypair(key_path); + } + + let kp = identity::Keypair::generate_ed25519(); + let bytes = kp + .to_protobuf_encoding() + .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?; + + if let Some(parent) = key_path.parent() { + std::fs::create_dir_all(parent)?; + } - if let Some(parent) = key_path.parent() { - std::fs::create_dir_all(parent)?; + match create_new_key_file(key_path, &bytes) { + Ok(()) => { + info!( + path = %key_path.display(), + peer_id = %PeerId::from(kp.public()), + "generated new p2p identity" + ); + Ok(kp) } + // Something already occupies the path: another node process won the + // race between the existence check and the exclusive create, or the + // path is a symlink. Whatever is on disk is the identity of record, so + // read it back rather than failing the boot or overwriting it. + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => read_p2p_keypair(key_path), + Err(e) => Err(anyhow::Error::new(e) + .context(format!("failed to write p2p key to {}", key_path.display()))), + } +} - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::write(key_path, &bytes)?; - std::fs::set_permissions(key_path, std::fs::Permissions::from_mode(0o600))?; +/// Create the key file exclusively, with owner-only permissions applied at +/// creation time so the bytes are never visible to other users. `create_new` +/// maps to `O_EXCL`, so an existing path entry (including a dangling symlink) +/// is refused rather than followed or truncated. +fn create_new_key_file(key_path: &Path, bytes: &[u8]) -> std::io::Result<()> { + use std::io::Write; + + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + + let mut file = opts.open(key_path)?; + file.write_all(bytes)?; + file.sync_all() +} + +/// Read an existing key file, refusing one whose permissions or contents make +/// it untrustworthy. Never regenerates: a node that silently replaces an +/// unreadable key file would change its PeerId without the operator knowing. +fn read_p2p_keypair(key_path: &Path) -> Result { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + let mode = std::fs::metadata(key_path) + .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))? + .permissions() + .mode() + & 0o777; + if mode & 0o077 != 0 { + anyhow::bail!( + "p2p key at {} has mode {:04o}, which grants access beyond its owner; \ + run `chmod 600 {}` or delete the file to regenerate the identity", + key_path.display(), + mode, + key_path.display() + ); } - #[cfg(not(unix))] - std::fs::write(key_path, &bytes)?; + } - info!( - path = %key_path.display(), - peer_id = %PeerId::from(kp.public()), - "generated new p2p identity" + let bytes = std::fs::read(key_path) + .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; + + // An empty file decodes as a valid protobuf with a key type of RSA, so + // without this the operator gets a misleading complaint about a missing + // `rsa` cargo feature instead of being told the file is empty. + if bytes.is_empty() { + anyhow::bail!( + "p2p key file {} is empty; restore it from backup, \ + or delete it to regenerate the identity", + key_path.display() ); - Ok(kp) } + + let kp = identity::Keypair::from_protobuf_encoding(&bytes) + .with_context(|| format!("invalid p2p key in {}", key_path.display()))?; + info!(path = %key_path.display(), "loaded existing p2p identity"); + Ok(kp) } /// Start the libp2p swarm. Returns a handle for sending commands and the @@ -505,7 +566,15 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("keys").join("p2p.key"); - load_or_create_p2p_keypair(&path).unwrap(); + // Create the key under a fully permissive umask, otherwise a restrictive + // ambient umask masks the bits down to 0600 on its own and the assertion + // below passes whether or not the code pins the mode. + // SAFETY: `umask` is always safe to call; it only reads and replaces the + // process-wide value. + let prev_umask = unsafe { libc::umask(0o000) }; + let result = load_or_create_p2p_keypair(&path); + unsafe { libc::umask(prev_umask) }; + result.unwrap(); let mode = std::fs::metadata(&path).unwrap().permissions().mode(); assert_eq!( @@ -515,11 +584,83 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn p2p_key_file_with_loose_permissions_is_rejected() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + + let kp = identity::Keypair::generate_ed25519(); + std::fs::write(&path, kp.to_protobuf_encoding().unwrap()).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let err = load_or_create_p2p_keypair(&path) + .expect_err("a group/world-readable key file must be an error"); + let msg = format!("{err:#}"); + assert!( + msg.contains(&path.display().to_string()) && msg.contains("0644"), + "error must name the key path and the observed mode, got: {msg}" + ); + // The rejection must not have regenerated the identity behind the + // operator's back. + let on_disk = std::fs::read(&path).unwrap(); + assert_eq!(on_disk, kp.to_protobuf_encoding().unwrap()); + } + + #[test] + fn p2p_empty_key_file_reports_the_file_as_empty() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + std::fs::write(&path, b"").unwrap(); + // Keep the permission guard out of the way so this exercises the + // empty-file path and not the mode check. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + } + + let err = load_or_create_p2p_keypair(&path).expect_err("an empty key file must be an error"); + let msg = format!("{err:#}"); + assert!( + msg.contains(&path.display().to_string()) && msg.contains("empty"), + "error must name the key path and say the file is empty, got: {msg}" + ); + assert!( + !msg.contains("rsa"), + "an empty file must not be reported as an RSA decoding problem, got: {msg}" + ); + } + + #[cfg(unix)] + #[test] + fn p2p_dangling_symlink_does_not_write_through_to_the_target() { + let dir = tempfile::tempdir().unwrap(); + let link = dir.path().join("p2p.key"); + let target = dir.path().join("elsewhere.key"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + load_or_create_p2p_keypair(&link).expect_err("a dangling symlink must not be followed"); + assert!( + !target.exists(), + "no key may be written through the symlink to {}", + target.display() + ); + } + #[test] fn p2p_corrupt_key_file_is_an_error_not_a_panic() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("p2p.key"); std::fs::write(&path, [0xFFu8; 7]).unwrap(); + // Keep the permission guard out of the way so this exercises decoding. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + } let err = load_or_create_p2p_keypair(&path).expect_err("a corrupt key file must be an error"); @@ -528,6 +669,10 @@ mod tests { msg.contains(&path.display().to_string()), "error must name the key path, got: {msg}" ); + assert!( + msg.contains("invalid p2p key"), + "a corrupt key must be reported as a decoding failure, got: {msg}" + ); } #[test] diff --git a/infra/fly/fly.toml b/infra/fly/fly.toml index 05445e5a3..ffda6e95b 100644 --- a/infra/fly/fly.toml +++ b/infra/fly/fly.toml @@ -12,6 +12,7 @@ primary_region = "iad" GITLAWB_P2P_PORT = "7546" GITLAWB_REPOS_DIR = "/data/repos" GITLAWB_KEY = "/data/keys/identity.pem" + GITLAWB_P2P_KEY = "/data/keys/p2p.key" GITLAWB_PUBLIC_URL = "https://gitlawb-node-test.fly.dev" GITLAWB_BOOTSTRAP_PEERS = "https://node.gitlawb.com,https://node2.gitlawb.com,https://node3.gitlawb.com" GITLAWB_AUTO_SYNC = "true" diff --git a/infra/fly/gitlawb-node-2.fly.toml b/infra/fly/gitlawb-node-2.fly.toml index 785e4da7b..16037afe9 100644 --- a/infra/fly/gitlawb-node-2.fly.toml +++ b/infra/fly/gitlawb-node-2.fly.toml @@ -15,6 +15,7 @@ primary_region = 'sjc' GITLAWB_HOST = '0.0.0.0' GITLAWB_KEY = '/data/keys/identity.pem' GITLAWB_MAX_PACK_BYTES = '524288000' + GITLAWB_P2P_KEY = '/data/keys/p2p.key' GITLAWB_P2P_PORT = '7546' GITLAWB_PORT = '7545' GITLAWB_PUBLIC_URL = 'https://node2.gitlawb.com' diff --git a/infra/fly/gitlawb-node-3.fly.toml b/infra/fly/gitlawb-node-3.fly.toml index d1ca979aa..85d49302d 100644 --- a/infra/fly/gitlawb-node-3.fly.toml +++ b/infra/fly/gitlawb-node-3.fly.toml @@ -15,6 +15,7 @@ primary_region = 'nrt' GITLAWB_HOST = '0.0.0.0' GITLAWB_KEY = '/data/keys/identity.pem' GITLAWB_MAX_PACK_BYTES = '524288000' + GITLAWB_P2P_KEY = '/data/keys/p2p.key' GITLAWB_P2P_PORT = '7546' GITLAWB_PORT = '7545' GITLAWB_PUBLIC_URL = 'https://node3.gitlawb.com' From f7fcd5edf5cb92d257879273ef82eca1d194c918 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:20:03 -0500 Subject: [PATCH 04/14] fix(node): publish the p2p key atomically and pin its directory Write the key to a scratch file in the same directory and hard-link it onto the final path. The bytes are durable before any name points at them, so a crash cannot leave a partial key that fails to load on the next start and takes the node off the network until someone reads the logs. A concurrent reader can no longer observe a half-written file either, since the final name appears complete or not at all. hard_link rather than rename: rename replaces its destination silently, so refusing to clobber an existing key would depend on a check followed by a separate rename, and a concurrent start can land in that gap. hard_link is atomic and refuses an occupied path, including a symlink, which it does not follow. Create the key directory 0700 and tighten it when an existing one grants group or other access. A 0600 key under a writable directory can still be replaced or unlinked. Tightening rather than refusing to start, because existing installs already have 0755 there and refusing would take p2p down on all of them through a path that only warns. Formatting on the branch is swept up here; it was already failing cargo fmt --check before this change. --- crates/gitlawb-node/src/main.rs | 5 + crates/gitlawb-node/src/p2p/mod.rs | 260 ++++++++++++++++++++++++++--- 2 files changed, 244 insertions(+), 21 deletions(-) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index e5a830355..283b1302f 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -251,6 +251,11 @@ async fn main() -> Result<()> { } } } + // Deliberately non-fatal, and the cost is worth naming: an + // unreadable key file takes the node off the p2p network for the + // whole run while /health keeps reporting healthy, so the outage is + // visible only to whoever reads the logs. Making it fatal, or + // surfacing it in the health response, is its own change. Err(e) => { tracing::warn!(err = %e, "failed to load p2p identity key — continuing without p2p"); None diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 00a2f76a8..809113505 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -168,6 +168,13 @@ struct GitlawbBehaviour { /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { + // Runs on both the load and the create path: the directory guards the key + // just as much as the key's own mode does, and an existing directory keeps + // whatever mode it was made with. + if let Some(parent) = key_path.parent().filter(|p| !p.as_os_str().is_empty()) { + ensure_key_dir(parent)?; + } + if key_path.exists() { return read_p2p_keypair(key_path); } @@ -177,11 +184,7 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result .to_protobuf_encoding() .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?; - if let Some(parent) = key_path.parent() { - std::fs::create_dir_all(parent)?; - } - - match create_new_key_file(key_path, &bytes) { + match write_key_atomically(key_path, &bytes) { Ok(()) => { info!( path = %key_path.display(), @@ -191,33 +194,170 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result Ok(kp) } // Something already occupies the path: another node process won the - // race between the existence check and the exclusive create, or the - // path is a symlink. Whatever is on disk is the identity of record, so - // read it back rather than failing the boot or overwriting it. + // race between the existence check and the atomic publish, or the path + // is a symlink. Whatever is on disk is the identity of record, so read + // it back rather than failing the boot or overwriting it. Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => read_p2p_keypair(key_path), Err(e) => Err(anyhow::Error::new(e) .context(format!("failed to write p2p key to {}", key_path.display()))), } } -/// Create the key file exclusively, with owner-only permissions applied at -/// creation time so the bytes are never visible to other users. `create_new` -/// maps to `O_EXCL`, so an existing path entry (including a dangling symlink) -/// is refused rather than followed or truncated. -fn create_new_key_file(key_path: &Path, bytes: &[u8]) -> std::io::Result<()> { - use std::io::Write; +/// Create the directory holding the key with owner-only permissions, and +/// tighten it if it already exists with a looser mode. Write permission on this +/// directory is enough to unlink or replace the 0600 key inside it, so the +/// directory guards the key as much as the key's own mode does. +/// +/// `create_dir_all` takes 0777 masked by the umask, which lands 0755 under a +/// normal umask and 0777 under a permissive one. `DirBuilder`'s mode fixes that +/// for directories it creates, but an existing directory keeps whatever mode it +/// was made with, so the load path has to check too. +/// +/// A loose existing directory is repaired rather than rejected. Rejecting it +/// would refuse to boot on every node whose directory already landed 0755, +/// which is the common case, and through `main.rs`'s non-fatal handling that +/// would read as a silent p2p outage rather than a clear failure. Tightening +/// applies exactly the remedy the alternative would have asked the operator to +/// run by hand. Failure to tighten is fatal, since at that point the key cannot +/// be protected. +/// +/// `~/.gitlawb/identity.pem` lives in this directory too, so this covers both +/// keys. Issue #231 owns the sibling gap in `main.rs`'s own creation path for +/// that file; nothing here touches it. +fn ensure_key_dir(dir: &Path) -> Result<()> { + let mut builder = std::fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + // On non-unix this is exactly `create_dir_all`; there is no mode to pin. + builder + .create(dir) + .with_context(|| format!("failed to create key directory {}", dir.display()))?; - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create_new(true); #[cfg(unix)] { - use std::os::unix::fs::OpenOptionsExt; - opts.mode(0o600); + use std::os::unix::fs::PermissionsExt; + + let mode = std::fs::metadata(dir) + .with_context(|| format!("failed to stat key directory {}", dir.display()))? + .permissions() + .mode() + & 0o777; + if mode & 0o077 != 0 { + warn!( + dir = %dir.display(), + mode = format!("{mode:04o}"), + "key directory grants access beyond its owner; tightening it to 0700" + ); + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)).with_context( + || { + format!( + "key directory {} has mode {:04o}, which lets other users replace \ + the keys it holds, and it could not be tightened; run `chmod 700 {}`", + dir.display(), + mode, + dir.display() + ) + }, + )?; + } + } + + Ok(()) +} + +/// Write the key to a scratch file in the same directory, then publish it to +/// `key_path` in one atomic step, so no reader ever sees a partial key and a +/// crash mid-write cannot leave a truncated file at the final path. +/// +/// The publish is `link(2)`, not `rename(2)`. Rename would replace an existing +/// key silently, throwing away the `O_EXCL` protection the previous code got +/// from `create_new`; guarding it with an existence check first only narrows +/// the window rather than closing it, since a concurrent start can land its own +/// key between the check and the rename. `hard_link` is atomic and fails with +/// `AlreadyExists` if anything already occupies the path (a real file, or a +/// symlink, which it does not follow), so the two properties hold together +/// without a check-then-act gap. The scratch file is unlinked either way, so a +/// failed start leaves the key directory as it found it. +fn write_key_atomically(key_path: &Path, bytes: &[u8]) -> std::io::Result<()> { + let dir = key_path.parent().unwrap_or_else(|| Path::new(".")); + let (tmp_path, mut file) = create_scratch_key_file(dir)?; + let result = fill_and_publish(&mut file, bytes, &tmp_path, key_path); + drop(file); + // Unconditional: on success the key is reachable through `key_path`, and on + // failure nothing may be left behind. + let _ = std::fs::remove_file(&tmp_path); + result +} + +/// Open a uniquely named scratch file in `dir` with owner-only permissions +/// applied at creation time. The name carries the pid so concurrent node starts +/// do not pick the same one, and `create_new` (`O_EXCL`) plus the retry makes a +/// collision with a leftover or a sibling thread impossible rather than merely +/// unlikely. +fn create_scratch_key_file(dir: &Path) -> std::io::Result<(std::path::PathBuf, std::fs::File)> { + let pid = std::process::id(); + for attempt in 0..64u32 { + let tmp_path = dir.join(format!(".p2p.key.{pid}.{attempt}.tmp")); + + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + + match opts.open(&tmp_path) { + Ok(file) => return Ok((tmp_path, file)), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + format!("no free scratch key file name in {}", dir.display()), + )) +} + +fn fill_and_publish( + file: &mut std::fs::File, + bytes: &[u8], + tmp_path: &Path, + key_path: &Path, +) -> std::io::Result<()> { + use std::io::Write; + + #[cfg(test)] + if FAIL_KEY_WRITE.with(|f| f.get()) { + file.write_all(&bytes[..bytes.len() / 2])?; + return Err(std::io::Error::other("injected key-write failure")); } - let mut file = opts.open(key_path)?; file.write_all(bytes)?; - file.sync_all() + // The bytes must be durable before the name that points at them appears, + // otherwise a crash can leave the entry pointing at an empty file. + file.sync_all()?; + std::fs::hard_link(tmp_path, key_path)?; + + // Make the new directory entry itself durable. Best-effort: the key is + // already written and linked, and not every platform allows this. + if let Some(dir) = key_path.parent() { + if let Ok(dir_file) = std::fs::File::open(dir) { + let _ = dir_file.sync_all(); + } + } + Ok(()) +} + +#[cfg(test)] +thread_local! { + /// Test-only fault injection for the key write. Thread-local so an armed + /// test cannot disturb the others running beside it. + static FAIL_KEY_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; } /// Read an existing key file, refusing one whose permissions or contents make @@ -582,6 +722,83 @@ mod tests { 0o600, "key file must be owner-read/write only" ); + + // The directory was created inside the same permissive-umask window, so + // this proves the directory mode is pinned by the code and not by the + // ambient umask. Write permission on the directory alone is enough to + // unlink or replace the 0600 key inside it. + let dir_mode = std::fs::metadata(path.parent().unwrap()) + .unwrap() + .permissions() + .mode(); + assert_eq!(dir_mode & 0o777, 0o700, "key directory must be owner-only"); + } + + #[cfg(unix)] + #[test] + fn p2p_existing_key_dir_with_loose_permissions_is_tightened() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let key_dir = dir.path().join("keys"); + std::fs::create_dir(&key_dir).unwrap(); + std::fs::set_permissions(&key_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let path = key_dir.join("p2p.key"); + + // Creation path: a pre-existing loose directory is detected and repaired. + let created = load_or_create_p2p_keypair(&path).expect("boot must not fail on a loose dir"); + assert_eq!( + std::fs::metadata(&key_dir).unwrap().permissions().mode() & 0o777, + 0o700, + "an existing loose key directory must be tightened" + ); + + // Load path: same check, on a directory loosened after the key exists. + std::fs::set_permissions(&key_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let loaded = load_or_create_p2p_keypair(&path).expect("reload must not fail"); + assert_eq!( + std::fs::metadata(&key_dir).unwrap().permissions().mode() & 0o777, + 0o700, + "the load path must tighten the key directory too" + ); + assert_eq!( + PeerId::from(created.public()), + PeerId::from(loaded.public()), + "tightening must not change the identity" + ); + } + + #[test] + fn p2p_failed_key_write_leaves_no_file_at_the_final_path() { + let dir = tempfile::tempdir().unwrap(); + let key_dir = dir.path().join("keys"); + let path = key_dir.join("p2p.key"); + + FAIL_KEY_WRITE.with(|f| f.set(true)); + let result = load_or_create_p2p_keypair(&path); + FAIL_KEY_WRITE.with(|f| f.set(false)); + + result.expect_err("an interrupted key write must not report success"); + assert!( + !path.exists(), + "a partially written key must never be observable at {}", + path.display() + ); + + // Nor may a half-written scratch file be left behind for an operator to + // trip over on the next boot. + let leftovers: Vec<_> = std::fs::read_dir(&key_dir) + .map(|rd| rd.filter_map(|e| e.ok()).map(|e| e.path()).collect()) + .unwrap_or_default(); + assert!( + leftovers.is_empty(), + "a failed write must clean up after itself, found: {leftovers:?}" + ); + + // The next boot must be able to create the identity normally. + let kp = load_or_create_p2p_keypair(&path).expect("a retry after a failed write must work"); + let reloaded = load_or_create_p2p_keypair(&path).unwrap(); + assert_eq!(PeerId::from(kp.public()), PeerId::from(reloaded.public())); } #[cfg(unix)] @@ -622,7 +839,8 @@ mod tests { std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); } - let err = load_or_create_p2p_keypair(&path).expect_err("an empty key file must be an error"); + let err = + load_or_create_p2p_keypair(&path).expect_err("an empty key file must be an error"); let msg = format!("{err:#}"); assert!( msg.contains(&path.display().to_string()) && msg.contains("empty"), From 2a69a0bdfcfbc17841bcde23f6e5f985f723512e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:39:33 -0500 Subject: [PATCH 05/14] style(node): comma instead of a dash in the key-load warning House style avoids em dashes in text we write. The swarm-failure warning beside it predates this branch and is left alone. --- crates/gitlawb-node/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 283b1302f..bb612298a 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -257,7 +257,7 @@ async fn main() -> Result<()> { // visible only to whoever reads the logs. Making it fatal, or // surfacing it in the health response, is its own change. Err(e) => { - tracing::warn!(err = %e, "failed to load p2p identity key — continuing without p2p"); + tracing::warn!(err = %e, "failed to load p2p identity key, continuing without p2p"); None } } From 31b117646735e7fa07d2b12c8e4e7b8c39fd6b11 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:04:23 -0500 Subject: [PATCH 06/14] fix(node): require the p2p key path to name a directory A bare filename in GITLAWB_P2P_KEY put the key in whatever directory the process started from, and the directory guard was skipped entirely on that path: Path::parent returns Some("") for a bare filename, which the caller filtered out before ever reaching ensure_key_dir. The key file was created 0600 inside a directory that kept whatever mode it already had. Config::validate now rejects a p2p key path that names no directory, so the node says so at boot instead of starting with a key it cannot protect. That placement is the point: an error raised in the p2p start path is logged and stepped over, leaving the node running without p2p and reporting healthy. The check is lexical on the tilde-resolved path. canonicalize would fail on a parent that does not exist yet, which is the shipped ~/.gitlawb default and every container's first boot, and comparing against the working directory would reject /data/p2p.key under the image's WORKDIR, an absolute directory the operator did name. Three sites answered the parent question differently, which is how the gap arose: one filtered the empty case out, one already normalized it, and one opened "" and silently skipped its fsync. They now share key_parent, and Config::validate calls it rather than adding a fourth answer. load_or_create_p2p_keypair also refuses a path naming no directory. That is a backstop behind the config gate, not the gate, so a later caller that skips validation cannot quietly restore the old behaviour. --- crates/gitlawb-node/src/config.rs | 89 +++++++++++++++++++++++++++++- crates/gitlawb-node/src/p2p/mod.rs | 55 ++++++++++++++---- 2 files changed, 133 insertions(+), 11 deletions(-) diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 7104054b5..0f63e756e 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -1,5 +1,5 @@ use clap::Parser; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// Upper bound on `git_service_timeout_secs` and `ipfs_request_budget_secs`, in seconds /// (100 years). @@ -597,6 +597,32 @@ impl Config { floor )); } + + // A p2p key path naming no directory puts the node's private key in + // whatever directory the process was started from. The node cannot + // protect that: `ensure_key_dir` would have to chmod a directory the + // operator never nominated as a key directory, and a directory it + // cannot secure is one where any local user with write access can + // replace the key and choose the node's libp2p identity. Refuse it here, + // where the denial actually stops the process, rather than in the p2p + // start path, where main.rs logs the error and keeps serving with a + // green /health. + // + // Decided lexically on the resolved path: `canonicalize` would fail on a + // parent that does not exist yet (the shipped `~/.gitlawb` default, and + // every container's first boot), and comparing against the process + // working directory would reject `/data/p2p.key` under the image's + // WORKDIR, an absolute directory the operator did name. + let p2p_key_path = self.resolved_p2p_key_path(); + if crate::p2p::key_parent(&p2p_key_path) == Path::new(".") { + return Err(format!( + "GITLAWB_P2P_KEY ({}) must include a directory, such as ./keys/p2p.key or \ + /data/keys/p2p.key: the node will not store its p2p identity key in the \ + working directory, where the directory holding it cannot be secured.", + self.p2p_key_path + )); + } + Ok(()) } } @@ -987,4 +1013,65 @@ mod tests { "db_max_connections at the floor (pushes + headroom) must validate" ); } + + fn config_with_p2p_key(path: &str) -> Config { + Config::parse_from(["gitlawb-node", "--p2p-key-path", path]) + } + + /// A p2p key path that names no directory component would put the node's + /// private key in whatever directory the process happens to be started from, + /// which `ensure_key_dir` cannot protect without tightening a directory the + /// operator never nominated. Reject it at boot instead. + #[test] + fn p2p_key_path_without_a_directory_component_is_rejected() { + for path in ["p2p.key", "./p2p.key", "././p2p.key", "p2p.key/", ""] { + let err = config_with_p2p_key(path) + .validate() + .expect_err(&format!("{path:?} names no directory and must be rejected")); + assert!( + err.contains("directory"), + "{path:?} must be rejected for naming no directory, got: {err}" + ); + } + } + + /// The mirror of the above, and the case that stops the predicate widening + /// into "reject every relative path". The shipped default is included on + /// purpose: a predicate that rejects it is a boot failure for every node. + #[test] + fn p2p_key_path_naming_a_directory_is_accepted() { + for path in [ + "keys/p2p.key", + "./keys/p2p.key", + "/data/keys/p2p.key", + "../p2p.key", + "/data/p2p.key", + "~/.gitlawb/p2p.key", + ] { + assert!( + config_with_p2p_key(path).validate().is_ok(), + "{path:?} names a directory and must be accepted" + ); + } + + Config::parse_from(["gitlawb-node"]) + .validate() + .expect("the shipped default p2p key path must validate"); + } + + /// The one input that separates validating the raw config string from + /// validating `resolved_p2p_key_path()`. Raw, `~/` has an empty parent and + /// would be rejected; resolved, it is the home directory, whose parent is a + /// real directory, so it is accepted. Every other tilde path is accepted + /// under both readings and therefore proves nothing. + #[test] + fn p2p_key_path_is_checked_after_tilde_expansion() { + if dirs_next::home_dir().is_none() { + panic!("this test needs a home directory to distinguish raw from resolved"); + } + assert!( + config_with_p2p_key("~/").validate().is_ok(), + "`~/` resolves to the home directory, whose parent is a real directory" + ); + } } diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 809113505..26450843f 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -9,7 +9,7 @@ use std::collections::{hash_map::DefaultHasher, HashMap}; use std::hash::{Hash, Hasher}; -use std::path::Path; +use std::path::{Component, Path}; use std::sync::Arc; use std::time::Duration; @@ -165,15 +165,50 @@ struct GitlawbBehaviour { identify: identify::Behaviour, } +/// The directory holding `key_path`, and the single answer to that question for +/// every site in this module plus `Config::validate`. +/// +/// `Path::parent` is not enough on its own. A bare filename yields `Some("")` +/// and `./p2p.key` yields `Some(".")`, both naming the process working +/// directory while looking different; an empty path yields `None`. Collapsing +/// all of those to `.` keeps the callers from each inventing their own answer, +/// which is what they used to do: one filtered the empty case out and skipped +/// the directory guard entirely, one already normalized correctly, and one +/// opened `""` and silently did nothing. +/// +/// The `.` return is the "names no directory" signal, not a usable directory. +/// `Config::validate` rejects a p2p key path that lands here, so a validated +/// config never reaches it; `load_or_create_p2p_keypair` refuses it as well, as +/// a backstop rather than the gate. +pub(crate) fn key_parent(key_path: &Path) -> &Path { + match key_path.parent() { + Some(parent) if parent.components().any(|c| c != Component::CurDir) => parent, + _ => Path::new("."), + } +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { + let parent = key_parent(key_path); + + // Backstop, not the gate. `Config::validate` rejects a key path naming no + // directory before the node starts, which is where the operator gets a + // useful error. Refusing it here too means a future caller that skips + // config validation cannot quietly resurrect the old behaviour of writing + // the key into the working directory and chmodding whatever that happens + // to be. + if parent == Path::new(".") { + return Err(anyhow::anyhow!( + "p2p key path {} names no directory; give it one, such as ./keys/p2p.key", + key_path.display() + )); + } + // Runs on both the load and the create path: the directory guards the key // just as much as the key's own mode does, and an existing directory keeps // whatever mode it was made with. - if let Some(parent) = key_path.parent().filter(|p| !p.as_os_str().is_empty()) { - ensure_key_dir(parent)?; - } + ensure_key_dir(parent)?; if key_path.exists() { return read_p2p_keypair(key_path); @@ -283,7 +318,7 @@ fn ensure_key_dir(dir: &Path) -> Result<()> { /// without a check-then-act gap. The scratch file is unlinked either way, so a /// failed start leaves the key directory as it found it. fn write_key_atomically(key_path: &Path, bytes: &[u8]) -> std::io::Result<()> { - let dir = key_path.parent().unwrap_or_else(|| Path::new(".")); + let dir = key_parent(key_path); let (tmp_path, mut file) = create_scratch_key_file(dir)?; let result = fill_and_publish(&mut file, bytes, &tmp_path, key_path); drop(file); @@ -344,11 +379,11 @@ fn fill_and_publish( std::fs::hard_link(tmp_path, key_path)?; // Make the new directory entry itself durable. Best-effort: the key is - // already written and linked, and not every platform allows this. - if let Some(dir) = key_path.parent() { - if let Ok(dir_file) = std::fs::File::open(dir) { - let _ = dir_file.sync_all(); - } + // already written and linked, and not every platform allows this. Goes + // through `key_parent` like every other site; opening a bare `""` here used + // to fail silently, which looked like a working fsync and was not. + if let Ok(dir_file) = std::fs::File::open(key_parent(key_path)) { + let _ = dir_file.sync_all(); } Ok(()) } From 806c262fae3b57d88a26ed4e37a09f51445c1d0f Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:47:06 -0500 Subject: [PATCH 07/14] test(node): run the key-permission probe in its own process The probe zeroes the umask so the assertion means something: under a restrictive ambient umask the bits are masked to 0600 regardless of whether the code pins the mode, and the check passes either way. Zeroing it in the shared test process is the problem. umask is process-global and cargo runs these tests on threads, so any test creating a file in that window inherits 000. Measured before this change: an unrelated concurrent test's file was created 0666. The probe now runs in a child process, where the zeroed umask cannot reach a sibling and dies with the child. The parent is an ordinary test that runs concurrently with everything else. Double-gated with #[ignore] plus an env check so a bare --ignored sweep does not zero the umask in the shared process after all. The parent asserts the child ran exactly one test and that it passed, not just that it exited 0. A libtest filter matching nothing runs zero tests and still exits 0, so without that assertion a renamed fixture would read as a green permission check while asserting nothing. Verified by pointing the filter at a name that does not exist and watching the parent fail. --- crates/gitlawb-node/src/p2p/mod.rs | 80 ++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 26450843f..f6e29d165 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -733,23 +733,56 @@ mod tests { ); } + // ---- Permission probe, run in a child process ------------------------- + // + // The probe has to create the key under a zeroed umask, otherwise a + // restrictive ambient umask masks the bits down to 0600 by itself and the + // assertion passes whether or not the code pins the mode. That zeroing is + // the problem: `umask` is process-global and cargo runs these tests on + // threads, so any test creating a file in that window inherits 000. Measured + // before this change, an unrelated concurrent test's file was created 0666. + // + // So the probe runs in a dedicated child process, where the zeroed umask + // cannot reach a sibling and dies with the child. The parent test below is + // an ordinary `#[test]` that runs concurrently with everything else. + // + // Two halves, and the split is worth naming: the child's assertions are the + // committed deterministic guard, and the concurrency leak itself was proven + // out of band by a throwaway probe rather than by a committed test. A race + // on process-global state has no reliable committed red-green. + + /// Re-invoke this test binary to run one `#[ignore]`d fixture test. + #[cfg(unix)] + fn fixture_command(fixture_test: &str) -> std::process::Command { + let mut cmd = std::process::Command::new(std::env::current_exe().expect("current_exe")); + cmd.args([fixture_test, "--exact", "--ignored", "--nocapture"]) + .env("GITLAWB_TEST_FIXTURE", "p2p-key-perms"); + cmd + } + + /// Fixture: create the key under a zeroed umask and assert the modes the + /// code is supposed to pin. Double-gated so it is inert unless the parent + /// invoked it: `#[ignore]` keeps it out of a normal run, and the env check + /// keeps it inert even under a bare `--ignored` sweep, which would otherwise + /// zero the umask inside the shared test process. #[cfg(unix)] #[test] - fn p2p_key_file_is_0600_on_unix() { + #[ignore = "self-exec fixture: only runs under GITLAWB_TEST_FIXTURE=p2p-key-perms"] + fn fixture_p2p_key_perms_under_zero_umask() { use std::os::unix::fs::PermissionsExt; + if std::env::var("GITLAWB_TEST_FIXTURE").ok().as_deref() != Some("p2p-key-perms") { + return; + } + let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("keys").join("p2p.key"); - // Create the key under a fully permissive umask, otherwise a restrictive - // ambient umask masks the bits down to 0600 on its own and the assertion - // below passes whether or not the code pins the mode. - // SAFETY: `umask` is always safe to call; it only reads and replaces the - // process-wide value. - let prev_umask = unsafe { libc::umask(0o000) }; - let result = load_or_create_p2p_keypair(&path); - unsafe { libc::umask(prev_umask) }; - result.unwrap(); + // SAFETY: `umask` only reads and replaces the process-wide value, and + // this process exists solely for this probe. No restore: the value dies + // with the child. + unsafe { libc::umask(0o000) }; + load_or_create_p2p_keypair(&path).expect("key creation under a permissive umask"); let mode = std::fs::metadata(&path).unwrap().permissions().mode(); assert_eq!( @@ -769,6 +802,33 @@ mod tests { assert_eq!(dir_mode & 0o777, 0o700, "key directory must be owner-only"); } + #[cfg(unix)] + #[test] + fn p2p_key_file_is_0600_on_unix() { + let output = fixture_command("p2p::tests::fixture_p2p_key_perms_under_zero_umask") + .output() + .expect("spawn the permission fixture"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!( + output.status.success(), + "the permission fixture must pass in its child process\n\ + --- stdout ---\n{stdout}\n--- stderr ---\n{stderr}" + ); + + // Not redundant with the status check, and this is the assertion that + // keeps the whole fixture from passing vacuously: a filter matching no + // test runs zero tests and still exits 0, so a renamed or mistyped + // fixture would look like a green permission check while asserting + // nothing at all. + assert!( + stdout.contains("1 passed"), + "the fixture filter must select exactly one test that passed; a filter matching \ + nothing exits 0 and would make this check vacuous\n--- stdout ---\n{stdout}" + ); + } + #[cfg(unix)] #[test] fn p2p_existing_key_dir_with_loose_permissions_is_tightened() { From 77331acbdb5ad93b4beb23a350754fd5794a43e6 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:49:16 -0500 Subject: [PATCH 08/14] docs(node): scope the p2p key permission claim and note the rotation The old wording said the key file is "created with owner-only permissions" without qualification, which is only true on Unix: every permission path in p2p/mod.rs is cfg(unix), so on other platforms the file inherits whatever the directory gives it and nothing is enforced. Say what is actually enforced and where. Also document what operators now have to do rather than leaving them to discover it: - GITLAWB_P2P_KEY must name a directory, since a bare filename is refused at startup. - The PeerId rotates once on the first start after upgrading, so a GITLAWB_P2P_BOOTSTRAP multiaddr pinning a peer's old id with a /p2p/ suffix needs updating or dropping. Suffix-less addresses and the HTTP seed list are unaffected. - If the node reports tightening a loose key directory, the key that was in it should be treated as possibly exposed and deleted so a fresh one is generated. --- .env.example | 12 +++++++++--- README.md | 23 ++++++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/.env.example b/.env.example index 77527ed9e..97ced9043 100644 --- a/.env.example +++ b/.env.example @@ -7,9 +7,15 @@ # Generate with: gl identity new GITLAWB_KEY=/data/keys/identity.pem -# Path to the node's persistent libp2p identity key file. Generated on first -# start with owner-only permissions; keep it on a persistent volume so the -# PeerId survives redeploys. Default: ~/.gitlawb/p2p.key +# Path to the node's persistent libp2p identity key file. Must include a +# directory; the node refuses to start on a bare filename, because it will not +# keep its p2p identity key in the working directory. On Unix it is created +# 0600 inside a 0700 directory, and a loose key directory is tightened to 0700 +# on start; on other platforms no permissions are enforced. If the node logs +# that it tightened a loose key directory, treat the key that was sitting there +# as possibly exposed: delete it so a fresh identity is generated on the next +# start. Keep it on a persistent volume so the PeerId survives redeploys. +# Default: ~/.gitlawb/p2p.key #GITLAWB_P2P_KEY=/data/keys/p2p.key # Publicly reachable URL of this node (used in peer announcements) diff --git a/README.md b/README.md index fd132e5c4..8688f3005 100644 --- a/README.md +++ b/README.md @@ -339,7 +339,7 @@ Important node settings: | `GITLAWB_P2P_PORT` | libp2p QUIC/UDP port. Use `0` to disable. | | `GITLAWB_BOOTSTRAP_PEERS` | Comma-separated HTTP peer URLs. | | `GITLAWB_P2P_BOOTSTRAP` | Comma-separated libp2p multiaddrs. | -| `GITLAWB_P2P_KEY` | Path to the node's persistent libp2p identity key file, which fixes the PeerId across restarts. Created with owner-only permissions on first start. Default `~/.gitlawb/p2p.key`; point it at a persistent volume when running in a container. | +| `GITLAWB_P2P_KEY` | Path to the node's persistent libp2p identity key file, which fixes the PeerId across restarts. Must include a directory, such as `/data/keys/p2p.key` or `./keys/p2p.key`; the node refuses to start on a bare filename, because it will not keep its p2p identity key in the working directory. On Unix the file is created `0600` inside a `0700` directory, and a loose key directory is tightened to `0700` on start; on other platforms no permissions are enforced. Default `~/.gitlawb/p2p.key`; point it at a persistent volume when running in a container. | | `GITLAWB_BOOTSTRAP_DISABLE_SEEDS` | Disable embedded seed peers for isolated dev/test networks. | | `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. | | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. | @@ -363,6 +363,27 @@ Important node settings: Production note: change the default Postgres password before exposing a node publicly. +### Upgrading: the PeerId rotates once + +This node's libp2p identity is now a keypair generated on first start and kept +at `GITLAWB_P2P_KEY`, rather than one derived from the node DID. Every node +therefore gets a new PeerId once, on the first start after upgrading, and keeps +it from then on as long as that key file survives (put it on a persistent volume +in a container). + +Two things to check before upgrading: + +- Any `GITLAWB_P2P_BOOTSTRAP` multiaddr that pins a peer's old PeerId with a + `/p2p/` suffix stops matching once that peer upgrades. Update the + suffix, or drop it and let identify supply the current one. Addresses without + the suffix keep working untouched. +- `GITLAWB_P2P_KEY` must name a directory. A bare filename is refused at + startup, since the node will not keep its identity key in the working + directory. + +Peers found over `GITLAWB_BOOTSTRAP_PEERS` and the embedded seed list are +unaffected, since those are HTTP URLs and carry no PeerId. + --- ## Optional node staking From 1948d3d31b9648b0ccfff91dbc963def1f58e936 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:44:07 -0500 Subject: [PATCH 09/14] fix(review): close a `..` escape from the p2p key-path check Two reviewers found the same hole independently: the check rejected a path naming no directory, but a relative parent that walks back out through `..` named one and still landed in the working directory. `a/../p2p.key` and `./keys/../p2p.key` resolve to the cwd itself and `../p2p.key` resolves above it, so all three put the key exactly where the check exists to keep it out of, and had ensure_key_dir chmod that directory to 0700 on the way. Verified by running the paths through the predicate and printing where each parent lands. The rule is now that a relative key path must name a directory and must not walk back out: at least one Normal component, no ParentDir. `..` inside an absolute path stays accepted, since it cannot depend on where the process started. The predicate moves into names_no_usable_directory next to key_parent, and the config gate and the load_or_create_p2p_keypair backstop both call it, so they cannot drift apart. Also fixes two smaller gaps found in the same pass: - The permission fixture could report "1 passed" while asserting nothing. Its env gate returns early, and an early return is a passing test, so a renamed variable would look green. It now prints a sentinel after its assertions and the parent requires it. Confirmed by pointing the child at a different variable and watching the parent fail. - A GITLAWB_P2P_KEY starting with `~/` is refused when no home directory resolves, instead of creating a literal `~` directory relative to wherever the node happened to start. The backstop had no test, so it has one now, along with a both-directions test for the predicate. That test cleans up after itself: with the guard removed it really does write a key next to the source, which broke a later run once. --- crates/gitlawb-node/src/config.rs | 43 ++++++- crates/gitlawb-node/src/p2p/mod.rs | 178 +++++++++++++++++++++++++++-- 2 files changed, 208 insertions(+), 13 deletions(-) diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 0f63e756e..afd513481 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -613,12 +613,28 @@ impl Config { // every container's first boot), and comparing against the process // working directory would reject `/data/p2p.key` under the image's // WORKDIR, an absolute directory the operator did name. + // `resolved_p2p_key_path` expands a leading `~/` only when a home + // directory is resolvable, and otherwise hands back the literal string. + // That would leave the shipped default naming a directory called `~` + // relative to wherever the process started, which is a real directory + // the node would create and chmod, and whose location moves with the + // working directory. It passes the check below because `~` is an + // ordinary path component, so it has to be caught separately. let p2p_key_path = self.resolved_p2p_key_path(); - if crate::p2p::key_parent(&p2p_key_path) == Path::new(".") { + if self.p2p_key_path.starts_with("~/") && p2p_key_path == Path::new(&self.p2p_key_path) { return Err(format!( - "GITLAWB_P2P_KEY ({}) must include a directory, such as ./keys/p2p.key or \ - /data/keys/p2p.key: the node will not store its p2p identity key in the \ - working directory, where the directory holding it cannot be secured.", + "GITLAWB_P2P_KEY ({}) starts with `~/` but no home directory could be resolved, \ + so it would name a literal `~` directory relative to the working directory. \ + Set an absolute path such as /data/keys/p2p.key.", + self.p2p_key_path + )); + } + if crate::p2p::names_no_usable_directory(&p2p_key_path) { + return Err(format!( + "GITLAWB_P2P_KEY ({}) must include a directory that does not walk back through \ + `..`, such as ./keys/p2p.key or /data/keys/p2p.key: the node will not store its \ + p2p identity key in the working directory, where the directory holding it \ + cannot be secured.", self.p2p_key_path )); } @@ -1024,7 +1040,20 @@ mod tests { /// operator never nominated. Reject it at boot instead. #[test] fn p2p_key_path_without_a_directory_component_is_rejected() { - for path in ["p2p.key", "./p2p.key", "././p2p.key", "p2p.key/", ""] { + for path in [ + // No directory component at all. + "p2p.key", + "./p2p.key", + "././p2p.key", + "p2p.key/", + "", + // Looks like it names a directory and does not: each of these + // resolves back to the working directory or above it, so accepting + // them would defeat the check and chmod an unnominated directory. + "a/../p2p.key", + "./keys/../p2p.key", + "../p2p.key", + ] { let err = config_with_p2p_key(path) .validate() .expect_err(&format!("{path:?} names no directory and must be rejected")); @@ -1044,9 +1073,11 @@ mod tests { "keys/p2p.key", "./keys/p2p.key", "/data/keys/p2p.key", - "../p2p.key", "/data/p2p.key", "~/.gitlawb/p2p.key", + // Absolute paths are judged unambiguously, so `..` inside one is + // fine: it cannot depend on where the process was started. + "/data/keys/../p2p.key", ] { assert!( config_with_p2p_key(path).validate().is_ok(), diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index f6e29d165..4299c25c1 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -187,6 +187,53 @@ pub(crate) fn key_parent(key_path: &Path) -> &Path { } } +/// Whether `key_path` fails to name a directory the node is willing to manage. +/// +/// This is the gate `Config::validate` applies, kept next to `key_parent` +/// because the two answer the same question and drifting apart is how the +/// original defect happened. +/// +/// An absolute path always names its directory unambiguously, so it passes. +/// A relative path is judged lexically against two ways of failing to name one: +/// +/// * no directory at all, so the parent is empty or nothing but `.` +/// (`p2p.key`, `./p2p.key`, `p2p.key/`, `""`), and +/// * a parent that walks back out through `..` (`a/../p2p.key`, +/// `./keys/../p2p.key`, `../p2p.key`). +/// +/// The second case is the one that is easy to miss and was missed once: those +/// paths look like they name a directory, and they do not. `a/..` and +/// `./keys/..` resolve to the working directory itself, and `..` resolves above +/// it, so accepting them would put the key exactly where this check exists to +/// keep it out of, and would have the node chmod that directory to 0700 on the +/// way. Any `..` in a relative parent makes the target depend on where the +/// process was started, which is the property being refused, so the whole class +/// is rejected rather than resolved. +/// +/// Lexical on purpose: no `canonicalize` (the parent legitimately does not exist +/// yet on a first start) and no `current_dir` comparison (it would reject +/// `/data/p2p.key` under a `/data` WORKDIR, an absolute directory the operator +/// named). +pub(crate) fn names_no_usable_directory(key_path: &Path) -> bool { + if key_path.is_absolute() { + return false; + } + match key_path.parent() { + None => true, + Some(parent) => { + let mut named_a_directory = false; + for component in parent.components() { + match component { + Component::ParentDir => return true, + Component::Normal(_) => named_a_directory = true, + _ => {} + } + } + !named_a_directory + } + } +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { @@ -198,9 +245,10 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result // config validation cannot quietly resurrect the old behaviour of writing // the key into the working directory and chmodding whatever that happens // to be. - if parent == Path::new(".") { + if names_no_usable_directory(key_path) { return Err(anyhow::anyhow!( - "p2p key path {} names no directory; give it one, such as ./keys/p2p.key", + "p2p key path {} names no directory the node can manage; give it one that does not \ + walk back through `..`, such as ./keys/p2p.key", key_path.display() )); } @@ -751,6 +799,12 @@ mod tests { // out of band by a throwaway probe rather than by a committed test. A race // on process-global state has no reliable committed red-green. + /// Printed by the permission fixture only after its assertions have run, + /// and required by the parent. See the parent test for why "1 passed" is + /// not sufficient on its own. + #[cfg(unix)] + const FIXTURE_SENTINEL: &str = "p2p-key-perms: asserted"; + /// Re-invoke this test binary to run one `#[ignore]`d fixture test. #[cfg(unix)] fn fixture_command(fixture_test: &str) -> std::process::Command { @@ -800,6 +854,13 @@ mod tests { .permissions() .mode(); assert_eq!(dir_mode & 0o777, 0o700, "key directory must be owner-only"); + + // Proof-of-work sentinel, printed only after both assertions have run. + // "1 passed" alone does not prove this fixture asserted anything: the + // early return above is itself a passing test, so an env-var mismatch + // (a renamed variable, a changed value) would report 1 passed while + // checking nothing. The parent requires this line. + println!("{FIXTURE_SENTINEL}"); } #[cfg(unix)] @@ -817,16 +878,119 @@ mod tests { --- stdout ---\n{stdout}\n--- stderr ---\n{stderr}" ); - // Not redundant with the status check, and this is the assertion that - // keeps the whole fixture from passing vacuously: a filter matching no - // test runs zero tests and still exits 0, so a renamed or mistyped - // fixture would look like a green permission check while asserting - // nothing at all. + // Two separate vacuity holes, and each assertion closes one the other + // does not. + // + // A filter matching no test runs zero tests and still exits 0, so a + // renamed or mistyped fixture name would look like a green permission + // check. "1 passed" closes that. assert!( stdout.contains("1 passed"), "the fixture filter must select exactly one test that passed; a filter matching \ nothing exits 0 and would make this check vacuous\n--- stdout ---\n{stdout}" ); + + // But "1 passed" does not prove the fixture ASSERTED anything: its + // env-var gate returns early, and an early return is itself a passing + // test. A renamed variable or a changed value would report 1 passed + // having checked nothing. The sentinel is printed only after both mode + // assertions, so requiring it closes that second hole. + assert!( + stdout.contains(FIXTURE_SENTINEL), + "the fixture must print {FIXTURE_SENTINEL:?} after its assertions; without it the \ + child may have returned early at its env gate and still reported 1 passed\ + \n--- stdout ---\n{stdout}" + ); + } + + /// The backstop inside `load_or_create_p2p_keypair`, exercised directly. + /// + /// `Config::validate` rejects these paths before the node starts, so in a + /// running node this branch is unreachable. That is exactly why it needs its + /// own test: it exists for a future caller that does not go through config + /// validation, and a guard whose only justification is a caller that does + /// not exist yet is otherwise never executed by anything. + /// + /// No file is created for any of these, so there is nothing to clean up. + #[test] + fn p2p_key_path_naming_no_directory_is_refused_without_the_config_gate() { + for path in [ + "p2p.key", + "./p2p.key", + "a/../p2p.key", + "./keys/../p2p.key", + "../p2p.key", + ] { + let result = load_or_create_p2p_keypair(Path::new(path)); + + // Clean up BEFORE asserting, and unconditionally. When the guard is + // working none of these paths is ever created, so this is a no-op. + // When it is not, the call really does write a key relative to the + // test process's working directory, which is the crate root, and + // leaving that behind breaks every later run in this checkout. That + // is not hypothetical: a mutation run that removed the guard left a + // real 0600 key and an `a/` directory in crates/gitlawb-node, and + // the next baseline failed because of it. + let leaked = Path::new(path).exists(); + let _ = std::fs::remove_file(path); + for stray_dir in ["a", "keys"] { + let _ = std::fs::remove_dir(stray_dir); + } + + let err = result.expect_err(&format!("{path:?} must be refused by the backstop")); + let msg = format!("{err:#}"); + assert!( + msg.contains("names no directory the node can manage"), + "{path:?} must be refused for naming no usable directory, got: {msg}" + ); + assert!(!leaked, "{path:?} must not have been created"); + } + } + + /// The predicate itself, over the whole input space in both directions. + /// + /// Deliberately does not call `load_or_create_p2p_keypair` on the accepted + /// paths: that would create directories and write a real key relative to + /// whatever directory the test process happens to run in. The rejected + /// direction is covered above, where nothing is created by construction, + /// and the gate and the backstop call this same function so they cannot + /// disagree. + #[test] + fn names_no_usable_directory_covers_both_directions() { + for path in [ + // No directory component. + "p2p.key", + "./p2p.key", + "././p2p.key", + "p2p.key/", + "", + // Resolves back to the working directory or above it. + "a/../p2p.key", + "./keys/../p2p.key", + "../p2p.key", + "keys/../../p2p.key", + ] { + assert!( + names_no_usable_directory(Path::new(path)), + "{path:?} must be rejected" + ); + } + + for path in [ + "keys/p2p.key", + "./keys/p2p.key", + "keys/nested/p2p.key", + "/data/keys/p2p.key", + "/data/p2p.key", + // `..` inside an absolute path cannot depend on the working + // directory, so it stays accepted. + "/data/keys/../p2p.key", + ] { + assert!( + !names_no_usable_directory(Path::new(path)), + "{path:?} must be accepted" + ); + } } #[cfg(unix)] From f29235dc317f006f745b1cb6053d13351476a9b7 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:56:43 -0500 Subject: [PATCH 10/14] fix(review): reject `..` in an absolute p2p key path too The previous commit closed this for relative paths and exempted absolute ones, reasoning that an absolute path cannot depend on the working directory. That is true and it is not the hazard. `key_parent` hands `ensure_key_dir` the lexical parent, so `/data/keys/../p2p.key` chmods `/data` rather than the `keys` directory the path appears to name, and `/data/../p2p.key` run as root would try to tighten `/` to 0700. The exemption also had a test asserting the first of those was fine, so the gap was written down as intended behaviour. `..` is now rejected wherever it appears. An absolute path's root counts as naming a directory, so `/p2p.key` still validates and `/data/keys/p2p.key` is unaffected. Found by a second-model review pass after the in-process reviewers had cleared the relative half. --- crates/gitlawb-node/src/config.rs | 7 +++-- crates/gitlawb-node/src/p2p/mod.rs | 46 +++++++++++++++++++----------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index afd513481..26d37d50a 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -1053,6 +1053,10 @@ mod tests { "a/../p2p.key", "./keys/../p2p.key", "../p2p.key", + // Absolute too: the lexical parent is what gets chmodded, so these + // would tighten /data and / rather than the named directory. + "/data/keys/../p2p.key", + "/data/../p2p.key", ] { let err = config_with_p2p_key(path) .validate() @@ -1075,9 +1079,6 @@ mod tests { "/data/keys/p2p.key", "/data/p2p.key", "~/.gitlawb/p2p.key", - // Absolute paths are judged unambiguously, so `..` inside one is - // fine: it cannot depend on where the process was started. - "/data/keys/../p2p.key", ] { assert!( config_with_p2p_key(path).validate().is_ok(), diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 4299c25c1..d5edd4ce1 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -215,23 +215,32 @@ pub(crate) fn key_parent(key_path: &Path) -> &Path { /// `/data/p2p.key` under a `/data` WORKDIR, an absolute directory the operator /// named). pub(crate) fn names_no_usable_directory(key_path: &Path) -> bool { - if key_path.is_absolute() { - return false; - } - match key_path.parent() { - None => true, - Some(parent) => { - let mut named_a_directory = false; - for component in parent.components() { - match component { - Component::ParentDir => return true, - Component::Normal(_) => named_a_directory = true, - _ => {} - } + let Some(parent) = key_path.parent() else { + return true; + }; + + let mut named_a_directory = false; + for component in parent.components() { + match component { + // Rejected wherever it appears, absolute paths included. An earlier + // version exempted absolute paths on the reasoning that they cannot + // depend on the working directory, which is true and beside the + // point: `key_parent` hands `ensure_key_dir` the LEXICAL parent, so + // `/data/keys/../p2p.key` chmods `/data` rather than the `keys` + // directory the path appears to name, and `/data/../p2p.key` run as + // root would try to tighten `/` to 0700. The hazard is chmodding a + // resolved ancestor nobody nominated, and that does not care whether + // the path was absolute. + Component::ParentDir => return true, + // `/` is a directory the operator named, so an absolute path's root + // counts the same way a normal component does. + Component::Normal(_) | Component::RootDir | Component::Prefix(_) => { + named_a_directory = true } - !named_a_directory + Component::CurDir => {} } } + !named_a_directory } /// Load the node's persistent libp2p identity from `key_path`, generating and @@ -969,6 +978,11 @@ mod tests { "./keys/../p2p.key", "../p2p.key", "keys/../../p2p.key", + // Absolute paths are rejected on `..` too. The lexical parent is + // what gets chmodded, so these tighten `/data` and `/` rather than + // the directory the path appears to name. + "/data/keys/../p2p.key", + "/data/../p2p.key", ] { assert!( names_no_usable_directory(Path::new(path)), @@ -982,9 +996,7 @@ mod tests { "keys/nested/p2p.key", "/data/keys/p2p.key", "/data/p2p.key", - // `..` inside an absolute path cannot depend on the working - // directory, so it stays accepted. - "/data/keys/../p2p.key", + "/p2p.key", ] { assert!( !names_no_usable_directory(Path::new(path)), From 3a296485fb78905c6d67e589174fcf0012648dbc Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 07:11:22 -0500 Subject: [PATCH 11/14] fix(node): scrub the serialized p2p private key from memory Two buffers held the private key in its protobuf form and dropped without scrubbing: the encoding produced when a new identity is generated, and the file contents read back on every subsequent start. Both are now Zeroizing, matching what gitlawb-core already does for its own key material. Scope worth being honest about: this scrubs our copies of the serialized form, not the libp2p Keypair itself, which owns the secret for the process lifetime and exposes no way to zeroize it. The gain is that the encoded bytes do not outlive the write and the read. zeroize was already in the tree through gitlawb-core, so this promotes it to a direct dependency of gitlawb-node and adds no packages; the lockfile change is the one line recording that. --- Cargo.lock | 1 + crates/gitlawb-node/Cargo.toml | 1 + crates/gitlawb-node/src/p2p/mod.rs | 19 ++++++++++++++----- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7050bc6c..ff0e5b9fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3484,6 +3484,7 @@ dependencies = [ "tracing-subscriber", "unicode-normalization", "uuid", + "zeroize", "zstd", ] diff --git a/crates/gitlawb-node/Cargo.toml b/crates/gitlawb-node/Cargo.toml index c583569cb..b4b8b8be8 100644 --- a/crates/gitlawb-node/Cargo.toml +++ b/crates/gitlawb-node/Cargo.toml @@ -33,6 +33,7 @@ sqlx = { version = "0.8", features = ["postgres", "runtime-tokio-rustls", "chron clap = { version = "4", features = ["derive", "env"] } bytes = "1" libc = "0.2" +zeroize = "1" cid = { workspace = true } hex = { workspace = true } sha2 = { workspace = true } diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index d5edd4ce1..dff499717 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -25,6 +25,7 @@ use libp2p_swarm::{NetworkBehaviour, Swarm, SwarmEvent}; use tokio::sync::{mpsc, oneshot}; use tracing::{debug, info, warn}; use uuid::Uuid; +use zeroize::Zeroizing; use crate::db::{Db, ReceivedRefUpdate}; @@ -272,9 +273,13 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result } let kp = identity::Keypair::generate_ed25519(); - let bytes = kp - .to_protobuf_encoding() - .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?; + // The serialized form carries the private key, so scrub it on drop rather + // than leaving it in a heap buffer for the rest of the process. Same + // convention `gitlawb-core` applies to its own key material. + let bytes = Zeroizing::new( + kp.to_protobuf_encoding() + .map_err(|e| anyhow::anyhow!("failed to serialize p2p key: {e}"))?, + ); match write_key_atomically(key_path, &bytes) { Ok(()) => { @@ -476,8 +481,12 @@ fn read_p2p_keypair(key_path: &Path) -> Result { } } - let bytes = std::fs::read(key_path) - .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; + // Same reason as the write path: this is the private key, so it gets + // scrubbed on drop instead of lingering in a heap buffer. + let bytes = Zeroizing::new( + std::fs::read(key_path) + .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?, + ); // An empty file decodes as a valid protobuf with a key type of RSA, so // without this the operator gets a misleading complaint about a missing From 6dde36b02d20ad6a7c05141c668b9b0b9364aaaa Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:41:55 -0500 Subject: [PATCH 12/14] fix(node): refuse a p2p key or key directory owned by another user Mode bits were the only thing checked, and they do not make something node-owned. A 0700 directory or a 0600 key file belonging to a different user passes every permission check here while that user keeps the ability to replace what is inside it, which means they choose the node's libp2p identity. That is the capability the persisted key exists to take away. Both sites now bail rather than warn. Unlike a loose mode this is not repairable: chown needs privilege the node should not have, and taking ownership of someone else's file would be wrong even if it could. In ensure_key_dir the check runs before the mode repair, because a directory we do not own fails its chmod with EPERM and reports "could not be tightened", which describes the symptom and sends the operator at the wrong thing. Testing this needed a seam. A test cannot chown a fixture to another user without root, so a fixture-based test could only ever exercise the matching case, which is a guard nobody has watched refuse anything. So the decision is a pure function taking both uids, and a #[cfg(test)] euid override (the same thread-local shape as the existing FAIL_KEY_WRITE injector) lets the wiring tests drive the real read and directory paths while pretending to be a different user. A further test pins that the seam defaults to the real geteuid, since one that quietly stopped consulting it would leave every other ownership test passing against nothing. Found during review of the key-persistence change by two independent reviewers. --- crates/gitlawb-node/src/p2p/mod.rs | 229 +++++++++++++++++++++++++++-- 1 file changed, 219 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index dff499717..587db471f 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -244,6 +244,43 @@ pub(crate) fn names_no_usable_directory(key_path: &Path) -> bool { !named_a_directory } +/// Why a key file or key directory owned by another user is refused, if it is. +/// +/// Mode bits alone do not make something node-owned. A `0700` directory or a +/// `0600` file belonging to a different user passes every permission check here +/// while that user keeps the ability to replace what is inside it, which means +/// they choose the node's libp2p identity. That is the capability the persisted +/// key exists to take away, so it is refused rather than warned about. +/// +/// Unlike a loose mode this is not repairable: `chown` needs privilege the node +/// should not have, and taking ownership of another user's file would be the +/// wrong move even if it could. So the callers bail instead of tightening. +/// +/// Pure, and takes both uids as arguments, so both directions are testable +/// without privilege. A test cannot `chown` a fixture to another user without +/// root, and a guard that can only be exercised in one direction is the shape +/// that ships unproven. +#[cfg(unix)] +fn foreign_ownership_error(what: &str, path: &Path, owner_uid: u32, euid: u32) -> Option { + if owner_uid == euid { + return None; + } + Some(format!( + "p2p {what} {} is owned by uid {} but this node runs as uid {}; that user can \ + replace it and so decides the node's libp2p identity, which is what the persisted \ + key exists to prevent. Point {} at a location this user owns, or have the owner \ + hand it over; the node will not adopt it.", + path.display(), + owner_uid, + euid, + if what == "key directory" { + "GITLAWB_P2P_KEY's directory" + } else { + "GITLAWB_P2P_KEY" + } + )) +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { @@ -336,13 +373,24 @@ fn ensure_key_dir(dir: &Path) -> Result<()> { #[cfg(unix)] { + use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(dir) - .with_context(|| format!("failed to stat key directory {}", dir.display()))? - .permissions() - .mode() - & 0o777; + let md = std::fs::metadata(dir) + .with_context(|| format!("failed to stat key directory {}", dir.display()))?; + + // Before the mode repair below, not after. A directory we do not own + // cannot be repaired by us: the chmod would fail with EPERM and report + // "could not be tightened", which describes the symptom and hides the + // cause. It also matters that a foreign directory sitting at 0700 + // passes the mode check silently today, so ownership is the only thing + // that catches it. + let euid = effective_uid(); + if let Some(err) = foreign_ownership_error("key directory", dir, md.uid(), euid) { + anyhow::bail!(err); + } + + let mode = md.permissions().mode() & 0o777; if mode & 0o077 != 0 { warn!( dir = %dir.display(), @@ -455,6 +503,27 @@ thread_local! { /// Test-only fault injection for the key write. Thread-local so an armed /// test cannot disturb the others running beside it. static FAIL_KEY_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; + + /// Test-only override for the process effective uid, same thread-local + /// shape and for the same reason as `FAIL_KEY_WRITE`. + /// + /// The ownership refusal is otherwise untestable end to end: proving that + /// `read_p2p_keypair` and `ensure_key_dir` actually consult it needs a + /// fixture owned by a different user, and a test cannot `chown` one without + /// root. Pretending to be a different uid against a fixture we do own + /// exercises the identical branch and needs no privilege. + static EUID_OVERRIDE: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} + +/// The effective uid the ownership checks compare against. +#[cfg(unix)] +fn effective_uid() -> u32 { + #[cfg(test)] + if let Some(uid) = EUID_OVERRIDE.with(|c| c.get()) { + return uid; + } + // SAFETY: `geteuid` only reads the calling process's effective uid. + unsafe { libc::geteuid() } } /// Read an existing key file, refusing one whose permissions or contents make @@ -463,13 +532,24 @@ thread_local! { fn read_p2p_keypair(key_path: &Path) -> Result { #[cfg(unix)] { + use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(key_path) - .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))? - .permissions() - .mode() - & 0o777; + // One stat feeds both checks. Statting twice would leave a window in + // which the file the ownership check approved is not the file the mode + // check measured. + let md = std::fs::metadata(key_path) + .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))?; + + // Ownership first: a key owned by someone else is not made safe by its + // mode, and saying "mode is fine" about a file we do not own would be + // the more misleading error of the two. + let euid = effective_uid(); + if let Some(err) = foreign_ownership_error("key", key_path, md.uid(), euid) { + anyhow::bail!(err); + } + + let mode = md.permissions().mode() & 0o777; if mode & 0o077 != 0 { anyhow::bail!( "p2p key at {} has mode {:04o}, which grants access beyond its owner; \ @@ -973,6 +1053,135 @@ mod tests { /// direction is covered above, where nothing is created by construction, /// and the gate and the backstop call this same function so they cannot /// disagree. + /// Guard for the seam itself: with no override armed, the checks use the + /// real process uid. Without this, every ownership test below could pass + /// against a seam that had quietly stopped consulting `geteuid` at all. + #[cfg(unix)] + #[test] + fn effective_uid_defaults_to_the_real_process_uid() { + // SAFETY: `geteuid` only reads the calling process's effective uid. + assert_eq!(effective_uid(), unsafe { libc::geteuid() }); + } + + /// `read_p2p_keypair` actually consults the ownership check. + /// + /// The fixture is owned by this user (a test cannot chown one to anyone + /// else without root), so the override supplies a different euid instead. + /// That drives the identical branch: the file's uid and the process uid + /// disagree. + #[cfg(unix)] + #[test] + fn read_p2p_keypair_refuses_a_key_owned_by_another_user() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + let kp = identity::Keypair::generate_ed25519(); + std::fs::write(&path, kp.to_protobuf_encoding().unwrap()).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let real_uid = std::fs::metadata(&path).unwrap().uid(); + let other = real_uid.wrapping_add(1); + + EUID_OVERRIDE.with(|c| c.set(Some(other))); + let result = read_p2p_keypair(&path); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a foreign-owned key must be refused") + ); + assert!( + err.contains("owned by uid") && err.contains("will not adopt it"), + "must be refused for ownership, not something else, got: {err}" + ); + + // And the same file loads once the uids agree, so the refusal is about + // ownership and not about the fixture being broken. + assert!( + read_p2p_keypair(&path).is_ok(), + "the same key must load when the owner matches" + ); + } + + /// `ensure_key_dir` consults it too, and does so BEFORE trying to repair the + /// mode. A loose directory we do not own must report ownership, not a failed + /// chmod, and a 0700 directory we do not own must still be refused even + /// though the mode check alone would pass it. + #[cfg(unix)] + #[test] + fn ensure_key_dir_refuses_a_directory_owned_by_another_user() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + for mode in [0o700, 0o777] { + let dir = tempfile::tempdir().unwrap(); + let keys = dir.path().join("keys"); + std::fs::create_dir(&keys).unwrap(); + std::fs::set_permissions(&keys, std::fs::Permissions::from_mode(mode)).unwrap(); + + let real_uid = std::fs::metadata(&keys).unwrap().uid(); + EUID_OVERRIDE.with(|c| c.set(Some(real_uid.wrapping_add(1)))); + let result = ensure_key_dir(&keys); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a foreign-owned key directory must be refused") + ); + assert!( + err.contains("owned by uid"), + "mode {mode:04o} must be refused for ownership, got: {err}" + ); + assert!( + !err.contains("could not be tightened"), + "ownership must be reported before the chmod is attempted, got: {err}" + ); + } + } + + /// Both directions of the ownership refusal, without needing root. + /// + /// The reason this is a pure function taking two uids rather than a stat of + /// a real fixture: a test cannot chown a file to another user without + /// privilege, so a fixture-based version could only ever exercise the + /// matching case. That is the shape that ships a guard nobody has seen + /// refuse anything. + #[cfg(unix)] + #[test] + fn foreign_ownership_is_refused_and_matching_ownership_is_not() { + let path = Path::new("/data/keys/p2p.key"); + + // Same user: no complaint, whatever the uid happens to be. + for uid in [0u32, 1000, 65534] { + assert!( + foreign_ownership_error("key", path, uid, uid).is_none(), + "uid {uid} owning its own key must not be refused" + ); + } + + // Different user: refused, and the message has to name both uids or an + // operator cannot tell which side is wrong. + let err = foreign_ownership_error("key", path, 1000, 1001) + .expect("a key owned by another uid must be refused"); + assert!( + err.contains("1000") && err.contains("1001"), + "the refusal must name both the owner and the running uid, got: {err}" + ); + assert!( + err.contains("/data/keys/p2p.key"), + "the refusal must name the path, got: {err}" + ); + + // Root running against a user-owned file is still a mismatch. This is + // the case worth pinning: root can read it anyway, so it is tempting to + // treat it as fine, but the other user can still replace the file and + // therefore still chooses the identity. + assert!( + foreign_ownership_error("key directory", path, 1000, 0).is_some(), + "a user-owned path under a root-run node is still foreign" + ); + } + #[test] fn names_no_usable_directory_covers_both_directions() { for path in [ From 5a93f2435c27c4d7f10786e24bf4b35256a4ef86 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:19:53 -0500 Subject: [PATCH 13/14] fix(review): close the ancestor path and the stat/read window Review found the leaf checks were not the trust boundary. Both fixes come from the same observation: what the guard inspects and what the node then uses were not provably the same thing. An ancestor the node does not control launders an unsafe path into a safe looking one. A user owning /home/them/base can have the node use /home/them/base/keys/p2p.key; the node creates keys and the key, so both are node-owned, 0700 and 0600, and pass every check. That owner can then rename keys aside, let the node generate a fresh identity, and move the old directory back before a restart. They never own anything the leaf checks look at, and they decide which identity the node presents and when it rolls back. ensure_key_dir now walks the existing ancestors first, before creating anything, since a directory the node made would pass afterwards by construction. Root counts as trusted, or /data under a root-owned / refuses on every normal deployment. The mode rule is world-writable-without-sticky rather than group too: group write is a narrower capability that needs group membership, and refusing it would reject an ordinary umask-002 directory. Someone in an ancestor's group can still rename the key directory; that residual is real and stated rather than papered over. read_p2p_keypair statted the path and then read the path again, so the file approved by uid was not provably the file whose bytes became the identity. It now opens once with O_NOFOLLOW, takes uid and mode from that handle, and reads from it. The flag also refuses a symlink at the final component instead of following it. Two of the tests were weaker than their names. The ordering assertion passed under either ordering, because a test-owned fixture makes the chmod succeed so "could not be tightened" never appears; it now asserts the mode is untouched, which is what actually separates them. The call-site assertions matched a shared substring, so a swapped argument or a uid/gid mixup would have gone unnoticed; they now name both uids in order and check which knob the remediation points at. Also moves a doc comment that had drifted onto the wrong test. --- crates/gitlawb-node/src/p2p/mod.rs | 238 ++++++++++++++++++++++++++--- 1 file changed, 219 insertions(+), 19 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 587db471f..f4d8ff810 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -281,6 +281,85 @@ fn foreign_ownership_error(what: &str, path: &Path, owner_uid: u32, euid: u32) - )) } +/// Refuse a key directory whose existing ancestors are controlled by someone +/// else, before creating anything inside them. +/// +/// Checking only the key directory and the key file is not enough, and the way +/// it fails is worth spelling out because it looks safe. A user who owns +/// `/home/them/base` can have the node use `/home/them/base/keys/p2p.key`. On +/// first start the node creates `keys` and the key itself, so both are +/// node-owned, `0700` and `0600`, and pass every check here. That owner never +/// needs to own either one: they can rename `keys` aside, let the node generate +/// a fresh identity in a new `keys`, and move the old directory back before a +/// later restart. Both directories pass, and they decide which identity the +/// node presents and when it rolls back. +/// +/// So the trust boundary is the whole existing chain, not the leaf. Walking up +/// from the deepest component that exists today, every ancestor must be owned by +/// this user or by root, and must not be group or other writable. +/// +/// Root counts as trusted on purpose. Requiring every ancestor to be +/// node-owned would refuse `/data/keys` under a root-owned `/data`, and `/` +/// itself, which is most real deployments. Root can already replace the binary, +/// so treating it as an attacker here would buy nothing. +/// +/// Only existing ancestors are inspected. The ones this call is about to create +/// inherit their parent, which the walk has already cleared. +#[cfg(unix)] +fn foreign_ancestor_error(dir: &Path, euid: u32) -> Option { + use std::os::unix::fs::MetadataExt; + use std::os::unix::fs::PermissionsExt; + + // `ancestors()` yields the path itself first, and that one is deliberately + // skipped. `ensure_key_dir` exists to create and tighten the key directory, + // so judging it here would refuse exactly the loose-but-ours case the repair + // is written for. + for ancestor in dir.ancestors().skip(1) { + let md = match std::fs::metadata(ancestor) { + Ok(md) => md, + // Does not exist yet, so it is one of the directories this call + // creates; keep walking up to the part of the path that is real. + Err(_) => continue, + }; + + let owner = md.uid(); + if owner != euid && owner != 0 { + return Some(format!( + "p2p key directory {} sits under {}, which is owned by uid {} rather than this \ + node (uid {}) or root; that user can rename or replace the directory holding \ + the key and so control which identity the node presents. Put the key somewhere \ + this user or root owns the whole path.", + dir.display(), + ancestor.display(), + owner, + euid + )); + } + + let mode = md.permissions().mode() & 0o777; + // The sticky bit is what makes a shared directory like /tmp survivable, + // since it stops non-owners removing entries someone else created. + let sticky = md.permissions().mode() & 0o1000 != 0; + // World-writable only, not group-writable. Group write on an ancestor is + // a real but much narrower capability (it needs group membership), and + // refusing it would reject an ordinary umask-002 home or service + // directory, which is most of them. The residual is stated in the PR + // rather than papered over: someone in the group of an ancestor can + // still rename the key directory. + if mode & 0o002 != 0 && !sticky { + return Some(format!( + "p2p key directory {} sits under {}, which has mode {:04o} and is writable \ + beyond its owner; anyone with that write access can rename or replace the \ + directory holding the key and so control which identity the node presents.", + dir.display(), + ancestor.display(), + mode + )); + } + } + None +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { @@ -359,6 +438,19 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result /// keys. Issue #231 owns the sibling gap in `main.rs`'s own creation path for /// that file; nothing here touches it. fn ensure_key_dir(dir: &Path) -> Result<()> { + // Before anything is created. Creating the directory first and checking + // afterwards is what lets a foreign-owned ancestor launder an unsafe path + // into a node-owned 0700 child: the child passes every check precisely + // because the node made it, while the ancestor's owner keeps the ability to + // swap it out. + #[cfg(unix)] + { + let euid = effective_uid(); + if let Some(err) = foreign_ancestor_error(dir, euid) { + anyhow::bail!(err); + } + } + let mut builder = std::fs::DirBuilder::new(); builder.recursive(true); #[cfg(unix)] @@ -505,7 +597,8 @@ thread_local! { static FAIL_KEY_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; /// Test-only override for the process effective uid, same thread-local - /// shape and for the same reason as `FAIL_KEY_WRITE`. + /// shape and for the same reason as `FAIL_KEY_WRITE`. Unix-gated like its + /// only reader, or a non-unix test build carries it as dead code. /// /// The ownership refusal is otherwise untestable end to end: proving that /// `read_p2p_keypair` and `ensure_key_dir` actually consult it needs a @@ -530,15 +623,40 @@ fn effective_uid() -> u32 { /// it untrustworthy. Never regenerates: a node that silently replaces an /// unreadable key file would change its PeerId without the operator knowing. fn read_p2p_keypair(key_path: &Path) -> Result { + // One open, then everything is answered from that handle: the ownership + // check, the mode check, and the read itself. + // + // Statting the path and then reading the path again is the window that + // matters, and an earlier version of this had it. Between the two lookups + // the name can be pointed somewhere else, so the file approved by uid is + // not provably the file whose bytes become the identity. `fstat` on the fd + // cannot drift like that. + // + // `O_NOFOLLOW` refuses a symlink at the final component outright rather + // than reading through it. Planting one needs write access to the key + // directory, which the checks above are meant to deny, so this is the + // belt to that brace. #[cfg(unix)] - { + let bytes = { + use std::io::Read; use std::os::unix::fs::MetadataExt; + use std::os::unix::fs::OpenOptionsExt; use std::os::unix::fs::PermissionsExt; - // One stat feeds both checks. Statting twice would leave a window in - // which the file the ownership check approved is not the file the mode - // check measured. - let md = std::fs::metadata(key_path) + let mut file = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(key_path) + .with_context(|| { + format!( + "failed to open p2p key at {} (a symlink here is refused rather than \ + followed)", + key_path.display() + ) + })?; + + let md = file + .metadata() .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))?; // Ownership first: a key owned by someone else is not made safe by its @@ -559,10 +677,16 @@ fn read_p2p_keypair(key_path: &Path) -> Result { key_path.display() ); } - } - // Same reason as the write path: this is the private key, so it gets - // scrubbed on drop instead of lingering in a heap buffer. + // Same reason as the write path: this is the private key, so it gets + // scrubbed on drop instead of lingering in a heap buffer. + let mut buf = Zeroizing::new(Vec::new()); + file.read_to_end(&mut buf) + .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; + buf + }; + + #[cfg(not(unix))] let bytes = Zeroizing::new( std::fs::read(key_path) .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?, @@ -1045,14 +1169,6 @@ mod tests { } } - /// The predicate itself, over the whole input space in both directions. - /// - /// Deliberately does not call `load_or_create_p2p_keypair` on the accepted - /// paths: that would create directories and write a real key relative to - /// whatever directory the test process happens to run in. The rejected - /// direction is covered above, where nothing is created by construction, - /// and the gate and the backstop call this same function so they cannot - /// disagree. /// Guard for the seam itself: with no override armed, the checks use the /// real process uid. Without this, every ownership test below could pass /// against a seam that had quietly stopped consulting `geteuid` at all. @@ -1091,9 +1207,18 @@ mod tests { "{:#}", result.expect_err("a foreign-owned key must be refused") ); + // Naming both uids in the expected order is what makes this fail if the + // last two arguments are ever swapped, or if the check reads gid rather + // than uid. A shared substring like "owned by uid" passes under both. + assert!( + err.contains(&format!( + "owned by uid {real_uid} but this node runs as uid {other}" + )), + "the refusal must name the file's owner and the running uid in that order, got: {err}" + ); assert!( - err.contains("owned by uid") && err.contains("will not adopt it"), - "must be refused for ownership, not something else, got: {err}" + err.contains("GITLAWB_P2P_KEY") && !err.contains("GITLAWB_P2P_KEY's directory"), + "the key path refusal must point at the key knob, not the directory one, got: {err}" ); // And the same file loads once the uids agree, so the refusal is about @@ -1136,7 +1261,74 @@ mod tests { !err.contains("could not be tightened"), "ownership must be reported before the chmod is attempted, got: {err}" ); + // The message check above passes under EITHER ordering, because the + // fixture is test-owned so the chmod would succeed and never emit + // "could not be tightened". This is the assertion that actually + // separates them: if the ownership check ran after the repair, the + // mode would have been rewritten to 0700 before the bail. + assert_eq!( + std::fs::metadata(&keys).unwrap().permissions().mode() & 0o777, + mode, + "a refused directory must not have been chmodded first" + ); + } + } + + /// A foreign-owned ancestor is refused before anything is created under it. + /// + /// This is the case that survived the leaf checks: the node creates the key + /// directory and the key itself, so both are node-owned and correctly moded + /// and pass every other guard, while whoever owns the directory above can + /// rename the whole thing aside and swap an older one back. They choose the + /// identity without ever owning anything the leaf checks look at. + #[cfg(unix)] + #[test] + fn ensure_key_dir_refuses_a_foreign_owned_ancestor() { + use std::os::unix::fs::MetadataExt; + + let base = tempfile::tempdir().unwrap(); + let nested = base.path().join("keys"); + + let real_uid = std::fs::metadata(base.path()).unwrap().uid(); + EUID_OVERRIDE.with(|c| c.set(Some(real_uid.wrapping_add(1)))); + let result = ensure_key_dir(&nested); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a directory under a foreign-owned ancestor must be refused") + ); + assert!( + err.contains("sits under") && err.contains("control which identity"), + "must be refused for the ancestor, got: {err}" + ); + // Refused BEFORE creation: this is the whole point, since a directory + // the node created would pass the leaf ownership check afterwards. + assert!( + !nested.exists(), + "the key directory must not have been created under a foreign ancestor" + ); + } + + /// Root-owned ancestors are trusted, or nearly every real deployment breaks. + #[cfg(unix)] + #[test] + fn ensure_key_dir_accepts_a_root_owned_ancestor() { + use std::os::unix::fs::MetadataExt; + + // /usr is root-owned and not group/other writable on any sane system; + // skip rather than assert if this box disagrees. + let probe = Path::new("/usr"); + let Ok(md) = std::fs::metadata(probe) else { + return; + }; + if md.uid() != 0 { + return; } + assert!( + foreign_ancestor_error(&probe.join("nonexistent-gitlawb-keys"), 1000).is_none(), + "a root-owned ancestor must be trusted for a non-root node" + ); } /// Both directions of the ownership refusal, without needing root. @@ -1182,6 +1374,14 @@ mod tests { ); } + /// The predicate itself, over the whole input space in both directions. + /// + /// Deliberately does not call `load_or_create_p2p_keypair` on the accepted + /// paths: that would create directories and write a real key relative to + /// whatever directory the test process happens to run in. The rejected + /// direction is covered above, where nothing is created by construction, + /// and the gate and the backstop call this same function so they cannot + /// disagree. #[test] fn names_no_usable_directory_covers_both_directions() { for path in [ From 66fb3ffc954fc094733465b6e1bd931eb5609a42 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 13:08:37 -0500 Subject: [PATCH 14/14] test(node): make the ownership guards actually observable Re-running the mutation matrix after the ancestor check landed turned three entries from load-bearing into inconclusive. The guards had not changed; the tests had stopped being able to see them. The ancestor walk masked both leaf checks. With the euid override armed the whole path chain looks foreign, so a nested fixture tripped the ancestor error first, and that message also contains "owned by uid", so the assertions matched either way. Remove the leaf ownership check entirely and the tests stayed green. They now target the tempdir itself, whose ancestors are /tmp: root-owned and sticky, therefore trusted, so only the leaf is foreign. The uid/gid mixup was invisible because uid equals gid on an ordinary single-user machine, which makes reading the wrong field indistinguishable from reading the right one. The fixture now chgrps to a supplementary group, which needs no privilege, and degrades to the old behaviour where no such group exists rather than quietly proving less. With those two fixed and the ancestor and ordering mutations reshaped to name the assertion that actually separates the cases, all eight entries come back load-bearing. --- crates/gitlawb-node/src/p2p/mod.rs | 38 ++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index f4d8ff810..058b43c60 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -1196,6 +1196,17 @@ mod tests { std::fs::write(&path, kp.to_protobuf_encoding().unwrap()).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + // Give the fixture a group that is NOT its owning uid, where the box + // allows it. On a machine where uid == gid (this one, and most + // single-user boxes) a check that read gid instead of uid would be + // indistinguishable from the correct one, so the mutation covering that + // mixup cannot fail. chgrp to a group we already belong to needs no + // privilege. If no such group exists the test still runs and simply + // does not carry that particular distinction. + if let Some(gid) = other_group() { + let _ = std::os::unix::fs::chown(&path, None, Some(gid)); + } + let real_uid = std::fs::metadata(&path).unwrap().uid(); let other = real_uid.wrapping_add(1); @@ -1239,9 +1250,15 @@ mod tests { use std::os::unix::fs::{MetadataExt, PermissionsExt}; for mode in [0o700, 0o777] { + // The tempdir ITSELF is the key directory under test, not a child of + // it. With the euid override armed the whole path chain looks + // foreign, so a nested fixture would trip the ancestor walk first + // and this test would pass through that error instead, leaving the + // leaf ownership check unbound. /tmp is root-owned and sticky, so + // the ancestors of the tempdir are trusted and only the leaf is + // foreign. let dir = tempfile::tempdir().unwrap(); - let keys = dir.path().join("keys"); - std::fs::create_dir(&keys).unwrap(); + let keys = dir.path().to_path_buf(); std::fs::set_permissions(&keys, std::fs::Permissions::from_mode(mode)).unwrap(); let real_uid = std::fs::metadata(&keys).unwrap().uid(); @@ -1274,6 +1291,23 @@ mod tests { } } + /// A group this process belongs to that is not its effective gid, if there + /// is one. Used to build a fixture whose uid and gid differ so a uid/gid + /// mixup is observable; returns None on a box with no secondary groups, + /// where that distinction simply cannot be drawn. + #[cfg(unix)] + fn other_group() -> Option { + // SAFETY: getegid only reads the calling process's effective gid. + let egid = unsafe { libc::getegid() }; + let mut buf = [0 as libc::gid_t; 64]; + // SAFETY: writes at most buf.len() entries into buf and returns the count. + let n = unsafe { libc::getgroups(buf.len() as libc::c_int, buf.as_mut_ptr()) }; + if n <= 0 { + return None; + } + buf[..n as usize].iter().copied().find(|g| *g != egid) + } + /// A foreign-owned ancestor is refused before anything is created under it. /// /// This is the case that survived the leaf checks: the node creates the key