feat(encryption): previousMasterKeys keyring rotation surface (LAB-685) - #103
Open
27Bslash6 wants to merge 7 commits into
Open
feat(encryption): previousMasterKeys keyring rotation surface (LAB-685)#10327Bslash6 wants to merge 7 commits into
27Bslash6 wants to merge 7 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.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesThe change adds decrypt-only master-key rotation across TypeScript and WebAssembly bindings. It validates exact key sizes and keyring limits, forwards previous keys through runtime configuration, retains current-key-only writes, and adds native, cache, secure-cache, and Workers coverage. Master-key rotation contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SecureCache
participant EncryptionManagerCore
participant NativeTenantKeys
participant CacheBackend
SecureCache->>EncryptionManagerCore: Configure current and previous master keys
EncryptionManagerCore->>NativeTenantKeys: Derive tenant keys and keyring
SecureCache->>CacheBackend: Read encrypted entry
CacheBackend-->>SecureCache: Return ciphertext
SecureCache->>NativeTenantKeys: Decrypt with current key, then previous keys
NativeTenantKeys-->>SecureCache: Return plaintext or decryption failure
SecureCache->>NativeTenantKeys: Encrypt new entry with current key
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
Author
|
@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.
27Bslash6
dismissed stale reviews from kodus-27b[bot] and coderabbitai[bot]
via
August 8, 2026 03:47
8fa3a61
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Resolved merge conflict with |
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 `@packages/cachekit-core-wasm/README.md`:
- Around line 39-43: Update the README example around deriveTenantKeys and
rotating to call rotating.free() deterministically after all demonstrated
operations using the handle are complete.
In `@packages/cachekit-core-wasm/src/lib.rs`:
- Around line 213-217: Wrap the owned key-copy staging buffer created in
derive_tenant_keys with zeroizing::Zeroizing so its contents are wiped on drop,
including early-return paths. Add the required zeroize import and declare the
dependency in the wasm crate’s Cargo.toml, preserving the existing key
conversion behavior.
In `@packages/cachekit/README.md`:
- Around line 210-211: Update the key-material lifecycle statement in the README
to remove the claim that all key material enters native memory once and is fully
zeroized on disposal. Document that derived native keyring material is zeroized
when released, while managed-runtime configuration strings and environment
variables remain sensitive for their entire lifetime because
initialization-retry state is retained.
In `@packages/cachekit/src/encryption/manager-core.test.ts`:
- Around line 223-239: Update the wrong-entry-count test around
EncryptionManagerCore to provide the mocked derived-key handle with a free spy,
then assert that it is called after the version-skew rejection. Match the
sibling skew test’s assertion and preserve the existing deriveTenantKeys
key-count mismatch setup.
In `@packages/cachekit/src/encryption/manager-core.ts`:
- Around line 198-228: Update the key-derivation flow around
native.deriveTenantKeys to zeroize masterKeyBytes and every previousKeyBytes
entry in a finally block, covering successful, validation-failure, and thrown
derivation paths. Correct the nearby comment to state that buffers are
explicitly wiped after use, while preserving the existing keyring attestation
and avoiding key material in errors or logs.
In `@packages/cachekit/src/encryption/manager.integration.test.ts`:
- Around line 369-382: Update the three napi.deriveTenantKeys assertions in the
“enforces the keyring invariants natively too” test to match the exact
cachekit-core error text for the forward-only collision, previous-key cap, and
wrong-length key rules. Use precise error matchers rather than bare toThrow(),
preserving each test case’s existing inputs and rule coverage.
🪄 Autofix
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 Plus
Run ID: b6279aa3-05eb-406c-a486-0b75c3b0ace8
⛔ Files ignored due to path filters (2)
packages/cachekit-core-ts/Cargo.lockis excluded by!**/*.lockpackages/cachekit-core-wasm/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.secrets.baselinepackages/cachekit-core-ts/Cargo.tomlpackages/cachekit-core-ts/README.mdpackages/cachekit-core-ts/index.d.tspackages/cachekit-core-ts/src/lib.rspackages/cachekit-core-wasm/Cargo.tomlpackages/cachekit-core-wasm/README.mdpackages/cachekit-core-wasm/index.d.tspackages/cachekit-core-wasm/src/lib.rspackages/cachekit/README.mdpackages/cachekit/src/cache.rotation.test.tspackages/cachekit/src/cache.tspackages/cachekit/src/constants.tspackages/cachekit/src/encryption/manager-core.test.tspackages/cachekit/src/encryption/manager-core.tspackages/cachekit/src/encryption/manager.integration.test.tspackages/cachekit/src/encryption/manager.tspackages/cachekit/src/errors.tspackages/cachekit/src/intents-core.tspackages/cachekit/src/intents.test.tspackages/cachekit/src/types/cache.tspackages/cachekit/src/workers/runtime.tspackages/cachekit/test/integration/encryption-real-crypto.integration.test.tspackages/cachekit/test/workers/encryption.protocol.workers.test.ts
…B-685) CodeRabbit round (6 findings) + expert-panel follow-up: - wasm: previous-master-key staging copies wrapped in Zeroizing; master key now crosses as a JS handle and is copied under Zeroizing too (the &[u8] ABI copied it into linear memory and freed it unwiped — panel finding on the CodeRabbit fix) - manager-core: decoded master/previous key buffers wiped in a finally once the binding has consumed them, error paths included - READMEs: zeroization claim corrected (JS hex strings cannot be scrubbed); wasm example frees the rotating keyring handle - tests: wrong-count skew path asserts the orphaned handle is freed; previous-key bytes snapshot at call time + post-call wipe asserted; native keyring-invariant errors matched by exact text
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:
|
Contributor
Author
|
@coderabbitai review |
|
Contributor
Author
|
Expert panel (critical-stakes crypto gate) — CodeRabbit remediation round, recorded per project rule. Scope: the zeroization remediation diff (
Pre-existing, out of this PR's scope: Local gates on |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cachekit/src/encryption/manager-core.ts (1)
219-239: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winFree the key handle on every failed attestation path.
Line 225 can throw before either explicit
tenantKeys.free?.()call. The outercatchconverts that error but leaves the derived keyring allocated.Wrap the attestation and disposed checks in a
try/finally. Transfer ownership only after assigningthis.tenantKeys. CalltenantKeys.free?.()infinallyif ownership was not transferred.Proposed fix
- if (previousKeyBytes.length > 0) { + let transferred = false; + try { + if (previousKeyBytes.length > 0) { const built = tenantKeys.keyringEntryCount?.() ?? 1; if (built !== 1 + previousKeyBytes.length) { - tenantKeys.free?.(); throw new ConfigurationError(/* existing message */); } } if (this.disposed) { - tenantKeys.free?.(); throw new EncryptionError('EncryptionManager has been disposed'); } this.tenantKeys = tenantKeys; + transferred = true; + } finally { + if (!transferred) tenantKeys.free?.(); + }As per path instructions: “verify buffer lengths, null checks, and that keys/secrets are zeroized on drop.”
🤖 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 `@packages/cachekit/src/encryption/manager-core.ts` around lines 219 - 239, Update the initialization flow around the keyring attestation and disposed check to use try/finally cleanup. Track ownership transfer only after assigning tenantKeys to this.tenantKeys, and invoke tenantKeys.free?.() in finally whenever ownership was not transferred, including errors from keyringEntryCount or the disposed check; preserve the existing ConfigurationError and EncryptionError behavior and ensure key material is zeroized on every failed path.Source: Path instructions
🤖 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.
Outside diff comments:
In `@packages/cachekit/src/encryption/manager-core.ts`:
- Around line 219-239: Update the initialization flow around the keyring
attestation and disposed check to use try/finally cleanup. Track ownership
transfer only after assigning tenantKeys to this.tenantKeys, and invoke
tenantKeys.free?.() in finally whenever ownership was not transferred, including
errors from keyringEntryCount or the disposed check; preserve the existing
ConfigurationError and EncryptionError behavior and ensure key material is
zeroized on every failed path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a48c615-5880-4507-ba89-7d46ee3dd34b
⛔ Files ignored due to path filters (1)
packages/cachekit-core-wasm/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/cachekit-core-wasm/Cargo.tomlpackages/cachekit-core-wasm/README.mdpackages/cachekit-core-wasm/src/lib.rspackages/cachekit/README.mdpackages/cachekit/src/encryption/manager-core.test.tspackages/cachekit/src/encryption/manager-core.tspackages/cachekit/src/encryption/manager.integration.test.ts
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.
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.Summary by CodeRabbit
New Features
Bug Fixes