Skip to content

Commit 8d38043

Browse files
authored
Merge pull request #290 from Gitlawb/fix/273-peer-authority-gate
fix(node): bind a peer row to its DID so only its keyholder can repoint it (#273)
2 parents c10ccf1 + 5df7d2a commit 8d38043

9 files changed

Lines changed: 2194 additions & 116 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ POST /api/v1/peers/announce
309309
POST /api/v1/sync/notify
310310
```
311311

312-
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted on those two routes, but signed requests are verified when signature headers are present. Once all live peers upgrade, operators can set:
312+
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted on those two routes, but signed requests are verified when signature headers are present. Staged rollout relaxes who may announce, not who owns a peer row. An unsigned announce may register a previously unseen peer, and it may refresh an existing row whose `http_url` is unchanged, but changing an existing peer's `http_url` requires an RFC 9421 signature from that peer's own DID and is refused with 403 otherwise. An unsigned announce can only register a `did:key`, since that is the only method whose verifying key can be resolved and therefore the only kind of row anyone could ever correct through the signed path; any other DID is refused with 400. Once all live peers upgrade, operators can set:
313313

314314
```bash
315315
GITLAWB_REQUIRE_SIGNED_PEER_WRITES=true

‎crates/gitlawb-core/src/did.rs‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,19 @@ impl Did {
8080
)));
8181
}
8282

83+
// Refuse an oversized id before decoding. base58 decoding is quadratic
84+
// in its input, and both callers of this function take the string from
85+
// an untrusted request: the unauthenticated peer-announce gate, and the
86+
// HTTP-signature keyid. An ed25519 did:key method-id is a fixed 48
87+
// characters, so this bound is slack rather than a behavior change, and
88+
// it has to sit ahead of the decode to be worth anything.
89+
const MAX_METHOD_ID_LEN: usize = 64;
90+
if self.method_id().len() > MAX_METHOD_ID_LEN {
91+
return Err(Error::InvalidDid(
92+
"did:key method-specific id too long".to_string(),
93+
));
94+
}
95+
8396
let (_, bytes) =
8497
multibase::decode(self.method_id()).map_err(|e| Error::InvalidDid(e.to_string()))?;
8598

@@ -308,4 +321,52 @@ mod tests {
308321
assert_eq!(Did::web("example.com").method_id(), "example.com");
309322
assert_eq!(Did::gitlawb("z6MkSomeKey").method_id(), "z6MkSomeKey");
310323
}
324+
325+
/// An oversized method-specific id is refused on its length, before the
326+
/// base58 decode runs.
327+
///
328+
/// base58 decoding is quadratic in the input, and `to_verifying_key` is
329+
/// reached from two places that take an attacker-supplied string: the
330+
/// unauthenticated peer-announce gate, and the HTTP-signature keyid. A
331+
/// 64k-character DID measured 8s through the router without this guard
332+
/// against 4.6ms with it, on a tokio worker with no spawn_blocking, so a
333+
/// single caller could buy CPU-seconds by the thousand.
334+
///
335+
/// Asserting the MESSAGE is what makes this load-bearing: an oversized id
336+
/// errors either way, so a test that only checked `is_err()` would pass
337+
/// with the guard removed, having merely paid the cost it exists to avoid.
338+
#[test]
339+
fn to_verifying_key_rejects_an_oversized_method_id_before_decoding() {
340+
let oversized = format!("did:key:z{}", "z".repeat(64_000));
341+
let did: Did = oversized.parse().expect("method is still `key`");
342+
343+
let start = std::time::Instant::now();
344+
let err = did.to_verifying_key().expect_err("must be refused");
345+
let elapsed = start.elapsed();
346+
347+
assert!(
348+
err.to_string().contains("too long"),
349+
"must be refused on length, not after the decode: {err}"
350+
);
351+
assert!(
352+
elapsed < std::time::Duration::from_secs(1),
353+
"refusal must not pay the quadratic decode: took {elapsed:?}"
354+
);
355+
}
356+
357+
/// The bound must not reject a real key. Every `did:key` this project
358+
/// mints is a fixed 48 characters, so the limit is slack, not a behavior
359+
/// change.
360+
#[test]
361+
fn to_verifying_key_still_accepts_a_real_did_key() {
362+
use ed25519_dalek::SigningKey;
363+
let signing = SigningKey::from_bytes(&[7u8; 32]);
364+
let did = Did::from_verifying_key(&signing.verifying_key());
365+
assert_eq!(
366+
did.method_id().len(),
367+
48,
368+
"did:key method-id is fixed-width"
369+
);
370+
did.to_verifying_key().expect("a real did:key must resolve");
371+
}
311372
}

0 commit comments

Comments
 (0)