Fix cache key hash collision: length-prefix extended cache key components#946
Fix cache key hash collision: length-prefix extended cache key components#9464gust wants to merge 2 commits into
Conversation
…ents
The extended cache key serialization concatenated sorted key+value pairs
with no delimiters, which is not injective: distinct component sets such as
{fmi_path: 'value'} and {fmi_pat: 'hvalue'} serialized to the same string
and hashed to the same cache key, causing cache-slot collisions and
redundant token re-fetches.
Adopt MSAL Go's length-prefixed (netstring) encoding
(<byteLen(key)>:<key><byteLen(value)>:<value> per sorted key, UTF-8 byte
lengths), which is injective and byte-identical across the MSAL SDK family.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6efa99f2-b923-4ea8-b5b8-40696d1470a5
There was a problem hiding this comment.
Pull request overview
This pull request fixes extended token cache key hash collisions by changing _compute_ext_cache_key to use an injective, UTF-8 byte-length-prefixed (“netstring”-style) serialization before SHA-256 + base64url encoding, aligning with the cross-SDK scheme MSAL is converging on.
Changes:
- Update
msal/token_cache.py::_compute_ext_cache_keyto length-prefix each key/value using UTF-8 byte lengths prior to hashing. - Update cross-MSAL compatibility tests to the new expected hashes and wording.
- Add collision-resistance tests (boundary ambiguities, delimiter-like content, UTF-8 byte-length cases, fuzz/injectivity checks, golden vectors).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
msal/token_cache.py |
Implements length-prefixed serialization to prevent extended cache key hash collisions. |
tests/test_token_cache.py |
Updates expected hashes and adds new tests to validate injectivity and collision resistance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Keys must be distinct and truthy to survive field-selection; | ||
| # empty keys/values are dropped, so skip combos that collapse. | ||
| comps = {k: v for k, v in ((k1, v1), (k2, v2)) if k and v} | ||
| if not comps: | ||
| continue |
There was a problem hiding this comment.
Good catch - fixed in ca29f0c. The comprehension could silently overwrite when k1 == k2. Now I filter truthy pairs, explicitly continue when both surviving pairs share a key, and build the dict from that list so 2-entry cases aren't collapsed.
| dict(seen[h]), comps, h)) | ||
| else: | ||
| seen[h] = canonical | ||
| self.assertGreater(count, 100) |
There was a problem hiding this comment.
Agreed - fixed in ca29f0c. Dropped the raw iteration counter and now assert len(seen) > 100, since seen holds exactly one entry per distinct component set, so it genuinely reflects the number of unique inputs exercised.
Skip same-key pair combinations explicitly (a dict comprehension would silently overwrite and collapse intended 2-entry cases), and assert on len(seen) -- the count of distinct component sets exercised -- instead of a raw iteration counter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6efa99f2-b923-4ea8-b5b8-40696d1470a5
| The length prefixes make the serialization injective, so semantically | ||
| different component sets can never collide onto the same cache key. (A plain | ||
| ``key + value`` concatenation is ambiguous: ``{fmi_path: "value"}`` and | ||
| ``{fmi_pat: "hvalue"}`` would both serialize to ``fmi_pathvalue``.) The byte |
| The length prefixes make the serialization injective, so semantically | ||
| different component sets can never collide onto the same cache key. |
| """The length-prefix ("netstring") serialization must be injective: no two | ||
| semantically different component sets may hash to the same cache key. |
Problem
The FMI / agent-identity feature hashes extended cache-key components (extra body params such as
fmi_path, and cache-key-only pseudo-params likeclient_claims) into the token cache key via_compute_ext_cache_keyinmsal/token_cache.py.The previous serialization concatenated the sorted
key + valuepairs with no delimiters, which is not injective. Semantically different component sets serialize to the same string and therefore hash to the same cache key:{fmi_path: "value"}and{fmi_pat: "hvalue"}→ bothfmi_pathvalue{a: "b", cd: "e"}and{ab: "c", d: "e"}→ bothabcdeImpact: a cache-slot collision — one token entry overwrites/evicts another, forcing a redundant token re-fetch.
Fix
Replace the delimiter-less concatenation with an injective length-prefixed ("netstring") encoding, matching MSAL Go's
CacheExtKeyGenerator(microsoft-authentication-library-for-go#629). For each key sorted ascending, append<byteLen(key)>:<key><byteLen(value)>:<value>and concatenate; then SHA256 → base64url (no padding) → lowercase, exactly as before.len(s.encode("utf-8"))), not the Unicode code-point count, so the hash stays byte-identical to MSAL Go/.NET/Java/JS as the SDK family converges on this scheme._EXT_CACHE_KEY_EXCLUDED_FIELDS, truthy values) and the empty-input early return are unchanged.Tests
TestCrossMsalCacheKeyCompatibilityfor the new encoding and reworded it to reflect convergence on the shared MSAL length-prefix hash. The cache-key format (atextsegment layout) assertions are unchanged.TestExtCacheKeyCollisionResistancecovering:<len>:<data>delimiter characters;:,|,\, empty string,é, emoji, combining accent) asserting no two distinct component dicts collide;évse+ combining accent must not collide — proving byte length, not code-point length, is used);python -m pytest tests/test_token_cache.py→ 52 passed. (tests/test_fmi_e2e.pyis a live lab test requiring theazurepackage + credentials and only asserts that keys differ; no hard-coded hashes.)Compatibility note
Because the serialization changes, the computed
ext_cache_keyhashes change. On upgrade, previously cached FMI / extended-cache-key access tokens will not be found under their new keys, causing a one-time cache miss (a single extra token fetch per affected entry). This is benign and self-healing — no action required. Only extended-cache-key entries (e.g. FMI) are affected; regular access tokens are unchanged.Notes
CHANGELOG.md, so no changefile is added.