feat(encryption): previousMasterKeys keyring rotation surface (LAB-685) - #103
feat(encryption): previousMasterKeys keyring rotation surface (LAB-685)#10327Bslash6 wants to merge 5 commits into
Conversation
Master-key rotation without invalidating existing entries, per protocol decisions/key-rotation.md + spec/encryption.md 'Key Rotation (Keyring)': - previousMasterKeys config (max 3, hex identical to masterKey; env CACHEKIT_PREVIOUS_MASTER_KEYS comma-separated). Load-time rejection: >3 keys throws (never truncates), masterKey in the list throws (forward-only rule, case-insensitive hex compare). - Keyring decrypt loop behind the NAPI boundary via cachekit-core 0.5.0 Keyring: sequential attempts, current key first, identical AAD per attempt; only auth failures advance. wasm binding mirrors NAPI so the same config works on Workers. Single-key path unchanged (no keyring, no per-decrypt HKDF). - Key bytes cross the boundary once at init; keyring material zeroizes on drop in cachekit-core. No derived-key bytes retained in JS. - NonceExhaustedError guidance now names forward-only rotation and links the rotation runbook (page authored by LAB-687). - cachekit-core pins bumped 0.4.0 -> 0.5.0 in both crates.
Expert-panel (critical-stakes) findings applied:
- MAJ: attest the keyring survived the FFI boundary. NAPI silently drops
extra arguments, so a version-skewed prebuilt binary would build a
single-key handle and every pre-rotation entry would silently degrade
to a miss (LAB-241 class). Both bindings now expose keyringEntryCount();
init throws ConfigurationError on mismatch, and the method's absence
(pre-keyring binary) is itself the skew signal.
- Reject duplicate previousMasterKeys entries (case-insensitive) — they
silently burn keyring cap slots.
- Per-key index in validation errors (Previous master key N ...).
- Rotation guidance + runbook URL now lives once, in the
NonceExhaustedError default message.
- MIN_MASTER_KEY_* renamed MASTER_KEY_* (internal): validation enforces
exact length, the 'minimum' name and '32+/min 32 bytes' doc claims were
looser than shipped behavior.
- README manual-config example no longer teaches raw split(',') env
parsing; the Master-Key Rotation section is the single reference.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 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 Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (25)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
…licit test-key fixture (LAB-685) - js-sys exact-pinned to =0.3.98, the release paired with the already exact-pinned wasm-bindgen 0.2.121 (same ABI-tracking rationale). OSV + cargo-audit evidence added to the PR description. - Drop the unused MIN_MASTER_KEY_BYTES import in the real-crypto integration test — the constant was renamed to MASTER_KEY_BYTES in this PR and the import was never used, so the integration lane's tsc-less vitest run masked it. - Rotation test master keys now come from a testMasterKeyHex helper that documents they are deterministic fixtures, not secrets.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@kody start-review |
Resolves the README conflict from #104 (LAB-238, ciphertext in L1): both sides annotated the same encryption config comment with orthogonal facts — L1 zero-knowledge parity and the rotation pointer — so the resolution keeps both rather than picking a side.
…B-685) Merging main brought in #104 (LAB-238), which makes L1 hold ciphertext for a secure cache. That creates a path neither branch could test on its own: an L2 read under a previous key repopulates L1 with bytes the current key cannot open, so every subsequent L1 hit has to run the keyring loop again. The existing rotation tests all disable L1 — correct when they were written, since L1 then held plaintext and rotation could not reach it. Without keyring coverage on that path decodeL1Entry drops the entry and falls through to L2 on every read for the whole grace window: a silent L1 bypass under degradation, a throw on every old-key read without it. Verified by mutation — breaking the L1 decrypt path turns the single backend.get into two, and the test fails.
8fa3a61
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Resolves LAB-685 (stage 2 of the LAB-516 key-rotation train).
What
Master-key rotation without invalidating existing entries, per
protocol/decisions/key-rotation.md+spec/encryption.md→ "Key Rotation (Keyring)" (protocol PR #34):previousMasterKeys: string[]onEncryptionConfig/createCache.secure()(max 3, hex validation identical tomasterKey; env fallbackCACHEKIT_PREVIOUS_MASTER_KEYS, comma-separated). Load-time rejection withConfigurationError: >3 keys (never truncates),masterKeyin the list (forward-only rule, case-insensitive), duplicate entries.Keyring: sequential attempts, current key first, identical AAD rebuilt per attempt; only AES-GCM auth failures advance; structural/config errors terminal. The wasm binding mirrors NAPI so the same config works on Workers (manager-core is shared — a platform-conditional config surface would be a security lie).previousMasterKeysis absent.keyringEntryCount(); init throwsConfigurationErrorif a version-skewed native binary silently dropped the keyring argument (NAPI ignores extra args — without this, rotation config would silently no-op into whole-cache misses, LAB-241 class).NonceExhaustedErrorguidance: forward-only rotation (never re-promote a retired key) + runbook linkhttps://docs.cachekit.io/concepts/key-rotation/(page authored by LAB-687 — anchor contract noted on the ticket).Tests (714 unit + 98 workers, all green)
masterKey=k₂, previousMasterKeys=[k₁], fails with[]; writes stay on the current key; native-layer invariants (cap/collision/length) asserted directly against the binding.EncryptionErrorwithdegradation: false.keyringEntryCountrefuses init loudly.Expert panel (critical-stakes crypto gate — mandatory, recorded here)
4-agent panel (bug-hunter-supreme, security-specialist, code-craftsman, catchphrase) on d8f1b7c:
Keyring::new, no case bypass), AAD invariance across attempts (single buffer by construction), no key-index oracle (exhaustion collapses to the sameAuthenticationFailedas single-key), no truncation path, no key material in errors/logs, LAB-683 config-vs-auth error separation honored (config errors throw synchronously atcreateCache, before any fail-open path exists).NonceExhaustedErrordefault, README env-parsing example cut,MIN_MASTER_KEY_*→MASTER_KEY_*rename (internal; "minimum"/"32+ bytes" claims were looser than the exact-length check actually shipped), qualified the "no key bytes retained" comment (hex config strings remain, per the documented masterKey pattern).Out of scope (per ticket)
Nonce-exhaustion handling (monitoring unchanged), fingerprint frame field (ts is the sequential-attempt branch by spec), runbook page authoring + feature-matrix flip (LAB-687, stage 3).
Dependency / SCA evidence (Kody review, 2026-08-08)
New/changed crates in this PR:
cachekit-core 0.4.0 → 0.5.0(both binding crates) andjs-sys(new, wasm crate only — ships from the wasm-bindgen workspace and tracks its ABI).js-sysis now exact-pinned to=0.3.98, the release paired with the already exact-pinnedwasm-bindgen =0.2.121(commit 159e570).Audit results (2026-08-08):
cargo auditonpackages/cachekit-core-ts/Cargo.lock(140 crates): 0 vulnerabilities. One pre-existing allowlisted unmaintained notice — RUSTSEC-2024-0436 forpaste 1.0.15viarmp-serde ← cachekit-core; present onmainsince cachekit-core 0.4.0 (#91), not introduced here.cargo auditonpackages/cachekit-core-wasm/Cargo.lock(118 crates): 0 vulnerabilities, 0 warnings.pasteunmaintained notice, no known vulnerabilities forjs-sys 0.3.98,cachekit-core 0.5.0, or any other resolved version.