feat(googlechat): keyless ADC auth + send-once for the unified adapter - #1512
feat(googlechat): keyless ADC auth + send-once for the unified adapter#1512sebastian-hsu wants to merge 2 commits into
Conversation
openab-app
Bot
added
the
closing-soon
639b61f to
f7baf96
Compare
openab-app
Bot
removed
the
closing-soon
canyugs
left a comment
canyugs
left a comment
There was a problem hiding this comment.
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_partscomment contradicts the code. The comment says ADC is enabled "only when no SA key was resolved," butadapter.metadata_source = use_adc.then(MetadataTokenSource::new)is unconditional; only theinfo!is gated. Behaviour is correct becauseget_tokenshort-circuits ontoken_cache. Related: ifsa_key_fileis unreadable or the JSON is invalid, both paths onlywarn!and ADC silently takes over — an unannounced identity switch. Awarn!naming the fallback would help.ADC_TOKEN_LIFETIME_SECScomment. "IAM caps impersonated tokens at 3600s" — 1 hour is the default maximum, extendable to 12 hours viaconstraints/iam.allowServiceAccountCredentialLifetimeExtension. Theclampis safely conservative, but the comment reads as an absolute cap.expireTimefallback is unreachable. The API reference states the expiration time is always set, so.unwrap_or(ADC_TOKEN_LIFETIME_SECS)will not trigger. Harmless; theErr(_)arm insidettl_from_expire_timeis the one doing real defensive work.- Cache is disabled for short-lived tokens.
elapsed < ttl.saturating_sub(300)means anyttl <= 300mints on every send. Same formula asGoogleChatTokenCache, so not new, but ADC pays an extra IAM hop per mint. - Case-sensitive bool parsing, duplicated three times.
v == "true" || v == "1"inconfig.rsand twice inlib.rsmeansGOOGLE_CHAT_USE_ADC=Truesilently does nothing.config.rsalready hasenv_flag_true_one, which is case-insensitive. Consistent with the adjacentallow_all_usersprecedent, so it's a judgement call. - The env path for
use_adcis untested. The resolve test only exercisesuse_adc: Some(true);GOOGLE_CHAT_USE_ADCis added to theremove_varlist but never set and asserted — which is precisely where the previous item would bite. MetadataTokenSource::{metadata_base, iam_credentials_base}arepub. 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'sOPENAB_BUILD_FEATURESlooks like scope creep. Nothing in the repo passes it; the comment says "the reviewer image passesgooglechat" but no such caller exists here. It's also in a file AGENTS.md marks deprecated in favour ofDockerfile.unified, which already builds--features unifiedand therefore already includesgooglechat. Suggest dropping it from this PR.- IAM propagation takes about 30 seconds. The
serviceAccountTokenCreatorbinding 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.
|
CI and smoke test failing. Changing status to Draft until fixed. |
f7baf96 to
0b57f3b
Compare
|
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 1. Stale base — clippy on pre-existing 2. 3. §Why now also carries your stronger point: On CI: the Reviewed head is now the rebased commit. Thanks again for the thoroughness. |
canyugs
left a comment
canyugs
left a comment
There was a problem hiding this comment.
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.mditself 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_partscomment still contradicting the unconditionalmetadata_sourceassignment, theADC_TOKEN_LIFETIME_SECScomment still describing 3600 s as a hard cap when it is the default maximum, andDockerfile.claude's unusedOPENAB_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.ymlanddocker-smoke-test-unified.ymlboth gate onif: 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.
|
Marking ready for review. All three Round 1 blockers are fixed and independently re-verified in @canyugs' Round 2: The |
This comment has been minimized.
This comment has been minimized.
0b57f3b to
882bd62
Compare
|
Thanks @chaodu-obk — the escalations are fair; F1 (auth-principal selection, not just a stale comment) and F5 (a concrete exfil path, not just a test seam) are the right lens. All eight findings (F1–F8) are addressed. The rebuilt image was also verified end-to-end on a live GCP workload: keyless ADC minted a F1 — a configured-but-unreadable SA key no longer falls open silently. F2 — ADC failure now falls through to the static token. The F3 — TTL edge cases + fixture. Added F4 — serve-stale-on-error. Inside the refresh window, a transient mint error now serves the still-valid cached token (while F5 — endpoint-override exfil path closed. F6 — key→ADC migration cleanup documented. Added a migration note to F7 — stale streaming bullet fixed. F8 — dropped On the non-blocking items you and @canyugs agree on (case-sensitive bool parsing ×3, the untested Verified on the rebased tree: |
This comment has been minimized.
This comment has been minimized.
882bd62 to
f0d196f
Compare
|
Thanks @chaodu-obk — F10 fixed, and thanks for the F1–F8 fix-verification. Both
Comment-only change: the delta from the previous head is exactly these two comment blocks (zero code lines), so the F1–F8 fixes you verified at |
This comment has been minimized.
This comment has been minimized.
|
Thanks @chaodu-obk — F12 and F13 fixed.
Comment/doc-only change: the delta from the previous head is the two |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Maintainer follow-up on review round 5: all open findings (F15-F28) are fixed in commit |
- F15: bound every token-mint request (SA-key exchange, metadata, IAM Credentials) with a 10s TOKEN_REQUEST_TIMEOUT so a hung connection cannot stall senders behind the cache write lock or defeat the ADC -> static token degradation path - F16: reject empty/whitespace minted tokens at all three extraction sites (SA-key exchange, metadata base token, generateAccessToken) so a malformed response follows the degradation path instead of being cached as valid - F17: correct the shorthand precedence wording in config.toml.example, config.rs, config-reference.md, google-chat.md env table, and values.yaml to name the configured-but-unloadable-key -> ADC fallback - F19: refuse edit_message for non-resource-name (synthetic unified_) ids locally instead of sending a doomed patch (400 INVALID_ARGUMENT) - F20: cross-reference the two sibling streaming gates (resolve_streaming / platform_supports_streaming) in both docs - F21: document get_token precedence and its asymmetric failure behavior at the function - F22: replace from_parts' five positional args with a named GoogleChatParts struct; all call sites and tests name their fields - F23: install metadata_source only when no SA key loaded, so the code encodes the precedence it documents - F24: drop private review-numbering labels (F1/F2/F4/F5) from source comments and test comments - F25: fix the self-contradictory 'immutable after creation' GCE scope wording in docs/google-chat.md Option C - F26: identify the orphaned Secret (agentFullname convention + discovery commands) in the key-to-ADC migration note - F27: log the resolved service-account identity on successful mint - F28: classify generateAccessToken failures (insufficient_scope / missing_role / api_not_enabled) in the error string New regression tests: loaded-key-suppresses-ADC-source, blank-minted- token rejection (wiremock), synthetic-id edit_message no-op (wiremock, expect(0)), and error-classification table.
|
Thanks @chaodu-obk — F15–F28 addressed. I took the preferred path from your round-5 follow-up: cherry-picked Verified before pushing:
The delta from the previously reviewed head ( |
This comment has been minimized.
This comment has been minimized.
![chaodu-obk[bot]](https://avatars.githubusercontent.com/u/262003297?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1512 (comment)
| // 3. Exchange the base token for a chat.bot-scoped token via IAM | ||
| // Credentials generateAccessToken (the SA impersonates itself). | ||
| let url = format!( | ||
| "{}/v1/projects/-/serviceAccounts/{email}:generateAccessToken", |
There was a problem hiding this comment.
🔴 F1 - Replace prohibited access-token self-impersonation
This URL targets the same service account whose short-lived metadata access token authenticates the request. Google Cloud explicitly prohibits using a service account's short-lived credential to generate a new access token for that same service account and documents FAILED_PRECONDITION.
Official reference: https://docs.cloud.google.com/iam/docs/service-account-creds#self-impersonation
Requested change: require a distinct Google Chat target service account, grant the runtime service account Token Creator on that target, and reject runtime/target equality before obtaining the base token.
|
Thanks @chaodu-obk. On F1 I'd push back with current evidence; F2 I read as pre-existing send-once semantics rather than a defect this PR introduces. I've left #1513 / F1 — self-impersonation is working in this configuration; it is not returning The keyless mint was exercised end-to-end on a live GCP workload today, with the same runtime identity that authenticates the request as the This is not a stale or different-identity result. The same SA authenticates and is the target; the only difference is the requested scope ( That said, a distinct Chat target SA (runtime SA impersonates a separate Chat SA) is a valid and arguably more portable alternative. I'd frame it as an optional deployment topology rather than a correctness fix, since it adds a second SA and cross-project IAM surface. I'm happy to (a) document it as an alternative in Option C, and (b) add a defensive log/guard if a self-impersonation config ever does return F2 — real behavior, but pre-existing send-once semantics on the WebSocket path, not a defect introduced here. Correct that I do agree googlechat can report a delivery failure (unlike LINE), so surfacing it is a genuine improvement. I'd propose tracking it as a follow-up next to F29 — send-once acknowledgement modeled as a capability separate from cosmetic streaming, applied to every send-once platform that can ack — rather than folding a cross-platform ack redesign into this PR. If you'd prefer it in-scope here, I can add a googlechat-scoped version that awaits the adapter Head unchanged ( |
github-actions
Bot
added
the
closing-soon
|
Caution This PR has been waiting on the author for more than 2 days (labeled @sebastian-hsu — You must add a new comment on this PR to remove the |
|
Still active — keeping this open. Status at head
@thepagent — this is now a design judgement rather than a defect, and the empirical result contradicts the documented failure mode. Could you rule on whether self-impersonation is acceptable here, or whether a distinct Chat target service account should be mandated? I'll implement whichever you decide — likewise on whether F2 belongs in this PR or as a follow-up. |
|
🔒 Auto-closing: this PR has had the If you'd like to continue working on this, please submit a new PR and link to this one if necessary. |
|
Important CHANGES REQUESTED What This PR DoesThis PR adds keyless Google Chat authentication through GCE metadata and IAM Credentials, preserves the existing SA-key and static-token paths, and forces Google Chat into send-once mode because the unified synthetic message id cannot be patched. How It Works
Findings
Finding Details🔴 F1: Replace prohibited access-token self-impersonationThe metadata endpoint returns a short-lived access token for the default runtime service account. The implementation then puts that same metadata email in the Google Cloud's current official service-account credentials documentation explicitly states that the Service Account Credentials API prohibits "using a short-lived credential for a service account to generate a new access token for the service account." It documents this failure:
Official reference: https://docs.cloud.google.com/iam/docs/service-account-creds#self-impersonation (last updated 2026-08-28 UTC). The external live result is useful evidence about one observed deployment, but it does not supersede the current vendor contract. The official rule is based on caller and target identity, not whether the requested scope differs. This also exposes a rationale mismatch: the PR body claims runtime/bot identity separation, while the implementation always uses the metadata service account as both caller and target. Requested change: require a distinct Google Chat target service account, grant the attached runtime service account 🟡 F2: Preserve delivery acknowledgement independently of streaming
This is in scope even though LINE and LINE WORKS already use legacy fire-and-forget semantics: before this PR, Google Chat could be configured to stream and therefore carried an acknowledgement ID; adding it to Requested change: model normal-reply acknowledgement separately from cosmetic streaming. Google Chat should remain send-once while carrying and awaiting a request ID; 🟢 F3: What is done wellSee "What's Good" below. Group Review Summary
Addressing External Reviewer Feedback@canyugs (Rounds 1 and 2)
Partially superseded by authoritative vendor documentation: the original blockers remain fixed, but Google's current service-account credential contract explicitly prohibits the exact short-lived-access-token self-impersonation performed here. F1 follows that higher-authority contract while preserving the empirical result as an important discrepancy to report upstream. @sebastian-hsu
Not accepted for F1: the official prohibition does not contain a different-scope exception; it keys on a short-lived caller credential and the same target service account. Not deferred for F2: this PR newly forces Google Chat from configurable streaming/acknowledged behavior to unconditional send-once/no-ack behavior on the WebSocket path, so the false-success regression belongs to this change. The F15-F28 fixes are present, and F29 is acceptably tracked by #1514. @thepagent
Resolved for the reviewed commit: the PR is open and ready for review. Earlier exact-head runs at Baseline Check
Validation
5. Three Reasons We Might Not Need This PR
What's Good (🟢)
|
|
Important CHANGES REQUESTED Scope of this reviewIndependent read of the reconciled PR diff only — 14 files, +879/-57 against merge-base The decisive blocker is the ADC self-impersonation contract issue already raised; nothing below supersedes it. If that forces a redesign to a distinct-target-SA topology, findings 1–2 are revisited as part of that rewrite. Findings (supplementary)
Finding Details🔴 F1: no-redirect client fails openclient: reqwest::Client::builder()
.redirect(reqwest::redirect::Policy::none())
.build()
.unwrap_or_default(),The adjacent comment states the metadata bearer must never follow a redirect to a third host. But on a builder error 🔴 F2: proxies not disabled on the token-mint clientThe client is built without 🟡 F3: test isolation
🟡 F4: contradictory doc comment
🟡 F5: cross-credential fallback
Test coverage gaps (non-blocking)
Verified
|
|
close in favor of #1513 |
|
Correction, for anyone reading this thread later: my push-back on the round-7 F1 finding above was wrong. I argued that keyless ADC self-impersonation was acceptable because the requested scope differed ( The observation that it worked in practice was real, but the right reading of that is "not currently enforced here", not "supported" — and that is not a basis for a production auth path. We have since adopted the distinct-target design from #1513 in production and verified it, including a cross-project target. Full correction and the production validation are on #1513. |
4 participants
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-mintedaccessToken. 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.bottoken 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.patchrejects it with400 INVALID_ARGUMENTbefore 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 CredentialsgenerateAccessToken, the service account impersonating itself). Also declare Google Chat as send-once so core stops attempting cosmetic edits the API rejects.Non-goals
saKeyJsonand static-accessTokenpaths — they stay, and keep precedence over ADC.roles/iam.serviceAccountTokenCreatorand enables the IAM Credentials API.Accepted Residual Risks
saKeyJson/accessToken. Documented indocs/google-chat.md, and it is opt-in (use_adc = falseby default).serviceAccountTokenCreatorover itself and the IAM Credentials API enabled. A misconfiguration surfaces as an explicit token-mint error on the first send, not a silent failure.Acceptance Criteria
GOOGLE_CHAT_USE_ADC=trueon a GCP workload running as the Chat-app SA (no key file mounted), the adapter mints achat.bottoken and posts a message successfully.get_token:saKeyJson(if set) > ADC (metadata) > staticaccessToken.NON_STREAMING_PLATFORMS;resolve_streamingforces send-once on both the embedded-dispatch and the WebSocket gateway paths (no post-then-edit).platform-schemaconformance passes: everysourceref indocs/platforms/schema/googlechat.tomlresolves to a real symbol.cargo clippy --workspaceand--features unifiedare clean;cargo testpasses.Follow-ups
MetadataTokenSourceis general enough that keyless / workload-identity auth for other Google-API-backed adapters could reuse it — deferred, non-blocking.At a Glance
Prior Art & Industry Research
OpenClaw (
openclaw/openclaw) — largest open-source AI agent gatewayGoogle 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.botscope." Inbound verifies the request'sAuthorization: BearerID token against a configuredaudience.docs/channels/googlechat.md. Grepping that surface foradc|application default|workload|metadata|impersonat|keylessreturns 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+ platformsGoogle 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) explicitservice_account_json, (2)GOOGLE_APPLICATION_CREDENTIALS, (3)google.auth.default()— ADC on Cloud Run / GCE / GKE with an attached workload identity, scopeschat.bot+pubsub.plugins/platforms/google_chat/adapter.py(_load_sa_credentials), docgoogle_chat.md.google.auth.default()— it uses the attached SA's own metadata token directly. It does not call IAM CredentialsgenerateAccessTokento self-impersonate, so the attached SA must itself carrychat.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
generateAccessToken(gated byroles/iam.serviceAccountTokenCreator) — the exact mechanism this PR uses.Comparison
chat.bot; no keyless pathGOOGLE_APPLICATION_CREDENTIALS→ plaingoogle.auth.default()(attached SA's own token); no self-impersonationgenerateAccessTokengenerateAccessTokenself-impersonationchat.bottoken; auto-refresh, no key on diskProposed Solution
Keyless ADC outbound token (
MetadataTokenSource,crates/openab-gateway/src/adapters/googlechat.rs). A new token source that, whenuse_adcis set:generateAccessTokenso the service account impersonates itself, scoped tohttps://www.googleapis.com/auth/chat.bot;RwLockwith a double-checked refresh, using the same 300 s margin as the existing paths (ttl_from_expire_timeclamps the lifetime to[0, 3600]).get_tokengains a strict precedence:token_cache(SA-key exchange) →metadata_source(ADC) → staticaccess_token. Wiring:GoogleChatConfig.use_adc(config.rs), plumbed throughGatewayGoogleChatConfigand the threefrom_partscall sites (lib.rs,src/main.rs), surfaced asGOOGLE_CHAT_USE_ADCand the chart'sgateway.googleChat.useAdc.Send-once streaming.
NON_STREAMING_PLATFORMS(renamed fromNON_EDITABLE_PLATFORMS,gateway.rs) now gates a newresolve_streaming(platform, adapter_prefers_streaming)(adapter.rs) used by the embeddedstream_prompt_blockspath, mirroring whatplatform_supports_streamingalready did for the WebSocket path. Google Chat therefore never attempts a post-then-edit loop. The schema (googlechat.toml) is updated frompartialtonot_implementedwith the rationale, and itssourcerefs point at the real symbols.Why this approach?
Weighed against the alternatives from the research:
google.auth.default()) keyless fallback. Plain ADC forces the node / workload identity to be the Chat bot — it must carrychat.botdirectly. Self-impersonation decouples the runtime identity from the bot identity: the node keeps a minimal identity and is grantedroles/iam.serviceAccountTokenCreatoron a dedicated Chat-bot SA, and we request precisely thechat.botscope viagenerateAccessTokenregardless 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.botis a Workspace scope and is not a subset ofcloud-platform, so no?scopes=parameter on the metadata token can produce it —generateAccessTokenself-impersonation is therefore not merely cleaner here, it is the only viable keyless way to obtain achat.bottoken.Accepted trade-offs (honest costs):
roles/iam.serviceAccountTokenCreatoron 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 = falseby default); the key-file and static-token paths are untouched for everyone else.Alternatives Considered
google.auth.default()), like Hermes Agent's keyless fallback. Simpler, but it makes the node/workload identity be the bot (the attached SA must carrychat.bot). We chosegenerateAccessTokenself-impersonation instead to decouple the runtime identity from the bot identity and request exactly thechat.botscope — see §Why.accessToken. Kept as a fallback, but not a fix: it expires and needs out-of-band refresh.unified_<hex>id is not a valid resource name, sopatchrejects it with400 INVALID_ARGUMENTbefore 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
stablepinned to the CI toolchain (2026-07-13 ≈ 1.97.0):Rust:
cargo clippy --workspace -- -D warnings— cleancargo clippy --workspace --features unified -- -D warnings— cleancargo test -p openab-gatewaypasses, incl.config_first_conformance(every_platform_env_var_has_a_config_section_field)platform-schemaconformance — all foursourcerefs ingooglechat.tomlverified 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):useAdc→GOOGLE_CHAT_USE_ADCenv, plususeAdcin the$hasGoogleChatguard). No existingcharts/openab/tests/*_test.yamlasserts the gateway Google Chat env, so it does not disturb current unittests;helm unittest charts/openabruns on CI (helm not installed locally).Manual:
GOOGLE_CHAT_USE_ADC=trueand no key file: the bot mints achat.bottoken and replies in a space; confirm precedence by additionally settingsaKeyJsonand observing the key path win.Note: my machine's default
stableis 1.98, whose neweruseless_format/doc_lazy_continuationlints flag pre-existing code (e.g.setup/wizard.rs) that the CI toolchain does not; the doc-comment lint on the newresolve_streamingdoc was the one real hit and is fixed in this branch. The unrelatedopenab-coresecrets::tests::resolve_exec_nonzero_exitfailure is a pre-existing, environment-specific subprocess-error-string assertion (that file is not touched by this PR).