Skip to content

Conversation

@andyrzhao
Copy link
Contributor

Only attempt to create a default MtlsProvider in "determineEndpoint" if client certificate usage is enabled by the env var GOOGLE_API_USE_CLIENT_CERTIFICATE.

Advisory Note: GOOGLE_API_USE_CLIENT_CERTIFICATE will default to true (if not set) in a future release, so if you do not wish to enable mTLS (such as for testing environments) please explicitly set GOOGLE_API_USE_CLIENT_CERTIFICATE to false before executing your test suite.

Fixes #3911 ☕️

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Sep 23, 2025
Comment on lines 303 to 307
} catch (IOException e) {
LOG.log(
Level.WARNING,
"DefaultMtlsProviderFactory encountered unexpected IOException: " + e.getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be a reason for this and I've since lost the context for this. Do you remember why this is logged and not thrown back to the user?

This IOException occurs when trying to create the MtlsProvider when a user enables Mtls (via Env Var).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this warning is logged when the SDK detects the necessary mTLS config on the device, but the mTLS helper fails unexpectedly (ex. WARNING: DefaultMtlsProviderFactory encountered unexpected IOException: SecureConnect: Cert provider command failed with exit code: 139). In this case, mTLS is still disabled, the app doesn't crash, but a warning is logged because the helper command should not be failing - this reveals an underlying bug with the helper that should be fixed by the owning team. In other words, WARN if the user intends to use mTLS and it fails, and NO WARN if user never intended to use mTLS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, in that case can the logs add some like mTLS is disabled and falling back to TLS?

Would there be any possible security implications where we fall back to TLS and not use mTLS without erroring out? This may be already covered/ approved, but I'm wondering if the user doesn't see this warning and assumes that they're running on mTLS when it turns out it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question - what happens is that if the user (or the workload) is under CAA-policy enforcement ("require mTLS"), they will receive an "Access Denied" error when trying to access GCP resources. I will include the message you suggested to help with debugging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow up question for this. What about if the user/ workload is not under CAA-policy enforcement? Or is that going to be enabled for everyone in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user/workload is not under CAA-policy enforcement, then the requests will simply succeed even when hitting the non-mTLS endpoints.

@lqiu96
Copy link
Member

lqiu96 commented Sep 23, 2025

/gcbrun

@andyrzhao andyrzhao requested a review from lqiu96 September 24, 2025 06:24
@lqiu96
Copy link
Member

lqiu96 commented Sep 24, 2025

/gcbrun

@lqiu96
Copy link
Member

lqiu96 commented Sep 24, 2025

there looks to be some flaky tests in the CI. Re-running the jobs.

@lqiu96
Copy link
Member

lqiu96 commented Sep 24, 2025

/gcbrun

@lqiu96 lqiu96 merged commit e5948d0 into googleapis:main Sep 24, 2025
81 of 85 checks passed
mpeddada1 added a commit that referenced this pull request Oct 2, 2025
🤖 I have created a release *beep* *boop* --- <details><summary>2.62.3</summary> ## [2.62.3](v2.62.2...v2.62.3) (2025-10-02) ### Bug Fixes * **mtls:** Fix EndpointContext's determineEndpoint logic to respect env var ([#3912](#3912)) ([e5948d0](e5948d0)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Mridula <66699525+mpeddada1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: s Pull request size is small.

2 participants