test(oauth2): add mTLS in-process socket tests and live GCP WIF integration tests - #14220
Draft
macastelaz wants to merge 23 commits into
Draft
test(oauth2): add mTLS in-process socket tests and live GCP WIF integration tests#14220macastelaz wants to merge 23 commits into
macastelaz wants to merge 23 commits into
Conversation
Implementation of Phase 1-3 of the Cert-Bound Oauth2 Design Document: 1. Extend IdentityPoolCredentialSource to parse actorTokenFieldName. 2. Relax mutual exclusivity to allow BOTH file and certificate configurations. 3. Parse actor_token_type in ExternalAccountCredentials. 4. Refactor FileIdentityPoolTokenSupplier and track file timestamp via volatile CachedFile for the parsed JSON payload. 5. Inject actor_token and actor_token_type into StsTokenExchangeRequest using ActingParty. 6. Enforce that actor token extraction requires an mTLS STS configuration.
Fixes test failures and thread synchronization bugs regarding actor token credentials from https://paste.googleplex.com/5381957298028544
- Mark CachedFile and X509Provider transient to ensure clean serialization. - Add static modifier to FileIdentityPoolTokenSupplier serialVersionUID. - Make IdentityPoolActorTokenSupplier public with @NullMarked annotation. - Preserve actorTokenSupplier in IdentityPoolCredentials Builder copy constructor. - Mask actor_token in Slf4jLoggingHelpers sensitive keys. - Add no-arg constructor to MtlsHttpTransportFactory for serialization support. - Handle Data.isNull in FileIdentityPoolTokenSupplier JSON parsing. - Add comprehensive test coverage for supplier caching, builder, serialization, and log masking.
…uilder copy constructor - Guard actorTokenSupplier assignment with if (this.credentialSource == null) in Builder copy constructor. - Add getIdentityPoolActorTokenSupplier getter for test assertions. - Add createScoped tests for both file-sourced and supplier-sourced credentials with actor tokens.
…s and FileIdentityPoolTokenSupplier - Add builder_actorTokenTypeWithoutSupplier_throws testing missing supplier validation. - Add builder_fileWithCertificateConfig_initializesMtlsTransport testing mTLS initialization for composite file + cert sources. - Add toBuilder_preservesConfiguration testing builder reconstruction. - Add parseToken_textFormat_succeeds and parseToken_jsonFormat_missingFieldName_throws testing static token parsing methods.
Summary of changes: - Comment 1: Fix Javadoc referencing package-private API - Comment 2: Replace instanceof with isMtlsConfigured() check - Comment 3: Per-cycle cert pinning with KeyStore snapshot, 401 retry - Comment 4: Atomic subject+actor token read via readTokens() - Comment 5: Make setActorTokenSupplier/Type package-private - Comment 6: Add Javadocs to builder setter methods - Comment 7: Fix Builder copy constructor (always copy actorTokenType) - Comment 8: Annotate no-arg MtlsHttpTransportFactory with @internalapi - Comment 9: Revert class rename to FileIdentityPoolSubjectTokenSupplier - Comment 10: Remove CachedFile/volatile caching mechanism - Comment 11: Add actorTokenFieldName validation - Comment 12: Integration tests noted for follow-up PR Added overload exchangeExternalCredentialForAccessToken(request, factory) for per-cycle transport factory threading. Added 12 new unit tests covering readTokens(), validation, and mTLS. All 982 existing + new tests pass.
- Fix copyright year (2024 -> 2026) in FileIdentityPoolSubjectTokenSupplier - Add Javadoc explaining class name retained for serialization compatibility - Add hasKeyStore() to MtlsHttpTransportFactory for watertight mTLS validation - Update isMtlsConfigured() to verify KeyStore is non-null via hasKeyStore() - Update no-arg constructor Javadoc to explain serialization requirement - Add comment to Builder copy constructor explaining supplier reconstruction - Add 3 unit tests for hasKeyStore() and no-arg factory validation
- Set explicit static serialVersionUID in FileIdentityPoolSubjectTokenSupplier matching pre-PR synthetic SUID (7152208690659890358L). - Revert MtlsHttpTransportFactory.create() return type to NetHttpTransport for binary compatibility. - Update MtlsHttpTransportFactory.hasKeyStore() to verify mtlsKeyStore is non-null and size > 0. - Make IdentityPoolCredentials.x509Provider non-final transient and implement custom readObject to restore x509Provider and transportFactory on deserialization. - Add comprehensive unit tests for MtlsHttpTransportFactory, backward-compatible deserialization, readObject restoration, and fromStream production paths.
…freshAccessToken_pinsTransportForStsExchange
…nfigured - Add validation in IdentityPoolCredentials constructor to reject plain public googleapis.com endpoints when actor tokens are configured - Allow mTLS (.mtls.), Private Service Connect (.p.), and custom domain endpoints - Add comprehensive unit tests covering plain public, mTLS, PSC, and custom domain endpoints
…ration tests - Add MtlsPipelineLocalTest providing hermetic in-process socket tests over JDK HttpsServer with client certificate authentication (peer cert verification, 401 retry with cert rotation, concurrent refreshes, atomic token read, IAM impersonation mTLS transport pinning, and 401 retry with fresh cert). - Add ITWorkloadIdentityFederationTest extensions for certificate-bound workload + actor token JSON config and programmatic mTLS token suppliers covering both direct STS and Service Account Impersonation. - Fix OAuthException to safely handle null HTTP error response content.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for actor token extraction and certificate-bound OAuth 2.0 token exchanges (mTLS) within IdentityPoolCredentials. Key updates include implementing an atomic reader for both subject and actor tokens from JSON files, adding 401 retry logic with certificate reloading, and enhancing ImpersonatedCredentials to propagate custom transport factories. The feedback highlights two important issues: first, MtlsHttpTransportFactory must implement Serializable and mark its non-serializable KeyStore field as transient to avoid serialization failures; second, the raw file paths concatenated into JSON strings in MtlsPipelineLocalTest must escape backslashes to prevent JSON parsing errors on Windows systems.
Comment on lines
56
to
+57
| public class MtlsHttpTransportFactory implements HttpTransportFactory { | ||
| private final KeyStore mtlsKeyStore; | ||
| @Nullable private final KeyStore mtlsKeyStore; |
Contributor
There was a problem hiding this comment.
Since MtlsHttpTransportFactory is stored in the serializable transportFactory field of IdentityPoolCredentials and is expected to be serialized, it must implement java.io.Serializable. Additionally, java.security.KeyStore is not serializable, so the mtlsKeyStore field must be marked as transient to avoid NotSerializableException during serialization.
Suggested change
| public class MtlsHttpTransportFactory implements HttpTransportFactory { | |
| private final KeyStore mtlsKeyStore; | |
| @Nullable private final KeyStore mtlsKeyStore; | |
| public class MtlsHttpTransportFactory implements HttpTransportFactory, java.io.Serializable { | |
| private static final long serialVersionUID = 1L; | |
| @Nullable private transient final KeyStore mtlsKeyStore; |
| + "/v1/token\",\n" | ||
| + " \"credential_source\": {\n" | ||
| + " \"file\": \"" | ||
| + tokenFile.toString() |
Contributor
There was a problem hiding this comment.
On Windows systems, tokenFile.toString() (and other file paths like certConfigFile, dynamicCertFile, dynamicKeyFile) will contain backslashes (\). When concatenated directly into a JSON string, these backslashes act as unescaped control characters, resulting in invalid JSON and causing parsing failures. To ensure cross-platform compatibility, escape the backslashes by using .replace("\\", "\\\\") or construct the JSON programmatically using GenericJson.
Suggested change
| + tokenFile.toString() | |
| + tokenFile.toString().replace("\\", "\\\\") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds comprehensive integration tests for Certificate-Bound OAuth 2.0 (mTLS), Actor Tokens, and Service Account Impersonation transport pinning across both hermetic in-process socket tests and live GCP Workload Identity Federation suites.
Summary of Changes:
MtlsPipelineLocalTest.java):HttpsServeronlocalhostrequiring client authentication (setNeedClientAuth(true)).testMtlsPipeline_verifiesPeerCertAndPayload: Verifies server extracts client peer certificate fromSSLSessionand parses token exchange body (grant_type,subject_token,actor_token,actor_token_type).testMtlsPipeline_withImpersonation_usesSameCertForStsAndIam: Verifies both STS and IAM calls execute on the exact same pinned mTLS transport and verifies the intermediateAuthorization: Bearer <token>header.testMtlsPipeline_withImpersonation_401OnIam_retriesWithFreshCert: Verifies full-cycle recovery when IAM returns 401 (reloads fresh KeyStore from disk, re-exchanges at STS with new cert, and calls IAM with new token + cert).testMtlsPipeline_concurrentRefreshes: Multi-threaded refresh verifying thread-isolated transport snapshots.testMtlsPipeline_atomicTokenRead: Verifies single-pass disk I/O for subject + actor tokens.ITWorkloadIdentityFederationTest.java):identityPoolCredentials_withCertificateBoundWorkloadAndActorToken: E2E verification of JSON config with certificate config and actor tokens.identityPoolCredentials_withProgrammaticMtlsAndActorToken: E2E verification of programmatic builder configuration.OAuthException.java:Verification:
mvn com.spotify.fmt:fmt-maven-plugin:2.25:check).Stacked on feat(oauth2): implement IAM impersonation mTLS transport pinning and 401 recovery #14212
Source-Link: b/542238030