Skip to content

fix(sync): never regenerate the device identity on a failed certificate load (#135)#138

Merged
psimaker merged 2 commits into
mainfrom
fix/issue-135-identity-fail-closed
Jul 21, 2026
Merged

fix(sync): never regenerate the device identity on a failed certificate load (#135)#138
psimaker merged 2 commits into
mainfrom
fix/issue-135-identity-fail-closed

Conversation

@psimaker

@psimaker psimaker commented Jul 21, 2026

Copy link
Copy Markdown
Owner

The bridge used upstream LoadOrGenerateCertificate, which falls back to
generating a fresh key pair whenever loading cert.pem/key.pem fails. On
iOS a transient read failure (file protection before first unlock,
corruption after a hard crash) therefore silently minted a new device
ID — invalidating every peer pairing and surfacing server-side as an
endless stream of new pending devices. A #135 field report burned five
identities in one day during a crash loop.

What could go wrong and why this is safe: the new loadOrCreateIdentity
generates only when both identity files are verifiably absent (true
first launch — unchanged behavior there). Corrupt, partial, or
unreadable state now fails the engine start with an error instead of
regenerating; the existing files are never rewritten, so recovery after
a transient outage is automatic on the next start. The failure is
user-visible through the established SyncUserError path ("Could Not
Start Sync"), matching the manual-recovery doctrine (decision 002): a
visible failed start is recoverable, a silently replaced identity is
not. Stat errors are treated as unknown state, not as absence, so an
iOS file-protection outage cannot trigger generation either.

Regression tests (#135): identity stability across restarts, corrupt
certificate, partial identity (either file missing), and an unreadable
directory each fail closed without touching the surviving files, and
the original identity loads again once access returns.

Verified: cd go && make patch && go test -tags noassets ./bridge
-count=1 green (56s), gofmt clean. Requires an xcframework rebuild
before the next archive (macOS).

Decision record: docs/decisions/026-device-identity-fail-closed.md.

Summary

  • Improves VaultSync startup behavior to fail closed when loading the device TLS certificate/key: it does not regenerate or overwrite the device identity if either file is corrupt, partial, unreadable, or in an ambiguous state.
  • A new identity is generated only on a confirmed first launch when both certificate and key files are verifiably absent, preserving existing peer pairings and avoiding unintended “replacement device” identity changes.
  • If identity loading fails (including transient iOS file-access/protection failures), sync now stops startup via the existing “Could Not Start Sync” path rather than silently mutating identity.
  • Adds documentation/changelog coverage for the new fail-closed identity policy.
  • Test coverage: regression tests cover restart stability, corrupt certificate, partial identities, unreadable identity/permission failures, and recovery after access is restored.

Go bridge tests and formatting checks pass. An xcframework rebuild is required before the next macOS archive.

…te load (#135)

The bridge used upstream LoadOrGenerateCertificate, which falls back to
generating a fresh key pair whenever loading cert.pem/key.pem fails. On
iOS a transient read failure (file protection before first unlock,
corruption after a hard crash) therefore silently minted a new device
ID — invalidating every peer pairing and surfacing server-side as an
endless stream of new pending devices. A #135 field report burned five
identities in one day during a crash loop.

What could go wrong and why this is safe: the new loadOrCreateIdentity
generates only when both identity files are verifiably absent (true
first launch — unchanged behavior there). Corrupt, partial, or
unreadable state now fails the engine start with an error instead of
regenerating; the existing files are never rewritten, so recovery after
a transient outage is automatic on the next start. The failure is
user-visible through the established SyncUserError path ("Could Not
Start Sync"), matching the manual-recovery doctrine (decision 002): a
visible failed start is recoverable, a silently replaced identity is
not. Stat errors are treated as unknown state, not as absence, so an
iOS file-protection outage cannot trigger generation either.

Regression tests (#135): identity stability across restarts, corrupt
certificate, partial identity (either file missing), and an unreadable
directory each fail closed without touching the surviving files, and
the original identity loads again once access returns.

Verified: cd go && make patch && go test -tags noassets ./bridge
-count=1 green (56s), gofmt clean. Requires an xcframework rebuild
before the next archive (macOS).

Decision record: docs/decisions/026-device-identity-fail-closed.md.
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f37429d-2780-42fe-8910-bb495a9ceaa0

📥 Commits

Reviewing files that changed from the base of the PR and between 0d26254 and 570d7d7.

📒 Files selected for processing (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
📜 Recent review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: Notify Tests
  • GitHub Check: M5/M6 Syncthing Transfer E2E
  • GitHub Check: Go Tests
  • GitHub Check: Helper Runtime Packaging

📝 Walkthrough

Walkthrough

The bridge now preserves device identity across restarts, generates certificates only on confirmed first launch, and fails startup instead of regenerating identities when stored certificate or key files are corrupt, partial, or unreadable.

Changes

Device identity continuity

Layer / File(s) Summary
Fail-closed identity loader
go/bridge/identity.go
Adds strict certificate/key presence checks, existing-pair loading, partial/corrupt/unreadable state errors, and first-launch generation.
Startup identity integration
go/bridge/syncthing.go
Routes Syncthing startup through the new loader instead of regeneration-on-error behavior.
Regression coverage and recorded policy
go/bridge/identity_test.go, docs/decisions/026-device-identity-fail-closed.md, CHANGELOG.md
Tests identity stability and failure cases, and records the fail-closed policy and fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

A cautious key stays in its place,
No crash can make a stranger’s face.
Missing halves raise a clear alarm,
First launch alone may forge the charm.
Pairings rest, identities calm.

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise, conventional-commit style, and accurately describes the device-identity fail-closed change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Private Note Leakage ✅ Passed Logs are bounded/sanitized or discarded, and diagnostics traffic uses random 256-byte payloads plus opaque bindings—not note contents, vault paths, or keys.
Bounded Ios Background Work ✅ Passed PASS: The only Swift change is a read-only diagnostics preflight API; no background-task code, expiration callbacks, or cleanup logic changed.
Bridge Contract Compatibility ✅ Passed No exported bridge signatures or JSON shapes changed; StartSyncthing still returns empty-string success and Swift still maps it to nil, with added regression tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-135-identity-fail-closed

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@psimaker
psimaker merged commit f38d4e9 into main Jul 21, 2026
20 checks passed
@psimaker
psimaker deleted the fix/issue-135-identity-fail-closed branch July 21, 2026 08:25
psimaker added a commit that referenced this pull request Jul 22, 2026
* chore(release): prepare VaultSync 2.0.1 build 37

Version bump only: app + widget to 2.0.1/37 in ios/project.yml, and the
CHANGELOG [Unreleased] section renamed to [2.0.1] — 2026-07-22. The only
shipped change since 2.0.0 is the device-identity fail-closed fix (#135,
merged as #138), which lives in go/bridge and therefore requires an
xcframework rebuild before the archive — rebuilt and verified during this
prep.

What could go wrong and why this is safe: a skewed widget version or a
missing CHANGELOG section would fail version-check.yml on the v2.0.1 tag;
both were verified locally with the workflow's own checks (2x 2.0.1,
single build number 37, section present). Full verification on this tree:
Go bridge suite green (real Syncthing instances), gofmt clean, xcframework
rebuilt from the patched tree, full Xcode plan 431/431 passed with zero
failures/skips (xcresulttool-verified), strings key parity (884 keys) and
design-token lint green. No StoreKit, entitlement, bundle-ID, or signing
values were touched.

* fix(deps): resolve GO-2026-5970 by bumping golang.org/x/text to 0.39.0

The merge-blocking govulncheck gate on PR #139 flagged GO-2026-5970
(infinite loop on invalid input in x/text's Unicode normalization, fixed
in 0.39.0): the bridge reaches norm.Form via StartSyncthing ->
suture.Supervisor.ServeBackground and syncthing.EnsureDir. x/sys 0.46.0
and x/tools 0.47.0 move along as transitive requirements of the bump.

What could go wrong and why this is safe: x/text is an indirect,
API-stable dependency of the embedded Syncthing; no code changes were
needed. Verified with the pinned scanner and the full suites on this
tree: govulncheck v1.6.0 reports 0 called vulnerabilities, Go bridge
suite green (real Syncthing instances), gofmt clean, xcframework
rebuilt, and the full Xcode plan re-run against that framework: 431/431
passed, 0 failed, 0 skipped (xcresulttool-verified). The CHANGELOG 2.0.1
section gains the Security entry; no StoreKit, entitlement, bundle-ID,
or signing values were touched.
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