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

* Fix cache key hash collision: length-prefix extended cache key components

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.

* Address review: harden injectivity fuzz test

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.

* Address review: clarify injectivity wording in docstrings

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.

Author: 4gust

msal/token_cache.py

@@ -88,15 +88,22 @@ def _compute_ext_cache_key(data):
 
     Returns an empty string when *data* has no hashable fields.
 
-    The algorithm matches MSAL .NET's ``ComputeAccessTokenExtCacheKey``: sorted
-    key+value pairs are concatenated (no separators) and SHA256 hashed, then
-    base64url encoded. This keeps the hash byte-identical to MSAL .NET.
-
-    MSAL Go's ``CacheExtKeyGenerator`` has since switched to a length-prefixed
-    encoding (AzureAD/microsoft-authentication-library-for-go#629) to make it
-    injective; Python deliberately tracks .NET instead, so these hashes are not
-    byte-identical to current Go. Caches are not shared across languages, so the
-    difference does not affect runtime correctness.
+    The algorithm uses a length-prefixed ("netstring") serialization matching
+    MSAL Go's ``CacheExtKeyGenerator``
+    (AzureAD/microsoft-authentication-library-for-go#629): for each key sorted
+    ascending, ``<byteLen(key)>:<key><byteLen(value)>:<value>`` is appended and
+    the parts concatenated, then SHA256 hashed and base64url (no padding)
+    encoded and lowercased.
+
+    The length prefixes make the *serialization* injective: distinct component
+    sets can never produce the same pre-hash string, so they cannot collide at
+    the serialization layer. (The final cache key is still a SHA-256 digest, so
+    only a cryptographically negligible hash collision remains possible.) A plain
+    ``key + value`` concatenation, by contrast, is ambiguous: ``{"fmi_path":
+    "value"}`` and ``{"fmi_pat": "hvalue"}`` would both serialize to
+    ``fmi_pathvalue``. The byte length (``len(s.encode("utf-8"))``), not the
+    Unicode code-point count, is used so the hash stays byte-identical across the
+    MSAL SDK family (Go/.NET/Java/JS) as they converge on this scheme.
     """
     if not data:
         return ""
@@ -106,11 +113,13 @@ def _compute_ext_cache_key(data):
     }
     if not cache_components:
         return ""
-    # Sort keys, then concatenate key+value pairs with no separators. This
-    # matches MSAL .NET's ComputeAccessTokenExtCacheKey byte-for-byte. (See the
-    # docstring re: the Go #629 length-prefixed divergence.)
+    # Sort keys, then length-prefix each key and value so the serialization is
+    # injective (see docstring). Byte-identical to MSAL Go's netstring encoding.
     key_str = "".join(
-        k + cache_components[k] for k in sorted(cache_components.keys())
+        "{klen}:{k}{vlen}:{v}".format(
+            klen=len(k.encode("utf-8")), k=k,
+            vlen=len(cache_components[k].encode("utf-8")), v=cache_components[k])
+        for k in sorted(cache_components.keys())
     )
     hash_bytes = hashlib.sha256(key_str.encode("utf-8")).digest()
     return base64.urlsafe_b64encode(hash_bytes).rstrip(b"=").decode("ascii").lower()

tests/test_token_cache.py

@@ -564,45 +564,49 @@ def test_non_fmi_tokens_not_affected_by_fmi_cache(self):
 
 
 class TestCrossMsalCacheKeyCompatibility(unittest.TestCase):
-    """Verify that _compute_ext_cache_key produces hashes identical to MSAL .NET
-    (CoreHelpers.ComputeAccessTokenExtCacheKey).
+    """Verify that _compute_ext_cache_key produces hashes identical to the shared
+    MSAL length-prefix ("netstring") encoding used by MSAL Go's
+    CacheExtKeyGenerator (AzureAD/microsoft-authentication-library-for-go#629).
 
     The algorithm:
       1. Sort key-value pairs alphabetically by key (ordinal / case-sensitive)
-      2. Concatenate them with no separators: "key1value1key2value2…"
+      2. Length-prefix each part with its UTF-8 byte length and concatenate:
+         "<len(key)>:<key><len(value)>:<value>..."
       3. SHA-256 hash
       4. Base64url encode (no padding), lowercased
 
-    The expected hashes below are copied from MSAL .NET's CacheKeyExtensionTests.cs
-    (RunHappyPathTest, CacheExtEnsurePopKeysFunctionAsync).
-
-    NOTE: MSAL Go's CacheExtKeyGenerator has since switched to a *length-prefixed*
-    encoding (AzureAD/microsoft-authentication-library-for-go#629), so these hashes
-    are intentionally NOT byte-identical to current Go; Python deliberately tracks
-    .NET here. The cache *key format* (the 'atext' segment layout, asserted below)
-    still matches both Go and .NET. Caches are not shared across languages, so this
-    cross-language hash difference does not affect runtime correctness.
+    The length prefixes make the *serialization* injective: distinct component
+    sets can never produce the same pre-hash string. (The final cache key is a
+    SHA-256 digest, so only a cryptographically negligible hash collision
+    remains possible.)
+
+    NOTE: This is the encoding the whole MSAL SDK family is converging on
+    (Go already merged it; .NET/Java/JS are landing the same change), so these
+    hashes are byte-identical across SDKs. The cache *key format* (the 'atext'
+    segment layout, asserted below) matches Go and .NET too. Caches are not
+    shared across languages, so cross-language parity is a convenience, not a
+    correctness requirement.
     """
 
-    def test_two_params_hash_matches_dotnet(self):
-        """.NET expected: bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi"""
+    def test_two_params_hash_matches_shared_encoding(self):
+        """Shared length-prefix expected: latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike"""
         result = _compute_ext_cache_key({"key1": "value1", "key2": "value2"})
-        self.assertEqual("bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi", result)
+        self.assertEqual("latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike", result)
 
-    def test_two_different_params_hash_matches_dotnet(self):
-        """.NET expected: 3-rg6_wyjx5bcy0c3cqq7gajtzgsqy3oxqpwj4y8k4u"""
+    def test_two_different_params_hash_matches_shared_encoding(self):
+        """Shared length-prefix expected: jjoe9jgfmdtnj0rzuetsqy7kzs2m1xfnjjxwsfxsrxq"""
         result = _compute_ext_cache_key({"key3": "value3", "key4": "value4"})
-        self.assertEqual("3-rg6_wyjx5bcy0c3cqq7gajtzgsqy3oxqpwj4y8k4u", result)
+        self.assertEqual("jjoe9jgfmdtnj0rzuetsqy7kzs2m1xfnjjxwsfxsrxq", result)
 
-    def test_five_params_hash_matches_dotnet(self):
-        """.NET expected (full hash): rn_gkpxxkkqjxcqnvnmr2duvxg66xanvkz6qfqpwp2e"""
+    def test_five_params_hash_matches_shared_encoding(self):
+        """Shared length-prefix expected (full hash): prrdp31y37ufw3lo7hly0oimjjvg_34m9ji30ocu4tw"""
         result = _compute_ext_cache_key({
             "key3": "value3", "key4": "value4",
             "key5": "value5", "key6": "value6", "key7": "value7",
         })
-        self.assertEqual("rn_gkpxxkkqjxcqnvnmr2duvxg66xanvkz6qfqpwp2e", result)
+        self.assertEqual("prrdp31y37ufw3lo7hly0oimjjvg_34m9ji30ocu4tw", result)
 
-    def test_order_independence_matches_dotnet(self):
+    def test_order_independence_matches_shared_encoding(self):
         """Same keys in different insertion order must produce the same hash
         (mirrors TestCacheKeyComponentHashConsistency in Go)."""
         h1 = _compute_ext_cache_key({"key3": "value3", "key4": "value4",
@@ -623,9 +627,9 @@ def test_at_cache_key_uses_atext_credential_type(self):
         key = key_maker(
             home_account_id="hid", environment="env", client_id="cid",
             realm="realm", target="scope",
-            ext_cache_key="bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi")
+            ext_cache_key="latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike")
         self.assertEqual(
-            "hid-env-atext-cid-realm-scope-bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi",
+            "hid-env-atext-cid-realm-scope-latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike",
             key)
 
     def test_at_cache_key_without_ext_uses_accesstoken(self):
@@ -637,9 +641,10 @@ def test_at_cache_key_without_ext_uses_accesstoken(self):
             realm="realm", target="scope")
         self.assertEqual("hid-env-accesstoken-cid-realm-scope", key)
 
-    def test_dotnet_style_full_at_cache_key(self):
-        """Reproduce the exact cache key from MSAL .NET CacheKeyExtensionTests:
-        expectedCacheKey1 = '-login.windows.net-atext-d3adb33f-c0de-ed0c-c0de-deadb33fc0d3-common-r1/scope1 r1/scope2-bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi'
+    def test_full_at_cache_key(self):
+        """Reproduce the full 'atext' cache key layout (mirrors MSAL .NET's
+        CacheKeyExtensionTests expectedCacheKey1 shape) with the shared
+        length-prefix hash for {key1:value1, key2:value2}.
         """
         cache = TokenCache()
         key_maker = cache.key_makers[TokenCache.CredentialType.ACCESS_TOKEN]
@@ -651,11 +656,12 @@ def test_dotnet_style_full_at_cache_key(self):
             realm="common",
             target="r1/scope1 r1/scope2",
             ext_cache_key=ext_hash)
-        expected = "-login.windows.net-atext-d3adb33f-c0de-ed0c-c0de-deadb33fc0d3-common-r1/scope1 r1/scope2-bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi"
+        expected = "-login.windows.net-atext-d3adb33f-c0de-ed0c-c0de-deadb33fc0d3-common-r1/scope1 r1/scope2-latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike"
         self.assertEqual(expected, key)
 
-    def test_dotnet_style_second_cache_key(self):
-        """Reproduce CacheKeyExtensionTests expectedCacheKey2."""
+    def test_second_full_at_cache_key(self):
+        """Reproduce the 'atext' cache key layout (mirrors expectedCacheKey2 shape)
+        with the shared length-prefix hash for {key3:value3, key4:value4}."""
         cache = TokenCache()
         key_maker = cache.key_makers[TokenCache.CredentialType.ACCESS_TOKEN]
         ext_hash = _compute_ext_cache_key({"key3": "value3", "key4": "value4"})
@@ -666,13 +672,13 @@ def test_dotnet_style_second_cache_key(self):
             realm="common",
             target="r1/scope1 r1/scope2",
             ext_cache_key=ext_hash)
-        expected = "-login.windows.net-atext-d3adb33f-c0de-ed0c-c0de-deadb33fc0d3-common-r1/scope1 r1/scope2-3-rg6_wyjx5bcy0c3cqq7gajtzgsqy3oxqpwj4y8k4u"
+        expected = "-login.windows.net-atext-d3adb33f-c0de-ed0c-c0de-deadb33fc0d3-common-r1/scope1 r1/scope2-jjoe9jgfmdtnj0rzuetsqy7kzs2m1xfnjjxwsfxsrxq"
         self.assertEqual(expected, key)
 
     def test_go_style_at_cache_key(self):
         """Reproduce the Go AccessToken.Key() *format* (segment layout):
-        'testhid-env-atext-clientid-realm-user.read-{hash}'. The hash follows our
-        .NET-matching encoding (see class note on the Go #629 divergence).
+        'testhid-env-atext-clientid-realm-user.read-{hash}'. The hash follows the
+        shared length-prefix encoding (byte-identical to MSAL Go).
         """
         cache = TokenCache()
         key_maker = cache.key_makers[TokenCache.CredentialType.ACCESS_TOKEN]
@@ -684,5 +690,122 @@ def test_go_style_at_cache_key(self):
             realm="realm",
             target="user.read",
             ext_cache_key=ext_hash)
-        expected = "testhid-env-atext-clientid-realm-user.read-bns2ytmx5hxkh4fnfixridmezpbbayhnmuh6t4bbghi"
+        expected = "testhid-env-atext-clientid-realm-user.read-latlwkpewb_a0rcsmjvkecqt0_huumkw4sflzociike"
         self.assertEqual(expected, key)
+
+
+class TestExtCacheKeyCollisionResistance(unittest.TestCase):
+    """The length-prefix ("netstring") serialization must be injective: no two
+    semantically different component sets may serialize to the same pre-hash
+    string. (The cache key itself is a SHA-256 digest, so the residual
+    collision risk is only the cryptographically negligible hash-layer one;
+    these tests pin the serialization layer that we control.)
+
+    A plain ``key + value`` concatenation (the old scheme) is ambiguous and
+    caused cache-slot collisions -- one FMI/agent-identity token entry could
+    evict another, forcing redundant token re-fetches. These tests pin the
+    boundary cases that the old scheme got wrong.
+    """
+
+    def test_key_value_boundary_ambiguity(self):
+        # Old scheme: both -> "fmi_pathvalue".
+        self.assertNotEqual(
+            _compute_ext_cache_key({"fmi_path": "value"}),
+            _compute_ext_cache_key({"fmi_pat": "hvalue"}))
+
+    def test_multi_entry_boundary_ambiguity(self):
+        # Old scheme: both -> "abcde".
+        self.assertNotEqual(
+            _compute_ext_cache_key({"a": "b", "cd": "e"}),
+            _compute_ext_cache_key({"ab": "c", "d": "e"}))
+
+    def test_value_containing_encoding_delimiters(self):
+        # Values that themselves contain the "<len>:<data>" delimiters must not
+        # be able to forge a different component layout.
+        self.assertNotEqual(
+            _compute_ext_cache_key({"a": "5:hello"}),
+            _compute_ext_cache_key({"a5": "hello"}))
+        self.assertNotEqual(
+            _compute_ext_cache_key({"x": "1:y1:z"}),
+            _compute_ext_cache_key({"x": "1:y", "1": "z"}))
+
+    def test_injectivity_over_adversarial_alphabet(self):
+        # Build many distinct component dicts from an adversarial alphabet that
+        # stresses the length-prefix delimiters and UTF-8 boundaries, then assert
+        # no two *distinct* dicts share a hash (and identical dicts agree).
+        import itertools
+        alphabet = ["", "0", "1", "9", ":", "|", "\\", "a", "ab",
+                    u"\u00e9", u"\U0001F600", u"e\u0301"]
+        seen = {}
+        for k1, v1 in itertools.product(alphabet, repeat=2):
+            for k2, v2 in itertools.product(alphabet, repeat=2):
+                # Keys must be truthy to survive field-selection, and distinct so
+                # the two pairs don't silently overwrite each other in a dict
+                # (which would collapse intended 2-entry cases). Values must be
+                # truthy too; empty values are dropped by field-selection.
+                pairs = [(k, v) for k, v in ((k1, v1), (k2, v2)) if k and v]
+                if len(pairs) == 2 and pairs[0][0] == pairs[1][0]:
+                    continue
+                comps = dict(pairs)
+                if not comps:
+                    continue
+                # Canonicalize so logically-equal dicts compare equal.
+                canonical = tuple(sorted(comps.items()))
+                h = _compute_ext_cache_key(comps)
+                if h in seen:
+                    self.assertEqual(
+                        seen[h], canonical,
+                        "Hash collision between {!r} and {!r} -> {}".format(
+                            dict(seen[h]), comps, h))
+                else:
+                    seen[h] = canonical
+        # seen holds one entry per *distinct* component set, so its size proves
+        # we exercised a meaningful number of unique inputs (not repeats).
+        self.assertGreater(len(seen), 100)
+
+    def test_utf8_byte_length_not_codepoint_length(self):
+        # 'é' as one 2-byte codepoint (U+00E9) vs 'e' + combining acute accent
+        # (U+0065 U+0301, 3 bytes) render alike but are distinct. A code-point
+        # length prefix (len(str)) would risk a collision at some boundary; the
+        # UTF-8 byte-length prefix keeps them apart and matches MSAL Go.
+        precomposed = u"\u00e9"          # 1 code point, 2 UTF-8 bytes
+        decomposed = u"e\u0301"          # 2 code points, 3 UTF-8 bytes
+        self.assertNotEqual(
+            _compute_ext_cache_key({"k": precomposed}),
+            _compute_ext_cache_key({"k": decomposed}))
+        # A concrete boundary pair the two length schemes disagree on:
+        # code-point length would make these ambiguous, byte length does not.
+        self.assertNotEqual(
+            _compute_ext_cache_key({"k": u"\u00e9x"}),
+            _compute_ext_cache_key({"k": u"e\u0301"}))
+
+    def test_key_order_independence(self):
+        self.assertEqual(
+            _compute_ext_cache_key({"b": "2", "a": "1", "c": "3"}),
+            _compute_ext_cache_key({"c": "3", "a": "1", "b": "2"}))
+
+    def test_empty_and_single_entry_edges(self):
+        self.assertEqual("", _compute_ext_cache_key(None))
+        self.assertEqual("", _compute_ext_cache_key({}))
+        self.assertEqual("", _compute_ext_cache_key({"fmi_path": ""}))
+        single = _compute_ext_cache_key({"fmi_path": "p"})
+        self.assertTrue(single)
+        self.assertEqual(single, _compute_ext_cache_key({"fmi_path": "p"}))
+
+    def test_golden_vectors(self):
+        # Shared length-prefix golden vectors: byte-identical across the MSAL SDK
+        # family (Go/.NET/Java/JS). Output is already lowercased.
+        golden = {
+            u"a0ry_zl4gccsdp7gnw927x8s0mrmnodv6tyilt0u07m":
+                {"fmi_path": "agent-app-id"},
+            u"cybgactkrvlzlen1aiwzwl3ay5krkyixommrobc-ri4":
+                {"a": "b", "cd": "e"},
+            u"n_lucewkadzv_nybtg-2wtorgf2nrns6ihlfa7vbuzg":
+                {"fmi_path": "value"},
+            u"tjtm16m-suk2_bkniblr25lyuki40qyceco7knuyu0k":
+                {"fmi_pat": "hvalue"},
+            u"xskzaoz4ibr3mznftyxctvg1ptuh-0fuzpty7ndbfls":
+                {u"\u00e9": u"\u00e9"},
+        }
+        for expected, components in golden.items():
+            self.assertEqual(expected, _compute_ext_cache_key(components))