feat(auth): mTLS endpoint for Regional Access Boundaries#13318
feat(auth): mTLS endpoint for Regional Access Boundaries#13318vverman wants to merge 23 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces centralized mTLS enablement checks and adds fallback support for SPIFFE credentials in MtlsUtils and X509Provider, alongside integrating mTLS transport initialization during regional access boundary refreshes. The review feedback suggests optimizing performance by removing redundant configuration checks and file parsing in X509Provider.getKeyStore() and GoogleCredentials.java, and improving robustness in RegionalAccessBoundary.java by replacing only the host name in the IAM credentials URL.
vverman
changed the title
lsirac
left a comment
lsirac
left a comment
There was a problem hiding this comment.
Should we be checking GOOGLE_API_USE_MTLS_ENDPOINT?
| * @throws IOException if the configuration file is present but contains missing or malformed | ||
| * files | ||
| */ | ||
| public static boolean canMtlsBeEnabled( |
There was a problem hiding this comment.
I’m not sure that cert being present == automatically use mTLS. They can be using different credentials / not using it at all. So then we’d be adding mTLS setup and calls for credentials that are not actually using it.
I think the decision should be based on the credential type, and perhaps expose some state from the credential that we can use to check if mTLS should happen for these calls.
2c53152 to
48c3b59
Compare
733b9bc to
c6985fe
Compare
|
|
||
| if (!canBeEnabled(envProvider, propProvider, certConfigPathOverride)) { | ||
| return baseTransportFactory; | ||
| } |
There was a problem hiding this comment.
Currently the RAB manager assumes any non-default transport supports mTLS and switches to the mTLS endpoint.
I think this is worth a discussion - what are we doing in other languages? At least here we don't know if the custom transport carries a cert.
There was a problem hiding this comment.
Done! The logic now ensures the library will only switch the endpoint URL to the mTLS variant (MTLS_IAM_ENDPOINT) if the upgraded transport factory is successfully created and is not the standard non-mTLS transport (OAuth2Utils.HTTP_TRANSPORT_FACTORY).
There was a problem hiding this comment.
The changes were discussed in this thread.
The idea is that a user's overriden transport will always be trusted. If they do provide a transport which doesn't support mtls, the RAB lookup errors out and goes into cooldown to avoid overwhelming the lookup.
@nbayati curious to hear if python does the same.
Added an E2E test to check the call to RAB mtls endpoint and check x-allowed-locations header. Unified redundant method(s) waitForRegionalAccessBoundary.
…TLS refresh - MtlsUtils: - Validate custom transport factory outside try-block to prevent swallowing exceptions. - Add null check for baseTransportFactory to prevent NullPointerException. - Wrap getWellKnownCertificateConfigFile call to enforce the exception contract. - Use case-insensitive matching for GOOGLE_API_USE_MTLS_ENDPOINT. - RegionalAccessBoundary & Manager: - Remove JVM-wide userMtlsPolicy static cache to prevent test pollution. - Inject EnvironmentProvider dynamically to refresh methods. - MockExternalAccountCredentialsTransport: - Strip .mtls. subdomain before looking up regional access boundaries. - Define host-only IAM_ENDPOINT and MTLS_IAM_ENDPOINT constants. - GoogleCredentialsTest: - Add test asserting boundary refresh hits .mtls. subdomain when required. - oauth2_http/pom.xml: - Set GOOGLE_API_USE_CLIENT_CERTIFICATE=false in surefire config to isolate tests.
… exception wrapping - MtlsUtils: - Trust and preserve developer's custom HttpTransportFactory when mTLS is enabled instead of throwing IOException. - X509Provider: - Move workload certificate configuration resolution inside the try-catch block to properly wrap certificate source errors into IOException. - RegionalAccessBoundary & Manager: - Move subdomain substitution (.mtls.) logic to RegionalAccessBoundaryManager, gating it on MtlsUtils.canBeEnabled. - Simplify RegionalAccessBoundary.refresh signature. - Tests: - Add unit tests verifying custom HttpTransportFactory retention under AUTO and ALWAYS policies. - Add tests validating proper wrapping of JSON parsing failures in X509Provider.
…s, adds 3-minute token clock skew buffer, and fixes mTLS fallback logic.
43d72dc to
ba2d3ca
Compare
| // Check if client certificate usage is allowed | ||
| String useClientCertificate = envProvider.getEnv("GOOGLE_API_USE_CLIENT_CERTIFICATE"); | ||
| MtlsEndpointUsagePolicy policy = getMtlsEndpointUsagePolicy(envProvider); | ||
| if ("false".equalsIgnoreCase(useClientCertificate)) { |
There was a problem hiding this comment.
AIP-4114 says:
"If user specifies an mTLS endpoint override but device certificate is not available, do not fail-fast, but let server return error when connecting."
So which endpoint to call, and whether to attach a cert should be evaluated separately.
There was a problem hiding this comment.
Good catch! I have updated the implementation to decouple the mTLS endpoint resolution from the client certificate availability check, in accordance with AIP-4114.
Under the always policy, the library will now still target the mTLS endpoint URL but will gracefully fall back to the standard transport factory (with no certificate attached) instead of throwing fail-fast exceptions during initialization. This allows the client to successfully boot up and let the connection fail downstream at connection time as required by the specification.
P.S.: we check for the mtls usage policy = never within canBeEnabled as, per AIP-4114
GOOGLE_API_USE_MTLS_ENDPOINT never: "The client library MUST NOT use an mTLS endpoint or a device certificate."
| return false; | ||
| } | ||
| return true; | ||
| return MtlsUtils.canBeEnabled(envProvider, propProvider, certConfigPathOverride); |
There was a problem hiding this comment.
It seems this was changed from answering "can this X509 provider supply a client certificate?" to answering "should mTLS be attempted".
This should still indicate whether a cert could actually be provided. It currently returns true based on mTLS configuration, not whether it has a usable cert. Please fix.
There was a problem hiding this comment.
My initial thought with this was to combine the isAvailable logic with this.
But per our discussion in the above comment to separate determining using the certificate with the mTLS policy, I've reverted X509Provider.isAvailable() back to the original try-catch check on getKeyStore() to ensure it indicates whether a certificate can actually be provided, independent of the global mTLS flags. I also added corresponding unit tests to cover this.
| // Atomically check if a refresh is already running. If compareAndSet returns true, | ||
| // this thread "won the race" and is responsible for starting the background task. | ||
| // All other concurrent threads will return false and exit immediately. | ||
| if (isRefreshing.compareAndSet(false, true)) { |
There was a problem hiding this comment.
We should check after acquiring the lock if another lookup is actually needed.
There was a problem hiding this comment.
Thanks for the catch! Added a check in to avoid duplicated refreshes.
| HttpTransportFactory upgradedTransportFactory = | ||
| MtlsUtils.prepareTransportFactoryIfMtlsEnabled( | ||
| transportFactory, envProvider, propProvider, null); | ||
| if (MtlsUtils.canBeEnabled(envProvider, propProvider, null) |
There was a problem hiding this comment.
prepareTransportFactoryIfMtlsEnabled should know if a cert is attached, we should use that here instead?
There was a problem hiding this comment.
We now upgrade the transporter to mTLS based on the result of the prepareTransportFactoryIfMtlsEnabled which calls MtlsUtils.canBeEnabled().
We change the endpoint to mTLS based on MtlsUtils.shouldMtlsEndpointBeUsed() which differentiates the cert-usage from the mTLS endpoint switch.
…per AIP-4114, removing fail-fast exceptions when client certificates are unavailable under always policy.
…ity rather than configuration flags, and add unit tests.
4 participants
Added logic to: