-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(auth): mTLS endpoint for Regional Access Boundaries #13318
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
base: regional-access-boundaries
Are you sure you want to change the base?
Changes from all commits
2429a2b
619805f
6d89929
c4e24f1
4c8b061
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,19 @@ public class MtlsUtils { | |
| static final String WELL_KNOWN_CERTIFICATE_CONFIG_FILE = "certificate_config.json"; | ||
| static final String CLOUDSDK_CONFIG_DIRECTORY = "gcloud"; | ||
|
|
||
| @com.google.common.annotations.VisibleForTesting | ||
| static String spiffeDirectory = "/var/run/secrets/workload-spiffe-credentials/"; | ||
|
|
||
| static final String SPIFFE_CREDENTIAL_BUNDLE_FILE = "credentialbundle.pem"; | ||
| static final String SPIFFE_CERTIFICATE_FILE = "certificates.pem"; | ||
| static final String SPIFFE_PRIVATE_KEY_FILE = "private_key.pem"; | ||
|
|
||
| public enum MtlsEndpointUsagePolicy { | ||
| ALWAYS, | ||
| NEVER, | ||
| AUTO | ||
| } | ||
|
Comment on lines
+61
to
+65
Member
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. nit: Add some small javadoc/ comments for this public enum. I think we can reference the AIP as well: https://google.aip.dev/auth/4114 |
||
|
|
||
| private MtlsUtils() { | ||
| // Prevent instantiation for Utility class | ||
| } | ||
|
|
@@ -137,4 +150,98 @@ private static File getWellKnownCertificateConfigFile( | |
| } | ||
| return new File(cloudConfigPath, WELL_KNOWN_CERTIFICATE_CONFIG_FILE); | ||
| } | ||
|
|
||
| /** | ||
| * Centralized helper method to determine if mutual TLS (mTLS) can be enabled. | ||
| * | ||
| * @param envProvider the environment provider to use for resolving environment variables | ||
| * @param propProvider the property provider to use for resolving system properties | ||
| * @param certConfigPathOverride optional override path for the configuration file | ||
| * @return true if mTLS should be enabled, false otherwise | ||
| * @throws IOException if the configuration file is present but contains missing or malformed | ||
| * files | ||
| */ | ||
| public static boolean canMtlsBeEnabled( | ||
|
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. I’m not sure that cert being present == automatically use mTLS. They can be using different credentials / not using it at all. So then we’d be adding mTLS setup and calls for credentials that are not actually using it. I think the decision should be based on the credential type, and perhaps expose some state from the credential that we can use to check if mTLS should happen for these calls. |
||
| EnvironmentProvider envProvider, PropertyProvider propProvider, String certConfigPathOverride) | ||
| throws IOException { | ||
|
|
||
| // Check if client certificate usage is allowed | ||
| String useClientCertificate = envProvider.getEnv("GOOGLE_API_USE_CLIENT_CERTIFICATE"); | ||
| if ("false".equalsIgnoreCase(useClientCertificate)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (getMtlsEndpointUsagePolicy(envProvider) == MtlsEndpointUsagePolicy.NEVER) { | ||
| return false; | ||
| } | ||
|
|
||
| // Locate and process the certificate configuration file | ||
| String envPath = envProvider.getEnv(CERTIFICATE_CONFIGURATION_ENV_VARIABLE); | ||
| if (certConfigPathOverride != null || !Strings.isNullOrEmpty(envPath)) { | ||
|
Member
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. Is it possible for both |
||
| File certConfigFile = | ||
| new File(certConfigPathOverride != null ? certConfigPathOverride : envPath); | ||
| if (!certConfigFile.isFile()) { | ||
| throw new CertificateSourceUnavailableException( | ||
| "Certificate configuration file does not exist or is not a file: " | ||
| + certConfigFile.getAbsolutePath()); | ||
| } | ||
| } | ||
|
|
||
| WorkloadCertificateConfiguration workloadCertConfig = null; | ||
| try { | ||
| workloadCertConfig = | ||
| getWorkloadCertificateConfiguration(envProvider, propProvider, certConfigPathOverride); | ||
| } catch (CertificateSourceUnavailableException e) { | ||
| // Config file is simply not present. This is fine, fallback to SPIFFE. | ||
| } catch (IOException e) { | ||
| // Config file exists but is malformed or points to invalid paths -> throw hard error | ||
| throw e; | ||
|
Comment on lines
+194
to
+198
Member
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. For these cases, do we want to either log a message and/or make these errors actionable? How will customers know that we have fallen back to SPIFFE or if there config malformed will the exception be clear enough for them to know that? |
||
| } | ||
|
|
||
| if (workloadCertConfig != null) { | ||
| // Validate referenced files exist | ||
| File certFile = new File(workloadCertConfig.getCertPath()); | ||
| File keyFile = new File(workloadCertConfig.getPrivateKeyPath()); | ||
| if (!certFile.isFile() || !keyFile.isFile()) { | ||
| throw new IOException( | ||
| String.format( | ||
| "Certificate configuration exists but referenced files are missing: cert_path=%s, key_path=%s", | ||
| workloadCertConfig.getCertPath(), workloadCertConfig.getPrivateKeyPath())); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Fallback to SPIFFE discovery if the directory exists | ||
| File spiffeDir = new File(spiffeDirectory); | ||
| if (spiffeDir.isDirectory()) { | ||
| File credentialBundle = new File(spiffeDir, SPIFFE_CREDENTIAL_BUNDLE_FILE); | ||
| if (credentialBundle.isFile()) { | ||
| return true; | ||
| } | ||
| File certsFile = new File(spiffeDir, SPIFFE_CERTIFICATE_FILE); | ||
| File keyFile = new File(spiffeDir, SPIFFE_PRIVATE_KEY_FILE); | ||
| if (certsFile.isFile() && keyFile.isFile()) { | ||
| return true; | ||
| } | ||
| } | ||
|
Comment on lines
+215
to
+226
Member
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. Tracing through the Hmm, is it possible to move the logic L201 - L212 into Side note: I'm wondering if we can break this into something like:
|
||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current mutual TLS endpoint usage policy. | ||
| * | ||
| * @param envProvider the environment provider to use for resolving environment variables | ||
| * @return the MtlsEndpointUsagePolicy enum value | ||
| */ | ||
| public static MtlsEndpointUsagePolicy getMtlsEndpointUsagePolicy( | ||
| EnvironmentProvider envProvider) { | ||
| String mtlsEndpointUsagePolicy = envProvider.getEnv("GOOGLE_API_USE_MTLS_ENDPOINT"); | ||
| if ("never".equals(mtlsEndpointUsagePolicy)) { | ||
| return MtlsEndpointUsagePolicy.NEVER; | ||
| } else if ("always".equals(mtlsEndpointUsagePolicy)) { | ||
| return MtlsEndpointUsagePolicy.ALWAYS; | ||
| } | ||
| return MtlsEndpointUsagePolicy.AUTO; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,10 @@ static RegionalAccessBoundary refresh( | |
| throw new IllegalArgumentException("The provided access token is expired."); | ||
| } | ||
|
|
||
| if (transportFactory instanceof com.google.auth.mtls.MtlsHttpTransportFactory) { | ||
|
Member
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. I believe the transportFactory is a setter for a bunch of credentials. Is it possible to make this check a bit more robust and not based on the |
||
| url = url.replace("iamcredentials.googleapis.com", "iamcredentials.mtls.googleapis.com"); | ||
| } | ||
|
vverman marked this conversation as resolved.
|
||
|
|
||
| HttpRequestFactory requestFactory = transportFactory.create().createRequestFactory(); | ||
| HttpRequest request = requestFactory.buildGetRequest(new GenericUrl(url)); | ||
| // Disable automatic logging by google-http-java-client to prevent leakage of sensitive tokens. | ||
|
|
||
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.
qq, is this change necessary for the PR? I know this is marked with @internalapi and this isn't exactly customers are expected to interact with directly.
There are some small source and binary compatibility changes (I think very small chance) but I would prefer to keep it as-is unless we absolutely need to