chore: Refactor x509Provider to create a shared Utils class for Mtls#1907
chore: Refactor x509Provider to create a shared Utils class for Mtls#1907vverman wants to merge 10 commits intogoogleapis:mainfrom
Conversation
vverman
force-pushed
the
refactor-x509-provider
branch
from
1500bea to
f78ed98
Compare
| * | ||
| * <p>For internal use only. | ||
| */ | ||
| public class SystemEnvironmentProvider implements EnvironmentProvider, Serializable { |
There was a problem hiding this comment.
qq, what is the difference between SystemEnvironmentProvider and EnvironmentProvider?
There was a problem hiding this comment.
An EnvProvider is an interface which can be implemented by test-classes to pass in their own env variables.
The SystemEnvProvider implements this EnvProvider and implements the associated functions such that the env-variables are fetched from the system env.
There was a problem hiding this comment.
Got it. Can you add @internalapi to this class and for SystemPropertyProvider?
Since this is a refactor, I'm going to change the title to |
lqiu96
changed the title
|
E
El martes, 24 de marzo de 2026, ***@***.***> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In oauth2_http/java/com/google/auth/mtls/X509Provider.java
<#1907 (comment)>
:
> + public X509Provider(
+ EnvironmentProvider envProvider,
+ PropertyProvider propProvider,
+ String certConfigPathOverride) {
Thanks for the suggestion.
I think we can leave this one out for now because I don't think the
argument list is big enough to justify the Builder's inclusion and we don't
anticipate any changes to X509Provider (in the short-term).
In case this changes, we can implement it then.
—
Reply to this email directly, view it on GitHub
<#1907?email_source=notifications&email_token=B5MKPO3RH26N37TDJ35VCOL4SL3TJA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGIZTKMBWGUYKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r2984325705>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B5MKPOZWHPYIKAJ5GNPMM5L4SL3TJAVCNFSM6AAAAACW43RKUSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBSGM2TANRVGA>
.
You are receiving this because you are subscribed to this thread.Message
ID: <googleapis/google-auth-library-java/pull/1907/review/4002350650@
github.com>
|
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class TestPropertyProvider implements PropertyProvider { |
There was a problem hiding this comment.
qq, is this TestPropertyProvider used anywhere in the tests? If not, can we remove it?
There was a problem hiding this comment.
Since this is used to test the System Properties, added in a test for the windows OS default file path in X509ProviderTest.
| } else { | ||
| String osName = propProvider.getProperty("os.name", "").toLowerCase(Locale.US); | ||
| if (osName.indexOf("windows") >= 0) { | ||
| File appDataPath = new File(envProvider.getEnv("APPDATA")); |
There was a problem hiding this comment.
If APPDATA is unset on Windows, envProvider.getEnv("APPDATA") returns null. Creating a new File(null) throws a NullPointerException.
We can align with how Linux handles its user.home fallback by checking user.home as a fallback when APPDATA is missing.
There was a problem hiding this comment.
I believe this is the same logic to detect the default 'directory' where the ADC file is, within the GoogleAuthUtils.getWellKnownCredentialsFile
I believe this should stay as is since the discovery for ADC and cert config should be the same and having a default for windows might lead to an unexpected outcome for cert_config as compared to ADC.
Do lmk your thoughts.
There was a problem hiding this comment.
That is fair, but I think we should at least check for null here, even if we don't want to fallback on user.home.
I've opened #1910 to fix it the potential nullpointer exception in the ADC flow too.
| @@ -0,0 +1,52 @@ | |||
| /* | |||
| * Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
| * Copyright 2025 Google LLC | |
| * Copyright 2026 Google LLC |
| @@ -0,0 +1,43 @@ | |||
| /* | |||
| * Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
| * Copyright 2025 Google LLC | |
| * Copyright 2026 Google LLC |
| @@ -0,0 +1,56 @@ | |||
| /* | |||
| * Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
| * Copyright 2025 Google LLC | |
| * Copyright 2026 Google LLC |
|
|
||
| /** Interface for an environment provider. */ | ||
| interface EnvironmentProvider { | ||
| import com.google.api.core.InternalApi; |
There was a problem hiding this comment.
I know this is an existing file, but it's missing the copyright header and maybe we can add it in this PR.
|
|
||
| private X509Provider getX509Provider( | ||
| Builder builder, IdentityPoolCredentialSource credentialSource) { | ||
| final IdentityPoolCredentialSource.CertificateConfig certConfig = |
There was a problem hiding this comment.
with the changes to this method, now certConfig is not used anymore and should be removed.
| private static String getExplicitCertConfigPath(IdentityPoolCredentialSource credentialSource) { | ||
| IdentityPoolCredentialSource.CertificateConfig certConfig = | ||
| credentialSource.getCertificateConfig(); | ||
| return certConfig.useDefaultCertificateConfig() |
There was a problem hiding this comment.
Potential null pointer access: The variable certConfig may be null at this location
There was a problem hiding this comment.
Since MtlsUtils is a new utility class that is now consumed by multiple classes (IdentityPoolCredentials and X509Provider), it would be great if we could add a dedicated MtlsUtilsTest.java file and ensure that MtlsUtils was fully tested.
There was a problem hiding this comment.
The current tests verify the success path and the 'missing config' path, but we could add test coverage for when the config exists but the referenced certificate/key files are missing or malformed.
Let's add tests for these two scenarios:
What happens if cert.pem doesn't exist?
What happens if the .pem contains invalid text?
4 participants
Fixes #1745
Addressed a concern raised by Andy refer.
Now X509Provider only exposes the necessary methods needed by the MtlsProvider interface