fix(encrypted_maps): harden derived key material caching#401
Open
marc0olo wants to merge 6 commits into
Open
Conversation
Derived key material for EncryptedMaps was persisted to IndexedDB indefinitely and keyed only by [mapOwner, mapName]. A same-origin attacker could use a persisted (non-extractable) key handle to decrypt without an authenticated session, and after an identity switch on the same origin a different principal could be served key material cached by a prior one. - Cache derived key material in memory by default (InMemoryDerivedKeyMaterialCache); persistence to IndexedDB (IndexedDbDerivedKeyMaterialCache) is now an explicit opt-in. - Scope cache entries to the authenticated caller's principal, closing the cross-identity cache-hit hole. EncryptedMapsClient gains getCallerPrincipal(). - Add EncryptedMaps.clearCache() for logout / identity change. - Make caching strategy configurable via the constructor. - Bump @icp-sdk/core to ^5.4.0. BREAKING CHANGE: EncryptedMaps no longer persists key material by default, and EncryptedMapsClient requires getCallerPrincipal(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens EncryptedMaps derived key material caching to reduce at-rest exposure and prevent cross-identity cache hits on the same origin, addressing the security review findings while keeping persistence available as an explicit opt-in.
Changes:
- Replaces implicit IndexedDB persistence with an explicit caching strategy (
InMemory…default;IndexedDb…opt-in) and addsEncryptedMaps.clearCache(). - Scopes derived-key cache entries to the authenticated caller via a new
EncryptedMapsClient.getCallerPrincipal()requirement. - Updates dependencies to
@icp-sdk/core@^5.4.0and adds tests covering cache behavior and caller scoping.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks @icp-sdk/core to 5.4.0 (and transitive updates). |
| frontend/ic_vetkeys/src/encrypted_maps/index.ts | Introduces pluggable cache strategy, caller-scoped cache keys, and clearCache(). |
| frontend/ic_vetkeys/src/encrypted_maps/encrypted_maps_canister.ts | Implements getCallerPrincipal() in the default client via the agent. |
| frontend/ic_vetkeys/src/encrypted_maps/cache.ts | Adds cache strategy interface plus in-memory and IndexedDB implementations. |
| frontend/ic_vetkeys/src/encrypted_maps/cache.test.ts | Adds unit tests for cache round-trips, caller scoping, and clearCache(). |
| frontend/ic_vetkeys/package.json | Bumps @icp-sdk/core dependency range to ^5.4.0. |
| frontend/ic_vetkeys/CHANGELOG.md | Documents the caching strategy changes, security implications, and breaking API updates. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…type-assertion The in-memory cache methods wrapped synchronous Map operations in async functions with no await (require-await), and a test used an unnecessary type assertion. Return promises explicitly and narrow with a guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Versions <=0.4.0 persisted derived key material to idb-keyval's default store. The new cache uses a dedicated store, so legacy entries are neither used nor cleared on upgrade. Document a surgical one-time cleanup snippet so upgraders can remove the residual at-rest capability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename EncryptedMapsClient.getCallerPrincipal -> get_caller_principal
to match the interface's snake_case canister-mirroring convention. A
holistic camelCase migration of the client interfaces is deferred to a
dedicated follow-up targeting the same unreleased 0.5.0.
- Tighten the legacy IndexedDB cleanup snippet to match the exact legacy
key shape [string, Uint8Array] before deleting, avoiding any chance of
removing unrelated array-keyed CryptoKey entries.
- Use {@link CryptoKey} instead of {@link !CryptoKey} for doc consistency.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…caller introspection Per review, the library should not reach back through the canister-client interface to introspect the caller's identity for cache scoping. Identity lifecycle is the consumer's responsibility, and that introspection was also the source of a TOCTOU race (the principal read for the cache key could differ from the identity used for the fetch). - Revert the `get_caller_principal()` addition to `EncryptedMapsClient`; the interface is once again a pure canister mirror (no breaking change for custom client implementers). - Cache key is `[mapOwner, mapName]` again. Cross-identity isolation is now a property of the cache: a per-identity in-memory instance (the default), or a per-identity-namespaced IndexedDb store. - Document the isolation contract on EncryptedMaps, clearCache(), and both cache implementations. Update tests to cover owner distinction, instance isolation, and IndexedDb namespace isolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-identity namespace hint used an inline code span containing a template literal's backticks, which renders incorrectly in Markdown/TypeDoc. Move it to a fenced code block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Context
A Zeropath security review flagged that the EncryptedMaps frontend SDK persisted per-map derived key material (a non-extractable WebCrypto
CryptoKey) in IndexedDB indefinitely, in a fixed store keyed only by[mapOwner, mapName].Security assessment
The finding is valid, with nuance:
CryptoKeyis non-extractable, so an attacker cannot exfiltrate the raw key bytes (crypto.subtle.exportKeythrows). Storing a non-extractable key in IndexedDB is itself the WebCrypto-recommended pattern.[mapOwner, mapName]in one shared store, after an identity switch on the same origin a different principal could be served key material cached by a prior one.Approach
Key material is cached, but identity lifecycle stays the consumer's responsibility — the library does not introspect the agent's identity. (An earlier revision routed the caller principal through the client interface to auto-scope the key; that was reverted because it broke the canister-mirror interface and introduced a TOCTOU race between the principal read for the key and the identity used for the fetch.)
Changes
InMemoryDerivedKeyMaterialCache) — never touches disk, discarded on reload. Persistence to IndexedDB (IndexedDbDerivedKeyMaterialCache) is now an explicit opt-in via the constructor.EncryptedMapsinstance per identity (orclearCache()on switch);EncryptedMaps.clearCache()— strongly recommended on logout / identity change to drop the still-usable decryption capability.new EncryptedMaps(client, { cache }). TheEncryptedMapsClientinterface is unchanged (still a pure canister mirror).@icp-sdk/coreto^5.4.0.EncryptedMapsno longer persists key material by default (one extra key derivation per map per page load unless opting into IndexedDB). No interface changes — existing customEncryptedMapsClientimplementations keep working.Upgrading from ≤0.4.0
Versions
0.1.0–0.4.0persisted derived key material to idb-keyval's default store. The new cache uses a dedicated store, so legacy entries are neither used nor cleared by this version. The CHANGELOG includes a surgical one-time cleanup snippet that deletes only legacy-shaped entries ([string, Uint8Array] -> CryptoKey) from the default store, leaving other app data untouched.Tests
cache.test.ts: in-memory & IndexedDB round-trip, non-extractable key survives persistence (exportKeystill rejects), cache hit/miss, owner distinction, prefix-collision safety, separate-instance isolation, IndexedDB namespace isolation, andclearCache()re-fetch.@icp-sdk/core5.4.0, on local + CI (Linux & macOS).tsc,vite build,lint, andprettier-checkclean.Reviewer notes (advisory, not blocking)
${mapOwner}|${hex(mapName)}— principal text is base32 ([a-z2-7-]) andbytesToHexis[0-9a-f], so the|delimiter can never appear inside a field; the key is injective.clearCache()on logout), not enforced by identity introspection — deliberately, to keep the canister-client interface stable and avoid a TOCTOU race.Note for maintainers
package.jsonstill reads0.4.0while the CHANGELOG targets0.5.0 - Unreleased; the breaking change above should land under0.5.0per the release flow.🤖 Generated with Claude Code