feat(devices): self-healing WCK rotation after revoke (#134)#137
Merged
Conversation
A failed revoke-path WCK rotation left the old epoch active with only a one-shot warning; events stayed readable by the revoked device until a manual keys rotate. Persist the owed rotation in a wck_rotation_pending local_meta marker; sync's rotation gate retries it every cycle (even with keys.rotate_max_age=0), clearing it only on this device's own successful Rotate — never on merely observing a newer epoch (dual-review HIGH: an uninformed peer's age rotation can grant the new epoch to the revoked device). An early retry failure warns and lets the cycle continue so the device.revoked event still propagates; a mid-commit failure (epoch advanced) stays fatal. devices revoke preflights the remaining approved recipients before the trust write; doctor warns while the rotation is owed. Fixes #134 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds recipient preflight validation and a persistent "owed WCK rotation" marker mechanism triggered by failed workspace key rotations during device revoke/lost. Sync retries owed rotations, doctor reports pending rotations, and keys rotate clears the marker. Includes new store method, tests, and spec updates. ChangesSelf-healing WCK Rotation After Revoke
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant DevicesCmd
participant Keyring
participant LocalMeta
participant Sync
participant Doctor
Operator->>DevicesCmd: devstrap devices revoke/lost
DevicesCmd->>DevicesCmd: warnMalformedRemainingRecipients (preflight)
DevicesCmd->>DevicesCmd: flip trust state
DevicesCmd->>Keyring: Rotate()
alt rotation fails
Keyring-->>DevicesCmd: error
DevicesCmd->>LocalMeta: markWCKRotationPending(epoch, since)
DevicesCmd-->>Operator: warn, revoked device may still read events
else rotation succeeds
Keyring-->>DevicesCmd: ok
DevicesCmd->>LocalMeta: clearWCKRotationPending()
end
Sync->>LocalMeta: wckRotationPendingSince()
LocalMeta-->>Sync: pending, since
Sync->>Keyring: Rotate() (owed retry)
alt success
Sync->>LocalMeta: clearWCKRotationPending()
else early failure
Sync-->>Sync: warn, continue cycle
else epoch-advanced failure
Sync-->>Sync: fatal cycle error
end
Doctor->>LocalMeta: wckRotationPendingSince()
LocalMeta-->>Doctor: owed since ts
Doctor-->>Operator: warning + remedy (sync or keys rotate)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/devices.go (1)
807-843: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPersist the owed-rotation marker when
CurrentEpochfails. If this read errors, the revoke still leaves the old key active, butsync/doctornever see an owed rotation, so the retry/surfacing path is lost. A best-effort pending row here would keep the revoke self-healing.🤖 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 `@internal/cli/devices.go` around lines 807 - 843, The CurrentEpoch read failure path in rotateWorkspaceKeyOnRevoke currently only logs and returns, which drops the owed-rotation state. Update rotateWorkspaceKeyOnRevoke to best-effort persist the pending rotation marker when kr.CurrentEpoch(ctx) returns an error, using the same store-backed helper used after Rotate failures so sync/doctor can retry and surface it. Keep the existing warning log, but add the pending-rotation write before returning, and ensure the epoch value is only used when the read succeeds.
🤖 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.
Inline comments:
In `@internal/cli/sync.go`:
- Around line 356-358: The key-epoch check in sync handling is conflating two
different failures and discarding the `CurrentKeyEpoch` read error. Update the
logic around `store.CurrentKeyEpoch(ctx)` to report a distinct message when
`aerr != nil`, wrapping `aerr` directly, and only use the “epoch advanced”
wording when `after != epoch`. Keep the fail-closed behavior, but make the error
path in `sync.go` clearly distinguish the DB read failure from the mid-commit
epoch change so operators debugging the revoke flow see the right cause.
---
Outside diff comments:
In `@internal/cli/devices.go`:
- Around line 807-843: The CurrentEpoch read failure path in
rotateWorkspaceKeyOnRevoke currently only logs and returns, which drops the
owed-rotation state. Update rotateWorkspaceKeyOnRevoke to best-effort persist
the pending rotation marker when kr.CurrentEpoch(ctx) returns an error, using
the same store-backed helper used after Rotate failures so sync/doctor can retry
and surface it. Keep the existing warning log, but add the pending-rotation
write before returning, and ensure the epoch value is only used when the read
succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 490bb316-376f-480b-b70e-3f5b76c01ae3
📒 Files selected for processing (12)
internal/cli/devices.gointernal/cli/doctor.gointernal/cli/keys.gointernal/cli/sync.gointernal/cli/wck_rotation.gointernal/cli/wck_rotation_test.gointernal/state/store.gointernal/workspacekeys/keyring.gospec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/13_CLI_DAEMON_API.mdspec/15_SECURITY_THREAT_MODEL.mdspec/18_WORK_LOG.md
Comment on lines
+356
to
+358
| if after, aerr := store.CurrentKeyEpoch(ctx); aerr != nil || after != epoch { | ||
| return false, fmt.Errorf("workspace key rotation failed mid-commit (epoch advanced %d→%d; grants may not have published): %w", epoch, after, rerr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Misleading error and dropped read error when CurrentKeyEpoch fails.
When aerr != nil, after is its zero value, so the message reads e.g. "epoch advanced 5→0; grants may not have published" — a mid-commit claim that didn't actually happen — and the real DB read error (aerr) is discarded (only rerr is wrapped). Failing closed here is correct, but the two cases should be reported distinctly so an operator debugging a revoke-containment failure isn't sent down the wrong path.
🩹 Distinguish the read-failure case
- if after, aerr := store.CurrentKeyEpoch(ctx); aerr != nil || after != epoch {
- return false, fmt.Errorf("workspace key rotation failed mid-commit (epoch advanced %d→%d; grants may not have published): %w", epoch, after, rerr)
- }
+ after, aerr := store.CurrentKeyEpoch(ctx)
+ if aerr != nil {
+ return false, fmt.Errorf("workspace key rotation failed and re-reading the epoch failed, so a mid-commit half-mint cannot be ruled out (rotate error: %v): %w", rerr, aerr)
+ }
+ if after != epoch {
+ return false, fmt.Errorf("workspace key rotation failed mid-commit (epoch advanced %d→%d; grants may not have published): %w", epoch, after, rerr)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if after, aerr := store.CurrentKeyEpoch(ctx); aerr != nil || after != epoch { | |
| return false, fmt.Errorf("workspace key rotation failed mid-commit (epoch advanced %d→%d; grants may not have published): %w", epoch, after, rerr) | |
| } | |
| after, aerr := store.CurrentKeyEpoch(ctx) | |
| if aerr != nil { | |
| return false, fmt.Errorf("workspace key rotation failed and re-reading the epoch failed, so a mid-commit half-mint cannot be ruled out (rotate error: %v): %w", rerr, aerr) | |
| } | |
| if after != epoch { | |
| return false, fmt.Errorf("workspace key rotation failed mid-commit (epoch advanced %d→%d; grants may not have published): %w", epoch, after, rerr) | |
| } |
🤖 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 `@internal/cli/sync.go` around lines 356 - 358, The key-epoch check in sync
handling is conflating two different failures and discarding the
`CurrentKeyEpoch` read error. Update the logic around
`store.CurrentKeyEpoch(ctx)` to report a distinct message when `aerr != nil`,
wrapping `aerr` directly, and only use the “epoch advanced” wording when `after
!= epoch`. Keep the fail-closed behavior, but make the error path in `sync.go`
clearly distinguish the DB read failure from the mid-commit epoch change so
operators debugging the revoke flow see the right cause.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #134.
What
When
devices revoke/lost's WCK rotation failed, nothing persisted that a rotation was owed — the old epoch stayed active and every event pushed until a manualdevstrap keys rotateremained readable by the revoked device. Age-triggered rotation wouldn't fire for ~90 days.How
Option 1 (auto-retry) plus the cheap preflight (option 3) from the issue:
wck_rotation_pendinglocal_metarow (JSON{epoch, since}; no schema migration — the genericGetLocalMeta/SetLocalMetaaccessors from P4-SYNC-02 already exist). New idempotentstate.Store.DeleteLocalMeta.maybeRotateWorkspaceKeyrotates when a rotation is owed regardless of epoch age — and even withkeys.rotate_max_age=0(disabling periodic rotation must not disable committed revoke containment).Rotate(sync retry,keys rotate, or a later revoke's rotation — every local Rotate excludes locally-revoked devices, exactly the proof required). Deliberately never by observing a newer epoch: a peer that hasn't pulled the revoke yet can rotate for age reasons and grant the new epoch to the revoked device, so "newer epoch" is not proof of exclusion. Worst case of ignoring a legitimate peer rotation: one redundant epoch.device.revokedevent itself still pushes andrun-loopdoesn't abort; a failure with the epoch advanced (mid-commit half-mint, grants possibly unpublished) stays fatal for the cycle.warnMalformedRemainingRecipientsvia newworkspacekeys.ValidateRecipient): the likeliest Rotate failure — a malformed age recipient on a remaining approved device — is named on stderr before the trust write. Advisory only: the revoke always proceeds (refusing would keep a compromised device approved, per the PR feat(devices): synced device-trust propagation (TRUST-01) #132 adversarial ruling).workspace key rotationcheck warnsowed since <ts>with the sync-auto-retry +keys rotateremedy; silent when nothing is owed.Accepted residual (spec/15): events pushed between the failed revoke rotation and this device's next successful rotation remain readable by the revoked device — now bounded and self-healing instead of "until someone remembers".
Tests
TestDeviceRevokeRotationFailureMarksPendingAndSyncRetries,TestMaybeRotateWarnsAndContinuesCycleOnEarlyOwedFailure,TestWCKRotationPendingSurvivesNewerEpoch(the HIGH's regression pin),TestKeysRotateClearsOwedRotation,TestWCKRotationPendingMalformedRecordStaysPending,TestDoctorWarnsWCKRotationPending,TestDeleteLocalMetaIdempotent.Specs
spec/07 (revoke lifecycle), spec/13 (doctor inventory), spec/15 (residual re-bounded), spec/18 work log.
Validation
gofmtclean ·golangci-lint run0 issues ·go test -race ./...ok ·spec-drift --base origin/mainpass · dual review (opus reviewer + Codex adversarial; both HIGHs fixed pre-merge, dispositions in the work log).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
doctornow reports when a workspace key rotation is pending and suggests a fix.Bug Fixes