| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
I validated this against real Microsoft Entra ID tokens from a production tenant, running them through the parser and a RoleBasedPolicy check. Confirmed:
One interop detail others may hit: it is valid to set auth_discovery_url to the v2.0 endpoint while the tenant still issues v1.0 tokens (iss: https://sts.windows.net/...). It works because the parser uses discovery only to fetch JWKS and Entra shares signing keys across v1 and v2, and because it runs with verify_aud=False and no issuer check. If stricter iss/aud validation is added later, it would need to allow v1.0 tokens against a v2.0 discovery document. |
Sorry, something went wrong.
|
/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 roles claim. The diff is the two parser changes plus tests and a docs note, and unit tests pass. As an outside contributor I can't self-label or start CI. If the /kind bug above doesn't take, could a maintainer add the label and run ok-to-test when convenient? |
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #6631 +/- ##
==========================================
+ Coverage 45.86% 45.88% +0.01%
==========================================
Files 414 414
Lines 49948 49950 +2
Branches 7140 7140
==========================================
+ Hits 22911 22918 +7
+ Misses 25435 25431 -4
+ Partials 1602 1601 -1
Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
@aniketpalu Can you please take a look ? |
Sorry, something went wrong.
|
Heads up on the red unit-test-python (3.12, ubuntu-latest) check here: it is a pre-existing flaky collection error in test_mcp_server.py, a KeyError importing feast.infra.mcp_servers.mcp_server under parallel collection. It is unrelated to the changes in this PR. I addressed it separately at its root in #6634 (issue #6633) to clear the blocker. The tests in this PR pass on the other CI matrices and locally. |
Sorry, something went wrong.
There was a problem hiding this comment.
PR looks good. Few nitpicks. Thanks for the important contribution @larrysingleton007
Sorry, something went wrong.
|
Thanks for the review @aniketpalu. All three suggestions are applied in 25c364f: typed the discovery_data fixture as Dict[str, str], split the Entra group-claim caveat into its own bullet, and added the debug log of the token's claim keys before the missing-username raise. Threads resolved, ready for another look when you have a moment. |
Sorry, something went wrong.
|
Thanks @larrysingleton007, LGTM. Please rebase. |
Sorry, something went wrong.
|
Rebased onto the latest master (clean, no conflicts). Thanks for the review @aniketpalu. |
Sorry, something went wrong.
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>
- Type the discovery_data test fixture as Dict[str, str]. - Split the Entra group-claim caveat into its own bullet in authz_manager.md. - Log the token's claim keys at debug level before raising on a missing username claim, to aid diagnosis (keys only, no values). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
…#6631) * fix: Support Entra ID (Azure AD) token claims in OIDC auth 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> * docs: Document Entra ID claim support in OIDC authorization 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> * fix: Harden Entra ID OIDC claim handling from review 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> * docs: Add Entra ID token example and group-claim caveat 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> * test: Cover OIDC verified-decode sourcing and trim redundant asserts 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> * test: Extract shared OIDC discovery and signing-key fixtures 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> * fix: Address review nitpicks on Entra OIDC PR - Type the discovery_data test fixture as Dict[str, str]. - Split the Entra group-claim caveat into its own bullet in authz_manager.md. - Log the token's claim keys at debug level before raising on a missing username claim, to aid diagnosis (keys only, no values). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
# [0.66.0](v0.65.0...v0.66.0) (2026-08-21) ### Bug Fixes * Add connection pre-warming for DynamoDB async client ([89240fa](89240fa)), closes [#6060](#6060) * Add remote registry client extra ([#6697](#6697)) ([b8dfcb0](b8dfcb0)) * Address review feedback on FIPS cipher suite configuration ([4a35fba](4a35fba)) * Allow remote-registry first apply for new projects ([39d408d](39d408d)) * Avoid importing feast.feature_store at mcp_server import time ([ddb2e9a](ddb2e9a)) * Bump pymssql to >=2.3.6 for macOS arm64 wheel support ([181eb35](181eb35)), closes [#5636](#5636) [#5193](#5193) [#5636](#5636) * Call ApplySavedDataset RPC instead of ApplyFeatureService in RemoteRegistry.apply_saved_dataset() ([934d341](934d341)) * Catch missing dbt parser dependency in dbt CLI commands ([#6534](#6534)) ([3c2ae3c](3c2ae3c)) * Default authentication to kubernetes auth ([6a4690a](6a4690a)) * Defer feature-freshness thread to post-fork to avoid Gunicorn deadlock ([#6648](#6648)) ([104ad10](104ad10)), closes [#6647](#6647) * Do not pass undeclared feature view columns to ODFV UDFs ([#6527](#6527)) ([75b9463](75b9463)) * downgrade mcp pin to 1.29.0 and fix CI lockfiles and unit tests ([98e5bca](98e5bca)), closes [#6706](#6706) * Feast apply silently ignoring ttl updates to None or timedelta(0) ([#6709](#6709)) ([97b0f25](97b0f25)), closes [#6703](#6703) * Fix mypy TorchTensor type alias error ([#6712](#6712)) ([34de6fa](34de6fa)), closes [#5563](#5563) * Fixed data source creation form gaps ([5d0f7d6](5d0f7d6)) * Handle parameterized and complex Trino types in type map ([326554d](326554d)) * Isolate default user permissions ([e37adbf](e37adbf)) * Isolate projection join key maps ([d1c709d](d1c709d)) * Map Postgres real to FLOAT instead of DOUBLE ([62db435](62db435)) * Merge shared ODFV source projections in feature resolution ([d269946](d269946)), closes [#6621](#6621) * More exhaustive athena types ([a9aaefc](a9aaefc)) * Normalize SQL registry read_path to the psycopg3 driver like path ([#6644](#6644)) ([996c6ea](996c6ea)), closes [#6643](#6643) * **operator:** add spec.services.onlineStore.disabled to opt out of the online store ([d81d4e3](d81d4e3)), closes [#6586](#6586) * Preinstall DuckDB delta extension for tests ([fd4d49d](fd4d49d)), closes [#6743](#6743) * Preserve event-time ordering within Redis online_write_batch ([40fb788](40fb788)), closes [#5163](#5163) * Prevent mutation of cached feature resolution results ([ea17419](ea17419)) * Remote feastRef FeatureStore fails first apply for a new feastProject ([9affee5](9affee5)) * Remove inert subjectaccessreviews and reorganize RBAC rules ([f771ea4](f771ea4)) * Report single-feature-view spark_application materialization success ([a9219d9](a9219d9)), closes [#6673](#6673) * Reset the global security manager after the permissions fixture ([7667215](7667215)) * Resolve kserve with pip --dry-run instead of installing it ([01da132](01da132)), closes [#6732](#6732) * Resolve write_to_offline_store feature view with a single registry lookup ([a42dc85](a42dc85)), closes [#4235](#4235) * Return False from __eq__ on cross-type comparison ([#6637](#6637)) ([0f149a9](0f149a9)), closes [#6636](#6636) * Reuse IdP-issued client tokens until near expiry ([602d752](602d752)) * Reuse the OIDC JWKS client across requests ([#6683](#6683)) ([a1e6fc2](a1e6fc2)) * Separate CronJob and feature-server ServiceAccounts ([398f643](398f643)) * Serialize UnixTimestamp proto values as raw int64 in remote online store transport ([1e7134f](1e7134f)) * Set FIPS cipher suites before pyarrow.flight import to prevent crash on IBM Power ([979b82a](979b82a)) * Support Entra ID (Azure AD) token claims in OIDC auth ([#6631](#6631)) ([f843c63](f843c63)) * UDF/ODFV source rehydrate (+ Postgres / online cache) ([#6655](#6655)) ([5fd7af7](5fd7af7)) * Updated projects-list.json in order to display newly added projects ([#6657](#6657)) ([3a6a103](3a6a103)) * Use correct image name in multi-arch imagetools push step ([faf85e0](faf85e0)) * Use join keys instead of entity names in ODFV materialization ([#6645](#6645)) ([abffebc](abffebc)), closes [#5965](#5965) * use matching proto class per feature view list in SqliteOnlineStore.plan() ([adb8c1c](adb8c1c)), closes [#6658](#6658) * Widen Athena integer type mapping for unsigned ints ([3425783](3425783)) ### Features * Add ConnectionRef to DataSource for pluggable external credential resolution ([28bde01](28bde01)) * Add Feature Service Create in UI ([0399380](0399380)) * Add hybrid to ValidOfflineStoreDBStorePersistenceTypes for HybridOfflineStore support ([#6707](#6707)) ([310ab51](310ab51)), closes [#6701](#6701) * Add MLflow integration support to Feast operator ([#6611](#6611)) ([52999f1](52999f1)) * Add opt-in filter_by_created_timestamp cutoff to get_historical_features ([#6617](#6617)) ([79b33ce](79b33ce)), closes [#6615](#6615) * Add optional OIDC token audience and issuer verification ([#6670](#6670)) ([ef307c6](ef307c6)) * Add packaged feature repository support to Feast Operator ([8112b1e](8112b1e)), closes [#6598](#6598) * add plan() support to DynamoDBOnlineStore ([51ce982](51ce982)), closes [#6658](#6658) [#6659](#6659) * Added optional namespace/colleciton to datasets ([165fcf2](165fcf2)) * Added SQL registry schema_mode and registry create command ([#6704](#6704)) ([037c4cd](037c4cd)) * Allow users to have protected project on shared registry ([f9923bc](f9923bc)) * Apply Intermediate TLS defaults on API fallback and handle transient errors ([#6587](#6587)) ([43ae993](43ae993)) * **cli:** Updated feast init demo by adding rag template ([#5946](#5946)) ([c8628eb](c8628eb)), closes [#5264](#5264) * Expose the OIDC JWKS tunables through the operator ([#6690](#6690)) ([fef4e78](fef4e78)), closes [#6683](#6683) * Making feast vector store with open ai search api compatible ([#6121](#6121)) ([54da19a](54da19a)) * Multi-arch publish for feast operator image ([b221036](b221036)) * OpenLineage lineage enhancements - full object coverage, richer UI, and API-level sync ([#6719](#6719)) ([120a868](120a868)) * **operator:** Add spec.services.initImage for init container image override ([#6598](#6598)) ([ca355cb](ca355cb)) * Pass optional OIDC audience and issuer through the operator ([#6677](#6677)) ([a13ed7b](a13ed7b)), closes [#6670](#6670) * **server:** Remote Materialization ([#6649](#6649)) ([b7ae488](b7ae488)), closes [#4526](#4526) * Support Lineage configs via operator ([bf1e54a](bf1e54a)) * Updated datasets UI to support grouping ([7ae64ec](7ae64ec))
| Back | FazBrowse Home | New Git URL |
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_username or upn. 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: azp on v2 tokens, appid on v1, then sub, 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 raises AuthenticationError.
Roles were read only from Keycloak's nested resource_access.<client_id>.roles. Entra puts app roles in the top-level roles claim, so the extracted list was always empty and RoleBasedPolicy never 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_username and upn are 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-level roles claim for every issuer, not only Entra. An installation whose IdP already emits a top-level roles claim (for example a Keycloak realm with a custom mapper) will see those roles merged into the user's Feast roles. The Keycloak resource_access.<client_id>.roles path 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
Testing Strategy
New unit tests in sdk/python/tests/unit/permissions/auth/test_token_parser.py cover: preferred_username and upn precedence over the application claims (including directly against appid/sub); azp-only, appid-only, and sub-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; Keycloak resource_access roles 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, or sub for client-credentials (app-only) tokens, and app roles in the top-level roles claim are merged with Keycloak's resource_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 (and ok-to-test for 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.