-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext [blocked on #13995] #14556
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.
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
Draft
macastelaz
wants to merge
18
commits into
googleapis:agentic-identities-bound-token
Choose a base branch
from
macastelaz:fix-conscrypt-tmf-13995
base: agentic-identities-bound-token
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
89428d6
feat(gax): support transparent retries during mTLS certificate rotations
macastelaz 49772f0
fix(gax): address mTLS cert config parsing and channel refresh lifecy…
macastelaz a2210c6
fix(gax): address PR 13995 AI review findings and Javadoc doclint errors
macastelaz 11bef87
fix(gax): release channel entry on early cancellation before call start
macastelaz 9fcb21f
fix(gax): improve mTLS certificate config validation and policy case …
macastelaz b69d12d
test(gax): add unit tests for mTLS endpoint policy case-sensitivity a…
macastelaz a266da7
refactor(auth,gax): consolidate mTLS discovery into auth library per …
macastelaz be0a495
fix(auth,gax): address PR 13995 review feedback and CI test failures
macastelaz 9be88f6
fix(auth,gax): align mTLS certificate discovery and error handling wi…
macastelaz 765eb3b
test(auth): add unit test in MtlsUtilsTest to provide coverage for EC…
macastelaz 4904aad
fix(auth,gax-grpc): address PR 13995 review feedback on cert discover…
macastelaz 10535d6
fix(gax,gax-grpc,gax-httpjson): address PR 13995 review feedback on r…
macastelaz a97680b
fix(gax-grpc): use javax.annotation.concurrent.GuardedBy to satisfy d…
macastelaz 7921396
fix(gax): address review feedback for mTLS certificate rotation retri…
macastelaz 92135bf
fix(gax-httpjson,gax-grpc): fix Conscrypt mTLS KeyManagerFactory init…
macastelaz 6ec0bc9
fix(gax-httpjson): add withoutAnnotations() to MtlsProvider mock for …
macastelaz 2f549e2
refactor(gax): make RetryingContext overload primary and simplify str…
macastelaz c6fcb75
fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLCont…
macastelaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,12 +34,17 @@ | |||||||||||||||||
| import com.google.auth.oauth2.EnvironmentProvider; | ||||||||||||||||||
| import com.google.auth.oauth2.PropertyProvider; | ||||||||||||||||||
| import com.google.common.base.Strings; | ||||||||||||||||||
| import com.google.common.io.BaseEncoding; | ||||||||||||||||||
| import java.io.File; | ||||||||||||||||||
| import java.io.FileInputStream; | ||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||
| import java.io.InputStream; | ||||||||||||||||||
| import java.nio.file.Files; | ||||||||||||||||||
| import java.nio.file.Paths; | ||||||||||||||||||
| import java.security.MessageDigest; | ||||||||||||||||||
| import java.util.Locale; | ||||||||||||||||||
| import org.jspecify.annotations.NullMarked; | ||||||||||||||||||
| import org.jspecify.annotations.Nullable; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Utility class for mTLS related operations. | ||||||||||||||||||
|
|
@@ -57,6 +62,168 @@ private MtlsUtils() { | |||||||||||||||||
| // Prevent instantiation for Utility class | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Returns if mutual TLS client certificate should be used. Returns true if valid workload | ||||||||||||||||||
| * certificates are configured or if GOOGLE_API_USE_CLIENT_CERTIFICATE is explicitly set to true | ||||||||||||||||||
| * (e.g. for Enterprise Certificate Proxy or custom MtlsProviders), unless explicitly disabled via | ||||||||||||||||||
| * GOOGLE_API_USE_CLIENT_CERTIFICATE=false. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static boolean useMtlsClientCertificate( | ||||||||||||||||||
| EnvironmentProvider envProvider, PropertyProvider propProvider) { | ||||||||||||||||||
| String useClientCertificate = envProvider.getEnv("GOOGLE_API_USE_CLIENT_CERTIFICATE"); | ||||||||||||||||||
| if ("false".equalsIgnoreCase(useClientCertificate)) { | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (getWorkloadCertPath(envProvider, propProvider) != null) { | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
| return "true".equalsIgnoreCase(useClientCertificate); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Resolves and returns the path to the mutual TLS client certificate, or null if none should be | ||||||||||||||||||
| * used. | ||||||||||||||||||
| * | ||||||||||||||||||
| * <p>Possible outcomes: | ||||||||||||||||||
| * | ||||||||||||||||||
| * <ol> | ||||||||||||||||||
| * <li><b>Non-null {@link String} (Valid happy path):</b> A valid workload certificate | ||||||||||||||||||
| * configuration was found and both the certificate and private key files exist and are | ||||||||||||||||||
| * readable. | ||||||||||||||||||
| * <li><b>{@link IllegalStateException} (Invalid state - fail closed):</b> An explicit {@code | ||||||||||||||||||
| * GOOGLE_API_CERTIFICATE_CONFIG} path or an existing default well-known certificate | ||||||||||||||||||
| * configuration file is missing, unreadable, malformed, or references missing/unreadable | ||||||||||||||||||
| * certificate or private key files. This is treated as an unrecoverable misconfiguration. | ||||||||||||||||||
| * <li><b>{@code null} (Safe fallback / fail open):</b> Client certificates are explicitly | ||||||||||||||||||
| * disabled via {@code GOOGLE_API_USE_CLIENT_CERTIFICATE=false}, no explicit configuration | ||||||||||||||||||
| * is set and the default well-known configuration file does not exist on disk, or the | ||||||||||||||||||
| * configuration specifies an non-workload source (e.g., ECP/PKCS11 without a {@code | ||||||||||||||||||
| * workload} section). Callers can proceed without workload certificate file polling. | ||||||||||||||||||
| * </ol> | ||||||||||||||||||
| */ | ||||||||||||||||||
| 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 | ||||||||||||||||||
| + "'."); | ||||||||||||||||||
| } | ||||||||||||||||||
| WorkloadCertificateConfiguration config; | ||||||||||||||||||
| try { | ||||||||||||||||||
| config = getWorkloadCertificateConfiguration(envProvider, propProvider, explicitConfigPath); | ||||||||||||||||||
| } catch (CertificateSourceUnavailableException e) { | ||||||||||||||||||
| // ECP / PKCS11 configuration without workload section; safe fallback | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||
| throw new IllegalStateException( | ||||||||||||||||||
| "Certificate configuration file specified via GOOGLE_API_CERTIFICATE_CONFIG at '" | ||||||||||||||||||
| + explicitConfigPath | ||||||||||||||||||
| + "' is malformed: " | ||||||||||||||||||
| + e.getMessage(), | ||||||||||||||||||
| e); | ||||||||||||||||||
| } | ||||||||||||||||||
| checkCertAndKeyFilesReadable(config, explicitConfigPath, false); | ||||||||||||||||||
| return config.getCertPath(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // 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."); | ||||||||||||||||||
| } | ||||||||||||||||||
| WorkloadCertificateConfiguration config = null; | ||||||||||||||||||
| try { | ||||||||||||||||||
| config = getWorkloadCertificateConfiguration(envProvider, propProvider, null); | ||||||||||||||||||
| } catch (CertificateSourceUnavailableException e) { | ||||||||||||||||||
| // ECP-only configuration without workload section; safe fallback | ||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||
| throw new IllegalStateException( | ||||||||||||||||||
| "Default certificate configuration file at '" | ||||||||||||||||||
| + defaultConfigFile.getAbsolutePath() | ||||||||||||||||||
| + "' is malformed: " | ||||||||||||||||||
| + e.getMessage(), | ||||||||||||||||||
| e); | ||||||||||||||||||
| } | ||||||||||||||||||
| if (config != null) { | ||||||||||||||||||
| checkCertAndKeyFilesReadable(config, defaultConfigFile.getAbsolutePath(), true); | ||||||||||||||||||
| return config.getCertPath(); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private static void checkCertAndKeyFilesReadable( | ||||||||||||||||||
| 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 | ||||||||||||||||||
| + "'."); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Computes the lower-case SHA-256 hex fingerprint of the certificate file at {@code certPath}. | ||||||||||||||||||
| * | ||||||||||||||||||
| * <p>Unlike {@link #getWorkloadCertPath}, which validates configuration at channel initialization | ||||||||||||||||||
| * and fails closed on errors, this method is called dynamically at runtime during active RPCs to | ||||||||||||||||||
| * detect certificate rotations on disk. External certificate rotators may temporarily delete, | ||||||||||||||||||
| * truncate, or rewrite the certificate file mid-RPC. Returning {@code null} on read/digest | ||||||||||||||||||
| * exceptions (which callers normalize to {@code ""}) allows runtime refresh checks to ignore | ||||||||||||||||||
| * transient mid-write states and keep the active healthy channel without failing in-flight RPCs. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static @Nullable String getCertificateFingerprint(@Nullable String certPath) { | ||||||||||||||||||
| if (certPath == null) { | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
| try { | ||||||||||||||||||
| byte[] certBytes = Files.readAllBytes(Paths.get(certPath)); | ||||||||||||||||||
| byte[] digest = MessageDigest.getInstance("SHA-256").digest(certBytes); | ||||||||||||||||||
| return BaseEncoding.base16().lowerCase().encode(digest); | ||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Returns the path to the client certificate file specified by the loaded workload certificate | ||||||||||||||||||
| * configuration. | ||||||||||||||||||
|
|
@@ -65,14 +232,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; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -92,7 +262,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) { | ||||||||||||||||||
|
|
@@ -106,7 +278,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()); | ||||||||||||||||||
|
Comment on lines
+281
to
284
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception message only mentions that the file does not exist or is not a file, but the condition also checks
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
config.getCertPath()orconfig.getPrivateKeyPath()is null or empty, passing them tonew File()will result in aNullPointerException. It is safer to validate that these paths are non-empty before attempting to instantiateFileobjects.