feat: cache keyring per-tenant derivation at construction via TenantKeyring (LAB-1638) - #71
Conversation
… 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.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 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 (3)
Comment |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@kody start-review |
|
@27Bslash6 — ready 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):
Two things I checked rather than took on trust, since they carry the proof burden:
No code changes made by this sweep — the tree is exactly Note: |
Closes LAB-1638.
Problem
Keyring::decrypt/decrypt_atre-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 returnedTenantKeyringdecrypts against the derived ring:decrypt/decrypt_at/encryption_fingerprintscalls = zero).AuthenticationFailedadvances, structural errors terminal, exhaustion = plainAuthenticationFailed(normative per protocolspec/encryption.md§ "Key Rotation (Keyring)"; no spec change).KeyringIndexOutOfRangepreserved ondecrypt_at;KeyDerivationstays config-class — it now surfaces atfor_tenantconstruction instead of first decrypt.Keyringdrops (including the partial-derivation error path, which wipes already-derived keys);TenantKeyringisZeroizeOnDrop; noDebug/Cloneon the new type.TenantKeyring::encryption_fingerprints()(infallible, derivation-free) is byte-identical toKeyring::encryption_fingerprints/derive_tenant_keys— test-asserted.Keyring::new/decrypt/decrypt_atuntouched.Expert panel (mandatory crypto gate) — run at critical-stakes
KeyDerivationtiming move is operator-facing config validation).Vec::zeroizeclears the vec, so the scan iterated nothing) — applied in c3f2f61: assert the emptied ring (the observable post-condition) and keep the compile-timeZeroizeOnDropbound. Craftsman explicitly ruled thedecryptloop duplication justified (7 lines, contract pinned by tests) and sharing the cached derivation with the non-consumingKeyring::encryption_fingerprintsgold-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_tenantcarries 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 encryptionclean.