Skip to content

Commit 8dbaa6f

Browse files
committed
Review session improvements
- 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
1 parent 3288350 commit 8dbaa6f

4 files changed

Lines changed: 68 additions & 6 deletions

File tree

google-auth-library-java/oauth2_http/java/com/google/auth/mtls/MtlsHttpTransportFactory.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,12 @@ public class MtlsHttpTransportFactory implements HttpTransportFactory {
5555
@Nullable private final KeyStore mtlsKeyStore;
5656

5757
/**
58-
* No-arg constructor required for deserialization via reflection. Not intended for direct use;
59-
* callers should use {@link #MtlsHttpTransportFactory(KeyStore)}.
58+
* No-arg constructor required for Java serialization. {@link IdentityPoolCredentials} stores this
59+
* factory in its serializable {@code transportFactory} field, and {@link
60+
* java.io.ObjectInputStream} needs a no-arg constructor to reconstruct it during
61+
* deserialization. Not intended for direct use; callers should use {@link
62+
* #MtlsHttpTransportFactory(KeyStore)}.
6063
*/
61-
@InternalApi
6264
public MtlsHttpTransportFactory() {
6365
this.mtlsKeyStore = null;
6466
}
@@ -74,6 +76,15 @@ public MtlsHttpTransportFactory(KeyStore mtlsKeyStore) {
7476
this.mtlsKeyStore = Objects.requireNonNull(mtlsKeyStore, "mtlsKeyStore cannot be null");
7577
}
7678

79+
/**
80+
* Returns whether this factory was constructed with a non-null {@link KeyStore} containing
81+
* client certificates for mTLS. A factory created via the no-arg constructor (e.g. during
82+
* deserialization) will return {@code false}.
83+
*/
84+
public boolean hasKeyStore() {
85+
return this.mtlsKeyStore != null;
86+
}
87+
7788
@Override
7889
public HttpTransport create() {
7990
try {

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/FileIdentityPoolSubjectTokenSupplier.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@
5252
/**
5353
* Internal provider for retrieving the subject and actor tokens for {@link IdentityPoolCredentials}
5454
* to exchange for GCP access tokens via a local file.
55+
*
56+
* <p>Note: Despite the name, this class handles both subject <em>and</em> actor tokens. The class
57+
* name retains "Subject" for serialization backward compatibility; renaming it would break
58+
* deserialization of previously serialized credentials.
5559
*/
5660
@NullMarked
5761
class FileIdentityPoolSubjectTokenSupplier

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,14 @@ public class IdentityPoolCredentials extends ExternalAccountCredentials {
167167

168168
/**
169169
* Checks whether mTLS is properly configured by verifying that an X509Provider is set or the
170-
* transport factory is an MtlsHttpTransportFactory. This avoids relying solely on instanceof
171-
* checks which could pass for a misconfigured factory.
170+
* transport factory is an MtlsHttpTransportFactory with a non-null KeyStore. This avoids false
171+
* positives from a no-arg-constructed MtlsHttpTransportFactory (e.g. after deserialization)
172+
* that has no actual certificates.
172173
*/
173174
private boolean isMtlsConfigured() {
174-
return this.x509Provider != null || this.transportFactory instanceof MtlsHttpTransportFactory;
175+
return this.x509Provider != null
176+
|| (this.transportFactory instanceof MtlsHttpTransportFactory
177+
&& ((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore());
175178
}
176179

177180
@Override
@@ -338,6 +341,10 @@ public static class Builder extends ExternalAccountCredentials.Builder {
338341
this.subjectTokenSupplier = credentials.subjectTokenSupplier;
339342
this.actorTokenSupplier = credentials.actorTokenSupplier;
340343
}
344+
// Note: when credentialSource is present, subjectTokenSupplier and actorTokenSupplier
345+
// are intentionally NOT copied here. They will be reconstructed from credentialSource
346+
// during build(), which ensures they share the same FileIdentityPoolSubjectTokenSupplier
347+
// instance for atomic token reads.
341348
this.actorTokenType = credentials.actorTokenType;
342349
this.x509Provider = credentials.x509Provider;
343350
}

google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/IdentityPoolCredentialsTest.java

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1730,6 +1730,46 @@ void builder_actorTokenWithoutMtls_throws() {
17301730
"Actor tokens are only supported for mTLS token exchanges."));
17311731
}
17321732

1733+
@Test
1734+
void builder_actorTokenWithNoArgMtlsFactory_throws() throws Exception {
1735+
// A no-arg MtlsHttpTransportFactory (e.g. from deserialization) has no KeyStore,
1736+
// so isMtlsConfigured() should return false and building should fail.
1737+
MtlsHttpTransportFactory noArgFactory = new MtlsHttpTransportFactory();
1738+
assertFalse(noArgFactory.hasKeyStore());
1739+
1740+
IllegalArgumentException e =
1741+
assertThrows(
1742+
IllegalArgumentException.class,
1743+
() ->
1744+
IdentityPoolCredentials.newBuilder()
1745+
.setSubjectTokenSupplier(testProvider)
1746+
.setActorTokenSupplier(testActorSupplier)
1747+
.setActorTokenType("urn:ietf:params:oauth:token-type:jwt")
1748+
.setHttpTransportFactory(noArgFactory)
1749+
.setAudience("audience")
1750+
.setSubjectTokenType("subjectTokenType")
1751+
.setTokenUrl("https://sts.mtls.googleapis.com/v1/token")
1752+
.build());
1753+
assertTrue(
1754+
e.getMessage()
1755+
.contains(
1756+
"Actor tokens are only supported for mTLS token exchanges."));
1757+
}
1758+
1759+
@Test
1760+
void mtlsHttpTransportFactory_hasKeyStore_withKeyStore_returnsTrue() throws Exception {
1761+
KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());
1762+
ks.load(null, null);
1763+
MtlsHttpTransportFactory factory = new MtlsHttpTransportFactory(ks);
1764+
assertTrue(factory.hasKeyStore());
1765+
}
1766+
1767+
@Test
1768+
void mtlsHttpTransportFactory_hasKeyStore_noArg_returnsFalse() {
1769+
MtlsHttpTransportFactory factory = new MtlsHttpTransportFactory();
1770+
assertFalse(factory.hasKeyStore());
1771+
}
1772+
17331773
// ==================================================================================
17341774
// Section A: Cert Pinning & Transport Factory Tests
17351775
// ==================================================================================

0 commit comments

Comments
 (0)