Skip to content

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

Merged
Nilesh Choudhary (4gust) merged 3 commits into
devfrom
4gust-fix-ext-cache-key-collision
Aug 6, 2026
Merged

Fix cache key hash collision: length-prefix extended cache key components#946
Nilesh Choudhary (4gust) merged 3 commits into
devfrom
4gust-fix-ext-cache-key-collision

Conversation

@4gust

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
Nilesh Choudhary (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 thread tests/test_token_cache.py Outdated
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 Outdated
Comment thread tests/test_token_cache.py Outdated
Comment thread tests/test_token_cache.py Outdated
Reword the token_cache and test docstrings to state that the length-prefix
scheme makes the *serialization* injective (distinct inputs cannot produce
the same pre-hash string), rather than implying absolute impossibility of a
collision at the SHA-256 hash layer. Also quote the dict-literal examples as
valid Python.

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 23, 2026 13:16

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 1 comment.

Comments suppressed due to low confidence (1)

tests/test_token_cache.py:777

  • The phrase "code-point length would make these ambiguous" is misleading here: the two strings differ regardless of which length you compute. The intent is to assert the implementation uses UTF-8 byte length (not len(str)); rewording this comment would make that clearer.
        # A concrete boundary pair the two length schemes disagree on:
        # code-point length would make these ambiguous, byte length does not.

Comment thread tests/test_token_cache.py
@bgavrilMS

Copy link
Copy Markdown
Member

Please update the milestone.

@4gust Nilesh Choudhary (4gust) added this to the 1.38.0 milestone Aug 6, 2026
@4gust
Nilesh Choudhary (4gust) merged commit 43ed3a8 into dev Aug 6, 2026
8 checks passed
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.

4 participants