Skip to content

Commit 2a7ed47

Browse files
committed
fix(node): close remaining #194 gaps (CI lib, key parent, gossip shutdown)
Run join_or_abort lib tests in the deny-harness CI step, repair group/world-writable key parents on existing-key load, abort gossip on HTTP shutdown with per-peer shutdown polling, and log list_peers errors.
1 parent 692b894 commit 2a7ed47

3 files changed

Lines changed: 84 additions & 6 deletions

File tree

‎.github/workflows/pr-checks.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ jobs:
124124
- name: cargo test (real-node deny harness)
125125
env:
126126
DATABASE_URL: postgres://postgres:postgres@localhost:5432/gitlawb_test
127-
run: cargo test --locked -p gitlawb-node --features test-harness --test deny_harness
127+
run: cargo test --locked -p gitlawb-node --features test-harness --lib --test deny_harness
128128

129129
build-release:
130130
name: build --release

‎crates/gitlawb-node/src/db/mod.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8279,6 +8279,7 @@ mod peer_authority_tests {
82798279
/// | `seed_local_peer` (sync.rs) | excluded by test-module location: a deliberate `upsert_peer` bypass for `file://` fixtures, which the public-URL gate rejects |
82808280
/// | `a_legacy_row_can_still_refresh_its_liveness` (db/mod.rs) | test-only. Seeds a PRE-GATE row by raw SQL on purpose: `upsert_peer` cannot create one, since the gate it is testing refuses exactly that DID. The fixture models what a deployed table already holds |
82818281
/// | `manual_ping_uses_readiness_without_mutating_federation_gate` (api/peers.rs) | test-only fixture seed, same shape and same reason: the row under test must pre-exist so the assertion is about what the ping does NOT rewrite |
8282+
/// | `gossip_ping_round_requires_two_failures_before_persisting_unreachable` (lib.rs) | test-only fixture seed for the gossip hysteresis round; the row must pre-exist so the assertion is about when `mark_peer_ping` fires |
82828283
///
82838284
/// And the `upsert_peer` CALL-SITE authority table, which the ledger above
82848285
/// structurally cannot hold, because the bootstrap site issues no SQL of its own
@@ -8368,6 +8369,10 @@ mod peers_table_writer_guard {
83688369
"manual_ping_uses_readiness_without_mutating_federation_gate",
83698370
1,
83708371
),
8372+
(
8373+
"gossip_ping_round_requires_two_failures_before_persisting_unreachable",
8374+
1,
8375+
),
83718376
("mark_peer_ping", 1),
83728377
("prune_non_public_peers", 1),
83738378
("prune_self_peers", 1),

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

Lines changed: 78 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -585,14 +585,14 @@ pub async fn run() -> Result<()> {
585585
}
586586

587587
// Spawn background gossip: announce to bootstrap peers, then ping known peers periodically
588-
{
588+
let gossip_handle = {
589589
let gossip_state = state.clone();
590590
let bootstrap_peers = config.bootstrap_peers.clone();
591591
let shutdown_rx = state.subscribe_shutdown();
592592
tokio::spawn(async move {
593593
gossip_task(gossip_state, bootstrap_peers, shutdown_rx).await;
594-
});
595-
}
594+
})
595+
};
596596

597597
// Start multi-node sync worker if auto_sync is enabled
598598
if config.auto_sync {
@@ -684,6 +684,7 @@ pub async fn run() -> Result<()> {
684684
if let Some(h) = metrics_handle {
685685
h.abort();
686686
}
687+
gossip_handle.abort();
687688
serve_result?;
688689
info!("clean exit");
689690
Ok(())
@@ -1127,11 +1128,15 @@ async fn gossip_task(
11271128
_ = interval.tick() => {
11281129
let peers = match state.db.list_peers().await {
11291130
Ok(p) => p,
1130-
Err(_) => continue,
1131+
Err(e) => {
1132+
warn!(err = %e, "gossip: list_peers failed; skipping ping round");
1133+
continue;
1134+
}
11311135
};
11321136
gossip_ping_round(
11331137
state.db.as_ref(),
11341138
client.as_ref(),
1139+
&mut shutdown_rx,
11351140
&mut failed_once,
11361141
peers,
11371142
)
@@ -1150,6 +1155,7 @@ async fn gossip_task(
11501155
async fn gossip_ping_round(
11511156
db: &Db,
11521157
client: &reqwest::Client,
1158+
shutdown_rx: &mut tokio::sync::watch::Receiver<bool>,
11531159
failed_once: &mut HashSet<String>,
11541160
peers: Vec<db::PeerRecord>,
11551161
) {
@@ -1158,7 +1164,14 @@ async fn gossip_ping_round(
11581164
failed_once.retain(|did| current_dids.contains(did.as_str()));
11591165
}
11601166
for peer in peers {
1161-
let ok = ping_peer_readiness(client, &peer.http_url).await;
1167+
if *shutdown_rx.borrow() {
1168+
return;
1169+
}
1170+
let ok = tokio::select! {
1171+
biased;
1172+
_ = shutdown_rx.changed() => return,
1173+
ready = ping_peer_readiness(client, &peer.http_url) => ready,
1174+
};
11621175
match peer_ping_db_update(failed_once, &peer.did, ok) {
11631176
Some(reachable) => {
11641177
if let Err(error) = db.mark_peer_ping(&peer.did, reachable).await {
@@ -3407,6 +3420,26 @@ fn load_or_create_keypair_with(
34073420
// concurrently-publishing winner is handled by the retry in
34083421
// boot_load_key).
34093422
if key_path.exists() {
3423+
if let Some(parent) = key_path.parent().filter(|p| !p.as_os_str().is_empty()) {
3424+
#[cfg(unix)]
3425+
{
3426+
use std::os::unix::fs::PermissionsExt;
3427+
let mode = std::fs::metadata(parent)
3428+
.with_context(|| {
3429+
format!("stat identity key directory {}", parent.display())
3430+
})?
3431+
.permissions()
3432+
.mode()
3433+
& 0o777;
3434+
if mode & 0o022 != 0 {
3435+
ensure_key_parent_dir_private(parent)?;
3436+
}
3437+
}
3438+
#[cfg(not(unix))]
3439+
{
3440+
ensure_key_parent_dir_private(parent)?;
3441+
}
3442+
}
34103443
match boot_load_key(key_path, recovery_allowed, seam.load_fault) {
34113444
BootLoad::Loaded(kp) => {
34123445
release_recovery_claims(key_path, &mut claims);
@@ -3669,9 +3702,11 @@ mod gossip_ssrf_tests {
36693702
.unwrap();
36703703

36713704
let mut failed_once = HashSet::from(["did:key:z6MkRemovedPeer".to_owned()]);
3705+
let (_shutdown_tx, mut shutdown_rx) = tokio::sync::watch::channel(false);
36723706
gossip_ping_round(
36733707
state.db.as_ref(),
36743708
&production_http_client(),
3709+
&mut shutdown_rx,
36753710
&mut failed_once,
36763711
state.db.list_peers().await.unwrap(),
36773712
)
@@ -3700,6 +3735,7 @@ mod gossip_ssrf_tests {
37003735
gossip_ping_round(
37013736
state.db.as_ref(),
37023737
&production_http_client(),
3738+
&mut shutdown_rx,
37033739
&mut failed_once,
37043740
state.db.list_peers().await.unwrap(),
37053741
)
@@ -4909,6 +4945,43 @@ mod identity_key_tests {
49094945
assert_eq!(format!("{}", loaded.did()), format!("{}", kp.did()));
49104946
}
49114947

4948+
/// #194 (INV-23(a)): loading an existing key still repairs a writable parent.
4949+
#[test]
4950+
fn existing_key_load_repairs_writable_parent_dir_to_0700() {
4951+
use std::os::unix::fs::PermissionsExt;
4952+
4953+
let root = tempfile::tempdir().expect("tempdir");
4954+
let parent = root.path().join("keys");
4955+
std::fs::create_dir_all(&parent).expect("keys dir");
4956+
std::fs::set_permissions(&parent, std::fs::Permissions::from_mode(0o777))
4957+
.expect("world-writable parent");
4958+
let key_path = parent.join("identity.pem");
4959+
let kp = Keypair::generate();
4960+
std::fs::write(&key_path, kp.to_pem().expect("pem")).expect("write key");
4961+
std::fs::set_permissions(&key_path, std::fs::Permissions::from_mode(0o600))
4962+
.expect("key mode");
4963+
4964+
let loaded = load_or_create_keypair_with(
4965+
&key_path,
4966+
&|| {},
4967+
&|| {},
4968+
PublishFaults::NONE,
4969+
RecoverySeam::NONE,
4970+
)
4971+
.expect("load existing key under a writable parent must repair the parent dir");
4972+
4973+
let mode = std::fs::metadata(&parent)
4974+
.expect("parent exists")
4975+
.permissions()
4976+
.mode()
4977+
& 0o777;
4978+
assert_eq!(
4979+
mode, 0o700,
4980+
"the identity key parent directory must be owner-only after load"
4981+
);
4982+
assert_eq!(format!("{}", loaded.did()), format!("{}", kp.did()));
4983+
}
4984+
49124985
/// #194 (INV-23(b)): fallback publish markers must land 0600 even under a
49134986
/// permissive umask when the write fails and the marker survives for recovery.
49144987
#[test]

0 commit comments

Comments
 (0)