fix(googlechat): supported keyless ADC and reliable send-once - #1513
fix(googlechat): supported keyless ADC and reliable send-once#1513chaodu-obk[bot] wants to merge 5 commits into
Conversation
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>
- 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.
Route delete_message with the other unsupported Google Chat commands so it returns before token resolution, logging, or network work instead of falling through to the empty-send response path. Add a regression test that distinguishes the old fallthrough behavior and update the platform schema feature/quirk notes to document the explicit no-op.
Share one non-whitespace token validator across the SA-key, metadata, and
IAM response paths and cover empty/whitespace/valid values in a table test.
Require exact spaces/{space}/messages/{message} edit targets, cover malformed
resource shapes, and assert the valid edit path issues exactly one PATCH.
chaodu-obk
Bot
changed the title
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: #1513 (comment)
GitHub event: COMMENT - self-review delivery only; this is not an approval.
- require a distinct adc_target_service_account for keyless ADC; reject runtime/target equality before requesting the metadata base token because Google prohibits access-token self-impersonation - plumb GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT through config, env, Helm, docs, schema, and config-first conformance; log both runtime and target SAs - classify documented FAILED_PRECONDITION self-impersonation errors - decouple normal-reply acknowledgements from cosmetic streaming so Google Chat remains send-once but carries/awaits request_id; promised ack failures, channel closure, and timeout now fail closed instead of reporting gw_sent - remove unverifiable ADC/static-token identity-equivalence claims and log static fallback as a possible identity switch - add 30s failed-refresh cooldown so queued senders reuse a still-valid token instead of serially repeating metadata/IAM timeouts - add regression tests for distinct-target enforcement, ack error propagation, and refresh retry suppression
chaodu-obk
Bot
changed the title
|
Note LGTM ✅ - All prior findings are resolved at exact head What This PR DoesThis PR delivers supported keyless Google Chat authentication through a runtime service account impersonating a distinct Chat-app service account. It also keeps Google Chat send-once while preserving correlated delivery success and failure, and hardens token validation, fallback diagnostics, command routing, and refresh behavior. How It WorksThe gateway reads the attached runtime service account identity and metadata credential, rejects runtime/target equality before requesting the metadata token, and calls IAM Credentials Findings
Finding DetailsF1: Supported two-identity ADC flowResolved in F2: Send-once with reliable delivery acknowledgementResolved in F3: Explicit fallback identity semanticsResolved in F4: Shared refresh failure cooldownResolved in Addressing Inline Review Feedback
All four inline threads are resolved by the current exact head. No reviewer names or internal coordination details are included here. Baseline Check
Validation
What's Good
5. Three Reasons We Might Not Need This PR
These trade-offs do not introduce a correctness blocker for the current implementation. |
1 participant
What this PR does
Carries #1512 plus focused maintainer fixes. The final design uses two distinct service accounts: the attached runtime SA obtains its metadata credential and impersonates a dedicated Google Chat SA for
chat.bot. Google prohibits access-token self-impersonation, so runtime/target equality is rejected before a base token is requested. Google Chat remains send-once, but normal delivery acknowledgement is independent from cosmetic streaming so API/auth failures reach core.Review Contract
Goal
Deliver keyless Google Chat auth without SA keys using Google's supported runtime-SA -> distinct-Chat-SA impersonation flow; keep send-once rendering while correlating delivery success/failure; and resolve the complete review set (timeouts, token validation, precedence, command guards, docs, logging, backoff, and regression coverage).
Non-goals
Accepted Residual Risks
unclassifiedwith the raw body truncated to 400 chars.Acceptance Criteria
GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT/adc_target_service_account/ HelmadcTargetServiceAccountis required for ADC and propagated end-to-end.generateAccessTokenfor the configured distinct Chat SA; both identities are logged.success=false, promised-channel closure, and promised timeout returnErrto core.check, Review Contract, platform-schema, Helm, standard + unified Docker smoke) is green at the final SHA.Follow-ups
GoogleChatTokenCache/MetadataTokenSource, decideuse_adc: boolvsauth_mode, and consider recorded non-blocking env parsing/retry improvements.Commits
bd62ee49- resolves F15-F28 (timeouts, boundary validation, precedence/docs/readability/operability).904f626c- makes delete an explicit no-op.fa9d58a9- locks shared token and edit-resource boundaries.3a1ce860- replaces prohibited self-impersonation with distinct-target ADC, decouples delivery acknowledgement from streaming, clarifies fallback identity, and adds refresh-failure cooldown.Validation
Exact-head CI is the executable gate. The prior head passed all 45 checks; the final SHA is re-running them. Independent correctness, security, platform-spec, testing, architecture, readability, docs/UX, operability, and simplicity audits will be re-run against the final delta.