Skip to content

Commit e3eac37

Browse files
committed
fix(node): publish the identity key atomically and wait a real deadline
The create path did create_new(final)+write_all, so the final path appeared as an empty inode before the PEM was flushed, and a losing/fast-path start only retried ~100ms (50x2ms). On a slow or stalled filesystem the winner's write can exceed that window, so every other start failed boot with 'invalid PEM key' (#194). Publish atomically instead: write the full PEM to a sibling temp, then hard_link it into place. hard_link is atomic and fails if the target exists, so the final path only ever appears COMPLETE (no partial-read window), a lost race never clobbers the winner, and a crashed writer leaves only a temp rather than a partial final that would wedge later starts. load_racing_key now polls on a wall-clock KEY_RACE_DEADLINE (5s) instead of a fixed 100ms count. Hoisted load_existing_key/load_racing_key to module level for testability. Adversarial RED->GREEN: a 250ms-slow winner is waited out (RED at the old 100ms); a reader watching the final never sees a partial file (RED with create_new+write); a losing publish does not clobber the winner; 500 tests pass.
1 parent fb8685d commit e3eac37

1 file changed

Lines changed: 243 additions & 105 deletions

File tree

‎crates/gitlawb-node/src/lib.rs‎

Lines changed: 243 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,63 +1081,133 @@ fn ensure_key_mode_0600(path: &std::path::Path) -> Result<()> {
10811081
Ok(())
10821082
}
10831083

1084-
fn load_or_create_keypair(config: &Config) -> Result<Keypair> {
1085-
let key_path = config.resolved_key_path();
1084+
/// Outcome of an atomic key publish.
1085+
enum KeyPublish {
1086+
/// We created the key file; it now holds the complete PEM.
1087+
Won,
1088+
/// Another concurrent start already created it; ours was discarded.
1089+
Lost,
1090+
}
10861091

1087-
// Load an already-provisioned key. On Unix, defensively tighten looser
1088-
// permissions to 0600 (do NOT reject a loose key — that would break
1089-
// existing deployments; just narrow them).
1090-
fn load_existing(path: &std::path::Path) -> Result<Keypair> {
1091-
#[cfg(unix)]
1092-
{
1093-
use std::os::unix::fs::PermissionsExt;
1094-
if let Ok(meta) = std::fs::metadata(path) {
1095-
if meta.permissions().mode() & 0o777 != 0o600 {
1096-
// Do NOT silently ignore a failed tighten (#194, F2): surface it
1097-
// so a key we cannot secure is not read and used exposed.
1098-
std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))
1099-
.with_context(|| {
1100-
format!("could not tighten identity key {} to 0600", path.display())
1101-
})?;
1102-
}
1092+
/// Wall-clock budget for reading a key another start may still be publishing.
1093+
/// The atomic publish (`publish_key_atomically`) means a *present* key file is
1094+
/// always complete, so this normally succeeds on the first read; the deadline
1095+
/// only covers cross-host cache lag. It is a wall-clock budget, NOT a fixed
1096+
/// retry count, so a slow/stalled filesystem cannot starve a losing start after
1097+
/// an arbitrarily short (~100ms) window (#194).
1098+
const KEY_RACE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5);
1099+
1100+
/// Load an already-provisioned identity key. On Unix, defensively tighten looser
1101+
/// permissions to 0600 (do NOT reject a loose key — that would break existing
1102+
/// deployments; just narrow them), then verify the mode is actually 0600.
1103+
fn load_existing_key(path: &std::path::Path) -> Result<Keypair> {
1104+
#[cfg(unix)]
1105+
{
1106+
use std::os::unix::fs::PermissionsExt;
1107+
if let Ok(meta) = std::fs::metadata(path) {
1108+
if meta.permissions().mode() & 0o777 != 0o600 {
1109+
// Do NOT silently ignore a failed tighten (#194, F2): surface it
1110+
// so a key we cannot secure is not read and used exposed.
1111+
std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))
1112+
.with_context(|| {
1113+
format!("could not tighten identity key {} to 0600", path.display())
1114+
})?;
11031115
}
1104-
// Verify the key is actually 0600 before using it — a chmod that
1105-
// succeeded-but-no-op'd (some mounts) still leaves it exposed.
1106-
ensure_key_mode_0600(path)?;
11071116
}
1108-
let pem = std::fs::read_to_string(path)
1109-
.with_context(|| format!("failed to read key from {}", path.display()))?;
1110-
let kp = Keypair::from_pem(&pem).map_err(|e| anyhow::anyhow!("invalid PEM key: {e}"))?;
1111-
info!(path = %path.display(), "loaded existing identity");
1112-
Ok(kp)
1117+
// Verify the key is actually 0600 before using it — a chmod that
1118+
// succeeded-but-no-op'd (some mounts) still leaves it exposed.
1119+
ensure_key_mode_0600(path)?;
11131120
}
1121+
let pem = std::fs::read_to_string(path)
1122+
.with_context(|| format!("failed to read key from {}", path.display()))?;
1123+
let kp = Keypair::from_pem(&pem).map_err(|e| anyhow::anyhow!("invalid PEM key: {e}"))?;
1124+
info!(path = %path.display(), "loaded existing identity");
1125+
Ok(kp)
1126+
}
11141127

1115-
// Load a key another concurrent start may still be writing. `create_new`
1116-
// makes the inode appear before its PEM is flushed, so a race loser — or a
1117-
// fast-path `exists()` that caught the winner mid-write — can momentarily
1118-
// read an empty/partial file. Retry briefly until it parses rather than
1119-
// crashing the boot. Bounded (~100ms); an already-provisioned key parses on
1120-
// the first attempt with no sleep.
1121-
fn load_racing(path: &std::path::Path) -> Result<Keypair> {
1122-
let mut last_err = None;
1123-
for _ in 0..50 {
1124-
match load_existing(path) {
1125-
Ok(kp) => return Ok(kp),
1126-
Err(e) => {
1127-
last_err = Some(e);
1128-
std::thread::sleep(std::time::Duration::from_millis(2));
1129-
}
1130-
}
1128+
/// Load a key another concurrent start may still be publishing, polling until it
1129+
/// parses or `KEY_RACE_DEADLINE` elapses. An already-provisioned key parses on
1130+
/// the first attempt with no sleep.
1131+
fn load_racing_key(path: &std::path::Path) -> Result<Keypair> {
1132+
let deadline = std::time::Instant::now() + KEY_RACE_DEADLINE;
1133+
let mut last_err;
1134+
loop {
1135+
match load_existing_key(path) {
1136+
Ok(kp) => return Ok(kp),
1137+
Err(e) => last_err = e,
11311138
}
1132-
Err(last_err.unwrap_or_else(|| {
1133-
anyhow::anyhow!("key at {} unreadable after create race", path.display())
1134-
}))
1139+
if std::time::Instant::now() >= deadline {
1140+
return Err(last_err);
1141+
}
1142+
std::thread::sleep(std::time::Duration::from_millis(2));
1143+
}
1144+
}
1145+
1146+
/// Publish `pem` to `final_path` atomically: write the full bytes to a sibling
1147+
/// temp file, then `hard_link` the temp into place. `hard_link` is atomic and
1148+
/// fails if `final_path` already exists, which gives three guarantees at once:
1149+
/// (a) the final path only ever appears with COMPLETE content — a concurrent
1150+
/// reader never observes an empty/half-written key, unlike a
1151+
/// `create_new`+`write_all` that exposes an empty inode before the PEM is
1152+
/// flushed (#194);
1153+
/// (b) a lost race never clobbers the winner — exactly one publisher links;
1154+
/// (c) a crashed publisher leaves only the temp, never a partial final that
1155+
/// would wedge every later start on `invalid PEM key`.
1156+
/// `before_link` is a no-op in production; tests use it to widen the
1157+
/// post-write / pre-link window deterministically.
1158+
fn publish_key_atomically(
1159+
final_path: &std::path::Path,
1160+
pem: &[u8],
1161+
before_link: &dyn Fn(),
1162+
) -> Result<KeyPublish> {
1163+
use std::io::Write;
1164+
let dir = final_path
1165+
.parent()
1166+
.filter(|p| !p.as_os_str().is_empty())
1167+
.unwrap_or_else(|| std::path::Path::new("."));
1168+
let stem = final_path
1169+
.file_name()
1170+
.and_then(|s| s.to_str())
1171+
.unwrap_or("identity.pem");
1172+
// Unique per call (process-global counter) so concurrent publishers never
1173+
// collide on the temp name; `create_new` below is the backstop.
1174+
static SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
1175+
let seq = SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
1176+
let tmp = dir.join(format!(".{stem}.tmp.{}.{seq}", std::process::id()));
1177+
1178+
let mut opts = std::fs::OpenOptions::new();
1179+
opts.write(true).create_new(true);
1180+
#[cfg(unix)]
1181+
{
1182+
use std::os::unix::fs::OpenOptionsExt;
1183+
opts.mode(0o600);
11351184
}
1185+
let mut f = opts
1186+
.open(&tmp)
1187+
.with_context(|| format!("create temp key {}", tmp.display()))?;
1188+
// On a failed write, remove the temp so nothing partial is ever linked (#194, F1).
1189+
write_key_or_cleanup(&tmp, f.write_all(pem))?;
1190+
drop(f);
1191+
1192+
before_link();
1193+
1194+
let linked = std::fs::hard_link(&tmp, final_path);
1195+
let _ = std::fs::remove_file(&tmp);
1196+
match linked {
1197+
Ok(()) => Ok(KeyPublish::Won),
1198+
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => Ok(KeyPublish::Lost),
1199+
Err(e) => Err(e)
1200+
.with_context(|| format!("link identity key into place at {}", final_path.display())),
1201+
}
1202+
}
1203+
1204+
fn load_or_create_keypair(config: &Config) -> Result<Keypair> {
1205+
let key_path = config.resolved_key_path();
11361206

11371207
// Fast path for the common already-provisioned case (still race-safe: a
1138-
// concurrently-writing winner is handled by the retry inside `load_racing`).
1208+
// concurrently-publishing winner is handled by the retry in load_racing_key).
11391209
if key_path.exists() {
1140-
return load_racing(&key_path);
1210+
return load_racing_key(&key_path);
11411211
}
11421212

11431213
let kp = Keypair::generate();
@@ -1149,61 +1219,15 @@ fn load_or_create_keypair(config: &Config) -> Result<Keypair> {
11491219
std::fs::create_dir_all(parent)?;
11501220
}
11511221

1152-
// Create atomically. `create_new` fails if the file already exists, which
1153-
// closes the exists()->write TOCTOU: a lost race deterministically loads
1154-
// the winner's key instead of overwriting it.
1155-
#[cfg(unix)]
1156-
{
1157-
use std::io::Write;
1158-
use std::os::unix::fs::OpenOptionsExt;
1159-
match std::fs::OpenOptions::new()
1160-
.write(true)
1161-
.create_new(true)
1162-
.mode(0o600)
1163-
.open(&key_path)
1164-
{
1165-
Ok(mut f) => {
1166-
// The file is 0600 from creation, so the PEM is never
1167-
// world-readable, even momentarily. On a failed write, remove the
1168-
// empty/partial file so a later start regenerates (#194, F1).
1169-
write_key_or_cleanup(&key_path, f.write_all(pem.as_bytes()))?;
1170-
}
1171-
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
1172-
// A concurrent start won the race and wrote the identity; load
1173-
// theirs rather than propagating the error or overwriting.
1174-
return load_racing(&key_path);
1175-
}
1176-
Err(e) => {
1177-
return Err(e)
1178-
.with_context(|| format!("failed to create key at {}", key_path.display()));
1179-
}
1180-
}
1181-
}
1182-
#[cfg(not(unix))]
1183-
{
1184-
use std::io::Write;
1185-
match std::fs::OpenOptions::new()
1186-
.write(true)
1187-
.create_new(true)
1188-
.open(&key_path)
1189-
{
1190-
Ok(mut f) => {
1191-
// On a failed write, remove the empty/partial file so a later
1192-
// start regenerates instead of re-parsing it forever (#194, F1).
1193-
write_key_or_cleanup(&key_path, f.write_all(pem.as_bytes()))?;
1194-
}
1195-
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
1196-
return load_racing(&key_path);
1197-
}
1198-
Err(e) => {
1199-
return Err(e)
1200-
.with_context(|| format!("failed to create key at {}", key_path.display()));
1201-
}
1222+
// Publish atomically: the final path only ever appears complete, and a lost
1223+
// race loads the winner's key rather than overwriting it.
1224+
match publish_key_atomically(&key_path, pem.as_bytes(), &|| {})? {
1225+
KeyPublish::Won => {
1226+
info!(path = %key_path.display(), did = %kp.did(), "generated new node identity");
1227+
Ok(kp)
12021228
}
1229+
KeyPublish::Lost => load_racing_key(&key_path),
12031230
}
1204-
1205-
info!(path = %key_path.display(), did = %kp.did(), "generated new node identity");
1206-
Ok(kp)
12071231
}
12081232

12091233
#[cfg(test)]
@@ -1269,10 +1293,124 @@ mod gossip_ssrf_tests {
12691293

12701294
#[cfg(all(test, unix))]
12711295
mod identity_key_tests {
1272-
use super::{load_or_create_keypair, Config};
1296+
use super::{
1297+
load_or_create_keypair, load_racing_key, publish_key_atomically, Config, KeyPublish,
1298+
Keypair,
1299+
};
12731300
use clap::Parser;
12741301
use std::os::unix::fs::PermissionsExt;
12751302

1303+
// #194 (P2): the racing loader must wait out a SLOW winner on a meaningful
1304+
// wall-clock deadline, not give up after an arbitrary ~100ms window. Simulate
1305+
// a winner that only publishes the key after 250ms (past the old 100ms budget)
1306+
// and assert the loader waits it out instead of failing with `invalid PEM key`.
1307+
// RED with the old `for _ in 0..50` (2ms) fixed-count loop.
1308+
#[test]
1309+
fn load_racing_key_waits_out_a_slow_winner() {
1310+
let dir = tempfile::tempdir().expect("tempdir");
1311+
let key_path = dir.path().join("identity.pem");
1312+
let pem = Keypair::generate().to_pem().expect("pem");
1313+
1314+
let writer_path = key_path.clone();
1315+
let writer = std::thread::spawn(move || {
1316+
std::thread::sleep(std::time::Duration::from_millis(250));
1317+
publish_key_atomically(&writer_path, pem.as_bytes(), &|| {}).expect("publish");
1318+
});
1319+
1320+
let started = std::time::Instant::now();
1321+
let kp = load_racing_key(&key_path)
1322+
.expect("racing load must wait out the slow winner, not fail");
1323+
let waited = started.elapsed();
1324+
writer.join().expect("writer joins");
1325+
1326+
assert!(
1327+
waited >= std::time::Duration::from_millis(200),
1328+
"loader should have waited for the ~250ms-slow winner, only waited {waited:?}"
1329+
);
1330+
assert!(
1331+
!format!("{}", kp.did()).is_empty(),
1332+
"loaded the published key"
1333+
);
1334+
}
1335+
1336+
// #194 (P2): the atomic publish preserves the single-winner no-clobber
1337+
// guarantee — a second publish of a DIFFERENT key must lose and leave the
1338+
// winner's key untouched — and it cleans up its temp files.
1339+
#[test]
1340+
fn publish_wins_then_second_publish_loses_without_clobbering() {
1341+
let dir = tempfile::tempdir().expect("tempdir");
1342+
let key_path = dir.path().join("identity.pem");
1343+
let pem_a = Keypair::generate().to_pem().expect("pem a");
1344+
let pem_b = Keypair::generate().to_pem().expect("pem b");
1345+
assert_ne!(pem_a.as_str(), pem_b.as_str(), "fixtures must differ");
1346+
1347+
let out = publish_key_atomically(&key_path, pem_a.as_bytes(), &|| {}).expect("publish a");
1348+
assert!(matches!(out, KeyPublish::Won), "first publish wins");
1349+
assert_eq!(
1350+
std::fs::read_to_string(&key_path).unwrap().as_str(),
1351+
pem_a.as_str(),
1352+
"final holds the full PEM"
1353+
);
1354+
1355+
let out = publish_key_atomically(&key_path, pem_b.as_bytes(), &|| {}).expect("publish b");
1356+
assert!(matches!(out, KeyPublish::Lost), "second publish loses");
1357+
assert_eq!(
1358+
std::fs::read_to_string(&key_path).unwrap().as_str(),
1359+
pem_a.as_str(),
1360+
"the winner's key must NOT be clobbered by a losing publish"
1361+
);
1362+
1363+
let leftovers: Vec<_> = std::fs::read_dir(dir.path())
1364+
.unwrap()
1365+
.filter_map(|e| e.ok())
1366+
.filter(|e| e.file_name().to_string_lossy().contains(".tmp."))
1367+
.collect();
1368+
assert!(
1369+
leftovers.is_empty(),
1370+
"temp files must be cleaned up, found {leftovers:?}"
1371+
);
1372+
}
1373+
1374+
// #194 (P2, the core of jatmn's option b): while a winner is between writing
1375+
// its temp and linking it into place, a reader watching the FINAL path must
1376+
// see it absent or COMPLETE — never empty/partial. RED with a
1377+
// create_new(final)+write approach, which exposes an empty final for the whole
1378+
// write window.
1379+
#[test]
1380+
fn publish_never_exposes_a_partial_final() {
1381+
let dir = tempfile::tempdir().expect("tempdir");
1382+
let key_path = std::sync::Arc::new(dir.path().join("identity.pem"));
1383+
let pem = Keypair::generate().to_pem().expect("pem");
1384+
1385+
let wp = key_path.clone();
1386+
let writer = std::thread::spawn(move || {
1387+
// Hold the post-write / pre-link window open for 200ms.
1388+
publish_key_atomically(&wp, pem.as_bytes(), &|| {
1389+
std::thread::sleep(std::time::Duration::from_millis(200));
1390+
})
1391+
.expect("publish");
1392+
});
1393+
1394+
let deadline = std::time::Instant::now() + std::time::Duration::from_millis(400);
1395+
let mut saw_complete = false;
1396+
while std::time::Instant::now() < deadline {
1397+
if key_path.exists() {
1398+
let body = std::fs::read_to_string(&*key_path).unwrap_or_default();
1399+
assert!(
1400+
Keypair::from_pem(&body).is_ok(),
1401+
"final key observed in a partial/empty state: {body:?}"
1402+
);
1403+
saw_complete = true;
1404+
}
1405+
std::thread::sleep(std::time::Duration::from_millis(1));
1406+
}
1407+
writer.join().expect("writer joins");
1408+
assert!(
1409+
saw_complete,
1410+
"reader should have observed the completed key within the window"
1411+
);
1412+
}
1413+
12761414
// A freshly created identity key must be 0600 immediately, with no
12771415
// world-readable disclosure window (the atomic create_new(...).mode(0o600)
12781416
// guarantee). This is the RED-then-GREEN anchor for the perms fix: the old
@@ -1340,10 +1478,10 @@ mod identity_key_tests {
13401478

13411479
// The create-race arm the single-threaded tests skip (they enter via the
13421480
// `exists()` fast path). N concurrent starts on ONE fresh path must converge
1343-
// on a single identity: the `create_new` losers hit `AlreadyExists` and load
1344-
// the winner's key rather than overwriting it. RED: revert the write to a
1345-
// plain `fs::write` (no `create_new`) and each thread returns its own
1346-
// freshly generated key, so the DIDs diverge.
1481+
// on a single identity: the atomic publish links exactly one winner and the
1482+
// losers hit `AlreadyExists` and load the winner's key rather than overwriting
1483+
// it. RED: replace the atomic publish with a plain `fs::write` and each thread
1484+
// returns its own freshly generated key, so the DIDs diverge.
13471485
#[test]
13481486
fn concurrent_starts_converge_on_one_identity() {
13491487
let dir = tempfile::tempdir().expect("tempdir");

0 commit comments

Comments
 (0)