Harden forwarded_client_claims (cross-MSAL review fixes)

Apply six fixes derived from the sibling MSAL PRs (go #629, java #1039,
js #8686) to the forwarded_client_claims port:

1. Make _compute_ext_cache_key injective. Switch from separator-less
   key+value concatenation to length-prefixed pairs ("{len(k)}:{k}{len(v)}:{v}"),
   matching Go's post-collision-fix CacheExtKeyGenerator. Without this,
   fmi_path + client_claims (which now co-occur in acquire_token_for_client)
   could collide and return the wrong cached token. Adds boundary-collision
   regression tests. NOTE: hashes are now intentionally not byte-identical to
   MSAL .NET (which still uses unprefixed concat); caches are not shared across
   languages, so within-process injectivity is what matters.

2. Remove the MSIv1 client-side allow-list (_validate_msiv1_claims). Forward
   any JSON-object claims value as-is and let IMDS decide which keys it accepts,
   matching go/java.

3. Validate the managed-identity source before the cache read. Reject
   unsupported sources (Service Fabric, App Service, Machine Learning, Azure Arc)
   up front so an unsupported source never returns a cached client-claims token.
   _obtain_token keeps its per-source guards as a backstop.

4. Add merge-conflict precedence tests: on a direct leaf conflict the
   client-originated value wins (merged last); disjoint claims are preserved.

5. Drop the first-party xms_az_nwperimid example from public docstrings; use
   generic "client-originated claims" wording.

6. Document that the same forwarded_client_claims value must be sent on every
   request that should share the cached token (it is part of the cache key).

204 tests pass across test_token_cache.py, test_mi.py, test_application.py.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Robbie-Microsoft

msal/application.py

@@ -67,8 +67,8 @@ def _merge_claims_challenge_and_capabilities(capabilities, claims_challenge):
 def _stash_client_claims(forwarded_client_claims, data):
     """Validate ``forwarded_client_claims`` and stash it into the request ``data``.
 
-    ``forwarded_client_claims`` carries *client-originated* claims (for example a
-    network security perimeter ``xms_az_nwperimid`` claim). The raw value is
+    ``forwarded_client_claims`` carries *client-originated* claims supplied by the
+    caller. The raw value is
     stored in ``data`` (under the internal ``client_claims`` key) so that it
     (a) contributes to the extended cache key -- isolating cache entries by
     claims value -- and (b) is stripped from the request body by the oauth2
@@ -1303,11 +1303,12 @@ def acquire_token_by_authorization_code(
             returned from the UserInfo Endpoint and/or in the ID Token and/or Access Token.
             It is a string of a JSON object which contains lists of claims being requested from these locations.
         :param str forwarded_client_claims:
-            Optional. A JSON string of *client-originated* claims (for example
-            a network security perimeter ``xms_az_nwperimid`` claim) to include
-            in the token request. Unlike ``claims_challenge`` (server-issued,
-            which bypasses the cache), tokens acquired with ``forwarded_client_claims``
-            **are cached** and keyed on the claims value, so use stable,
+            Optional. A JSON string of *client-originated* claims to include in
+            the token request. Unlike ``claims_challenge`` (server-issued, which
+            bypasses the cache), tokens acquired with ``forwarded_client_claims``
+            **are cached** and keyed on the claims value. Send the *same* value on
+            every request that should share the cached token; omitting or changing
+            it routes to a different cache entry (a cache miss), so use stable,
             non-dynamic values. The value is merged into the standard OAuth
             ``claims`` request parameter sent on the wire.
 
@@ -1588,11 +1589,11 @@ def acquire_token_silent_with_error(
             returned from the UserInfo Endpoint and/or in the ID Token and/or Access Token.
             It is a string of a JSON object which contains lists of claims being requested from these locations.
         :param str forwarded_client_claims:
-            Optional. A JSON string of *client-originated* claims (for example
-            a network security perimeter ``xms_az_nwperimid`` claim) to include
-            when a cached token is missing and a network request is made. Tokens
-            are **cached** and keyed on the claims value (different values yield
-            separate cache entries), so use stable, non-dynamic values.
+            Optional. A JSON string of *client-originated* claims to include when
+            a cached token is missing and a network request is made. Tokens are
+            **cached** and keyed on the claims value (different values yield
+            separate cache entries), so send the *same* value on every call that
+            should reuse the cached token, and use stable, non-dynamic values.
 
             Not to be confused with the constructor ``client_claims`` parameter
             (a ``dict`` of extra claims signed into the client-assertion JWT).
@@ -2592,16 +2593,16 @@ def acquire_token_for_client(self, scopes, claims_challenge=None, fmi_path=None,
                 )
         :param str forwarded_client_claims:
             Optional. A JSON string containing *client-originated* claims to
-            include in the token request (for example a network security
-            perimeter ``xms_az_nwperimid`` claim).
+            include in the token request.
 
             Unlike ``claims_challenge`` (which carries *server-issued* claims
             challenges and bypasses the cache), tokens acquired with
             ``forwarded_client_claims`` **are cached**, and the cache entry is keyed on the
-            claims value. Different ``forwarded_client_claims`` values produce separate
-            cache entries, so use stable, non-dynamic values to avoid unbounded
-            cache growth. The value is merged into the standard OAuth ``claims``
-            request parameter sent on the wire.
+            claims value. Send the *same* value on every request that should share
+            the cached token; different values produce separate cache entries, so
+            use stable, non-dynamic values to avoid unbounded cache growth. The
+            value is merged into the standard OAuth ``claims`` request parameter
+            sent on the wire.
 
             Not to be confused with the constructor ``client_claims`` parameter
             (a ``dict`` of extra claims signed into the client-assertion JWT).
@@ -2699,11 +2700,12 @@ def acquire_token_on_behalf_of(self, user_assertion, scopes, claims_challenge=No
             returned from the UserInfo Endpoint and/or in the ID Token and/or Access Token.
             It is a string of a JSON object which contains lists of claims being requested from these locations.
         :param str forwarded_client_claims:
-            Optional. A JSON string of *client-originated* claims (for example
-            a network security perimeter ``xms_az_nwperimid`` claim) to include
-            in the token request. Unlike ``claims_challenge`` (server-issued,
-            which bypasses the cache), tokens acquired with ``forwarded_client_claims``
-            **are cached** and keyed on the claims value, so use stable,
+            Optional. A JSON string of *client-originated* claims to include in
+            the token request. Unlike ``claims_challenge`` (server-issued, which
+            bypasses the cache), tokens acquired with ``forwarded_client_claims``
+            **are cached** and keyed on the claims value. Send the *same* value on
+            every request that should share the cached token; omitting or changing
+            it routes to a different cache entry (a cache miss), so use stable,
             non-dynamic values. The value is merged into the standard OAuth
             ``claims`` request parameter sent on the wire.
 
@@ -2770,11 +2772,12 @@ def acquire_token_by_user_federated_identity_credential(
             returned from the UserInfo Endpoint and/or in the ID Token and/or Access Token.
             It is a string of a JSON object which contains lists of claims being requested from these locations.
         :param str forwarded_client_claims:
-            Optional. A JSON string of *client-originated* claims (for example
-            a network security perimeter ``xms_az_nwperimid`` claim) to include
-            in the token request. Unlike ``claims_challenge`` (server-issued,
-            which bypasses the cache), tokens acquired with ``forwarded_client_claims``
-            **are cached** and keyed on the claims value, so use stable,
+            Optional. A JSON string of *client-originated* claims to include in
+            the token request. Unlike ``claims_challenge`` (server-issued, which
+            bypasses the cache), tokens acquired with ``forwarded_client_claims``
+            **are cached** and keyed on the claims value. Send the *same* value on
+            every request that should share the cached token; omitting or changing
+            it routes to a different cache entry (a cache miss), so use stable,
             non-dynamic values. The value is merged into the standard OAuth
             ``claims`` request parameter sent on the wire.
 

msal/managed_identity.py

@@ -26,8 +26,6 @@ class ManagedIdentityError(ValueError):
     pass
 
 
-_XMS_AZ_NWPERIMID = "xms_az_nwperimid"
-
 _CLIENT_CLAIMS_UNSUPPORTED_SOURCE = (
     "forwarded_client_claims is only supported for the IMDS (Azure VM) managed identity "
     "source. The detected source ({source}) does not support forwarding "
@@ -292,18 +290,17 @@ def acquire_token_for_client(
         :param forwarded_client_claims:
             Optional.
             A string representation of a JSON object containing
-            *client-originated* claims to forward to the identity endpoint
-            (for example a network security perimeter ``xms_az_nwperimid`` claim).
+            *client-originated* claims to forward to the identity endpoint.
 
             Unlike ``claims_challenge`` (server-issued, which bypasses the cache),
             tokens acquired with ``forwarded_client_claims`` **are cached**, and the cache
-            entry is keyed on the claims value. Different ``forwarded_client_claims`` values
-            produce separate cache entries, so use stable, non-dynamic values to
-            avoid unbounded cache growth.
+            entry is keyed on the claims value. Send the *same* value on every
+            request that should share the cached token; different values produce
+            separate cache entries, so use stable, non-dynamic values to avoid
+            unbounded cache growth.
 
             Only the IMDS (Azure VM) managed identity source supports this
-            parameter; other sources raise an error. On IMDS v1, the claims JSON
-            may contain only the ``xms_az_nwperimid`` key.
+            parameter; other sources raise an error.
 
         .. note::
 
@@ -325,6 +322,9 @@ def acquire_token_for_client(
                     "forwarded_client_claims must be a string, got {}".format(
                         type(forwarded_client_claims).__name__))
             _parse_claims_or_raise(forwarded_client_claims)  # Fail fast on malformed JSON
+            # Reject unsupported sources before any cache read, so an unsupported
+            # source never returns a cached client-claims token.
+            _raise_if_claims_unsupported_source()
         # Client-originated claims isolate the cache: a distinct claims value gets
         # a distinct cache entry. (Server-issued claims_challenge, by contrast,
         # bypasses the cache and is keyed normally.)
@@ -447,6 +447,29 @@ def get_managed_identity_source():
     return DEFAULT_TO_VM
 
 
+# Managed-identity sources that cannot forward client-originated claims. Keep in
+# sync with the per-source guards inside _obtain_token (the backstop). Cloud Shell
+# is intentionally absent: it falls through to the Azure VM / IMDS path, which
+# does support claims.
+_CLIENT_CLAIMS_UNSUPPORTED_SOURCES = {
+    SERVICE_FABRIC: "Service Fabric",
+    APP_SERVICE: "App Service",
+    MACHINE_LEARNING: "Machine Learning",
+    AZURE_ARC: "Azure Arc",
+}
+
+
+def _raise_if_claims_unsupported_source():
+    """Fail fast -- before any cache read -- when the detected managed-identity
+    source cannot forward client-originated claims. ``_obtain_token`` enforces the
+    same rule per source as a backstop, but validating up front avoids a cache
+    lookup (and returning a cached token) for an unsupported source."""
+    name = _CLIENT_CLAIMS_UNSUPPORTED_SOURCES.get(get_managed_identity_source())
+    if name:
+        raise ManagedIdentityError(
+            _CLIENT_CLAIMS_UNSUPPORTED_SOURCE.format(source=name))
+
+
 def _obtain_token(
     http_client, managed_identity, resource,
     *,
@@ -521,22 +544,6 @@ def _adjust_param(params, managed_identity, types_mapping=None):
     if id_name:
         params[id_name] = managed_identity[ManagedIdentity.ID]
 
-def _validate_msiv1_claims(client_claims):
-    """MSIv1 (IMDS v1) only supports the single ``xms_az_nwperimid`` custom claim.
-
-    Any other top-level key makes IMDS return HTTP 400 with no useful diagnostic,
-    so validate early and raise a clear error. Mirrors MSAL .NET's
-    ``AbstractManagedIdentity.ValidateMsiv1Claims``.
-    """
-    parsed = _parse_claims_or_raise(client_claims)
-    for key in parsed:
-        if key != _XMS_AZ_NWPERIMID:
-            raise ManagedIdentityError(
-                "MSIv1 (IMDS v1) only supports the `{expected}` custom claim. "
-                "The claims JSON contained the unsupported key `{actual}`. "
-                "Remove all keys other than `{expected}` when using forwarded_client_claims "
-                "with MSIv1.".format(expected=_XMS_AZ_NWPERIMID, actual=key))
-
 
 def _obtain_token_on_azure_vm(http_client, managed_identity, resource, client_claims=None):
     # Based on https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token#get-a-token-using-http
@@ -547,8 +554,8 @@ def _obtain_token_on_azure_vm(http_client, managed_identity, resource, client_cl
         }
     _adjust_param(params, managed_identity)
     if client_claims:
-        # IMDS v1 (MSIv1) only supports the single xms_az_nwperimid claim.
-        _validate_msiv1_claims(client_claims)
+        # Forward client-originated claims as-is; IMDS decides which keys it
+        # accepts (no client-side allow-list, matching the other MSALs).
         params["claims"] = client_claims  # http_client.get url-encodes query params
     resp = http_client.get(
         os.getenv(

msal/token_cache.py

@@ -83,8 +83,15 @@ def _compute_ext_cache_key(data):
 
     Returns an empty string when *data* has no hashable fields.
 
-    The algorithm matches the Go MSAL implementation (CacheExtKeyGenerator):
-    sorted key+value pairs are concatenated and SHA256 hashed, then base64url encoded.
+    The algorithm matches the Go MSAL implementation (CacheExtKeyGenerator,
+    post-collision-fix): length-prefixed key/value pairs (sorted by key) are
+    concatenated and SHA256 hashed, then base64url encoded. The length prefixes
+    make the encoding injective, so two distinct component sets can never collide
+    onto the same cache key. (MSAL .NET's ``ComputeAccessTokenExtCacheKey`` still
+    uses an unprefixed concatenation, so the hash is intentionally not
+    byte-identical to current .NET; the cache *key format* still matches both.
+    Caches are not shared across languages, so this only affects within-process
+    isolation, where injectivity is what matters.)
     """
     if not data:
         return ""
@@ -94,9 +101,16 @@ def _compute_ext_cache_key(data):
     }
     if not cache_components:
         return ""
-    # Sort keys for consistent hashing (matches Go implementation)
+    # Concatenate length-prefixed key/value pairs so component boundaries are
+    # unambiguous (matches Go's CacheExtKeyGenerator). A plain key+value
+    # concatenation with no separators can collide when one value happens to
+    # contain another component's key or value -- and client_claims is arbitrary
+    # caller-supplied JSON that may embed e.g. "fmi_path" at a boundary -- mapping
+    # two distinct component sets onto the same hash and returning the wrong
+    # cached token. Length prefixes make the encoding injective.
     key_str = "".join(
-        k + cache_components[k] for k in sorted(cache_components.keys())
+        "{}:{}{}:{}".format(len(k), k, len(v), v)
+        for k, v in sorted(cache_components.items())
     )
     hash_bytes = hashlib.sha256(key_str.encode("utf-8")).digest()
     return base64.urlsafe_b64encode(hash_bytes).rstrip(b"=").decode("ascii").lower()

tests/test_application.py

@@ -1063,7 +1063,32 @@ def mock_post(url, headers=None, data=None, *args, **kwargs):
             "claims_challenge, capabilities, and forwarded_client_claims must all merge")
         self.assertNotIn("client_claims", captured_data)
 
-    def test_same_client_claims_returns_cached_token(self):
+    def test_forwarded_client_claims_win_on_leaf_conflict_with_challenge(self):
+        # If the server-issued claims_challenge and forwarded_client_claims set
+        # the SAME claim, the client-originated value wins (it is merged in last),
+        # while disjoint claims from the challenge are preserved. Documents the
+        # conflict-resolution behavior the other MSAL reviewers asked about.
+        app = self._build_app()
+        captured_data = {}
+
+        def mock_post(url, headers=None, data=None, *args, **kwargs):
+            captured_data.update(data or {})
+            return MinimalResponse(status_code=200, text=json.dumps({
+                "access_token": "an AT", "expires_in": 3600}))
+
+        challenge = ('{"access_token": {"acrs": {"values": ["server"]},'
+                     ' "nbf": {"essential": true}}}')
+        client = '{"access_token": {"acrs": {"values": ["client"]}}}'
+        app.acquire_token_for_client(
+            ["scope"], claims_challenge=challenge,
+            forwarded_client_claims=client, post=mock_post)
+        merged = json.loads(captured_data["claims"])["access_token"]
+        self.assertEqual(
+            {"values": ["client"]}, merged["acrs"],
+            "forwarded_client_claims must win a direct leaf conflict")
+        self.assertEqual(
+            {"essential": True}, merged["nbf"],
+            "Disjoint claims from the challenge must be preserved")
         app = self._build_app()
         call_count = [0]
 

tests/test_mi.py

@@ -288,11 +288,18 @@ def test_no_claims_param_when_client_claims_absent(self):
             self.app.acquire_token_for_client(resource="R")
             self.assertNotIn("claims", mock_get.call_args.kwargs["params"])
 
-    def test_msiv1_rejects_non_nwperimid_claim(self):
-        with self.assertRaises(ManagedIdentityError):
-            self.app.acquire_token_for_client(
-                resource="R",
-                forwarded_client_claims='{"some_other_claim": {"essential": true}}')
+    def test_non_nwperimid_claim_is_forwarded_not_rejected(self):
+        # MSAL no longer enforces a client-side allow-list (matching the other
+        # MSALs); any JSON-object claims value is forwarded as-is and IMDS decides
+        # which keys it accepts.
+        other = '{"some_other_claim": {"essential": true}}'
+        with self._mock_get() as mock_get:
+            result = self.app.acquire_token_for_client(
+                resource="R", forwarded_client_claims=other)
+            self.assertIn("access_token", result)
+            self.assertEqual(
+                other, mock_get.call_args.kwargs["params"].get("claims"),
+                "Non-nwperimid claims must be forwarded, not rejected")
 
     def test_invalid_json_claims_raises(self):
         for bad in ["not json", "[1, 2]", "null"]: