path: root/first-review.md

# MeshBay — First Architecture & Security Review

> Date: 2026-08-10
> Scope: design-level review of the cryptographic architecture, trust model, and
> security properties as specified in `docs/meshbay-draft-v3.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).