bingqin2 commented on code in PR #72954:
URL: https://github.com/apache/airflow/pull/72954#discussion_r4189748257
##########
providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py:
##########
@@ -399,7 +399,10 @@ def
_get_credentials_using_credential_config_file_and_token_supplier(self):
info =
_get_info_from_credential_configuration_file(self.credential_config_file)
info["subject_token_supplier"] =
ClientCredentialsGrantFlowTokenSupplier(
- oidc_issuer_url=self.idp_issuer_url, client_id=self.client_id,
client_secret=self.client_secret
+ oidc_issuer_url=self.idp_issuer_url,
Review Comment:
This path doesn't involve custom suppliers. The extra parameters go to the
constructor of `ClientCredentialsGrantFlowTokenSupplier`, the provider's own
class, which already accepted them as `**extra_params_kwargs` (the hook just
never passed them). `get_subject_token(context, request)` keeps the signature
google-auth calls, and
`_get_credentials_using_credential_config_file_and_token_supplier` always
builds this class, so a user-defined `SubjectTokenSupplier` never sees these
arguments.
What could break at this line was a reserved name in the extras: `client_id`
or `client_secret` failed with a confusing `TypeError` (multiple values for a
keyword argument). The change for your next comment turns that into a clear
error.
##########
providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py:
##########
@@ -399,7 +399,10 @@ def
_get_credentials_using_credential_config_file_and_token_supplier(self):
info =
_get_info_from_credential_configuration_file(self.credential_config_file)
info["subject_token_supplier"] =
ClientCredentialsGrantFlowTokenSupplier(
- oidc_issuer_url=self.idp_issuer_url, client_id=self.client_id,
client_secret=self.client_secret
+ oidc_issuer_url=self.idp_issuer_url,
+ client_id=self.client_id,
+ client_secret=self.client_secret,
+ **(self.idp_extra_params_dict or {}),
Review Comment:
Good point, done. The supplier merges the extras last, so `grant_type` could
replace `client_credentials`, and `client_id` / `client_secret` failed with a
`TypeError`. Now the credentials provider rejects `grant_type`, `client_id` and
`client_secret` with a `ValueError` that points to the connection fields,
before any request is made, and the supplier rejects `grant_type` for direct
callers too (`RESERVED_TOKEN_REQUEST_FIELDS`).
I left `audience` and `subject_token_type` out on purpose.
`subject_token_type` and the STS `audience` come from the credential
configuration file, which the extras never touch. In the IdP request itself,
`audience` is a legitimate extra: Auth0 requires it for client-credentials
grants, as Entra ID requires `scope`.
##########
providers/google/src/airflow/providers/google/common/hooks/base_google.py:
##########
@@ -360,7 +360,11 @@ def get_credentials_and_project_id(self) ->
tuple[Credentials, str | None]:
idp_issuer_url: str | None = self._get_field("idp_issuer_url", None)
client_id: str | None = self._get_field("client_id", None)
client_secret: str | None = self._get_field("client_secret", None)
- idp_extra_params: str | None = self._get_field("idp_extra_params",
None)
+ # The connection form saves this field as ``idp_extra_parameters``;
``idp_extra_params`` is
+ # still accepted for extras that were written by hand against the
previous field name.
+ idp_extra_params: str | None = self._get_field("idp_extra_parameters",
None) or self._get_field(
Review Comment:
Done for the task logs: after parsing, the hook passes the dict to
`mask_secret`, which walks it and masks the values under sensitive key names
(`*secret*`, `*token*`, `*password*` and the rest of the default list, plus
anything added through `[core] sensitive_var_conn_names`), the same rule
connection extras follow. Masking every value would also hide things like
`openid` or an audience URL wherever they appear in logs. Nothing in this path
logs the payload: the supplier logs only that it is requesting a token, and
HTTP errors carry the URL, not the body.
In the connection form this stays a plain text field like the other
non-secret extras; the client secret already has its own masked field, which is
where secrets belong. I also added a check that the field holds a JSON object,
since a JSON list or string would otherwise fail later with an unclear error.
--
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]
