99 lines
9.2 KiB
Markdown
99 lines
9.2 KiB
Markdown
• ## Review outcome
|
||
|
||
The core direction is sound: per-device Ed25519 identity, pinned QUIC responders, signed forwarded events, explicit TOFU semantics, and keeping content trust out of scope.
|
||
However, I would not implement from this plan yet. Several security claims do not follow from the proposed mechanics, and the phases are not independently shippable as written.
|
||
|
||
### Findings
|
||
|
||
1. Blocker — a signed listener address still is not authenticated.
|
||
A signature proves who asserted listen_addr, not that they control it. The plan permits a verified handshake to rebind records (PEER_AUTH_PLAN.md:316), while the current
|
||
collision rule evicts the peer already occupying that address (crates/lanspread-peer/src/peer_db.rs:78). An authenticated attacker can therefore claim another peer’s listener
|
||
and still trigger T4. Outbound handshakes should retain the endpoint actually reached under the pinned key; inbound claims need a pinned connect-back or equivalent proof. An
|
||
unproven collision must never evict an established identity.
|
||
|
||
2. Blocker — Phase 2 authenticates only the responder.
|
||
Client certificates are omitted and application signatures arrive in Phase 3 (PEER_AUTH_PLAN.md:202), yet Phase 2 lets inbound handshakes mutate peer state. Today
|
||
accept_inbound_hello immediately trusts the supplied identity, history, and address (crates/lanspread-peer/src/services/handshake.rs:152). A copied public key in an unsigned
|
||
Hello proves no possession. Signed Hello must move into Phase 2, inbound mutation must wait, or Phases 2–3 must be combined.
|
||
|
||
Similarly, Phase 3 still lets B forge A’s relayed Call-to-Play history until signed events arrive in Phase 4. Those phases must ship together or temporarily reject third-
|
||
party snapshot authority.
|
||
|
||
3. Blocker — compacted Call-to-Play tombstones lack creator proof.
|
||
The proposed call_id needs create_nonce, but that nonce appears in neither the proposed body nor today’s schema (PEER_AUTH_PLAN.md:281, crates/lanspread-proto/src/lib.rs:43).
|
||
More fundamentally, current compaction discards Create and retains only Start/Cancel (crates/lanspread-peer/src/call_to_play.rs:340). A fresh peer can verify the terminal
|
||
signer but cannot prove that signer created the call. Retain the signed root or carry a verifiable root/creator commitment in the tombstone. The replication contract cannot
|
||
remain “exactly unchanged.”
|
||
|
||
4. Blocker — event IDs and the verification cache remain attacker-controlled.
|
||
Event IDs remain globally arbitrary, while the store rejects same-ID/different-body objects atomically (crates/lanspread-peer/src/call_to_play.rs:100). An attacker can reuse
|
||
an observed legitimate ID, pre-seed a fresh peer, and make later legitimate snapshots conflict. An ID-only signature cache (PEER_AUTH_PLAN.md:300) can also skip verification
|
||
for a different body after compaction. Event identity should be author/content-or-nonce bound; cache entries must bind the exact signed-object digest and have bounded
|
||
lifetime.
|
||
|
||
5. High — the ±10-minute event rule rejects valid history.
|
||
Full terminal history lasts 15 minutes and tombstones last for the session, while active or scheduled calls may be older still. The proposed local-clock check
|
||
(PEER_AUTH_PLAN.md:303) would make legitimate historical acceptance peer-dependent. Apply future-skew checks at direct publication if desired, but do not expire signatures
|
||
merely because an event is relayed later.
|
||
|
||
6. Blocker — T8/T9 and general resource exhaustion remain open.
|
||
Terminal histories and tombstones are exempt from the proposed bounds, so one key can issue unlimited Create+Cancel pairs. Sybil keys and relayed authors defeat per-author
|
||
quotas; sixteen suggested 256-event quotas still fill the global 4096 unresolved-event capacity. Add absolute object and byte limits covering unresolved events, terminal
|
||
history, tombstones, caches, and distinct authors, plus local-capacity reservation and defined eviction/rejection behavior.
|
||
|
||
The same threat model also requires connection, stream, signature-verification, transfer, disk-read, and expensive-operation limits. The server currently spawns work per
|
||
connection and stream (crates/lanspread-peer/src/services/server.rs:58); self-issued signatures do not make a requester trustworthy. These controls cannot remain optional
|
||
Phase 6 hardening.
|
||
|
||
7. High — replay protection does not establish T7.
|
||
A 1024-entry LRU can evict a still-fresh nonce, restart loses the cache, concurrent check-and-insert needs atomicity, and the ±120-second window introduces hard clock trust
|
||
despite the non-goal (PEER_AUTH_PLAN.md:249). A delayed legitimate Goodbye can also arrive after a newer handshake and remove the new incarnation. Use session/challenge or
|
||
incarnation binding—particularly for Goodbye—or persist all still-valid replay state and narrow the stated guarantee.
|
||
|
||
8. Blocker — identity storage needs a crash-safe backend state machine.
|
||
“Keyring unavailable” cannot mean the same thing as “no key exists”: falling through to a file can fork an established identity. The sidecar-selected backend must become
|
||
authoritative, with explicit handling for missing, locked, denied, corrupt, and mismatched states. The secret and identity.json also cannot be atomically committed together;
|
||
concurrent starts, crashes, import, reset, and rotation need a process lock, generation-based commit protocol, and reconciliation rules.
|
||
|
||
The persistence rationale is also inaccurate for the GUI: production passes Tauri’s app_data_dir, not the core ~/.lanspread fallback (crates/lanspread-tauri-deno-ts/src-
|
||
tauri/src/lib.rs:2437). Reinstall survival needs a platform/package matrix. The fixed keyring account additionally describes one identity per OS account, not necessarily one
|
||
per installation.
|
||
|
||
9. Blocker — mandatory expected identity is not threaded through the runtime.
|
||
The plan bans address-only connections (PEER_AUTH_PLAN.md:210), but direct connect is still ConnectPeer(SocketAddr) (crates/lanspread-peer/src/lib.rs:263), and downloads,
|
||
retries, streamed installs, healing, shutdown, and liveness frequently retain only addresses. Phase 2 needs a first-class PeerEndpoint { peer_id, addr } throughout. Direct
|
||
connect must require an expected fingerprint/ID or define an explicit user-confirmed TOFU bootstrap.
|
||
|
||
10. High — trust and identity lifecycle semantics are incomplete.
|
||
Blocking at direct handshake/connection does not block A’s events relayed by allowed B. The common event-merge boundary needs a block/admission policy, including existing
|
||
history and unblock recovery. The trust schema also has only known | blocked, so it cannot represent the promised persistent “new,” reviewed, or acknowledged state; security-
|
||
sensitive writes such as block/pin must not be merely debounced.
|
||
|
||
Phase 1 can already produce fatal identity errors, but usable repair is deferred to Phase 5; today startup failure is only logged (crates/lanspread-tauri-deno-ts/src-tauri/
|
||
src/lib.rs:2118). Minimum retry/import/reset/export must accompany storage. Seed export also creates two indistinguishable live devices unless backup versus transfer,
|
||
retirement, and stale-copy behavior are defined.
|
||
|
||
11. High — the cryptographic wire contract is not normative enough.
|
||
The signature transcript still contains an ellipsis (PEER_AUTH_PLAN.md:247); it must define every field, order, tag value, integer width/endian, encoding, and protocol-
|
||
version binding, plus require recipient-local, expected-context, and response-sender-equals-TLS-pinned-responder checks. Add golden vectors.
|
||
|
||
Additional concrete corrections:
|
||
- A custom rustls verifier must verify TLS 1.3 CertificateVerify, not only inspect SPKI (/home/pfs/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustls-0.23.42/src/
|
||
verify.rs:69).
|
||
|
||
- Bytes becomes a JSON numeric array, not base64, without an explicit serializer.
|
||
- The proposed crate graph is contradictory: lanspread-identity cannot construct lanspread-proto types while both dependency directions are forbidden
|
||
(PEER_AUTH_PLAN.md:370).
|
||
|
||
- rcgen must disable its default ring feature to preserve the one-crypto-stack claim.
|
||
- The P-256 fallback needs a binding visible before accepting the TLS channel; an identity statement delivered only inside that channel is too late.
|
||
|
||
12. High — adversarial tests and documentation arrive too late.
|
||
The hostile harness is described as the trust model’s regression net but deferred to Phase 7 (PEER_AUTH_PLAN.md:523). Each trust boundary must land with its negative
|
||
scenarios and documentation. Per-phase gates should include targeted just peer-cli-tests, the honest-path matrix after major protocol phases, and just build for GUI changes—
|
||
not only manual containers, whose recipes can use an old image (justfile:46). Phase 7 should be a final audit, not the first security acceptance pass.
|
||
|
||
My recommended prerequisite is a revised Phase 0 that settles the endpoint model, exact wire transcripts, storage state machine, resource budgets, crate graph, and TLS spike.
|
||
Then Phase 1 can ship storage with recovery; Phase 2 can ship mutually authenticated handshakes and address proof; signed envelopes and independently verifiable relayed events
|
||
should ship atomically or behind a safe intermediate restriction.
|