diff --git a/.env.example b/.env.example index b70d1117..97ced904 100644 --- a/.env.example +++ b/.env.example @@ -7,6 +7,17 @@ # Generate with: gl identity new GITLAWB_KEY=/data/keys/identity.pem +# 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) GITLAWB_PUBLIC_URL=https://your-node.example.com diff --git a/Cargo.lock b/Cargo.lock index b7050bc6..ff0e5b9f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3484,6 +3484,7 @@ dependencies = [ "tracing-subscriber", "unicode-normalization", "uuid", + "zeroize", "zstd", ] diff --git a/Dockerfile b/Dockerfile index 3b466945..f030de50 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 1588161f..8688f300 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. 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. | @@ -362,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 diff --git a/crates/gitlawb-node/Cargo.toml b/crates/gitlawb-node/Cargo.toml index c583569c..b4b8b8be 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/config.rs b/crates/gitlawb-node/src/config.rs index fefa063e..26d37d50 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). @@ -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 @@ -583,6 +597,48 @@ 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. + // `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 self.p2p_key_path.starts_with("~/") && p2p_key_path == Path::new(&self.p2p_key_path) { + return Err(format!( + "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 + )); + } + Ok(()) } } @@ -973,4 +1029,81 @@ 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 [ + // 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", + // 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() + .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", + "/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/main.rs b/crates/gitlawb-node/src/main.rs index 10aaca5b..bb612298 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -229,22 +229,35 @@ 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 + } + } } + // 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 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 80e28a4a..dff49971 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -4,15 +4,16 @@ //! - 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}; +use std::path::{Component, 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}; @@ -24,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}; @@ -164,34 +166,358 @@ 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("."), + } +} + +/// 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 { + 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 + } + Component::CurDir => {} + } + } + !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 { + 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 names_no_usable_directory(key_path) { + return Err(anyhow::anyhow!( + "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() + )); + } + + // 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. + ensure_key_dir(parent)?; + + if key_path.exists() { + return read_p2p_keypair(key_path); + } + + let kp = identity::Keypair::generate_ed25519(); + // 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(()) => { + 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 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 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()))?; + + #[cfg(unix)] + { + 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_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); + // 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")); + } + + file.write_all(bytes)?; + // 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. 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(()) +} + +#[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 +/// 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() + ); + } + } + + // 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 + // `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() + ); + } + + 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 /// 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"); @@ -443,6 +769,410 @@ 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" + ); + } + + // ---- 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. + + /// 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 { + 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] + #[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"); + + // 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!( + mode & 0o777, + 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"); + + // 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)] + #[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}" + ); + + // 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", + // 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)), + "{path:?} must be rejected" + ); + } + + for path in [ + "keys/p2p.key", + "./keys/p2p.key", + "keys/nested/p2p.key", + "/data/keys/p2p.key", + "/data/p2p.key", + "/p2p.key", + ] { + assert!( + !names_no_usable_directory(Path::new(path)), + "{path:?} must be accepted" + ); + } + } + + #[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)] + #[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"); + let msg = format!("{err:#}"); + assert!( + 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] fn ref_update_event_round_trip_with_owner_did() { let event = RefUpdateEvent { diff --git a/infra/fly/fly.toml b/infra/fly/fly.toml index 05445e5a..ffda6e95 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 785e4da7..16037afe 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 d1ca979a..85d49302 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'