feat: expose winning-key index via decrypt_indexed for rotation drain observability (LAB-1645) - #73
Merged
Merged
Conversation
… observability (LAB-1645) During a rotation grace window an operator has no signal for when it is safe to drop a retiring master key: Keyring::decrypt and TenantKeyring::decrypt collapse the result to plaintext-or-error, discarding which entry satisfied the read, so "previous-key hit rate has reached zero" is unobservable and dropping a key risks a hard cut-over. Add decrypt_indexed to both Keyring and TenantKeyring, returning (plaintext, winning index) with 0 = current key. The sequencing loop moves into decrypt_indexed and decrypt delegates to it, so attempt semantics (current-first, identical AAD, only AuthenticationFailed advances, structural/config errors terminal, exhaustion = plain AuthenticationFailed) live in exactly one place per type and cannot drift between the two surfaces. Existing signatures unchanged; the new surface carries index only — no key material.
…45 panel) Expert-panel findings: the feature's whole point (non-zero index on a previous-key read) had no runnable example in a crate where doc-tests are the executable docs — add one asserting index == 1 for a retiring- key read. TenantKeyring::decrypt_indexed restated the drain narrative verbatim; cut to the Keyring cross-ref plus this type's genuine deltas (no HKDF, no KeyDerivation class) so duplicated prose cannot drift.
This comment has been minimized.
This comment has been minimized.
Contributor
|
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 (1)
WalkthroughThe change adds indexed decryption to ChangesKeyring decryption
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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}
Contributor
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 `@src/encryption/keyring.rs`:
- Around line 254-271: Update the Keyring doctest to import zeroize::Zeroize,
declare k1, k2, and tenant_key as mutable, and call zeroize() on each after its
final use before scope exit. Preserve the existing encryption, key rotation, and
decrypt_indexed assertions.
🪄 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: d566c4b2-9499-4d87-8b85-4c54cf1e5304
📒 Files selected for processing (1)
src/encryption/keyring.rs
…decrypt_indexed doctest Keyring::new copies the master keys, so ZeroizeOnDrop clears only the keyring's copies — the example now wipes the caller-owned buffers after their last use, modelling the full hygiene a crypto-crate example should teach. CodeRabbit-Resolved: src/encryption/keyring.rs:271:Zeroise the doctest key buffers
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
|
27Bslash6
added a commit
to cachekit-io/cachekit-rs
that referenced
this pull request
Sep 19, 2026
#74) Closes LAB-1678. Consumes the cachekit-core 0.6.0 `decrypt_indexed` surface ([cachekit-io/cachekit-core#73](cachekit-io/cachekit-core#73)) that [#63](#63) deferred: a previous-key hit is now observable, so an operator can watch a rotation drain reach zero before dropping the retired master key. ## What changed - `crates/cachekit/Cargo.toml`: `cachekit-core` pin `0.5` → `0.6` (features unchanged; 0.6.0 is additive). - `EncryptionLayer::decrypt` reads via `Keyring::decrypt_indexed`. Plaintext, attempt sequencing (core-owned, LAB-683) and the error-class mapping (`KeyDerivation`/`KeyringIndexOutOfRange` → `Config`, else `Encryption`) are unchanged; existing rotation tests pass unmodified. - Drain signal: one `AtomicU64` per previous key on the layer. Index 0 (current key) is not counted; index `i ≥ 1` increments slot `i - 1`. `EncryptionLayer::previous_key_hits() -> Vec<u64>` (`hits[i]` ↔ `previous_keys[i]`) plus a `SecureCache::previous_key_hits()` passthrough so builder-configured clients can reach it. Pull-style, matching the crate's `MetricsProvider` pattern; no new dependency, no user code on the decrypt hot path. The payload is positions and counts only, no key material, by type. - Tests: unit tests for the index-0 silent case, the index-N counted-at-position case (`current=k3, previous=[k2, k1]`), and a failed decrypt counting nothing; an integration test reading the signal off `SecureCache`; a doc-test on `previous_key_hits` asserting the drain signal. - Docs: README `### Key Rotation` gains the operator workflow paragraph; `EncryptionLayer` rustdoc gains a "Rotation drain signal" section mirroring core's `decrypt_indexed` language. ## Verification - `cargo clippy --all-targets --features "cachekitio,redis,encryption,l1,macros,memcached,file" -- -D warnings`: clean. - `cargo test` with the same feature set: all suites green (92 lib tests, doc-tests included). - `cargo +1.85 check --all-targets --features "cachekitio,redis,encryption,l1,macros"`: passes (MSRV). - `cargo check --target wasm32-unknown-unknown --no-default-features --features workers,cachekitio,encryption`: builds; the 33 unused-import warnings are pre-existing in backend modules. - `prek run --all-files`: every hook passes except detect-secrets, which flags `tests/vectors/interop-mode.json` on a clean `main` too (pre-existing, not touched here). ## Dependency bump evidence (cachekit-core 0.5 → 0.6) - `Security` workflow (`cargo deny check` advisories/bans/licenses/sources with `--all-features` + `cargo audit` on `Cargo.lock`) passed on this branch: [run 33617757729](https://github.com/cachekit-io/cachekit-rs/actions/runs/33617757729). - OSV query for `cachekit-core@0.6.0` (crates.io) returns no advisories (2026-09-03). - `Cargo.lock`: sole change against `main` is `cachekit-core 0.5.0 → 0.6.0`, checksum `93adc5646956ba8da140f4179a02e60a2cc7a401b83ebb1270cc4d03748e1fac` (matches the published `.crate`); no new transitive packages. (`main` already carries the `async-trait` 0.1.92 bump that clears the beta `double_must_use` canary, so this PR no longer carries it — the branch is merged up to `main` @ `92637cc`.) - The 0.5→0.6 source diff is additive (verified independently by the panel's security-specialist and by the adversarial reviewer's packaged-crate comparison). ## Adversarial review (Helly R) — applied at `067ad65` The "safe to drop" guidance omitted the runbook's write-fence and longest-TTL preconditions. README `### Key Rotation` and the `previous_key_hits` rustdoc now state: complete the two-phase promotion first, start the clock only when that deploy completes fleet-wide (a lagging instance still writes under the retiring key and reads it silently as index 0), wait at least the longest TTL in use including explicit `set_with_ttl` values, aggregate across instances, and only then read a flat counter as drained. Positions are comparable across instances only once they share one keyring configuration. Out of scope, deliberately: switching the layer to core's new `TenantKeyring` (LAB-1638 follow-up), py/ts exposure (neither has multi-key decrypt), the docs.cachekit.io runbook page (LAB-687). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added visibility into reads still using previous encryption keys, helping teams monitor key-rotation progress. - Previous-key usage is tracked separately for each rotation key and excludes reads using the current key. - Failed authentication attempts do not increase previous-key usage counts. - **Documentation** - Expanded key-rotation guidance to explain how to interpret usage counts, account for multiple running instances, wait through a complete TTL period, and safely remove old keys. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- kody-pr-summary:start --> # Summary Despite the PR title referencing a rotation drain signal via `decrypt_indexed` (LAB-1678), the actual code changes in this PR are limited to documentation updates in `README.md`. ## Changes The PR adds documentation to the security features section of the README describing the **cache-key path encoding protection (CWE-22)**: - A new row is added to the security features table explaining that cache keys are percent-encoded into the CachekitIO request path, and that any key encoding to a reserved segment (`.`, `..`, `health`, `ttl`, `lock`) is rejected rather than sent. - A detailed explanatory paragraph is added covering: - Keys are percent-encoded via `urlencoding::encode` so a key can only address `/v1/cache/{key}`. - Five reserved path segments are rejected with a permanent error to prevent path traversal / route hijacking. - Why encoding cannot neutralize the risk (WHATWG URL parser in `reqwest` strips dot segments before the request leaves the process, and `health`/`ttl`/`lock` are live route tokens). - Consistency notes comparing this behavior against the `cachekit-ts` twin and the stricter-than `cachekit-py` handling, and confirming no legitimate CacheKit keys are affected. ## Note Based solely on the provided diff, this PR contains only documentation additions. No functional/source code changes (e.g., to `decrypt_indexed` or a rotation drain signal) are present in the changes shown. --- Based on the code changes provided, here's a description for this pull request: ## Description Despite the PR title referencing rotation drain signals via `decrypt_indexed`, the actual code changes in this PR are **documentation and packaging updates**. The changes fall into three areas: ### 1. Version Bump in README (`0.5` → `0.7`) All dependency examples throughout the README have been updated from `cachekit-rs = "0.5"` to `cachekit-rs = "0.7"`, covering the default, Redis, Memcached, File, and Cloudflare Workers configurations. ### 2. New "Intent Presets" Documentation A new **Intent Presets (recommended)** section was added to the Quick Start guide, documenting four one-call preset builders: - `CacheKit::minimal(url)` — development/public data, speed-first, no extras - `CacheKit::production(url)` — user sessions and production services with L1, reliability, and auto-reconnect - `CacheKit::encrypted(url, key)` — PII/payments/GDPR-HIPAA data with AES-256-GCM encryption - `CacheKit::io(api_key)` — serverless/edge compute via cachekit.io without running Redis The section includes a comparison table (backend, L1, encryption, reliability, auto-reconnect, default TTL), a runnable code example, and a detailed **Resilience contract** explaining connection-failure behavior at construction and mid-run for each preset (auto-reconnect vs. fail-fast semantics, initial connection handling, and master-key validation ordering). The Overview intro was also rewritten to highlight the intent-preset approach. ### 3. docs.rs Build Configuration (`Cargo.toml`) Added a `[package.metadata.docs.rs]` section that enables the `cachekitio`, `redis`, `encryption`, `l1`, `reliability`, `macros`, `memcached`, and `file` features when building on docs.rs. This ensures the Redis intent presets and optional backends appear in the rendered documentation (docs.rs otherwise builds with default features only). The `workers` feature is intentionally excluded due to being mutually exclusive with the other features. --- **Note:** The code changes shown do not include any modifications to `decrypt_indexed` or rotation drain signal logic referenced in the PR title. The provided patches only touch `README.md` and `crates/cachekit/Cargo.toml`. If the rotation drain functionality is part of this PR, those source changes were not included in the diff provided for analysis. <!-- kody-pr-summary:end -->
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.
Closes LAB-1645.
Problem
During a rotation grace window an operator has no signal for when it is safe to drop a retiring master key:
Keyring::decryptandTenantKeyring::decryptcollapse the result to plaintext-or-error, discarding which entry satisfied the read. "Previous-key hit rate has reached zero" is unobservable, so dropping a key is guesswork — and guessing wrong is a hard cut-over (every pre-rotation entry becomes an error). Raised by CodeRabbit on cachekit-io/cachekit-rs#63 and deferred to core because SDKs must not re-implement keyring attempt logic (LAB-683 decision).Change
Additive
decrypt_indexedon bothKeyringandTenantKeyring, returning(Vec<u8>, usize)— the plaintext plus the winning keyring entry index (0 = current key, 1.. = decrypt-only keys in list order). SDKs count non-zero-index reads; when that rate reaches zero the retiring key is drained and safe to drop.The sequencing loop moved into
decrypt_indexedand the existingdecrypton each type became a one-line delegate, so attempt semantics (current-first, identical AAD, onlyAuthenticationFailedadvances, structural/config errors terminal, exhaustion = plainAuthenticationFailed) live in exactly one place per type and cannot drift between the two surfaces. Existing signatures unchanged — plain additivefeat:(0.x minor). The new surface carries the index only: no key material, no fingerprint; ZeroizeOnDrop discipline untouched.Tests
test_decrypt_indexed_reports_winning_entry— index 0 on a current-key hit, 1 on a previous-key hit, plaintext identical todecrypt.test_decrypt_indexed_exhaustion_and_terminal_errors_match_decrypt— exhaustion stays plainAuthenticationFailed;InvalidCiphertext/KeyDerivationstay terminal (LAB-683 no-collapse rule).test_tenant_keyring_decrypt_indexed_matches_unbound— same contract on the tenant-bound (SDK steady-state) path.Keyring::decrypt_indexedasserting the drain signal itself (index == 1for a retiring-key read).cargo fmt --check,cargo clippy --all-targets --all-features -D warnings,cargo test --all-features(99 unit + 4 doc-tests + integration targets) all green locally.Review gate
Expert panel (critical-stakes, crypto surface) run pre-PR: bug-hunter, security-specialist, and catchphrase returned no findings; code-craftsman's two findings (runnable doc-test for the drain signal; dedupe the
TenantKeyringrustdoc to a cross-ref plus genuine deltas) are applied in the second commit.Docs
Rustdoc on the new surface states the operator workflow (previous-key hit rate → zero ⇒ safe to drop) with an executable example. No other doc surfaces change: SDK exposure is explicitly out of scope (follow-up children per SDK), no wire/spec behaviour changes, README mentions the keyring only at architecture-diagram level, CHANGELOG is release-please-managed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests