Skip to content

Fix cache key hash collision: length-prefix extended cache key components#946

Open
4gust wants to merge 2 commits into
devfrom
4gust-fix-ext-cache-key-collision
Open

Fix cache key hash collision: length-prefix extended cache key components#946
4gust wants to merge 2 commits into
devfrom
4gust-fix-ext-cache-key-collision

Conversation

@4gust

@4gust 4gust commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Problem

The FMI / agent-identity feature hashes extended cache-key components (extra body params such as fmi_path, and cache-key-only pseudo-params like client_claims) into the token cache key via _compute_ext_cache_key in msal/token_cache.py.

The previous serialization concatenated the sorted key + value pairs 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"} → both fmi_pathvalue
  • {a: "b", cd: "e"} and {ab: "c", d: "e"} → both abcde

Impact: 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.

  • Uses the UTF-8 byte length (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.
  • Field-selection (_EXT_CACHE_KEY_EXCLUDED_FIELDS, truthy values) and the empty-input early return are unchanged.
  • The docstring/comments are updated to describe the length-prefix scheme (dropping the old "matches .NET / Go differs" wording).

Tests

  • Recomputed the expected hashes in TestCrossMsalCacheKeyCompatibility for the new encoding and reworded it to reflect convergence on the shared MSAL length-prefix hash. The cache-key format (atext segment layout) assertions are unchanged.
  • Added TestExtCacheKeyCollisionResistance covering:
    • boundary-ambiguity pairs the old scheme collided on (key/value and multi-entry);
    • values containing the <len>:<data> delimiter characters;
    • an injectivity fuzz test over an adversarial alphabet (digits, :, |, \, empty string, é, emoji, combining accent) asserting no two distinct component dicts collide;
    • UTF-8 byte-length correctness (precomposed é vs e + combining accent must not collide — proving byte length, not code-point length, is used);
    • empty / single-entry edges;
    • golden-vector regression with hashes that are byte-identical across the MSAL SDK family.

python -m pytest tests/test_token_cache.py → 52 passed. (tests/test_fmi_e2e.py is a live lab test requiring the azure package + credentials and only asserts that keys differ; no hard-coded hashes.)

Compatibility note

Because the serialization changes, the computed ext_cache_key hashes 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

  • MSAL Python does not use beachball and has no CHANGELOG.md, so no changefile is added.

…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
Copilot AI review requested due to automatic review settings July 21, 2026 10:19
@4gust
4gust requested a review from a team as a code owner July 21, 2026 10:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_key to 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.

Comment thread tests/test_token_cache.py Outdated
Comment on lines +738 to +742
# 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/test_token_cache.py Outdated
dict(seen[h]), comps, h))
else:
seen[h] = canonical
self.assertGreater(count, 100)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copilot AI review requested due to automatic review settings July 21, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread msal/token_cache.py
Comment on lines +98 to +101
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
Comment thread tests/test_token_cache.py
Comment on lines +578 to +579
The length prefixes make the serialization injective, so semantically
different component sets can never collide onto the same cache key.
Comment thread tests/test_token_cache.py
Comment on lines +696 to +697
"""The length-prefix ("netstring") serialization must be injective: no two
semantically different component sets may hash to the same cache key.
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.

2 participants