feat(devices): self-healing WCK rotation after revoke (#134)

internal/cli/devices.go

@@ -15,6 +15,7 @@ import (
 	"github.com/Reederey87/DevStrap/internal/pairing"
 	"github.com/Reederey87/DevStrap/internal/state"
 	dssync "github.com/Reederey87/DevStrap/internal/sync"
+	"github.com/Reederey87/DevStrap/internal/workspacekeys"
 	"github.com/spf13/cobra"
 	"golang.org/x/term"
 )
@@ -302,6 +303,11 @@ func newDeviceTrustCommand(stdout io.Writer, opts *options, use, trustState stri
 				}
 			}
 			if trustState == "revoked" || trustState == "lost" {
+				// Issue #134: preflight the post-revoke rotation's recipients
+				// BEFORE the trust write so the by-far-likeliest rotation
+				// failure (a malformed recipient on a REMAINING device) is
+				// named while the operator is still looking at the revoke.
+				warnMalformedRemainingRecipients(cmd.Context(), stderr, store, args[0])
 				// TRUST-01: the trust flip and the synced device.revoked/lost
 				// event land in ONE transaction (P6-DATA-03), BEFORE the WCK
 				// rotation below — a rotation failure can never orphan the
@@ -816,12 +822,46 @@ func rotateWorkspaceKeyOnRevoke(ctx context.Context, stderr io.Writer, opts *opt
 		// the revoked device (it still holds the old WCK). The trust flip is
 		// deliberately NOT rolled back: refusing the revoke because rotation
 		// failed would keep a compromised device APPROVED, which is worse.
-		// Be loud about the confidentiality gap and the remedy instead.
+		// Be loud about the confidentiality gap and record the owed rotation
+		// so sync's rotation gate retries it every cycle (issue #134).
 		_, _ = fmt.Fprintf(stderr, "warning: workspace key rotation FAILED: %v\n", rerr)
-		_, _ = fmt.Fprintf(stderr, "warning: events pushed before a successful rotation remain readable by the revoked device (it holds the current epoch key). Run 'devstrap keys rotate' until it succeeds, then 'devstrap sync'.\n")
+		if merr := markWCKRotationPending(ctx, store, epoch); merr != nil {
+			_, _ = fmt.Fprintf(stderr, "warning: could not record the owed rotation (%v); sync will NOT auto-retry — run 'devstrap keys rotate' until it succeeds, then 'devstrap sync'\n", merr)
+			return
+		}
+		_, _ = fmt.Fprintf(stderr, "warning: events pushed before a successful rotation remain readable by the revoked device (it holds the current epoch key). The next 'devstrap sync' retries the rotation automatically; run 'devstrap keys rotate' to close the gap now.\n")
 		return
 	}
+	// A successful Rotate excludes every locally-revoked device, so it also
+	// satisfies any rotation still owed from an earlier failed revoke.
+	if cerr := clearWCKRotationPending(ctx, store); cerr != nil {
+		_, _ = fmt.Fprintf(stderr, "warning: could not clear the owed-rotation marker (%v); sync may rotate once more\n", cerr)
+	}
 	if len(grants) > 0 {
 		_, _ = fmt.Fprintf(stderr, "Rotated workspace key to epoch %d; granted to %d remaining device(s); run 'devstrap sync' to publish\n", newEpoch, len(grants))
 	}
 }
+
+// warnMalformedRemainingRecipients preflights the post-revoke rotation (issue
+// #134): the by-far-likeliest Rotate failure is a malformed age recipient on a
+// REMAINING approved device, and by the time Rotate reports it the trust flip
+// is already committed. Parsing each remaining recipient up front names the
+// offending device while the operator is still looking at the revoke.
+// Advisory only — the revoke proceeds regardless (refusing to revoke because a
+// bystander's row is malformed would keep the compromised device approved),
+// and Rotate's wrap-first ordering remains the enforcement. Empty recipients
+// are skipped to mirror ApprovedRecipients.
+func warnMalformedRemainingRecipients(ctx context.Context, stderr io.Writer, store *state.Store, target string) {
+	devices, err := store.ListDevices(ctx)
+	if err != nil {
+		return
+	}
+	for _, dev := range devices {
+		if dev.ID == target || dev.TrustState != "approved" || strings.TrimSpace(dev.PublicKey) == "" {
+			continue
+		}
+		if err := workspacekeys.ValidateRecipient(dev.PublicKey); err != nil {
+			_, _ = fmt.Fprintf(stderr, "warning: remaining device %s has a malformed age recipient (%v); the post-revoke key rotation will fail until it is re-enrolled with a valid recipient\n", dev.ID, err)
+		}
+	}
+}

internal/cli/doctor.go

@@ -228,6 +228,7 @@ func runDoctorChecks(ctx context.Context, opts *options) []checkResult {
 
 			results = append(results, checkSkippedEvents(ctx, store)...)
 			results = append(results, checkWorkspaceKeyAge(ctx, opts, store)...)
+			results = append(results, checkWCKRotationPending(ctx, store)...)
 			results = append(results, checkForgeCLIs(ctx, opts, store)...)
 			results = append(results, checkAgentRunSweep(ctx, opts, store)...)
 			results = append(results, checkSandboxViolations(ctx, store)...)
@@ -472,6 +473,28 @@ func gradeWorkspaceKeyAge(epoch int64, created time.Time, maxAge time.Duration,
 	return checkResult{Name: "workspace key age", Status: checkOK, Detail: detail}
 }
 
+// checkWCKRotationPending surfaces an owed WCK rotation (issue #134): a device
+// revoke could not rotate the epoch, so events keep sealing under a key the
+// revoked device still holds until a rotation lands. Silent when nothing is
+// owed — the marker exists only after a failed revoke-path rotation and is
+// cleared only by this device's own successful Rotate (see wck_rotation.go).
+func checkWCKRotationPending(ctx context.Context, store *state.Store) []checkResult {
+	since, pending, err := wckRotationPendingSince(ctx, store)
+	if err != nil || !pending {
+		return nil
+	}
+	detail := "a device revoke could not rotate the workspace key; events remain readable by the revoked device"
+	if !since.IsZero() {
+		detail = fmt.Sprintf("owed since %s: %s", since.Format(time.RFC3339), detail)
+	}
+	return []checkResult{{
+		Name:   "workspace key rotation",
+		Status: checkWarn,
+		Detail: detail,
+		Remedy: "run 'devstrap sync' (retries the rotation automatically) or 'devstrap keys rotate'",
+	}}
+}
+
 // checkSkippedEvents surfaces the durable P6-SYNC-02 skip records: hub
 // objects this device's pulls keep dropping, each holding its origin device's
 // cursor at a seq gap until it applies. The remedy depends on the reason.

internal/cli/keys.go

@@ -57,6 +57,11 @@ re-encrypts blobs and flags secrets for source rotation.`,
 			if err != nil {
 				return err
 			}
+			// A successful Rotate excludes every locally-revoked device, so
+			// it also satisfies a rotation owed from a failed revoke (#134).
+			if cerr := clearWCKRotationPending(cmd.Context(), store); cerr != nil {
+				_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: could not clear the owed-rotation marker (%v); sync may rotate once more\n", cerr)
+			}
 			_, err = fmt.Fprintf(stdout, "Rotated workspace key to epoch %d; queued %d grant event(s); run 'devstrap sync' to publish\n", newEpoch, len(grants))
 			return err
 		},

internal/cli/sync.go

@@ -323,31 +323,69 @@ func keyRotateMaxAge(opts *options) time.Duration {
 // registry is per-device), so a fleet device unknown to the rotator lands on
 // the P6-SEC-03 grace→quarantine→replay path until any device that knows it
 // re-approves it.
+//
+// Issue #134: it also retries a rotation OWED from a failed revoke-path
+// rotation (the wck_rotation_pending marker). The owed retry runs even when
+// keys.rotate_max_age=0 — disabling PERIODIC rotation must not disable the
+// revoke containment a device already committed to.
 func maybeRotateWorkspaceKey(ctx context.Context, stdout io.Writer, opts *options, store *state.Store) (bool, error) {
-	maxAge := keyRotateMaxAge(opts)
-	if maxAge <= 0 {
-		return false, nil
+	pendingSince, pending, err := wckRotationPendingSince(ctx, store)
+	if err != nil {
+		return false, err
 	}
 	epoch, created, err := store.ActiveKeyEpochAge(ctx)
 	if err != nil {
 		return false, err
 	}
-	if epoch == 0 || time.Since(created) < maxAge {
+	if epoch == 0 {
+		return false, nil
+	}
+	maxAge := keyRotateMaxAge(opts)
+	aged := maxAge > 0 && time.Since(created) >= maxAge
+	if !pending && !aged {
 		return false, nil
 	}
 	kr := buildKeyring(ctx, opts, store)
 	newEpoch, grants, rerr := kr.Rotate(ctx)
 	if rerr != nil {
-		// FATAL for the cycle (post-#56 Codex review, P1): Rotate wraps every
-		// grant before writing any state, so a failure here is either
-		// harmless-and-early (nothing recorded; a malformed recipient row
-		// needs fixing, not retrying) or a rare DB/custody fault mid-commit —
-		// and in the latter case pushing would seal this cycle's events under
-		// a half-minted epoch whose grants never published, while the fresh
-		// created_at suppresses the retry. Aborting keeps the cycle's events
-		// queued and the failure loud.
+		// FATAL for the cycle when the failure could be a mid-commit
+		// half-mint (post-#56 Codex review, P1): if the active epoch advanced
+		// under this failed Rotate, pushing would seal this cycle's events
+		// under an epoch whose grants never fully published. Detect it by
+		// re-reading the epoch rather than assuming.
+		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)
+		}
+		if pending {
+			// Early failure (nothing recorded — the malformed-recipient
+			// class). Do NOT fail the cycle: aborting here would also block
+			// pushing the device.revoked event itself, so the fleet never
+			// learns about the revoke — strictly worse than pushing under the
+			// old epoch, which is the already-documented exposure
+			// (adversarial-review finding, issue #134). Warn loudly, keep the
+			// marker, retry next cycle; deliberately NOT progressf-gated.
+			_, _ = fmt.Fprintf(stdout, "warning: workspace key rotation owed since %s still failing (events remain readable by the revoked device until it succeeds): %v\n",
+				pendingSince.Format(time.RFC3339), rerr)
+			_, _ = fmt.Fprintf(stdout, "warning: check 'devstrap devices list' for a malformed age recipient, or run 'devstrap keys rotate' after fixing it\n")
+			return false, nil
+		}
+		// Age-triggered early failure keeps the shipped fatal semantics: a
+		// malformed recipient row needs fixing, not retrying, and there is no
+		// queued revoke event whose propagation the abort would block.
 		return false, fmt.Errorf("periodic workspace key rotation failed (fix the cause or disable via keys.rotate_max_age=0 / --key-max-age 0): %w", rerr)
 	}
+	if pending {
+		// The ONLY resolution path for the owed marker (see wck_rotation.go:
+		// a newer epoch alone is not proof the revoked device was excluded; a
+		// local Rotate is). A delete failure surfaces as a cycle error so the
+		// marker cannot silently outlive its rotation.
+		if cerr := clearWCKRotationPending(ctx, store); cerr != nil {
+			return true, cerr
+		}
+		opts.progressf(stdout, "Rotated workspace key to epoch %d (rotation owed since %s after a device revoke); %d grant event(s) ride this push\n",
+			newEpoch, pendingSince.Format(time.RFC3339), len(grants))
+		return true, nil
+	}
 	opts.progressf(stdout, "Rotated workspace key to epoch %d (epoch %d exceeded keys.rotate_max_age %s); %d grant event(s) ride this push\n",
 		newEpoch, epoch, maxAge, len(grants))
 	return true, nil

internal/cli/wck_rotation.go

@@ -0,0 +1,64 @@
+package cli
+
+import (
+	"context"
+	"encoding/json"
+	"time"
+
+	"github.com/Reederey87/DevStrap/internal/state"
+)
+
+// wckRotationPendingMetaKey is the local_meta row recording that a WCK
+// rotation is OWED: a `devices revoke`/`lost` could not rotate the epoch, so
+// events keep sealing under a key the revoked device still holds until a
+// rotation lands (issue #134). Machine-local retry bookkeeping only — it never
+// rides the event log.
+const wckRotationPendingMetaKey = "wck_rotation_pending"
+
+// wckRotationPendingRecord is the JSON value of the pending row. Epoch records
+// the epoch that was ACTIVE when the rotation failed (diagnostic only — see
+// wckRotationPendingSince for why it must NOT drive resolution).
+type wckRotationPendingRecord struct {
+	Epoch int64     `json:"epoch"`
+	Since time.Time `json:"since"`
+}
+
+// markWCKRotationPending persists the owed-rotation marker after a failed
+// revoke-path rotation so sync's rotation gate retries it on every cycle.
+func markWCKRotationPending(ctx context.Context, store *state.Store, epoch int64) error {
+	raw, err := json.Marshal(wckRotationPendingRecord{Epoch: epoch, Since: time.Now().UTC()})
+	if err != nil {
+		return err
+	}
+	return store.SetLocalMeta(ctx, wckRotationPendingMetaKey, string(raw))
+}
+
+// wckRotationPendingSince reports whether a rotation is still owed. The marker
+// resolves ONLY via clearWCKRotationPending after THIS device's own successful
+// Rotate — never by observing a newer epoch (adversarial-review HIGH, issue
+// #134): a peer that has not yet pulled the revoke can rotate for age reasons
+// and grant the new epoch to the still-approved-in-its-registry revoked
+// device, so "any newer epoch is active" is NOT proof the revoked device was
+// excluded. A locally-run Rotate always excludes locally-revoked devices,
+// which is exactly the proof the marker needs; the worst case of ignoring a
+// legitimate peer rotation is one redundant epoch. A marker that fails to
+// parse stays pending with a zero Since (fail-closed).
+func wckRotationPendingSince(ctx context.Context, store *state.Store) (time.Time, bool, error) {
+	raw, ok, err := store.GetLocalMeta(ctx, wckRotationPendingMetaKey)
+	if err != nil || !ok {
+		return time.Time{}, false, err
+	}
+	var rec wckRotationPendingRecord
+	if jerr := json.Unmarshal([]byte(raw), &rec); jerr != nil {
+		return time.Time{}, true, nil
+	}
+	return rec.Since, true, nil
+}
+
+// clearWCKRotationPending removes the marker. Called ONLY after a successful
+// local Rotate (sync's owed retry, `keys rotate`, or a later revoke whose
+// rotation succeeded) — every local Rotate wraps to ApprovedRecipients, which
+// excludes all locally-revoked devices, satisfying the owed containment.
+func clearWCKRotationPending(ctx context.Context, store *state.Store) error {
+	return store.DeleteLocalMeta(ctx, wckRotationPendingMetaKey)
+}