Skip to content

Feat: port mtls_utils and send Agent Registry requests over mTLS - #608

Open
AmaadMartin wants to merge 6 commits into
mainfrom
feat/mtls-utils-agent-registry
Open

Feat: port mtls_utils and send Agent Registry requests over mTLS#608
AmaadMartin wants to merge 6 commits into
mainfrom
feat/mtls-utils-agent-registry

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: adk-python has a shared mTLS utility (src/google/adk/utils/_mtls_utils.py) that every Google Cloud REST caller consults before talking to a *.googleapis.com endpoint: it decides whether to present a client certificate, attaches it to the HTTP session, and rewrites the endpoint to *.mtls.googleapis.com so certificate-bound access tokens are honored. adk-js has no equivalent — the one Google Cloud REST caller, core/src/integrations/agent_registry/agent_registry.ts, always hits https://agentregistry.googleapis.com/v1alpha with a bare fetch. Users in organizations that enforce certificate-based access cannot use the adk-js Agent Registry client at all, and users who do have a device certificate silently get non-certificate-bound tokens.

Solution: port the module to core/src/utils/mtls_utils.ts and adopt it in AgentRegistry.

New module core/src/utils/mtls_utils.ts (Node-only, deliberately not added to index.ts/common.ts, so the browser bundle is untouched):

Export Mirrors in _mtls_utils.py Purpose
MtlsEndpointSetting (auto/always/never) MtlsEndpoint parsed GOOGLE_API_USE_MTLS_ENDPOINT
effectiveGoogleapisEndpoint(url, hasClientCert) effective_googleapis_endpoint() + is_non_mtls_googleapis_endpoint() + _should_use_mtls_endpoint() the endpoint to actually call: rewrite the host when the environment and certificate state call for it, otherwise return url unchanged
createMtlsDispatcher() configure_session_for_mtls() build the certificate-presenting dispatcher, or undefined

Python splits the endpoint decision across three functions because it has three separate callers for the pieces; adk-js has one, so the whole policy -- the never opt-out, always, and auto + certificate -- is applied in a single function that reads the setting once. use_client_cert_effective() is not a separate export either: it was a bare wrapper over this repo's existing getBooleanEnvVar, so it is inlined at its one call site.

AgentRegistry.makeRequest() now resolves the certificate once per instance, attaches a certificate-presenting dispatcher when one exists, and targets agentregistry.mtls.googleapis.com when the resolved endpoint says so.

With no environment variables set, nothing changes: same URL, same fetch init (the dispatcher key is added conditionally, so it is absent entirely), and no filesystem access at all. All 52 pre-existing assertions in core/test/integrations/agent_registry_test.ts pass unmodified.

Collision check. Before starting I listed all 506 open PRs on this fork (gh pr list --limit 1000) and diffed every plausibly adjacent one. No open PR creates core/src/utils/mtls_utils.ts or modifies core/src/integrations/agent_registry/agent_registry.ts. The 7 PRs whose body mentions mTLS (#529, #578, #194, #462, #535, #537, #464) all touch other clients only. #334 touches core/test/integrations/agent_registry_test.ts, which is why the new registry tests live in a new file rather than being appended to that one — no conflict either way.

New dependency: undici (the one genuinely reviewable item)

Node's global fetch cannot present a client certificate on its own and does not accept a node:https.Agent. The options were (a) an undici Agent passed as the non-standard dispatcher init property, (b) rewriting callers onto node:https, or (c) another HTTP client. This PR takes (a); (b) would fork the request path in two and leave every other fetch-based caller uncovered.

Verified empirically on this branch, Node v22.22.2 with the resolved undici 7.29.0, against a local node:https server started with requestCert: true:

  • fetch(url, {dispatcher: new Agent({connect: {ca}})}) → server sees no peer certificate;
  • fetch(url, {dispatcher: new Agent({connect: {ca, cert, key}})}) → server sees subject.CN === 'probe-client'.

That scenario is now a committed test (tests/integration/mtls_dispatcher_test.ts), not just a one-off probe.

Disclosures about the dependency, each checked against a source of truth rather than quoted from memory:

  • node_modules/undici/package.json declares "engines": {"node": ">=20.18.1"} and "license": "MIT". This repo declares no engines field in either package.json, and .github/workflows/validation.yaml uses actions/setup-node@v6 with no pinned node-version, so there is no existing floor for this to contradict — but it is stated here rather than smuggled in.
  • undici is imported lazily (await import('undici') inside createMtlsDispatcher), so importing @google/adk does not load it and the module stays importable below undici's engine floor. This is the one deliberate inline import in the change.
  • scripts/check_license.sh only checks source-file headers, so no allowlist change is needed.
  • package-lock.json is touched only by this one real dependency addition (9 added lines, resolved pointing at registry.npmjs.org). No version-bump churn, no CHANGELOG.md.

Deliberate scope decisions (nothing silently dropped)

Not ported from _mtls_utils.py:

  • get_api_endpoint(location, default_template, mtls_template) — its Python callers are the Secret Manager and Parameter Manager regional clients, which do not exist in adk-js. Porting it would ship a parameter with no reader.
  • MtlsClientCerts — extracts the certificate to a temp directory for consumers that need on-disk paths (gRPC-style channels). Nothing in adk-js needs on-disk paths; the dispatcher takes the bytes directly.

Adoption sites deliberately left for follow-ups, to keep this reviewable:

  • the OAuth2 token exchange (core/src/auth/oauth2/oauth2_utils.ts), which mirrors oauth2_credential_util.py and interacts with the existing SSRF guard;
  • rewriting the MCP/A2A connection URIs returned by getConnectionUri(). Rewriting a URL to an mTLS host only helps if the transport that dials it can present the certificate, and neither the MCP StreamableHTTPConnectionParams transport nor the A2A client is wired for a custom dispatcher today. Shipping the rewrite alone would move traffic to an mTLS host with no certificate on it, so getConnectionUri() is untouched.

Cross-language parity notes (where JS and Python conventions conflict)

  • Parity wins on the wire. Enum string values (auto/always/never), the .mtls.googleapis.com host form, the snake_case certificate_config.json keys, and the precedence rules (always > cert presence; never always wins) all match Python exactly.
  • Local convention wins in-process. Module decomposition follows the JS side (one policy function instead of Python's three). The client-certificate check reuses this repo's getBooleanEnvVar(), which accepts 'true' or '1' (case-insensitive), whereas Python accepts only 'true'. This is a deliberate superset: reusing the repo helper is the right call for process-internal parsing, and it is a widening, not a behaviour change for anyone.
  • Certificate discovery differs by necessity. Python delegates to google.auth.transport.mtls. The Node google-auth-library v10 exports no mTLS helper, so this mirrors the resolution order used by its certificatesubjecttokensupplier: GOOGLE_API_CERTIFICATE_CONFIG, else CLOUDSDK_CONFIG, else %APPDATA%\gcloud on Windows / $HOME/.config/gcloud elsewhere, then certificate_config.jsoncert_configs.workload.{cert_path,key_path}.
  • Fail open, like Python. configure_session_for_mtls() returns False on any certificate problem; createMtlsDispatcher() returns undefined and logs one logger.warn. It never throws. Certificate and key bytes never reach a log, an error message, or disk (there is a test asserting exactly that).

Notes for the reviewer

  • No type-checker or linter suppressions were addedgit diff fork/main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|: any' returns nothing. Worth noting because the obvious way to type the dispatcher init hits a real obstacle: dispatcher is not on the DOM RequestInit, and eslint's no-undef fires on the bare type name RequestInit (the existing workaround at dev/src/server/adk_api_client.ts:264 is an // eslint-disable-next-line no-undef). Instead of copying that, FetchInitWithDispatcher extends NonNullable<Parameters<typeof fetch>[1]> — derived from the global fetch itself, so there is no free type identifier for no-undef to trip on and no suppression. undici's own RequestInit was tried first and is not assignable to the global fetch (body differs: AsyncIterable<Uint8Array> is not a DOM BodyInit).
  • Single PR, not a stack. 1022 added lines, but only 273 are source (mtls_utils.ts 215, agent_registry.ts +58); the rest is tests. It is also one logical checkpoint — splitting it would make part 1 a module with no reader, and a stacked base would stop CI from running on the later parts.
  • No console.log, no narration comments, no internal references; the diff and all four commit messages were grepped for both.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Commands run on the exact pushed commit:

npx vitest run --project unit:core core/test/utils/mtls_utils_test.ts \
  core/test/integrations/agent_registry_mtls_test.ts \
  core/test/integrations/agent_registry_test.ts     # 97 passed (3 files)
npx vitest run --project integration tests/integration/mtls_dispatcher_test.ts \
  tests/integration/agent_registry/agent_registry_test.ts   # 3 passed (2 files)
npm run build        # ok
npm run lint         # ok
npm run format:check # ok
npm run docs:check   # ok (warnings-as-errors)
npx secretlint <the four new files>   # clean
npx tsc --noEmit     # 281 errors, identical to the count on fork/main with this
                     # branch stashed -- this change adds none

Coverage of the new code (vitest --coverage.include, v8 provider):

File           | % Stmts | % Branch | % Funcs | % Lines
mtls_utils.ts  |     100 |      100 |     100 |     100

agent_registry.ts reports 98.88% lines / 94.16% branches; every uncovered line is pre-existing (getAuthHeaders's client.credentials.access_token fallback, lines 143-147), untouched by this change. All branches added here are covered.

Proof that each test can fail. Every new test was run against mutated source; the mutations and their exact failure messages:

# Mutation Test that failed Message
a Delete the NEVER early-return in effectiveGoogleapisEndpoint leaves the url unchanged when the setting is "never" expected 'https://oauth2.mtls.googleapis.com/to…' to be 'https://oauth2.googleapis.com/token'
b Replace the hostname-suffix check with url.includes('googleapis.com') classifies https://evil-googleapis.com.attacker.test/x as false and classifies https://googleapis.com/token as false expected true to be false
c Drop memoization — call resolveMtlsTransport() per request loads the certificate once across sequential requests and …across concurrent first requests expected "spy" to be called 1 times, but got 2 times
d Attach the dispatcher unconditionally (dispatcher, instead of ...(dispatcher ? {dispatcher} : {})) 3 tests, incl. uses the plain host with no dispatcher when no certificate is available expected { method: 'GET', …(2) } to not have property "dispatcher"
e Memoize the resolved value instead of the in-flight promise only loads the certificate once across concurrent first requests expected "spy" to be called 1 times, but got 2 times

Mutation (g) exists because inlining useClientCertEffective moved its assertions onto createMtlsDispatcher; it confirms the true/1 parsing is still pinned there. Mutation (e) is the interesting one: it is invisible to the sequential test and caught only by the concurrent one, which is why both exist. Mutation (f) covers the cross-language contract: those three strings are read from a shared environment variable and must not drift from Python.

Error paths are exercised, not just the happy path. mtls_utils_test.ts covers: config file missing, config not valid JSON, cert_configs.workload absent, cert_path missing, key_path missing, a PEM file that fails to read, a rejection that is not an Error, and a dispatcher constructor that throws — each asserting undefined plus exactly one logger.warn, and one asserting the warning contains neither the certificate nor the key bytes. It also asserts that with the feature disabled the filesystem is never touched. agent_registry_mtls_test.ts covers createMtlsDispatcher() rejecting (with an Error and with a non-Error) and proves the request still succeeds against the plain host.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

tests/integration/mtls_dispatcher_test.ts is the automated version of the manual check: it generates a throwaway CA plus server and client certificates with openssl into an fs.mkdtemp directory (deleted in afterAll; no PEM fixture is committed, secretlint scans **/*), starts a node:https server with requestCert: true, and asserts the server sees CN=adk-test-client for a fetch made through the dispatcher, and no peer certificate when the feature is off. It skipIfs cleanly when openssl is not on PATH, so a runner without it does not fail -- though in practice CI's windows-latest runner does have it and the test genuinely ran and passed there (✓ integration tests/integration/mtls_dispatcher_test.ts (2 tests) 990ms), alongside ubuntu-latest and macos-latest. Because the throwaway CA cannot be trusted at runtime (NODE_EXTRA_CA_CERTS is only read at process start — verified) and createMtlsDispatcher deliberately exposes no ca option, that file sets NODE_TLS_REJECT_UNAUTHORIZED=0 in beforeAll and restores it in afterAll; the assertions are about the client certificate, and vitest isolates the file in its own worker.

On a workstation with a real provisioned device certificate:

export GOOGLE_API_USE_CLIENT_CERTIFICATE=true
# optional: export GOOGLE_API_CERTIFICATE_CONFIG=/path/to/certificate_config.json
node -e "import('@google/adk').then(async ({AgentRegistry}) => {
  const r = new AgentRegistry({projectId: process.env.PROJECT, location: 'us-central1'});
  console.log(await r.listAgents());
})"

Confirm from the process's network activity that the request goes to agentregistry.mtls.googleapis.com; re-run with GOOGLE_API_USE_MTLS_ENDPOINT=never and confirm it returns to agentregistry.googleapis.com; then unset both variables and confirm the plain-host behaviour is untouched.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 6 commits August 3, 2026 19:39
Ports adk-python's src/google/adk/utils/_mtls_utils.py to TypeScript so that
JS callers of Google Cloud REST APIs can honour certificate-based access
policies. The module resolves the GOOGLE_API_USE_CLIENT_CERTIFICATE and
GOOGLE_API_USE_MTLS_ENDPOINT settings, rewrites *.googleapis.com hosts to
their .mtls.googleapis.com variants, and builds an undici dispatcher that
presents the application-default client certificate to the global fetch.

Certificate loading fails open: any missing or malformed configuration logs a
warning and degrades to a plain non-mTLS request.
…e is configured

AgentRegistry.makeRequest() now resolves the client certificate once per
instance, attaches a certificate-presenting dispatcher to its fetch calls, and
targets agentregistry.mtls.googleapis.com when GOOGLE_API_USE_MTLS_ENDPOINT
says so. The in-flight promise is memoized rather than its result, so
concurrent first calls share a single certificate load.

With no environment configuration the request is byte-for-byte what it was:
the plain host and a fetch init with no dispatcher property.
Adds an integration test that generates a throwaway CA plus server and client
certificates with openssl, starts a node:https server with requestCert, and
asserts that a fetch made through the dispatcher built by
createMtlsDispatcher() is seen by the server with the expected client common
name -- and that no peer certificate arrives when the feature is disabled.

The suite skips cleanly when openssl is not on PATH.
With GOOGLE_API_USE_MTLS_ENDPOINT=always a failed certificate load still
targets the mTLS host (parity with adk-python), so the fallback is a request
without a client certificate rather than a request to the plain host.
Adds an assertion that MtlsEndpointSetting still serialises to auto/always/
never, since those strings are read from a shared environment variable and
must not drift from the Python implementation. Also normalises the module doc
comment to plain ASCII punctuation.
Review feedback: three of the module's exports had exactly one caller each and
existed only so a unit test could reach them, and the NEVER opt-out was
encoded twice -- once in shouldUseMtlsEndpoint and again, unreachably, inside
effectiveGoogleapisEndpoint, which was only ever called behind that gate.

- Merge shouldUseMtlsEndpoint, isNonMtlsGoogleapisEndpoint and
  effectiveGoogleapisEndpoint into effectiveGoogleapisEndpoint(url,
  hasClientCert), which reads the setting once. The caller in AgentRegistry is
  now a single expression, and the single-use hostnameOf helper is gone.
- Inline useClientCertEffective, a bare wrapper over getBooleanEnvVar, into
  createMtlsDispatcher.
- Inline the single-producer, single-consumer ClientCertificate interface.
- Drop the outer .catch() in resolveMtlsTransport: createMtlsDispatcher
  catches everything after its env check and cannot reject, so the second
  fail-open handler was dead code with a second warning string for the same
  event. The two tests pinning it are removed with it.
- Correct the FetchInit comment. The alias is not about importability -- it
  exists because eslint's no-undef rejects the bare RequestInit global, which
  npm run lint still confirms.

No behaviour change: same precedence rules, same rewrite, same wire strings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant