fix(auth): Agentic Identites mTLS gaps fix _is_mtls and SslCredentials.#17387
fix(auth): Agentic Identites mTLS gaps fix _is_mtls and SslCredentials.#17387vverman wants to merge 6 commits into
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
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.