Skip to content

Single S2AChannelCredentials + use AdvancedTls. #3989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Single S2AChannelCredentials + use AdvancedTls. #3989

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ddbe44
Single S2AChannelCredentials + use AdvancedTls.
rmehta19 Nov 14, 2025
e35914f
add comment.
rmehta19 Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 63 additions & 38 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
import io.grpc.TlsChannelCredentials;
import io.grpc.alts.GoogleDefaultChannelCredentials;
import io.grpc.auth.MoreCallCredentials;
import io.grpc.util.AdvancedTlsX509KeyManager;
import io.grpc.util.AdvancedTlsX509TrustManager;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.Method;
Expand All @@ -78,6 +80,7 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
Expand Down Expand Up @@ -161,6 +164,8 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;

private static volatile ChannelCredentials s2aChannelCredentialsObject;
Copy link

@xmenxk xmenxk Nov 14, 2025

Choose a reason for hiding this comment

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

nit: maybe add a comment here that we are using this single credentials object?

Copy link
Contributor Author

@rmehta19 rmehta19 Nov 14, 2025

Choose a reason for hiding this comment

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

Good call, added a comment about this.


/*
* Experimental feature
*
Expand Down Expand Up @@ -556,13 +561,19 @@ ChannelCredentials buildS2AChannelCredentials(
*/
@VisibleForTesting
ChannelCredentials createMtlsToS2AChannelCredentials(
File trustBundle, File privateKey, File certChain) throws IOException {
File trustBundle, File privateKey, File certChain)
throws IOException, GeneralSecurityException {
if (trustBundle == null || privateKey == null || certChain == null) {
return null;
}
AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
keyManager.updateIdentityCredentials(certChain, privateKey);
AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();
Copy link

@gemini-code-assist gemini-code-assist bot Nov 14, 2025

Choose a reason for hiding this comment

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

high

Creating a new ScheduledExecutorService here without a mechanism to shut it down will cause a resource leak. The thread from this executor will run indefinitely and may prevent the JVM from shutting down gracefully.

The AdvancedTlsX509TrustManager documentation specifies that the caller is responsible for shutting down the executor. Since this executor is used for periodic credential refreshes, it should live for the duration of the application.

A better approach would be to use a shared, static ScheduledExecutorService configured with daemon threads. This ensures the background thread doesn't block application exit. For example:

private static final ScheduledExecutorService S2A_TRUST_MANAGER_EXECUTOR = 
    Executors.newSingleThreadScheduledExecutor(
        r -> {
          Thread t = new Thread(r, "s2a-trust-manager-updater");
          t.setDaemon(true);
          return t;
        });

You could then use this static executor in the updateTrustCredentials call.

trustManager.updateTrustCredentials(trustBundle, 1, TimeUnit.HOURS, executor);
return TlsChannelCredentials.newBuilder()
.keyManager(privateKey, certChain)
.trustManager(trustBundle)
.keyManager(keyManager)
.trustManager(trustManager)
.build();
}

Expand Down Expand Up @@ -595,43 +606,57 @@ ChannelCredentials createPlaintextToS2AChannelCredentials(String plaintextAddres
* @return {@link ChannelCredentials} configured to use S2A to create mTLS connection.
*/
ChannelCredentials createS2ASecuredChannelCredentials() {
SecureSessionAgentConfig config = s2aConfigProvider.getConfig();
String plaintextAddress = config.getPlaintextAddress();
String mtlsAddress = config.getMtlsAddress();
if (Strings.isNullOrEmpty(mtlsAddress)) {
// Fallback to plaintext connection to S2A.
LOG.log(
Level.INFO,
"Cannot establish an mTLS connection to S2A because autoconfig endpoint did not return a mtls address to reach S2A.");
return createPlaintextToS2AChannelCredentials(plaintextAddress);
}
// Currently, MTLS to MDS is only available on GCE. See:
// https://cloud.google.com/compute/docs/metadata/overview#https-mds
// Try to load MTLS-MDS creds.
File rootFile = new File(MTLS_MDS_ROOT_PATH);
File certKeyFile = new File(MTLS_MDS_CERT_CHAIN_AND_KEY_PATH);
if (rootFile.isFile() && certKeyFile.isFile()) {
// Try to connect to S2A using mTLS.
ChannelCredentials mtlsToS2AChannelCredentials = null;
try {
mtlsToS2AChannelCredentials =
createMtlsToS2AChannelCredentials(rootFile, certKeyFile, certKeyFile);
} catch (IOException ignore) {
// Fallback to plaintext-to-S2A connection on error.
LOG.log(
Level.WARNING,
"Cannot establish an mTLS connection to S2A due to error creating MTLS to MDS TlsChannelCredentials credentials, falling back to plaintext connection to S2A: "
+ ignore.getMessage());
return createPlaintextToS2AChannelCredentials(plaintextAddress);
if (s2aChannelCredentialsObject == null) {
synchronized (InstantiatingGrpcChannelProvider.class) {
if (s2aChannelCredentialsObject == null) {
Comment on lines +611 to +613
Copy link

@xmenxk xmenxk Nov 14, 2025

Choose a reason for hiding this comment

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

curious why doing the null check on both line 609 and 611?

Copy link
Contributor Author

@rmehta19 rmehta19 Nov 14, 2025

Choose a reason for hiding this comment

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

Thanks for reviewing! This is part of the double checked locking pattern, WDYT?

SecureSessionAgentConfig config = s2aConfigProvider.getConfig();
String plaintextAddress = config.getPlaintextAddress();
String mtlsAddress = config.getMtlsAddress();
if (Strings.isNullOrEmpty(mtlsAddress)) {
// Fallback to plaintext connection to S2A.
LOG.log(
Level.INFO,
"Cannot establish an mTLS connection to S2A because autoconfig endpoint did not return a mtls address to reach S2A.");
s2aChannelCredentialsObject = createPlaintextToS2AChannelCredentials(plaintextAddress);
return s2aChannelCredentialsObject;
}
// Currently, MTLS to MDS is only available on GCE. See:
// https://cloud.google.com/compute/docs/metadata/overview#https-mds
// Try to load MTLS-MDS creds.
File rootFile = new File(MTLS_MDS_ROOT_PATH);
File certKeyFile = new File(MTLS_MDS_CERT_CHAIN_AND_KEY_PATH);
if (rootFile.isFile() && certKeyFile.isFile()) {
// Try to connect to S2A using mTLS.
ChannelCredentials mtlsToS2AChannelCredentials = null;
try {
mtlsToS2AChannelCredentials =
createMtlsToS2AChannelCredentials(rootFile, certKeyFile, certKeyFile);
} catch (IOException | GeneralSecurityException ignore) {
// Fallback to plaintext-to-S2A connection on error.
LOG.log(
Level.WARNING,
"Cannot establish an mTLS connection to S2A due to error creating MTLS to MDS TlsChannelCredentials credentials, falling back to plaintext connection to S2A: "
+ ignore.getMessage());
s2aChannelCredentialsObject =
createPlaintextToS2AChannelCredentials(plaintextAddress);
return s2aChannelCredentialsObject;
}
s2aChannelCredentialsObject =
buildS2AChannelCredentials(mtlsAddress, mtlsToS2AChannelCredentials);
return s2aChannelCredentialsObject;
} else {
// Fallback to plaintext-to-S2A connection if MTLS-MDS creds do not exist.
LOG.log(
Level.INFO,
"Cannot establish an mTLS connection to S2A because MTLS to MDS credentials do not"
+ " exist on filesystem, falling back to plaintext connection to S2A");
s2aChannelCredentialsObject = createPlaintextToS2AChannelCredentials(plaintextAddress);
return s2aChannelCredentialsObject;
}
}
}
return buildS2AChannelCredentials(mtlsAddress, mtlsToS2AChannelCredentials);
} else {
// Fallback to plaintext-to-S2A connection if MTLS-MDS creds do not exist.
LOG.log(
Level.INFO,
"Cannot establish an mTLS connection to S2A because MTLS to MDS credentials do not exist on filesystem, falling back to plaintext connection to S2A");
return createPlaintextToS2AChannelCredentials(plaintextAddress);
}
return s2aChannelCredentialsObject;
}
Comment on lines 610 to 662
Copy link

@gemini-code-assist gemini-code-assist bot Nov 14, 2025

Choose a reason for hiding this comment

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

medium

The logic inside the synchronized block has become quite complex and contains duplicated code, particularly the fallback to createPlaintextToS2AChannelCredentials(plaintextAddress). This makes the method harder to read and maintain.

Consider refactoring this block to simplify the control flow. A clearer structure would be to attempt creating mTLS credentials first, and if that fails for any reason, then fall back to creating plaintext credentials. This would centralize the fallback logic.

For example, you could extract the creation logic into a helper method that returns the created credentials, which simplifies the double-checked locking block:

// In createS2ASecuredChannelCredentials():
if (s2aChannelCredentialsObject == null) {
  synchronized (InstantiatingGrpcChannelProvider.class) {
    if (s2aChannelCredentialsObject == null) {
      s2aChannelCredentialsObject = createS2ACredentialsOnce();
    }
  }
}
return s2aChannelCredentialsObject;

// New helper method:
private ChannelCredentials createS2ACredentialsOnce() {
  // ... logic to try mTLS and fallback to plaintext ...
}


private ManagedChannel createSingleChannel() throws IOException {
Expand Down
Loading