fix: Support Entra ID (Azure AD) token claims in OIDC auth#6631
fix: Support Entra ID (Azure AD) token claims in OIDC auth#6631larrysingleton007 wants to merge 6 commits into
Conversation
The OIDC token parser was written against Keycloak's token shape, which rejected Microsoft Entra ID tokens in two independent ways. Username extraction accepted only preferred_username or upn. Entra client-credentials (app-only) tokens carry neither, so every machine-to-machine caller failed authentication. Fall back to the calling application's own identity when no human claim is present: azp on v2 tokens, appid on v1, then sub, which every issuer sets. A token with none of the five claims still raises AuthenticationError. Roles were read only from Keycloak's nested resource_access.<client_id>.roles. Entra emits app roles in the top-level roles claim, so the extracted list was always empty and RoleBasedPolicy could never match. Merge the top-level claim into the existing extraction, preserving order and dropping duplicates. Both changes are additive, so Keycloak behavior is unchanged: the username fallbacks only fire when preferred_username and upn are both absent, and the merge only adds roles. Token validation is untouched. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
The OIDC assumptions listed for the auth manager described only Keycloak's token shape. Record that roles are also read from the top-level roles claim and merged, and that the username falls back through upn, azp, appid and sub when preferred_username is absent. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Refine the initial Entra ID support in response to review: - Merge roles with list(dict.fromkeys(...)), the codebase's order- preserving dedup idiom (feast/utils.py), so duplicates within the top-level roles claim are also collapsed, not only cross-claim ones. - Skip username claims whose value is null or non-string instead of returning them, so a present-but-null early claim no longer shadows a usable later claim; the -> str contract now holds. - Correct the _extract_username_or_raise_error docstring: the raise fires only when a token provides none of the five claims as a string. Extend the tests: intra-claim role de-duplication, human-claim precedence over appid/sub, and rejection of tokens whose only identity claims are null or non-string. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Add an app-only (client-credentials) Entra ID token example alongside the Keycloak one, and note that Entra emits group object IDs (GUIDs) rather than names and omits the groups claim under the overage limit, so GroupBasedPolicy on Entra must reference those IDs. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Add a test that gives the unverified routing decode and the verified decode different payloads and asserts identity and roles come from the verified one, so a regression that read claims from the unverified decode would be caught. Drop the has_matching_role assertions in the roles-merge test: the exact roles-list equality already pins the result, and has_matching_role has its own coverage in test_user.py. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
The OIDC token-parser tests each re-declared the same mock discovery document and JWKS signing key. Move both into conftest.py fixtures and have the tests consume them, removing the repeated setup blocks. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
|
I validated this against real Microsoft Entra ID tokens from a production tenant, running them through the parser and a Confirmed:
One interop detail others may hit: it is valid to set |
|
/kind bug Small, self-contained fix for a filed issue (#6630): the OIDC parser now accepts Microsoft Entra ID (Azure AD) tokens, with a username fallback for app-only tokens and support for the top-level As an outside contributor I can't self-label or start CI. If the |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6631 +/- ##
==========================================
+ Coverage 45.74% 45.76% +0.01%
==========================================
Files 414 414
Lines 49668 49669 +1
Branches 7078 7078
==========================================
+ Hits 22723 22729 +6
+ Misses 25366 25362 -4
+ Partials 1579 1578 -1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@aniketpalu Can you please take a look ? |
|
Heads up on the red |
What this PR does / why we need it:
The OIDC token parser is written against Keycloak's token shape, and that breaks Microsoft Entra ID (Azure AD) in two independent places.
Username extraction accepted only
preferred_usernameorupn. Entra client-credentials (app-only) tokens carry neither, so every machine-to-machine caller was rejected at authentication. This falls back to the calling application's own identity when no human claim is present:azpon v2 tokens,appidon v1, thensub, which every issuer sets. A claim that is present but null or non-string is skipped rather than returned, and a token providing none of the five claims as a string still raisesAuthenticationError.Roles were read only from Keycloak's nested
resource_access.<client_id>.roles. Entra puts app roles in the top-levelrolesclaim, so the extracted list was always empty andRoleBasedPolicynever matched. This merges the top-level claim into the existing extraction, preserving order and dropping duplicates.Keycloak's default behavior is unchanged: the username fallbacks only fire when
preferred_usernameandupnare both absent, the merge only adds roles, and token validation (signature and expiry) is untouched. One behavior change worth calling out for existing deployments: role resolution now also reads the top-levelrolesclaim for every issuer, not only Entra. An installation whose IdP already emits a top-levelrolesclaim (for example a Keycloak realm with a custom mapper) will see those roles merged into the user's Feast roles. The Keycloakresource_access.<client_id>.rolespath itself is unchanged.We run this exact change against an Entra tenant in production as a build-time patch on 0.64.
Which issue(s) this PR fixes:
Fixes #6630. Supersedes #4934, which requested the roles half and was closed stale.
Checks
git commit -s)Testing Strategy
New unit tests in
sdk/python/tests/unit/permissions/auth/test_token_parser.pycover:preferred_usernameandupnprecedence over the application claims (including directly againstappid/sub);azp-only,appid-only, andsub-only tokens each authenticating with that value; tokens whose only identity claims are null or non-string still raising; top-level roles extracted on their own; Keycloakresource_accessroles still working; the de-duplicated merge when both shapes are present, including duplicates within the top-level claim; and that identity and roles are read from the signature-verified decode rather than the unverified routing decode.Misc
Release note: OIDC authentication now accepts Microsoft Entra ID (Azure AD) tokens. The username falls back to
azp,appid, orsubfor client-credentials (app-only) tokens, and app roles in the top-levelrolesclaim are merged with Keycloak'sresource_access.<client_id>.roles.The PR template here does not have an auto-generated release-note field, so the note is inline above. I can't apply labels as an outside contributor, so a maintainer will need to add
kind/bug(andok-to-testfor CI).The last two
test:commits are self-contained (a fidelity test plus a fixture extraction) and can be dropped without affecting the fix if a smaller diff is preferred.