Regional access boundaries#13318
Conversation
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.
| public KeyStore getKeyStore() throws CertificateSourceUnavailableException, IOException { | ||
| if (!MtlsUtils.canMtlsBeEnabled(envProvider, propProvider, certConfigPathOverride)) { | ||
| throw new CertificateSourceUnavailableException("mTLS is not enabled or cannot be established."); | ||
| } | ||
|
|
||
| // 1. Attempt to load from resolved Config File | ||
| WorkloadCertificateConfiguration workloadCertConfig = null; | ||
| try { | ||
| this.getKeyStore(); | ||
| } catch (CertificateSourceUnavailableException e) { | ||
| return false; | ||
| workloadCertConfig = MtlsUtils.getWorkloadCertificateConfiguration(envProvider, propProvider, certConfigPathOverride); | ||
| } catch (IOException e) { | ||
| // Ignore configuration file errors here to fall back to SPIFFE discovery | ||
| } | ||
|
|
||
| if (workloadCertConfig != null) { | ||
| try (InputStream certStream = new FileInputStream(new File(workloadCertConfig.getCertPath())); | ||
| InputStream privateKeyStream = new FileInputStream(new File(workloadCertConfig.getPrivateKeyPath())); | ||
| SequenceInputStream certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream)) { | ||
| return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream); | ||
| } catch (Exception e) { | ||
| throw new IOException("X509Provider: Unexpected error loading from config file:", e); | ||
| } | ||
| } | ||
|
|
||
| // 2. Fallback: Load from SPIFFE Credentials | ||
| File spiffeDir = new File(MtlsUtils.spiffeDirectory); | ||
| if (spiffeDir.isDirectory()) { | ||
| File credentialBundle = new File(spiffeDir, MtlsUtils.SPIFFE_CREDENTIAL_BUNDLE_FILE); | ||
| if (credentialBundle.isFile()) { | ||
| try (InputStream bundleStream = new FileInputStream(credentialBundle)) { | ||
| return SecurityUtils.createMtlsKeyStore(bundleStream); | ||
| } catch (Exception e) { | ||
| throw new IOException("X509Provider: Unexpected error loading from SPIFFE bundle:", e); | ||
| } | ||
| } | ||
|
|
||
| File certsFile = new File(spiffeDir, MtlsUtils.SPIFFE_CERTIFICATE_FILE); | ||
| File keyFile = new File(spiffeDir, MtlsUtils.SPIFFE_PRIVATE_KEY_FILE); | ||
| if (certsFile.isFile() && keyFile.isFile()) { | ||
| try (InputStream certStream = new FileInputStream(certsFile); | ||
| InputStream privateKeyStream = new FileInputStream(keyFile); | ||
| SequenceInputStream certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream)) { | ||
| return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream); | ||
| } catch (Exception e) { | ||
| throw new IOException("X509Provider: Unexpected error loading from separate SPIFFE files:", e); | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
|
|
||
| throw new CertificateSourceUnavailableException("mTLS is enabled, but no certificate source was resolved."); | ||
| } |
There was a problem hiding this comment.
The getKeyStore() method currently calls MtlsUtils.canMtlsBeEnabled(...) at the beginning, which performs redundant file existence checks and parses the JSON configuration file. Immediately after, it calls MtlsUtils.getWorkloadCertificateConfiguration(...) which parses the same JSON configuration file again. This results in unnecessary file I/O and CPU overhead.
We can optimize this by directly attempting to load the configuration and credentials, and throwing CertificateSourceUnavailableException if mTLS is not enabled or cannot be established. This avoids duplicate file checks and JSON parsing.
public KeyStore getKeyStore() throws CertificateSourceUnavailableException, IOException {
String useClientCertificate = envProvider.getEnv("GOOGLE_API_USE_CLIENT_CERTIFICATE");
if ("false".equalsIgnoreCase(useClientCertificate)) {
throw new CertificateSourceUnavailableException("mTLS is explicitly disabled via GOOGLE_API_USE_CLIENT_CERTIFICATE.");
}
// 1. Attempt to load from resolved Config File
WorkloadCertificateConfiguration workloadCertConfig = null;
try {
workloadCertConfig = MtlsUtils.getWorkloadCertificateConfiguration(envProvider, propProvider, certConfigPathOverride);
} catch (CertificateSourceUnavailableException e) {
// Config file is simply not present. This is fine, fallback to SPIFFE.
}
if (workloadCertConfig != null) {
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()));
}
try (InputStream certStream = new FileInputStream(certFile);
InputStream privateKeyStream = new FileInputStream(keyFile);
SequenceInputStream certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream)) {
return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream);
} catch (Exception e) {
throw new IOException("X509Provider: Unexpected error loading from config file:", e);
}
}
// 2. Fallback: Load from SPIFFE Credentials
File spiffeDir = new File(MtlsUtils.spiffeDirectory);
if (spiffeDir.isDirectory()) {
File credentialBundle = new File(spiffeDir, MtlsUtils.SPIFFE_CREDENTIAL_BUNDLE_FILE);
if (credentialBundle.isFile()) {
try (InputStream bundleStream = new FileInputStream(credentialBundle)) {
return SecurityUtils.createMtlsKeyStore(bundleStream);
} catch (Exception e) {
throw new IOException("X509Provider: Unexpected error loading from SPIFFE bundle:", e);
}
}
File certsFile = new File(spiffeDir, MtlsUtils.SPIFFE_CERTIFICATE_FILE);
File keyFile = new File(spiffeDir, MtlsUtils.SPIFFE_PRIVATE_KEY_FILE);
if (certsFile.isFile() && keyFile.isFile()) {
try (InputStream certStream = new FileInputStream(certsFile);
InputStream privateKeyStream = new FileInputStream(keyFile);
SequenceInputStream certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream)) {
return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream);
} catch (Exception e) {
throw new IOException("X509Provider: Unexpected error loading from separate SPIFFE files:", e);
}
}
}
throw new CertificateSourceUnavailableException("mTLS is enabled, but no certificate source was resolved.");
}References
- When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
| try { | ||
| if (MtlsUtils.canMtlsBeEnabled( | ||
| SystemEnvironmentProvider.getInstance(), | ||
| SystemPropertyProvider.getInstance(), | ||
| null)) { | ||
| X509Provider x509Provider = new X509Provider( | ||
| SystemEnvironmentProvider.getInstance(), | ||
| SystemPropertyProvider.getInstance(), | ||
| null); | ||
| KeyStore mtlsKeyStore = x509Provider.getKeyStore(); | ||
| if (mtlsKeyStore != null) { | ||
| transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| // Graceful fallback to standard transport if mTLS initialization fails | ||
| } |
There was a problem hiding this comment.
Calling MtlsUtils.canMtlsBeEnabled(...) here is redundant because x509Provider.getKeyStore() already internally checks if mTLS can be enabled and throws a CertificateSourceUnavailableException if it cannot. Removing this redundant check avoids duplicate file I/O and JSON parsing during credential initialization/refresh.
try {
X509Provider x509Provider = new X509Provider(
SystemEnvironmentProvider.getInstance(),
SystemPropertyProvider.getInstance(),
null);
KeyStore mtlsKeyStore = x509Provider.getKeyStore();
if (mtlsKeyStore != null) {
transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore);
}
} catch (Exception e) {
// Graceful fallback to standard transport if mTLS initialization fails
}References
- When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
| if (transportFactory instanceof com.google.auth.mtls.MtlsHttpTransportFactory) { | ||
| url = url.replace("https://iamcredentials.googleapis.com/", "https://iamcredentials.mtls.googleapis.com/"); | ||
| } |
There was a problem hiding this comment.
Replacing the entire prefix https://iamcredentials.googleapis.com/ is fragile because it depends on the exact scheme and trailing slash. It is more robust to replace only the host name iamcredentials.googleapis.com with iamcredentials.mtls.googleapis.com.
| if (transportFactory instanceof com.google.auth.mtls.MtlsHttpTransportFactory) { | |
| url = url.replace("https://iamcredentials.googleapis.com/", "https://iamcredentials.mtls.googleapis.com/"); | |
| } | |
| if (transportFactory instanceof com.google.auth.mtls.MtlsHttpTransportFactory) { | |
| url = url.replace("iamcredentials.googleapis.com", "iamcredentials.mtls.googleapis.com"); | |
| } |
Added logic to: