Skip to content

feat(googlechat): keyless ADC auth + send-once for the unified adapter - #1512

Open
sebastian-hsu wants to merge 1 commit into
openabdev:mainfrom
sebastian-hsu:feat/googlechat-adc-pr
Open

feat(googlechat): keyless ADC auth + send-once for the unified adapter#1512
sebastian-hsu wants to merge 1 commit into
openabdev:mainfrom
sebastian-hsu:feat/googlechat-adc-pr

Conversation

@sebastian-hsu

@sebastian-hsu sebastian-hsu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What problem does this solve?

The Google Chat gateway adapter can authenticate to the Chat API in only two ways today: a service-account JSON key mounted into the pod (saKeyJson), or a static pre-minted accessToken. Both mean handling a long-lived secret — the key file has to be created, mounted, rotated and protected; the static token expires and must be refreshed by hand.

For a bot that already runs on GCP as its own Chat-app service account (GCE / GKE Workload Identity), that key file is redundant attack surface: the workload can already prove it is the service account. This PR adds a third, keyless option — mint the chat.bot token from the pod's own GCP identity via the metadata server + IAM Credentials self-impersonation, with no key file to mount or leak.

It also corrects the Google Chat streaming declaration: the API has no usable per-token streaming, so the adapter now declares send-once instead of attempting a post-then-edit loop. The decisive reason is structural — the unified adapter's synthetic message id is not a valid resource name, so spaces.messages.patch rejects it with 400 INVALID_ARGUMENT before any edit applies; the documented 1 write/sec-per-space quota is a further constraint.

Closes #

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1542421126976643112

Review Contract

Goal

Let a Google Chat bot running on GCP authenticate to the Chat API without a service-account key file, by minting a chat.bot-scoped token from its own GCP identity (ADC → GCE metadata server → IAM Credentials generateAccessToken, the service account impersonating itself). Also declare Google Chat as send-once so core stops attempting cosmetic edits the API rejects.

Non-goals

  • Not changing or removing the existing saKeyJson and static-accessToken paths — they stay, and keep precedence over ADC.
  • Not adding keyless auth for any other platform/adapter in this PR.
  • Not implementing real streaming for Google Chat (the write rate limit makes per-token editing impractical).
  • Not managing GCP IAM setup — the operator grants roles/iam.serviceAccountTokenCreator and enables the IAM Credentials API.

Accepted Residual Risks

  • GCP-only: the ADC path needs a reachable GCE metadata server (GCE / GKE). Off-GCP it fails; the operator must fall back to saKeyJson / accessToken. Documented in docs/google-chat.md, and it is opt-in (use_adc = false by default).
  • Requires self-impersonation IAM: the SA needs serviceAccountTokenCreator over itself and the IAM Credentials API enabled. A misconfiguration surfaces as an explicit token-mint error on the first send, not a silent failure.
  • Token freshness: the minted token is cached with a 300 s refresh margin (3600 s lifetime); a metadata / IAM Credentials outage blocks outbound until it recovers — the same failure class as the existing key/static-token paths.

Acceptance Criteria

  • With GOOGLE_CHAT_USE_ADC=true on a GCP workload running as the Chat-app SA (no key file mounted), the adapter mints a chat.bot token and posts a message successfully.
  • Auth precedence holds in get_token: saKeyJson (if set) > ADC (metadata) > static accessToken.
  • Google Chat is in NON_STREAMING_PLATFORMS; resolve_streaming forces send-once on both the embedded-dispatch and the WebSocket gateway paths (no post-then-edit).
  • platform-schema conformance passes: every source ref in docs/platforms/schema/googlechat.toml resolves to a real symbol.
  • cargo clippy --workspace and --features unified are clean; cargo test passes.

Follow-ups

  • MetadataTokenSource is general enough that keyless / workload-identity auth for other Google-API-backed adapters could reuse it — deferred, non-blocking.
  • The refresh margin (300 s) and token lifetime (3600 s) are currently fixed; making them configurable is a possible later hardening if a deployment needs it.

At a Glance

Outbound to Google Chat — token source (get_token precedence)

  saKeyJson set? ──yes──▶ SA-key JWT-bearer exchange ──▶ token_cache
      │no
      ▼
  use_adc = true? ──yes──▶ MetadataTokenSource
      │no                     │
      ▼                       │ 1. GET metadata server: default SA email + base token
  static accessToken          │ 2. IAM Credentials generateAccessToken
                              │    (SA impersonates ITSELF, scope = chat.bot)
                              │ 3. cache token (3600 s, 300 s refresh margin)
                              ▼
                         chat.bot access token ──▶ Google Chat REST API

Streaming: googlechat ∈ NON_STREAMING_PLATFORMS
  resolve_streaming(platform, adapter_prefers) ──▶ send-once
    · embedded dispatch path  ──▶ compute full reply, POST once
    · WebSocket gateway path  ──▶ same
  (no post-then-edit: synthetic unified_<hex> id → patch 400 INVALID_ARGUMENT;
   1/sec-per-space quota is a further documented constraint)

Prior Art & Industry Research

Scope: how comparable open-source agent gateways authenticate their Google Chat bot identity — downloaded service-account (SA) JSON key file vs. keyless (ADC / workload identity / self-impersonation) — and how they obtain/refresh the chat.bot access token. Both reference projects were read at main as of 2026-08-27; links point at file paths (line numbers drift).

OpenClaw (openclaw/openclaw) — largest open-source AI agent gateway

Google Chat support: yes, first-party plugin @openclaw/googlechat (HTTP webhook inbound, no Pub/Sub).

How they authenticate: downloaded SA JSON key file only — no keyless/ADC path. The channel doc's setup is literally "Create a Service Account … Keys → Add Key → JSON", and the plugin "authenticates exclusively as a service account with the chat.bot scope." Inbound verifies the request's Authorization: Bearer ID token against a configured audience.

  • Evidence: docs/channels/googlechat.md. Grepping that surface for adc|application default|workload|metadata|impersonat|keyless returns zero hits — keyless is genuinely absent, not just undocumented.

What we learn: even the largest gateway ships the long-lived-key pattern openab is moving away from → keyless is a real differentiator. Their per-space serialized outbound queue + write quotas corroborate our send-once decision.

Hermes Agent (NousResearch/hermes-agent) — self-hosted agent, 27+ platforms

Google Chat support: yes, plugins/platforms/google_chat/ (Pub/Sub-pull inbound + REST outbound).

How they authenticate: documented path is a downloaded SA JSON key (GOOGLE_CHAT_SERVICE_ACCOUNT_JSON), but the adapter code also supports keyless ADC as a fallback. _load_sa_credentials() priority: (1) explicit service_account_json, (2) GOOGLE_APPLICATION_CREDENTIALS, (3) google.auth.default() — ADC on Cloud Run / GCE / GKE with an attached workload identity, scopes chat.bot + pubsub.

  • Evidence: plugins/platforms/google_chat/adapter.py (_load_sa_credentials), doc google_chat.md.
  • Nuance: Hermes' keyless mode is plain google.auth.default() — it uses the attached SA's own metadata token directly. It does not call IAM Credentials generateAccessToken to self-impersonate, so the attached SA must itself carry chat.bot.

What we learn: closest prior art; confirms google.auth.default() on GCE/GKE as the sanctioned keyless entry point. openab's design is a stricter, more explicit superset — see the comparison and §Why.

Other industry practice

Comparison

Project Google Chat Auth model How the bot token is obtained
OpenClaw Yes (HTTP webhook) Key file only — SA JSON, chat.bot; no keyless path Google client libs mint/refresh from the SA private key
Hermes Agent Yes (Pub/Sub + REST) Key file (docs) + keyless ADC fallback (code) explicit JSON → GOOGLE_APPLICATION_CREDENTIALSplain google.auth.default() (attached SA's own token); no self-impersonation
Google (official) recommends keyless / short-lived; SA key = last resort ADC + workload identity; short-lived tokens via generateAccessToken
openab (this PR) Yes Keyless — ADC + metadata + IAM generateAccessToken self-impersonation ADC base identity from metadata → self-impersonate the Chat-bot SA → short-lived chat.bot token; auto-refresh, no key on disk

Proposed Solution

Keyless ADC outbound token (MetadataTokenSource, crates/openab-gateway/src/adapters/googlechat.rs). A new token source that, when use_adc is set:

  1. reads the default service-account email and a base access token from the GCE metadata server;
  2. calls IAM Credentials generateAccessToken so the service account impersonates itself, scoped to https://www.googleapis.com/auth/chat.bot;
  3. caches the result behind an RwLock with a double-checked refresh, using the same 300 s margin as the existing paths (ttl_from_expire_time clamps the lifetime to [0, 3600]).

get_token gains a strict precedence: token_cache (SA-key exchange) → metadata_source (ADC) → static access_token. Wiring: GoogleChatConfig.use_adc (config.rs), plumbed through GatewayGoogleChatConfig and the three from_parts call sites (lib.rs, src/main.rs), surfaced as GOOGLE_CHAT_USE_ADC and the chart's gateway.googleChat.useAdc.

Send-once streaming. NON_STREAMING_PLATFORMS (renamed from NON_EDITABLE_PLATFORMS, gateway.rs) now gates a new resolve_streaming(platform, adapter_prefers_streaming) (adapter.rs) used by the embedded stream_prompt_blocks path, mirroring what platform_supports_streaming already did for the WebSocket path. Google Chat therefore never attempts a post-then-edit loop. The schema (googlechat.toml) is updated from partial to not_implemented with the rationale, and its source refs point at the real symbols.

Why this approach?

Weighed against the alternatives from the research:

  • vs. OpenClaw / Hermes' documented key-file path. A downloaded SA JSON is a long-lived secret (Google notes such keys can be valid for years) that must be stored, mounted, rotated, and kept out of logs / backups / images — a standing exfiltration target. Removing it shrinks the on-disk credential surface to zero and eliminates key rotation, matching Google's explicit "avoid SA keys" guidance. This is the primary driver.
  • vs. Hermes' plain-ADC (google.auth.default()) keyless fallback. Plain ADC forces the node / workload identity to be the Chat bot — it must carry chat.bot directly. Self-impersonation decouples the runtime identity from the bot identity: the node keeps a minimal identity and is granted roles/iam.serviceAccountTokenCreator on a dedicated Chat-bot SA, and we request precisely the chat.bot scope via generateAccessToken regardless of the node SA's default scopes. Cleaner for least-privilege and multi-tenant separation; tokens are short-lived and auto-refresh with nothing persisted. Crucially, chat.bot is a Workspace scope and is not a subset of cloud-platform, so no ?scopes= parameter on the metadata token can produce it — generateAccessToken self-impersonation is therefore not merely cleaner here, it is the only viable keyless way to obtain a chat.bot token.

Accepted trade-offs (honest costs):

  1. Environment coupling — works only where a metadata server + ADC exist (GCE / GKE / Cloud Run). Off-GCP there is no metadata identity, so the SA-key path stays as an explicit fallback (not a default), mirroring Hermes' priority chain.
  2. IAM prerequisite — the runtime SA must hold roles/iam.serviceAccountTokenCreator on the target/self SA, plus one extra hop to the IAM Credentials API per token mint. This is a deliberate, auditable IAM binding replacing an unauditable file on disk.

The feature is opt-in (use_adc = false by default); the key-file and static-token paths are untouched for everyone else.

Alternatives Considered

  • Plain ADC (google.auth.default()), like Hermes Agent's keyless fallback. Simpler, but it makes the node/workload identity be the bot (the attached SA must carry chat.bot). We chose generateAccessToken self-impersonation instead to decouple the runtime identity from the bot identity and request exactly the chat.bot scope — see §Why.
  • Keep only the SA-key JSON file. Rejected as the default for GCP-hosted bots: a long-lived secret that must be mounted, protected and rotated, when the workload can already prove its identity. Retained as a fallback.
  • Static pre-minted accessToken. Kept as a fallback, but not a fix: it expires and needs out-of-band refresh.
  • Full Workload Identity Federation (off-GCP → GCP). Out of scope — this feature targets workloads already running on GCP as the Chat-app SA; WIF for external identities is a larger, separate design.
  • Attempt cosmetic streaming with rate-limit backoff. Rejected: the unified adapter's synthetic unified_<hex> id is not a valid resource name, so patch rejects it with 400 INVALID_ARGUMENT before any edit — per-token editing cannot work at all; the documented 1 write/sec-per-space quota is a further constraint. Send-once is what the API actually supports.

Validation

Run locally on stable pinned to the CI toolchain (2026-07-13 ≈ 1.97.0):

Rust:

  • cargo clippy --workspace -- -D warnings — clean
  • cargo clippy --workspace --features unified -- -D warnings — clean
  • cargo test -p openab-gateway passes, incl. config_first_conformance (every_platform_env_var_has_a_config_section_field)
  • platform-schema conformance — all four source refs in googlechat.toml verified to resolve (gateway.rs#NON_STREAMING_PLATFORMS, adapter.rs#resolve_streaming, adapter.rs#uses_native_streaming, googlechat.rs#MetadataTokenSource). The crate's own test wasn't run locally (my .claude/worktrees/ checkout nests under the repo, which confuses cargo's standalone-workspace resolution — a local artifact only); CI runs it on a flat checkout.

Helm (chart touched — gateway.yaml, values.yaml):

  • Change is one additive conditional (useAdcGOOGLE_CHAT_USE_ADC env, plus useAdc in the $hasGoogleChat guard). No existing charts/openab/tests/*_test.yaml asserts the gateway Google Chat env, so it does not disturb current unittests; helm unittest charts/openab runs on CI (helm not installed locally).

Manual:

  • On a GKE workload running as the Chat-app SA with GOOGLE_CHAT_USE_ADC=true and no key file: the bot mints a chat.bot token and replies in a space; confirm precedence by additionally setting saKeyJson and observing the key path win.

Note: my machine's default stable is 1.98, whose newer useless_format / doc_lazy_continuation lints flag pre-existing code (e.g. setup/wizard.rs) that the CI toolchain does not; the doc-comment lint on the new resolve_streaming doc was the one real hit and is fixed in this branch. The unrelated openab-core secrets::tests::resolve_exec_nonzero_exit failure is a pre-existing, environment-specific subprocess-error-string assertion (that file is not touched by this PR).

@openab-app openab-app Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Aug 27, 2026
@sebastian-hsu
sebastian-hsu force-pushed the feat/googlechat-adc-pr branch from 639b61f to f7baf96 Compare August 27, 2026 06:28
@openab-app openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Aug 27, 2026

@canyugs canyugs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 review — verified by live deployment, not just reading

I ran the auth flow against the real GCP and Google Chat APIs rather than only reading the diff, because the novel part of this PR is an interaction with two external services that unit tests can only assert against the author's own assumptions. All identifiers below are redacted; no token values are reproduced.

The keyless ADC design works and I recommend it lands. I posted a real message to a real space with a token minted entirely keylessly:

POST /v1/projects/-/serviceAccounts/<sa>:generateAccessToken
  → HTTP 200, expireTime 2026-08-27T15:34:25Z
tokeninfo on the minted token
  → scope: https://www.googleapis.com/auth/chat.bot   (and nothing else)
POST /v1/spaces/<space>/messages
  → HTTP 200, sender.type = BOT

That third line is the important one. The base token was cloud-platform-scoped and the minted token carries only chat.bot. This empirically validates the central argument in §Why for generateAccessToken self-impersonation over Hermes' plain google.auth.default() — the scope really is narrowed, and the runtime identity really is decoupled from the bot identity. Worth noting that the official docs make this argument even stronger than the PR does: chat.bot is a Workspace scope and is not a subset of cloud-platform, so no ?scopes= parameter on the metadata server can produce it. Self-impersonation is not merely cleaner here, it is the only viable keyless path. I'd put that in §Why.

I also confirmed the explicit IAM grant in the docs is genuinely required: roles/owner does not include iam.serviceAccounts.getAccessToken, so an owner still gets IAM_PERMISSION_DENIED without the serviceAccountTokenCreator binding.

Cross-checks against official docs that all hold: the projects/-/ wildcard is required as used; iam.serviceAccounts.getAccessToken is contained in roles/iam.serviceAccountTokenCreator; accessToken/expireTime are exactly the response fields; expireTime is RFC 3339 and may carry fractional digits or a non-Z offset, both of which parse_from_rfc3339 accepts; the metadata token endpoint returns {"access_token":…,"expires_in":…,"token_type":…}, matching the parsing and the wiremock fixture. The chat.bot scope requires no admin approval, so no domain-wide delegation is needed for an impersonated token.

I verified the OpenClaw half of §Prior Art directly against a local checkout: extensions/googlechat/ has zero genuine hits for adc|application.default|workload.identity|metadata.google|impersonat|generateAccessToken|keyless (the one apparent match is readCredentialsFile matching adc case-insensitively). The claim holds. I did not verify the Hermes Agent claims.


Recommended blockers

1. Stale base — check fails on a lint this PR did not introduce

cargo clippy --workspace -- -D warnings fails at crates/openab-core/src/setup/wizard.rs:163 (useless_format). That line was fixed on main in #1511; this branch's merge base predates it. Merging origin/main clears it — I verified locally on the merged tree:

cargo clippy --workspace -- -D warnings                     → clean
cargo clippy --workspace --features unified -- -D warnings   → clean
cargo test -p openab-gateway                                 → 315 passed, 0 failed
cargo test --manifest-path crates/platform-schema/Cargo.toml  → 17 passed

main touches 6 files, none overlapping this PR, so no conflicts. Incidentally the platform-schema conformance run that §Validation lists as unverified locally does pass — all four source refs resolve.

The one openab-core failure, secrets::tests::resolve_exec_nonzero_exit, also fails on unmodified origin/main on macOS, so §Validation's characterisation of it as pre-existing and environment-specific is accurate.

The smoke-test (Dockerfile.hermes) failure is an upstream install-script download returning curl exit 22; smoke-test-unified (hermes) is green. Unrelated.

2. docs/google-chat.md Option C will fail as written on GCE

Calling generateAccessToken requires the caller's token to carry https://www.googleapis.com/auth/iam or cloud-platform. The metadata server returns a token bounded by the instance's access scopes, and GCE's default scope set contains neither. Reproduced with a base token restricted to exactly the GCE defaults, same SA and same IAM binding that succeeded above:

base scope: devstorage.read_only logging.write monitoring.write
            service.management.readonly servicecontrol trace.append

POST …:generateAccessToken
  → HTTP 403 PERMISSION_DENIED
    "Request had insufficient authentication scopes."

So the IAM prerequisites can be entirely correct and this still fails. Option C lists GKE / GCE / Cloud Run and the prerequisites cover the IAM binding and API enablement, but not the access scope. GKE Workload Identity and Cloud Run are cloud-platform-scoped and unaffected; a default-scope GCE VM is not, and VM scopes are immutable after creation (gcloud compute instances set-scopes plus a stop/start). Please add the requirement, and ideally name the 403 string so operators can match on it — it says "scopes", not "permission", which is the only signal distinguishing it from a missing IAM role.

3. The streaming rationale in googlechat.toml asserts behaviour I could not reproduce

The recorded rationale for send-once is that per-token editing "would immediately 429" under the 1 write/sec/space limit. I could not reproduce that:

8 sequential PATCH to one message (~3/s)   → 8 × HTTP 200
25 concurrent PATCH to the same message     → 25 × HTTP 200

No 429 at any point. The quota table does document per-space writes as 1/second, but enforcement is evidently burst-tolerant. Caveat: my space was a DM, and the same page notes additional internal limits that are not exposed, so I am not claiming the limit does not exist — only that "would immediately 429" is not supported by observation.

That matters because the second, structural reason is solid and is the one that actually justifies send-once. A synthetic id cannot be patched at all:

PATCH /v1/spaces/<space>/messages/unified_a1b2c3d4e5f6
  → HTTP 400 INVALID_ARGUMENT
    "Missing or malformed message resource name in the request…"
PATCH /v1/spaces/<space>/messages/<real id>
  → HTTP 200

It is 400 INVALID_ARGUMENT, not 404 — the id does not parse as a resource name, so this fails earlier than the PR describes. 404 appears in the PR body, the googlechat.toml note, and the gateway.rs comment.

The conclusion is right; send-once is correct. I'm asking for the recorded reasoning to be corrected because docs/platforms/schema/googlechat.toml is durable rationale future maintainers will cite. Suggest leading with the resource-name failure and its real status code, and citing the quota as a documented constraint rather than an asserted runtime behaviour.


Contract challenges (Round 1)

The residual-risk claim about diagnosability does not match the code. §Accepted Residual Risks states a misconfiguration "surfaces as an explicit token-mint error on the first send, not a silent failure." On the reply path, get_token() returning None yields:

info!(text = %reply.content.text,
      "googlechat reply (dry-run, no credentials configured)");
// → GatewayResponse { success: false, error: "no credentials configured" }

For the ADC case that message is wrong — credentials are configured (use_adc = true); the mint failed. The real cause is on a separate error! line from get_token. And the reply is dropped, so the Chat user sees nothing at all. So it is a silent failure from the user's side and a misleading one from the operator's side. Combined with finding 2, an operator who follows Option C onto a default-scope GCE VM gets: no reply, a log line blaming missing credentials, and the real 403 elsewhere. I'd either amend the claim or distinguish the ADC failure in that message.

The environment constraint is stated as the wrong axis. §Accepted Residual Risks frames it as GCP-only versus off-GCP. The sharper constraint is the access scope: an on-GCP GCE workload can fail too. Worth restating in those terms, since that is what an operator has to check.

Suggested Acceptance Criterion. The existing manual criterion covers the success path on GKE. Given finding 2, please also cover a default-scope GCE VM and assert the failure is diagnosable, so criterion 2 above cannot silently regress.


Non-blocking

  • from_parts comment contradicts the code. The comment says ADC is enabled "only when no SA key was resolved," but adapter.metadata_source = use_adc.then(MetadataTokenSource::new) is unconditional; only the info! is gated. Behaviour is correct because get_token short-circuits on token_cache. Related: if sa_key_file is unreadable or the JSON is invalid, both paths only warn! and ADC silently takes over — an unannounced identity switch. A warn! naming the fallback would help.
  • ADC_TOKEN_LIFETIME_SECS comment. "IAM caps impersonated tokens at 3600s" — 1 hour is the default maximum, extendable to 12 hours via constraints/iam.allowServiceAccountCredentialLifetimeExtension. The clamp is safely conservative, but the comment reads as an absolute cap.
  • expireTime fallback is unreachable. The API reference states the expiration time is always set, so .unwrap_or(ADC_TOKEN_LIFETIME_SECS) will not trigger. Harmless; the Err(_) arm inside ttl_from_expire_time is the one doing real defensive work.
  • Cache is disabled for short-lived tokens. elapsed < ttl.saturating_sub(300) means any ttl <= 300 mints on every send. Same formula as GoogleChatTokenCache, so not new, but ADC pays an extra IAM hop per mint.
  • Case-sensitive bool parsing, duplicated three times. v == "true" || v == "1" in config.rs and twice in lib.rs means GOOGLE_CHAT_USE_ADC=True silently does nothing. config.rs already has env_flag_true_one, which is case-insensitive. Consistent with the adjacent allow_all_users precedent, so it's a judgement call.
  • The env path for use_adc is untested. The resolve test only exercises use_adc: Some(true); GOOGLE_CHAT_USE_ADC is added to the remove_var list but never set and asserted — which is precisely where the previous item would bite.
  • MetadataTokenSource::{metadata_base, iam_credentials_base} are pub. Documented as test seams, but as public fields production code can retarget the exchange, and the bearer sent there is a live metadata token. A private field with a #[cfg(test)] setter would keep the seam without widening the credential path.
  • Dockerfile.claude's OPENAB_BUILD_FEATURES looks like scope creep. Nothing in the repo passes it; the comment says "the reviewer image passes googlechat" but no such caller exists here. It's also in a file AGENTS.md marks deprecated in favour of Dockerfile.unified, which already builds --features unified and therefore already includes googlechat. Suggest dropping it from this PR.
  • IAM propagation takes about 30 seconds. The serviceAccountTokenCreator binding needed roughly 30 s before impersonation succeeded. refresh() has no retry, so the first send right after a fresh deployment can fail. Fine operationally, worth a line in the docs.

One underclaimed win

resolve_streaming fixes more than Google Chat. Following the call chain, Dispatcher::stream_prompt_blocks delegates to AdapterRouter::stream_prompt_blocks, which reads adapter.use_streaming() — the unified adapter's global Telegram flag — not the platform_supports_streaming-gated local in run_gateway_adapter (that one only feeds GatewayAdapter::use_streaming on the standalone path). So in unified/embedded mode line and lineworks were also inheriting the Telegram flag and attempting edits on platforms with no edit API. This PR fixes that too. Worth stating in the description so reviewers see the real blast radius.


Summary

The ADC half is sound and I verified it end to end against live APIs. My recommended blockers are one rebase, one documentation gap that will break real GCE deployments, and one correction to durable recorded rationale — none of them defects in the ADC implementation itself. Everything else is non-blocking.

I don't have maintainer rights here, so this is a review comment rather than a formal block, and the contract freeze is @thepagent's call.

Evidence gathered against a scratch GCP project I own; the temporary human-account impersonation grant used for testing has been removed.

@thepagent

Copy link
Copy Markdown
Collaborator

CI and smoke test failing. Changing status to Draft until fixed.

@thepagent
thepagent marked this pull request as draft August 27, 2026 16:40
Keyless ADC (MetadataTokenSource): mint a chat.bot-scoped token from the
workload's own GCP identity — GCE metadata (SA email + base token) -> IAM
Credentials generateAccessToken (self-impersonation). No SA key file.
Config [googlechat].use_adc / GOOGLE_CHAT_USE_ADC; auth precedence SA key >
ADC > static token; cache under the IAM-granted expireTime (fallback 3600s).

Send-once for Google Chat: its write rate limit is 1/sec/space
(create+patch+delete combined) so per-token streaming edits 429, and the
unified adapter returns a synthetic message id that patch can't target (404).
googlechat added to NON_STREAMING_PLATFORMS (renamed from
NON_EDITABLE_PLATFORMS); resolve_streaming forces send-once on both the
embedded dispatch (stream_prompt_blocks) and WebSocket gateway paths.

Also: Dockerfile.claude OPENAB_BUILD_FEATURES arg, Helm googleChat.useAdc
value, docs + config-first conformance entry + googlechat.toml schema record.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sebastian-hsu
sebastian-hsu force-pushed the feat/googlechat-adc-pr branch from f7baf96 to 0b57f3b Compare August 28, 2026 00:29
@sebastian-hsu

Copy link
Copy Markdown
Contributor Author

Thanks @canyugs — verifying the auth flow against real GCP/Chat rather than the diff is exactly the right lens here, and the scope-narrowing evidence (a cloud-platform-scoped base token producing a chat.bot-only minted token) is the strongest possible confirmation of the §Why argument. All three blockers are addressed.

1. Stale base — clippy on pre-existing wizard.rs. Rebased onto current origin/main (now includes #1511), which clears the setup/wizard.rs:163 useless_format. Verified on the rebased tree: cargo clippy --workspace -- -D warnings and --features unified both clean, cargo test -p openab-gateway green. The openab-core secrets::tests::resolve_exec_nonzero_exit failure is pre-existing on unmodified main, as you confirmed.

2. docs/google-chat.md Option C — GCE access scope. Added the missing requirement: the metadata base token must carry cloud-platform (or .../auth/iam) scope, because generateAccessToken requires it on the caller. A default-scope GCE VM returns 403 PERMISSION_DENIED: "Request had insufficient authentication scopes." even with the IAM binding correct — I named that string and pointed out it says scopes, not permission, so operators can tell it apart from a missing role. Also noted GKE Workload Identity / Cloud Run are cloud-platform-scoped and unaffected, and that GCE scopes are immutable after creation (set-scopes + stop/start).

3. googlechat.toml streaming rationale. Corrected to lead with the decisive, reproducible reason: the synthetic unified_<hex> id is not a valid resource name, so patch rejects it with 400 INVALID_ARGUMENT — not 404, and it fails before any edit applies. The 1 write/sec-per-space quota is now cited as a documented constraint on high-frequency editing, not an asserted "would immediately 429" (your 8-sequential / 25-concurrent PATCH-all-200 result is noted — enforcement is burst-tolerant). The same 404→400 / quota-framing correction is applied in the PR body (What problem / At a Glance / Alternatives).

§Why now also carries your stronger point: chat.bot is a Workspace scope and is not a subset of cloud-platform, so no ?scopes= on the metadata token can produce it — generateAccessToken self-impersonation is the only viable keyless path, not merely the cleaner one.

On CI: the smoke-test (Dockerfile.hermes) failure is the upstream install-script curl exit 22 you identified (the -unified hermes variant is green) — unrelated to this change.

Reviewed head is now the rebased commit. Thanks again for the thoroughness.

@canyugs canyugs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 — fix verification

Scoped per docs/review-contract.md for later rounds: unresolved Round 1 findings, changes since the last reviewed commit, regressions from those changes, and Acceptance Criteria compliance. No new architecture discovery.

Reviewed head: 0b57f3ba (previous: f7baf967).

All three Round 1 blockers verified fixed

1. Stale base. origin/main is now an ancestor of the head, so the wizard.rs:163 useless_format from the old merge base is gone. Independently re-run on 0b57f3ba:

cargo clippy --workspace -- -D warnings                       → clean
cargo clippy --workspace --features unified -- -D warnings     → clean
cargo test -p openab-gateway                                   → 315 passed, 0 failed
cargo test --locked -p platform-schema (standalone manifest)    → 17 passed, 0 failed

The check job has flipped from fail to pass on CI, matching.

2. GCE access scope, docs/google-chat.md. Resolved, and more completely than I asked. Option C now states that the metadata base token must carry cloud-platform (or .../auth/iam) because generateAccessToken requires it on the caller; reproduces the exact 403 PERMISSION_DENIED: "Request had insufficient authentication scopes."; tells operators to match on scopes rather than permission to distinguish it from a missing role; notes GKE Workload Identity and Cloud Run satisfy it automatically; and covers the part that actually bites, that GCE scopes are immutable after creation, with the set-scopes plus stop/start remedy. That is the failure mode from Round 1 finding 2 fully described.

3. googlechat.toml streaming rationale. Resolved. The note now leads with the decisive and reproducible reason — the synthetic unified_<hex> id is not a valid resource name, so patch rejects it with 400 INVALID_ARGUMENT before any content is applied — and demotes the quota to "a documented constraint on high-frequency editing rather than an observed hard failure, since enforcement is burst-tolerant in practice." That is an accurate description of what I measured. 404 no longer appears in any Google Chat surface.

The §Why addition is a good call: stating that chat.bot is a Workspace scope and not a subset of cloud-platform, so no ?scopes= on the metadata token can produce it, turns the design rationale from a preference into a necessity.

Regression check: clean, and narrowly so

Comparing each head's net diff against its own merge base, file by file, only two files differ between f7baf967 and 0b57f3ba:

* changed   docs/google-chat.md
* changed   docs/platforms/schema/googlechat.toml
= unchanged Dockerfile.claude, charts/…/gateway.yaml, charts/…/values.yaml,
            config.toml.example, crates/openab-core/src/{adapter,config,gateway}.rs,
            crates/openab-gateway/src/adapters/googlechat.rs,
            crates/openab-gateway/src/lib.rs,
            crates/openab-gateway/tests/config_first_conformance.rs,
            docs/config-reference.md, docs/platforms/schema/lineworks.toml,
            src/main.rs

The file list is identical at both heads and the Rust delta is empty, so there is no behavioural regression surface introduced by this round. The rebase is clean with nothing smuggled in.

Unresolved from Round 1 — ORIGINAL, and I am not blocking on any of them

Recording these so they are not lost, not to reopen them. The Round 1 contract challenges and the ten non-blocking items are unchanged in 0b57f3ba. My position is that none of them meet the Late Blocker Gate and they should not hold this PR:

  • §Accepted Residual Risks still reads "A misconfiguration surfaces as an explicit token-mint error on the first send, not a silent failure," and still frames the constraint as GCP-only. Both are now narrower than what docs/google-chat.md itself says after fix 2, so the contract text and the docs disagree. Since the docs are where operators look and they are now correct, this is bookkeeping.
  • No Acceptance Criterion covers the default-scope GCE case, so fix 2 is documentation-only with no test guarding it. Worth a Follow-up entry rather than a blocker.
  • The ten non-blocking items from Round 1 stand as written, including the from_parts comment still contradicting the unconditional metadata_source assignment, the ADC_TOKEN_LIFETIME_SECS comment still describing 3600 s as a hard cap when it is the default maximum, and Dockerfile.claude's unused OPENAB_BUILD_FEATURES.

Two process notes, neither a review finding:

  • I do not see a contract freeze record from the maintainer, so Round 1 was never formally closed under the lifecycle in docs/review-contract.md. I have applied later-round scoping anyway, since the practical effect is the same.
  • The PR is still a Draft, set on the previous head over CI failures that are now fixed. That status is currently self-sustaining: docker-smoke-test.yml and docker-smoke-test-unified.yml both gate on if: github.event.pull_request.draft == false, so the smoke tests cannot run to clear the condition they were paused for. Someone with write access will need to mark it Ready.

Bottom line

Every blocker I raised in Round 1 is fixed and independently verified, this round introduced no regressions, and the only code-affecting criteria I can check locally all pass. No blockers remain from my side. The remaining items are contract wording and optional hardening, and the freeze and merge decisions are @thepagent's.

@sebastian-hsu
sebastian-hsu marked this pull request as ready for review August 28, 2026 05:56
@sebastian-hsu

Copy link
Copy Markdown
Contributor Author

Marking ready for review. All three Round 1 blockers are fixed and independently re-verified in @canyugs' Round 2: cargo clippy --workspace (and --features unified) clean on the rebased head, gateway/platform-schema tests green, and the docs (google-chat.md GCE access-scope + 403 string) and googlechat.toml rationale (patch of the synthetic id is 400 INVALID_ARGUMENT, quota framed as a documented constraint) corrected.

The docker-smoke-test / docker-smoke-test-unified jobs are gated on draft == false, so they couldn't run while this sat in Draft — moving it out of Draft lets them run. @thepagent — the contract freeze and merge decisions remain yours.

@chaodu-obk

chaodu-obk Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Important

CHANGES REQUESTED ⚠️ - The keyless ADC design is sound and was validated end-to-end against live GCP APIs by an external reviewer, but the credential fallback semantics (a broken SA key silently switching to the ADC identity; an ADC mint failure never reaching a configured static token) and several token-cache edge cases need tightening before merge.

What This PR Does

Adds a third, keyless authentication option for the Google Chat gateway adapter: when use_adc is set, the adapter mints a chat.bot-scoped token from the workload's own GCP identity (GCE metadata server + IAM Credentials generateAccessToken self-impersonation) instead of requiring a mounted service-account key file or a static token. It also corrects the streaming declaration: Google Chat is now send-once (NON_STREAMING_PLATFORMS), because the unified adapter's synthetic message id cannot be patched (400 INVALID_ARGUMENT) and the API documents a 1 write/sec-per-space quota.

How It Works

  • New MetadataTokenSource (crates/openab-gateway/src/adapters/googlechat.rs): reads the default SA email and a base token from the metadata server, exchanges it via generateAccessToken (SA impersonates itself, scope chat.bot), and caches the result behind an RwLock with double-checked refresh and a 300 s margin.
  • get_token precedence: SA-key cache -> ADC -> static token.
  • resolve_streaming(platform, adapter_prefers) in crates/openab-core/src/adapter.rs now gates the embedded dispatch path with the same platform list the WebSocket path already used; NON_EDITABLE_PLATFORMS renamed to NON_STREAMING_PLATFORMS and gains googlechat.
  • Config plumbing: use_adc / GOOGLE_CHAT_USE_ADC through config.rs, lib.rs, src/main.rs, the Helm chart, and docs.

Findings

# Severity Finding Location
1 🟡 Configured-but-unreadable/malformed SA key falls open to the ADC identity, contradicting the documented "SA key wins" precedence crates/openab-gateway/src/adapters/googlechat.rs (from_parts / get_token)
2 🟡 ADC mint failure returns None immediately; a configured static access_token is never reached and the reply is dropped crates/openab-gateway/src/adapters/googlechat.rs (get_token)
3 🟡 TTL edge cases: a ttl = 0 mint result is cached and the already-expired token is served for the current send; any ttl <= 300 disables caching entirely (one IAM mint per send); the wiremock fixture uses an already-past expireTime and asserts the expired token is returned crates/openab-gateway/src/adapters/googlechat.rs (get_token / ttl_from_expire_time, test metadata_token_source_mints_chat_bot_token)
4 🟡 Inside the 300 s refresh window, a transient mint error is returned even though the cached token is still valid; serve-stale-on-error would avoid an avoidable outage crates/openab-gateway/src/adapters/googlechat.rs (MetadataTokenSource::get_token)
5 🟡 pub metadata_base / pub iam_credentials_base are a concrete bearer-exfiltration vector: no HTTPS/scheme validation, default redirect-following client, and the metadata bearer is sent to whatever base is configured crates/openab-gateway/src/adapters/googlechat.rs (MetadataTokenSource)
6 🟡 Key-to-ADC migration leaves the old SA-key Secret in the cluster: gateway-secret.yaml gates on saKeyJson/accessToken only (not useAdc) and carries helm.sh/resource-policy: keep, so the retained key material undercuts the "no key to leak" goal; needs a documented cleanup step charts/openab/templates/gateway-secret.yaml:10,21
7 🟡 Stale doc contradicts this PR: the features list still advertises "Streaming (edit_message) - the bot edits its initial reply in-place" for Google Chat, which this PR disables docs/google-chat.md:223
8 🟡 Dockerfile.claude OPENAB_BUILD_FEATURES has zero callers in the repo and zero CI coverage (the smoke matrix builds it with empty build_args), and the file is deprecated in favor of Dockerfile.unified; drop it from this PR Dockerfile.claude:4, .github/workflows/docker-smoke-test.yml:25
9 🟢 Externally validated E2E on live GCP; no token material logged; good wiremock coverage of the happy path; resolve_streaming also fixes a latent unified-path bug where line/lineworks inherited the Telegram streaming flag -
Finding Details

🟡 F1: SA-key failure falls open to ADC identity

from_parts converts an unreadable key file or malformed key JSON into token_cache = None with only a warn!, and then installs metadata_source unconditionally when use_adc is true. get_token therefore silently sends as the workload identity instead of the configured key identity, contradicting the documented "Ignored when saKeyJson is set" precedence in values.yaml, config-reference.md, and docs/google-chat.md. The in-code comment ("ADC only when no SA key was resolved") describes intent the code does not implement - an external reviewer noted the comment mismatch; the escalation here is that this is an auth-principal selection bug, not just a comment bug.

Requested change: track whether a key was configured (not just whether it parsed), and on read/parse failure either fail closed or emit an explicit warn! naming the identity switch; add regression tests for malformed and unreadable SA keys with use_adc = true.

🟡 F2: ADC failure never falls through to the static token

In get_token, the metadata_source arm returns None on error. A deployment that configures both use_adc and a static access_token (as a deliberate fallback) gets dropped replies during a metadata/IAM outage even though a valid credential is available. If ADC failure is intended to be terminal, document that; otherwise fall through to self.access_token and log the degradation.

🟡 F3: TTL edge cases and a test fixture that masks one

refresh() can legitimately produce ttl = 0 (already-expired expireTime, e.g. clock skew); get_token stores it and returns the token for the current send anyway. Separately, any ttl <= TOKEN_REFRESH_MARGIN_SECS makes elapsed < ttl.saturating_sub(300) always false, so every send re-mints (an extra IAM round-trip per message). The metadata_token_source_mints_chat_bot_token fixture uses expireTime: 2026-08-25T12:00:00Z, which is already in the past at test-run time - the test asserts an expired token is served without noticing. Use a far-future expireTime in the fixture and decide the ttl = 0 behavior explicitly (error, or mint-and-serve with a warning).

🟡 F4: Transient refresh error discards a still-valid cached token

Once elapsed >= ttl - 300, a refresh failure propagates as Err even though the cached token remains valid for up to 300 more seconds, and concurrent callers then serially retry under the write lock. Serving the stale-but-valid token on refresh failure (and only erroring after real expiry) removes an avoidable outage window and the retry pile-up.

🟡 F5: Public endpoint-override fields widen the credential path

metadata_base and iam_credentials_base are pub mutable Strings consumed directly by request construction, with reqwest::Client::new() (redirect-following by default) and bearer_auth(base_token). Any in-process caller that retargets iam_credentials_base to a plain-HTTP or attacker-controlled host receives the live metadata bearer. An external reviewer flagged the fields as test seams; the escalation here is the concrete exfil path. Make the fields private with a #[cfg(test)] setter, enforce https for the IAM base, and use a no-redirect client for both calls.

🟡 F6: Helm key-to-ADC migration retains the old key Secret

gateway-secret.yaml's $hasGoogleChat does not include useAdc (correct for fresh ADC-only installs - no Secret is rendered), but the Secret carries helm.sh/resource-policy: keep. Upgrading an existing release from saKeyJson to ADC stops managing the Secret while leaving the base64 key material in the cluster indefinitely. Add a migration note (delete the orphaned Secret) to docs/google-chat.md Option C, or reconsider the keep policy interaction.

🟡 F7: Stale streaming bullet in docs/google-chat.md

Line 223 still lists "Streaming (edit_message) - when OAB streaming is enabled, the bot edits its initial reply in-place as tokens arrive (typewriter effect)" as a feature. This PR makes that impossible by design. Update the bullet to describe send-once.

🟡 F8: OPENAB_BUILD_FEATURES is unexercised scope creep

Verified: zero callers repo-wide, and the CI smoke matrix builds Dockerfile.claude with build_args: "", so the new conditional never runs in CI. The comment references a "reviewer image" caller that does not exist in this repo, and AGENTS.md deprecates this Dockerfile in favor of Dockerfile.unified (which already builds --features unified, including googlechat). This confirms an external reviewer's suggestion to drop it from this PR.

🟢 F9: What is done well

See "What's Good" below.

Addressing External Reviewer Feedback

Reviewer @canyugs (Rounds 1 and 2)

Round 1 blockers: stale base (clippy on pre-existing wizard.rs); docs/google-chat.md Option C missing the GCE access-scope requirement; googlechat.toml streaming rationale asserting an unreproduced "immediate 429".

Addressed and independently re-verified in their Round 2 at this head (0b57f3ba): rebase clean, clippy clean on both feature sets, the Option C scope requirement documented with the exact 403 string, and the schema rationale now leads with the 400 INVALID_ARGUMENT structural reason. We confirm the same from the local tree.

Round 1/2 non-blocking items (comment/code mismatch in from_parts, pub test seams, case-sensitive bool parsing, untested env path, unused OPENAB_BUILD_FEATURES, IAM propagation delay, etc.)

⚠️ Partially escalated: this review escalates the from_parts mismatch to F1 (auth-principal selection bug, not just a stale comment), the pub seams to F5 (concrete bearer-exfil path), and the Dockerfile arg to F8 (verified zero CI coverage). The remaining items (case-sensitive bool parsing x3, untested GOOGLE_CHAT_USE_ADC env path, IAM propagation retry) stand as recorded and we agree they are non-blocking individually, though the bool-parsing and env-path items would naturally be fixed together with F1's regression tests.

Reviewer @thepagent

CI and smoke test failing. Changing status to Draft until fixed.

Resolved: the PR was rebased, all 49 check runs are green at 0b57f3ba (including both smoke-test matrices), and the PR is out of Draft.

Baseline Check
  • PR opened: 2026-08-27; head 0b57f3bafd07f8b493eed72177c3b7db991cb131; base main; merge-base d4f376f (= current main HEAD, clean rebase); diff +445/-33 across 15 files.
  • Main already has: SA-key JWT-bearer exchange and static-token auth for Google Chat; NON_EDITABLE_PLATFORMS gating on the WebSocket path only.
  • Net-new value: keyless ADC token source (no key file on GCP), and closing the embedded-dispatch streaming gap (which also fixes line/lineworks inheriting the Telegram flag on the unified path).
  • All four source refs in docs/platforms/schema/googlechat.toml verified to resolve to real symbols in the tree.

5. Three Reasons We Might Not Need This PR

  1. Speculative deployment surface - the ADC path only pays off for deployments running the Chat bot on GCP as its own SA. If no current openab deployment matches that shape, this is maintenance surface bought ahead of demand. (Mitigated: opt-in, off by default, and the key-file path is untouched.)
  2. It entrenches the name-list stopgap - gateway.rs itself says the right long-term model is a capability handshake; this PR extends the platform-name list into a second gate (resolve_streaming) instead of building that handshake, making the eventual migration slightly larger.
  3. A third token path in one adapter - MetadataTokenSource re-implements GoogleChatTokenCache's double-checked cache/refresh machinery. A shared token-source abstraction first would have avoided duplicating subtle locking/TTL logic that F3/F4 show is easy to get slightly wrong.
What's Good (🟢)
  • The core design choice (self-impersonation via generateAccessToken rather than plain ADC) is correct and, as the external review proved empirically, the only viable keyless way to obtain a chat.bot token - the PR body documents this necessity clearly.
  • End-to-end validation against live GCP/Chat APIs by an external reviewer, including scope-narrowing evidence and IAM-binding necessity.
  • No token material is ever logged; error strings carry context without secrets.
  • resolve_streaming quietly fixes a real latent bug: on the unified path, line/lineworks inherited the Telegram streaming flag and attempted edits on platforms with no edit API.
  • Docs and schema rationale are unusually thorough, including the exact 403 diagnostic string operators will need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants