fix(encryption)!: resolve tenant_id once for HKDF and AAD (LAB-4668) - #133
Conversation
manager-core.ts derived HKDF keys with `tenantId ?? 'default'` but built the AAD tenant component with `tenantId ?? ''` - two independent fallbacks for the same value. With no tenantId configured, secure() ciphertext was bound to AAD tenant "" while the key was derived for "default", so it can never authenticate against a conformant py/rs reader (spec/intent-presets.md section Master Key Input, rule 5). Hoists a single effectiveTenantId field, resolved once in the constructor, and reads it from both call sites - no second ?? fallback remains. BREAKING CHANGE: existing secure() ciphertext written by cachekit-ts with no tenantId configured used AAD tenant "" and will fail authentication after this fix (the key is now correctly derived and bound to "default" everywhere). Zero-knowledge caches hold no irreplaceable state - flush/invalidate secure-preset entries written by an unconfigured-tenant cachekit-ts client before upgrading; there is no read-fallback for the old AAD.
…4668) Expert-panel follow-up on the tenant_id single-resolution fix: - Drop the "private readonly" modifier on the tenantId constructor parameter. Nothing read it after construction once effectiveTenantId existed, and a dead field with a name one character off from the resolved value is exactly the ambiguity the original bug came from. - Trim the effectiveTenantId JSDoc to one line; the ticket narrative belongs in the PR, not permanently embedded as a comment block. - Drop the redundant third assertion in the new regression test - it was implied by the two literal-equality assertions preceding it. Spec citation (spec/intent-presets.md Master Key Input rule 5) verified against cachekit-io/protocol main this pass.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 110 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: cachekit-io/cachekit-ts/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cachekit-io/cachekit-ts/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughChangesTenant ID consistency
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The encryption preset now consistently uses the protocol default tenant ID. Existing cache entries created with the former unset-tenant behavior require the stated invalidation before upgrade, but no actionable implementation risk remains in the reviewed change. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
Kody Code Review — 2 suggested fixes. 🛠️ Open Agent Prompt |
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
…t" (LAB-4668) The existing real-crypto edge case round-tripped on one instance, which passes even with the HKDF/AAD tenant mismatch this PR fixes: the same instance binds the same wrong AAD on both sides. Encrypting with an unset-tenant manager and decrypting with an explicit 'default' manager (and back) is what a py/rs reader does; verified to fail with "Authentication verification failed" against the pre-fix manager-core.ts.
27Bslash6
changed the title
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Overview
EncryptionManagerCoreused two independent fallbacks for an unconfiguredtenantId:tenantId ?? 'default'when callingderiveTenantKeys(HKDF), andtenantId ?? ''when assembling the AAD inbuildAAD. Ciphertext produced by asecure()client with no tenant configured was therefore keyed to"default"but bound to"", and could not authenticate against a spec-conformant reader.Changes
packages/cachekit/src/encryption/manager-core.tsprivate readonly effectiveTenantId: stringfield, assigned once in the constructor astenantId ?? 'default'.tenantIdis no longer aprivate readonlyproperty — the raw, unresolved value is no longer retained on the instance, eliminating any path to a second fallback.deriveTenantKeysandbuildAADnow both readthis.effectiveTenantId; the localeffectiveTenantIdvariable previously computed inside the init path was removed.Constructor signature and arity are unchanged;
tenantIdremainsstring | undefinedand externally optional. The only observable API change is the removal of the internaltenantIdproperty from the instance shape.Test Coverage
manager-core.test.tsadds adecodeAadTenantIdhelper that parses component 1 out of the serialized AAD (4-byte big-endian length prefix at offset 1, payload at offset 5) and two regression cases asserting that the tenant ID passed toderiveTenantKeysmatches the tenant component of the AAD handed toencryptWithTenantKeys— verified for both the unset ("default") and explicitly configured ("acme-corp") cases.Compatibility
Entries written by an unconfigured-tenant
secureclient prior to this change carry AAD tenant"". Decryption now rebuilds the AAD with"default", so the AES-GCM tag verification fails. No read-side fallback to the legacy AAD is implemented; affected cache entries must be invalidated.Summary
Strengthens encryption test coverage for the default tenant identifier, addressing LAB-4668 (tenant_id mismatch between HKDF key derivation and AAD construction).
Changes
packages/cachekit/src/encryption/manager.integration.test.tsThe edge-case test for an unset tenant ID has been rewritten from a same-instance round-trip to a cross-instance compatibility check:
'default'.finallyblock.Rationale
A single-manager encrypt/decrypt round-trip cannot detect a discrepancy between the tenant value used for HKDF derivation and the one embedded in the AAD, since both operations share the same (possibly inconsistent) resolution path. Exercising the boundary between an unset tenant and an explicit
'default'— the value written by the Python and Rust implementations — validates that the resolved tenant identifier is consistent across key derivation and authenticated data, preserving cross-language payload interoperability.API Impact
None. The change is confined to test code; no public interfaces were modified.
Breaking change
BREAKING CHANGE: a
securecache created without atenantIdnow binds tenant"default"into the AES-GCM AAD instead of"". That is the value HKDF already derived its key from and the value the protocol requires, so a tenant-less cachekit-tssecurecache now shares ciphertext with cachekit-py and cachekit-rs on tenant"default". Caches with an explicittenantIdare unaffected.Entries written by a tenant-less
securecache on@cachekit-io/cachekit0.1.5 or earlier no longer authenticate, so every read of one fails decryption. To migrate, delete those entries (or switch to a fresh namespace or key prefix) once no pre-upgrade instance is still writing; during a rolling upgrade, old and new instances cannot read each other's entries. Do not wait out the TTL instead. Under thesecurepreset's default reliability settings, each undecryptable read is retried and counted as a circuit-breaker failure, so five within a minute open the breaker, and while it is open every read misses and every write is dropped, for all keys.BEGIN_COMMIT_OVERRIDE
fix(encryption)!: resolve tenant_id once for HKDF and AAD (LAB-4668) (#133)
BREAKING CHANGE: a
securecache created without atenantIdnow binds tenant"default"into the AES-GCM AAD instead of"". That is the value HKDF already derived its key from and the value the protocol requires, so a tenant-less cachekit-tssecurecache now shares ciphertext with cachekit-py and cachekit-rs on tenant"default". Caches with an explicittenantIdare unaffected. Entries written by a tenant-lesssecurecache on@cachekit-io/cachekit0.1.5 or earlier no longer authenticate, so every read of one fails decryption. To migrate, delete those entries (or switch to a fresh namespace or key prefix) once no pre-upgrade instance is still writing; during a rolling upgrade, old and new instances cannot read each other's entries. Do not wait out the TTL instead. Under thesecurepreset's default reliability settings, each undecryptable read is retried and counted as a circuit-breaker failure, so five within a minute open the breaker, and while it is open every read misses and every write is dropped, for all keys.END_COMMIT_OVERRIDE