diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-11 00:19:06 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-11 00:19:06 +0200 |
| commit | f059cb118c556d1f0279350507f74b8a47d5a98a (patch) | |
| tree | 9a97762a844038a06134b4b7dcead1758477dfc1 /docs/first-review.md | |
| parent | b045ba0010d69360b6a0265eb7c73a07900fe328 (diff) | |
| download | meshbay-f059cb118c556d1f0279350507f74b8a47d5a98a.tar.gz | |
docs: remove the documents MESHBAY_DESIGN.md replaces
Twenty-four files, about 17 000 lines: the two architecture drafts, the three
security reviews, eleven design notes, the roadmap, the decisions file, the v1–v4
archive, the deprecated user guide and the stale quickstart. Their content is in
MESHBAY_DESIGN.md, and git history holds the originals.
The reason to delete rather than keep bannered: a document that is superseded but
present still gets read, and a reader cannot always tell which of two accounts of
one mechanism is the live one. That was the argument for retiring the user guide
rather than repairing it, and it applies to the whole set.
What made this safe is the concordance. Roughly 290 comments and docstrings cite
these files by section — `musicbay.md §6`, `mediacenter.md §5.5`, `draft-v6 §2.11`
— and section 16 maps every one onto its replacement, so not a single comment
needs editing to stay followable. It now says plainly that the files are gone and
where to recover them, and it gained rows for the three reviews (their findings
are section 13), and for the two guides.
Four kept documents pointed into the set and were repointed first: `playlists.md`
(nine references — it is a live proposal and must not dangle), `WINDOWS-PORT.md`,
and CLAUDE.md's example. No dangling reference remains outside section 16.
Two files were dropped from the list after checking what they hold. `HTTPS.md` is
an operational runbook — Caddy, certificate renewal, DNS, troubleshooting — and
MESHBAY_DESIGN.md deliberately covers no operations, so nothing would replace it;
the versioned Caddyfile is the config, not the procedure. `cast-smart-tv.md` is
the plan for the unbuilt DLNA phase of a feature whose first two phases ship, and
section 11.4 summarises it in four lines rather than carrying the SSDP/UPnP work.
There is no user guide now, and section 0.1 says so rather than leaving a reader
to discover it.
Suites green: 2258 passed, 4 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVoHVCcfBqud6ZjG4db3y7
Diffstat (limited to 'docs/first-review.md')
| -rw-r--r-- | docs/first-review.md | 382 |
1 files changed, 0 insertions, 382 deletions
diff --git a/docs/first-review.md b/docs/first-review.md deleted file mode 100644 index 17d5b36..0000000 --- a/docs/first-review.md +++ /dev/null @@ -1,382 +0,0 @@ -# MeshBay — First Architecture & Security Review - -> **Superseded by `MESHBAY_DESIGN.md`.** This was the first security review; its design -> content now lives in §13.1, and the invariants each finding names throughout. -> -> It is kept because code comments, tests and other documents cite its -> sections and its labels, and because it records reasoning a synthesis -> compresses. **Where it disagrees with `MESHBAY_DESIGN.md`, the design -> document is right; where either disagrees with the code, the code is.** -> `MESHBAY_DESIGN.md` §16 maps every section reference here onto its -> replacement, and §13 defines every label. - -> Date: 2026-08-10 -> Scope: design-level review of the cryptographic architecture, trust model, and -> security properties as specified in draft v3 (archived in `old-draft.md`) and implemented -> through Phases 1-6 (81 tests, demo-v2 validated). -> -> This review does NOT assess the security of the demo/test deployment. It evaluates -> whether the architecture, as designed, provides a robust foundation for a secure -> decentralized platform. - ---- - -## Executive Summary - -The cryptographic architecture is **strong and well-designed**. The algorithm -choices are modern and correct, the trust model is sound, and the key hierarchy -is properly separated. The six POC spikes were genuinely useful — the jti fix -(Spike 3), the Argon2id recalibration (Spike 1), and the GEK wrapping protocol -confirmation (Spike 6) are exactly the kind of findings that save projects from -shipping real vulnerabilities. - -There are **no fatal design flaws**. The issues found are fixable before Phase 7, -and the most important one (Double Ratchet group model) should be resolved -before writing production chat code. - -Classification: **Critical** (must fix before production), **Significant** (design -gap, fix before Phase 8), **Minor** (improvement, can schedule), **Note** (observation, -no action required). - ---- - -## What Is Solid - -These design decisions are correct and represent genuine security engineering: - -**1. Hub-blind GEK wrapping (ECIES-like)** -The wrapping protocol (ephemeral X25519 + HKDF + ChaCha20-Poly1305 with AAD) is -textbook ECIES done right. The hub stores opaque blobs, the ephemeral keypair -ensures each wrapping produces different ciphertext, and the AAD binding to -`pk_recipient` prevents bundle swapping attacks. This is the most important -crypto decision in the system and it's correct. - -**2. Ed25519 identity verification independent of TLS** -Nodes use self-signed TLS certs for transport confidentiality only. Client -verifies the node's Ed25519 public key (from hub) at the MNP handshake layer. -This decouples transport security from identity — the right design for a system -where nodes can't get CA-signed certificates. - -**3. Mandatory jti in JWT** -The Spike 3 finding was critical. Ed25519 is deterministic — without jti, two -tokens issued in the same second are bit-identical. Adding UUID4 jti to every -token was the correct fix. The architecture now enables per-token revocation. - -**4. On-the-fly encryption model** -Files stored in plaintext on the node, encrypted at read time with per-chunk keys -derived from the GEK via HKDF. This avoids the double-storage problem -(encrypted + plaintext) and makes GEK rotation feasible without re-encrypting -terabytes on disk. - -**5. Domain separation in HKDF** -Every key derivation uses a distinct `info` string (`meshbay:gek_wrap:v1`, -`meshbay:ratchet:root:v1`, etc.). The AES variant adds `:aes` suffix to chunk -key derivation. This is a small detail that prevents cross-protocol key reuse -and shows mature crypto engineering. - -**6. Transport abstraction layer** -The `Transport` interface allowing TCP+TLS v1 → QUIC v2 migration without -protocol changes was a good architectural decision confirmed by the successful -demo-v2 QUIC validation. - -**7. Refresh token stored as blake3 hash** -Server never stores the raw refresh token — only its hash. Correct pattern. -Database breach doesn't leak usable refresh tokens. - ---- - -## Critical Issues - -### C1 — Double Ratchet is not suitable for group chat as described - -**Location:** `meshbay_common/ratchet.py`, draft v3 section 6.6 - -**Problem:** The draft states "all members share the same ratchet state seeded -from the group GEK." The Signal Double Ratchet is designed for **pairwise** -(1:1) communication. It fundamentally cannot work as a shared group state: - -- If all N members share a single ratchet state, each member advancing the - sending chain desynchronizes all other members. Message 5 from Alice and - message 5 from Bob would use the same chain key, producing a nonce/key reuse - — a catastrophic failure for ChaCha20-Poly1305 and AES-GCM. -- The current `RatchetState` class has one `CKs` (sending chain) and one `CKr` - (receiving chain), confirming it's a pairwise protocol. - -**What Signal actually does for groups:** Signal uses a different protocol called -**Sender Keys** (described in their "Group Protocol" specification). Each member -has their own symmetric sending chain key. When a member joins a group, all -existing members send their current sender key to the new member via pairwise -Double Ratchet channels. This gives forward secrecy per member, not per message. - -**Impact:** If implemented as described, group chat will either: -- Silently corrupt messages (if state is truly shared), or -- Require N*(N-1)/2 pairwise ratchet sessions (O(N^2) state, impractical for - groups > 10 members) - -**Recommendation:** Before Phase 7.5 (chat), decide between: -1. **Sender Keys** (Signal Groups approach): each member maintains one symmetric - sending chain. Forward secrecy at member rotation granularity. O(N) state. - Simpler to implement, good enough for most threat models. -2. **Pairwise Double Ratchet**: keep the current implementation but use it for - 1:1 messages only. Group messages would be encrypted N-1 times. O(N^2) cost - per message — only feasible for small groups (<20). -3. **MLS (Message Layer Security, RFC 9420)**: the modern standard for group - messaging. Tree-based ratcheting, O(log N) state and messages. More complex - but future-proof. Python implementations exist (`openmls` bindings, or - `mls-protocol`). - -Recommendation: **Sender Keys** for v1 (pragmatic, Signal-proven), with the -option to migrate to MLS later if group sizes grow. - -### C2 — No group membership verification in MNP handshake - -**Location:** `quic_server.py:125-146`, `server.py:130-151` - -**Problem:** The MNP handshake verifies the JWT signature and expiration, but does -NOT check whether the authenticated user is a member of the group being accessed. -Any valid JWT holder can request any file from any group served by the node. - -The draft says the JWT carries "hub-signed groups membership claim" (section 4.1.4), -but the actual `issue_access_token()` in `auth.py:104-125` does not include any -group membership claims. The JWT contains only `sub`, `pk_user`, `hub_id`, `jti`, -`iat`, `exp`. - -**Impact in Phase 7 (multi-group):** A user authenticated for group A can request -files from group B on the same node. Since all groups share one QUIC port, this -becomes an authorization bypass. - -**Recommendation:** -- Add group membership claims to the JWT: `"groups": ["group_id_1", "group_id_2"]` -- Node verifies the requested group_id is in the JWT's groups claim -- This is a simple change to `issue_access_token()` + handshake verification -- The JWT is already verified offline with the hub's Ed25519 key — adding claims - doesn't change the verification flow - ---- - -## Significant Issues - -### S1 — Admin revocation endpoint has no authorization check - -**Location:** `revocation.py:149-194` - -**Problem:** The `admin_revoke` endpoint requires authentication (`get_current_user`) -but does NOT verify that the current user is a hub admin. Any authenticated user -can revoke any other user or any group. The docstring says "Admin only (user must -be hub admin — user_id in config)" but no such check is implemented. - -**Impact:** Any registered user can revoke any other user or group on the hub. -This is a privilege escalation vulnerability. - -**Recommendation:** Phase 8 plans admin roles (8.1: `hub_admin` flag on User). This -check must be added before the revocation endpoint is used in any non-demo context. -For now, the endpoint exists but is only callable by someone who knows the API — -acceptable for a test deployment, not for production. - -### S2 — Email stored in plaintext in the database - -**Location:** `models.py:42`, draft v3 section 4.1.1 - -**Problem:** The spec says "Email and phone are stored encrypted at rest in the -database." The actual `User` model stores email as `String(256)` — plaintext. -A database breach would expose all user emails. - -**Recommendation:** Encrypt email (and future phone field) with a server-side key -derived from a secret not stored in the database (e.g., from the hub config file). -Use AES-256-GCM with a deterministic IV derived from user_id (for lookups) or -accept that encrypted email cannot be searched by value. - -### S3 — No jti denylist distribution to nodes - -**Location:** draft v3 section 4.1.4, open question #9 - -**Problem:** The architecture describes a jti denylist for immediate token revocation, -but: -- The hub has no `GET /v1/revoke/denylist` endpoint (marked [TBD]) -- Nodes don't check any denylist during JWT verification -- The revocation WebSocket pushes revocation tokens to nodes, but nodes don't - persist or check them during MNP handshake - -**Impact:** A revoked user's JWT remains valid for up to 1 hour (until natural -expiration). The revocation WebSocket can close active connections, but new -connections with the same JWT will succeed. - -**Recommendation:** Two options: -1. **Push + local cache** (recommended): when the node receives a revocation via - WebSocket, it adds the jti to an in-memory set. MNP handshake checks this set. - Simple, real-time, no polling. -2. **Pull**: node periodically fetches the denylist from the hub. Adds latency - between revocation and enforcement. - -Option 1 is simpler and already half-built (the WebSocket channel exists). - -### S4 — AES-GCM keystore uses non-standard 128-bit IV - -**Location:** `crypto.py:148` — `iv = os.urandom(16)` - -**Problem:** AES-GCM is specified for 96-bit (12-byte) nonces (NIST SP 800-38D). -The keystore encryption uses a 128-bit (16-byte) IV. The `cryptography` library -accepts this and processes it through GHASH to derive the internal counter, which -is secure — but it's a deviation from the standard. - -**Impact:** No direct vulnerability. AES-GCM with >96-bit IVs has a slightly -different security proof (birthday bound applies to the GHASH reduction). For a -keystore that's encrypted once and rarely re-encrypted, the practical risk is zero. - -**Recommendation:** Change to `os.urandom(12)` for standard compliance. Simple -one-line fix. The existing keystore files would need re-encryption on next save -(which happens naturally when the user updates their keystore). - -### S5 — Refresh token not rotated on use - -**Location:** `users.py:158-179` - -**Problem:** When a refresh token is used to obtain a new access token, the same -refresh token remains valid. If an attacker intercepts a refresh token, they can -use it repeatedly alongside the legitimate user, and neither party detects the -theft. - -**Recommendation:** Implement refresh token rotation: each use of a refresh token -issues a new refresh token and invalidates the old one. If the old token is used -again (by the attacker), the hub detects the reuse and revokes all tokens for -that user (indicating theft). This is the OAuth 2.0 Security BCP recommendation -(RFC 6819, section 5.2.2.3). - ---- - -## Minor Issues - -### M1 — Username enumeration via registration and pubkeys endpoints - -The registration endpoint returns "Username already taken" (409), and -`GET /v1/users/{username}/pubkeys` returns 404 vs a valid response. Both allow -enumerating valid usernames. For a decentralized platform where users have public -identities, this may be acceptable by design, but it should be a conscious -decision. - -### M2 — TLS self-signed certificate uses RSA-2048 - -**Location:** `tls_cert.py:36` - -The TLS cert uses RSA-2048 while the rest of the system uses Ed25519. Since the -cert is only for transport confidentiality (identity is verified via Ed25519), -this is acceptable. However, using an Ed25519 TLS certificate would be more -consistent and is supported by modern TLS 1.3 stacks. RSA-2048 is ~112-bit -security; Ed25519 is ~128-bit. - -### M3 — No rate limiting on GEK retrieval and pubkeys endpoints - -Only `/register` and `/login` have rate limiting. An attacker could enumerate -pubkeys or attempt to retrieve GEK bundles at high frequency. While GEK bundles -are opaque (no direct attack), rate limiting on all authenticated endpoints is -good hygiene. - -### M4 — Single admin per group with no delegation or recovery - -If the admin's node goes offline, the group becomes inaccessible: no new members -can be added, no GEK rotation, no moderation. There's no mechanism for admin -delegation or recovery. For a personal file-sharing platform this may be -acceptable, but for any group with more than a few members, this is a -single-point-of-failure. - -### M5 — Chunk key derivation uses HKDF salt=None - -**Location:** `crypto.py:46-51` - -The code uses `salt=None` and puts the file context in `info`. This is actually -correct HKDF usage (salt is for randomizing extraction when IKM might be -non-uniform; GEK is from CSPRNG so salt isn't needed; info is for domain -separation). However, the draft v3 spec describes it as using `salt`, which -creates a spec/code discrepancy. Update the spec to match the code, since the -code is correct. - -### M6 — Argon2id production parameters not yet applied - -**Location:** `crypto.py:131-133`, `auth.py:24-26`, `keyderive.py:33-35` - -All three Argon2id usage sites still use the dev parameters (iterations=3, -memory=64MB, ~78ms). Production target is iterations=4, memory=256MB, ~500ms. -Phase 7.7 plans a calibration CLI command. This must be done before any -real-world deployment. The comments document this correctly. - ---- - -## Notes (No Action Required) - -### N1 — Forward secrecy model is appropriate - -File encryption uses GEK-derived symmetric keys — no forward secrecy at the -application layer. If GEK is compromised, past files are decryptable. This is -documented and accepted: the alternative (per-session file encryption keys) -would break seeking, caching, and multi-source download. The transport layer -(TLS 1.3 / QUIC) provides forward secrecy for data in transit. - -### N2 — Error messages in login are correct - -`login()` returns the same "Invalid credentials" for both user-not-found and -wrong-password. This is the correct behavior to prevent user enumeration through -the login flow (even though registration and pubkeys endpoints allow it — see M1). - -### N3 — Hub legal exposure model is well-positioned - -The hub stores no content, no metadata, no node IPs (beyond ephemeral signaling). -GEK bundles are opaque. The hub's legal exposure is analogous to a domain -registrar or email provider — it knows who registered but not what they share. -LCEN/DSA compliance is addressed through IP logging with 1-year retention. - -### N4 — QUIC NAT probe content is fine - -`punch_nat()` sends `b'MESHBAY:NAT:PUNCH'` as a fixed probe. Some NAT -implementations might filter constant payloads, but in practice this works -(demo-v2 confirmed). The content of the probe packet doesn't matter for NAT -entry creation — only the 5-tuple (src_ip, src_port, dst_ip, dst_port, proto) -matters. - -### N5 — Web/CLI key derivation mismatch is by design - -Strategy A (Argon2id) and Strategy B (PBKDF2-SHA512 in browser) produce -different keys from the same password. The code and docs correctly explain this: -users pick one registration path. The web client uses random keypairs stored -encrypted on the hub, not password-derived keys. This avoids the mismatch -entirely. - ---- - -## Prioritized Action Plan - -| # | Issue | Severity | When to fix | -|---|---|---|---| -| C1 | Double Ratchet group model | Critical | Before Phase 7.5 (chat) | -| C2 | No group membership in JWT/handshake | Critical | Phase 7.3 (multi-group) | -| S1 | Admin revocation has no authz check | Significant | Phase 8.1 (admin roles) | -| S2 | Email stored in plaintext | Significant | Phase 8 | -| S3 | No jti denylist on nodes | Significant | Phase 7.2 (signaling) | -| S4 | AES-GCM 128-bit IV | Significant | Any time (1 line) | -| S5 | Refresh token rotation | Significant | Phase 8 | -| M1 | Username enumeration | Minor | Accept or Phase 8 | -| M2 | RSA-2048 TLS cert | Minor | Phase 7 or later | -| M3 | Rate limiting gaps | Minor | Phase 8.6 | -| M4 | Single admin SPOF | Minor | Phase 8+ | -| M5 | Spec/code HKDF discrepancy | Minor | Update spec | -| M6 | Argon2id prod params | Minor | Phase 7.7 | - ---- - -## Conclusion - -MeshBay's security architecture is built on solid foundations. The cryptographic -primitive choices are modern and correct. The trust model (hub-blind, node-hosted, -E2E encrypted) is well-designed and consistently applied. The POC spikes caught -real issues (jti, Argon2id calibration, NAT behavior) that would have been -difficult to fix post-deployment. - -The two critical issues (C1: group ratchet model, C2: group membership -authorization) are both design decisions that need to be made before Phase 7 -produces production chat and multi-group code. They are not retroactive problems -— they are forward-looking decisions that the architecture leaves room for. - -The significant issues (S1-S5) are implementation gaps that should be addressed -during Phases 7-8, in the natural course of hardening the hub and node. - -Overall assessment: **good foundations, ready for Phase 7** after deciding the -group chat encryption model (C1) and adding group claims to the JWT (C2). |