potiuk opened a new pull request, #71735: URL: https://github.com/apache/airflow/pull/71735
The Azure `id_token` signature is verified against Microsoft's key set, but the decode call passes no `claims_options`, so authlib's `claims.validate()` enforces neither the **issuer** nor the **audience**. The key set in use is the multi-tenant one — `login.microsoftonline.com/common/discovery/keys` — which serves signing keys for every Azure tenant. A correctly-signed token from *any* tenant therefore satisfies the signature check, and `get_oauth_user_info()` then reads the login identity (`oid`, `email`, `roles`) straight out of it. ### What this changes Both claims are now pinned: * `iss` must be the configured tenant, accepting the v1.0 (`sts.windows.net/<tenant>/`) and v2.0 (`login.microsoftonline.com/<tenant>/v2.0`) issuer forms, since either may be returned depending on which endpoints are configured. * `aud` must be this application's `client_id`. The tenant is resolved from an explicit `tenant_id` in `client_kwargs` when set, and otherwise from the tenant segment of the configured endpoints — which is where the [documented configuration](https://github.com/apache/airflow/blob/main/providers/fab/docs/auth-manager/sso.rst) already puts it: ```python "api_base_url": "https://login.microsoftonline.com/<tenant-id>/oauth2/v2.0/", ``` **Deployments following the documented setup need no configuration change.** ### Behaviour change worth reviewing A configuration that identifies no single tenant — the `common`, `organizations` or `consumers` endpoints — now raises `AirflowConfigException` rather than accepting tokens whose issuer it cannot check. Those deployments must set `tenant_id` explicitly. I chose fail-closed deliberately: a silent fallback would leave exactly the behaviour this PR is removing. But it is a startup-time break for multi-tenant configurations, so it is the main thing I would like a second opinion on. The alternative is to log loudly and continue, which I think is worse but is a defensible call. ### Three things I would like your view on 1. **Fail-closed vs. warn-and-continue** for the tenant-less case, as above. 2. **Where `tenant_id` comes from.** I added it as an optional `client_kwargs` key and fall back to parsing the endpoints. Parsing-only would mean no new config surface at all; explicit-only would be cleaner but breaks every existing deployment. The current shape tries to get both. 3. **Both issuer forms.** I accept v1.0 and v2.0. If Airflow only ever requests v2.0 tokens, the `sts.windows.net` entry should go. ### Also worth noting `_validate_jwt()` (the Authentik path, same file) has the identical `authlib_jwt.decode(id_token, keyset)` shape with no `claims_options`. Its blast radius is smaller because the Authentik JWKS is deployment-specific rather than multi-tenant, so there is no cross-issuer concern — but `aud` is still unchecked there. I left it out to keep this diff reviewable; happy to fold it in here or do it separately, whichever you prefer. ### Testing Added coverage for tenant resolution (explicit, from `api_base_url`, from `access_token_url`, and the tenant-agnostic endpoints), for the fail-closed path, and for the claim options actually passed to `decode`. The existing `test_decode_and_validate_azure_jwt_verifies_signature_by_default` needed updating: tenant resolution now happens before the key set is fetched, so the mock had to grow a tenant-bearing endpoint. All 42 tests in `test_override.py` pass locally. --- Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
