Skip to content

feat(secrets): add dotf secrets rotate — replace a credential and prove the replacement took - #1003

Merged
mlorentedev merged 3 commits into
mainfrom
feat/cli-037-secrets-rotate
Aug 15, 2026
Merged

feat(secrets): add dotf secrets rotate — replace a credential and prove the replacement took#1003
mlorentedev merged 3 commits into
mainfrom
feat/cli-037-secrets-rotate

Conversation

@mlorentedev

Copy link
Copy Markdown
Owner

Refs #996. Spec: specs/CLI-037-secrets-rotate/.

Not a closing keyword: the adversarial review cannot launch while #988 is open, so the spec cannot archive. Same constraint as every other spec PR today.

Why

Rotating one credential by hand is five steps, and two of them fail silently. This was walked live on 2026-08-15 while rotating a DockerHub PAT that had to be replaced urgently, and both failures happened:

  • The daemon sync is invisible and load-bearing. bw serve answers reads from its own local cache. Skip the sync and the write succeeds while every read keeps returning the old value — correct write, stale read, and nothing anywhere indicating a step is missing. The operator's question was literally "parece que lo he hecho pero no me sale el valor actualizado, no sé si está funcionando".
  • A liveness probe cannot prove a rotation happened. The verification returned HTTP 200 — and would have returned 200 for the old credential too, had it not been revoked. "The new credential works" and "the credential was actually replaced" are different claims, and only the first was obtainable.

Neither is an operator mistake; the sequence offers no way to tell those states apart.

rotate is not a new idea either: secrets_set.go already documents itself as the primitive that "migrate (C4) and rotate (C7) compose", ADR-029 references rotation as C7, and the registry declares rotate: 90d on nearly every entry while nothing reads that field.

What it does

  1. Fingerprints the current value — sha256, first 12 hex, never the value.
  2. Reads the new value from stdin when piped, else a hidden prompt (reusing set's reader).
  3. Refuses a no-op. Writing the same value back is a typo; set reports it as success because for set it is one. For a rotation it means the credential you meant to retire is still live.
  4. Writes through set's idempotent path.
  5. Syncs the daemon — the step humans forget.
  6. Re-resolves through the normal read path and fails if the fingerprint did not move, catching a write that never reached the reader.
  7. Runs the entry's declared validate: probe, reusing what sync ci already gates uploads with.

It never creates. Provisioning and replacing are different acts, and conflating them is how a locked vault becomes a duplicate item (#612).

Evidence

Six tests, each pinning a state the manual sequence could not distinguish:

Test What it pins
WritesSyncsAndProvesTheChange sync called exactly once; before→after reported; no secret value in the output
RefusesANoOp identical value → error, zero writes
FailsWhenTheReadPathStillServesTheOldValue write accepted but reader unchanged → failure, not success
RefusesToCreate absent field → error pointing at set, zero writes
DryRunWritesNothing no write, no sync, reports the probe it would run
SyncFailureWarnsButDoesNotFail a completed write is not reported as a failed rotation
go build ./...   OK
go vet ./...     OK
go test ./...    OK

Known limitation, stated rather than hidden

rotate writes through bwWriter (the bw CLI shellout), so it inherits #993: it needs an ambient BW_SESSION until the write path moves to the daemon. That is the whole write half of ADR-028, not this command, and fixing it here would be the wrong place. BW_SESSION="$(bw unlock --raw)" dotf secrets rotate <ID> works today and confines the session to one process.

…rove it took

Rotating one credential by hand is five steps, and two of them fail silently.
Walked live while rotating a leaked DockerHub PAT: the bw serve daemon answers
reads from its own cache, so a correct write kept serving the old value with no
signal that a step was missing; and the liveness probe returned 200 for a token
that had not actually been replaced, because an unrevoked old credential
authenticates exactly as well as a new one.

rotate composes the existing primitives and closes both gaps:

- fingerprints the current value (sha256, first 12 hex — never the value), so
  "the value changed" is provable separately from "the value works". A probe
  alone cannot tell those apart, which is why a write landing in the wrong field
  looked successful.
- refuses a no-op: writing the same value back is a typo, and `set` reports it as
  success because for `set` it is one.
- syncs the daemon after writing, the step humans forget.
- re-resolves through the normal read path and fails if the fingerprint did not
  move, catching a write that never reached the reader.
- runs the entry's declared `validate:` probe, reusing what `sync ci` already has.

Never creates: provisioning and replacing are different acts, and conflating them
is how a locked vault becomes a duplicate item.

Refs #996
A diagnostic step piped vault item bodies through a sed redaction filter written
against the expected payload shape. The real shape differed, the filter passed it
through unchanged, and twelve full item bodies — two live PATs, plaintext password
history, the item key — landed in a session transcript that cannot be shredded.

The redaction was not skipped; it was present and ineffective, which is worse,
because it produced the confidence of having handled the problem. A filter is an
assertion about a format you are debugging precisely because you do not
understand it, and it fails open.

Refs #988
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@mlorentedev, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e6596b56-b01a-4a75-9cac-af371685a6c2

📥 Commits

Reviewing files that changed from the base of the PR and between 5e79333 and 8ca9b57.

📒 Files selected for processing (9)
  • cli/internal/cmd/secrets.go
  • cli/internal/cmd/secrets_rotate.go
  • cli/internal/cmd/secrets_rotate_test.go
  • cli/internal/secrets/bwserve.go
  • cli/internal/secrets/secrets.go
  • docs/lessons.md
  • specs/CLI-037-secrets-rotate/proposal.md
  • specs/CLI-037-secrets-rotate/tasks.md
  • specs/CLI-037-secrets-rotate/verification.md

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.

@mlorentedev
mlorentedev merged commit 3644847 into main Aug 15, 2026
13 checks passed
@mlorentedev
mlorentedev deleted the feat/cli-037-secrets-rotate branch August 15, 2026 09:00
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