Repository navigation
feat(auth): add bound token support for access and JWT id tokens for Cloud Run - #17698
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Google Auth library to request bound tokens from the Compute Engine metadata server using a POST request with the certificate chain in the body, rather than passing a fingerprint in the URL. To support this, get_agent_identity_certificate_and_bytes was introduced to retrieve both the parsed certificate and its raw bytes, and the metadata get helper was updated to support POST requests and bodies. Feedback on the changes suggests simplifying a redundant tuple check in credentials.py by directly unpacking the returned value from get_agent_identity_certificate_and_bytes.
|
Both ID token unit tests ( |
|
Can't be merged before CR MDS is ready. Currently targeting a date between July 31 and Aug 7. |
2766928 to
ccc2a5d
Compare
| return_none_for_not_found_error (Optional[bool]): If True, returns None | ||
| for 404 error instead of throwing an exception. | ||
| method (str): The HTTP method to use for the request. Defaults to "GET". | ||
| body (Optional[bytes]): The HTTP request body payload to send. Defaults to None. |
There was a problem hiding this comment.
Does it make sense to raise a ValueError (or similar) here to "exit early" if a body is specified byt the method is GET. While I think technically valid to include a body in GET requests (most often I think the body just gets ignored), it may lead a caller to think it is getting a bound token when in reality it isn't?
There was a problem hiding this comment.
Yes! that's a great suggestion! Done!
| ( | ||
| cert, | ||
| cert_bytes, | ||
| ) = _agent_identity_utils.get_agent_identity_certificate_and_bytes() |
There was a problem hiding this comment.
nit: It looks like both this and should_request_bound_token check GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES and call _mtls_helper._check_use_client_cert_env() - I wonder if we can optimize this in any way?
There was a problem hiding this comment.
Yeah you're right, they do both check the env var but I don't think we can eliminate it because the two methods have different callers and come from different paths (compute engine and identity pool) so we need to have the check in both places. We could probably do some refactoring, but I'm leaning toward keeping the code as is since the env var reading is not an expensive operation and this way we can keep the methods self contained.
| return None | ||
| return None, None | ||
|
|
||
| return parse_certificate(cert_bytes), cert_bytes |
There was a problem hiding this comment.
If cert_path points to a combined bundle (credentialbundle.pem), sending raw cert_file.read() puts the private key into certificate_chain over plain HTTP (and GKE MDS rejects non-CERTIFICATE PEM blocks with 400). Also, cert_bytes.decode("utf-8") will raise an uncaught UnicodeDecodeError if there are non-UTF-8 OpenSSL bag attributes outside the PEM boundaries.
We should extract only the CERTIFICATE blocks before returning, e.g. with a non-greedy r"-----BEGIN CERTIFICATE-----.+?-----END CERTIFICATE-----\r?\n?" (_mtls_helper._CERT_REGEX is greedy and would still grab an interleaved key).
There was a problem hiding this comment.
In this PR discovery only resolves cert_path from GOOGLE_API_CERTIFICATE_CONFIG (which on Cloud Run points to the standalone certificates.pem file). Automatic discovery of GKE's combined credentialbundle.pem is not active here.
In our follow-up PR adding GKE support, get_agent_identity_certificate_and_bytes() will be updated to extract only -----BEGIN CERTIFICATE-----...-----END CERTIFICATE----- blocks via non-greedy regex. That strips any private key blocks from combined bundles and discards any non-UTF-8 OpenSSL bag attributes outside the PEM boundaries prior to UTF-8 decoding.
I'll mark this as resolved since it's out of the scope of this PR and will be addressed in the GKE PR.
There was a problem hiding this comment.
We should fix this in this PR before merging rather than deferring to the GKE follow-up. GOOGLE_API_CERTIFICATE_CONFIG is not Cloud Run specific. GKE/GCE can set this today with cert_path and key_path pointing to the same combined PEM bundle.
Switch the MDS token acquisition from a GET to a POST request when the agentic cert is detected. * Add `get_agent_identity_certificate_and_bytes()` utility to read the raw certificate bytes alongside the parsed cert. * Update `_metadata.get_service_account_token()` (for access tokens) and `IDTokenCredentials.refresh()` (for ID tokens) to send a POST request with the `certificate_chain` payload instead of a GET request when bound tokens are supported. * Update `_metadata.get()` helper to support `method` and `body` params. * Add and update unit tests to verify the new POST request flows.
…and update docstrings and comments
…eplace outdated mocks
…llback to deprecated var
…certificate_and_bytes
ccc2a5d to
f7fdd75
Compare
| ) = _agent_identity_utils.get_agent_identity_certificate_and_bytes() | ||
|
|
||
| expected_certs = NON_AGENT_IDENTITY_CERT_BYTES + NON_AGENT_IDENTITY_CERT_BYTES | ||
| assert isinstance(cert, x509.Certificate) |
There was a problem hiding this comment.
Can we use AGENT_IDENTITY_CERT_BYTES as the first block and NON_AGENT_IDENTITY_CERT_BYTES as the second in combined_bundle, and assert _is_agent_identity_certificate(cert)? Right now both blocks are NON_AGENT_IDENTITY_CERT_BYTES, so changing return certs[0] to return certs[-1] in parse_certificate still passes the test suite.
There was a problem hiding this comment.
good call, updated!
| """Returns True only if bound tokens are explicitly disabled via env vars.""" | ||
| val = os.environ.get(environment_vars.GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN) | ||
| if val is not None: | ||
| return val.lower() == "false" |
There was a problem hiding this comment.
Do we need to strip() too to account for whitespace - this may be the case in the fallback check too? Additionally, what about an "" (which isn't None)?
Perhaps this can be simplified to:
val = (
os.environ.get(environment_vars.
GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN)
or os.environ.get(
environment_vars.
GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES
)
or "true"
)
return val.strip().lower() == "false"
There was a problem hiding this comment.
I kept the if/else branches for readability but added strip() to cover the whitespace scenario. Let me know if you think the simplified version would be better for readability / coding style though.
# Conflicts: # packages/google-auth/google/auth/aio/transport/sessions.py # packages/google-auth/google/auth/compute_engine/_metadata.py # packages/google-auth/tests/compute_engine/test_credentials.py
|
Found a few issues and questions while testing the certificate parsing and bound-token changes:
|
_X509Supplier.get_subject_token wasn't modified in this PR. On main, it already loads only the leaf certificate from cert_path (x509.load_pem_x509_certificate) and reads intermediate certificates from trust_chain_path. However, the described behavior is accurate, and is working as intended and matches the behavior across the other SDKs (Go, Java, etc.). Based on the X.509 WIF documentation and Automatically appending trailing certificates from |
Addressed the first part of the comment: However, the check remains scoped to full |
This is intentional and matches the documented contract for GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN in environment_vars.py ("Defaults to enabled; only a case-insensitive 'false' disables it"). In _is_bound_token_opted_out(), an empty string "" is treated as unset so it falls back to GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES (and defaults to enabled) rather than silently disabling token binding when an empty env var template is passed.
_build_token_request_options only requests bound tokens when GOOGLE_API_CERTIFICATE_CONFIG provides an Agent Identity SPIFFE certificate (matching the existing bindCertificateFingerprint behavior on main). AuthorizedSession and AuthorizedHttp are intentionally designed to require an explicit configure_mtls_channel() call to enable mTLS (which Google Cloud client libraries call automatically when workload certificates are present).
POST is only used when GOOGLE_API_CERTIFICATE_CONFIG provides an Agent Identity SPIFFE certificate (and compute_engine.Credentials already passes service_account="default"), so non-Agent service accounts will continue to use GET. When an Agent Identity certificate is active and bound tokens are enabled, failing closed rather than falling back to GET is intentional so transient errors or misconfigurations cannot silently downgrade a bound token request to an unbound bearer token (environments that need unbound tokens can opt out via GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false). |
|
Thanks Negar!
|
|
There is one edge case left in the multi-certificate PEM chain parsing added in this PR. The check for unmatched certificate markers was added when reading certificate files and parsing certificates, but not when reading the output of a certificate provider command. If a certificate provider command outputs a valid leaf certificate followed by a truncated intermediate certificate, the regex match strips out the incomplete trailing block and returns a single-certificate chain without raising a client certificate error. Running that same unmatched marker check on the certificate provider command output before joining the chain will keep both paths consistent. |
No, we intentionally kept a single control (
Third-party OIDC verifiers ignore the |
This PR adds the following:
Switch the MDS token acquisition from a GET to a POST request when the agentic cert is detected.
Add
get_agent_identity_certificate_and_bytes()utility to read the raw certificate bytes alongside the parsed cert.Update
_metadata.get_service_account_token()(for access tokens) andIDTokenCredentials.refresh()(for ID tokens) to send a POST request with thecertificate_chainpayload instead of a GET request when bound tokens are supported.Update
_metadata.get()helper to supportmethodandbodyparams.Add and update unit tests to verify the new POST request flows.
design: go/sdk-mds-bound-token
id token verification:
Note:
This PR relies on the existing pattern of locating the certificates using the path provided by the config file available at GOOGLE_API_CERTIFICATE_CONFIG. It does not currently fallback on checking the well known location if the env var is not set, which would limit the scope to CR, as GKE and GCE don't set this env var.
It uses the same condition to decide if a bound token should be requested for both access token and id token. We might decide to add a separate env var to opt out.
it still uses the existing
GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICESflag to opt out. We might update it and add a new env var, but will keep the old one for backward compatibility.