LAB-516: master-key rotation — client-side keyring with grace window (decision + normative spec)#34
Conversation
…window Decision record (decisions/key-rotation.md) + normative spec section replacing the never-implemented RotationAwareHeader story: - Keyring: forward-only current key + <=3 decrypt-only master keys; MUST reject oversized config. No wire change (AAD v0x03 and ciphertext layout untouched). - Key selection: per-entry derived-key fingerprint match where identity exists (cachekit-py CK frames, match is binding/terminal); sequential same-AAD attempts elsewhere (ts/rs/interop). AAD no-retry rule preserved. - Forward-only current key: re-promoting a used key resumes an unknowable nonce budget -> AES-GCM nonce reuse; normative MUST NOT. - Runbooks: three-phase zero-miss scheduled rotation (add decrypt-only -> promote -> drop after longest TTL from promotion-complete), non-expiring-entry precondition, compromise = empty keyring + flush; honest limits on both captured-ciphertext protection and managed-runtime key scrubbing. - Retires dead machinery for deletion in LAB-516 children: rotate_key() stub, KeyRotationState (relative 2-slot key_version cannot survive a second rotation), RotationAwareHeader, dead PyO3 bindings. - interop-mode.md: drop dangling RotationAwareHeader mention; encryption.md decryption flow gains the key-selection step; drop dead decisions/ link; README + CHANGELOG entries. Expert-panel reviewed (crypto gate): 4-agent panel; applied all surviving findings (forward-only rule, fingerprint-of-derived-key pin, two-phase rollout, window clock, terminal-match rule, >3-key rejection, hygiene honesty). Co-authored-by: multica-agent <github@multica.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe changes define client-side master-key rotation using one current key and up to three decrypt-only keys. They replace the retired rotation header specification, document fingerprint and sequential key selection, clarify interop behaviour, and add decision-record references and operational runbooks. ChangesClient-side key rotation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 6
🤖 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 `@decisions/key-rotation.md`:
- Line 1: Update decisions/key-rotation.md so the top-level “Decision Record...”
H1 appears before the existing Protocol breadcrumb, retaining the breadcrumb
immediately below the heading.
- Around line 62-64: Qualify the decryptability guarantee to state that
ciphertext remains format-compatible and decryptable only while the key that
encrypted each entry is retained. Update the statement in
decisions/key-rotation.md lines 62-64 and replace the unconditional wording in
spec/encryption.md lines 369-371 with the same conditional language.
- Around line 106-109: Update the key-rotation specification near the
forward-only rule to define enforcement: state that SDK configuration alone
cannot detect re-promotion after a retired key leaves the supplied key set, and
make non-repromotion an explicitly operator-enforced invariant or specify an
external, operator-maintained key history and validation mechanism.
In `@spec/encryption.md`:
- Around line 435-441: Update the decryption input definition to include the
optional per-entry key fingerprint in stored metadata. Specify that the
fingerprint is extracted from the CK frame, and ensure the existing
key-selection flow uses it when present while retaining sequential keyring
attempts when absent.
- Line 364: Update the shared fingerprint formula around the key-fingerprint
definition to explicitly use tenant_keys.encryption_key as the key input. Add
test vectors covering per-entry key selection, including deriving and
fingerprinting each tenant encryption key and matching the stored fingerprint,
so SDK implementations remain interoperable.
- Around line 438-441: Update the key-selection flow in “Key Rotation (Keyring)”
to state that authentication failures on intermediate sequential key attempts
continue to the next permitted key, while exhaustion applies the SDK’s fail-open
or fail-closed policy. Clarify that authentication failure is a hard error only
after the permitted keys are exhausted, and preserve the existing
fingerprint-match and current-key-first ordering.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ac59e08-4c9c-406c-863c-cb7288af5b83
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mddecisions/key-rotation.mdspec/encryption.mdspec/interop-mode.md
…ator-enforced forward-only rule, derived-key fingerprint pin, flow inputs + intermediate-retry semantics - Decryptability claims now conditional on the encrypting key remaining in the keyring (compromise cut-over / retirement are the deliberate exceptions). - Forward-only rule: explicitly operator-enforced (stateless SDKs cannot detect re-promotion once the retired key left config; treat retired keys as destroyed); SDKs MUST reject current-key-in-decrypt-only-list. - Key Fingerprint section pins the per-entry fingerprint input to the derived per-tenant encryption key, never the master key. - Decryption Flow: inputs include the optional per-entry key_fingerprint; step 1 distinguishes intermediate sequential-attempt failures (continue) from exhaustion (hard error / policy). - Fingerprint-selection conformance vector added to the LAB-687 (stage 3) scope. Co-authored-by: multica-agent <github@multica.ai>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
LAB-516 acceptance criterion 1: the design decision, recorded in
protocol/first.What this decides
Master-key rotation = a client-side keyring with an operator-controlled grace window. One forward-only current key (encrypts everything) plus at most 3 decrypt-only master keys. Rotation state is configuration — no state machine, no rotation API, no wire change, no server involvement (zero-knowledge makes the server useless here by construction). Invalidate-on-rotate survives as the degenerate case (empty decrypt-only list) and is the prescribed compromise response.
Rejected: the
RotationAwareHeader/key_versiondirection the dead code in cachekit-core pointed at — it's a wire migration for every reader, its relative 2-slot versioning cannot survive a second rotation, and the 16-byte derived-key fingerprint already provides absolute key identity where it's stored.Files
decisions/key-rotation.md(new) — rationale, rejected options, three-phase zero-miss rotation runbook + compromise runbook, per-repo consequences (the LAB-516 sub-issues).spec/encryption.md— normative Key Rotation (Keyring) section replaces the fictional RotationAwareHeader section (never written by any SDK); decryption flow gains the key-selection step; explicitly status-marked specified, not yet implemented.spec/interop-mode.md— dangling RotationAwareHeader mention removed; interop readers use sequential key attempts.README.md/CHANGELOG.md— navigation + changelog entries.../decisions/encryption-algorithm.mdlink (file never existed; this PR createsdecisions/and would have made the 404 look legitimate).Crypto review gate (project rule, 2026-07-17)
4-agent expert panel ran at high stakes before this PR. All surviving findings applied, notably:
Interaction with open PRs
Does not touch
sdk-feature-matrix.md— the rotation row (currently overclaiming ✅ for py/rs) is corrected to ❌ fleet-wide by #29 (LAB-275). This PR references that correction.After merge
Implementation lands as staged LAB-516 sub-issues (core deletions + shared keyring helper → py/ts/rs surfaces → docs runbook + matrix flips), parked until this design is approved.
Summary by CodeRabbit