fpopic commented on code in PR #53801:
URL: https://github.com/apache/airflow/pull/53801#discussion_r2997642445
##########
providers/hashicorp/src/airflow/providers/hashicorp/_internal_client/vault_client.py:
##########
@@ -352,25 +348,38 @@ def _auth_gcp(self, _client: hvac.Client) -> None:
import json
import time
- import googleapiclient
+ # Determine service account email
+ service_account_email = getattr(credentials, "service_account_email",
None)
+ if not service_account_email or not isinstance(service_account_email,
str):
+ service_account_email = getattr(credentials, "client_email", None)
+
+ if not service_account_email or not isinstance(service_account_email,
str):
+ # Fallback for Compute Engine credentials if email is not yet
populated
+ try:
+ from google.auth import compute_engine
- if self.gcp_keyfile_dict:
- creds = self.gcp_keyfile_dict
- elif self.gcp_key_path:
- with open(self.gcp_key_path) as f:
- creds = json.load(f)
+ if isinstance(credentials, compute_engine.Credentials):
+ if not getattr(credentials, "service_account_email", None):
+ from google.auth import transport
- service_account = creds["client_email"]
+ credentials.refresh(transport.requests.Request())
+ service_account_email = credentials.service_account_email
+ except Exception:
+ pass
+
+ if not service_account_email:
+ raise VaultError("Could not determine service account email from
credentials")
# Generate a payload for subsequent "signJwt()" call
- # Reference:
https://googleapis.dev/python/google-auth/latest/reference/google.auth.jwt.html#google.auth.jwt.Credentials
now = int(time.time())
expires = now + 900 # 15 mins in seconds, can't be longer.
- payload = {"iat": now, "exp": expires, "sub": credentials, "aud":
f"vault/{self.role_id}"}
+ payload = {"iat": now, "exp": expires, "sub": service_account_email,
"aud": f"vault/{self.role_id}"}
Review Comment:
> Hmmmm, what is the impact of this change? Was this just never working?
https://github.com/apache/airflow/issues/17500#:~:text=It%27s%20a%20bold%20statement%20as%20it%20means%20nobody%20has%20really%20used%20VaultBackend%20with%20these%20auth%20types.%20I%20may%20be%20wrong%2C%20but%20I%20would%20be%20surprised
:
> It's a bold statement as it means nobody has really used VaultBackend with
these auth types. I may be wrong, but I would be surprised.
Can totally be. 😓
The previous implementation was passing the entire `credentials` object to
`json.dumps()`, which resulted in a `TypeError` during serialization. This
means the `gcp` auth type was likely non-functional for any signed JWT flow
until now. I've corrected it to pass the service account email string, which
aligns with the GCP `signJwt` API requirements.
--
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]
