feat(mtls): Add support for X.509-based mTLS-transport in Java GAX lib #3852
Conversation
…ateBasedAccess helper
…vider in channel providers.
| @@ -150,6 +153,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP | |||
| @Nullable private final Boolean allowNonDefaultServiceAccount; | |||
| @VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig; | |||
| @Nullable private final MtlsProvider mtlsProvider; | |||
| @Nullable private final CertificateBasedAccess certificateBasedAccess; | |||
There was a problem hiding this comment.
qq, I believe below has a default value for the certificateBasedAccess. What's the reason this should be Nullable?
There was a problem hiding this comment.
So the "CertificateBasedAccess" is essentially a couple of helper functions that was moved out of the original MtlsProvider implementation, and since the previous MtlsProvider was Nullable, this was also marked Nullable to retain the same semantics - my hunch is that we should keep it Nullable for maximum compatibility/flexibility.
| KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); | ||
| factory.init(mtlsKeyStore, new char[] {}); | ||
| return TlsChannelCredentials.newBuilder().keyManager(factory.getKeyManagers()).build(); | ||
| if (certificateBasedAccess != null && certificateBasedAccess.useMtlsClientCertificate()) { |
There was a problem hiding this comment.
similar to above, do we need the null check for certificateBasedAccess?
There was a problem hiding this comment.
See above, I think I'd rather keep the @nullable and null-check to retain original semantics. (Hypothetically, if we get back a null from CertificateBasedAccess.createWithSystemEnv, or a different implementation, we would want to fallback gracefully here.)
| try { | ||
| mtlsProvider = DefaultMtlsProviderFactory.create(); | ||
| } catch (CertificateSourceUnavailableException e) { | ||
| // This is okay. Leave mtlsProvider as null; |
There was a problem hiding this comment.
qq, can you elaborate why we can leave mtlsProvider as null here? What happens if leave this as null?
There was a problem hiding this comment.
This ties back to https://google.aip.dev/auth/4114. If mtlsProvider is null, the SDK will not automatically use mTLS endpoints. Let me know if you'd like me to elaborate this (or mention AIP 4114) in the code comment itself.
| factory.init(mtlsKeyStore, new char[] {}); | ||
| return TlsChannelCredentials.newBuilder().keyManager(factory.getKeyManagers()).build(); | ||
| if (certificateBasedAccess != null && certificateBasedAccess.useMtlsClientCertificate()) { | ||
| if (mtlsProvider != null) { |
There was a problem hiding this comment.
nit: can we refactor the null checks to fail fast to reduce nesting?
i.e. if (mtlsProvider == null) return null
| @@ -92,6 +93,7 @@ class InstantiatingGrpcChannelProviderTest extends AbstractMtlsTransportChannelT | |||
| private static final String API_KEY_AUTH_HEADER_KEY = "x-goog-api-key"; | |||
| private static String originalOSName; | |||
| private ComputeEngineCredentials computeEngineCredentials; | |||
| private CertificateBasedAccess cba; | |||
There was a problem hiding this comment.
nit: can you spell out the field variable name
| import com.google.api.gax.rpc.internal.EnvironmentProvider; | ||
|
|
||
| /** Utility class for handling certificate-based access configurations. */ | ||
| public class CertificateBasedAccess { |
There was a problem hiding this comment.
Can this class be package-private?
Regarding this class: Is the plan for this class to be a wrapper to be able to read the user env var GOOGLE_API_USE_CLIENT_CERTIFICATE to determine mtls usage policy? If so, can you add that to the javadocs for this class?
There was a problem hiding this comment.
Will add documentation and reference to AIP 4114 here. But this class cannot be made package-private AFAIK, since it is being widely used in unit tests (and across packages).
Fixes #3851