feat(auth) Regional Access Boundaries mTLS endpoint#8450
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors mTLS configuration and path resolution logic by extracting shared helpers into a new mtlsutils.ts module, and updates RegionalAccessBoundaryManager to support mTLS. The review feedback suggests caching the https.Agent using a WeakMap to avoid redundant disk I/O and enable TCP connection reuse across requests. Additionally, it recommends throwing CertificateSourceUnavailableError instead of a generic Error when referenced certificate or key files are missing to maintain consistency with the rest of the library.
| import * as https from 'https'; | ||
| import {canMtlsBeEnabled, getClientCertAndKey} from './mtlsutils'; | ||
|
|
||
| const log = makeLog('auth'); |
There was a problem hiding this comment.
To support efficient connection pooling and avoid test isolation issues, we should cache the https.Agent promise. Using a WeakMap keyed by the RegionalAccessBoundaryManager instance ensures that each manager instance reuses its agent across requests, while keeping tests isolated.
| import * as https from 'https'; | |
| import {canMtlsBeEnabled, getClientCertAndKey} from './mtlsutils'; | |
| const log = makeLog('auth'); | |
| import * as https from 'https'; | |
| import {canMtlsBeEnabled, getClientCertAndKey} from './mtlsutils'; | |
| const log = makeLog('auth'); | |
| const mtlsAgentCache = new WeakMap< | |
| RegionalAccessBoundaryManager, | |
| Promise<https.Agent | null> | |
| >(); |
| // If mTLS can be enabled, use mTLS agent and switch to the mTLS endpoint | ||
| try { | ||
| if (await canMtlsBeEnabled()) { | ||
| const {cert, key} = await getClientCertAndKey(); | ||
| opts.agent = new https.Agent({cert, key}); | ||
| regionalAccessBoundaryUrl = regionalAccessBoundaryUrl.replace( | ||
| 'iamcredentials.googleapis.com', | ||
| 'iamcredentials.mtls.googleapis.com', | ||
| ); | ||
| opts.url = regionalAccessBoundaryUrl; | ||
| } | ||
| } catch (e) { | ||
| log.error('RegionalAccessBoundary: Failed to initialize mTLS: ', e); | ||
| } |
There was a problem hiding this comment.
Creating a new https.Agent and reading the certificate/key files from disk on every single request is highly inefficient and prevents TCP connection reuse (keep-alive). We should reuse the cached agent promise from the WeakMap to enable connection pooling and avoid redundant I/O.
// If mTLS can be enabled, use mTLS agent and switch to the mTLS endpoint
let agentPromise = mtlsAgentCache.get(this);
if (!agentPromise) {
agentPromise = (async () => {
try {
if (await canMtlsBeEnabled()) {
const {cert, key} = await getClientCertAndKey();
return new https.Agent({cert, key});
}
} catch (e) {
log.error('RegionalAccessBoundary: Failed to initialize mTLS: ', e);
}
return null;
})();
mtlsAgentCache.set(this, agentPromise);
}
try {
const agent = await agentPromise;
if (agent) {
opts.agent = agent;
regionalAccessBoundaryUrl = regionalAccessBoundaryUrl.replace(
'iamcredentials.googleapis.com',
'iamcredentials.mtls.googleapis.com',
);
opts.url = regionalAccessBoundaryUrl;
}
} catch (e) {
log.error('RegionalAccessBoundary: Failed to initialize mTLS: ', e);
}| throw new Error( | ||
| `Certificate configuration exists but referenced files are missing: cert_path=${certPath}, key_path=${keyPath}`, | ||
| ); |
There was a problem hiding this comment.
For consistency with other certificate resolution errors in this library, we should throw CertificateSourceUnavailableError instead of a generic Error when the referenced certificate or key files are missing.
| throw new Error( | |
| `Certificate configuration exists but referenced files are missing: cert_path=${certPath}, key_path=${keyPath}`, | |
| ); | |
| throw new CertificateSourceUnavailableError( | |
| 'Certificate configuration exists but referenced files are missing: cert_path=' + certPath + ', key_path=' + keyPath, | |
| ); |
Added logic to:
Centralize mTLS enablement logic within the auth library.
Based on 1. determine whether mtls or regular RAB lookup endpoint should be called.
Added tests for the same.