Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
89428d6
feat(gax): support transparent retries during mTLS certificate rotations
macastelaz Aug 5, 2026
49772f0
fix(gax): address mTLS cert config parsing and channel refresh lifecy…
macastelaz Aug 5, 2026
a2210c6
fix(gax): address PR 13995 AI review findings and Javadoc doclint errors
macastelaz Aug 5, 2026
11bef87
fix(gax): release channel entry on early cancellation before call start
macastelaz Aug 5, 2026
9fcb21f
fix(gax): improve mTLS certificate config validation and policy case …
macastelaz Aug 5, 2026
b69d12d
test(gax): add unit tests for mTLS endpoint policy case-sensitivity a…
macastelaz Aug 5, 2026
a266da7
refactor(auth,gax): consolidate mTLS discovery into auth library per …
macastelaz Aug 10, 2026
be0a495
fix(auth,gax): address PR 13995 review feedback and CI test failures
macastelaz Aug 10, 2026
9be88f6
fix(auth,gax): align mTLS certificate discovery and error handling wi…
macastelaz Aug 18, 2026
765eb3b
test(auth): add unit test in MtlsUtilsTest to provide coverage for EC…
macastelaz Aug 18, 2026
4904aad
fix(auth,gax-grpc): address PR 13995 review feedback on cert discover…
macastelaz Aug 27, 2026
10535d6
fix(gax,gax-grpc,gax-httpjson): address PR 13995 review feedback on r…
macastelaz Aug 28, 2026
a97680b
fix(gax-grpc): use javax.annotation.concurrent.GuardedBy to satisfy d…
macastelaz Aug 28, 2026
7921396
fix(gax): address review feedback for mTLS certificate rotation retri…
macastelaz Sep 18, 2026
92135bf
fix(gax-httpjson,gax-grpc): fix Conscrypt mTLS KeyManagerFactory init…
macastelaz Sep 18, 2026
6ec0bc9
fix(gax-httpjson): add withoutAnnotations() to MtlsProvider mock for …
macastelaz Sep 18, 2026
2f549e2
refactor(gax): make RetryingContext overload primary and simplify str…
macastelaz Sep 18, 2026
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
Comment thread
nbayati marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.security.MessageDigest;
import java.util.Locale;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;

/**
* Utility class for mTLS related operations.
Expand All @@ -57,6 +59,176 @@ private MtlsUtils() {
// Prevent instantiation for Utility class
}

/**
* Returns if mutual TLS client certificate should be used. Delegates directly to
* getWorkloadCertPath to avoid duplicate logic.
*/
public static boolean useMtlsClientCertificate(
EnvironmentProvider envProvider, PropertyProvider propProvider) {
return getWorkloadCertPath(envProvider, propProvider) != null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previously, useMtlsClientCertificate() acted as a simple check for whether GOOGLE_API_USE_CLIENT_CERTIFICATE=true, telling GAX whether to attempt mTLS. This PR changes it to check getWorkloadCertPath(envProvider, propProvider) != null, which only looks for X.509 workload certificates (cert_configs.workload) on disk.

Because getWorkloadCertPath() returns null when no workload certificate file is present, useMtlsClientCertificate() now returns false for ECP setups and custom MtlsProvider instances, even when the user explicitly sets GOOGLE_API_USE_CLIENT_CERTIFICATE=true. As a result, GAX treats mTLS as disabled, skips ECP initialization (SecureConnectProvider), and silently downgrades requests to regular TLS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops - thanks for catching this! I've updated MtlsUtils.useMtlsClientCertificate(...) to:

  1. Return false if GOOGLE_API_USE_CLIENT_CERTIFICATE is "false".
  2. Return true if getWorkloadCertPath(envProvider, propProvider) != null (which also preserves fail-closed validation for malformed config files).
  3. Fall back to "true".equalsIgnoreCase(useClientCertificate) when no workload cert is present, restoring full ECP and custom MtlsProvider support.

Updated MtlsUtilsTest and CertificateBasedAccessTest accordingly.

}

/**
* Resolves and returns the path to the mutual TLS client certificate, or null if none should be
* used.
Comment on lines +84 to +85

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Can we enhance the javadocs a bit more explain the different possible outputs

From what I can see, there seem to be these three:

  1. String -> Valid happy path with working, readable file
  2. Exception -> Invalid State (non-readable or invalid file) and this isn't recoverable and will just fail
  3. Null -> Othercase, but does this indicate that we can proceed (sort of like a fail open)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done - the javadoc now covers all three possible outcomes

*/
public static @Nullable String getWorkloadCertPath(
EnvironmentProvider envProvider, PropertyProvider propProvider) {
String useClientCertificate = envProvider.getEnv("GOOGLE_API_USE_CLIENT_CERTIFICATE");
if ("false".equalsIgnoreCase(useClientCertificate)) {
return null;
}

String explicitConfigPath = envProvider.getEnv(CERTIFICATE_CONFIGURATION_ENV_VARIABLE);

// 1. Explicit Configuration Path (Fail Closed)
if (!Strings.isNullOrEmpty(explicitConfigPath)) {
File configFile = new File(explicitConfigPath);
if (!configFile.exists()) {
throw new IllegalStateException(
"Certificate configuration file specified via GOOGLE_API_CERTIFICATE_CONFIG at '"
+ explicitConfigPath
+ "' does not exist.");
}
if (!configFile.isFile() || !configFile.canRead()) {
throw new IllegalStateException(
"Failed to read certificate configuration file specified via"
+ " GOOGLE_API_CERTIFICATE_CONFIG at '"
+ explicitConfigPath
+ "'.");
}
try {
WorkloadCertificateConfiguration config =
getWorkloadCertificateConfiguration(envProvider, propProvider, explicitConfigPath);
validateCertAndKeyFiles(config, explicitConfigPath, false);
return config.getCertPath();
} catch (CertificateSourceUnavailableException e) {
// ECP / PKCS11 configuration without workload section; safe fallback
return null;
} catch (IllegalStateException e) {
throw e;
Comment thread
macastelaz marked this conversation as resolved.
Outdated
} catch (Exception e) {
throw new IllegalStateException(
"Certificate configuration file specified via GOOGLE_API_CERTIFICATE_CONFIG at '"
+ explicitConfigPath
+ "' is malformed: "
+ e.getMessage(),
e);
}
}

// 2. Implicit / Default gcloud Configuration Path
File defaultConfigFile = null;
try {
defaultConfigFile = getWellKnownCertificateConfigFile(envProvider, propProvider);
} catch (IOException e) {
// APPDATA missing on Windows, etc. Safe fallback.
}
if (defaultConfigFile != null && defaultConfigFile.exists()) {
if (!defaultConfigFile.isFile() || !defaultConfigFile.canRead()) {
throw new IllegalStateException(
"Default certificate configuration file at '"
+ defaultConfigFile.getAbsolutePath()
+ "' exists but could not be read.");
}
try {
WorkloadCertificateConfiguration config =
getWorkloadCertificateConfiguration(envProvider, propProvider, null);
validateCertAndKeyFiles(config, defaultConfigFile.getAbsolutePath(), true);
return config.getCertPath();
} catch (CertificateSourceUnavailableException e) {
// ECP-only configuration without workload section; safe fallback
} catch (IllegalStateException e) {
throw e;
} catch (Exception e) {
throw new IllegalStateException(
"Default certificate configuration file at '"
+ defaultConfigFile.getAbsolutePath()
+ "' is malformed: "
+ e.getMessage(),
e);
}
}

// 3. Platform SPIFFE Fallbacks (Stubs)
String gkeCertPath = getGkeWorkloadCertPath();
if (gkeCertPath != null) {
return gkeCertPath;
}

String gceCertPath = getGceWorkloadCertPath();
if (gceCertPath != null) {
return gceCertPath;
}
Comment thread
macastelaz marked this conversation as resolved.
Outdated

return null;
}

private static void validateCertAndKeyFiles(
Comment thread
macastelaz marked this conversation as resolved.
Outdated
WorkloadCertificateConfiguration config, String configPath, boolean isDefaultConfig) {
File certFile = new File(config.getCertPath());
File keyFile = new File(config.getPrivateKeyPath());
if (!certFile.isFile() || !certFile.canRead() || !keyFile.isFile() || !keyFile.canRead()) {
String sourcePrefix =
isDefaultConfig
? "referenced by default configuration '"
: "referenced by configuration '";
throw new IllegalStateException(
"Failed to read certificate/key file at '"
+ config.getCertPath()
+ "' or '"
+ config.getPrivateKeyPath()
+ "' "
+ sourcePrefix
+ configPath
+ "'.");
}
}

/** Dedicated GKE Fallback Resolution Path */
static @Nullable String getGkeWorkloadCertPath() {
// GKE workload certificate resolution is temporarily disabled (returns null)
// pending Phase 1 rollout of bound token support on GKE
// (go/agentic-bound-token-sdk-rollout-plan).
Comment thread
macastelaz marked this conversation as resolved.
Outdated
return null;
}

/** Dedicated GCE Fallback Resolution Path */
static @Nullable String getGceWorkloadCertPath() {
// GCE workload certificate resolution is temporarily disabled (returns null)
// pending Phase 2 rollout of bound token support on GCE
// (go/agentic-bound-token-sdk-rollout-plan).
return null;
}

/** Centralized SHA-256 Fingerprint Calculator */
public static @Nullable String getCertificateFingerprint(@Nullable String certPath) {
if (certPath == null) {
return null;
}
File file = new File(certPath);
if (!file.isFile() || !file.canRead()) {
return null;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

qq, this seems to diverge from the above where we throw an IllegalStateException. Is there a reason for an exception here now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question! The distinction comes down to configuration validation vs. best-effort digest calculation across both initialization and runtime:
- getWorkloadCertPath(...) enforces the mTLS configuration contract: if an explicit or default certificate_config.json is malformed or points to non-existent/unreadable paths, we fail closed (IllegalStateException) so we never silently downgrade an intended mTLS setup to regular TLS.
- getCertificateFingerprint(@Nullable String certPath) is a best-effort SHA-256 helper used both when initializing activeCertFingerprint and when polling during live RPCs (shouldRefresh() / refresh()). Because external certificate rotators may temporarily truncate or rewrite the file on disk either during startup or mid-RPC:
- At init time, returning null (normalized to activeCertFingerprint = "") allows initialization to proceed with an empty baseline so that as soon as the write completes on disk, !currentDiskFingerprint.equals("") triggers a refresh to load the valid cert.
- At runtime, returning null (normalized to "") allows shouldRefresh() / refresh() to short-circuit on currentDiskFingerprint.isEmpty() and keep using the active healthy channel until the file write completes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like the logic throughout the PR already checks the certPath and that the file is readable (asserted above in getWorkloadCertPath).

Would it be possible for this method to be simplified by just reading the bytes and doing the encoding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed! Simplified MtlsUtils.getCertificateFingerprint(...) by removing the redundant File.isFile() / File.canRead() checks and directly calling Files.readAllBytes(Paths.get(certPath)) and computing the SHA-256 hex digest inside the try / catch block.

try {
MessageDigest digest = MessageDigest.getInstance("SHA-256");
try (FileInputStream fis = new FileInputStream(file)) {
byte[] byteArray = new byte[1024];
int bytesCount;
while ((bytesCount = fis.read(byteArray)) != -1) {
digest.update(byteArray, 0, bytesCount);
}
}
StringBuilder sb = new StringBuilder();
for (byte b : digest.digest()) {
sb.append(String.format("%02x", b));
}
return sb.toString();
Comment thread
macastelaz marked this conversation as resolved.
Outdated
} catch (Exception e) {
return null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are the consequences of returning null here and how realistic would be for this file to not be able to be parsed?

The possible scenario I have in my head is this: activeFingerprint is say abc1234 and then the cert is rotated. Fingerprint is unable to be parsed and returns null and I believe will be converted to "". IIUC, that would be a fingerprint mismatch but not the intended effect (I could be wrong on this point).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That scenario is explicitly guarded against:

WhengetCertificateFingerprint(...) returns null (or when the file is temporarily 0 bytes mid-write), WorkloadCertificateUtils.getCertificateFingerprint(...) normalizes it to "". Both shouldRefresh() and refresh() explicitly check if(currentDiskFingerprint.isEmpty()) return false; (or return;) before evaluating !currentDiskFingerprint.equalsIgnoreCase(activeCertFingerprint).

As a result, an unreadable or empty mid-write file is never treated as a fingerprint mismatch—it safely short-circuits and keeps the existing active channel in use until the new certificate file finishes writing to disk. Added Javadoc to WorkloadCertificateUtils.getCertificateFingerprint(...) documenting this contract.

}
}

/**
* Returns the path to the client certificate file specified by the loaded workload certificate
* configuration.
Expand All @@ -65,14 +237,17 @@ private MtlsUtils() {
* @throws IOException if the certificate configuration cannot be found or loaded.
*/
public static String getCertificatePath(
EnvironmentProvider envProvider, PropertyProvider propProvider, String certConfigPathOverride)
EnvironmentProvider envProvider,
PropertyProvider propProvider,
@Nullable String certConfigPathOverride)
throws IOException {
String certPath =
getWorkloadCertificateConfiguration(envProvider, propProvider, certConfigPathOverride)
.getCertPath();
if (Strings.isNullOrEmpty(certPath)) {
throw new CertificateSourceUnavailableException(
"Certificate configuration loaded successfully, but does not contain a 'certificate_file' path.");
"Certificate configuration loaded successfully, but does not contain a"
+ " 'cert_configs.workload.cert_path' path.");
}
return certPath;
}
Expand All @@ -92,7 +267,9 @@ public static String getCertificatePath(
* @throws IOException if the configuration file cannot be found, read, or parsed
*/
static WorkloadCertificateConfiguration getWorkloadCertificateConfiguration(
EnvironmentProvider envProvider, PropertyProvider propProvider, String certConfigPathOverride)
EnvironmentProvider envProvider,
PropertyProvider propProvider,
@Nullable String certConfigPathOverride)
throws IOException {
File certConfig;
if (certConfigPathOverride != null) {
Expand All @@ -106,7 +283,7 @@ static WorkloadCertificateConfiguration getWorkloadCertificateConfiguration(
}
}

if (!certConfig.isFile()) {
if (!certConfig.isFile() || !certConfig.canRead()) {
throw new CertificateSourceUnavailableException(
"Certificate configuration file does not exist or is not a file: "
+ certConfig.getAbsolutePath());
Expand Down
Loading
Loading