fix(auth): Agentic Identites mTLS gaps fix _is_mtls and SslCredentials.#17387
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the mTLS configuration logic across the aio, grpc, and urllib3 transports to ensure that _is_mtls is explicitly set to False when falling back to standard connections or when custom requests cannot support mTLS. Additionally, grpc.py now utilizes mtls.has_default_client_cert_source() to determine the mTLS state. Corresponding unit tests have been added to verify these changes. I have no feedback to provide as there are no review comments to evaluate.
| ssl_credentials = google.auth.transport.grpc.SslCredentials() | ||
|
|
||
| # If a workload certificate config exists on the device (and use_client_cert is true), | ||
| # is_mtls must be True and get_client_ssl_credentials should be invoked. | ||
| assert ssl_credentials.ssl_credentials is not None | ||
| assert ssl_credentials.is_mtls |
There was a problem hiding this comment.
| ssl_credentials = google.auth.transport.grpc.SslCredentials() | |
| # If a workload certificate config exists on the device (and use_client_cert is true), | |
| # is_mtls must be True and get_client_ssl_credentials should be invoked. | |
| assert ssl_credentials.ssl_credentials is not None | |
| assert ssl_credentials.is_mtls | |
| ssl_credentials = google.auth.transport.grpc.SslCredentials() | |
| # If a workload certificate config exists on the device (and use_client_cert is true), | |
| # is_mtls must be True and get_client_ssl_credentials should be invoked. | |
| assert ssl_credentials.ssl_credentials is not None | |
| assert ssl_credentials.is_mtls |
nbayati
left a comment
There was a problem hiding this comment.
nit: Since this PR resolves existing bugs rather than introducing new features, the PR title prefix should be fix(auth) instead of feat(auth).
nbayati
left a comment
There was a problem hiding this comment.
Thank you for the PR! I have completed a review of the changes. Here are some comments regarding potential state leakage, resource leaks, and mismatches when configuration exceptions occur.
87a3463 to
e8bb0b7
Compare
e8bb0b7 to
19b8120
Compare
nbayati
left a comment
There was a problem hiding this comment.
Great work Pranav! Before we merge, there are just a few minor cleanups and test coverage gaps we need to address to ensure these fixes remain robust long-term.
| _mtls_helper.CONTEXT_AWARE_METADATA_PATH | ||
| ) | ||
| self._is_mtls = metadata_path is not None | ||
| self._is_mtls = mtls.has_default_client_cert_source() |
There was a problem hiding this comment.
This helper ignores CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH (the context-aware fallback configuration path), so we will have a mismatch state where use_client_cert will return true but has_default_client_cert_source misses that path.
It's an existing issue with has_default_client_cert_source() though, so probably out of scope for this PR.
I'll document this so we can fix it in a future PR.
There was a problem hiding this comment.
Done, I'll leave this open!
…se() raises an exception.
49899cd to
a9b434c
Compare
…s. (#17387) Resolve some mTLS transport state and gRPC workload certificate issues - Fix inconsistent `_is_mtls` flag in `urllib3` on fallback to default HTTP. - Reset `_is_mtls` to `False` in async `sessions` if a custom request handler cannot be reconfigured. - Update gRPC `SslCredentials` to recognize workload certificates by checking `has_default_client_cert_source` instead of only SecureConnect.
Resolve some mTLS transport state and gRPC workload certificate issues
Fix inconsistent
_is_mtlsflag inurllib3on fallback to default HTTP.Reset
_is_mtlstoFalsein asyncsessionsif a custom request handler cannot be reconfigured.Update gRPC
SslCredentialsto recognize workload certificates by checkinghas_default_client_cert_sourceinstead of only SecureConnect.