Skip to content

feat(encryption): keyring rotation — previous_master_keys + fingerprint selection (LAB-684) - #261

Merged
27Bslash6 merged 7 commits into
mainfrom
lab-684-keyring-rotation-py
Aug 8, 2026
Merged

feat(encryption): keyring rotation — previous_master_keys + fingerprint selection (LAB-684)#261
27Bslash6 merged 7 commits into
mainfrom
lab-684-keyring-rotation-py

Conversation

@27Bslash6

@27Bslash6 27Bslash6 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes LAB-684 (LAB-516 stage 2). Design: protocol decisions/key-rotation.md + spec/encryption.md → "Key Rotation (Keyring)" (protocol #34). Builds on the cachekit-core 0.5.0 Keyring helper (LAB-683, cachekit-core #67).

What

An operator can now rotate the master key without invalidating every encrypted entry: up to 3 decrypt-only previous master keys via CACHEKIT_PREVIOUS_MASTER_KEYS (comma-separated hex), with per-entry selection by exact derived-key fingerprint match — cachekit-py stores a per-entry key_fingerprint in CK frame metadata, so per spec L368 it must never trial-decrypt across the keyring.

  • Config (config/settings.py): previous_master_keys: list[SecretStr]. Load-time rejection (never truncation) of: >3 entries; non-hex / <32-byte keys (per-key validation identical to master_key); master_key re-appearing in the list (detectable subset of the spec's forward-only invariant — re-promotion resumes a used AES-GCM nonce budget). Redacted in repr/str/get_safe_repr.
  • Decrypt selection (serializers/encryption_wrapper.py): the frame's key_fingerprint is matched against each keyring entry's HKDF-derived per-tenant encryption-key fingerprint (never the master-key fingerprint), current key first. A match is binding: the matched entry is the only key used; its AES-GCM failure is terminal (straight to fail_closed/fail_open, no further entries). No match → pre-keyring mismatch semantics unchanged — all existing fingerprint-mismatch tests pass unmodified. Current-key reads keep the cached derived tenant keys (no per-read HKDF on the hot path).
  • FFI (rust/src/python_bindings.rs): new Keyring binding (constructor, encryption_fingerprints, decrypt_at). Key material enters once at config ingestion and never crosses back into Python; keyring material zeroizes on drop in Rust (cachekit-core ZeroizeOnDrop).
  • Deleted: the dead PyKeyRotationState binding at all three sites (pyclass block, import, add_class registration) — the LAB-275 trust bug, zero Python callers since inception. The 0.5.0 bump makes the removal compiler-enforced (key_rotation no longer exists in core). A test asserts KeyRotationState is no longer importable.
  • Docs: README, docs/configuration.md, and docs/features/zero-knowledge-encryption.md rotation sections rewritten to the real surface — including deleting the never-implemented CACHEKIT_MASTER_KEY_ROTATION env var the docs had invented. New doctests (executed in CI) cover the rotation round-trip and forward-only rejection; no notest/+SKIP added anywhere.

Tests

27 new tests in tests/unit/test_key_rotation_keyring.py: env parsing, cap/subset/per-key load rejection, redaction; spy-instrumented proofs that selection touches exactly one keyring entry (no trial decryption), that a binding match is terminal (remaining entries untried, no retreat to the current key), and that fail-open no-match attempts the current key only; end-to-end rotation round-trip through CacheSerializationHandler and the @cache decorator (k₁ write → k₂+[k₁] read without re-encryption or recompute → k₁ dropped → fail policy honored); FFI-hygiene assertions.

Local gates: ruff check/format clean, basedpyright 0 errors, cargo fmt/clippy/test clean, full doctest gate green, CI test selection green (2411 passed; only the pre-existing live-SaaS e2e suite fails without an API key, identical on main).

Out of scope (LAB-687, stage 3)

Rotation runbook docs page, protocol feature-matrix flip, conformance vectors. Sibling PRs: LAB-685 (ts), LAB-686 (rs).

Supply-chain evidence (dependency changes)

  • cachekit-core 0.4.0 → 0.5.0 (rust/Cargo.toml): first-party crate, published by this org from cachekit-core#67 + cachekit-core#68 (release cachekit-core-v0.5.0). OSV query for cachekit-core 0.5.0 (crates.io) returns zero known vulnerabilities (api.osv.dev/v1/query, 2026-08-08). Cargo.lock is updated in this diff and pins the exact version with its crates.io registry checksum (12089baa…); CI's Vulnerability Scan and License & Supply Chain checks are green on this PR.
  • h2 constrained to ≥4.4.1 (pyproject.toml [tool.uv] constraint-dependencies + uv.lock): fixes GHSA-6hr6-w5qg-qmwg (duplicate-Host request smuggling primitive) flagged by the PR's pip-audit gate; advisory published after main's last Security Fast pass, so main is equally affected on its next run. Local pip-audit: no known vulnerabilities. OSV: h2 4.4.1 (PyPI) has zero known advisories.

Summary by CodeRabbit

  • New Features

    • Added zero-downtime encryption key rotation with up to three decrypt-only previous keys.
    • Existing encrypted entries remain readable during the rotation window, while new entries use the current key.
    • Added fingerprint-based key selection and interoperability for entries without fingerprints.
    • Added secure key validation and masked sensitive values in configuration output and errors.
  • Documentation

    • Expanded guidance on configuration, rotation procedures, retention windows, validation, and fail-closed behaviour.

…ingerprint selection (LAB-684)

Implements protocol spec/encryption.md → 'Key Rotation (Keyring)' for
cachekit-py, building on the cachekit-core 0.5.0 Keyring helper (LAB-683):

- Config: CachekitConfig.previous_master_keys (list[SecretStr], env
  CACHEKIT_PREVIOUS_MASTER_KEYS, comma-separated hex). Load-time validation:
  cap of 3 (rejected, never truncated), per-key requirements identical to
  master_key (hex, >=32 bytes), and the forward-only subset check — the
  current master_key re-appearing in the decrypt-only list is rejected
  (re-promotion would resume a used AES-GCM nonce budget). Redacted in
  repr/str/get_safe_repr like master_key.
- EncryptionWrapper decrypt: fingerprint-based keyring selection. The frame's
  key_fingerprint is matched against each entry's HKDF-derived per-tenant
  encryption-key fingerprint (never the master-key fingerprint), current key
  first. A match is binding: the matched entry is the only key used and its
  authentication failure is terminal. No match preserves the pre-keyring
  fail_closed/fail-open semantics byte-for-byte; the current-key hot path
  keeps the cached derived tenant keys (no per-read HKDF).
- FFI: new Keyring binding (construction, per-tenant fingerprints,
  decrypt_at). Master-key material enters once at config ingestion and
  never returns to Python; keyring material zeroizes on drop in Rust.
- Deletes the dead KeyRotationState PyO3 binding at all three sites
  (LAB-275 trust bug — zero Python callers since inception); the
  cachekit-core 0.5.0 bump makes the removal compiler-enforced.
- Docs: README, docs/configuration.md, zero-knowledge-encryption.md rotation
  sections rewritten to the real keyring surface — including removal of the
  never-implemented CACHEKIT_MASTER_KEY_ROTATION env var the docs invented.
  Doctests cover the rotation round-trip and the forward-only rejection.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 614216a1-a79e-4609-98fd-d1c8fc44e98b

📥 Commits

Reviewing files that changed from the base of the PR and between bc7a91f and d4de35c.

📒 Files selected for processing (1)
  • src/cachekit/decorators/wrapper.py

Walkthrough

The change adds decrypt-only previous master keys, keyring-based fingerprint selection, sequential interop decryption, validation and redaction controls, updated Rust bindings, rotation documentation, and dependency constraints.

Changes

Encryption key rotation

Layer / File(s) Summary
Previous-key configuration and validation
src/cachekit/config/settings.py, pyproject.toml, docs/configuration.md, tests/unit/test_key_rotation_keyring.py
Settings parse and validate up to three previous keys. Validation errors and representations redact key material.
Rust keyring bindings
rust/Cargo.toml, rust/src/lib.rs, rust/src/python_bindings.rs, tests/unit/test_key_rotation_keyring.py
PyKeyring replaces PyKeyRotationState and exposes indexed and sequential decryption without exposing key material.
Fingerprint and interop decryption
src/cachekit/serializers/encryption_wrapper.py, src/cachekit/cache_handler.py, src/cachekit/serializers/__init__.py, tests/unit/test_key_rotation_keyring.py
EncryptionWrapper selects keys by tenant fingerprint and supports sequential decryption for entries without key identity. Cache handlers re-raise keyring configuration errors.
Rotation procedure and failure behaviour
README.md, docs/features/zero-knowledge-encryption.md, .secrets.baseline
Documentation describes key promotion, decrypt-only retention, TTL expiry, hard cut-over, interop entries, and fail-closed errors. The secrets baseline tracks the updated source location and timestamp.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CacheHandler
  participant EncryptionWrapper
  participant PyKeyring
  participant ZeroKnowledgeEncryptor
  CacheHandler->>EncryptionWrapper: Read encrypted entry
  EncryptionWrapper->>PyKeyring: Select fingerprint or try keys sequentially
  PyKeyring->>ZeroKnowledgeEncryptor: Decrypt ciphertext
  ZeroKnowledgeEncryptor-->>EncryptionWrapper: Return authenticated plaintext
  EncryptionWrapper-->>CacheHandler: Return deserialised value
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: keyring-based encryption-key rotation with previous keys and fingerprint selection.
Description check ✅ Passed The description covers the change, motivation, testing, dependencies, documentation, security considerations, and out-of-scope work, but omits the template headings and checkboxes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lab-684-keyring-rotation-py

Comment @coderabbitai help to get the list of available commands.

@kodus-27b

This comment has been minimized.

Comment thread docs/configuration.md
Comment thread rust/Cargo.toml
Comment thread src/cachekit/config/settings.py
Comment thread tests/unit/test_key_rotation_keyring.py

@kodus-27b kodus-27b Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found critical issues please review the requested changes

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.18584% with 19 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/cachekit/serializers/encryption_wrapper.py 79.54% 6 Missing and 3 partials ⚠️
src/cachekit/decorators/wrapper.py 16.66% 5 Missing ⚠️
src/cachekit/cache_handler.py 70.00% 3 Missing ⚠️
src/cachekit/config/settings.py 96.15% 0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Panel ran at critical stakes (bug-hunter-supreme, security-specialist,
code-craftsman, catchphrase-agent). Surviving findings, all applied:

- CRIT (error taxonomy): keyring config violations now raise ValueError,
  never EncryptionError — EncryptionError is a SerializationError, which
  handle_decrypt_failure classifies as corruption and fails OPEN even under
  fail_closed=True; a misconfigured keyring (e.g. programmatic master_key
  colliding with env previous keys) would have masked itself as 100% misses
  while evicting readable entries (the LAB-241/LAB-683 config-vs-crypto
  class). ValueError takes the established fail-loud path, matching the
  settings-load ValidationError.
- CRIT (interop rotation): interop entries carry no per-entry key
  fingerprint, so fingerprint selection could never use previous keys there
  — rotation would still have invalidated every interop entry while the docs
  claimed zero-downtime. Implements the spec's 'Decrypt — without per-entry
  key identity' row: sequential keyring attempts (current first, identical
  AAD, exhaustion = plain auth failure into the existing policy) via a new
  Keyring.decrypt binding + EncryptionWrapper.deserialize_without_key_identity;
  the single-key interop hot path keeps the cached tenant keys.
- MAJ (CWE-532): CachekitConfig sanitizes ValidationError — raw inputs
  (env-sourced master_key/previous_master_keys hex) no longer appear in
  str(e), .errors(), .json(), or the exception chain (hide_input_in_errors
  covers only __str__; __init__ rebuilds the error with inputs redacted and
  raises outside the except block so __context__ stays None).
- MAJ (packaging): pydantic-settings floor 2.0.0 → 2.6.0 (NoDecode).
- MIN: setup-time drift guard — keyring fingerprint[0] must equal the
  cached tenant-keys fingerprint, failing loud at construction instead of
  silently routing every read to the no-match path on core skew.
- MIN: previous_master_keys docstring states the env-merge provenance
  (explicit master_key still combines with env previous keys by design).
- Cut: dead fingerprint delegate on the test keyring spy.
@kodus-27b

This comment has been minimized.

@27Bslash6

Copy link
Copy Markdown
Contributor Author

Crypto expert-panel review — recorded per the 2026-07-17 project gate

Panel ran at critical stakes on cb33221: bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent (parallel), findings cross-filtered for pragmatism. Verdict: FIX-FIRST → all surviving findings applied in d4fb28b.

Surviving findings → disposition

Sev Finding Disposition
CRIT Keyring config errors raised EncryptionError (a SerializationError) → read path classifies as corruption → fails open even under fail_closed=True, masking a misconfigured keyring as 100% misses + entry-by-entry eviction (LAB-241/LAB-683 class) Fixed: config violations raise ValueError (fail-loud allowlist), never SerializationError; regression test asserts the class split
CRIT Interop entries have no per-entry fingerprint → previous keys were never consulted in interop mode; docs claimed zero-downtime rotation regardless Fixed: implemented the spec's "Decrypt — without per-entry key identity" row — sequential keyring attempts via new Keyring.decrypt binding + deserialize_without_key_identity; single-key hot path unchanged; docs caveat added; 3 new tests
MAJ CWE-532: pydantic ValidationError embeds raw env-sourced key hex in errors()/json()/exception chain Fixed: hide_input_in_errors + constructor-level error sanitization (inputs redacted, chain severed); leak test covers str/errors/json/__context__ on all three reject paths
MAJ pydantic-settings>=2.0.0 floor breaks on NoDecode import (needs ≥2.6) Fixed: floor bumped to 2.6.0
MIN Current-key fingerprint derived via two independent FFI paths — core skew would silently route all reads to no-match Fixed: setup-time drift guard fails loud at construction
MIN Env-merge provenance (explicit master_key + env previous keys) undocumented Fixed: docstring states it explicitly, [] opt-out documented

Rejected findings (with reason)

  • code-craftsman: "delete the wrapper's per-key ≥32-byte loop (third validation layer)" — rejected, factually refuted by the panel's own pragmatism pass: Rust Keyring::new enforces only ≥16 bytes; the loop is the sole ≥32 enforcement for wrappers constructed with explicit parameters (settings validation is bypassed there), and the spec requires per-key validation identical to master_key.

Cleared explicitly (no action)

Binding fingerprint-match with no cross-key fallback; forward-only guard at both layers; no nonce-reuse angle (writes always use current key); FFI exposes only fingerprints + plaintext; attacker-flipped key_fingerprint can only downgrade a readable entry to a decrypt failure (a capability a stored-bytes attacker already has); no-match semantics byte-identical to pre-keyring behaviour (127 pre-existing encryption tests pass unmodified).

Local gates after fixes: ruff + basedpyright clean, cargo fmt/clippy/test clean, doctest gate green (115), markdown-docs green (121), CI selection 2416 passed.

Comment thread src/cachekit/config/settings.py
Comment thread src/cachekit/serializers/encryption_wrapper.py
Comment thread tests/unit/test_key_rotation_keyring.py

@kodus-27b kodus-27b Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found critical issues please review the requested changes

pip-audit (Security Fast) went red on the PR: h2 4.3.0, a transitive dep
via httpx[http2], accepts duplicate Host headers and forwards both on
HTTP/2 -> HTTP/1.1 downgrade — a request smuggling primitive
(GHSA-6hr6-w5qg-qmwg, fixed in 4.4.1). Advisory published after main's
last Security Fast pass (2026-08-05), so main is equally affected on its
next run; this PR just hit it first.

Same [tool.uv] constraint-dependencies mechanism as the existing
urllib3/fonttools/werkzeug/pip pins. Verified: uv lock resolves h2 4.4.1,
local pip-audit reports no known vulnerabilities.
@27Bslash6

Copy link
Copy Markdown
Contributor Author

@kody start-review

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/features/zero-knowledge-encryption.md`:
- Around line 462-468: Update the key rotation troubleshooting answer in the
zero-knowledge encryption documentation to replace CACHEKIT_MASTER_KEY_ROTATION
with CACHEKIT_PREVIOUS_MASTER_KEYS, and direct readers to follow the keyring
rotation pattern above for retaining the retiring key as decrypt-only.

In `@pyproject.toml`:
- Line 59: Update the pydantic-settings dependency constraint in the project
configuration from >=2.6.0 to >=2.7.0 so NoDecode imports remain available;
leave the existing h2 constraint unchanged.

In `@rust/src/python_bindings.rs`:
- Around line 323-345: Separate authentication failures from configuration or
structural failures in rust/src/python_bindings.rs#L323-L345 by updating decrypt
and decrypt_at to match cachekit-core error kinds, preserving the current
PyValueError for authentication failures and raising a distinct Python exception
for configuration/structural errors. In
src/cachekit/serializers/encryption_wrapper.py#L603-L615, narrow the broad
exception handling to convert only the authentication-failure type into
DecryptionAuthenticationError; allow the distinct configuration/structural
exception to propagate as ValueError.
- Around line 258-283: Update PyKeyring::new to wrap each decrypt_only key in
zeroize::Zeroizing before constructing refs, ensuring the temporary Python-side
allocations are cleared when dropped while preserving the existing Keyring::new
call. Expand the constructor documentation to state the cachekit-core minimum
key length is 16 bytes, and retain the existing Python-side 32-byte validation.

In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 245-264: Move the
self._keyring.encryption_fingerprints(self.tenant_id) call out of the try block
in the encryption initialization flow, while leaving tenant-key derivation and
related EncryptionError handling unchanged. Update
test_keyring_config_errors_are_not_serialization_errors to simulate
fingerprint-derivation failure and assert the original ValueError propagates
unchanged rather than being wrapped as EncryptionError.

In `@tests/unit/test_key_rotation_keyring.py`:
- Around line 348-356: Pin CACHEKIT_DEPLOYMENT_UUID explicitly in both rotation
round-trip tests, including test_rotation_round_trip_then_drop and the test at
the additional referenced location. Set it with monkeypatch before constructing
or resetting the CacheSerializationHandler, matching the explicit setup used by
TestInteropRotation._handler, without relying on Path.home() or passing
deployment_uuid.
🪄 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: 2d765649-2e80-4086-bfb7-10bec61bac18

📥 Commits

Reviewing files that changed from the base of the PR and between 2607faf and 39bdd22.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • README.md
  • docs/configuration.md
  • docs/features/zero-knowledge-encryption.md
  • pyproject.toml
  • rust/Cargo.toml
  • rust/src/lib.rs
  • rust/src/python_bindings.rs
  • src/cachekit/cache_handler.py
  • src/cachekit/config/settings.py
  • src/cachekit/serializers/encryption_wrapper.py
  • tests/unit/test_key_rotation_keyring.py

Comment thread docs/features/zero-knowledge-encryption.md
Comment thread pyproject.toml Outdated
Comment thread rust/src/python_bindings.rs
Comment thread rust/src/python_bindings.rs
Comment thread src/cachekit/serializers/encryption_wrapper.py
Comment thread tests/unit/test_key_rotation_keyring.py
…-684)

The keyring decrypt path had no way to distinguish an AES-GCM authentication
failure from a configuration or structural one. The binding collapsed every
cachekit-core error into PyValueError and the wrapper's broad except converted
anything it caught into DecryptionAuthenticationError, which
handle_decrypt_failure records as auth_tamper — the event the docs tell
operators to alert on as a security incident. A keyring misconfiguration
therefore raised a false intrusion alert, and under fail_closed raised to the
caller and retained the entry.

cachekit-core already keeps the distinction: AuthenticationFailed is the sole
tag-verification signal, and KeyringIndexOutOfRange / KeyDerivation /
InvalidCiphertext / UnsupportedVersion are caller or operator faults. The
binding now maps only the former to the plain ValueError the wrapper treats as
tamper, and everything else to KeyringConfigurationError (a ValueError subclass,
so it still takes the fail-loud path).

Both wrapper decrypt sites are narrowed, not just the sequential one CodeRabbit
flagged: decrypt_at shares the binding and therefore shared the defect, and it
is the path a rotation-window read actually takes.

Also in this round:
- Keyring fingerprint derivation moved out of the try block whose handler raises
  EncryptionError. That handler relabelled a keyring config ValueError as a
  SerializationError, which the read policy treats as corruption and fails open
  — silent misses plus entry-by-entry eviction, the LAB-241/LAB-683 class this
  PR exists to remove.
- Zeroize the PyO3-side decrypt_only key vectors; Keyring::new copies them and
  the originals were freed with key material still in the heap pages.
- pydantic-settings floor 2.6.0 -> 2.7.0. NoDecode does not exist in 2.6.x and
  settings.py imports it at module scope, so the old floor could resolve to a
  hard ImportError. Verified against the 2.6.0/2.6.1/2.7.0 wheels.
- Pin CACHEKIT_DEPLOYMENT_UUID in TestEndToEndRotation: unset, the handler
  creates ~/.cachekit/deployment_uuid, so the suite wrote to the runner's home
  and derived keys from filesystem state outside the test.
- Troubleshooting doc referenced CACHEKIT_MASTER_KEY_ROTATION, which does not
  exist; the variable is CACHEKIT_PREVIOUS_MASTER_KEYS.
@kodus-27b

This comment has been minimized.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/cachekit/serializers/encryption_wrapper.py (1)

584-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document KeyringConfigurationError in the Raises section.

The method deliberately re-raises KeyringConfigurationError at Line 638. The Raises section lists ValueError only for an empty cache_key. KeyringConfigurationError subclasses ValueError, so a caller who reads this contract can write a broad except ValueError for the empty-key case and then swallow a keyring configuration fault. State the config class explicitly, as the same taxonomy note already does in the code comment.

The deserialize docstring at Lines 397-404 has the same gap.

📝 Proposed docstring change
         Raises:
             TypeError: If cache_key is not a string
             ValueError: If cache_key is empty
+            KeyringConfigurationError: On a keyring configuration or
+                ciphertext-structure failure (a ValueError subclass, raised
+                unconverted so it takes the fail-loud path)
             DecryptionAuthenticationError: When no keyring entry authenticates
                 the ciphertext
             EncryptionError: If deserialization fails after authenticated
                 decryption
🤖 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 `@src/cachekit/serializers/encryption_wrapper.py` around lines 584 - 590,
Update the Raises sections of both the referenced deserialization method and
deserialize to explicitly document KeyringConfigurationError separately from the
empty-cache-key ValueError, matching the existing taxonomy comment and
preserving the current exception 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.

Outside diff comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 584-590: Update the Raises sections of both the referenced
deserialization method and deserialize to explicitly document
KeyringConfigurationError separately from the empty-cache-key ValueError,
matching the existing taxonomy comment and preserving the current exception
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7f5cf064-a258-4f28-a3b5-24f546017523

📥 Commits

Reviewing files that changed from the base of the PR and between 39bdd22 and b46513c.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • docs/features/zero-knowledge-encryption.md
  • pyproject.toml
  • rust/Cargo.toml
  • rust/src/python_bindings.rs
  • src/cachekit/serializers/encryption_wrapper.py
  • tests/unit/test_key_rotation_keyring.py

coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 7, 2026
Comment thread src/cachekit/serializers/encryption_wrapper.py
Comment thread src/cachekit/serializers/encryption_wrapper.py
Comment thread src/cachekit/serializers/encryption_wrapper.py
Comment thread tests/unit/test_key_rotation_keyring.py

@kodus-27b kodus-27b Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found critical issues please review the requested changes

…ad-path swallow (LAB-684)

Expert panel (critical stakes, crypto gate) returned two CRITs against the
previous commit. Both were real; both are fixed here.

CRIT 1 -- the catch-all arm made the tamper alarm attacker-controlled. Mapping
"everything except AuthenticationFailed" to KeyringConfigurationError swept in
InvalidCiphertext, which decrypt_aes_gcm returns on a length check that runs
BEFORE the AES-GCM tag check. An attacker with backend write access -- the exact
threat model this feature exists for -- could truncate a stored entry to under 28
bytes and have the tamper reclassified as a local config fault, choosing whether
the alarm fires. The split is now by input provenance: only KeyDerivation and
KeyringIndexOutOfRange (inputs we control) are config; everything driven by the
stored ciphertext stays tamper-class. This also makes cachekit-py match
cachekit-rs, which already mapped exactly those two variants to its Config class,
and it dissolves the divergence where decrypt_with_keys and decrypt_at
classified identical bytes differently.

CRIT 2 -- "subclasses ValueError so it takes the fail-loud path" was false one
frame up. The L2 read path dispatches on except SerializationError, and
KeyringConfigurationError is a ValueError, so it fell through to the broad
except Exception and became a bare return None -- a silent fail-open miss with no
metric, no eviction, and no raise even under fail_closed=True. That is the
LAB-241/LAB-683 class this PR exists to remove, reintroduced through a new door.
All four L2 read sites now re-raise it explicitly. Not routed through
handle_decrypt_failure: there is no fail-open/fail-closed decision to make for a
misconfigured keyring, it always raises.

Verified by reverting cache_handler.py alone -- the new regression test fails
with "DID NOT RAISE" and the swallow shows up in the log as "Backend operation
failed for get".

Also from the panel:
- Re-export KeyringConfigurationError from cachekit.serializers. It can escape
  into application code, so it needs a public name to catch; it was reachable
  only via the private cachekit._rust_serializer.
- Set __module__ to cachekit._rust_serializer. create_exception! leaves the bare
  module name, so a ProcessPoolExecutor worker raising it would surface
  ModuleNotFoundError to the parent instead of the real failure.
- zeroize is now an optional dep behind the encryption feature, matching pyo3.
  Both feature combinations verified with cargo check.
- Corrected the test that asserted short-ciphertext was config-class; it would
  have pinned CRIT 1 as intended behaviour. Dropped the unreachable decrypt()
  stub on the test fake -- deserialize only reaches decrypt_at.
@kodus-27b

This comment has been minimized.

@27Bslash6

Copy link
Copy Markdown
Contributor Author

Review round addressed — 6/6 CodeRabbit items, with one deliberate deviation

Commits b46513c + bc7a91f.

Applied as written (5)

Finding Resolution
pyproject.toml:59 — pydantic-settings floor Raised to >=2.7.0. Verified against the 2.6.0 / 2.6.1 / 2.7.0 wheels: NoDecode does not appear in any module before 2.7.0, and settings.py:30 imports it at module scope — the old floor could resolve to a hard ImportError. Confirmed Major.
encryption_wrapper.py:264 — fingerprint derivation inside the EncryptionError handler Moved outside the try. Added the regression test you asked for (test_fingerprint_derivation_failure_is_not_a_serialization_error).
python_bindings.rs:283 — zeroise extracted decrypt-only keys, document the 16-byte core limit Both done. Doc comment now states the core floor is 16 bytes while cachekit-py enforces 32, and why the stricter one is the product contract.
test_key_rotation_keyring.py:356 — pin CACHEKIT_DEPLOYMENT_UUID Done as an autouse fixture on TestEndToEndRotation so it covers both tests and any future one, rather than two copies.
zero-knowledge-encryption.md:468CACHEKIT_MASTER_KEY_ROTATION does not exist Replaced with CACHEKIT_PREVIOUS_MASTER_KEYS plus the actual constraints.

Deviation — python_bindings.rs:345, the error-taxonomy split

Your diagnosis is right and I implemented the fix. I did not implement it with the boundary you specified, and the difference is security-relevant, so it needs stating explicitly.

You asked to "map configuration and structural failures to a distinct Python exception type." I mapped only configuration failures, and deliberately left structural ones on the tamper path.

Reason: decrypt_aes_gcm rejects a ciphertext shorter than nonce(12) + tag(16) on a length check that runs before the AES-GCM tag check (cachekit-core-0.5.0/src/encryption/core.rs:436). InvalidCiphertext is therefore driven by the stored bytes, which under this feature's own threat model are attacker-writable. Classifying it as configuration would let an attacker truncate an entry and thereby choose whether the tamper alarm fires — a strictly worse bug than the false-alarm one being fixed.

The split is by input provenance, not by "is it AuthenticationFailed":

  • Config (KeyringConfigurationError): KeyDerivation, KeyringIndexOutOfRange — inputs we control.
  • Tamper (plain ValueErrorDecryptionAuthenticationErrorauth_tamper): AuthenticationFailed, InvalidCiphertext, InvalidNonceLength, InvalidKeyLength, everything else — inputs the store controls.

This also makes cachekit-py match cachekit-rs, which already maps exactly KeyDerivation | KeyringIndexOutOfRange to its Config class — so the three SDKs now tell operators the same story.

I also narrowed both wrapper decrypt sites, not just the sequential path you flagged. decrypt_at shares the binding and therefore shared the defect, and it is the path a rotation-window read actually takes.

One thing neither of us caught

KeyringConfigurationError subclasses ValueError, and the L2 read path dispatches on except SerializationError (cache_handler.py:1368) — so the new exception fell through to the broad except Exception and became return None: a silent fail-open miss, no metric, no eviction, no raise even under fail_closed=True. That is the LAB-241/LAB-683 class this PR exists to remove, re-entering through a new door.

All four L2 read sites now re-raise it explicitly. Proven by reverting cache_handler.py alone — the new test fails with DID NOT RAISE and the swallow appears in the log as Backend operation failed for get.

Also from that pass: KeyringConfigurationError re-exported from cachekit.serializers (it can escape into user code, so it needs a public name to catch), __module__ set so it survives pickling out of a ProcessPoolExecutor worker, and zeroize made optional behind the encryption feature.

Local gates: 2184 passed / 22 skipped (tests/unit + tests/critical), ruff clean, ruff format clean, basedpyright 0 errors, cargo clippy --all-targets --all-features -D warnings clean, and cargo check verified with the encryption feature both on and off.

@coderabbitai review

Comment thread src/cachekit/cache_handler.py
Comment thread src/cachekit/cache_handler.py
Comment thread tests/unit/test_key_rotation_keyring.py
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== changed files =='
git diff --name-only "$(git merge-base HEAD origin/main 2>/dev/null || true)" HEAD 2>/dev/null || git diff --name-only HEAD~1 HEAD

printf '%s\n' '== source outlines =='
ast-grep outline rust/src/python_bindings.rs --items all --match 'Keyring|decrypt|Error' || true
ast-grep outline src/cachekit/serializers/encryption_wrapper.py --items all --match 'EncryptionWrapper|deserialize|Keyring' || true
ast-grep outline src/cachekit/cache_handler.py --items all --match 'get|deserialize|Keyring' || true
ast-grep outline src/cachekit/config/settings.py --items all --match 'CachekitConfig|previous_master' || true

printf '%s\n' '== all configuration-error references and handler contexts =='
rg -n -C 5 '\bKeyringConfigurationError\b' rust/src/python_bindings.rs src tests || true

printf '%s\n' '== encryption and deserialization error handler contexts =='
rg -n -C 8 'except (SerializationError|DecryptionAuthenticationError|KeyringConfigurationError|Exception)' src/cachekit/cache_handler.py src/cachekit/serializers/encryption_wrapper.py || true

printf '%s\n' '== dependency constraints =='
rg -n -C 3 'pydantic-settings|h2' pyproject.toml

printf '%s\n' '== exports and binding registration =='
rg -n -C 4 'KeyringConfigurationError|PyKeyring|add_class|add\(' rust/src/python_bindings.rs src/cachekit/serializers || true

printf '%s\n' '== rotation test symbols =='
rg -n -C 3 'fingerprint_derivation_failure|TestEndToEndRotation|DEPLOYMENT_UUID|KeyringConfigurationError|InvalidCiphertext' tests/unit/test_key_rotation_keyring.py || true

Length of output: 94698


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== relevant commit diffs =='
git diff --unified=25 b46513c^ bc7a91f -- \
  pyproject.toml rust/Cargo.toml rust/src/python_bindings.rs \
  src/cachekit/cache_handler.py src/cachekit/serializers/encryption_wrapper.py \
  src/cachekit/serializers/__init__.py tests/unit/test_key_rotation_keyring.py \
  docs/features/zero-knowledge-encryption.md || true

printf '%s\n' '== Rust binding implementation sections =='
sed -n '90,165p' rust/src/python_bindings.rs
sed -n '300,405p' rust/src/python_bindings.rs
sed -n '475,505p' rust/src/python_bindings.rs

printf '%s\n' '== wrapper setup and both decrypt methods =='
sed -n '215,285p' src/cachekit/serializers/encryption_wrapper.py
sed -n '485,655p' src/cachekit/serializers/encryption_wrapper.py

printf '%s\n' '== settings key parsing/redaction =='
sed -n '1,225p' src/cachekit/config/settings.py

printf '%s\n' '== every CacheOperationHandler deserialization invocation =='
rg -n -C 7 '\b(?:self\.serialization_handler|serialization_handler)\.deserialize_data\s*\(' src/cachekit/cache_handler.py

printf '%s\n' '== all cache-handler methods and tests for L2 configuration propagation =='
ast-grep outline src/cachekit/cache_handler.py --items all --match 'get_cached_value|deserialize_data'
rg -n -C 5 'get_cached_value_with_freshness|get_cached_value_async|freshness_async|KeyringConfigurationError' tests

printf '%s\n' '== taxonomy comments mentioning structural/ciphertext failures =='
rg -n -C 3 'ciphertext-structure|short ciphertext|InvalidCiphertext|config.*tamper|tamper.*config' \
  rust/src/python_bindings.rs src/cachekit/serializers/encryption_wrapper.py tests/unit/test_key_rotation_keyring.py

Length of output: 124006


@27Bslash6 I verified the reported fixes.

The deliberate error-taxonomy deviation is correct. KeyringConfigurationError maps only KeyDerivation and KeyringIndexOutOfRange. Stored-ciphertext failures remain on the tamper path. This includes InvalidCiphertext from truncated ciphertext.

The four L2 read methods re-raise KeyringConfigurationError before their broad exception handlers. The dependency floor, PyO3-side key zeroization, public export, and fingerprint-derivation regression path are also present.

One non-blocking correction remains:

  • src/cachekit/serializers/encryption_wrapper.py Line 633 has a stale comment. It says that “ciphertext-structure failure” and a “short ciphertext” are configuration failures. The Rust mapping and the test correctly classify short ciphertext as tamper-class. Update this comment so that it states only configuration failures propagate as KeyringConfigurationError.

No functional security issue remains from the reviewed changes. 🐇

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kodus-27b kodus-27b Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found critical issues please review the requested changes

…too (LAB-684)

The previous commit closed the swallow at the four L2 read sites but left the
two L1 sites in decorators/wrapper.py, which have the identical shape: an
except SerializationError guard followed by a catch-all that logs
'L1 cache deserialization failed', invalidates the entry, and falls through.

For a local keyring config fault none of that is true — the L1 entry is fine,
the message misattributes the failure, and the invalidate is gratuitous. Both
Kody and the expert panel flagged L1 separately; fixing only the sites the
first report named would have left the same defect in the sibling callers.
@kodus-27b

This comment has been minimized.

@27Bslash6

Copy link
Copy Markdown
Contributor Author

Kody round triaged — 1 real finding fixed, 6 rejected as rule misfires

Commit 1e9efd9.

Accepted — the L1 read sites (medium, Bug)

You were right, and I had only half-fixed it. KeyringConfigurationError escaping the decrypt-failure policy was real; I closed the four L2 sites in cache_handler.py in bc7a91f but left the two L1 sites in decorators/wrapper.py (:1201 sync, :1541 async). Both have the identical shape — an except SerializationError guard followed by a catch-all that logs "L1 cache deserialization failed", invalidates the entry, and falls through to L2.

For a local keyring config fault none of that is true: the L1 entry is fine, the message misattributes the failure, and the invalidate is gratuitous. Both L1 sites now re-raise, matching L2.

Worth noting the two duplicate threads on encryption_wrapper.py:548 described the L2 hole, which was already fixed when you reviewed — but chasing them is what surfaced the L1 gap, so the round earned its keep.

Rejected (6)

Thread Rule Why rejected
encryption_wrapper.py:17, cache_handler.py:40, serializers/__init__.py:18 Pin exact versions with digests for base images Misfire. These are Python import statements, not Dockerfile FROM lines. There is no base image in any of these files.
test_key_rotation_keyring.py:365, :371 Don't use assert for data validation Misfire — this is a Python -O rule about validating user input in production code. These are asserts inside #[test]-equivalent pytest functions, where assert is the framework's required idiom. Rewriting them as if ... raise would break pytest's assertion introspection for no safety gain. This rule has now fired on test files across this PR family repeatedly (cachekit-rs#63 hit it twice); it is worth scoping the rule to exclude tests/.
cache_handler.py:1374 (+1411, 1447, 1494) Add specific exception handling Pre-existing except Exception blocks not introduced by this PR — and the change under review moves in exactly the direction the rule wants, adding a specific except KeyringConfigurationError immediately above each one. Narrowing the remaining catch-alls is a separate, larger refactor of the read path.

Gate status

2184 passed / 22 skipped (tests/unit + tests/critical), ruff clean, ruff format clean, basedpyright 0 errors, cargo clippy --all-targets --all-features -D warnings clean, cargo check verified with the encryption feature both on and off. CodeRabbit approved this PR at 16:49Z.

@kody start-review

Comment thread src/cachekit/decorators/wrapper.py
Comment thread src/cachekit/decorators/wrapper.py
…aise (LAB-684)

Kody caught an asymmetry the panel missed: the new sync L1 guard re-raised
without reset_current_function_stats(token), while its DecryptionAuthenticationError
sibling nine lines up does reset. The sync wrapper has no outer finally, so that
exit leaked the token. The async guard is correct as written — its wrapper's
outer finally covers every exit path.
@kodus-27b

kodus-27b Bot commented Aug 7, 2026

Copy link
Copy Markdown

Kody Review Complete

Great news! 🎉
No issues were found that match your current review configurations.

Keep up the excellent work! 🚀

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

  • Validate Business Logic: Ask Kody to validate your code against business rules by adding a comment with the @kody -v business-logic command.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Access your configuration settings here.

@27Bslash6
27Bslash6 merged commit e1b05ce into main Aug 8, 2026
35 checks passed
@27Bslash6
27Bslash6 deleted the lab-684-keyring-rotation-py branch August 8, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant