Skip to content

Validate issuer and audience of Azure AD id_tokens in FAB auth manager - #71735

Merged
eladkal merged 1 commit into
apache:mainfrom
potiuk:fab-azure-validate-issuer-audience
Aug 18, 2026
Merged

Validate issuer and audience of Azure AD id_tokens in FAB auth manager#71735
eladkal merged 1 commit into
apache:mainfrom
potiuk:fab-azure-validate-issuer-audience

Conversation

@potiuk

@potiuk potiuk commented Aug 17, 2026

Copy link
Copy Markdown
Member

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 already puts it:

"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

@potiuk potiuk added the type:bug-fix Changelog: Bug Fixes label Aug 17, 2026
@eladkal eladkal removed the type:bug-fix Changelog: Bug Fixes label Aug 18, 2026
The Azure id_token signature is verified against Microsoft's key set, but
the decode call passed no claims_options, so authlib's claims.validate()
enforced 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 satisfied the signature check, and get_oauth_user_info() then
read the login identity (oid, email, roles) straight out of it.

Pin both claims:

* 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.
* aud must be this application's client_id.

The tenant is taken 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 already puts it. Deployments that
follow the documented setup therefore need no configuration change.

A configuration that identifies no single tenant - the common,
organizations or consumers endpoints - now raises rather than accepting
tokens it cannot attribute to an issuer. That is a behaviour change for
those deployments: they need to set tenant_id explicitly.

The existing test that asserted the verification branch is reached by
default now supplies a tenant-bearing endpoint, since tenant resolution
happens before the key set is fetched.

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
@eladkal
eladkal force-pushed the fab-azure-validate-issuer-audience branch from 09ff60a to 75ecbdd Compare August 18, 2026 13:07
@eladkal
eladkal merged commit 7b94de7 into apache:main Aug 18, 2026
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants