Skip to content

feat: backup brute-force detection via key-server attempts - #2560

Open
ethicnology wants to merge 2 commits into
developfrom
feat/bruteforce-telemetry
Open

feat: backup brute-force detection via key-server attempts#2560
ethicnology wants to merge 2 commits into
developfrom
feat/bruteforce-telemetry

Conversation

@ethicnology

Copy link
Copy Markdown
Member

A Bull user whose Backup File leaks has no idea their backup is being probed:
an attacker gets 3 password guesses per cooldown window, or can grief the
victim by keeping their identifier permanently rate-limited — and the user
finds out only when recovery fails. The key server now publishes advisory
telemetry (/attempts snapshot, attempt_status on successful fetch/trash,
wipe metadata on /info). This PR integrates it into Bull as warnings
only
, per the protocol's trust model: the server cannot distinguish an
attacker from the user or another of the user's devices, and a compromised
server can fabricate or suppress counters.

Depends on: recoverbull-client-dart telemetry PR and the deployed
recoverbull-server. The recoverbull dependency currently uses a local
path: (dev convenience); it is pinned to the reviewed merge SHA in a
follow-up chore commit once the client PR merges.

Commits

Commit 1 — feat: brute-force telemetry core and check orchestration

The whole core layer, self-contained (compiles and tests pass standalone).

  • Domain mirrors of the SDK telemetry types (KeyServerAttemptStatus,
    VaultKeyFetchResult, TelemetrySnapshotResult, KeyServerInfo) and a
    sealed RecoverbullTelemetryAlert entity — the core layer stays free of
    SDK types. New failures for the client's dedicated 429/503 subtypes, mapped
    in the repository: targeted lockout (alarm) vs global overload and capacity
    vs busy 503 (service pressure, never an attack).
  • Persistence (drift 14 → 15 → 16), scoped per key-server URL and
    invalidated when the URL changes: recoverbull_telemetry_server (ETag,
    last check, collection_started_at, consecutive failures) and
    recoverbull_telemetry_backup (this device's own operation counters with
    window tracking, warning dedup, acknowledgement). Only
    sha256(raw backup id) is stored, never the raw identifier — the baseline
    still reveals which backups are monitored, so it lives in the same
    protected store as other sensitive app state.
  • is_recoverbull_telemetry_enabled settings flag (default false),
    accessed through targeted datasource accessors — deliberately not part of
    SettingsModel/SettingsEntity to avoid rippling into every settings
    constructor call site.
  • Orchestration: CheckBackupTelemetryUsecase (cold-launch conditional
    poll with staleness skip, per-backup window-rollover-aware reconciliation,
    wipe detection that resets the baseline without an attack alarm,
    prolonged-unavailability soft warning, service-pressure mapping),
    RecordLocalAttemptUsecase (counts this device's operations, surfaces
    immediate suspicion from attempt_status), AcknowledgeTelemetryAlertUsecase,
    and the app-scoped RecoverbullTelemetryCubit holding the ephemeral alerts
    with persisted dedup. Changing the key-server URL invalidates the old
    server's baseline; ResetAppDataUsecase wipes it.

Commit 2 — feat: wire telemetry checks into startup and backup flows, warnings UI

  • Cold launch: AppStartupBloc fires checkOnColdLaunch() unawaited
    after Tor init — never blocking startup, a no-op when the flag is off, Tor
    is not ready, or the last check is still fresh. No background polling.
  • Backup flows: the recoverbull feature records this device's own
    operations (store, and fetch via the new status-aware usecase — the
    freshest signal, available even when /attempts is overloaded) and reports
    the targeted per-identifier lockout as an alarm. Global 429 / capacity 503
    map to unavailability, never to a lockout alarm.
  • Surfacing (all advisory, all dismissible): a banner on the wallet home
    and the backup settings screen; strong warnings also open a bottom sheet
    once per session. Acknowledgement is remembered so multi-device false
    positives do not train the user to ignore alerts.

Warning rules:

Signal Surfaced as
Snapshot entry or attempt_status shows attempts the user did not make Strong warning: unknown activity on this backup
Unexpected per-identifier 429 on the user's own fetch Strong warning (someone may be probing or griefing this backup)
Global lookup 429, capacity 503, nearly-full snapshot (vs max_attempt_identifiers from /info) Service pressure notice, not an attack
/attempts unreachable for several days Soft warning: telemetry unavailable (flooding the route is the realistic way to suppress it during an attack)
Counter regression or disappeared entry Nothing: normal cooldown expiry
Changed collection_started_at Neutral notice: counters wiped

Messaging rules (from the protocol threat model — the alarm itself is an
attack vector for phishing and panic):

  • Always say "unknown/suspicious activity", never "confirmed attack".
  • Always pair the warning with "check whether it was you or another of your
    devices first" — another device is the expected false positive.
  • Never ask for the seed or password because of this alert, and say so
    in the copy.
  • When unexplained activity is confirmed and the user still controls the
    wallet, recommend rotating keys / moving funds — as a user-initiated,
    guided action, never automatic.
  • English and French localization included.

Test plan

  • Cold launch checks only when stale and after Tor is ready; unchanged ETag
    causes no snapshot processing.
  • Known local attempts do not warn; unknown total attempts warn;
    planted-row attempts are caught through the total counter.
  • Targeted 429 and global 429 produce different surfaces; capacity 503
    maps to service pressure.
  • Telemetry network failures never block startup or recovery.
  • Changed collection_started_at resets the baseline without an attack
    alarm; regressed counters and disappeared entries stay silent.
  • Prolonged unavailability surfaces the soft warning; a nearly-full map
    surfaces service pressure.
  • Another-device activity is presented as uncertain, not malicious fact.
  • Changing the key-server URL invalidates ETag, baseline and
    collection_started_at; ResetAppDataUsecase wipes both tables.
  • Migration 14 → 15 → 16 upgrades cleanly (drift schema tests).
  • 960 tests pass; flutter analyze clean.

Deployment note

Server deployment and the client release must precede enabling this feature.
The feature flag (is_recoverbull_telemetry_enabled, default false) stays
off until the pinned client commit and the deployed server contract are
confirmed in production.

@ethicnology ethicnology self-assigned this Aug 6, 2026
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 51s —— View job


Code review — backup brute-force detection via key-server telemetry

  • Read repo guidelines (AGENTS.md) and PR diff
  • Review telemetry core (entities, repository, datasources)
  • Review orchestration usecases and cubit
  • Review persistence/migrations (drift 14→15→16)
  • Review startup/backup-flow wiring and UI surfacing
  • Post findings

Note: I couldn't post inline review comments (the runner denies gh api write calls beyond updating this comment), so findings are listed here with permalinks. I also couldn't run make deps/analyze/tests — finding 1 makes pub get impossible in this environment.

Overall this is a carefully built feature: the trust-model discipline in the copy, the targeted-vs-global 429 separation, the wipe detection, and the migration idempotency guards are all well done. The findings below are ordered by severity; the top three are correctness/security issues worth resolving before merge.


🔴 1. Local path: dependency breaks every build outside your machine — merge blocker

pubspec.yaml:95recoverbull: path: ../recoverbull-client-dart fails fvm flutter pub get --enforce-lockfile (CI, make deps, reproducible builds) for anyone without the sibling checkout, and the lockfile no longer pins a reviewable SHA, so the client's telemetry semantics can drift under this PR unreviewed. The PR body acknowledges this — flagging so it doesn't merge before the re-pin chore commit lands.

🔴 2. Window timestamps from attempt_status vs /attempts may never match → systematic false "unknown activity" alert after every recovery

record_local_attempt_usecase.dart:44-49 stores currentWindowStartedAt from attempt_status (exact-second epoch), while check_backup_telemetry_usecase.dart:204-211 compares it for equality against the snapshot entry's window — which your own entity doc says is hour-truncated (key_server_telemetry.dart:44). If the two endpoints don't serve bit-identical window timestamps, every reconcile after a user recovery sees a "window mismatch", resets expected to 0, and raises a SuspiciousActivityAlert for the user's own fetch (observed 1, expected 0) on the next cold launch. The unit tests can't catch this because they feed the same DateTime to both sides. Fix: truncate both sides identically before comparing (or compare with tolerance ≥ the server's truncation), and add a test where attempt_status and the snapshot disagree in sub-hour precision. This can't be verified against the client because of finding 1.

🔴 3. Counting store as a local attempt can mask exactly one attacker guess per window

bloc.dart store flow records the store operation with no attemptStatus, so record_local_attempt_usecase.dart:39 blindly does expected + 1. If the server does not count store operations in the /attempts map (rate limiting normally applies to fetch/trash guesses), then after a fetch has pinned the window, a subsequent store inflates expected by one — and one real attacker probe in that window becomes total == expected, i.e. silent. If the server does count stores, then the null-window store path has the finding-2 mismatch problem instead. Please verify the server contract and either stop counting stores or count them with their real window.

🟠 4. An attacker flooding /attempts suppresses telemetry forever without ever escalating

The PR body itself says "flooding the route is the realistic way to suppress it during an attack" — but in check_backup_telemetry_usecase.dart:107-130, KeyServerOverloadedFailure/KeyServerCapacityFailure return early: they never increment consecutiveFailures and never reach the unavailabilityThreshold check. Sustained global 429 for weeks surfaces only the mild "service issue, not an attack" notice, and the soft "you are not being warned of suspicious activity" alert never fires — exactly the outage mode the threat model calls realistic. Suggest: overload/capacity should still count toward prolonged unavailability (they can keep the softer immediate copy).

🟠 5. After wipe detection, reconciliation runs on the stale pre-reset rows

check_backup_telemetry_usecase.dart:86-95 upserts every baseline to expected: 0, window: null, but the in-memory backups list passed to _reconcileBackups (line 151) still carries the old counters/windows. If an old window happens to equal a post-wipe entry window, the stale expected is compared (and re-persisted via _rebuild), undoing the reset. Rebuild the list (or re-fetch) after the reset loop.

🟠 6. First-ever failed check claims "unreachable for 3 days"

check_backup_telemetry_usecase.dart:119-121: when lastSuccessfulCheckAt is null (feature just enabled, Tor still flaky on first launch), silentFor is set to the threshold so a single transient failure immediately shows the soft warning — and recoverbullTelemetryUnavailable renders "unreachable for 3 days", which is false. Either require N consecutive failures before the never-succeeded warning, or use dedicated first-check copy without a fabricated day count.

🟠 7. Service-pressure / unavailability / wipe notices are not dismissible, and dedup collapses distinct pressure kinds

  • The PR claims "all advisory, all dismissible", but telemetry_warnings.dart:68-70 gives non-strong cards onTap: null, and acknowledge filters by backupIdHash, which is null for these alerts — so they sit on the wallet home for the whole app session with no way to clear them.
  • _mergeAlerts dedups by runtimeType + hash, so ServicePressureAlert(global429) blocks a later ServicePressureAlert(capacity503) (the kind is ignored).

🟡 8. acknowledgedAt is written but never read — the "acknowledgement is remembered" claim doesn't hold

acknowledge_telemetry_alert_usecase.dart:32 persists the timestamp, but nothing consults it: cold-launch dedup uses lastWarningWindowStartedAt only, and the recordLocalAttempt path has no dedup at all — after the user acknowledges, the next recovery in the same window re-raises the identical alert. Either gate alert creation on acknowledgedAt (per window) or drop the column.

🟡 9. The feature flag has no writer — it can never be enabled in-app

setIsRecoverbullTelemetryEnabled has zero callers (grep confirms only the datasource/repository/interface definitions). Even QA can't turn the feature on without editing the DB. Fine if a toggle ships in the follow-up, but worth stating — right now all UI/flows in this PR are unreachable dead paths in production and in internal testing.

🟡 10. Smaller items

  • store_recoverbull_url_usecase.dart:16-19 — the catch (_) says "no previous URL stored yet" but also swallows a failed deleteTelemetryForServer; if the user later switches back to that URL, the stale ETag/baseline resurrects. Catch only the "no URL" case, or delete after storing succeeds.
  • backup_settings_screen.dart:68-73 — the edited children: block is indented one extra space; make format-check should fail on this. (I couldn't run it locally due to finding 1.)
  • trashVaultKeyWithStatus / trashWithStatus have no callers — dead code; and if trash is counted server-side, a user's own trash will read as unknown activity on the next check.
  • wallet_home_screen.dart listener — alerts merged before the home screen mounts never trigger the once-per-session bottom sheet (BlocListener only reacts to emissions after subscription). The banner still shows, so low impact, but the "strong warnings open a bottom sheet" behavior is timing-dependent.

🏛️ Architecture notes (per AGENTS.md — flag, don't force)

  • Drift rows cross the repository boundary (rule 6, "never bends"): RecoverbullTelemetryServerRow/RecoverbullTelemetryBackupRow are used directly in the domain usecases (check_backup_telemetry_usecase.dart:5 imports sqlite_database.dart). A small domain entity + mapper in the repository would restore the boundary.
  • SDK leaks past the boundary the PR itself defines: recoverbull.attemptsIdHashFromHex is called from the presentation cubit (recoverbull_telemetry_cubit.dart:80) and the domain usecase, despite the "core stays free of SDK types" goal. Hashing the identifier belongs behind the repository.

✅ What looks good

Migrations 14→15→16 are clean (guarded creates, DB-default backfill, schema tests present); only sha256(id) is persisted; failure mapping keeps targeted-lockout vs global-overload vs capacity distinct end-to-end; startup wiring is genuinely non-blocking (unawaited, fail-to-silence everywhere); the warning copy follows the threat model (never "attack", always "was it you?", never ask for seed/password) in both EN and FR.

--- · Branch: feat/bruteforce-telemetry

@ethicnology ethicnology changed the title feat: backup brute-force detection via key-server telemetry feat: backup brute-force detection via key-server attempts Aug 6, 2026
@ethicnology
ethicnology force-pushed the feat/bruteforce-telemetry branch 3 times, most recently from c7cd2be to a92534e Compare August 6, 2026 12:04
Adds the core of the key-server brute-force telemetry integration
(recoverbull-client-dart telemetry PR), keeping telemetry strictly
advisory: the server cannot distinguish an attacker from the user or
another of the user's devices, and a compromised server can fabricate
or suppress counters. Everything warns, never acts automatically.

Domain and data:
- Domain mirrors of the SDK telemetry types (KeyServerAttemptStatus,
  VaultKeyFetchResult, TelemetrySnapshotResult, KeyServerInfo) and a
  sealed RecoverbullTelemetryAlert entity (SuspiciousActivity,
  TargetedLockout, ServicePressure, TelemetryUnavailable,
  CountersWiped). The core layer stays free of SDK types.
- New failures for the client's dedicated 429/503 subtypes, mapped in
  the repository: targeted lockout (alarm) vs global overload and
  capacity vs busy 503 (service pressure, never an attack).
- Repository: status-aware fetch/trash, conditional /attempts, server
  info, and the telemetry baseline persistence.

Persistence (single drift migration, 14 -> 15):
- recoverbull_telemetry_server (per-URL polling state: ETag, last
  check, collection_started_at, consecutive failures) and
  recoverbull_telemetry_backup (per-backup own-operation counters with
  window tracking, warning dedup and acknowledgement). Only
  sha256(raw backup id) is stored, never the raw identifier.
- is_recoverbull_telemetry_enabled settings flag (default false),
  accessed through targeted datasource accessors — deliberately not
  part of SettingsModel/SettingsEntity to avoid rippling into every
  settings constructor call site.

Orchestration:
- CheckBackupTelemetryUsecase: cold-launch conditional poll with
  staleness skip, per-backup reconciliation (server total vs this
  device's own count, window-rollover aware), wipe detection via
  collection_started_at (resets the baseline, never an attack alarm),
  prolonged-unavailability soft warning, and service-pressure mapping.
- RecordLocalAttemptUsecase: counts this device's own operations and
  surfaces immediate suspicion from attempt_status.
- AcknowledgeTelemetryAlertUsecase and the app-scoped
  RecoverbullTelemetryCubit holding the (ephemeral) alerts with
  persisted dedup.
- Changing the key-server URL invalidates the old server's baseline;
  ResetAppDataUsecase wipes it.

Usecase, repository and migration tests included.
Wires the telemetry orchestration into the app and surfaces the alerts.

Checks:
- Cold launch: AppStartupBloc fires RecoverbullTelemetryCubit
  .checkOnColdLaunch() unawaited after Tor init — never blocking
  startup, a no-op when the flag is off, Tor is not ready, or the last
  check is still fresh. No background polling.
- Backup flows: the recoverbull feature records this device's own
  operations (store, and fetch via the new status-aware usecase) and
  reports the targeted per-identifier lockout as an alarm signal.
  Global 429 / capacity 503 map to unavailability, never to a lockout
  alarm.

Surfacing (all advisory, all dismissible):
- RecoverbullTelemetryWarnings banner on the wallet home and the backup
  settings screen; strong warnings (suspicious activity, targeted
  lockout) also open a bottom sheet once per session. Acknowledgement
  is remembered so multi-device false positives do not train the user
  to ignore alerts.
- Copy follows the trust model: 'unknown/suspicious activity', never
  'confirmed attack', always paired with 'check whether it was you or
  another of your devices' and 'never enter your seed words or password
  because of this alert'. English and French localization.

The feature stays behind is_recoverbull_telemetry_enabled (default
false) until the server is deployed and the client pinned by SHA.

Bloc test updated for the new dependencies.
@ethicnology
ethicnology force-pushed the feat/bruteforce-telemetry branch from a92534e to 290d331 Compare August 6, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant