Skip to content

feat: cache keyring per-tenant derivation at construction via TenantKeyring (LAB-1638) - #71

Merged
27Bslash6 merged 2 commits into
mainfrom
agent/winston/572ea258
Aug 8, 2026
Merged

feat: cache keyring per-tenant derivation at construction via TenantKeyring (LAB-1638)#71
27Bslash6 merged 2 commits into
mainfrom
agent/winston/572ea258

Conversation

@27Bslash6

Copy link
Copy Markdown
Contributor

Closes LAB-1638.

Problem

Keyring::decrypt / decrypt_at re-ran HKDF-SHA256 per-tenant derivation on every decrypt attempt — attempt 0 (the current key, the common case) included. Any SDK routing reads through the keyring paid a fresh HKDF per read, L1 hits included (L1 stores ciphertext), and all master keys stayed resident for the client's lifetime. Origin: LAB-686 expert-panel finding on cachekit-rs#63, deferred to core per the LAB-683 rule that SDKs never re-implement keyring attempt sequencing.

Change (additive only)

Keyring::for_tenant(tenant_id) consumes the keyring, derives each entry's per-tenant encryption key exactly once, and drops (zeroizes) the master keys. The returned TenantKeyring decrypts against the derived ring:

  • Zero HKDF at steady state — asserted by a thread-local derivation-counter test (construction = exactly one derivation per entry; N subsequent decrypt/decrypt_at/encryption_fingerprints calls = zero).
  • Sequential semantics byte-identical and test-covered: current-first order, identical AAD per attempt, only AuthenticationFailed advances, structural errors terminal, exhaustion = plain AuthenticationFailed (normative per protocol spec/encryption.md § "Key Rotation (Keyring)"; no spec change).
  • Error classes unchanged (LAB-683 no-collapse rule): KeyringIndexOutOfRange preserved on decrypt_at; KeyDerivation stays config-class — it now surfaces at for_tenant construction instead of first decrypt.
  • Zeroization: masters wiped when the consumed Keyring drops (including the partial-derivation error path, which wipes already-derived keys); TenantKeyring is ZeroizeOnDrop; no Debug/Clone on the new type.
  • Fingerprint parity: TenantKeyring::encryption_fingerprints() (infallible, derivation-free) is byte-identical to Keyring::encryption_fingerprints / derive_tenant_keys — test-asserted.
  • Keyring::new / decrypt / decrypt_at untouched.

Expert panel (mandatory crypto gate) — run at critical-stakes

  • bug-hunter-supreme: NO FINDINGS.
  • security-specialist: NO FINDINGS — verified no key material in Debug/errors (CWE-215), zeroization complete on error paths (no Vec reallocation), derived-ring residency reduces blast radius (single-tenant derived keys replace all-tenant masters), no new oracle (error classes and attempt-count timing identical; KeyDerivation timing move is operator-facing config validation).
  • code-craftsman (1 MIN) + catchphrase-agent (1 cut), converged: the zeroize test's byte-scan was vacuous (Vec::zeroize clears the vec, so the scan iterated nothing) — applied in c3f2f61: assert the emptied ring (the observable post-condition) and keep the compile-time ZeroizeOnDrop bound. Craftsman explicitly ruled the decrypt loop duplication justified (7 lines, contract pinned by tests) and sharing the cached derivation with the non-consuming Keyring::encryption_fingerprints gold-plating (would force master-key cloning for zero callers). Catchphrase verdict: "Lean for critical-stakes crypto."

Docs pass

rustdoc updated: module doc names the tenant-bound steady-state path; for_tenant carries a full executable doctest (runs in CI per the executable-docs convention). README claims checked — nothing stated per-decrypt derivation, architecture lines still accurate. Protocol spec: no change needed, semantics unchanged (explicitly out of scope per ticket). SDK adoption is follow-up work per SDK; LAB-1645 (winning-index surface) unblocks behind this.

Gates

cargo fmt / clippy --all-features --all-targets -D warnings / cargo test --all-features (105 tests + doctests) all green; cargo check --target wasm32-unknown-unknown --features encryption clean.

… construction (LAB-1638)

Keyring::decrypt re-ran HKDF-SHA256 per attempt on every call — attempt 0
(the current key, the common case) included — so every keyring-routed read
paid a fresh derivation, L1 hits included since L1 stores ciphertext, and
all master keys stayed resident for the client's lifetime.

Keyring::for_tenant(tenant_id) consumes the keyring, derives each entry's
per-tenant encryption key exactly once, and drops (zeroizes) the masters.
The returned TenantKeyring decrypts against the derived ring with zero HKDF
at steady state: same attempt order, identical AAD per attempt, only
AuthenticationFailed advances, structural/config errors terminal,
exhaustion = plain AuthenticationFailed. Error classes unchanged —
KeyDerivation surfaces at construction, KeyringIndexOutOfRange preserved
on decrypt_at (no-collapse rule). Derived ring is ZeroizeOnDrop;
fingerprints byte-identical to Keyring::encryption_fingerprints.

Additive only: Keyring::new / decrypt / decrypt_at unchanged. No-HKDF
steady state asserted by a thread-local derivation counter test.
…ding)

Vec::zeroize wipes elements then clears the vec, so iterating the keys
afterwards scanned nothing — the byte-check could never fail. Assert the
emptied ring instead; the compile-time ZeroizeOnDrop bound remains the
drop guarantee.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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: 15 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: c070e75b-f34b-453f-a0cc-890e07ed38df

📥 Commits

Reviewing files that changed from the base of the PR and between 11bb346 and c3f2f61.

📒 Files selected for processing (3)
  • src/encryption/keyring.rs
  • src/encryption/mod.rs
  • src/lib.rs

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

@kodus-27b

kodus-27b Bot commented Aug 7, 2026

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

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.

Comment thread src/encryption/keyring.rs
Comment thread src/encryption/keyring.rs

@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

@27Bslash6

Copy link
Copy Markdown
Contributor Author

@kody start-review

@27Bslash6

Copy link
Copy Markdown
Contributor Author

@27Bslash6ready for your signoff / merge. Automated review gates are clean and the mandatory crypto gate is satisfied.

Verified independently this pass (review-signoff-sweep, LAB-1638):

  • CI green across the full matrix — 1.85 MSRV, stable ubuntu/macos/windows, beta, security, CodeQL, Cargo Vet, and all 17 fuzz corpus targets (incl. encryption_key_derivation, key_derivation, encryption_aad_injection, encryption_nonce_reuse).
  • CodeRabbit: APPROVED. Kody: APPROVED after both CHANGES_REQUESTED findings were rebutted as language-mismatched rules (a Python assert-validation rule and a TypeScript as-cast rule applied to Rust assert! inside #[cfg(test)] mod tests). Zero code change was correct.
  • Expert panel (mandatory crypto gate — key-derivation surface) ran at critical stakes: bug-hunter and security-specialist no findings; the converged craftsman/catchphrase finding applied in c3f2f61.
  • Local re-verification of c3f2f61: cargo fmt --check clean, cargo clippy --all-features --all-targets -D warnings clean, cargo test --all-features 216 pass / 0 fail (18 keyring tests + 2 doctests).

Two things I checked rather than took on trust, since they carry the proof burden:

  1. Vec::with_capacity(entry_count()) in for_tenant is load-bearing for zeroization, not cosmetic — exact pre-allocation means no mid-push realloc can free an unwiped buffer holding already-derived key bytes, so the error path's keys.zeroize() has only one buffer to scrub.
  2. Neither proof-bearing test is vacuous. test_tenant_keyring_derives_exactly_once counts exactly 2 derivations at construction then asserts zero across 10 rounds of decrypt/decrypt_at/encryption_fingerprints — including the fall-through path where attempt 0 fails auth and attempt 1 succeeds. test_tenant_keyring_derived_keys_zeroize now asserts the observable post-condition plus a compile-time ZeroizeOnDrop bound, correctly fixing the panel's finding that the old byte-scan iterated an already-cleared vec and could never fail.

No code changes made by this sweep — the tree is exactly c3f2f61. I do not merge; that's yours.

Note: --add-reviewer 27Bslash6 no-ops here because you authored the PR, hence this mention instead.

@27Bslash6
27Bslash6 merged commit d3f0eb0 into main Aug 8, 2026
32 checks passed
@27Bslash6
27Bslash6 deleted the agent/winston/572ea258 branch August 8, 2026 00:17
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