feat(encryption): keyring rotation — previous_master_keys + sequential decrypt (LAB-686) - #63
Conversation
…equential decrypt (LAB-686) Rotating the master key previously invalidated every encrypted entry: the SDK had no rotation surface at all. Implements the stage-2 rs child of the LAB-516 key-rotation train, per protocol/decisions/key-rotation.md and spec/encryption.md 'Key Rotation (Keyring)'. - config: .previous_master_keys(...) builder (hex validation identical to .master_key()) + CACHEKIT_PREVIOUS_MASTER_KEYS env (comma-separated hex). Cap of 3 rejected never truncated; current-key self-collision rejected at build/load (forward-only rotation, detectable subset). - encryption: EncryptionLayer holds cachekit_core::Keyring (the shared stage-1 helper, LAB-683 / core 0.5.0 — no keyring logic re-implemented here). Decrypt attempts keys sequentially, current first, identical AAD per attempt; rs entries carry no per-entry key identity, so the sequential branch is the spec-assigned one (no fingerprint selection). Writes always encrypt under the current key. - client: encryption_from_bytes_with_previous(...) + from_env wiring. - core pin bumped 0.4 -> 0.5 (Keyring ships in 0.5.0). - docs: builder doc-test, README key-rotation section + env table row. Config-level cap constant mirrors core's (feature-gated) constant; drift-guard test asserts equality.
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 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 selected for processing (2)
WalkthroughCachekit adds decrypt-only previous master-key support. Configuration accepts up to three previous keys from the environment or builder. Encryption uses the current key for writes and ordered keyring fallback for reads. ChangesMaster-key rotation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant CacheKitBuilder
participant EncryptionLayer
participant Keyring
Application->>CacheKitBuilder: configure current and previous keys
CacheKitBuilder->>EncryptionLayer: create encryption layer
EncryptionLayer->>Keyring: initialise ordered keyring
Application->>EncryptionLayer: write data
EncryptionLayer-->>Application: ciphertext encrypted with current key
Application->>EncryptionLayer: read ciphertext
EncryptionLayer->>Keyring: try current key, then previous keys
Keyring-->>Application: decrypted data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cachekit/src/encryption.rs (1)
85-98: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueValidate the current master key before the previous keys, and note the error-variant asymmetry.
Two points about this validation block:
- The previous-key length loop runs before the current-key length check. If a caller supplies a short current key together with a short previous key, the returned error names the previous key. The current key is the more fundamental fault and should surface first.
- The same defect class returns different variants. A short previous key returns
CachekitError::Config. A short current key three lines below returnsCachekitError::Encryption. A caller that matches on the variant to distinguish operator configuration faults from crypto faults will misclassify a short current key.Point 2 is partly pre-existing, since the current-key check kept its original variant. Changing it would break the
master_key_too_shortexpectation. Reordering alone is safe and improves the diagnostic.♻️ Proposed reorder
) -> Result<Self, CachekitError> { - for (i, key) in previous_keys.iter().enumerate() { - if key.len() < 32 { - return Err(CachekitError::Config(format!( - "previous master key {i} must be at least 32 bytes; got {}", - key.len() - ))); - } - } if master_key_bytes.len() < 32 { return Err(CachekitError::Encryption(format!( "master key must be at least 32 bytes; got {}", master_key_bytes.len() ))); } + for (i, key) in previous_keys.iter().enumerate() { + if key.len() < 32 { + return Err(CachekitError::Config(format!( + "previous master key {i} must be at least 32 bytes; got {}", + key.len() + ))); + } + }🤖 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 `@crates/cachekit/src/encryption.rs` around lines 85 - 98, In the validation block containing the previous_keys loop and master_key_bytes check, move the current master-key length validation before iterating over previous_keys so a short current key is reported first when both are invalid. Preserve the existing CachekitError::Encryption variant and all current error messages; do not change the previous-key validation behavior.
🤖 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 `@crates/cachekit/src/client.rs`:
- Around line 1013-1019: Update the documentation for the encryption
configuration methods around EncryptionLayer::with_previous_keys and
encryption_from_bytes to state that the current master key and every previous
key must be at least 32 bytes. Replace the outdated “at least 16 bytes (32
recommended)” wording in encryption_from_bytes so both docs match the validation
enforced by the implementation.
In `@crates/cachekit/src/config.rs`:
- Around line 266-278: Update decode_master_key_hex to return Zeroizing<Vec<u8>>
and wrap the successfully decoded bytes before returning, while preserving its
existing validation and error behavior. Remove redundant Zeroizing::new wrapping
at all four callers, including the current- and previous-key validation paths,
so the returned wrapper owns the decoded key material for its full lifetime and
zeroizes it on drop.
- Around line 114-134: Update the CACHEKIT_PREVIOUS_MASTER_KEYS handling in the
configuration loading function to reject a configured non-empty previous-key
list when config.master_key is absent, before storing the keys, so encryption is
never silently disabled; preserve validate_previous_master_keys for
configurations with a current master key. Treat a blank or whitespace-only
environment value as unset and skip parsing it, while continuing to reject empty
entries within a non-empty comma-separated list.
In `@crates/cachekit/src/encryption.rs`:
- Around line 392-402: Add boundary-success tests for the three-key limit: in
crates/cachekit/src/encryption.rs:392-402, add a unit test using three distinct
32-byte keys with EncryptionLayer::with_previous_keys(K2, &refs, TEST_TENANT)
and assert success; in crates/cachekit/tests/config_tests.rs:209-219, add a
builder test passing three keys to previous_master_keys and assert
config.previous_master_keys.len() == 3.
- Around line 155-161: The decrypt flow in the keyring currently discards which
key succeeded, preventing operators from measuring previous-key usage. Update
decrypt and its callers or result type to expose the winning key position
(including the current key as index zero) and record a counter keyed by that
position, while preserving plaintext behavior. Also distinguish exhausted key
attempts from malformed ciphertext in the decryption error path.
- Around line 27-47: Update the documentation for EncryptionLayer to remove the
truncated or incomplete text and state the intended Keyring behavior clearly.
Keep the existing zeroization implementation unchanged, including Keyring’s
derived Zeroize and ZeroizeOnDrop behavior.
In `@crates/cachekit/tests/config_tests.rs`:
- Around line 316-320: Update the drift-guard comment above
previous_key_cap_matches_core_keyring_cap to state that MAX_PREVIOUS_MASTER_KEYS
is declared outside the encryption feature gate, while noting that the guard
runs in CI with encryption enabled.
---
Outside diff comments:
In `@crates/cachekit/src/encryption.rs`:
- Around line 85-98: In the validation block containing the previous_keys loop
and master_key_bytes check, move the current master-key length validation before
iterating over previous_keys so a short current key is reported first when both
are invalid. Preserve the existing CachekitError::Encryption variant and all
current error messages; do not change the previous-key validation behavior.
🪄 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: 388ca305-c5d1-4aac-ad69-62c8be43645a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.mdcrates/cachekit/Cargo.tomlcrates/cachekit/src/client.rscrates/cachekit/src/config.rscrates/cachekit/src/encryption.rscrates/cachekit/tests/config_tests.rscrates/cachekit/tests/encryption_tests.rs
…class (LAB-686 panel) Expert-panel findings on PR #63: - decrypt: EncryptionError::KeyDerivation / KeyringIndexOutOfRange now map to CachekitError::Config instead of folding into ::Encryption — per the LAB-683 decision, a config bug must never masquerade as a decrypt failure that fail-open callers read as a miss. - with_previous_keys: master-key length and tenant_id checks now return CachekitError::Config, matching the previous-key and Keyring checks in the same function (was: Encryption for master/tenant, Config for previous — inconsistent within one constructor). - from_env: CACHEKIT_PREVIOUS_MASTER_KEYS without CACHEKIT_MASTER_KEY is now a load-time Config error instead of silently never wiring the previous keys (the botched-rotation-deploy case). - intent_tests: short-master-key assertion updated to the Config class (the test's intent — validation before network I/O — unchanged).
This comment has been minimized.
This comment has been minimized.
Crypto Expert-Panel Review (mandatory gate, high stakes)Four-agent panel (bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent) ran against this diff + the Findings applied (97537b1)
Findings deferred (with reason)
Findings rejected (with reason)
Panel positivesForward-only invariant enforced at both load and build; SDK tightens core's 16-byte floor to 32; no key material in any Debug/error output; AAD built once per decrypt and identical across attempts (no oracle introduced); rotation e2e proves reads never rewrite stored ciphertext and hard cut-over fails closed; drift-guard pins the config cap to |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/cachekit/tests/config_tests.rs`:
- Around line 330-333: Update the environment setup in the
CachekitConfig::from_env test to use an RAII guard for CACHEKIT_MASTER_KEY and
CACHEKIT_PREVIOUS_MASTER_KEYS. Preserve each variable’s original value and
restore it automatically on scope exit, including assertion failures, instead of
unconditionally removing them.
🪄 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: e98a16b6-2f82-4d73-9372-84f65c9c29be
📒 Files selected for processing (4)
crates/cachekit/src/config.rscrates/cachekit/src/encryption.rscrates/cachekit/tests/config_tests.rscrates/cachekit/tests/intent_tests.rs
…, boundary tests (LAB-686) CodeRabbit + Kody round on #63: - decode_master_key_hex returns Zeroizing<Vec<u8>> so decoded key material is wiped on every path, including early-drop when a later validation step fails (CodeRabbit, security) - A wholly blank CACHEKIT_PREVIOUS_MASTER_KEYS is treated as unset: blanking a variable is how shell profiles / Compose / k8s manifests retire it after a completed rotation; a blank entry inside a non-blank list is still rejected (CodeRabbit, correctness) - Boundary-success tests for the cap: exactly three previous keys build (layer + config builder) — a >= regression would have passed the rejecting-side suite (CodeRabbit) - EnvGuard RAII helper in config_tests: env tests restore pre-test variable values on drop, including on assertion failure (CodeRabbit) - Doc fixes: encryption_from_bytes said 'at least 16 bytes' while the code rejects <32; with_previous doc states the 32-byte minimum; EncryptionLayer rotation doc line-wrap; drift-guard comment now says the cap const is declared outside the encryption feature gate
This comment has been minimized.
This comment has been minimized.
…lts test (LAB-686) Expert-panel findings on the review-round commit: - Convert the nine pre-existing env tests to EnvGuard: the old set/call/remove pattern skips cleanup when the call panics, poisoning every later #[serial] test — the exact class EnvGuard was added to kill - config_from_env_defaults now clears CACHEKIT_PREVIOUS_MASTER_KEYS too; a shell-exported value would fail it with an unrelated requires-master-key error - Comment why blank CACHEKIT_MASTER_KEY stays strict while blank previous-keys is tolerated: blank-as-unset on the master key would silently turn encryption off - Drop a comment duplicated from the encryption.rs twin test
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
|
@kody start-review |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/cachekit/src/config.rs`:
- Around line 112-149: The environment values read in the config loading path
must be wrapped in zeroize::Zeroizing<String> before parsing so copied secrets
are cleared. Update the relevant EnvGuard saved-value type to
Option<Zeroizing<String>> and restore saved variables using value.as_str();
apply the corresponding test fixture changes in
crates/cachekit/tests/config_tests.rs lines 199-230, while preserving existing
parsing and validation behavior.
🪄 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: fc2386ef-e509-4f82-9d3e-16353b85595a
📒 Files selected for processing (5)
README.mdcrates/cachekit/src/client.rscrates/cachekit/src/config.rscrates/cachekit/src/encryption.rscrates/cachekit/tests/config_tests.rs
std::env::var hands back an owned String of the hex-encoded key material for CACHEKIT_MASTER_KEY and CACHEKIT_PREVIOUS_MASTER_KEYS. The decoded bytes were already Zeroizing, but the hex source copy was dropped without being wiped — the same secret in a different encoding, equally recoverable from freed heap. EnvGuard in the config tests had the same hole: it saves each variable's pre-test shell value, which for those two vars is real operator key material.
This comment has been minimized.
This comment has been minimized.
Panel note: the comment implied wrapping the env string closes the exposure. It does not -- the process environ block holds the identical hex for the process lifetime and is not wiped. Name that, so the next reader does not over-trust it.
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:
|
Zeroise finding addressed —
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 7a7d3f0 79bee95
git show --format=fuller --find-renames --find-copies 7a7d3f0 -- \
crates/cachekit/src/config.rs crates/cachekit/tests/config_tests.rs
git show --format=fuller --find-renames --find-copies 79bee95 -- \
crates/cachekit/src/config.rs crates/cachekit/tests/config_tests.rs
ast-grep outline crates/cachekit/src/config.rs --items all --type function,struct,impl --match 'CachekitConfig|CachekitConfigBuilder|from_env|decode_master_key_hex'
ast-grep outline crates/cachekit/tests/config_tests.rs --items all --type function,struct,impl --match 'EnvGuard'
rg -n -C 8 'Zeroizing<String>|CACHEKIT_(MASTER_KEY|PREVIOUS_MASTER_KEYS)|decode_master_key_hex|struct EnvGuard|impl (Drop for )?EnvGuard' \
crates/cachekit/src/config.rs crates/cachekit/tests/config_tests.rsLength of output: 6961
I found no remaining issue with the zeroization finding. ✏️ Learnings added
You are interacting with an AI system. |
Kody thread on
|
1 participant
Stage 2 (rs) of the LAB-516 key-rotation train. Closes LAB-686.
Problem
cachekit-rs had no rotation surface: the config builder had
.master_key(hex)and nothing else, so rotating the master key invalidated every encrypted entry.What this adds
An operator can now configure up to 3 decrypt-only previous master keys and read entries written under any of them without re-encrypting — per
protocol/decisions/key-rotation.mdandprotocol/spec/encryption.md→ "Key Rotation (Keyring)"..previous_master_keys(&[hex, ...])onCachekitConfigBuilder(per-entry validation identical to.master_key()) +CACHEKIT_PREVIOUS_MASTER_KEYSenv var (comma-separated hex). More than 3 entries →CachekitError::Config, never truncated. Current key appearing in the previous list → rejected at build/load (detectable subset of the forward-only rule).EncryptionLayernow holds acachekit_core::Keyring(the shared stage-1 helper from LAB-683, core 0.5.0) — no keyring logic re-implemented in this crate. Sequential attempts, current key first, identical AAD per attempt. rs entries carry no per-entry key identity, so the sequential branch is the spec-assigned one (spec L369); no fingerprint selection added, by design.CacheKitBuilder::encryption_from_bytes_with_previous(...)+from_envwiring.cachekit-core 0.4 → 0.5(Keyring ships in 0.5.0).CACHEKIT_PREVIOUS_MASTER_KEYSenv row.Tests (all AC covered)
master=k₂, previous=[k₁]; fails withprevious=[]CachekitError::Encryptioncachekit_core::MAX_DECRYPT_ONLY_KEYSLocal gate:
cargo fmt --check,clippy -D warnings(CI feature set), full test suite incl. doc-tests — all green.Dependency bump evidence — cachekit-core 0.4 → 0.5 (supply chain)
https://api.osv.dev/v1/queryforcachekit-core0.5.0(crates.io) returns no known vulnerabilities (empty result, checked 2026-08-08).security.ymlworkflow (advisories + bans + licenses + sources,--all-features) is green on this PR head — run 31187683200.Cargo.lockresolvescachekit-core 0.5.0from crates.io (registry checksum pinned by the lockfile).0.5caret pin matches the repo's existing convention (was0.4), with the exact version pinned byCargo.lock.Out of scope (per ticket)
Fingerprint-based selection, nonce-exhaustion handling, per-tenant derivation changes, rotation runbook + feature-matrix flip (LAB-687, stage 3).
Summary by CodeRabbit
New Features
CACHEKIT_PREVIOUS_MASTER_KEYSenvironment variable configuration.Documentation