feat(fleet): per-worker keys in the desktop fleet server#91
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFleet heartbeats now use per-device keys issued through bootstrap enrollment. Key state is stored and migrated in SQLite, requests are rate-limited, and fleet state mutations are serialized across heartbeat, deletion, and stale-device cleanup paths. ChangesFleet key security
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 74.70% 74.92% +0.22%
==========================================
Files 14 14
Lines 3261 3418 +157
==========================================
+ Hits 2436 2561 +125
- Misses 651 669 +18
- Partials 174 188 +14
🚀 New features to boost your workflow:
|
Makes CashPilot-Desktop's fleet_server compatible with the per-worker-key protocol (web UI v1.0.0 + the CashPilot-android client). The heartbeat API validated a single shared token for every device; it now: - issues each device its own key on first contact (returned once as worker_key), - requires that key thereafter and CONFIRMS it on first own-key use, - rejects the shared token for a confirmed device (impersonation protection), - re-issues a fresh key each heartbeat until the device confirms, so a dropped response can't lock a device out. Store: per-device api_key_hash + key_confirmed (forward-only guarded migration); SetFleetDeviceKey / ConfirmFleetDeviceKey / FleetDeviceKeyState + HashFleetKey. Server: classifyFleetAuth (pure, table-tested) drives handleWorkerHeartbeat. Verified: go build/vet clean; go test -race ./... green.
GeiserX
force-pushed
the
feat/fleet-per-worker-keys
branch
from
32990d5 to
b2faa63
Compare
From the architect review (all 7 security checks passed): - SetFleetDeviceKey/ConfirmFleetDeviceKey now error on a 0-row UPDATE, so a device is never handed a worker_key whose hash wasn't actually stored (closes a narrow issued-but-not-stored -> lockout race). Test added. - Document that shared-token enrollment is trust-on-first-use (keep the token secret + the fleet API on the loopback default), and note the (kind,name) identity is derived from mutable client fields. go build/vet clean; go test -race ./... green.
…findings From the 7-lens PR review: - Serialize the read->classify->upsert->set/confirm sequence (new App.fleetMu, also held by the reap + manual-delete paths): closes the concurrent/retried-heartbeat and reap-race TOCTOU that could hand a client a key the DB discarded or blank a confirmed device's key. - Per-IP rate limiter on the heartbeat/enrollment endpoint (anti-flood/anti-abuse). - Constant-time shared-token check now compares fixed-length hashes (no token-length timing leak). - columnExists guards the interpolated table identifier (future-proofing). - Fix stale app.go comment referencing the removed validFleetBearer. - Tests: ALTER-backfill migration path (the real upgrade path), reissue-until- confirmed through the HTTP handler, concurrent-enroll under -race, rate limiter, identifier guard. Document TOFU/transport residuals (loopback default; TLS proxy for LAN; full TLS a follow-up). go build/vet clean; go test -race ./... green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app.go (1)
807-810: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueLock scope is correct; consider panic safety.
The explicit
Lock()/Unlock()(vsdefer) is the right call here — it releases the lock beforeGetFleetState()and error handling, minimizing contention with heartbeats. The heartbeat handler correctly usesdeferinstead since it has multiple return points.One minor trade-off: if
DeleteFleetDeviceorSweepStaleFleetDevicespanicked, the lock would remain held. In practice this is negligible (both are simpledb.Execcalls that return errors, and the store is nil-guarded upstream). If you ever want belt-and-suspenders, a small closure keeps both properties:🔒 Optional: panic-safe scoped lock
func (a *App) RemoveFleetDevice(id int64) (FleetState, error) { if err := a.ready(); err != nil { return FleetState{}, err } if id <= 0 { return FleetState{}, fmt.Errorf("the local desktop device cannot be removed") } - a.fleetMu.Lock() - err := a.store.DeleteFleetDevice(id) - a.fleetMu.Unlock() + var err error + func() { + a.fleetMu.Lock() + defer a.fleetMu.Unlock() + err = a.store.DeleteFleetDevice(id) + }() if err != nil { return FleetState{}, err } return a.GetFleetState() }Also applies to: 1115-1118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.go` around lines 807 - 810, Make the lock scope panic-safe in the fleet deletion paths by wrapping each Lock/operation/Unlock sequence around DeleteFleetDevice and SweepStaleFleetDevices in a small closure, while ensuring the lock is released before GetFleetState and subsequent error handling.fleet_server.go (1)
188-194: 🚀 Performance & Scalability | 🔵 Trivial
fleetMuserializes every heartbeat globally, not per device. Fine for a small loopback fleet, but as device count grows this single lock (held acrossFleetDeviceKeyState→UpsertFleetHeartbeat→ set/confirm) becomes a throughput chokepoint. If you later scale up, the cleaner fix is to run the read→classify→write as oneBEGIN IMMEDIATESQLite transaction (atomicity without a process-wide mutex) or shard the lock by(kind, name).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fleet_server.go` around lines 188 - 194, Replace the process-wide fleetMu serialization around the heartbeat read→classify→upsert→set/confirm sequence with per-device locking keyed by device kind and name, or an equivalent atomic BEGIN IMMEDIATE SQLite transaction. Preserve coordination with reap/delete paths while allowing heartbeats for different devices to proceed concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app.go`:
- Around line 807-810: Make the lock scope panic-safe in the fleet deletion
paths by wrapping each Lock/operation/Unlock sequence around DeleteFleetDevice
and SweepStaleFleetDevices in a small closure, while ensuring the lock is
released before GetFleetState and subsequent error handling.
In `@fleet_server.go`:
- Around line 188-194: Replace the process-wide fleetMu serialization around the
heartbeat read→classify→upsert→set/confirm sequence with per-device locking
keyed by device kind and name, or an equivalent atomic BEGIN IMMEDIATE SQLite
transaction. Preserve coordination with reap/delete paths while allowing
heartbeats for different devices to proceed concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2ec12f0-ef52-47c9-bb09-3e00f3d1a4bd
📒 Files selected for processing (7)
CHANGELOG.mdCLAUDE.mdapp.gofleet_server.gofleet_server_test.gointernal/store/store.gointernal/store/store_test.go
Close the codecov/patch gaps on the review-hardening branches: the handler 429 path, the rate-limiter's expired-IP sweep, and columnExists rejecting an unsafe table identifier. go test -race green.
1 participant
Makes CashPilot-Desktop's
fleet_servercompatible with the per-worker-key protocol shipped in the CashPilot web UI (v1.0.0) and the CashPilot-android client — so any client (mobile, docker worker) interoperates with the desktop fleet server the same way it does with the web UI.Before:
/api/workers/heartbeatvalidated a single shared token for every device.Now:
worker_key.401(impersonation protection; a leaked device key is scoped to one device).Store (
internal/store): per-deviceapi_key_hash+key_confirmedwith a forward-only, guarded SQLite migration;SetFleetDeviceKey/ConfirmFleetDeviceKey/FleetDeviceKeyState/HashFleetKey. A one-way hash is sufficient — the desktop fleet server only verifies inbound heartbeats and issues keys (it never calls out to devices), and it rotates the key on each unconfirmed reissue rather than re-delivering.Server (
fleet_server.go): the pure, table-testedclassifyFleetAuthdriveshandleWorkerHeartbeat.Verified:
go build ./...+go vet ./...clean;go test -race ./...green — including the store key lifecycle, theclassifyFleetAuthmatrix,bearerToken, and a full enroll→confirm→reject HTTP handler flow. gofmt clean.Companion: CashPilot-android PR #37 (client side). Beads: CashPilot-Desktop-jet/-xkf/-10u/-rxx (closed).
Summary by CodeRabbit