Skip to content

Guard and mask Google Cloud IdP extra request parameters - #72954

Open
bingqin2 wants to merge 1 commit into
apache:mainfrom
bingqin2:google-idp-extra-params
Open

Guard and mask Google Cloud IdP extra request parameters#72954
bingqin2 wants to merge 1 commit into
apache:mainfrom
bingqin2:google-idp-extra-params

Conversation

@bingqin2

@bingqin2 bingqin2 commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

#76161 made the "IdP Extra Request Parameters" of a Google Cloud connection reach the identity provider's token request. Now that these user-supplied values are sent, this PR adds three guards:

  • It rejects extras named grant_type, client_id, client_secret or oidc_issuer_url with a clear ValueError, before the token supplier is built.
    • The supplier merges extras last, so a grant_type extra could replace client_credentials.
    • The other three collide with the supplier's named arguments, so on main they fail with TypeError: got multiple values for keyword argument.
    • The supplier also rejects grant_type itself, for code that builds it directly.
    • audience and scope stay allowed, since IdPs such as Auth0 and Entra ID require them.
  • It masks values under sensitive key names in task logs through mask_secret, the same rule connection extras follow.
  • It requires the field to hold a JSON object. A list or a scalar now fails with a clear error, instead of a TypeError when the value is unpacked.

This PR originally also fixed the field name, the passing of the extras to the supplier and the cache key. #76161 fixed those in the meantime, so the rebase keeps main's version of them.

Changes

  • providers/google/src/airflow/providers/google/cloud/utils/external_token_supplier.py: adds RESERVED_TOKEN_REQUEST_FIELDS and checks it in ClientCredentialsGrantFlowTokenSupplier.__init__.
  • providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py: rejects reserved names and oidc_issuer_url before building the supplier.
  • providers/google/src/airflow/providers/google/common/hooks/base_google.py: requires a JSON object and calls mask_secret on the parsed dict. mask_secret comes from airflow.providers.common.compat.sdk, which exports it since common-compat 1.23.0, the provider's minimum.
  • Tests:
    • credentials provider: reserved names and oidc_issuer_url are rejected.
    • token supplier: grant_type is rejected; the request payload keeps the supplier's own fields next to the extras.
    • base hook: masking, non-object JSON, invalid JSON.

Testing

  • providers/google: tests/unit/google/cloud/utils/test_credentials_provider.py, tests/unit/google/cloud/utils/test_external_token_supplier.py and tests/unit/google/common/hooks/test_base_google.py on main 0e7068d.
    • Result: 161 passed, 2 skipped. The two skipped tests need default Google Cloud credentials.
    • Without the source changes, 7 of the new tests fail. The other two cover existing behavior: the token payload and invalid JSON.
  • ruff and mypy on the changed files.
Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Claude Fable 5.1, Claude Opus 5.5) following the guidelines. I reviewed and understand all changes; the tests were run locally as listed above.

🤖 Generated with Claude Code

@bingqin2
bingqin2 requested a review from shahar1 as a code owner September 11, 2026 13:53
@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Sep 11, 2026
@bingqin2 bingqin2 mentioned this pull request Sep 11, 2026
Open
7 of 12 tasks
@bingqin2 bingqin2 mentioned this pull request Sep 30, 2026
Merged
1 task done
@bingqin2

bingqin2 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @VladaZakharova. Pushed: reserved names in the extras (grant_type, client_id, client_secret) now get a clear error, values under sensitive key names are masked in task logs, and the field must hold a JSON object. Details are in the threads.

Looking at the supplier for your second point, I also found that its token cache key used the extra parameters' names but not their values, so two connections that differ only in audience could be handed each other's token within one process. The key now includes the values, with a test that fails on main. The token supplier, credentials provider and base hook tests pass (153).

@bingqin2
bingqin2 force-pushed the google-idp-extra-params branch from e7c39da to be55ad3 Compare October 5, 2026 23:05
shahar1
shahar1 reviewed Oct 31, 2026
View reviewed changes

@shahar1 shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is right: the connection form saves idp_extra_parameters while main reads idp_extra_params, so the field never reached the IdP, and the new guards (reserved names, JSON-object check, mask_secret, value-aware cache key) each have tests. I checked that mask_secret on a dict masks only values under sensitive key names, and that the extras reach only the form payload of the POST to the IdP URL, not a client-library constructor, so the "connection extras must not be forwarded blindly" rule in providers/AGENTS.md is not engaged. One optional nit inline. VladaZakharova's three threads are answered and resolved; a re-review from her would clear the standing change request.

Smaller observations

  • See inline comments on providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py:402.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

More on how Apache Airflow handles maintainer review:
Contributing guide.

Comment thread providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py Resolved
@bingqin2

bingqin2 commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

@VladaZakharova, your three threads are answered and resolved, and 12a9bbc adds the oidc_issuer_url check shahar1 suggested. Could you take another look when you have time? If you're happy with it, approving would clear your change request.

@shahar1

shahar1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

@bingqin2 Thanks for fixing, could you please resolve conflicts?

@bingqin2 @claude
Guard and mask Google Cloud IdP extra request parameters
d1db7fe 
Since apache#76161 the IdP extra request parameters of a Google Cloud connection reach the token
request. Extras that would replace grant_type, client_id or client_secret, or collide with the
supplier's oidc_issuer_url argument, now fail with a clear ValueError; the field must hold a JSON
object; and values under sensitive key names are masked in task logs.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@bingqin2
bingqin2 force-pushed the google-idp-extra-params branch from 12a9bbc to d1db7fe Compare November 7, 2026 17:32
@bingqin2 bingqin2 changed the title Pass IdP extra request parameters through to the Google token supplier Guard and mask Google Cloud IdP extra request parameters Nov 7, 2026
@bingqin2

bingqin2 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

@shahar1 Rebased onto current main. In the meantime, #76161 merged the core of the original fix: the field name, passing the extras to the supplier, and a value-aware cache key. I kept main's version of those, so this PR now only adds the guards on top: rejecting reserved names (including oidc_issuer_url), the JSON-object check, and mask_secret. The guards are the same code the earlier review threads looked at. The branch is now a single commit, and I updated the title and description to match.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahar1 shahar1 shahar1 left review comments
+1 more reviewer
@VladaZakharova VladaZakharova VladaZakharova requested changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bingqin2 @shahar1 @VladaZakharova