From 4aa4d056f8cc30e0d338f5485556dc29f1c91ac8 Mon Sep 17 00:00:00 2001 From: sebastian-hsu Date: Wed, 26 Aug 2026 09:10:20 +0800 Subject: [PATCH 01/12] feat(googlechat): keyless ADC auth + send-once for the unified adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- charts/openab/templates/gateway.yaml | 6 +- charts/openab/values.yaml | 1 + config.toml.example | 1 + crates/openab-core/src/adapter.rs | 47 +- crates/openab-core/src/config.rs | 22 + crates/openab-core/src/gateway.rs | 38 +- .../openab-gateway/src/adapters/googlechat.rs | 451 +++++++++++++++++- crates/openab-gateway/src/lib.rs | 11 + .../tests/config_first_conformance.rs | 1 + docs/config-reference.md | 1 + docs/google-chat.md | 28 +- docs/platforms/schema/googlechat.toml | 14 +- docs/platforms/schema/lineworks.toml | 4 +- src/main.rs | 1 + 14 files changed, 588 insertions(+), 38 deletions(-) diff --git a/charts/openab/templates/gateway.yaml b/charts/openab/templates/gateway.yaml index 2a89dc79a..40454f165 100644 --- a/charts/openab/templates/gateway.yaml +++ b/charts/openab/templates/gateway.yaml @@ -157,10 +157,14 @@ spec: value: "false" {{- end }} {{- end }} - {{- $hasGoogleChat := or (($cfg.gateway).googleChat).saKeyJson (($cfg.gateway).googleChat).accessToken (($cfg.gateway).googleChat).audience }} + {{- $hasGoogleChat := or (($cfg.gateway).googleChat).saKeyJson (($cfg.gateway).googleChat).accessToken (($cfg.gateway).googleChat).audience (($cfg.gateway).googleChat).useAdc }} {{- if $hasGoogleChat }} - name: GOOGLE_CHAT_ENABLED value: "true" + {{- if (($cfg.gateway).googleChat).useAdc }} + - name: GOOGLE_CHAT_USE_ADC + value: "true" + {{- end }} {{- if (($cfg.gateway).googleChat).audience }} - name: GOOGLE_CHAT_AUDIENCE value: {{ ($cfg.gateway).googleChat.audience | quote }} diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index fd37b023c..2258f0e54 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -480,6 +480,7 @@ agents: audience: "" # JWT audience → GOOGLE_CHAT_AUDIENCE (set to your webhook URL to enable JWT verification) saKeyJson: "" # Service account key JSON string → GOOGLE_CHAT_SA_KEY_JSON (recommended, auto-refresh) accessToken: "" # Static OAuth2 access token → GOOGLE_CHAT_ACCESS_TOKEN (fallback, 1-hour TTL) + useAdc: false # Keyless ADC → GOOGLE_CHAT_USE_ADC. Mints chat.bot token from the pod's own GCP identity (GCE metadata + IAM Credentials generateAccessToken). No SA key file. Needs roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com enabled. Ignored when saKeyJson is set. webhookPath: "" # Gateway default: /webhook/googlechat → GOOGLE_CHAT_WEBHOOK_PATH # WeCom (企业微信) adapter config (gateway-side env vars) # See docs/wecom.md for full setup guide diff --git a/config.toml.example b/config.toml.example index 00add1a9b..8c0fddb6c 100644 --- a/config.toml.example +++ b/config.toml.example @@ -123,6 +123,7 @@ allowed_channels = ["1234567890"] # ↑ omitted + non-empty list → auto- # sa_key_json = "${GOOGLE_CHAT_SA_KEY_JSON}" # inline SA key; wins over sa_key_file # sa_key_file = "/etc/openab/sa.json" # env fallback: GOOGLE_CHAT_SA_KEY_FILE # access_token = "${GOOGLE_CHAT_ACCESS_TOKEN}" # static token alternative +# use_adc = true # keyless ADC: mint chat.bot token from the workload's own GCP identity (GCE metadata + IAM Credentials); no SA key. env fallback: GOOGLE_CHAT_USE_ADC. Ignored when sa_key_* is set. # audience = "projects//..." # enables webhook JWT verification (L1) # webhook_path = "/webhook/googlechat" # env fallback: GOOGLE_CHAT_WEBHOOK_PATH # allow_all_users = false # env fallback: GOOGLE_CHAT_ALLOW_ALL_USERS diff --git a/crates/openab-core/src/adapter.rs b/crates/openab-core/src/adapter.rs index fa7e95dba..e69d7cda2 100644 --- a/crates/openab-core/src/adapter.rs +++ b/crates/openab-core/src/adapter.rs @@ -35,6 +35,22 @@ fn reply_message_limit(platform: &str, adapter_limit: usize) -> usize { } } +/// Whether to use cosmetic streaming (placeholder + in-place edits) for +/// `platform`, given the adapter's own preference. Forces send-once for: +/// - `acp`: streams append-only `agent_message_chunk` deltas, not edits. +/// - platforms in `NON_STREAMING_PLATFORMS` — no message-edit API, or an edit +/// API that cannot be driven per-token (e.g. `googlechat`) — see +/// `NON_STREAMING_PLATFORMS` for the per-platform rationale and +/// `platform_supports_streaming`. +/// +/// This is the embedded/unified dispatch gate; the WebSocket +/// `run_gateway_adapter` path applies the same `platform_supports_streaming` check. +fn resolve_streaming(platform: &str, adapter_prefers_streaming: bool) -> bool { + platform != "acp" + && crate::gateway::platform_supports_streaming(platform) + && adapter_prefers_streaming +} + /// Parse `[[key:value]]` directives from the beginning of agent output. /// Returns parsed directives and the remaining content (directives stripped). pub fn parse_output_directives(content: &str) -> (OutputDirectives, String) { @@ -701,15 +717,14 @@ impl AdapterRouter { let adapter = adapter.clone(); let thread_channel = thread_channel.clone(); let message_limit = reply_message_limit(&thread_channel.platform, adapter.message_limit()); - // ACP must not inherit the unified adapter's Telegram streaming flag (wrong - // coupling): it streams append-only `agent_message_chunk` deltas built from the - // post+edit (`edit_message` snapshot) path, i.e. streaming=false. Decide it - // explicitly by platform rather than by whatever Telegram happens to be set to. - let streaming = if thread_channel.platform == "acp" { - false - } else { - adapter.use_streaming(other_bot_present) - }; + // Decide streaming explicitly by platform, not by whatever the unified + // adapter's Telegram flag happens to be. ACP streams append-only deltas + // (not cosmetic edits); Google Chat / LINE can't sustain in-place edits. + // See `resolve_streaming`. + let streaming = resolve_streaming( + &thread_channel.platform, + adapter.use_streaming(other_bot_present), + ); // Keep the full turn text (incl. inter-tool narration) when streaming // (it was already shown live) OR when `[reactions] narration_display` is // set. Otherwise a send-once turn delivers only the final answer block. @@ -1746,6 +1761,20 @@ mod tests { assert_eq!(crate::format::split_message(&long, reply_message_limit("acp", 4096)).len(), 1); } + #[test] + fn resolve_streaming_forces_send_once_for_acp_and_googlechat() { + // Editable platforms honor the adapter's own streaming preference. + assert!(resolve_streaming("discord", true)); + assert!(!resolve_streaming("discord", false)); + assert!(resolve_streaming("telegram", true)); + // ACP streams append-only deltas, not cosmetic edits → always send-once. + assert!(!resolve_streaming("acp", true)); + // Google Chat: synthetic id can't be patched (400 INVALID_ARGUMENT) → send-once regardless of pref. + assert!(!resolve_streaming("googlechat", true)); + // LINE: no edit API → send-once. + assert!(!resolve_streaming("line", true)); + } + #[test] fn select_delivery_text_send_once_keeps_only_final_block() { // Simulates: narration "n1" → tool (answer_start→2) → narration "n2" diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index a9bc26abd..d52f4dce0 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -1225,6 +1225,12 @@ pub struct GoogleChatConfig { /// checked when `allow_all_users` resolves to `false`. Env fallback: /// `GOOGLE_CHAT_ALLOWED_USERS` (comma-separated). pub allowed_users: Option>, + /// Use keyless ADC (GCE metadata server + IAM Credentials + /// `generateAccessToken` self-impersonation) to mint the `chat.bot` token, + /// instead of a SA key file or a static token. Env fallback: + /// `GOOGLE_CHAT_USE_ADC` (`true`/`1`; default false). Ignored when a SA key + /// is configured — the SA key takes precedence. + pub use_adc: Option, } /// Fully resolved Google Chat settings (config → env → default applied). @@ -1234,6 +1240,7 @@ pub struct ResolvedGoogleChat { pub sa_key_json: Option, pub sa_key_file: Option, pub access_token: Option, + pub use_adc: bool, pub audience: Option, pub webhook_path: String, pub allow_all_users: bool, @@ -1259,6 +1266,11 @@ impl GoogleChatConfig { sa_key_json: opt_str(&self.sa_key_json, "GOOGLE_CHAT_SA_KEY_JSON"), sa_key_file: opt_str(&self.sa_key_file, "GOOGLE_CHAT_SA_KEY_FILE"), access_token: opt_str(&self.access_token, "GOOGLE_CHAT_ACCESS_TOKEN"), + use_adc: self.use_adc.unwrap_or_else(|| { + std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v == "true" || v == "1") + .unwrap_or(false) + }), audience: opt_str(&self.audience, "GOOGLE_CHAT_AUDIENCE"), webhook_path: opt_str(&self.webhook_path, "GOOGLE_CHAT_WEBHOOK_PATH") .unwrap_or_else(|| "/webhook/googlechat".into()), @@ -2966,6 +2978,7 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] "GOOGLE_CHAT_SA_KEY_JSON", "GOOGLE_CHAT_SA_KEY_FILE", "GOOGLE_CHAT_ACCESS_TOKEN", + "GOOGLE_CHAT_USE_ADC", "GOOGLE_CHAT_AUDIENCE", "GOOGLE_CHAT_WEBHOOK_PATH", ] { @@ -2974,9 +2987,18 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] // --- defaults --- let r = GoogleChatConfig::default().resolve(); assert!(!r.enabled); + assert!(!r.use_adc); assert!(r.audience.is_none()); assert_eq!(r.webhook_path, "/webhook/googlechat"); + // --- use_adc: config value resolves without touching env --- + let r = GoogleChatConfig { + use_adc: Some(true), + ..Default::default() + } + .resolve(); + assert!(r.use_adc); + // --- config wins over env --- std::env::set_var("GOOGLE_CHAT_ENABLED", "true"); std::env::set_var("GOOGLE_CHAT_AUDIENCE", "env-aud"); diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index a3b74adbd..0a39f0519 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -57,12 +57,23 @@ fn platform_acks_writes(platform: &str) -> bool { /// for a *capability*. The right long-term model is a capability handshake at /// gateway-connect time ("can this adapter edit messages?"); until that exists, /// any new gateway platform that lacks a message-edit API MUST be added here. -const NON_EDITABLE_PLATFORMS: &[&str] = &["line", "lineworks"]; +/// Platforms where cosmetic (typewriter) streaming — a placeholder message +/// then rapid in-place edits — is not viable, so replies are forced send-once: +/// - `line` / `lineworks`: no message-edit API at all. +/// - `googlechat`: has an edit API, but the unified adapter's synthetic +/// `unified_` message id is not a valid resource name, so `patch` +/// rejects it with 400 INVALID_ARGUMENT before any edit applies; the +/// documented 1 write/sec-per-space quota (create + patch + delete +/// combined) further constrains high-frequency editing. +/// See . +const NON_STREAMING_PLATFORMS: &[&str] = &["line", "lineworks", "googlechat"]; /// Whether cosmetic streaming (placeholder + in-place edits) is possible on -/// `platform`. See `NON_EDITABLE_PLATFORMS`. -fn platform_supports_streaming(platform: &str) -> bool { - !NON_EDITABLE_PLATFORMS.contains(&platform) +/// `platform`. See `NON_STREAMING_PLATFORMS`. `pub(crate)` so the shared +/// dispatch path (`AdapterRouter::stream_prompt_blocks`) can force send-once on +/// these platforms too, not just the WebSocket `run_gateway_adapter` path. +pub(crate) fn platform_supports_streaming(platform: &str) -> bool { + !NON_STREAMING_PLATFORMS.contains(&platform) } /// Shared filter parameters for gateway event gating. @@ -1669,17 +1680,18 @@ mod tests { assert!(!platform_supports_streaming("line")); } + #[test] + fn googlechat_rate_limit_forces_send_once() { + // Google Chat has an edit API, but the unified adapter's synthetic + // message id is not a valid resource name, so patch returns 400 + // INVALID_ARGUMENT; the documented 1 write/sec-per-space quota further + // constrains high-frequency editing. Force send-once. + assert!(!platform_supports_streaming("googlechat")); + } + #[test] fn editable_platforms_still_allow_streaming() { - for platform in [ - "telegram", - "slack", - "discord", - "feishu", - "teams", - "googlechat", - "wecom", - ] { + for platform in ["telegram", "slack", "discord", "feishu", "teams", "wecom"] { assert!( platform_supports_streaming(platform), "{platform} should still support streaming", diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 12d274ee4..e7150f7f4 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -263,6 +263,7 @@ impl GoogleChatJwtVerifier { pub struct GoogleChatAdapter { pub token_cache: Option, + pub metadata_source: Option, pub access_token: Option, pub jwt_verifier: Option, pub client: reqwest::Client, @@ -271,15 +272,19 @@ pub struct GoogleChatAdapter { impl GoogleChatAdapter { /// Build an adapter from resolved parts (#1379): SA key JSON (inline wins - /// over file path), optional static access token, optional JWT audience. + /// over file path), optional static access token, optional JWT audience, + /// and `use_adc` (keyless ADC via the GCE metadata server + IAM + /// Credentials). Auth precedence at send time: SA key > ADC > static token. /// Shared by env-derived construction and `apply_googlechat_config`. pub(crate) fn from_parts( sa_key_json: Option, sa_key_file: Option, access_token: Option, audience: Option, + use_adc: bool, ) -> Self { use tracing::{info, warn}; + let key_configured = sa_key_json.is_some() || sa_key_file.is_some(); let token_cache = sa_key_json .or_else(|| { sa_key_file.and_then(|path| { @@ -299,7 +304,24 @@ impl GoogleChatAdapter { info!("googlechat webhook JWT verification enabled (audience={aud})"); GoogleChatJwtVerifier::new(aud) }); - Self::new(token_cache, access_token, jwt_verifier) + // Precedence at send time (see `get_token`): SA key > ADC > static token. + if use_adc { + if key_configured && token_cache.is_none() { + // A key WAS configured but failed to load. Don't switch identity + // silently: name it. get_token still falls to ADC, but the operator + // is told the bot now presents the workload identity, not the key's. + warn!( + "Google Chat SA key was configured but could not be loaded; \ + falling back to the keyless ADC (workload) identity — this is NOT \ + the configured key identity. Fix the key, or unset use_adc." + ); + } else if token_cache.is_none() { + info!("googlechat keyless ADC enabled (chat.bot via IAM Credentials)"); + } + } + let mut adapter = Self::new(token_cache, access_token, jwt_verifier); + adapter.metadata_source = use_adc.then(MetadataTokenSource::new); + adapter } pub fn new( @@ -309,6 +331,7 @@ impl GoogleChatAdapter { ) -> Self { Self { token_cache, + metadata_source: None, access_token, jwt_verifier, client: reqwest::Client::new(), @@ -326,6 +349,24 @@ impl GoogleChatAdapter { } } } + if let Some(ref src) = self.metadata_source { + match src.get_token().await { + Ok(t) => return Some(t), + Err(e) => { + // F2: fall through to a configured static token instead of + // dropping the reply. ADC and the static token are the same + // workload's own credential, so this is not an identity switch. + if self.access_token.is_some() { + error!( + "googlechat ADC token mint failed ({e}); \ + falling back to the configured static access_token" + ); + } else { + error!("googlechat ADC token mint failed: {e}"); + } + } + } + } self.access_token.clone() } @@ -806,21 +847,40 @@ impl GoogleChatTokenCache { { let guard = self.token.read().await; if let Some((ref tok, ref ts, ttl)) = *guard { - if ts.elapsed().as_secs() < ttl.saturating_sub(TOKEN_REFRESH_MARGIN_SECS) { + if ts.elapsed().as_secs() < refresh_threshold(ttl) { return Ok(tok.clone()); } } } let mut guard = self.token.write().await; if let Some((ref tok, ref ts, ttl)) = *guard { - if ts.elapsed().as_secs() < ttl.saturating_sub(TOKEN_REFRESH_MARGIN_SECS) { + if ts.elapsed().as_secs() < refresh_threshold(ttl) { return Ok(tok.clone()); } } - let (new_token, expire) = self.refresh(client).await?; - *guard = Some((new_token.clone(), Instant::now(), expire)); - info!("googlechat access token refreshed (expires in {expire}s)"); - Ok(new_token) + match self.refresh(client).await { + Ok((new_token, expire)) => { + *guard = Some((new_token.clone(), Instant::now(), expire)); + info!("googlechat access token refreshed (expires in {expire}s)"); + Ok(new_token) + } + Err(e) => { + // F4: serve the still-valid cached token on a transient exchange + // failure instead of dropping the reply. + if let Some((ref tok, ref ts, ttl)) = *guard { + let elapsed = ts.elapsed().as_secs(); + if elapsed < ttl { + warn!( + "googlechat token refresh failed ({e}); serving cached token \ + still valid for {}s", + ttl - elapsed + ); + return Ok(tok.clone()); + } + } + Err(e) + } + } } async fn refresh(&self, client: &reqwest::Client) -> Result<(String, u64), String> { @@ -882,6 +942,222 @@ impl GoogleChatTokenCache { } } +// --- Keyless ADC token source (GCE metadata → IAM Credentials) --- + +/// Google Chat's `chat.bot` scope for the minted token. +const ADC_CHAT_BOT_SCOPE: &str = "https://www.googleapis.com/auth/chat.bot"; +/// Lifetime we request from IAM Credentials for the impersonated token, and +/// the fallback TTL we cache it under. IAM caps impersonated tokens at 3600s. +const ADC_TOKEN_LIFETIME_SECS: u64 = 3600; + +/// Cache TTL (seconds) to use for a minted token, derived from the IAM +/// response's `expireTime`. Falls back to the full lifetime when the field is +/// missing/unparseable, and clamps to `[0, ADC_TOKEN_LIFETIME_SECS]` so an +/// org-policy-shortened token isn't cached past its real expiry (0 forces a +/// fresh mint next call rather than serving a dead token). +fn ttl_from_expire_time(expire_time: &str, now: chrono::DateTime) -> u64 { + match chrono::DateTime::parse_from_rfc3339(expire_time) { + Ok(exp) => (exp.with_timezone(&chrono::Utc) - now) + .num_seconds() + .clamp(0, ADC_TOKEN_LIFETIME_SECS as i64) as u64, + Err(_) => ADC_TOKEN_LIFETIME_SECS, + } +} + +/// Age (seconds) at which a cached token must be refreshed: its ttl minus a +/// margin, where the margin is capped at half the ttl. A fixed 300 s margin +/// would make `elapsed < ttl - 300` always false for any `ttl <= 300`, forcing +/// a re-mint on every single send; capping keeps short-lived tokens cacheable. +/// For `ttl = 0` the threshold is 0, so an expired token is never served from +/// the cache. +fn refresh_threshold(ttl: u64) -> u64 { + ttl.saturating_sub(TOKEN_REFRESH_MARGIN_SECS.min(ttl / 2)) +} + +/// Mints a `chat.bot`-scoped access token **without** a service-account key +/// file, using the workload's own identity (keyless ADC). Flow, per refresh: +/// 1. read the default SA's email + a base token from the GCE metadata server +/// 2. call IAM Credentials `generateAccessToken` (self-impersonation) to +/// exchange the base token for a `chat.bot`-scoped token +/// +/// Requires `roles/iam.serviceAccountTokenCreator` on the SA over itself and +/// the `iamcredentials.googleapis.com` API enabled. The `*_base` fields are +/// overridable so tests can point them at a mock server. +pub struct MetadataTokenSource { + token: RwLock>, + // Private (F5): only `new` (prod, fixed trusted hosts) or the in-module + // `with_bases` (tests, mock server) may set these. Unexported ⇒ no in-process + // caller can retarget the metadata bearer to an arbitrary host. + metadata_base: String, + iam_credentials_base: String, + // No-redirect client (F5): a redirect from either endpoint must never carry + // the metadata bearer (`Authorization`) on to a third host. + client: reqwest::Client, +} + +impl Default for MetadataTokenSource { + fn default() -> Self { + Self::new() + } +} + +impl MetadataTokenSource { + /// Production constructor: the fixed, trusted GCP endpoints (IAM over HTTPS). + pub fn new() -> Self { + Self::with_bases( + "http://metadata.google.internal".into(), + "https://iamcredentials.googleapis.com".into(), + ) + } + + /// Construct with explicit endpoint bases. Prod always goes through `new` + /// with HTTPS IAM; tests point these at a mock server. + fn with_bases(metadata_base: String, iam_credentials_base: String) -> Self { + Self { + token: RwLock::new(None), + metadata_base, + iam_credentials_base, + client: reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap_or_default(), + } + } + + /// Cached-or-refresh, mirroring [`GoogleChatTokenCache::get_token`]: + /// double-checked locking around the RwLock so only one refresh runs. + pub async fn get_token(&self) -> Result { + { + let guard = self.token.read().await; + if let Some((ref tok, ref ts, ttl)) = *guard { + if ts.elapsed().as_secs() < refresh_threshold(ttl) { + return Ok(tok.clone()); + } + } + } + let mut guard = self.token.write().await; + if let Some((ref tok, ref ts, ttl)) = *guard { + if ts.elapsed().as_secs() < refresh_threshold(ttl) { + return Ok(tok.clone()); + } + } + match self.refresh().await { + Ok((token, ttl)) => { + if ttl == 0 { + // Freshly minted but already at/after its expireTime — almost + // always local clock skew (Google validates against its own + // clock, so the token may still work). Serve it once, but do + // not cache a dead token: the next call re-mints. + warn!( + "googlechat ADC minted a token with ttl=0 (clock skew?); \ + serving once without caching" + ); + return Ok(token); + } + *guard = Some((token.clone(), Instant::now(), ttl)); + info!("googlechat ADC token minted (chat.bot, ttl {ttl}s)"); + Ok(token) + } + Err(e) => { + // F4: during a transient metadata/IAM failure, serve the cached + // token while it is still valid rather than dropping the reply. + if let Some((ref tok, ref ts, ttl)) = *guard { + let elapsed = ts.elapsed().as_secs(); + if elapsed < ttl { + warn!( + "googlechat ADC refresh failed ({e}); serving cached token \ + still valid for {}s", + ttl - elapsed + ); + return Ok(tok.clone()); + } + } + Err(e) + } + } + } + + async fn refresh(&self) -> Result<(String, u64), String> { + // Use the source's own no-redirect client for every bearer-carrying call. + let client = &self.client; + // 1. Default SA email from the GCE metadata server. + let email = client + .get(format!( + "{}/computeMetadata/v1/instance/service-accounts/default/email", + self.metadata_base + )) + .header("Metadata-Flavor", "Google") + .send() + .await + .map_err(|e| format!("metadata email request failed: {e}"))? + .error_for_status() + .map_err(|e| format!("metadata email status: {e}"))? + .text() + .await + .map_err(|e| format!("metadata email read failed: {e}"))?; + let email = email.trim(); + if email.is_empty() { + return Err("metadata returned empty SA email".into()); + } + + // 2. Base access token for the default SA from the metadata server. + let base: serde_json::Value = client + .get(format!( + "{}/computeMetadata/v1/instance/service-accounts/default/token", + self.metadata_base + )) + .header("Metadata-Flavor", "Google") + .send() + .await + .map_err(|e| format!("metadata token request failed: {e}"))? + .error_for_status() + .map_err(|e| format!("metadata token status: {e}"))? + .json() + .await + .map_err(|e| format!("metadata token parse failed: {e}"))?; + let base_token = base + .get("access_token") + .and_then(|v| v.as_str()) + .ok_or("metadata token response missing access_token")?; + + // 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", + self.iam_credentials_base + ); + let resp: serde_json::Value = client + .post(&url) + .bearer_auth(base_token) + .json(&serde_json::json!({ + "scope": [ADC_CHAT_BOT_SCOPE], + "lifetime": format!("{ADC_TOKEN_LIFETIME_SECS}s"), + })) + .send() + .await + .map_err(|e| format!("generateAccessToken request failed: {e}"))? + .error_for_status() + .map_err(|e| format!("generateAccessToken status: {e}"))? + .json() + .await + .map_err(|e| format!("generateAccessToken parse failed: {e}"))?; + let token = resp + .get("accessToken") + .and_then(|v| v.as_str()) + .ok_or("generateAccessToken response missing accessToken")? + .to_string(); + // Cache under the server-granted lifetime (respects an org policy that + // shortens impersonated tokens below the requested 3600s), falling back + // to the full lifetime when expireTime is absent/unparseable. + let ttl = resp + .get("expireTime") + .and_then(|v| v.as_str()) + .map(|e| ttl_from_expire_time(e, chrono::Utc::now())) + .unwrap_or(ADC_TOKEN_LIFETIME_SECS); + Ok((token, ttl)) + } +} + /// Convert markdown to Google Chat native formatting. /// /// Called by both `send_message` and `edit_message`. Assumes the caller passes @@ -1811,6 +2087,165 @@ mod tests { assert!(result.is_ok()); } + // --- Keyless ADC (MetadataTokenSource) tests --- + + #[test] + fn ttl_from_expire_time_derives_and_clamps() { + use chrono::{DateTime, Utc}; + let now: DateTime = "2026-08-25T00:00:00Z".parse().unwrap(); + // Normal: 30 min out → 1800s. + assert_eq!(ttl_from_expire_time("2026-08-25T00:30:00Z", now), 1800); + // Beyond the 3600s cap → clamped to 3600. + assert_eq!(ttl_from_expire_time("2026-08-25T05:00:00Z", now), 3600); + // Already expired → 0 (forces refresh next call, never caches a dead token). + assert_eq!(ttl_from_expire_time("2026-08-24T23:00:00Z", now), 0); + // Unparseable → safe fallback to the full lifetime. + assert_eq!(ttl_from_expire_time("not-a-timestamp", now), 3600); + } + + #[tokio::test] + async fn metadata_token_source_mints_chat_bot_token() { + use wiremock::matchers::{header, method, path, path_regex}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + // GCE metadata: default SA email. + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .and(header("Metadata-Flavor", "Google")) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("openab-host@dev-seba.iam.gserviceaccount.com"), + ) + .mount(&server) + .await; + // GCE metadata: base access token. + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .and(header("Metadata-Flavor", "Google")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "base-tok", + "expires_in": 3600, + "token_type": "Bearer" + }))) + .mount(&server) + .await; + // IAM Credentials: generateAccessToken (self-impersonation) → chat.bot token. + Mock::given(method("POST")) + .and(path_regex( + r"/v1/projects/-/serviceAccounts/.*:generateAccessToken", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "accessToken": "chat-bot-tok", + "expireTime": "2099-01-01T00:00:00Z" + }))) + .mount(&server) + .await; + + let src = MetadataTokenSource::with_bases(server.uri(), server.uri()); + + let token = src + .get_token() + .await + .expect("should mint a chat.bot token"); + assert_eq!(token, "chat-bot-tok"); + } + + #[test] + fn from_parts_use_adc_toggles_metadata_source() { + let with = GoogleChatAdapter::from_parts(None, None, None, None, true); + assert!(with.metadata_source.is_some(), "use_adc=true → ADC source"); + let without = GoogleChatAdapter::from_parts(None, None, None, None, false); + assert!( + without.metadata_source.is_none(), + "use_adc=false → no ADC source" + ); + } + + #[test] + fn from_parts_malformed_key_with_use_adc_installs_adc() { + // F1 regression: a configured-but-malformed SA key parses to + // token_cache=None; with use_adc=true the ADC source is still installed + // (from_parts logs a warning naming the identity switch). + let adapter = + GoogleChatAdapter::from_parts(Some("not valid json".into()), None, None, None, true); + assert!( + adapter.token_cache.is_none(), + "malformed SA key → no SA-key cache" + ); + assert!( + adapter.metadata_source.is_some(), + "use_adc=true → ADC source installed even when a key was configured but failed to load" + ); + } + + #[test] + fn from_parts_unreadable_key_file_with_use_adc_installs_adc() { + // F1 regression: an unreadable/absent key FILE also yields token_cache=None + // and must not suppress ADC. + let adapter = GoogleChatAdapter::from_parts( + None, + Some("/nonexistent/path/sa-key.json".into()), + None, + None, + true, + ); + assert!(adapter.token_cache.is_none(), "unreadable key file → no cache"); + assert!( + adapter.metadata_source.is_some(), + "use_adc=true → ADC source installed when the key file could not be read" + ); + } + + #[tokio::test] + async fn adc_takes_precedence_over_static_access_token() { + use wiremock::matchers::{method, path, path_regex}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("openab-host@dev-seba.iam.gserviceaccount.com"), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "base-tok", "expires_in": 3600 + }))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path_regex( + r"/v1/projects/-/serviceAccounts/.*:generateAccessToken", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "accessToken": "chat-bot-tok" + }))) + .mount(&server) + .await; + + // Adapter has BOTH an ADC source and a static token; ADC must win. + let mut adapter = + GoogleChatAdapter::from_parts(None, None, Some("static-tok".into()), None, true); + // Repoint the ADC source at the mock server (bases are private now). + adapter.metadata_source = + Some(MetadataTokenSource::with_bases(server.uri(), server.uri())); + let token = adapter.get_token().await.expect("a token"); + assert_eq!(token, "chat-bot-tok", "ADC should win over static token"); + } + // --- Bot filtering logic test --- #[test] diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index d2b257c7d..7f5c7bf26 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -199,6 +199,9 @@ impl AppState { std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), + std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v == "true" || v == "1") + .unwrap_or(false), )) } else { None @@ -454,6 +457,7 @@ impl AppState { cfg.sa_key_file, cfg.access_token, cfg.audience, + cfg.use_adc, )) } else { None @@ -554,6 +558,7 @@ pub struct GatewayGoogleChatConfig { pub sa_key_file: Option, pub access_token: Option, pub audience: Option, + pub use_adc: bool, pub webhook_path: String, } @@ -752,6 +757,9 @@ pub async fn serve(config: ServeConfig) -> anyhow::Result<()> { std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), + std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v == "true" || v == "1") + .unwrap_or(false), )) } else { None @@ -1249,6 +1257,7 @@ mod l1_audit_tests { sa_key_file: None, access_token: Some("tok".into()), audience: None, + use_adc: false, webhook_path: "/hook/gc".into(), }); assert!(s.google_chat.is_some()); @@ -1262,6 +1271,7 @@ mod l1_audit_tests { sa_key_file: None, access_token: Some("tok".into()), audience: Some("aud".into()), + use_adc: false, webhook_path: "/hook/gc".into(), }); assert!(flagged(&s).is_empty()); @@ -1273,6 +1283,7 @@ mod l1_audit_tests { sa_key_file: None, access_token: None, audience: None, + use_adc: false, webhook_path: "/hook/gc".into(), }); assert!(s.google_chat.is_none()); diff --git a/crates/openab-gateway/tests/config_first_conformance.rs b/crates/openab-gateway/tests/config_first_conformance.rs index eeefc8d8c..4c8f19877 100644 --- a/crates/openab-gateway/tests/config_first_conformance.rs +++ b/crates/openab-gateway/tests/config_first_conformance.rs @@ -92,6 +92,7 @@ const COVERED: &[&str] = &[ "GOOGLE_CHAT_SA_KEY_JSON", "GOOGLE_CHAT_SA_KEY_FILE", "GOOGLE_CHAT_ACCESS_TOKEN", + "GOOGLE_CHAT_USE_ADC", "GOOGLE_CHAT_AUDIENCE", "GOOGLE_CHAT_WEBHOOK_PATH", "GOOGLE_CHAT_ALLOW_ALL_USERS", diff --git a/docs/config-reference.md b/docs/config-reference.md index af30e0940..13d863c82 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -225,6 +225,7 @@ Full first-class Google Chat section (config-first parity, #1379) — credential | `sa_key_json` | string | — | Inline service-account key JSON (wins over `sa_key_file`). Env: `GOOGLE_CHAT_SA_KEY_JSON`. | | `sa_key_file` | string | — | Path to a service-account key file. Env: `GOOGLE_CHAT_SA_KEY_FILE`. | | `access_token` | string | — | Static access token alternative. Env: `GOOGLE_CHAT_ACCESS_TOKEN`. | +| `use_adc` | bool | `false` | Keyless ADC — mint the `chat.bot` token from the workload's own GCP identity (GCE metadata + IAM Credentials `generateAccessToken` self-impersonation); no SA key file. Needs `roles/iam.serviceAccountTokenCreator` on the SA over itself + `iamcredentials.googleapis.com`. Ignored when a SA key is set. Env: `GOOGLE_CHAT_USE_ADC`. | | `audience` | string | — | JWT audience — enables webhook JWT verification (L1). Env: `GOOGLE_CHAT_AUDIENCE`. | | `webhook_path` | string | `/webhook/googlechat` | Env: `GOOGLE_CHAT_WEBHOOK_PATH`. | | `allow_all_users` | bool \| omit | `false` (deny-all) | Env: `GOOGLE_CHAT_ALLOW_ALL_USERS`. | diff --git a/docs/google-chat.md b/docs/google-chat.md index dcee33d0e..6cb2f1ee8 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -74,7 +74,7 @@ Google Chat uses a service account to authenticate outbound API calls (bot repli ## 3. Configure the Gateway -The gateway supports two authentication methods for sending replies: +The gateway supports three authentication methods for sending replies: ### Option A: Service Account Key (recommended — auto-refresh) @@ -112,6 +112,29 @@ docker run -d --name openab-gateway \ ghcr.io/openabdev/openab-gateway:latest ``` +### Option C: Keyless ADC (recommended on GCP — no key file) + +When the gateway runs on GCP (GKE / GCE / Cloud Run) **as** a service account, it can mint the `chat.bot` token from that identity — no service-account key file to mount, manage, or leak. The gateway reads a base token from the GCE metadata server and calls IAM Credentials `generateAccessToken` (the SA impersonates itself) for a `chat.bot`-scoped token, then caches and auto-refreshes it. + +Prerequisites: + +- The workload's service account **is** the Chat app's service account (the identity that sends must be the space member). +- Grant that SA `roles/iam.serviceAccountTokenCreator` **on itself**. +- Enable `iamcredentials.googleapis.com`. +- The **base token from the metadata server must carry `cloud-platform` (or `.../auth/iam`) scope** — `generateAccessToken` requires it on the caller. GKE Workload Identity and Cloud Run tokens are `cloud-platform`-scoped and satisfy this automatically. A **default-scope GCE VM does not**: it returns `403 PERMISSION_DENIED: "Request had insufficient authentication scopes."` even when the IAM binding above is correct — match on the word *scopes* (not *permission*) to tell it apart from a missing role. GCE access scopes are **immutable after creation**: create the VM with `--scopes=cloud-platform`, or run `gcloud compute instances set-scopes --scopes=cloud-platform` followed by a stop/start. + +> Why self-impersonation (not plain ADC): the `chat.bot` scope is a Workspace scope and is **not** a subset of `cloud-platform`, so no `?scopes=` parameter on the metadata token can produce it. `generateAccessToken` is the only keyless way to obtain a `chat.bot` token. + +```bash +# The pod/VM already runs as the target service account; no key is mounted. +export GOOGLE_CHAT_ENABLED=true +export GOOGLE_CHAT_USE_ADC=true +``` + +Precedence: if a SA key (`GOOGLE_CHAT_SA_KEY_JSON` / `GOOGLE_CHAT_SA_KEY_FILE`) is also set **and loads successfully**, the SA key wins and ADC is ignored. If a key is configured but **fails to load** (unreadable file / malformed JSON), the adapter falls back to the keyless ADC (workload) identity and logs a warning naming the switch — fix the key, or unset `use_adc` to make that failure explicit. + +> **Migrating an existing release from a SA key to ADC:** the chart renders the Google Chat Secret only when `saKeyJson` / `accessToken` is set, and that Secret carries `helm.sh/resource-policy: keep`. Switching to ADC-only stops Helm from managing it but **leaves the old key material in the cluster indefinitely**. Delete the orphaned Secret after the switch (`kubectl delete secret `), otherwise the "no key to mount or leak" benefit is undercut. + ### Local development ```bash @@ -199,7 +222,7 @@ Each field falls back to its `GOOGLE_CHAT_ALLOW_ALL_USERS` / `GOOGLE_CHAT_ALLOWE - Links: `[text](url)` → `` - Inline code, fenced code blocks: pass through unchanged - Tables and other unsupported syntax pass through as-is -- **Streaming (edit_message)** — when OAB streaming is enabled, the bot edits its initial reply in-place as tokens arrive (typewriter effect) +- **Send-once (no streaming)** — Google Chat is a request/response REST surface, so the adapter posts the full reply once with no in-place editing. It is in `NON_STREAMING_PLATFORMS`; see `docs/platforms/schema/googlechat.toml` for why (the unified adapter's synthetic message id is not a valid resource name → `patch` returns `400 INVALID_ARGUMENT`, and the API documents a 1 write/sec-per-space quota). - **Inbound attachments** — image, text file, and audio attachments are downloaded via Google Chat Media API and stored to `~/.openab/media/inbound/` (colocate filesystem store): - Images: resized to ≤1200px JPEG (q75); GIFs preserved. Max 10 MB. - Text files: only known text extensions (`.txt`, `.md`, `.json`, `.py`, `.rs`, etc.). Max 512 KB. @@ -221,6 +244,7 @@ Each field falls back to its `GOOGLE_CHAT_ALLOW_ALL_USERS` / `GOOGLE_CHAT_ALLOWE | `GOOGLE_CHAT_SA_KEY_JSON` | No | — | Service account key JSON string (enables auto-refresh) | | `GOOGLE_CHAT_SA_KEY_FILE` | No | — | Path to service account key JSON file (alternative to `SA_KEY_JSON`) | | `GOOGLE_CHAT_ACCESS_TOKEN` | No | — | Static OAuth2 access token (fallback, expires in 1 hour) | +| `GOOGLE_CHAT_USE_ADC` | No | `false` | Keyless ADC auth via the GCE metadata server + IAM Credentials `generateAccessToken` self-impersonation (GCP-hosted only) — see Option C. Ignored when `SA_KEY_JSON`/`SA_KEY_FILE` is set | | `GOOGLE_CHAT_WEBHOOK_PATH` | No | `/webhook/googlechat` | Webhook endpoint path | ## Security: Webhook Verification diff --git a/docs/platforms/schema/googlechat.toml b/docs/platforms/schema/googlechat.toml index 375e62663..797a5f5e7 100644 --- a/docs/platforms/schema/googlechat.toml +++ b/docs/platforms/schema/googlechat.toml @@ -131,9 +131,9 @@ pr = "" [[openab_features]] feature = "streaming" -status = "partial" -note = "No native streaming API. Gateway adapter leaves uses_native_streaming=false, so core streams via the post-then-edit_message loop; each edit sends the full accumulated text as a patch call." -source = ["crates/openab-gateway/src/adapters/googlechat.rs#edit_message", "crates/openab-core/src/adapter.rs#uses_native_streaming"] +status = "not_implemented" +note = "Send-once by design (no cosmetic streaming). The decisive reason is structural: the unified adapter returns a synthetic `unified_` message id that is not a valid resource name, so `spaces.messages.patch` rejects it with 400 INVALID_ARGUMENT ('Missing or malformed message resource name') before any content is applied — per-token post-then-edit cannot work at all. Separately, Google Chat documents a 1 write/sec-per-space quota (create+patch+delete combined; https://developers.google.com/workspace/chat/limits); treat that as a documented constraint on high-frequency editing rather than an observed hard failure, since enforcement is burst-tolerant in practice. googlechat is therefore in NON_STREAMING_PLATFORMS, and `resolve_streaming` forces send-once on BOTH the embedded dispatch and the WebSocket gateway paths — matching Google Chat's documented send-once default. (Previously 'partial': core attempted post-then-edit, which failed on every edit.)" +source = ["crates/openab-core/src/gateway.rs#NON_STREAMING_PLATFORMS", "crates/openab-core/src/adapter.rs#resolve_streaming", "crates/openab-core/src/adapter.rs#uses_native_streaming"] pr = "" [[openab_features]] @@ -245,6 +245,14 @@ kind = "openab_decision" source = "crates/openab-gateway/src/adapters/googlechat.rs#build_jwt" refs = [] +[[quirks]] +date = "2026-08-25" +title = "Keyless ADC outbound path (no SA key)" +note = "Outbound creds have a third option beside the SA-key JWT-bearer exchange and the static token: keyless ADC (use_adc / GOOGLE_CHAT_USE_ADC). MetadataTokenSource reads the default SA email + a base token from the GCE metadata server, then calls IAM Credentials generateAccessToken (the SA impersonates itself) for a chat.bot-scoped token, cached with the same 300 s refresh margin. Meant for workloads that already run as the Chat app's service account on GCP — no key file to mount or leak. Requires roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com. Auth precedence in get_token: SA key (token_cache) > ADC (metadata_source) > static access_token." +kind = "openab_decision" +source = "crates/openab-gateway/src/adapters/googlechat.rs#MetadataTokenSource" +refs = [] + [[quirks]] date = "2026-07-04" title = "Reactions are structurally impossible for the bot" diff --git a/docs/platforms/schema/lineworks.toml b/docs/platforms/schema/lineworks.toml index 066944446..9b6cd013e 100644 --- a/docs/platforms/schema/lineworks.toml +++ b/docs/platforms/schema/lineworks.toml @@ -132,8 +132,8 @@ pr = "" [[openab_features]] feature = "streaming" status = "n_a" -note = "No edit API to drive post+edit streaming. The platform is listed in NON_EDITABLE_PLATFORMS so the core forces streaming off and the cosmetic edit/delete commands are dropped by the dispatcher." -source = ["crates/openab-core/src/gateway.rs#NON_EDITABLE_PLATFORMS", "crates/openab-gateway/src/adapters/lineworks.rs#dispatch_lineworks_reply"] +note = "No edit API to drive post+edit streaming. The platform is listed in NON_STREAMING_PLATFORMS so the core forces streaming off and the cosmetic edit/delete commands are dropped by the dispatcher." +source = ["crates/openab-core/src/gateway.rs#NON_STREAMING_PLATFORMS", "crates/openab-gateway/src/adapters/lineworks.rs#dispatch_lineworks_reply"] pr = "" [[openab_features]] diff --git a/src/main.rs b/src/main.rs index a2ee786ac..03f86b748 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1284,6 +1284,7 @@ async fn main() -> anyhow::Result<()> { sa_key_json: r.sa_key_json, sa_key_file: r.sa_key_file, access_token: r.access_token, + use_adc: r.use_adc, audience: r.audience, webhook_path: r.webhook_path, }); From bd62ee49a56e0f50ea3e9cd57b9badc2a901c414 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 19:59:32 +0000 Subject: [PATCH 02/12] fix(googlechat): address review findings F15-F28 - 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. --- charts/openab/values.yaml | 2 +- config.toml.example | 2 +- crates/openab-core/src/adapter.rs | 5 +- crates/openab-core/src/config.rs | 6 +- crates/openab-core/src/gateway.rs | 6 + .../openab-gateway/src/adapters/googlechat.rs | 333 ++++++++++++++++-- crates/openab-gateway/src/lib.rs | 44 ++- docs/config-reference.md | 2 +- docs/google-chat.md | 6 +- 9 files changed, 339 insertions(+), 67 deletions(-) diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index 2258f0e54..0d7cee8d2 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -480,7 +480,7 @@ agents: audience: "" # JWT audience → GOOGLE_CHAT_AUDIENCE (set to your webhook URL to enable JWT verification) saKeyJson: "" # Service account key JSON string → GOOGLE_CHAT_SA_KEY_JSON (recommended, auto-refresh) accessToken: "" # Static OAuth2 access token → GOOGLE_CHAT_ACCESS_TOKEN (fallback, 1-hour TTL) - useAdc: false # Keyless ADC → GOOGLE_CHAT_USE_ADC. Mints chat.bot token from the pod's own GCP identity (GCE metadata + IAM Credentials generateAccessToken). No SA key file. Needs roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com enabled. Ignored when saKeyJson is set. + useAdc: false # Keyless ADC → GOOGLE_CHAT_USE_ADC. Mints chat.bot token from the pod's own GCP identity (GCE metadata + IAM Credentials generateAccessToken). No SA key file. Needs roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com enabled. Ignored when saKeyJson is set and loads; a key that fails to load falls back to ADC with a warning (see docs/google-chat.md Option C). webhookPath: "" # Gateway default: /webhook/googlechat → GOOGLE_CHAT_WEBHOOK_PATH # WeCom (企业微信) adapter config (gateway-side env vars) # See docs/wecom.md for full setup guide diff --git a/config.toml.example b/config.toml.example index 8c0fddb6c..a1de58cbe 100644 --- a/config.toml.example +++ b/config.toml.example @@ -123,7 +123,7 @@ allowed_channels = ["1234567890"] # ↑ omitted + non-empty list → auto- # sa_key_json = "${GOOGLE_CHAT_SA_KEY_JSON}" # inline SA key; wins over sa_key_file # sa_key_file = "/etc/openab/sa.json" # env fallback: GOOGLE_CHAT_SA_KEY_FILE # access_token = "${GOOGLE_CHAT_ACCESS_TOKEN}" # static token alternative -# use_adc = true # keyless ADC: mint chat.bot token from the workload's own GCP identity (GCE metadata + IAM Credentials); no SA key. env fallback: GOOGLE_CHAT_USE_ADC. Ignored when sa_key_* is set. +# use_adc = true # keyless ADC: mint chat.bot token from the workload's own GCP identity (GCE metadata + IAM Credentials); no SA key. env fallback: GOOGLE_CHAT_USE_ADC. Ignored when a configured sa_key_* loads; a key that fails to load falls back to ADC with a warning (docs/google-chat.md Option C). # audience = "projects//..." # enables webhook JWT verification (L1) # webhook_path = "/webhook/googlechat" # env fallback: GOOGLE_CHAT_WEBHOOK_PATH # allow_all_users = false # env fallback: GOOGLE_CHAT_ALLOW_ALL_USERS diff --git a/crates/openab-core/src/adapter.rs b/crates/openab-core/src/adapter.rs index e69d7cda2..2bf3a7c6d 100644 --- a/crates/openab-core/src/adapter.rs +++ b/crates/openab-core/src/adapter.rs @@ -44,7 +44,10 @@ fn reply_message_limit(platform: &str, adapter_limit: usize) -> usize { /// `platform_supports_streaming`. /// /// This is the embedded/unified dispatch gate; the WebSocket -/// `run_gateway_adapter` path applies the same `platform_supports_streaming` check. +/// `run_gateway_adapter` path applies the same `platform_supports_streaming` +/// check but has no `acp` case (ACP is embedded-only). The two gates are +/// siblings: adding a platform to `NON_STREAMING_PLATFORMS` covers both +/// paths, while an embedded-only carve-out belongs here. fn resolve_streaming(platform: &str, adapter_prefers_streaming: bool) -> bool { platform != "acp" && crate::gateway::platform_supports_streaming(platform) diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index d52f4dce0..eedce548a 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -1228,8 +1228,10 @@ pub struct GoogleChatConfig { /// Use keyless ADC (GCE metadata server + IAM Credentials /// `generateAccessToken` self-impersonation) to mint the `chat.bot` token, /// instead of a SA key file or a static token. Env fallback: - /// `GOOGLE_CHAT_USE_ADC` (`true`/`1`; default false). Ignored when a SA key - /// is configured — the SA key takes precedence. + /// `GOOGLE_CHAT_USE_ADC` (`true`/`1`; default false). Ignored when a + /// configured SA key loads successfully — the SA key takes precedence; a + /// key that fails to load falls back to ADC with a warning naming the + /// identity switch (see docs/google-chat.md Option C). pub use_adc: Option, } diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index 0a39f0519..970100edf 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -72,6 +72,12 @@ const NON_STREAMING_PLATFORMS: &[&str] = &["line", "lineworks", "googlechat"]; /// `platform`. See `NON_STREAMING_PLATFORMS`. `pub(crate)` so the shared /// dispatch path (`AdapterRouter::stream_prompt_blocks`) can force send-once on /// these platforms too, not just the WebSocket `run_gateway_adapter` path. +/// +/// Sibling gate: the embedded/unified dispatch path wraps this in +/// `adapter::resolve_streaming`, which additionally forces send-once for +/// `acp` (embedded-only, streams append-only deltas). An embedded-only +/// non-streaming platform must be handled there — adding it to the shared +/// list above covers both paths, but a platform-specific carve-out does not. pub(crate) fn platform_supports_streaming(platform: &str) -> bool { !NON_STREAMING_PLATFORMS.contains(&platform) } diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index e7150f7f4..c3385911b 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -21,6 +21,12 @@ const AUDIO_MAX_DOWNLOAD: u64 = 25 * 1024 * 1024; // 25 MB /// Per-request timeout for Google Chat Media API downloads. Prevents a hung /// connection from blocking the spawned download task indefinitely. const MEDIA_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); + +/// Bound every token-mint request (SA-key exchange, metadata, IAM Credentials) +/// so a hung connection cannot stall senders queued behind the token cache's +/// write lock (the refresh runs while holding it) or prevent the ADC → static +/// token degradation path from engaging. +const TOKEN_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); /// Cap on text file attachments per message (matches Discord/Slack). const TEXT_FILE_COUNT_CAP: usize = 5; /// Cap on aggregate text file bytes per message (matches Discord/Slack 1 MB). @@ -270,20 +276,32 @@ pub struct GoogleChatAdapter { pub api_base: String, } +/// Named construction parts for [`GoogleChatAdapter::from_parts`], so call +/// sites name each field instead of counting five positional arguments. +#[derive(Default)] +pub(crate) struct GoogleChatParts { + pub sa_key_json: Option, + pub sa_key_file: Option, + pub access_token: Option, + pub audience: Option, + pub use_adc: bool, +} + impl GoogleChatAdapter { /// Build an adapter from resolved parts (#1379): SA key JSON (inline wins /// over file path), optional static access token, optional JWT audience, /// and `use_adc` (keyless ADC via the GCE metadata server + IAM /// Credentials). Auth precedence at send time: SA key > ADC > static token. /// Shared by env-derived construction and `apply_googlechat_config`. - pub(crate) fn from_parts( - sa_key_json: Option, - sa_key_file: Option, - access_token: Option, - audience: Option, - use_adc: bool, - ) -> Self { + pub(crate) fn from_parts(parts: GoogleChatParts) -> Self { use tracing::{info, warn}; + let GoogleChatParts { + sa_key_json, + sa_key_file, + access_token, + audience, + use_adc, + } = parts; let key_configured = sa_key_json.is_some() || sa_key_file.is_some(); let token_cache = sa_key_json .or_else(|| { @@ -320,7 +338,12 @@ impl GoogleChatAdapter { } } let mut adapter = Self::new(token_cache, access_token, jwt_verifier); - adapter.metadata_source = use_adc.then(MetadataTokenSource::new); + // Install ADC only when it can actually be consulted: a loaded SA key + // wins outright at send time (see `get_token`), so don't construct a + // dead-at-runtime source behind it. A configured key that FAILED to + // load still installs ADC — that is the named fallback warned above. + adapter.metadata_source = + (use_adc && adapter.token_cache.is_none()).then(MetadataTokenSource::new); adapter } @@ -339,6 +362,14 @@ impl GoogleChatAdapter { } } + /// Resolve the outbound bearer token. Precedence: SA key (`token_cache`) + /// > ADC (`metadata_source`) > static `access_token`. + /// + /// Failure behavior is asymmetric by design: an SA-key exchange error + /// hard-fails (`None` — the operator explicitly configured that identity, + /// so nothing is silently substituted), while an ADC mint error falls + /// through to the static token (both are the same workload's credential, + /// so degrading is not an identity switch). async fn get_token(&self) -> Option { if let Some(ref cache) = self.token_cache { match cache.get_token(&self.client).await { @@ -353,7 +384,7 @@ impl GoogleChatAdapter { match src.get_token().await { Ok(t) => return Some(t), Err(e) => { - // F2: fall through to a configured static token instead of + // Fall through to a configured static token instead of // dropping the reply. ADC and the static token are the same // workload's own credential, so this is not an identity switch. if self.access_token.is_some() { @@ -407,6 +438,20 @@ impl GoogleChatAdapter { match reply.command.as_deref() { Some("add_reaction") | Some("remove_reaction") | Some("create_topic") => return, Some("edit_message") => { + // Google Chat is send-once (see core's `NON_STREAMING_PLATFORMS`): + // the unified adapter's synthetic `unified_` id is not a + // valid `spaces/*/messages/*` resource name, and `patch` rejects + // it with 400 INVALID_ARGUMENT before any edit applies. Refuse + // non-resource-name ids here instead of sending a doomed request, + // so a future caller cannot silently reintroduce that failure. + if !reply.reply_to.starts_with("spaces/") { + tracing::warn!( + reply_to = %reply.reply_to, + "googlechat edit_message ignored: not a message resource name \ + (synthetic ids cannot be patched)" + ); + return; + } self.edit_message(&reply.reply_to, &reply.content.text).await; return; } @@ -865,7 +910,7 @@ impl GoogleChatTokenCache { Ok(new_token) } Err(e) => { - // F4: serve the still-valid cached token on a transient exchange + // Serve the still-valid cached token on a transient exchange // failure instead of dropping the reply. if let Some((ref tok, ref ts, ttl)) = *guard { let elapsed = ts.elapsed().as_secs(); @@ -887,6 +932,7 @@ impl GoogleChatTokenCache { let jwt = self.build_jwt().map_err(|e| format!("JWT build error: {e}"))?; let resp = client .post("https://oauth2.googleapis.com/token") + .timeout(TOKEN_REQUEST_TIMEOUT) .form(&[ ("grant_type", "urn:ietf:params:oauth:grant-type:jwt-bearer"), ("assertion", &jwt), @@ -903,12 +949,15 @@ impl GoogleChatTokenCache { let token = body .get("access_token") .and_then(|v| v.as_str()) + // Boundary validation: an empty token would be cached as "valid" + // and fail every send with 401 until the refresh threshold. + .filter(|s| !s.trim().is_empty()) .ok_or_else(|| { let err = body .get("error_description") .and_then(|v| v.as_str()) .unwrap_or("unknown error"); - format!("token exchange failed: {err}") + format!("token exchange returned missing/empty access_token: {err}") })? .to_string(); @@ -985,12 +1034,12 @@ fn refresh_threshold(ttl: u64) -> u64 { /// overridable so tests can point them at a mock server. pub struct MetadataTokenSource { token: RwLock>, - // Private (F5): only `new` (prod, fixed trusted hosts) or the in-module + // Private: only `new` (prod, fixed trusted hosts) or the in-module // `with_bases` (tests, mock server) may set these. Unexported ⇒ no in-process // caller can retarget the metadata bearer to an arbitrary host. metadata_base: String, iam_credentials_base: String, - // No-redirect client (F5): a redirect from either endpoint must never carry + // No-redirect client: a redirect from either endpoint must never carry // the metadata bearer (`Authorization`) on to a third host. client: reqwest::Client, } @@ -1042,24 +1091,31 @@ impl MetadataTokenSource { } } match self.refresh().await { - Ok((token, ttl)) => { + Ok(minted) => { + let MintedToken { token, ttl, sa_email } = minted; if ttl == 0 { // Freshly minted but already at/after its expireTime — almost // always local clock skew (Google validates against its own // clock, so the token may still work). Serve it once, but do // not cache a dead token: the next call re-mints. warn!( + service_account = %sa_email, "googlechat ADC minted a token with ttl=0 (clock skew?); \ serving once without caching" ); return Ok(token); } *guard = Some((token.clone(), Instant::now(), ttl)); - info!("googlechat ADC token minted (chat.bot, ttl {ttl}s)"); + // Name the resolved identity so on-call can confirm which SA + // self-impersonation actually used when diagnosing send errors. + info!( + service_account = %sa_email, + "googlechat ADC token minted (chat.bot, ttl {ttl}s)" + ); Ok(token) } Err(e) => { - // F4: during a transient metadata/IAM failure, serve the cached + // During a transient metadata/IAM failure, serve the cached // token while it is still valid rather than dropping the reply. if let Some((ref tok, ref ts, ttl)) = *guard { let elapsed = ts.elapsed().as_secs(); @@ -1077,7 +1133,7 @@ impl MetadataTokenSource { } } - async fn refresh(&self) -> Result<(String, u64), String> { + async fn refresh(&self) -> Result { // Use the source's own no-redirect client for every bearer-carrying call. let client = &self.client; // 1. Default SA email from the GCE metadata server. @@ -1087,6 +1143,7 @@ impl MetadataTokenSource { self.metadata_base )) .header("Metadata-Flavor", "Google") + .timeout(TOKEN_REQUEST_TIMEOUT) .send() .await .map_err(|e| format!("metadata email request failed: {e}"))? @@ -1107,6 +1164,7 @@ impl MetadataTokenSource { self.metadata_base )) .header("Metadata-Flavor", "Google") + .timeout(TOKEN_REQUEST_TIMEOUT) .send() .await .map_err(|e| format!("metadata token request failed: {e}"))? @@ -1118,7 +1176,8 @@ impl MetadataTokenSource { let base_token = base .get("access_token") .and_then(|v| v.as_str()) - .ok_or("metadata token response missing access_token")?; + .filter(|s| !s.trim().is_empty()) + .ok_or("metadata token response missing or empty access_token")?; // 3. Exchange the base token for a chat.bot-scoped token via IAM // Credentials generateAccessToken (the SA impersonates itself). @@ -1126,25 +1185,45 @@ impl MetadataTokenSource { "{}/v1/projects/-/serviceAccounts/{email}:generateAccessToken", self.iam_credentials_base ); - let resp: serde_json::Value = client + let resp = client .post(&url) .bearer_auth(base_token) .json(&serde_json::json!({ "scope": [ADC_CHAT_BOT_SCOPE], "lifetime": format!("{ADC_TOKEN_LIFETIME_SECS}s"), })) + .timeout(TOKEN_REQUEST_TIMEOUT) .send() .await - .map_err(|e| format!("generateAccessToken request failed: {e}"))? - .error_for_status() - .map_err(|e| format!("generateAccessToken status: {e}"))? + .map_err(|e| format!("generateAccessToken request failed: {e}"))?; + let status = resp.status(); + if !status.is_success() { + // Classify the common GCP causes so on-call can act on the log + // line directly instead of decoding GCP error prose (mirrors the + // operator guidance in docs/google-chat.md Option C). + let body: String = resp + .text() + .await + .unwrap_or_default() + .chars() + .take(400) + .collect(); + let reason = classify_generate_access_token_error(status.as_u16(), &body); + return Err(format!( + "generateAccessToken failed (status {status}, reason={reason}): {body}" + )); + } + let resp: serde_json::Value = resp .json() .await .map_err(|e| format!("generateAccessToken parse failed: {e}"))?; let token = resp .get("accessToken") .and_then(|v| v.as_str()) - .ok_or("generateAccessToken response missing accessToken")? + // Boundary validation: an empty token would be cached as "valid" + // for its full TTL and bypass the static-token degradation path. + .filter(|s| !s.trim().is_empty()) + .ok_or("generateAccessToken response missing or empty accessToken")? .to_string(); // Cache under the server-granted lifetime (respects an org policy that // shortens impersonated tokens below the requested 3600s), falling back @@ -1154,7 +1233,37 @@ impl MetadataTokenSource { .and_then(|v| v.as_str()) .map(|e| ttl_from_expire_time(e, chrono::Utc::now())) .unwrap_or(ADC_TOKEN_LIFETIME_SECS); - Ok((token, ttl)) + Ok(MintedToken { + token, + ttl, + sa_email: email.to_string(), + }) + } +} + +/// A successfully minted ADC token plus the identity that minted it, so the +/// caller can log which service account self-impersonation resolved to. +struct MintedToken { + token: String, + ttl: u64, + sa_email: String, +} + +/// Best-effort classification of common GCP `generateAccessToken` failures. +/// Ordering matters: the insufficient-scope 403 body says "scopes" (not +/// "permission"), which is the documented signal distinguishing it from a +/// missing `roles/iam.serviceAccountTokenCreator` binding. +fn classify_generate_access_token_error(status: u16, body: &str) -> &'static str { + let b = body.to_ascii_lowercase(); + if b.contains("api has not been used") || b.contains("service_disabled") || b.contains("is disabled") + { + "api_not_enabled" + } else if status == 403 && b.contains("scopes") { + "insufficient_scope" + } else if status == 403 { + "missing_role" + } else { + "unclassified" } } @@ -2157,9 +2266,12 @@ mod tests { #[test] fn from_parts_use_adc_toggles_metadata_source() { - let with = GoogleChatAdapter::from_parts(None, None, None, None, true); + let with = GoogleChatAdapter::from_parts(GoogleChatParts { + use_adc: true, + ..Default::default() + }); assert!(with.metadata_source.is_some(), "use_adc=true → ADC source"); - let without = GoogleChatAdapter::from_parts(None, None, None, None, false); + let without = GoogleChatAdapter::from_parts(GoogleChatParts::default()); assert!( without.metadata_source.is_none(), "use_adc=false → no ADC source" @@ -2168,11 +2280,14 @@ mod tests { #[test] fn from_parts_malformed_key_with_use_adc_installs_adc() { - // F1 regression: a configured-but-malformed SA key parses to + // Regression: a configured-but-malformed SA key parses to // token_cache=None; with use_adc=true the ADC source is still installed // (from_parts logs a warning naming the identity switch). - let adapter = - GoogleChatAdapter::from_parts(Some("not valid json".into()), None, None, None, true); + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + sa_key_json: Some("not valid json".into()), + use_adc: true, + ..Default::default() + }); assert!( adapter.token_cache.is_none(), "malformed SA key → no SA-key cache" @@ -2185,15 +2300,13 @@ mod tests { #[test] fn from_parts_unreadable_key_file_with_use_adc_installs_adc() { - // F1 regression: an unreadable/absent key FILE also yields token_cache=None + // Regression: an unreadable/absent key FILE also yields token_cache=None // and must not suppress ADC. - let adapter = GoogleChatAdapter::from_parts( - None, - Some("/nonexistent/path/sa-key.json".into()), - None, - None, - true, - ); + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + sa_key_file: Some("/nonexistent/path/sa-key.json".into()), + use_adc: true, + ..Default::default() + }); assert!(adapter.token_cache.is_none(), "unreadable key file → no cache"); assert!( adapter.metadata_source.is_some(), @@ -2201,6 +2314,58 @@ mod tests { ); } + #[test] + fn from_parts_loaded_key_suppresses_metadata_source() { + // A successfully loaded SA key wins outright at send time, so no ADC + // source is installed behind it (dead-at-runtime otherwise). + let key = serde_json::json!({ + "client_email": "sa@example.iam.gserviceaccount.com", + "private_key": "-----BEGIN PRIVATE KEY-----\nnot-a-real-key\n-----END PRIVATE KEY-----\n", + }) + .to_string(); + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + sa_key_json: Some(key), + use_adc: true, + ..Default::default() + }); + assert!(adapter.token_cache.is_some(), "valid key JSON → SA-key cache"); + assert!( + adapter.metadata_source.is_none(), + "loaded SA key → ADC source not installed" + ); + } + + #[test] + fn classify_generate_access_token_error_covers_documented_cases() { + // Insufficient scope: the 403 body says "scopes" — the documented + // signal distinguishing it from a missing IAM role. + assert_eq!( + classify_generate_access_token_error( + 403, + r#"{"error":{"status":"PERMISSION_DENIED","message":"Request had insufficient authentication scopes."}}"# + ), + "insufficient_scope" + ); + // Missing serviceAccountTokenCreator: 403 without the scopes wording. + assert_eq!( + classify_generate_access_token_error( + 403, + r#"{"error":{"status":"IAM_PERMISSION_DENIED","message":"Permission 'iam.serviceAccounts.getAccessToken' denied on resource"}}"# + ), + "missing_role" + ); + // IAM Credentials API not enabled. + assert_eq!( + classify_generate_access_token_error( + 403, + r#"{"error":{"message":"IAM Service Account Credentials API has not been used in project 123 before or it is disabled."}}"# + ), + "api_not_enabled" + ); + // Anything else stays unclassified rather than guessing. + assert_eq!(classify_generate_access_token_error(500, "boom"), "unclassified"); + } + #[tokio::test] async fn adc_takes_precedence_over_static_access_token() { use wiremock::matchers::{method, path, path_regex}; @@ -2237,8 +2402,11 @@ mod tests { .await; // Adapter has BOTH an ADC source and a static token; ADC must win. - let mut adapter = - GoogleChatAdapter::from_parts(None, None, Some("static-tok".into()), None, true); + let mut adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + access_token: Some("static-tok".into()), + use_adc: true, + ..Default::default() + }); // Repoint the ADC source at the mock server (bases are private now). adapter.metadata_source = Some(MetadataTokenSource::with_bases(server.uri(), server.uri())); @@ -2246,6 +2414,93 @@ mod tests { assert_eq!(token, "chat-bot-tok", "ADC should win over static token"); } + #[tokio::test] + async fn metadata_token_source_rejects_blank_minted_token() { + use wiremock::matchers::{method, path, path_regex}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("openab-host@dev-seba.iam.gserviceaccount.com"), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "base-tok", "expires_in": 3600 + }))) + .mount(&server) + .await; + // A malformed IAM response with an empty accessToken must surface as a + // mint error (and thus follow the static-token degradation path), not + // be cached as a "valid" credential. + Mock::given(method("POST")) + .and(path_regex( + r"/v1/projects/-/serviceAccounts/.*:generateAccessToken", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "accessToken": "" + }))) + .mount(&server) + .await; + + let src = MetadataTokenSource::with_bases(server.uri(), server.uri()); + let err = src.get_token().await.expect_err("blank token must be rejected"); + assert!( + err.contains("missing or empty accessToken"), + "unexpected error: {err}" + ); + } + + #[tokio::test] + async fn handle_reply_edit_message_ignores_synthetic_unified_id() { + use wiremock::matchers::method; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + // Expect ZERO requests: a synthetic `unified_` id is not a valid + // message resource name, so the edit must be refused locally instead + // of being sent to the API (which would 400 INVALID_ARGUMENT). + let mock_server = MockServer::start().await; + Mock::given(method("PATCH")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&mock_server) + .await; + + let (event_tx, _event_rx) = tokio::sync::broadcast::channel::(16); + let mut adapter = GoogleChatAdapter::new(None, Some("fake-token".into()), None); + adapter.api_base = mock_server.uri(); + + let reply = GatewayReply { + schema: "openab.gateway.reply.v1".into(), + reply_to: "unified_a1b2c3d4e5f6".into(), + platform: "googlechat".into(), + channel: ReplyChannel { + id: "spaces/SP".into(), + thread_id: None, + }, + content: Content { + content_type: "text".into(), + attachments: Vec::new(), + text: "updated text".into(), + }, + command: Some("edit_message".into()), + request_id: None, + quote_message_id: None, + }; + + adapter.handle_reply(&reply, &event_tx).await; + // MockServer verifies the expect(0) on drop. + } + // --- Bot filtering logic test --- #[test] diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index 7f5c7bf26..92799eaa2 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -195,13 +195,15 @@ impl AppState { .unwrap_or(false); if enabled { Some(adapters::googlechat::GoogleChatAdapter::from_parts( - std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), - std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), - std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), - std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), - std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v == "true" || v == "1") - .unwrap_or(false), + adapters::googlechat::GoogleChatParts { + sa_key_json: std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), + sa_key_file: std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), + access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), + audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), + use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v == "true" || v == "1") + .unwrap_or(false), + }, )) } else { None @@ -453,11 +455,13 @@ impl AppState { self.googlechat_webhook_path = cfg.webhook_path; self.google_chat = if cfg.enabled { Some(adapters::googlechat::GoogleChatAdapter::from_parts( - cfg.sa_key_json, - cfg.sa_key_file, - cfg.access_token, - cfg.audience, - cfg.use_adc, + adapters::googlechat::GoogleChatParts { + sa_key_json: cfg.sa_key_json, + sa_key_file: cfg.sa_key_file, + access_token: cfg.access_token, + audience: cfg.audience, + use_adc: cfg.use_adc, + }, )) } else { None @@ -753,13 +757,15 @@ pub async fn serve(config: ServeConfig) -> anyhow::Result<()> { info!(path = %googlechat_webhook_path, "googlechat adapter enabled"); app = app.route(&googlechat_webhook_path, post(adapters::googlechat::webhook)); Some(adapters::googlechat::GoogleChatAdapter::from_parts( - std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), - std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), - std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), - std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), - std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v == "true" || v == "1") - .unwrap_or(false), + adapters::googlechat::GoogleChatParts { + sa_key_json: std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), + sa_key_file: std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), + access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), + audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), + use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v == "true" || v == "1") + .unwrap_or(false), + }, )) } else { None diff --git a/docs/config-reference.md b/docs/config-reference.md index 13d863c82..a8820d88d 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -225,7 +225,7 @@ Full first-class Google Chat section (config-first parity, #1379) — credential | `sa_key_json` | string | — | Inline service-account key JSON (wins over `sa_key_file`). Env: `GOOGLE_CHAT_SA_KEY_JSON`. | | `sa_key_file` | string | — | Path to a service-account key file. Env: `GOOGLE_CHAT_SA_KEY_FILE`. | | `access_token` | string | — | Static access token alternative. Env: `GOOGLE_CHAT_ACCESS_TOKEN`. | -| `use_adc` | bool | `false` | Keyless ADC — mint the `chat.bot` token from the workload's own GCP identity (GCE metadata + IAM Credentials `generateAccessToken` self-impersonation); no SA key file. Needs `roles/iam.serviceAccountTokenCreator` on the SA over itself + `iamcredentials.googleapis.com`. Ignored when a SA key is set. Env: `GOOGLE_CHAT_USE_ADC`. | +| `use_adc` | bool | `false` | Keyless ADC — mint the `chat.bot` token from the workload's own GCP identity (GCE metadata + IAM Credentials `generateAccessToken` self-impersonation); no SA key file. Needs `roles/iam.serviceAccountTokenCreator` on the SA over itself + `iamcredentials.googleapis.com`. Ignored when a SA key is set and loads successfully; a configured key that fails to load falls back to ADC with a warning (see `docs/google-chat.md` Option C). Env: `GOOGLE_CHAT_USE_ADC`. | | `audience` | string | — | JWT audience — enables webhook JWT verification (L1). Env: `GOOGLE_CHAT_AUDIENCE`. | | `webhook_path` | string | `/webhook/googlechat` | Env: `GOOGLE_CHAT_WEBHOOK_PATH`. | | `allow_all_users` | bool \| omit | `false` (deny-all) | Env: `GOOGLE_CHAT_ALLOW_ALL_USERS`. | diff --git a/docs/google-chat.md b/docs/google-chat.md index 6cb2f1ee8..1823562d7 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -121,7 +121,7 @@ Prerequisites: - The workload's service account **is** the Chat app's service account (the identity that sends must be the space member). - Grant that SA `roles/iam.serviceAccountTokenCreator` **on itself**. - Enable `iamcredentials.googleapis.com`. -- The **base token from the metadata server must carry `cloud-platform` (or `.../auth/iam`) scope** — `generateAccessToken` requires it on the caller. GKE Workload Identity and Cloud Run tokens are `cloud-platform`-scoped and satisfy this automatically. A **default-scope GCE VM does not**: it returns `403 PERMISSION_DENIED: "Request had insufficient authentication scopes."` even when the IAM binding above is correct — match on the word *scopes* (not *permission*) to tell it apart from a missing role. GCE access scopes are **immutable after creation**: create the VM with `--scopes=cloud-platform`, or run `gcloud compute instances set-scopes --scopes=cloud-platform` followed by a stop/start. +- The **base token from the metadata server must carry `cloud-platform` (or `.../auth/iam`) scope** — `generateAccessToken` requires it on the caller. GKE Workload Identity and Cloud Run tokens are `cloud-platform`-scoped and satisfy this automatically. A **default-scope GCE VM does not**: it returns `403 PERMISSION_DENIED: "Request had insufficient authentication scopes."` even when the IAM binding above is correct — match on the word *scopes* (not *permission*) to tell it apart from a missing role. GCE access scopes **cannot be changed while the VM is running**: create the VM with `--scopes=cloud-platform`, or run `gcloud compute instances set-scopes --scopes=cloud-platform` followed by a stop/start. > Why self-impersonation (not plain ADC): the `chat.bot` scope is a Workspace scope and is **not** a subset of `cloud-platform`, so no `?scopes=` parameter on the metadata token can produce it. `generateAccessToken` is the only keyless way to obtain a `chat.bot` token. @@ -133,7 +133,7 @@ export GOOGLE_CHAT_USE_ADC=true Precedence: if a SA key (`GOOGLE_CHAT_SA_KEY_JSON` / `GOOGLE_CHAT_SA_KEY_FILE`) is also set **and loads successfully**, the SA key wins and ADC is ignored. If a key is configured but **fails to load** (unreadable file / malformed JSON), the adapter falls back to the keyless ADC (workload) identity and logs a warning naming the switch — fix the key, or unset `use_adc` to make that failure explicit. -> **Migrating an existing release from a SA key to ADC:** the chart renders the Google Chat Secret only when `saKeyJson` / `accessToken` is set, and that Secret carries `helm.sh/resource-policy: keep`. Switching to ADC-only stops Helm from managing it but **leaves the old key material in the cluster indefinitely**. Delete the orphaned Secret after the switch (`kubectl delete secret `), otherwise the "no key to mount or leak" benefit is undercut. +> **Migrating an existing release from a SA key to ADC:** the chart renders the Google Chat Secret only when `saKeyJson` / `accessToken` is set, and that Secret carries `helm.sh/resource-policy: keep`. Switching to ADC-only stops Helm from managing it but **leaves the old key material in the cluster indefinitely**. Delete the orphaned Secret after the switch: it is the gateway Secret named by the chart's `openab.agentFullname` helper — `--gateway` by default (or the agent's `nameOverride`) — and it contains the `google-chat-sa-key-json` key. Find it with `kubectl get secrets -o name | grep gateway`, confirm with `kubectl get secret -o jsonpath='{.data}' | grep -o google-chat-sa-key-json`, then `kubectl delete secret `. Otherwise the "no key to mount or leak" benefit is undercut. ### Local development @@ -244,7 +244,7 @@ Each field falls back to its `GOOGLE_CHAT_ALLOW_ALL_USERS` / `GOOGLE_CHAT_ALLOWE | `GOOGLE_CHAT_SA_KEY_JSON` | No | — | Service account key JSON string (enables auto-refresh) | | `GOOGLE_CHAT_SA_KEY_FILE` | No | — | Path to service account key JSON file (alternative to `SA_KEY_JSON`) | | `GOOGLE_CHAT_ACCESS_TOKEN` | No | — | Static OAuth2 access token (fallback, expires in 1 hour) | -| `GOOGLE_CHAT_USE_ADC` | No | `false` | Keyless ADC auth via the GCE metadata server + IAM Credentials `generateAccessToken` self-impersonation (GCP-hosted only) — see Option C. Ignored when `SA_KEY_JSON`/`SA_KEY_FILE` is set | +| `GOOGLE_CHAT_USE_ADC` | No | `false` | Keyless ADC auth via the GCE metadata server + IAM Credentials `generateAccessToken` self-impersonation (GCP-hosted only) — see Option C. Ignored when `SA_KEY_JSON`/`SA_KEY_FILE` is set and loads; a configured key that fails to load falls back to ADC with a warning (Option C) | | `GOOGLE_CHAT_WEBHOOK_PATH` | No | `/webhook/googlechat` | Webhook endpoint path | ## Security: Webhook Verification From 904f626cc9d29dd04dc545d86e1c496b3372d273 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:40:17 +0000 Subject: [PATCH 03/12] fix(googlechat): make delete_message an explicit no-op 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. --- .../openab-gateway/src/adapters/googlechat.rs | 40 ++++++++++++++++++- docs/platforms/schema/googlechat.toml | 6 +-- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index c3385911b..0d1cfca15 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -436,7 +436,13 @@ impl GoogleChatAdapter { ) { // Command routing match reply.command.as_deref() { - Some("add_reaction") | Some("remove_reaction") | Some("create_topic") => return, + // Google Chat does not support these gateway commands. Return before + // token resolution or send-path logging/network I/O; in particular, + // `delete_message` must not fall through as an empty send. + Some("add_reaction") + | Some("remove_reaction") + | Some("create_topic") + | Some("delete_message") => return, Some("edit_message") => { // Google Chat is send-once (see core's `NON_STREAMING_PLATFORMS`): // the unified adapter's synthetic `unified_` id is not a @@ -2501,6 +2507,38 @@ mod tests { // MockServer verifies the expect(0) on drop. } + #[tokio::test] + async fn handle_reply_delete_message_is_explicit_noop() { + let (event_tx, mut event_rx) = tokio::sync::broadcast::channel::(16); + let adapter = GoogleChatAdapter::new(None, Some("fake-token".into()), None); + let reply = GatewayReply { + schema: "openab.gateway.reply.v1".into(), + reply_to: "spaces/SP/messages/msg1".into(), + platform: "googlechat".into(), + channel: ReplyChannel { + id: "spaces/SP".into(), + thread_id: None, + }, + content: Content { + content_type: "text".into(), + attachments: Vec::new(), + text: String::new(), + }, + command: Some("delete_message".into()), + // Under the old fallthrough behavior this request id produced an + // "empty message" GatewayResponse. Explicit command routing emits + // no response and performs no token/network work. + request_id: Some("req_delete".into()), + quote_message_id: None, + }; + + adapter.handle_reply(&reply, &event_tx).await; + assert!( + event_rx.try_recv().is_err(), + "delete_message must return before the empty-send response path" + ); + } + // --- Bot filtering logic test --- #[test] diff --git a/docs/platforms/schema/googlechat.toml b/docs/platforms/schema/googlechat.toml index 797a5f5e7..07285ff7e 100644 --- a/docs/platforms/schema/googlechat.toml +++ b/docs/platforms/schema/googlechat.toml @@ -153,7 +153,7 @@ pr = "" [[openab_features]] feature = "delete_message" status = "not_implemented" -note = "GatewayAdapter overrides delete_message to emit a fire-and-forget command:\"delete_message\" instead of the trait default (edit-to-zero-width). The googlechat adapter does not match delete_message in handle_reply (only add_reaction/remove_reaction/create_topic/edit_message), so it falls through to the send path with empty text → hits the empty-message short-circuit and sends nothing. Net: delete is a no-op on Google Chat." +note = "GatewayAdapter emits a fire-and-forget command:\"delete_message\" instead of the trait default (edit-to-zero-width). The googlechat adapter explicitly matches delete_message with the other unsupported commands and returns before token resolution, logging, or network I/O. Net: delete is an intentional no-op on Google Chat." source = ["crates/openab-gateway/src/adapters/googlechat.rs#handle_reply", "crates/openab-core/src/gateway.rs#delete_message", "crates/openab-core/src/adapter.rs#delete_message"] pr = "" @@ -295,8 +295,8 @@ refs = [] [[quirks]] date = "2026-07-04" -title = "Delete is a silent no-op (not even the edit fallback)" -note = "Unlike platforms where delete_message falls back to the trait's edit-to-zero-width, on Google Chat the delete_message command isn't matched in handle_reply, falls through to the send path with empty text, and hits the empty-message short-circuit — so nothing is sent and no edit occurs. Streaming-placeholder cleanup that relies on delete is therefore a no-op here." +title = "Delete is an explicit no-op (not even the edit fallback)" +note = "Unlike platforms where delete_message falls back to the trait's edit-to-zero-width, Google Chat matches the delete_message command in handle_reply and returns before token resolution, logging, or network I/O. Streaming-placeholder cleanup that relies on delete is therefore an intentional no-op here." kind = "openab_decision" source = "crates/openab-gateway/src/adapters/googlechat.rs#handle_reply" refs = [] From fa9d58a9e55a38f509f7cd3c7bd56c14de4f01a2 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:50:59 +0000 Subject: [PATCH 04/12] test(googlechat): lock token and edit boundaries 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. --- .../openab-gateway/src/adapters/googlechat.rs | 53 +++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 0d1cfca15..30d8df70a 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -265,6 +265,32 @@ impl GoogleChatJwtVerifier { } } +/// Accept an external token value only when it contains non-whitespace bytes. +/// All three OAuth boundaries (SA-key exchange, metadata base token, IAM +/// Credentials mint) share this validator so their failure semantics cannot +/// drift independently. +fn non_empty_token(value: &str) -> Option<&str> { + (!value.trim().is_empty()).then_some(value) +} + +/// Google Chat edit targets must be full `spaces/{space}/messages/{message}` +/// resource names. Reject synthetic ids and incomplete `spaces/` prefixes +/// before token resolution or network I/O. +fn is_google_chat_message_name(name: &str) -> bool { + let mut parts = name.split('/'); + matches!( + ( + parts.next(), + parts.next(), + parts.next(), + parts.next(), + parts.next(), + ), + (Some("spaces"), Some(space), Some("messages"), Some(message), None) + if !space.is_empty() && !message.is_empty() + ) +} + // --- Adapter (encapsulates all Google Chat state) --- pub struct GoogleChatAdapter { @@ -450,7 +476,7 @@ impl GoogleChatAdapter { // it with 400 INVALID_ARGUMENT before any edit applies. Refuse // non-resource-name ids here instead of sending a doomed request, // so a future caller cannot silently reintroduce that failure. - if !reply.reply_to.starts_with("spaces/") { + if !is_google_chat_message_name(&reply.reply_to) { tracing::warn!( reply_to = %reply.reply_to, "googlechat edit_message ignored: not a message resource name \ @@ -957,7 +983,7 @@ impl GoogleChatTokenCache { .and_then(|v| v.as_str()) // Boundary validation: an empty token would be cached as "valid" // and fail every send with 401 until the refresh threshold. - .filter(|s| !s.trim().is_empty()) + .and_then(non_empty_token) .ok_or_else(|| { let err = body .get("error_description") @@ -1182,7 +1208,7 @@ impl MetadataTokenSource { let base_token = base .get("access_token") .and_then(|v| v.as_str()) - .filter(|s| !s.trim().is_empty()) + .and_then(non_empty_token) .ok_or("metadata token response missing or empty access_token")?; // 3. Exchange the base token for a chat.bot-scoped token via IAM @@ -1228,7 +1254,7 @@ impl MetadataTokenSource { .and_then(|v| v.as_str()) // Boundary validation: an empty token would be cached as "valid" // for its full TTL and bypass the static-token degradation path. - .filter(|s| !s.trim().is_empty()) + .and_then(non_empty_token) .ok_or("generateAccessToken response missing or empty accessToken")? .to_string(); // Cache under the server-granted lifetime (respects an org policy that @@ -2372,6 +2398,24 @@ mod tests { assert_eq!(classify_generate_access_token_error(500, "boom"), "unclassified"); } + #[test] + fn external_token_values_must_be_non_whitespace() { + assert_eq!(None::<&str>.and_then(non_empty_token), None); + assert_eq!(non_empty_token(""), None); + assert_eq!(non_empty_token(" \t\n"), None); + assert_eq!(non_empty_token(" token "), Some(" token ")); + } + + #[test] + fn edit_targets_require_full_message_resource_names() { + assert!(is_google_chat_message_name("spaces/SP/messages/msg1")); + assert!(!is_google_chat_message_name("unified_a1b2c3d4")); + assert!(!is_google_chat_message_name("spaces/")); + assert!(!is_google_chat_message_name("spaces/SP")); + assert!(!is_google_chat_message_name("spaces/SP/messages/")); + assert!(!is_google_chat_message_name("spaces/SP/messages/msg1/extra")); + } + #[tokio::test] async fn adc_takes_precedence_over_static_access_token() { use wiremock::matchers::{method, path, path_regex}; @@ -2974,6 +3018,7 @@ mod tests { .respond_with(ResponseTemplate::new(200).set_body_json( serde_json::json!({"name": "spaces/SP/messages/msg1"}), )) + .expect(1) .mount(&mock_server) .await; From 3a1ce8601cb9092893642a4537b8c17badc7bf67 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:32:28 +0000 Subject: [PATCH 05/12] fix(googlechat): use supported ADC impersonation and delivery acks - 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 --- charts/openab/templates/gateway.yaml | 2 + charts/openab/values.yaml | 3 +- config.toml.example | 3 +- crates/openab-core/src/config.rs | 27 +- crates/openab-core/src/gateway.rs | 110 ++++-- .../openab-gateway/src/adapters/googlechat.rs | 318 ++++++++++++++---- crates/openab-gateway/src/lib.rs | 15 + .../tests/config_first_conformance.rs | 1 + docs/config-reference.md | 3 +- docs/google-chat.md | 20 +- docs/platforms/schema/googlechat.toml | 2 +- src/main.rs | 1 + 12 files changed, 395 insertions(+), 110 deletions(-) diff --git a/charts/openab/templates/gateway.yaml b/charts/openab/templates/gateway.yaml index 40454f165..3482d4cc5 100644 --- a/charts/openab/templates/gateway.yaml +++ b/charts/openab/templates/gateway.yaml @@ -164,6 +164,8 @@ spec: {{- if (($cfg.gateway).googleChat).useAdc }} - name: GOOGLE_CHAT_USE_ADC value: "true" + - name: GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT + value: {{ required "gateway.googleChat.adcTargetServiceAccount is required when useAdc=true" (($cfg.gateway).googleChat).adcTargetServiceAccount | quote }} {{- end }} {{- if (($cfg.gateway).googleChat).audience }} - name: GOOGLE_CHAT_AUDIENCE diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index 0d7cee8d2..78c3e695f 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -480,7 +480,8 @@ agents: audience: "" # JWT audience → GOOGLE_CHAT_AUDIENCE (set to your webhook URL to enable JWT verification) saKeyJson: "" # Service account key JSON string → GOOGLE_CHAT_SA_KEY_JSON (recommended, auto-refresh) accessToken: "" # Static OAuth2 access token → GOOGLE_CHAT_ACCESS_TOKEN (fallback, 1-hour TTL) - useAdc: false # Keyless ADC → GOOGLE_CHAT_USE_ADC. Mints chat.bot token from the pod's own GCP identity (GCE metadata + IAM Credentials generateAccessToken). No SA key file. Needs roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com enabled. Ignored when saKeyJson is set and loads; a key that fails to load falls back to ADC with a warning (see docs/google-chat.md Option C). + useAdc: false # Keyless ADC → GOOGLE_CHAT_USE_ADC. Runtime SA impersonates the distinct adcTargetServiceAccount; no SA key file. Needs roles/iam.serviceAccountTokenCreator on the target + iamcredentials.googleapis.com. Self-impersonation is prohibited. + adcTargetServiceAccount: "" # Dedicated Chat-app SA email → GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT. Required when useAdc=true; MUST differ from the pod/runtime SA. webhookPath: "" # Gateway default: /webhook/googlechat → GOOGLE_CHAT_WEBHOOK_PATH # WeCom (企业微信) adapter config (gateway-side env vars) # See docs/wecom.md for full setup guide diff --git a/config.toml.example b/config.toml.example index a1de58cbe..e12ce4b9b 100644 --- a/config.toml.example +++ b/config.toml.example @@ -123,7 +123,8 @@ allowed_channels = ["1234567890"] # ↑ omitted + non-empty list → auto- # sa_key_json = "${GOOGLE_CHAT_SA_KEY_JSON}" # inline SA key; wins over sa_key_file # sa_key_file = "/etc/openab/sa.json" # env fallback: GOOGLE_CHAT_SA_KEY_FILE # access_token = "${GOOGLE_CHAT_ACCESS_TOKEN}" # static token alternative -# use_adc = true # keyless ADC: mint chat.bot token from the workload's own GCP identity (GCE metadata + IAM Credentials); no SA key. env fallback: GOOGLE_CHAT_USE_ADC. Ignored when a configured sa_key_* loads; a key that fails to load falls back to ADC with a warning (docs/google-chat.md Option C). +# use_adc = true # keyless ADC via GCE metadata + IAM Credentials; runtime SA impersonates a DISTINCT target (self-impersonation is prohibited). env: GOOGLE_CHAT_USE_ADC +# adc_target_service_account = "chat-bot@project.iam.gserviceaccount.com" # required with use_adc; env: GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT # audience = "projects//..." # enables webhook JWT verification (L1) # webhook_path = "/webhook/googlechat" # env fallback: GOOGLE_CHAT_WEBHOOK_PATH # allow_all_users = false # env fallback: GOOGLE_CHAT_ALLOW_ALL_USERS diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index eedce548a..0894e7ef1 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -1226,13 +1226,15 @@ pub struct GoogleChatConfig { /// `GOOGLE_CHAT_ALLOWED_USERS` (comma-separated). pub allowed_users: Option>, /// Use keyless ADC (GCE metadata server + IAM Credentials - /// `generateAccessToken` self-impersonation) to mint the `chat.bot` token, - /// instead of a SA key file or a static token. Env fallback: - /// `GOOGLE_CHAT_USE_ADC` (`true`/`1`; default false). Ignored when a - /// configured SA key loads successfully — the SA key takes precedence; a - /// key that fails to load falls back to ADC with a warning naming the - /// identity switch (see docs/google-chat.md Option C). + /// `generateAccessToken`) to mint the `chat.bot` token for a distinct target + /// service account. Env fallback: `GOOGLE_CHAT_USE_ADC` (`true`/`1`; default + /// false). Ignored when a configured SA key loads successfully. pub use_adc: Option, + /// Dedicated Google Chat service account impersonated by the attached + /// runtime service account. Required when `use_adc=true`; MUST differ from + /// the runtime identity because Google prohibits access-token + /// self-impersonation. Env: `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT`. + pub adc_target_service_account: Option, } /// Fully resolved Google Chat settings (config → env → default applied). @@ -1243,6 +1245,7 @@ pub struct ResolvedGoogleChat { pub sa_key_file: Option, pub access_token: Option, pub use_adc: bool, + pub adc_target_service_account: Option, pub audience: Option, pub webhook_path: String, pub allow_all_users: bool, @@ -1273,6 +1276,10 @@ impl GoogleChatConfig { .map(|v| v == "true" || v == "1") .unwrap_or(false) }), + adc_target_service_account: opt_str( + &self.adc_target_service_account, + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", + ), audience: opt_str(&self.audience, "GOOGLE_CHAT_AUDIENCE"), webhook_path: opt_str(&self.webhook_path, "GOOGLE_CHAT_WEBHOOK_PATH") .unwrap_or_else(|| "/webhook/googlechat".into()), @@ -2981,6 +2988,7 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] "GOOGLE_CHAT_SA_KEY_FILE", "GOOGLE_CHAT_ACCESS_TOKEN", "GOOGLE_CHAT_USE_ADC", + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", "GOOGLE_CHAT_AUDIENCE", "GOOGLE_CHAT_WEBHOOK_PATH", ] { @@ -2996,10 +3004,17 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] // --- use_adc: config value resolves without touching env --- let r = GoogleChatConfig { use_adc: Some(true), + adc_target_service_account: Some( + "chat-bot@project.iam.gserviceaccount.com".into(), + ), ..Default::default() } .resolve(); assert!(r.use_adc); + assert_eq!( + r.adc_target_service_account.as_deref(), + Some("chat-bot@project.iam.gserviceaccount.com") + ); // --- config wins over env --- std::env::set_var("GOOGLE_CHAT_ENABLED", "true"); diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index 970100edf..bd704c5e0 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -10,8 +10,12 @@ use tokio::sync::Mutex; use tokio_tungstenite::tungstenite::Message; use tracing::{error, info, warn}; -/// Timeout for waiting on gateway reply acknowledgement. +/// Legacy timeout for streaming platforms that may not acknowledge normal sends. const GATEWAY_REPLY_TIMEOUT_SECS: u64 = 5; +/// Acknowledged send-once replies can include bounded auth refresh (up to three +/// 10-second requests), so allow that work to finish; unlike the legacy path, +/// timeout is an error because the adapter promised a delivery response. +const ACKED_GATEWAY_REPLY_TIMEOUT_SECS: u64 = 35; /// Platforms whose gateway adapter emits a `GatewayResponse` for `edit_message` /// so core can observe edit success or failure (used to gate the per-edit @@ -40,6 +44,22 @@ fn platform_acks_writes(platform: &str) -> bool { EDIT_RESPONSE_PLATFORMS.contains(&platform) } + +/// Platforms whose gateway adapters acknowledge normal send replies with a +/// `GatewayResponse`. This capability is independent of cosmetic streaming: +/// Google Chat is send-once, but its adapter reports API/auth failures and core +/// must retain the request id to observe them. +const REPLY_RESPONSE_PLATFORMS: &[&str] = &["googlechat"]; + +fn platform_acks_replies(platform: &str) -> bool { + REPLY_RESPONSE_PLATFORMS.contains(&platform) +} + +/// Preserve legacy request/response waits for streaming adapters while also +/// supporting send-once adapters that explicitly acknowledge delivery. +fn reply_requires_ack(platform: &str, streaming: bool) -> bool { + streaming || platform_acks_replies(platform) +} /// Gateway platforms whose messaging API cannot edit a message after it is sent. /// /// Cosmetic (typewriter) streaming works by posting a placeholder and then @@ -230,6 +250,18 @@ struct GatewayResponse { error: Option, } +fn gateway_delivery_result(resp: GatewayResponse) -> Result { + if resp.success { + Ok(resp.message_id.unwrap_or_else(|| "gw_sent".into())) + } else { + Err(anyhow::anyhow!( + "gateway reported failure: {}", + resp.error + .unwrap_or_else(|| "gateway reported failure".to_string()) + )) + } +} + // --- GatewayAdapter: ChatAdapter over WebSocket --- type PendingRequests = Arc>>>; @@ -279,7 +311,7 @@ impl GatewayAdapter { content: &str, quote_message_id: Option<&str>, ) -> Result { - let req_id = if self.streaming { + let req_id = if reply_requires_ack(self.platform_name, self.streaming) { Some(format!("req_{}", uuid::Uuid::new_v4())) } else { None @@ -315,33 +347,42 @@ impl GatewayAdapter { return Err(e.into()); } let msg_id = if let (Some(rx), Some(ref id)) = (pending_rx, &req_id) { - match tokio::time::timeout(std::time::Duration::from_secs(GATEWAY_REPLY_TIMEOUT_SECS), rx).await { - Ok(Ok(resp)) if resp.success => resp.message_id.unwrap_or_else(|| "gw_sent".into()), - Ok(Ok(resp)) => { - // Gateway explicitly reported failure (success=false). Surface - // as Err so dispatch sets ❌ instead of 🆗 over an incomplete - // delivery. Examples: Feishu edit cap reached after append-new - // fallback also failed; chunked send delivered N/M chunks. - let err_msg = resp.error.clone() - .unwrap_or_else(|| "gateway reported failure".to_string()); - tracing::warn!(request_id = %id, error = %err_msg, "gateway replied with failure"); - return Err(anyhow::anyhow!("gateway reported failure: {err_msg}")); - } + let ack_required = platform_acks_replies(self.platform_name); + let timeout_secs = if ack_required { + ACKED_GATEWAY_REPLY_TIMEOUT_SECS + } else { + GATEWAY_REPLY_TIMEOUT_SECS + }; + match tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), rx).await { + Ok(Ok(resp)) => match gateway_delivery_result(resp) { + Ok(message_id) => message_id, + Err(e) => { + tracing::warn!(request_id = %id, error = %e, "gateway replied with failure"); + return Err(e); + } + }, Ok(Err(_)) => { - // Channel closed (gateway shutting down or pending dropped). - // Maintain legacy behavior — adapters that don't implement - // GatewayResponse for all reply types (LINE, Teams) rely on - // this for non-failure outcomes. + if ack_required { + return Err(anyhow::anyhow!( + "gateway acknowledgement channel closed for {}", + self.platform_name + )); + } + // Legacy streaming adapters may not acknowledge normal sends. tracing::warn!(request_id = %id, "gateway response channel closed"); "gw_sent".into() } Err(_) => { - // Timeout. Many adapters (LINE, Teams) intentionally do not - // emit GatewayResponse for replies, so timeout is the expected - // path for them. Maintain legacy behavior to avoid breaking - // platforms that have not yet wired request/response feedback. - tracing::warn!(request_id = %id, "gateway reply timed out"); self.pending.lock().await.remove(id); + if ack_required { + return Err(anyhow::anyhow!( + "gateway delivery acknowledgement timed out after {timeout_secs}s for {}", + self.platform_name + )); + } + // Preserve legacy behavior for adapters that do not promise + // a GatewayResponse for normal sends. + tracing::warn!(request_id = %id, "gateway reply timed out"); "gw_sent".into() } } @@ -1695,6 +1736,29 @@ mod tests { assert!(!platform_supports_streaming("googlechat")); } + #[test] + fn googlechat_send_once_still_requires_delivery_ack() { + assert!(platform_acks_replies("googlechat")); + assert!(reply_requires_ack("googlechat", false)); + assert!(!reply_requires_ack("line", false)); + // Preserve legacy behavior: streaming adapters still carry request IDs. + assert!(reply_requires_ack("discord", true)); + } + + #[test] + fn acknowledged_reply_failure_is_propagated() { + let err = gateway_delivery_result(GatewayResponse { + schema: "openab.gateway.response.v1".into(), + request_id: "req_test".into(), + success: false, + thread_id: None, + message_id: None, + error: Some("googlechat API returned 403".into()), + }) + .expect_err("success=false must reach core as Err"); + assert!(err.to_string().contains("googlechat API returned 403")); + } + #[test] fn editable_platforms_still_allow_streaming() { for platform in ["telegram", "slack", "discord", "feishu", "teams", "wecom"] { diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 30d8df70a..0e8f9c950 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -311,13 +311,18 @@ pub(crate) struct GoogleChatParts { pub access_token: Option, pub audience: Option, pub use_adc: bool, + /// Dedicated Google Chat service account impersonated by the attached + /// workload identity. It MUST differ from the metadata server's default SA; + /// Google prohibits access-token self-impersonation. + pub adc_target_service_account: Option, } impl GoogleChatAdapter { /// Build an adapter from resolved parts (#1379): SA key JSON (inline wins /// over file path), optional static access token, optional JWT audience, - /// and `use_adc` (keyless ADC via the GCE metadata server + IAM - /// Credentials). Auth precedence at send time: SA key > ADC > static token. + /// and keyless ADC via GCE metadata + IAM Credentials for a distinct + /// `adc_target_service_account`. Auth precedence at send time: + /// SA key > ADC target > static token. /// Shared by env-derived construction and `apply_googlechat_config`. pub(crate) fn from_parts(parts: GoogleChatParts) -> Self { use tracing::{info, warn}; @@ -327,6 +332,7 @@ impl GoogleChatAdapter { access_token, audience, use_adc, + adc_target_service_account, } = parts; let key_configured = sa_key_json.is_some() || sa_key_file.is_some(); let token_cache = sa_key_json @@ -349,27 +355,41 @@ impl GoogleChatAdapter { GoogleChatJwtVerifier::new(aud) }); // Precedence at send time (see `get_token`): SA key > ADC > static token. - if use_adc { - if key_configured && token_cache.is_none() { + let adc_target = adc_target_service_account + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_owned); + if use_adc && token_cache.is_none() { + if key_configured { // A key WAS configured but failed to load. Don't switch identity - // silently: name it. get_token still falls to ADC, but the operator - // is told the bot now presents the workload identity, not the key's. - warn!( - "Google Chat SA key was configured but could not be loaded; \ - falling back to the keyless ADC (workload) identity — this is NOT \ - the configured key identity. Fix the key, or unset use_adc." + // silently: name the distinct target identity selected instead. + match adc_target.as_deref() { + Some(target) => warn!( + target_service_account = %target, + "Google Chat SA key was configured but could not be loaded; \ + falling back to keyless ADC impersonation of the configured \ + target — this is NOT the failed key identity" + ), + None => error!( + "Google Chat SA key was configured but could not be loaded, and \ + use_adc=true has no adc_target_service_account; ADC is disabled" + ), + } + } else if let Some(target) = adc_target.as_deref() { + info!( + target_service_account = %target, + "googlechat keyless ADC enabled (distinct target, chat.bot via IAM Credentials)" ); - } else if token_cache.is_none() { - info!("googlechat keyless ADC enabled (chat.bot via IAM Credentials)"); } } let mut adapter = Self::new(token_cache, access_token, jwt_verifier); - // Install ADC only when it can actually be consulted: a loaded SA key - // wins outright at send time (see `get_token`), so don't construct a - // dead-at-runtime source behind it. A configured key that FAILED to - // load still installs ADC — that is the named fallback warned above. - adapter.metadata_source = - (use_adc && adapter.token_cache.is_none()).then(MetadataTokenSource::new); + // Install ADC only when it can actually be consulted and a distinct + // target is configured. A loaded SA key wins outright; use_adc without + // a target fails closed instead of attempting prohibited self-impersonation. + if use_adc && adapter.token_cache.is_none() { + adapter.metadata_source = adc_target.map(MetadataTokenSource::new); + } adapter } @@ -392,10 +412,10 @@ impl GoogleChatAdapter { /// > ADC (`metadata_source`) > static `access_token`. /// /// Failure behavior is asymmetric by design: an SA-key exchange error - /// hard-fails (`None` — the operator explicitly configured that identity, - /// so nothing is silently substituted), while an ADC mint error falls - /// through to the static token (both are the same workload's credential, - /// so degrading is not an identity switch). + /// hard-fails (`None` — the operator explicitly configured that identity), + /// while an ADC mint error can fall through to an explicitly configured + /// static token. The static token is opaque and may represent a different + /// principal, so that degradation is logged as a possible identity switch. async fn get_token(&self) -> Option { if let Some(ref cache) = self.token_cache { match cache.get_token(&self.client).await { @@ -410,13 +430,14 @@ impl GoogleChatAdapter { match src.get_token().await { Ok(t) => return Some(t), Err(e) => { - // Fall through to a configured static token instead of - // dropping the reply. ADC and the static token are the same - // workload's own credential, so this is not an identity switch. + // A configured static token is an explicit fallback, but it + // is opaque: the adapter cannot prove it represents the same + // principal as the ADC target. Name the possible identity + // switch rather than claiming equivalence. if self.access_token.is_some() { error!( - "googlechat ADC token mint failed ({e}); \ - falling back to the configured static access_token" + "googlechat ADC token mint failed ({e}); falling back to \ + configured static access_token (possible identity switch)" ); } else { error!("googlechat ADC token mint failed: {e}"); @@ -1030,6 +1051,10 @@ const ADC_CHAT_BOT_SCOPE: &str = "https://www.googleapis.com/auth/chat.bot"; /// Lifetime we request from IAM Credentials for the impersonated token, and /// the fallback TTL we cache it under. IAM caps impersonated tokens at 3600s. const ADC_TOKEN_LIFETIME_SECS: u64 = 3600; +/// After a failed refresh while a cached token remains valid, reuse that token +/// for this cooldown instead of making each queued sender repeat all metadata/ +/// IAM calls serially under the write lock. +const ADC_REFRESH_RETRY_COOLDOWN_SECS: u64 = 30; /// Cache TTL (seconds) to use for a minted token, derived from the IAM /// response's `expireTime`. Falls back to the full lifetime when the field is @@ -1056,16 +1081,18 @@ fn refresh_threshold(ttl: u64) -> u64 { } /// Mints a `chat.bot`-scoped access token **without** a service-account key -/// file, using the workload's own identity (keyless ADC). Flow, per refresh: -/// 1. read the default SA's email + a base token from the GCE metadata server -/// 2. call IAM Credentials `generateAccessToken` (self-impersonation) to -/// exchange the base token for a `chat.bot`-scoped token +/// file, using two distinct identities. Flow, per refresh: +/// 1. read the attached runtime SA's email + base token from GCE metadata +/// 2. call IAM Credentials `generateAccessToken` for the separately configured +/// Google Chat target SA and request the `chat.bot` scope /// -/// Requires `roles/iam.serviceAccountTokenCreator` on the SA over itself and -/// the `iamcredentials.googleapis.com` API enabled. The `*_base` fields are -/// overridable so tests can point them at a mock server. +/// The runtime SA requires `roles/iam.serviceAccountTokenCreator` on the target +/// SA. The identities MUST differ: Google prohibits using a service account's +/// short-lived access token to generate another access token for itself. pub struct MetadataTokenSource { token: RwLock>, + refresh_retry_after: RwLock>, + target_service_account: String, // Private: only `new` (prod, fixed trusted hosts) or the in-module // `with_bases` (tests, mock server) may set these. Unexported ⇒ no in-process // caller can retarget the metadata bearer to an arbitrary host. @@ -1076,16 +1103,11 @@ pub struct MetadataTokenSource { client: reqwest::Client, } -impl Default for MetadataTokenSource { - fn default() -> Self { - Self::new() - } -} - impl MetadataTokenSource { - /// Production constructor: the fixed, trusted GCP endpoints (IAM over HTTPS). - pub fn new() -> Self { + /// Production constructor: fixed trusted endpoints and a distinct target SA. + pub fn new(target_service_account: String) -> Self { Self::with_bases( + target_service_account, "http://metadata.google.internal".into(), "https://iamcredentials.googleapis.com".into(), ) @@ -1093,9 +1115,15 @@ impl MetadataTokenSource { /// Construct with explicit endpoint bases. Prod always goes through `new` /// with HTTPS IAM; tests point these at a mock server. - fn with_bases(metadata_base: String, iam_credentials_base: String) -> Self { + fn with_bases( + target_service_account: String, + metadata_base: String, + iam_credentials_base: String, + ) -> Self { Self { token: RwLock::new(None), + refresh_retry_after: RwLock::new(None), + target_service_account, metadata_base, iam_credentials_base, client: reqwest::Client::builder() @@ -1118,31 +1146,50 @@ impl MetadataTokenSource { } let mut guard = self.token.write().await; if let Some((ref tok, ref ts, ttl)) = *guard { - if ts.elapsed().as_secs() < refresh_threshold(ttl) { + let elapsed = ts.elapsed().as_secs(); + if elapsed < refresh_threshold(ttl) { + return Ok(tok.clone()); + } + // A previous refresh failed but this token is still valid. While + // the cooldown is active, queued/subsequent callers reuse it + // immediately instead of repeating up to three HTTP timeouts. + if elapsed < ttl + && self + .refresh_retry_after + .read() + .await + .is_some_and(|deadline| deadline > Instant::now()) + { return Ok(tok.clone()); } } match self.refresh().await { Ok(minted) => { - let MintedToken { token, ttl, sa_email } = minted; + *self.refresh_retry_after.write().await = None; + let MintedToken { + token, + ttl, + runtime_service_account, + target_service_account, + } = minted; if ttl == 0 { // Freshly minted but already at/after its expireTime — almost // always local clock skew (Google validates against its own // clock, so the token may still work). Serve it once, but do // not cache a dead token: the next call re-mints. warn!( - service_account = %sa_email, + runtime_service_account = %runtime_service_account, + target_service_account = %target_service_account, "googlechat ADC minted a token with ttl=0 (clock skew?); \ serving once without caching" ); return Ok(token); } *guard = Some((token.clone(), Instant::now(), ttl)); - // Name the resolved identity so on-call can confirm which SA - // self-impersonation actually used when diagnosing send errors. info!( - service_account = %sa_email, - "googlechat ADC token minted (chat.bot, ttl {ttl}s)" + runtime_service_account = %runtime_service_account, + target_service_account = %target_service_account, + "googlechat ADC token minted (distinct target, chat.bot, ttl {ttl}s)" ); Ok(token) } @@ -1152,10 +1199,17 @@ impl MetadataTokenSource { if let Some((ref tok, ref ts, ttl)) = *guard { let elapsed = ts.elapsed().as_secs(); if elapsed < ttl { + *self.refresh_retry_after.write().await = Some( + Instant::now() + + std::time::Duration::from_secs( + ADC_REFRESH_RETRY_COOLDOWN_SECS, + ), + ); warn!( "googlechat ADC refresh failed ({e}); serving cached token \ - still valid for {}s", - ttl - elapsed + still valid for {}s; retry suppressed for {}s", + ttl - elapsed, + ADC_REFRESH_RETRY_COOLDOWN_SECS ); return Ok(tok.clone()); } @@ -1184,9 +1238,15 @@ impl MetadataTokenSource { .text() .await .map_err(|e| format!("metadata email read failed: {e}"))?; - let email = email.trim(); - if email.is_empty() { - return Err("metadata returned empty SA email".into()); + let runtime_service_account = email.trim(); + if runtime_service_account.is_empty() { + return Err("metadata returned empty runtime SA email".into()); + } + if runtime_service_account.eq_ignore_ascii_case(&self.target_service_account) { + return Err(format!( + "ADC target service account must differ from runtime service account \ + (self-impersonation is prohibited): {runtime_service_account}" + )); } // 2. Base access token for the default SA from the metadata server. @@ -1211,11 +1271,11 @@ impl MetadataTokenSource { .and_then(non_empty_token) .ok_or("metadata token response missing or empty access_token")?; - // 3. Exchange the base token for a chat.bot-scoped token via IAM - // Credentials generateAccessToken (the SA impersonates itself). + // 3. Exchange the runtime SA's base token for a chat.bot-scoped token + // for the distinct configured Chat-app service account. let url = format!( - "{}/v1/projects/-/serviceAccounts/{email}:generateAccessToken", - self.iam_credentials_base + "{}/v1/projects/-/serviceAccounts/{}:generateAccessToken", + self.iam_credentials_base, self.target_service_account ); let resp = client .post(&url) @@ -1268,17 +1328,19 @@ impl MetadataTokenSource { Ok(MintedToken { token, ttl, - sa_email: email.to_string(), + runtime_service_account: runtime_service_account.to_string(), + target_service_account: self.target_service_account.clone(), }) } } -/// A successfully minted ADC token plus the identity that minted it, so the -/// caller can log which service account self-impersonation resolved to. +/// A successfully minted ADC token plus the source and target identities for +/// audit logging of the supported two-service-account impersonation flow. struct MintedToken { token: String, ttl: u64, - sa_email: String, + runtime_service_account: String, + target_service_account: String, } /// Best-effort classification of common GCP `generateAccessToken` failures. @@ -1287,7 +1349,11 @@ struct MintedToken { /// missing `roles/iam.serviceAccountTokenCreator` binding. fn classify_generate_access_token_error(status: u16, body: &str) -> &'static str { let b = body.to_ascii_lowercase(); - if b.contains("api has not been used") || b.contains("service_disabled") || b.contains("is disabled") + if b.contains("failed_precondition") || b.contains("same service account") { + "self_impersonation_prohibited" + } else if b.contains("api has not been used") + || b.contains("service_disabled") + || b.contains("is disabled") { "api_not_enabled" } else if status == 403 && b.contains("scopes") { @@ -2275,7 +2341,7 @@ mod tests { }))) .mount(&server) .await; - // IAM Credentials: generateAccessToken (self-impersonation) → chat.bot token. + // IAM Credentials: runtime SA impersonates distinct Chat target. Mock::given(method("POST")) .and(path_regex( r"/v1/projects/-/serviceAccounts/.*:generateAccessToken", @@ -2287,7 +2353,11 @@ mod tests { .mount(&server) .await; - let src = MetadataTokenSource::with_bases(server.uri(), server.uri()); + let src = MetadataTokenSource::with_bases( + "chat-bot@project.iam.gserviceaccount.com".into(), + server.uri(), + server.uri(), + ); let token = src .get_token() @@ -2296,10 +2366,90 @@ mod tests { assert_eq!(token, "chat-bot-tok"); } + #[tokio::test] + async fn metadata_token_source_rejects_self_impersonation_before_token_request() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + let same_sa = "runtime@project.iam.gserviceaccount.com"; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with(ResponseTemplate::new(200).set_body_string(same_sa)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let src = MetadataTokenSource::with_bases( + same_sa.into(), + server.uri(), + server.uri(), + ); + let err = src + .get_token() + .await + .expect_err("same runtime/target SA must be rejected"); + assert!(err.contains("self-impersonation is prohibited"), "{err}"); + } + + #[tokio::test] + async fn adc_refresh_failure_cooldown_prevents_queued_retry_storm() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("runtime@project.iam.gserviceaccount.com"), + ) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .respond_with(ResponseTemplate::new(503)) + .expect(1) + .mount(&server) + .await; + + let src = MetadataTokenSource::with_bases( + "chat-bot@project.iam.gserviceaccount.com".into(), + server.uri(), + server.uri(), + ); + *src.token.write().await = Some(( + "cached-token".into(), + Instant::now() - std::time::Duration::from_secs(6), + 10, + )); + + // First call attempts refresh, gets 503, serves the still-valid token, + // and starts cooldown. Second call must reuse it without another GET. + assert_eq!(src.get_token().await.unwrap(), "cached-token"); + assert_eq!(src.get_token().await.unwrap(), "cached-token"); + } + #[test] fn from_parts_use_adc_toggles_metadata_source() { let with = GoogleChatAdapter::from_parts(GoogleChatParts { use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); assert!(with.metadata_source.is_some(), "use_adc=true → ADC source"); @@ -2310,6 +2460,18 @@ mod tests { ); } + #[test] + fn from_parts_use_adc_without_target_fails_closed() { + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + use_adc: true, + ..Default::default() + }); + assert!( + adapter.metadata_source.is_none(), + "use_adc without adc_target_service_account must not install ADC" + ); + } + #[test] fn from_parts_malformed_key_with_use_adc_installs_adc() { // Regression: a configured-but-malformed SA key parses to @@ -2318,6 +2480,7 @@ mod tests { let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { sa_key_json: Some("not valid json".into()), use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); assert!( @@ -2337,6 +2500,7 @@ mod tests { let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { sa_key_file: Some("/nonexistent/path/sa-key.json".into()), use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); assert!(adapter.token_cache.is_none(), "unreadable key file → no cache"); @@ -2358,6 +2522,7 @@ mod tests { let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { sa_key_json: Some(key), use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); assert!(adapter.token_cache.is_some(), "valid key JSON → SA-key cache"); @@ -2369,6 +2534,13 @@ mod tests { #[test] fn classify_generate_access_token_error_covers_documented_cases() { + assert_eq!( + classify_generate_access_token_error( + 400, + r#"{"error":{"status":"FAILED_PRECONDITION","message":"You can't create a token for the same service account that you used to authenticate the request."}}"#, + ), + "self_impersonation_prohibited" + ); // Insufficient scope: the 403 body says "scopes" — the documented // signal distinguishing it from a missing IAM role. assert_eq!( @@ -2455,11 +2627,15 @@ mod tests { let mut adapter = GoogleChatAdapter::from_parts(GoogleChatParts { access_token: Some("static-tok".into()), use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); // Repoint the ADC source at the mock server (bases are private now). - adapter.metadata_source = - Some(MetadataTokenSource::with_bases(server.uri(), server.uri())); + adapter.metadata_source = Some(MetadataTokenSource::with_bases( + "chat-bot@project.iam.gserviceaccount.com".into(), + server.uri(), + server.uri(), + )); let token = adapter.get_token().await.expect("a token"); assert_eq!(token, "chat-bot-tok", "ADC should win over static token"); } @@ -2502,7 +2678,11 @@ mod tests { .mount(&server) .await; - let src = MetadataTokenSource::with_bases(server.uri(), server.uri()); + let src = MetadataTokenSource::with_bases( + "chat-bot@project.iam.gserviceaccount.com".into(), + server.uri(), + server.uri(), + ); let err = src.get_token().await.expect_err("blank token must be rejected"); assert!( err.contains("missing or empty accessToken"), diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index 92799eaa2..ef2aa86e1 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -203,6 +203,11 @@ impl AppState { use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") .map(|v| v == "true" || v == "1") .unwrap_or(false), + adc_target_service_account: std::env::var( + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", + ) + .ok() + .filter(|s| !s.trim().is_empty()), }, )) } else { @@ -461,6 +466,7 @@ impl AppState { access_token: cfg.access_token, audience: cfg.audience, use_adc: cfg.use_adc, + adc_target_service_account: cfg.adc_target_service_account, }, )) } else { @@ -563,6 +569,7 @@ pub struct GatewayGoogleChatConfig { pub access_token: Option, pub audience: Option, pub use_adc: bool, + pub adc_target_service_account: Option, pub webhook_path: String, } @@ -765,6 +772,11 @@ pub async fn serve(config: ServeConfig) -> anyhow::Result<()> { use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") .map(|v| v == "true" || v == "1") .unwrap_or(false), + adc_target_service_account: std::env::var( + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", + ) + .ok() + .filter(|s| !s.trim().is_empty()), }, )) } else { @@ -1264,6 +1276,7 @@ mod l1_audit_tests { access_token: Some("tok".into()), audience: None, use_adc: false, + adc_target_service_account: None, webhook_path: "/hook/gc".into(), }); assert!(s.google_chat.is_some()); @@ -1278,6 +1291,7 @@ mod l1_audit_tests { access_token: Some("tok".into()), audience: Some("aud".into()), use_adc: false, + adc_target_service_account: None, webhook_path: "/hook/gc".into(), }); assert!(flagged(&s).is_empty()); @@ -1290,6 +1304,7 @@ mod l1_audit_tests { access_token: None, audience: None, use_adc: false, + adc_target_service_account: None, webhook_path: "/hook/gc".into(), }); assert!(s.google_chat.is_none()); diff --git a/crates/openab-gateway/tests/config_first_conformance.rs b/crates/openab-gateway/tests/config_first_conformance.rs index 4c8f19877..447b2216f 100644 --- a/crates/openab-gateway/tests/config_first_conformance.rs +++ b/crates/openab-gateway/tests/config_first_conformance.rs @@ -93,6 +93,7 @@ const COVERED: &[&str] = &[ "GOOGLE_CHAT_SA_KEY_FILE", "GOOGLE_CHAT_ACCESS_TOKEN", "GOOGLE_CHAT_USE_ADC", + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", "GOOGLE_CHAT_AUDIENCE", "GOOGLE_CHAT_WEBHOOK_PATH", "GOOGLE_CHAT_ALLOW_ALL_USERS", diff --git a/docs/config-reference.md b/docs/config-reference.md index a8820d88d..818003484 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -225,7 +225,8 @@ Full first-class Google Chat section (config-first parity, #1379) — credential | `sa_key_json` | string | — | Inline service-account key JSON (wins over `sa_key_file`). Env: `GOOGLE_CHAT_SA_KEY_JSON`. | | `sa_key_file` | string | — | Path to a service-account key file. Env: `GOOGLE_CHAT_SA_KEY_FILE`. | | `access_token` | string | — | Static access token alternative. Env: `GOOGLE_CHAT_ACCESS_TOKEN`. | -| `use_adc` | bool | `false` | Keyless ADC — mint the `chat.bot` token from the workload's own GCP identity (GCE metadata + IAM Credentials `generateAccessToken` self-impersonation); no SA key file. Needs `roles/iam.serviceAccountTokenCreator` on the SA over itself + `iamcredentials.googleapis.com`. Ignored when a SA key is set and loads successfully; a configured key that fails to load falls back to ADC with a warning (see `docs/google-chat.md` Option C). Env: `GOOGLE_CHAT_USE_ADC`. | +| `use_adc` | bool | `false` | Enable keyless ADC: the attached runtime SA impersonates a distinct Chat-app target via IAM Credentials. Requires `roles/iam.serviceAccountTokenCreator` on the target + `iamcredentials.googleapis.com`. Self-impersonation is prohibited. Env: `GOOGLE_CHAT_USE_ADC`. | +| `adc_target_service_account` | string | — | Dedicated Chat-app SA email to impersonate. Required with `use_adc=true`; MUST differ from the runtime SA. Env: `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT`. | | `audience` | string | — | JWT audience — enables webhook JWT verification (L1). Env: `GOOGLE_CHAT_AUDIENCE`. | | `webhook_path` | string | `/webhook/googlechat` | Env: `GOOGLE_CHAT_WEBHOOK_PATH`. | | `allow_all_users` | bool \| omit | `false` (deny-all) | Env: `GOOGLE_CHAT_ALLOW_ALL_USERS`. | diff --git a/docs/google-chat.md b/docs/google-chat.md index 1823562d7..bd707a31a 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -114,24 +114,27 @@ docker run -d --name openab-gateway \ ### Option C: Keyless ADC (recommended on GCP — no key file) -When the gateway runs on GCP (GKE / GCE / Cloud Run) **as** a service account, it can mint the `chat.bot` token from that identity — no service-account key file to mount, manage, or leak. The gateway reads a base token from the GCE metadata server and calls IAM Credentials `generateAccessToken` (the SA impersonates itself) for a `chat.bot`-scoped token, then caches and auto-refreshes it. +When the gateway runs on GCP (GKE / GCE / Cloud Run), its attached **runtime service account** can impersonate a separate **Google Chat service account** and mint a `chat.bot` token without any key file. The gateway reads the runtime identity and a base token from the GCE metadata server, then calls IAM Credentials `generateAccessToken` for the configured Chat-app target identity. + +The two identities **must be different**. Google prohibits using a service account's short-lived access token to generate another access token for that same service account (`FAILED_PRECONDITION`); see [Service account credentials — Self-impersonation](https://cloud.google.com/iam/docs/service-account-creds#self-impersonation). Prerequisites: -- The workload's service account **is** the Chat app's service account (the identity that sends must be the space member). -- Grant that SA `roles/iam.serviceAccountTokenCreator` **on itself**. +- Attach a runtime SA to the workload (for example `openab-runtime@PROJECT.iam.gserviceaccount.com`). +- Use a distinct SA as the Google Chat app identity (for example `openab-chat@PROJECT.iam.gserviceaccount.com`); that target SA must be the app/space member that sends messages. +- Grant the runtime SA `roles/iam.serviceAccountTokenCreator` **on the target Chat SA**. - Enable `iamcredentials.googleapis.com`. -- The **base token from the metadata server must carry `cloud-platform` (or `.../auth/iam`) scope** — `generateAccessToken` requires it on the caller. GKE Workload Identity and Cloud Run tokens are `cloud-platform`-scoped and satisfy this automatically. A **default-scope GCE VM does not**: it returns `403 PERMISSION_DENIED: "Request had insufficient authentication scopes."` even when the IAM binding above is correct — match on the word *scopes* (not *permission*) to tell it apart from a missing role. GCE access scopes **cannot be changed while the VM is running**: create the VM with `--scopes=cloud-platform`, or run `gcloud compute instances set-scopes --scopes=cloud-platform` followed by a stop/start. +- The metadata base token must carry `cloud-platform` (or `.../auth/iam`) scope. A default-scope GCE VM returns `403 PERMISSION_DENIED: "Request had insufficient authentication scopes."`; match *scopes* to distinguish it from a missing role. GCE access scopes cannot be changed while the VM is running: create the VM with `--scopes=cloud-platform`, or use `gcloud compute instances set-scopes` followed by a stop/start. -> Why self-impersonation (not plain ADC): the `chat.bot` scope is a Workspace scope and is **not** a subset of `cloud-platform`, so no `?scopes=` parameter on the metadata token can produce it. `generateAccessToken` is the only keyless way to obtain a `chat.bot` token. +`chat.bot` is a Workspace scope and is not a subset of `cloud-platform`, so the runtime metadata token cannot call Chat directly. The supported flow is runtime SA → distinct target Chat SA via `generateAccessToken`. ```bash -# The pod/VM already runs as the target service account; no key is mounted. export GOOGLE_CHAT_ENABLED=true export GOOGLE_CHAT_USE_ADC=true +export GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT="openab-chat@PROJECT.iam.gserviceaccount.com" ``` -Precedence: if a SA key (`GOOGLE_CHAT_SA_KEY_JSON` / `GOOGLE_CHAT_SA_KEY_FILE`) is also set **and loads successfully**, the SA key wins and ADC is ignored. If a key is configured but **fails to load** (unreadable file / malformed JSON), the adapter falls back to the keyless ADC (workload) identity and logs a warning naming the switch — fix the key, or unset `use_adc` to make that failure explicit. +Precedence: if a configured SA key loads successfully, it wins and ADC is ignored. If a key is configured but fails to load, the adapter uses the configured ADC target and logs the identity switch. `GOOGLE_CHAT_USE_ADC=true` without `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT` fails closed (ADC is not installed). If ADC fails and a static token is explicitly configured, the adapter degrades to it with a warning that it may represent a different identity. > **Migrating an existing release from a SA key to ADC:** the chart renders the Google Chat Secret only when `saKeyJson` / `accessToken` is set, and that Secret carries `helm.sh/resource-policy: keep`. Switching to ADC-only stops Helm from managing it but **leaves the old key material in the cluster indefinitely**. Delete the orphaned Secret after the switch: it is the gateway Secret named by the chart's `openab.agentFullname` helper — `--gateway` by default (or the agent's `nameOverride`) — and it contains the `google-chat-sa-key-json` key. Find it with `kubectl get secrets -o name | grep gateway`, confirm with `kubectl get secret -o jsonpath='{.data}' | grep -o google-chat-sa-key-json`, then `kubectl delete secret `. Otherwise the "no key to mount or leak" benefit is undercut. @@ -244,7 +247,8 @@ Each field falls back to its `GOOGLE_CHAT_ALLOW_ALL_USERS` / `GOOGLE_CHAT_ALLOWE | `GOOGLE_CHAT_SA_KEY_JSON` | No | — | Service account key JSON string (enables auto-refresh) | | `GOOGLE_CHAT_SA_KEY_FILE` | No | — | Path to service account key JSON file (alternative to `SA_KEY_JSON`) | | `GOOGLE_CHAT_ACCESS_TOKEN` | No | — | Static OAuth2 access token (fallback, expires in 1 hour) | -| `GOOGLE_CHAT_USE_ADC` | No | `false` | Keyless ADC auth via the GCE metadata server + IAM Credentials `generateAccessToken` self-impersonation (GCP-hosted only) — see Option C. Ignored when `SA_KEY_JSON`/`SA_KEY_FILE` is set and loads; a configured key that fails to load falls back to ADC with a warning (Option C) | +| `GOOGLE_CHAT_USE_ADC` | No | `false` | Enable keyless ADC: attached runtime SA impersonates a distinct Chat-app target via IAM Credentials — see Option C | +| `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT` | With ADC | — | Email of the dedicated Chat-app SA to impersonate. Required when `USE_ADC=true`; MUST differ from the runtime SA | | `GOOGLE_CHAT_WEBHOOK_PATH` | No | `/webhook/googlechat` | Webhook endpoint path | ## Security: Webhook Verification diff --git a/docs/platforms/schema/googlechat.toml b/docs/platforms/schema/googlechat.toml index 07285ff7e..c43489d18 100644 --- a/docs/platforms/schema/googlechat.toml +++ b/docs/platforms/schema/googlechat.toml @@ -248,7 +248,7 @@ refs = [] [[quirks]] date = "2026-08-25" title = "Keyless ADC outbound path (no SA key)" -note = "Outbound creds have a third option beside the SA-key JWT-bearer exchange and the static token: keyless ADC (use_adc / GOOGLE_CHAT_USE_ADC). MetadataTokenSource reads the default SA email + a base token from the GCE metadata server, then calls IAM Credentials generateAccessToken (the SA impersonates itself) for a chat.bot-scoped token, cached with the same 300 s refresh margin. Meant for workloads that already run as the Chat app's service account on GCP — no key file to mount or leak. Requires roles/iam.serviceAccountTokenCreator on the SA over itself + iamcredentials.googleapis.com. Auth precedence in get_token: SA key (token_cache) > ADC (metadata_source) > static access_token." +note = "Outbound creds have a third option beside the SA-key JWT-bearer exchange and the static token: keyless ADC (use_adc / GOOGLE_CHAT_USE_ADC). MetadataTokenSource reads the attached runtime SA email + base token from GCE metadata, then calls IAM Credentials generateAccessToken for the distinct adc_target_service_account / GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT and requests chat.bot. Google prohibits access-token self-impersonation, so runtime and target identities MUST differ; the code rejects equality before minting. The runtime SA needs roles/iam.serviceAccountTokenCreator on the target. Auth precedence: SA key > ADC target > explicitly configured static token (ADC-to-static degradation logs a possible identity switch)." kind = "openab_decision" source = "crates/openab-gateway/src/adapters/googlechat.rs#MetadataTokenSource" refs = [] diff --git a/src/main.rs b/src/main.rs index 03f86b748..f49879778 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1285,6 +1285,7 @@ async fn main() -> anyhow::Result<()> { sa_key_file: r.sa_key_file, access_token: r.access_token, use_adc: r.use_adc, + adc_target_service_account: r.adc_target_service_account, audience: r.audience, webhook_path: r.webhook_path, }); From 3b08312c6dd0afa08252807e5ea1f0ea2eacf985 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Fri, 4 Sep 2026 09:13:34 -0400 Subject: [PATCH 06/12] fix(googlechat): harden ADC metadata client (no proxy, fail-loud build) The MetadataTokenSource client carries the plaintext GCE metadata bearer. Two hardening fixes on its construction: - Add .no_proxy(): reqwest honors HTTP(S)_PROXY by default, which could route the metadata access-token response through an operator/attacker proxy hop. Disable proxies so the bearer never leaves the metadata path. - Replace .build().unwrap_or_default() with .expect(): on a builder error unwrap_or_default() yields a DEFAULT client that follows redirects and honors proxies, silently defeating the no-redirect/no-proxy guarantee this source exists to uphold. Fail loud at construction instead. Addresses supplementary review findings on #1513 (F3/F4). --- crates/openab-gateway/src/adapters/googlechat.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 0e8f9c950..9afbc816a 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -1127,9 +1127,16 @@ impl MetadataTokenSource { metadata_base, iam_credentials_base, client: reqwest::Client::builder() + // The metadata bearer must never follow a redirect onto a third + // host, nor traverse a proxy (the plaintext metadata token would + // then transit an operator/attacker-controlled hop). Fail loud + // at construction rather than silently falling back to a default + // client that does both — a default client here would defeat the + // guarantee this source exists to uphold. .redirect(reqwest::redirect::Policy::none()) + .no_proxy() .build() - .unwrap_or_default(), + .expect("no-redirect, no-proxy metadata client must build"), } } From 6115377202aac69ce5ed5192bda547a7be9ef5eb Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:02:27 +0000 Subject: [PATCH 07/12] fix(googlechat): address ADC review findings --- crates/openab-core/src/config.rs | 19 +- .../openab-gateway/src/adapters/googlechat.rs | 218 ++++++++++++++++-- 2 files changed, 209 insertions(+), 28 deletions(-) diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index 0894e7ef1..d4593f094 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -1254,13 +1254,13 @@ pub struct ResolvedGoogleChat { impl GoogleChatConfig { /// Resolve every field: config value (if set) → `GOOGLE_CHAT_*` env → - /// default. String fields filter empty strings from `${}` expansion. + /// default. String fields filter empty or whitespace-only `${}` expansion. pub fn resolve(&self) -> ResolvedGoogleChat { let opt_str = |cfg: &Option, env: &str| -> Option { cfg.as_ref() - .filter(|s| !s.is_empty()) + .filter(|s| !s.trim().is_empty()) .cloned() - .or_else(|| std::env::var(env).ok()) + .or_else(|| std::env::var(env).ok().filter(|s| !s.trim().is_empty())) }; ResolvedGoogleChat { enabled: self.enabled.unwrap_or_else(|| { @@ -3001,6 +3001,15 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] assert!(r.audience.is_none()); assert_eq!(r.webhook_path, "/webhook/googlechat"); + // --- whitespace-only secrets/targets are absent at the config boundary --- + std::env::set_var("GOOGLE_CHAT_ACCESS_TOKEN", " \t "); + std::env::set_var("GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", " \t "); + let r = GoogleChatConfig::default().resolve(); + assert!(r.access_token.is_none()); + assert!(r.adc_target_service_account.is_none()); + std::env::remove_var("GOOGLE_CHAT_ACCESS_TOKEN"); + std::env::remove_var("GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT"); + // --- use_adc: config value resolves without touching env --- let r = GoogleChatConfig { use_adc: Some(true), @@ -3028,9 +3037,9 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] assert!(!r.enabled); // config false wins over env true assert_eq!(r.audience.as_deref(), Some("cfg-aud")); - // --- empty-string ${} expansion falls through to env --- + // --- whitespace-only ${} expansion falls through to env --- let cfg = GoogleChatConfig { - audience: Some("".into()), + audience: Some(" \t ".into()), ..Default::default() }; let r = cfg.resolve(); diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 9afbc816a..d53038304 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -381,6 +381,10 @@ impl GoogleChatAdapter { target_service_account = %target, "googlechat keyless ADC enabled (distinct target, chat.bot via IAM Credentials)" ); + } else { + error!( + "googlechat use_adc=true has no adc_target_service_account; ADC is disabled" + ); } } let mut adapter = Self::new(token_cache, access_token, jwt_verifier); @@ -398,6 +402,10 @@ impl GoogleChatAdapter { access_token: Option, jwt_verifier: Option, ) -> Self { + // Static credentials enter through config and env construction paths. + // Reject whitespace-only values here so every caller shares the same + // outbound bearer boundary without altering valid token bytes. + let access_token = access_token.filter(|token| non_empty_token(token).is_some()); Self { token_cache, metadata_source: None, @@ -445,7 +453,10 @@ impl GoogleChatAdapter { } } } - self.access_token.clone() + self.access_token + .as_deref() + .and_then(non_empty_token) + .map(str::to_owned) } async fn edit_message(&self, message_name: &str, text: &str) { @@ -1098,9 +1109,11 @@ pub struct MetadataTokenSource { // caller can retarget the metadata bearer to an arbitrary host. metadata_base: String, iam_credentials_base: String, - // No-redirect client: a redirect from either endpoint must never carry - // the metadata bearer (`Authorization`) on to a third host. - client: reqwest::Client, + // Metadata is plaintext/link-local: never redirect it or allow a proxy hop. + metadata_client: reqwest::Client, + // IAM is a public HTTPS API and must retain the deployment's configured + // egress proxy while still refusing bearer-carrying redirects. + iam_client: reqwest::Client, } impl MetadataTokenSource { @@ -1119,6 +1132,45 @@ impl MetadataTokenSource { target_service_account: String, metadata_base: String, iam_credentials_base: String, + ) -> Self { + let metadata_client = Self::build_metadata_client(reqwest::Client::builder()); + let iam_client = Self::build_iam_client(reqwest::Client::builder()); + Self::with_bases_and_clients( + target_service_account, + metadata_base, + iam_credentials_base, + metadata_client, + iam_client, + ) + } + + fn build_metadata_client(builder: reqwest::ClientBuilder) -> reqwest::Client { + builder + // The metadata bearer must never follow a redirect onto a third + // host, nor traverse a proxy (the plaintext metadata token would + // then transit an operator/attacker-controlled hop). Fail loud + // rather than silently falling back to an unsafe default client. + .redirect(reqwest::redirect::Policy::none()) + .no_proxy() + .build() + .expect("no-redirect, no-proxy metadata client must build") + } + + fn build_iam_client(builder: reqwest::ClientBuilder) -> reqwest::Client { + builder + // IAM is HTTPS egress, so preserve HTTP(S)_PROXY/system proxy + // routing while preventing a bearer-carrying redirect. + .redirect(reqwest::redirect::Policy::none()) + .build() + .expect("no-redirect IAM client must build") + } + + fn with_bases_and_clients( + target_service_account: String, + metadata_base: String, + iam_credentials_base: String, + metadata_client: reqwest::Client, + iam_client: reqwest::Client, ) -> Self { Self { token: RwLock::new(None), @@ -1126,17 +1178,8 @@ impl MetadataTokenSource { target_service_account, metadata_base, iam_credentials_base, - client: reqwest::Client::builder() - // The metadata bearer must never follow a redirect onto a third - // host, nor traverse a proxy (the plaintext metadata token would - // then transit an operator/attacker-controlled hop). Fail loud - // at construction rather than silently falling back to a default - // client that does both — a default client here would defeat the - // guarantee this source exists to uphold. - .redirect(reqwest::redirect::Policy::none()) - .no_proxy() - .build() - .expect("no-redirect, no-proxy metadata client must build"), + metadata_client, + iam_client, } } @@ -1227,10 +1270,11 @@ impl MetadataTokenSource { } async fn refresh(&self) -> Result { - // Use the source's own no-redirect client for every bearer-carrying call. - let client = &self.client; + // Metadata and IAM have different proxy requirements. Both clients + // reject redirects, but only the plaintext metadata path disables proxying. + let metadata_client = &self.metadata_client; // 1. Default SA email from the GCE metadata server. - let email = client + let email = metadata_client .get(format!( "{}/computeMetadata/v1/instance/service-accounts/default/email", self.metadata_base @@ -1257,7 +1301,7 @@ impl MetadataTokenSource { } // 2. Base access token for the default SA from the metadata server. - let base: serde_json::Value = client + let base: serde_json::Value = metadata_client .get(format!( "{}/computeMetadata/v1/instance/service-accounts/default/token", self.metadata_base @@ -1284,7 +1328,8 @@ impl MetadataTokenSource { "{}/v1/projects/-/serviceAccounts/{}:generateAccessToken", self.iam_credentials_base, self.target_service_account ); - let resp = client + let resp = self + .iam_client .post(&url) .bearer_auth(base_token) .json(&serde_json::json!({ @@ -2409,6 +2454,69 @@ mod tests { assert!(err.contains("self-impersonation is prohibited"), "{err}"); } + #[tokio::test] + async fn metadata_bypasses_proxy_while_iam_uses_proxy_client() { + use wiremock::matchers::{header, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let metadata_server = MockServer::start().await; + let iam_proxy = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_string("runtime@project.iam.gserviceaccount.com"), + ) + .expect(1) + .mount(&metadata_server) + .await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/token", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "base-tok", "expires_in": 3600 + }))) + .expect(1) + .mount(&metadata_server) + .await; + Mock::given(method("POST")) + .and(header("authorization", "Bearer base-tok")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "accessToken": "chat-bot-tok" + }))) + .expect(1) + .mount(&iam_proxy) + .await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&iam_proxy) + .await; + + // Start both production policy helpers from a builder carrying the + // same explicit proxy. Metadata must clear it; IAM must retain it. + let metadata_client = MetadataTokenSource::build_metadata_client( + reqwest::Client::builder() + .proxy(reqwest::Proxy::all(iam_proxy.uri()).unwrap()), + ); + let iam_client = MetadataTokenSource::build_iam_client( + reqwest::Client::builder() + .proxy(reqwest::Proxy::all(iam_proxy.uri()).unwrap()), + ); + let src = MetadataTokenSource::with_bases_and_clients( + "chat-bot@project.iam.gserviceaccount.com".into(), + metadata_server.uri(), + "http://iam.invalid".into(), + metadata_client, + iam_client, + ); + + assert_eq!(src.get_token().await.unwrap(), "chat-bot-tok"); + } + #[tokio::test] async fn adc_refresh_failure_cooldown_prevents_queued_retry_storm() { use wiremock::matchers::{method, path}; @@ -2469,14 +2577,45 @@ mod tests { #[test] fn from_parts_use_adc_without_target_fails_closed() { - let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { - use_adc: true, - ..Default::default() + use std::io::Write; + use std::sync::{Arc as StdArc, Mutex as StdMutex}; + + #[derive(Clone)] + struct Capture(StdArc>>); + impl Write for Capture { + fn write(&mut self, bytes: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(bytes); + Ok(bytes.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + let buffer = StdArc::new(StdMutex::new(Vec::new())); + let capture = Capture(buffer.clone()); + let subscriber = tracing_subscriber::fmt() + .with_writer(move || capture.clone()) + .with_ansi(false) + .without_time() + .finish(); + let adapter = tracing::subscriber::with_default(subscriber, || { + GoogleChatAdapter::from_parts(GoogleChatParts { + use_adc: true, + ..Default::default() + }) }); assert!( adapter.metadata_source.is_none(), "use_adc without adc_target_service_account must not install ADC" ); + let output = String::from_utf8(buffer.lock().unwrap().clone()).unwrap(); + assert!( + output.contains( + "googlechat use_adc=true has no adc_target_service_account; ADC is disabled" + ), + "missing ADC target must emit a startup diagnostic: {output}" + ); } #[test] @@ -2585,6 +2724,39 @@ mod tests { assert_eq!(non_empty_token(" token "), Some(" token ")); } + #[tokio::test] + async fn static_access_token_rejects_whitespace_at_adapter_boundary() { + let adapter = GoogleChatAdapter::new(None, Some(" \t\n".into()), None); + assert!(adapter.access_token.is_none()); + assert_eq!(adapter.get_token().await, None); + } + + #[tokio::test] + async fn adc_failure_does_not_fall_back_to_whitespace_static_token() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/computeMetadata/v1/instance/service-accounts/default/email", + )) + .respond_with(ResponseTemplate::new(503)) + .expect(1) + .mount(&server) + .await; + + let mut adapter = GoogleChatAdapter::new(None, Some(" \t\n".into()), None); + adapter.metadata_source = Some(MetadataTokenSource::with_bases( + "chat-bot@project.iam.gserviceaccount.com".into(), + server.uri(), + server.uri(), + )); + + assert_eq!(adapter.get_token().await, None); + assert!(adapter.access_token.is_none()); + } + #[test] fn edit_targets_require_full_message_resource_names() { assert!(is_google_chat_message_name("spaces/SP/messages/msg1")); From 31cea79d0c057cdbb57424d6ef65c5f703e53416 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Fri, 4 Sep 2026 14:04:39 -0400 Subject: [PATCH 08/12] fix(googlechat): complete reliable ADC delivery paths Close the remaining review gaps in the corrected keyless Google Chat flow: - share a direct Result-returning delivery path between standalone WebSocket handling and the unified in-process adapter, so unified auth/API failures reach core instead of becoming synthetic success; preserve real message IDs - bound the complete delivery to 30s (inside core's 35s ack window), add 10s per-mutation timeouts, and stop on the first failed chunk with explicit partial-delivery context - validate/normalize user-managed target SA emails before IAM URL construction, rejecting numeric unique IDs and path/query/trailing-dot aliases; keep the MetadataTokenSource constructor private behind that invariant - attach an existing ServiceAccount to gateway pods with precedence gateway > per-agent > global, document GKE Workload Identity setup, and add Helm tests (including numeric-looking KSA names rendered as strings) - classify the proxy-routing test as ignored integration, accept common GOOGLE_CHAT_USE_ADC case/whitespace forms, and avoid false failures when an accepted 2xx response omits a message resource name Validated on macmini: workspace all-feature check, gateway+unified clippy with -D warnings, gateway tests (332 pass/1 ignored), ignored proxy test explicitly, unified/core regressions, feature-off core check, and Helm unittest 46/46. --- charts/openab/README.md | 1 + charts/openab/templates/gateway.yaml | 5 + .../tests/gateway_serviceaccount_test.yaml | 69 ++++ charts/openab/values.yaml | 4 + crates/openab-core/src/config.rs | 7 +- .../openab-gateway/src/adapters/googlechat.rs | 372 +++++++++++------- crates/openab-gateway/src/lib.rs | 4 +- docs/config-reference.md | 6 + docs/google-chat.md | 21 + src/unified_adapter.rs | 89 ++++- 10 files changed, 429 insertions(+), 149 deletions(-) create mode 100644 charts/openab/tests/gateway_serviceaccount_test.yaml diff --git a/charts/openab/README.md b/charts/openab/README.md index dfac33a38..e7244cd87 100644 --- a/charts/openab/README.md +++ b/charts/openab/README.md @@ -48,6 +48,7 @@ Each agent lives under `agents.`. | `stt.baseUrl` | STT API base URL. | `"https://api.groq.com/openai/v1"` | | `gateway.enabled` | Enable the gateway config block for webhook-based platforms. | `false` | | `gateway.deploy` | Deploy the gateway Deployment and Service. | `true` | +| `gateway.serviceAccountName` | Existing ServiceAccount attached to the gateway pod. Empty falls back to the per-agent `serviceAccountName`, then the chart-global value. Required for GKE Workload Identity when `gateway.googleChat.useAdc=true`; the chart does not create or annotate the ServiceAccount. | `""` | | `cron.usercronEnabled` | Enable user-provided cron configuration. | `false` | | `cronjobs` | Config-driven scheduled messages for an agent. | `[]` | | `persistence.enabled` | Enable persistent storage for auth and settings. | `true` | diff --git a/charts/openab/templates/gateway.yaml b/charts/openab/templates/gateway.yaml index 3482d4cc5..c8dd95b04 100644 --- a/charts/openab/templates/gateway.yaml +++ b/charts/openab/templates/gateway.yaml @@ -4,6 +4,8 @@ {{- $gwCfg := omit $cfg "nameOverride" }} {{- $d := dict "ctx" $ "agent" (printf "%s-gateway" $name) "cfg" $gwCfg }} {{- $agentD := dict "ctx" $ "agent" $name "cfg" $cfg }} +{{- $agentSvcAcct := default $.Values.serviceAccountName $cfg.serviceAccountName }} +{{- $gatewaySvcAcct := default $agentSvcAcct (($cfg.gateway).serviceAccountName) }} {{- $hasTeams := and (($cfg.gateway).teams).appId (($cfg.gateway).teams).appSecret }} {{- $hasTelegram := (($cfg.gateway).telegram).botToken }} {{- $hasLine := (($cfg.gateway).line).channelSecret }} @@ -32,6 +34,9 @@ spec: securityContext: {{- toYaml . | nindent 8 }} {{- end }} + {{- if $gatewaySvcAcct }} + serviceAccountName: {{ $gatewaySvcAcct | quote }} + {{- end }} containers: - name: gateway image: {{ printf "%s:%s" (($cfg.gateway).image | default "ghcr.io/openabdev/openab-gateway") (($cfg.gateway).tag | default $.Chart.AppVersion) }} diff --git a/charts/openab/tests/gateway_serviceaccount_test.yaml b/charts/openab/tests/gateway_serviceaccount_test.yaml new file mode 100644 index 000000000..a63b1930d --- /dev/null +++ b/charts/openab/tests/gateway_serviceaccount_test.yaml @@ -0,0 +1,69 @@ +suite: gateway serviceAccountName support + +templates: + - templates/gateway.yaml + +set: + agents.kiro.gateway.enabled: true + +tests: + - it: does not render a gateway serviceAccountName when no value is set + documentIndex: 0 + asserts: + - notExists: + path: spec.template.spec.serviceAccountName + + - it: falls back to the chart-global serviceAccountName + set: + serviceAccountName: global-runtime + documentIndex: 0 + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: global-runtime + + - it: falls back to the per-agent serviceAccountName before the global value + set: + serviceAccountName: global-runtime + agents.kiro.serviceAccountName: agent-runtime + documentIndex: 0 + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: agent-runtime + + - it: gateway serviceAccountName overrides per-agent and global values + set: + serviceAccountName: global-runtime + agents.kiro.serviceAccountName: agent-runtime + agents.kiro.gateway.serviceAccountName: gateway-runtime + documentIndex: 0 + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: gateway-runtime + + - it: renders ADC target and the dedicated gateway runtime service account together + set: + agents.kiro.gateway.serviceAccountName: googlechat-runtime + agents.kiro.gateway.googleChat.useAdc: true + agents.kiro.gateway.googleChat.adcTargetServiceAccount: openab-chat@project.iam.gserviceaccount.com + documentIndex: 0 + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: googlechat-runtime + - contains: + path: spec.template.spec.containers[0].env + content: + name: GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT + value: openab-chat@project.iam.gserviceaccount.com + + - it: quotes numeric-looking gateway serviceAccountName as a string + set: + agents.kiro.gateway.serviceAccountName: "123" + documentIndex: 0 + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: "123" diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index acb8e4f42..9854240fa 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -395,6 +395,10 @@ agents: gateway: enabled: false # set to true + provide url to enable the [gateway] config block deploy: true # set to false to skip Gateway Deployment/Service (config-only mode) + # Existing Kubernetes ServiceAccount for the gateway pod. Empty falls back + # to agents..serviceAccountName, then chart-global serviceAccountName. + # Required on GKE when googleChat.useAdc uses Workload Identity. + serviceAccountName: "" url: "" # e.g. ws://openab-gateway:8080/ws platform: "telegram" # default platform when gateway is enabled token: "" # optional shared secret (injected via GATEWAY_WS_TOKEN env var) diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index d4593f094..7c744ecf0 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -1273,7 +1273,7 @@ impl GoogleChatConfig { access_token: opt_str(&self.access_token, "GOOGLE_CHAT_ACCESS_TOKEN"), use_adc: self.use_adc.unwrap_or_else(|| { std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v == "true" || v == "1") + .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) .unwrap_or(false) }), adc_target_service_account: opt_str( @@ -3010,6 +3010,11 @@ allowed_users = ["U1234567890abcdef0123456789abcdef"] std::env::remove_var("GOOGLE_CHAT_ACCESS_TOKEN"); std::env::remove_var("GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT"); + // --- identity-selecting bool env accepts common case/whitespace forms --- + std::env::set_var("GOOGLE_CHAT_USE_ADC", " True "); + assert!(GoogleChatConfig::default().resolve().use_adc); + std::env::remove_var("GOOGLE_CHAT_USE_ADC"); + // --- use_adc: config value resolves without touching env --- let r = GoogleChatConfig { use_adc: Some(true), diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index d53038304..a3d0cbbe0 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -27,6 +27,30 @@ const MEDIA_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_sec /// write lock (the refresh runs while holding it) or prevent the ADC → static /// token degradation path from engaging. const TOKEN_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +/// Bound each Google Chat API mutation independently. The enclosing delivery +/// deadline below is stricter than core's 35-second acknowledgement window and +/// covers token resolution plus every sequential chunk. +const CHAT_API_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +/// End-to-end budget for one outbound Google Chat reply. Core waits 35 seconds +/// for an acknowledged gateway reply; finishing within 30 seconds leaves room +/// for response serialization and WebSocket/broadcast scheduling. +const GOOGLE_CHAT_DELIVERY_DEADLINE: std::time::Duration = + std::time::Duration::from_secs(30); + +async fn within_delivery_deadline( + deadline: std::time::Duration, + future: F, +) -> Result +where + F: std::future::Future>, +{ + tokio::time::timeout(deadline, future).await.map_err(|_| { + format!( + "googlechat delivery timed out after {}s", + deadline.as_secs() + ) + })? +} /// Cap on text file attachments per message (matches Discord/Slack). const TEXT_FILE_COUNT_CAP: usize = 5; /// Cap on aggregate text file bytes per message (matches Discord/Slack 1 MB). @@ -273,6 +297,51 @@ fn non_empty_token(value: &str) -> Option<&str> { (!value.trim().is_empty()).then_some(value) } +/// Normalize and validate the dedicated ADC target identity before it can be +/// interpolated into an IAM Credentials resource path. Only user-managed +/// service-account emails are accepted: this intentionally rejects numeric +/// unique IDs (which could alias the runtime SA and bypass the equality guard), +/// trailing-dot aliases, and URL/path metacharacters. +fn normalize_adc_target_service_account(value: Option) -> Result, String> { + let Some(raw) = value else { + return Ok(None); + }; + let email = raw.trim().to_ascii_lowercase(); + if email.is_empty() { + return Ok(None); + } + + const SUFFIX: &str = ".iam.gserviceaccount.com"; + let (account, domain) = email.split_once('@').ok_or_else(|| { + "adc_target_service_account must be a service-account email, not an ID".to_string() + })?; + let project = domain.strip_suffix(SUFFIX).ok_or_else(|| { + format!("adc_target_service_account must end with {SUFFIX}") + })?; + let valid_label = |label: &str| { + !label.is_empty() + && label + .bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-') + && label + .as_bytes() + .first() + .is_some_and(u8::is_ascii_alphanumeric) + && label + .as_bytes() + .last() + .is_some_and(u8::is_ascii_alphanumeric) + }; + if !valid_label(account) || !valid_label(project) { + return Err( + "adc_target_service_account must use account@project.iam.gserviceaccount.com with only lowercase letters, digits, and interior hyphens" + .into(), + ); + } + + Ok(Some(email)) +} + /// Google Chat edit targets must be full `spaces/{space}/messages/{message}` /// resource names. Reject synthetic ids and incomplete `spaces/` prefixes /// before token resolution or network I/O. @@ -355,11 +424,19 @@ impl GoogleChatAdapter { GoogleChatJwtVerifier::new(aud) }); // Precedence at send time (see `get_token`): SA key > ADC > static token. - let adc_target = adc_target_service_account - .as_deref() - .map(str::trim) - .filter(|s| !s.is_empty()) - .map(str::to_owned); + // Validate at the adapter boundary so env-only/config-first construction + // and every future caller share the same IAM resource-path invariant. + let adc_target = if use_adc && token_cache.is_none() { + match normalize_adc_target_service_account(adc_target_service_account) { + Ok(target) => target, + Err(e) => { + error!("googlechat ADC target is invalid: {e}; ADC is disabled"); + None + } + } + } else { + None + }; if use_adc && token_cache.is_none() { if key_configured { // A key WAS configured but failed to load. Don't switch identity @@ -472,7 +549,15 @@ impl GoogleChatAdapter { ); let body = serde_json::json!({ "text": formatted }); - match self.client.patch(&url).bearer_auth(&token).json(&body).send().await { + match self + .client + .patch(&url) + .bearer_auth(&token) + .json(&body) + .timeout(CHAT_API_REQUEST_TIMEOUT) + .send() + .await + { Ok(r) if r.status().is_success() => { tracing::trace!(message_name = %message_name, "googlechat message edited"); } @@ -487,6 +572,66 @@ impl GoogleChatAdapter { } } + /// Deliver one normal Google Chat reply and return the created message + /// resource name. Both the standalone gateway acknowledgement path and the + /// in-process unified adapter call this method, so transport failures have + /// identical semantics instead of being observable only over WebSocket. + pub async fn deliver_message(&self, reply: &GatewayReply) -> Result { + within_delivery_deadline( + GOOGLE_CHAT_DELIVERY_DEADLINE, + self.deliver_message_inner(reply), + ) + .await + } + + async fn deliver_message_inner(&self, reply: &GatewayReply) -> Result { + info!( + space = %reply.channel.id, + thread_id = ?reply.channel.thread_id, + "gateway → googlechat" + ); + + let token = self + .get_token() + .await + .ok_or_else(|| "no credentials configured".to_string())?; + let chunks = split_text(&reply.content.text, GOOGLE_CHAT_MESSAGE_LIMIT); + if chunks.is_empty() { + return Err("empty message".into()); + } + + let total = chunks.len(); + let mut first_message_name = None; + for (index, chunk) in chunks.into_iter().enumerate() { + match send_message( + &self.client, + &token, + &reply.channel.id, + reply.channel.thread_id.as_deref(), + chunk, + &self.api_base, + ) + .await + { + Ok(name) => { + if first_message_name.is_none() { + first_message_name = Some(name); + } + } + Err(e) => { + return Err(format!( + "chunk {}/{} failed after {} successful chunk(s): {e}", + index + 1, + total, + index + )); + } + } + } + + first_message_name.ok_or_else(|| "googlechat delivery produced no message".into()) + } + pub async fn handle_reply( &self, reply: &GatewayReply, @@ -522,121 +667,25 @@ impl GoogleChatAdapter { _ => {} } - info!( - space = %reply.channel.id, - thread_id = ?reply.channel.thread_id, - "gateway → googlechat" - ); - - let Some(token) = self.get_token().await else { - info!( - text = %reply.content.text, - "googlechat reply (dry-run, no credentials configured)" - ); - if let Some(ref req_id) = reply.request_id { - let resp = crate::schema::GatewayResponse { - schema: "openab.gateway.response.v1".into(), - request_id: req_id.clone(), - success: false, - thread_id: None, - message_id: None, - error: Some("no credentials configured".into()), - }; - if let Ok(json) = serde_json::to_string(&resp) { - let _ = event_tx.send(json); - } - } - return; - }; - - let text = &reply.content.text; - let chunks = split_text(text, GOOGLE_CHAT_MESSAGE_LIMIT); - - // Empty message: short-circuit, send failure ack and skip API call - if chunks.is_empty() { - if let Some(ref req_id) = reply.request_id { - let resp = crate::schema::GatewayResponse { - schema: "openab.gateway.response.v1".into(), - request_id: req_id.clone(), - success: false, - thread_id: None, - message_id: None, - error: Some("empty message".into()), - }; - if let Ok(json) = serde_json::to_string(&resp) { - let _ = event_tx.send(json); - } - } - return; - } - - if chunks.len() == 1 { - let result = send_message( - &self.client, - &token, - &reply.channel.id, - reply.channel.thread_id.as_deref(), - text, - &self.api_base, - ) - .await; - - if let Some(ref req_id) = reply.request_id { - let (success, message_id, error) = match result { - Ok(name) => (true, Some(name), None), - Err(e) => (false, None, Some(e)), - }; - let resp = crate::schema::GatewayResponse { - schema: "openab.gateway.response.v1".into(), - request_id: req_id.clone(), - success, - thread_id: None, - message_id, - error, - }; - if let Ok(json) = serde_json::to_string(&resp) { - let _ = event_tx.send(json); - } - } - } else { - let mut first_msg_name: Option = None; - let mut first_error: Option = None; - for chunk in chunks { - match send_message( - &self.client, - &token, - &reply.channel.id, - reply.channel.thread_id.as_deref(), - chunk, - &self.api_base, - ) - .await - { - Ok(name) => { - if first_msg_name.is_none() { - first_msg_name = Some(name); - } - } - Err(e) => { - if first_error.is_none() { - first_error = Some(e); - } - } - } - } - if let Some(ref req_id) = reply.request_id { - let resp = crate::schema::GatewayResponse { - schema: "openab.gateway.response.v1".into(), - request_id: req_id.clone(), - success: first_msg_name.is_some() && first_error.is_none(), - thread_id: None, - message_id: first_msg_name, - error: first_error, - }; - if let Ok(json) = serde_json::to_string(&resp) { - let _ = event_tx.send(json); - } + let result = self.deliver_message(reply).await; + if let Some(ref req_id) = reply.request_id { + let (success, message_id, error) = match result { + Ok(name) => (true, Some(name), None), + Err(e) => (false, None, Some(e)), + }; + let resp = crate::schema::GatewayResponse { + schema: "openab.gateway.response.v1".into(), + request_id: req_id.clone(), + success, + thread_id: None, + message_id, + error, + }; + if let Ok(json) = serde_json::to_string(&resp) { + let _ = event_tx.send(json); } + } else if let Err(e) = result { + error!(error = %e, "googlechat reply delivery failed without acknowledgement id"); } } } @@ -1117,8 +1166,10 @@ pub struct MetadataTokenSource { } impl MetadataTokenSource { - /// Production constructor: fixed trusted endpoints and a distinct target SA. - pub fn new(target_service_account: String) -> Self { + /// Production constructor for a target already normalized and validated by + /// [`normalize_adc_target_service_account`]. Kept private so callers cannot + /// bypass that invariant. + fn new(target_service_account: String) -> Self { Self::with_bases( target_service_account, "http://metadata.google.internal".into(), @@ -1650,6 +1701,7 @@ async fn send_message( .post(&url) .bearer_auth(token) .json(&body) + .timeout(CHAT_API_REQUEST_TIMEOUT) .send() .await; @@ -1657,11 +1709,15 @@ async fn send_message( Ok(r) if r.status().is_success() => { let body = r.text().await.unwrap_or_default(); let parsed: serde_json::Value = serde_json::from_str(&body).unwrap_or_default(); - parsed - .get("name") - .and_then(|v| v.as_str()) - .map(String::from) - .ok_or_else(|| "missing message name in response".into()) + if let Some(name) = parsed.get("name").and_then(|v| v.as_str()) { + Ok(name.to_string()) + } else { + warn!( + response_body = %body, + "googlechat accepted message but omitted its resource name; using synthetic receipt" + ); + Ok("googlechat_sent".into()) + } } Ok(r) => { let status = r.status(); @@ -2455,6 +2511,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: validates reqwest proxy routing with local HTTP servers"] async fn metadata_bypasses_proxy_while_iam_uses_proxy_client() { use wiremock::matchers::{header, method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2618,6 +2675,19 @@ mod tests { ); } + #[test] + fn from_parts_use_adc_with_invalid_target_fails_closed() { + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + use_adc: true, + adc_target_service_account: Some("123456789012345678901".into()), + ..Default::default() + }); + assert!( + adapter.metadata_source.is_none(), + "numeric unique IDs must not bypass the service-account email invariant" + ); + } + #[test] fn from_parts_malformed_key_with_use_adc_installs_adc() { // Regression: a configured-but-malformed SA key parses to @@ -2724,6 +2794,41 @@ mod tests { assert_eq!(non_empty_token(" token "), Some(" token ")); } + #[test] + fn adc_target_requires_user_managed_service_account_email() { + assert_eq!( + normalize_adc_target_service_account(Some( + " Chat-Bot@Project-1.iam.gserviceaccount.com ".into() + )) + .unwrap() + .as_deref(), + Some("chat-bot@project-1.iam.gserviceaccount.com") + ); + for invalid in [ + "123456789012345678901", + "runtime@project-1.iam.gserviceaccount.com.", + "runtime@project-1.iam.gserviceaccount.com?x=y", + "runtime@project-1.iam.gserviceaccount.com/../other", + "-runtime@project-1.iam.gserviceaccount.com", + ] { + assert!( + normalize_adc_target_service_account(Some(invalid.into())).is_err(), + "accepted invalid ADC target: {invalid}" + ); + } + } + + #[tokio::test] + async fn delivery_deadline_cancels_hung_operation() { + let err = within_delivery_deadline( + std::time::Duration::from_millis(1), + std::future::pending::>(), + ) + .await + .expect_err("pending delivery must time out"); + assert!(err.contains("delivery timed out"), "{err}"); + } + #[tokio::test] async fn static_access_token_rejects_whitespace_at_adapter_boundary() { let adapter = GoogleChatAdapter::new(None, Some(" \t\n".into()), None); @@ -3458,9 +3563,10 @@ mod tests { #[tokio::test] async fn handle_reply_multi_chunk_partial_failure_reports_failure() { - // Mixed success/failure: chunk 1 succeeds, subsequent chunks fail. - // Expect success=false (any chunk failure marks overall as failed), - // but message_id is still set so core has a reference. + // Mixed success/failure: chunk 1 succeeds, chunk 2 fails. The direct + // Result contract reports no overall message receipt, but the error + // retains explicit partial-delivery context so operators know one chunk + // already reached the space and should not blindly retry the whole turn. use wiremock::{Mock, MockServer, ResponseTemplate}; use wiremock::matchers::{method, path_regex}; @@ -3511,9 +3617,11 @@ mod tests { let resp: GatewayResponse = serde_json::from_str(&received.unwrap()).unwrap(); assert_eq!(resp.request_id, "req_partial"); assert!(!resp.success, "partial failure must report success=false"); - assert_eq!(resp.message_id, Some("spaces/TEST/messages/first_chunk".into())); + assert!(resp.message_id.is_none(), "partial delivery is not an overall receipt"); let err = resp.error.expect("partial failure should set error"); - assert!(err.contains("500")); + assert!(err.contains("chunk 2/2"), "{err}"); + assert!(err.contains("after 1 successful chunk(s)"), "{err}"); + assert!(err.contains("500"), "{err}"); } // --- Attachment parsing tests --- diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index ef2aa86e1..c23f099be 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -201,7 +201,7 @@ impl AppState { access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v == "true" || v == "1") + .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) .unwrap_or(false), adc_target_service_account: std::env::var( "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", @@ -770,7 +770,7 @@ pub async fn serve(config: ServeConfig) -> anyhow::Result<()> { access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v == "true" || v == "1") + .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) .unwrap_or(false), adc_target_service_account: std::env::var( "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", diff --git a/docs/config-reference.md b/docs/config-reference.md index 5f3eab96a..6a6d9033b 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -232,6 +232,12 @@ Full first-class Google Chat section (config-first parity, #1379) — credential | `allow_all_users` | bool \| omit | `false` (deny-all) | Env: `GOOGLE_CHAT_ALLOW_ALL_USERS`. | | `allowed_users` | string[] | `[]` | User resource names (`users/`). Env: `GOOGLE_CHAT_ALLOWED_USERS`. | +For GKE Workload Identity, set Helm value +`agents..gateway.serviceAccountName` to an existing annotated Kubernetes +ServiceAccount for the runtime GSA. An empty value falls back to +`agents..serviceAccountName`, then chart-global `serviceAccountName`; the +chart does not create or annotate the ServiceAccount. + --- ## `[teams]` diff --git a/docs/google-chat.md b/docs/google-chat.md index bd707a31a..d4f044868 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -134,6 +134,27 @@ export GOOGLE_CHAT_USE_ADC=true export GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT="openab-chat@PROJECT.iam.gserviceaccount.com" ``` +For GKE + Helm, bind a Kubernetes ServiceAccount (KSA) to the **runtime** GSA +out of band, then attach that KSA to the gateway pod. The chart references an +existing KSA; it does not create or annotate one: + +```yaml +agents: + kiro: + gateway: + enabled: true + serviceAccountName: openab-googlechat-runtime + googleChat: + useAdc: true + adcTargetServiceAccount: openab-chat@PROJECT.iam.gserviceaccount.com +``` + +The `openab-googlechat-runtime` KSA must carry the usual +`iam.gke.io/gcp-service-account: openab-runtime@PROJECT.iam.gserviceaccount.com` +annotation and Workload Identity IAM binding. If `gateway.serviceAccountName` +is empty, the chart falls back to `agents..serviceAccountName`, then the +chart-global `serviceAccountName`. + Precedence: if a configured SA key loads successfully, it wins and ADC is ignored. If a key is configured but fails to load, the adapter uses the configured ADC target and logs the identity switch. `GOOGLE_CHAT_USE_ADC=true` without `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT` fails closed (ADC is not installed). If ADC fails and a static token is explicitly configured, the adapter degrades to it with a warning that it may represent a different identity. > **Migrating an existing release from a SA key to ADC:** the chart renders the Google Chat Secret only when `saKeyJson` / `accessToken` is set, and that Secret carries `helm.sh/resource-policy: keep`. Switching to ADC-only stops Helm from managing it but **leaves the old key material in the cluster indefinitely**. Delete the orphaned Secret after the switch: it is the gateway Secret named by the chart's `openab.agentFullname` helper — `--gateway` by default (or the agent's `nameOverride`) — and it contains the `google-chat-sa-key-json` key. Find it with `kubectl get secrets -o name | grep gateway`, confirm with `kubectl get secret -o jsonpath='{.data}' | grep -o google-chat-sa-key-json`, then `kubectl delete secret `. Otherwise the "no key to mount or leak" benefit is undercut. diff --git a/src/unified_adapter.rs b/src/unified_adapter.rs index c943f4d0d..92344482c 100644 --- a/src/unified_adapter.rs +++ b/src/unified_adapter.rs @@ -1,7 +1,7 @@ //! UnifiedGatewayAdapter — routes ChatAdapter calls through in-process gateway //! platform adapters based on the ChannelRef.platform field. -use anyhow::Result; +use anyhow::{anyhow, Result}; use async_trait::async_trait; use openab_core::adapter::{ChannelRef, ChatAdapter, MessageRef}; use openab_gateway::schema::{Content, GatewayReply, ReplyChannel}; @@ -24,8 +24,10 @@ impl UnifiedGatewayAdapter { } } - /// Dispatch a GatewayReply to the correct platform adapter. - async fn dispatch_reply(&self, reply: &GatewayReply) { + /// Dispatch a GatewayReply to the correct platform adapter. Platforms with + /// direct delivery receipts return the real message resource name; legacy + /// fire-and-forget adapters return `None`. + async fn dispatch_reply(&self, reply: &GatewayReply) -> Result> { let client = &self.gw_state.client; match reply.platform.as_str() { #[cfg(feature = "telegram")] @@ -69,7 +71,16 @@ impl UnifiedGatewayAdapter { #[cfg(feature = "googlechat")] "googlechat" => { if let Some(ref gc) = self.gw_state.google_chat { + if reply.command.is_none() { + return gc + .deliver_message(reply) + .await + .map(Some) + .map_err(anyhow::Error::msg); + } gc.handle_reply(reply, &self.gw_state.event_tx).await; + } else if reply.command.is_none() { + return Err(anyhow!("googlechat adapter is not configured")); } } #[cfg(feature = "wecom")] @@ -112,9 +123,13 @@ impl UnifiedGatewayAdapter { } } other => { - tracing::warn!(platform = other, "unified adapter: unknown platform, cannot route reply"); + tracing::warn!( + platform = other, + "unified adapter: unknown platform, cannot route reply" + ); } } + Ok(None) } /// Build a GatewayReply from ChatAdapter parameters. @@ -157,11 +172,18 @@ impl ChatAdapter for UnifiedGatewayAdapter { async fn send_message(&self, channel: &ChannelRef, content: &str) -> Result { let reply = self.build_reply(channel, content, None, None); - self.dispatch_reply(&reply).await; + let message_id = self.dispatch_reply(&reply).await?.unwrap_or_else(|| { + format!( + "unified_{:x}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + ) + }); Ok(MessageRef { channel: channel.clone(), - message_id: format!("unified_{:x}", std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH).unwrap_or_default().as_nanos()), + message_id, }) } @@ -172,7 +194,7 @@ impl ChatAdapter for UnifiedGatewayAdapter { title: &str, ) -> Result { let reply = self.build_reply(channel, title, Some("create_topic"), None); - self.dispatch_reply(&reply).await; + let _ = self.dispatch_reply(&reply).await?; // Return a thread channel ref with the trigger message as thread_id Ok(ChannelRef { platform: channel.platform.clone(), @@ -187,7 +209,7 @@ impl ChatAdapter for UnifiedGatewayAdapter { let mut reply = self.build_reply(&msg.channel, emoji, Some("add_reaction"), None); // Use the actual platform message_id (not origin_event_id which is a UUID) reply.reply_to = msg.message_id.clone(); - self.dispatch_reply(&reply).await; + let _ = self.dispatch_reply(&reply).await?; Ok(()) } @@ -195,7 +217,7 @@ impl ChatAdapter for UnifiedGatewayAdapter { let mut reply = self.build_reply(&msg.channel, emoji, Some("remove_reaction"), None); // Use the actual platform message_id (not origin_event_id which is a UUID) reply.reply_to = msg.message_id.clone(); - self.dispatch_reply(&reply).await; + let _ = self.dispatch_reply(&reply).await?; Ok(()) } @@ -203,7 +225,7 @@ impl ChatAdapter for UnifiedGatewayAdapter { let mut reply = self.build_reply(&msg.channel, content, Some("edit_message"), None); // Use the actual platform message_id (e.g. "draft" for streaming, or numeric for edits) reply.reply_to = msg.message_id.clone(); - self.dispatch_reply(&reply).await; + let _ = self.dispatch_reply(&reply).await?; Ok(()) } @@ -214,11 +236,18 @@ impl ChatAdapter for UnifiedGatewayAdapter { reply_to_message_id: &str, ) -> Result { let reply = self.build_reply(channel, content, None, Some(reply_to_message_id)); - self.dispatch_reply(&reply).await; + let message_id = self.dispatch_reply(&reply).await?.unwrap_or_else(|| { + format!( + "unified_{:x}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + ) + }); Ok(MessageRef { channel: channel.clone(), - message_id: format!("unified_{:x}", std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH).unwrap_or_default().as_nanos()), + message_id, }) } @@ -247,3 +276,35 @@ impl ChatAdapter for UnifiedGatewayAdapter { platform == "telegram" && self.gw_state.telegram_rich_messages } } + +#[cfg(all(test, feature = "googlechat"))] +mod tests { + use super::*; + use openab_gateway::adapters::googlechat::GoogleChatAdapter; + use tokio::sync::broadcast; + + #[tokio::test] + async fn googlechat_send_failure_propagates_in_unified_mode() { + let (event_tx, _event_rx) = broadcast::channel(4); + let mut state = AppState::test_default(event_tx); + // No credential source: delivery fails before any network access. + state.google_chat = Some(GoogleChatAdapter::new(None, None, None)); + let adapter = UnifiedGatewayAdapter::new(Arc::new(state)); + let channel = ChannelRef { + platform: "googlechat".into(), + channel_id: "spaces/TEST".into(), + thread_id: None, + parent_id: None, + origin_event_id: Some("evt_test".into()), + }; + + let err = adapter + .send_message(&channel, "hello") + .await + .expect_err("unified mode must not synthesize success after delivery failure"); + assert!( + err.to_string().contains("no credentials configured"), + "{err}" + ); + } +} From b298e0a8f6dec2e7f8b3ea35618c104066d5c8a4 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Fri, 4 Sep 2026 15:58:00 -0400 Subject: [PATCH 09/12] fix(googlechat): include queue wait in delivery deadline Close the final review gaps in standalone Google Chat delivery: - spawn Google Chat handling immediately from the WebSocket receive loop so replies no longer wait behind an inline send while core's ack clock runs - serialize actual sends with an adapter-owned delivery mutex, starting the absolute 30s deadline before lock acquisition so queue + token + chunks are all inside the 35s core acknowledgement window - replace the context-free outer timeout with phase/chunk-aware deadline waits; queue, token, and partial-chunk timeout errors now retain delivery context - add queue-wait and delayed partial-timeout regressions - classify every PR-added loopback-network/filesystem test as ignored integration and run all ten explicitly in validation Validated on macmini: workspace all-feature check, gateway/unified clippy with -D warnings, 324 normal gateway tests + 10 ignored integrations, and unified failure propagation regression all pass. --- .../openab-gateway/src/adapters/googlechat.rs | 194 ++++++++++++++---- crates/openab-gateway/src/lib.rs | 18 +- 2 files changed, 165 insertions(+), 47 deletions(-) diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index a3d0cbbe0..842e3d9d5 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -7,7 +7,7 @@ use jsonwebtoken::{decode, Algorithm, DecodingKey, Validation}; use serde::Deserialize; use std::sync::Arc; use std::time::Instant; -use tokio::sync::RwLock; +use tokio::sync::{Mutex, RwLock}; use tracing::{error, info, warn}; pub const GOOGLE_CHAT_API_BASE: &str = "https://chat.googleapis.com/v1"; @@ -29,28 +29,13 @@ const MEDIA_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_sec const TOKEN_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); /// Bound each Google Chat API mutation independently. The enclosing delivery /// deadline below is stricter than core's 35-second acknowledgement window and -/// covers token resolution plus every sequential chunk. +/// covers serialization queue wait, token resolution, and every sequential chunk. const CHAT_API_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); /// End-to-end budget for one outbound Google Chat reply. Core waits 35 seconds /// for an acknowledged gateway reply; finishing within 30 seconds leaves room /// for response serialization and WebSocket/broadcast scheduling. const GOOGLE_CHAT_DELIVERY_DEADLINE: std::time::Duration = std::time::Duration::from_secs(30); - -async fn within_delivery_deadline( - deadline: std::time::Duration, - future: F, -) -> Result -where - F: std::future::Future>, -{ - tokio::time::timeout(deadline, future).await.map_err(|_| { - format!( - "googlechat delivery timed out after {}s", - deadline.as_secs() - ) - })? -} /// Cap on text file attachments per message (matches Discord/Slack). const TEXT_FILE_COUNT_CAP: usize = 5; /// Cap on aggregate text file bytes per message (matches Discord/Slack 1 MB). @@ -367,6 +352,10 @@ pub struct GoogleChatAdapter { pub metadata_source: Option, pub access_token: Option, pub jwt_verifier: Option, + /// Serializes normal sends. Google Chat enforces a per-space write quota; + /// every caller starts its deadline before waiting on this lock so queueing + /// cannot outlive core's acknowledgement window. + delivery_lock: Mutex<()>, pub client: reqwest::Client, pub api_base: String, } @@ -488,6 +477,7 @@ impl GoogleChatAdapter { metadata_source: None, access_token, jwt_verifier, + delivery_lock: Mutex::new(()), client: reqwest::Client::new(), api_base: GOOGLE_CHAT_API_BASE.into(), } @@ -573,27 +563,47 @@ impl GoogleChatAdapter { } /// Deliver one normal Google Chat reply and return the created message - /// resource name. Both the standalone gateway acknowledgement path and the - /// in-process unified adapter call this method, so transport failures have - /// identical semantics instead of being observable only over WebSocket. + /// resource name. The absolute deadline starts before waiting for the + /// serialization lock, so queueing + token resolution + every chunk all fit + /// inside core's acknowledgement window. pub async fn deliver_message(&self, reply: &GatewayReply) -> Result { - within_delivery_deadline( - GOOGLE_CHAT_DELIVERY_DEADLINE, - self.deliver_message_inner(reply), + self.deliver_message_before( + reply, + tokio::time::Instant::now() + GOOGLE_CHAT_DELIVERY_DEADLINE, ) .await } - async fn deliver_message_inner(&self, reply: &GatewayReply) -> Result { + async fn deliver_message_before( + &self, + reply: &GatewayReply, + deadline: tokio::time::Instant, + ) -> Result { + let _delivery_guard = tokio::time::timeout_at(deadline, self.delivery_lock.lock()) + .await + .map_err(|_| { + format!( + "googlechat delivery queue timed out after {}s before sending any chunk", + GOOGLE_CHAT_DELIVERY_DEADLINE.as_secs() + ) + })?; + self.deliver_message_inner(reply, deadline).await + } + + async fn deliver_message_inner( + &self, + reply: &GatewayReply, + deadline: tokio::time::Instant, + ) -> Result { info!( space = %reply.channel.id, thread_id = ?reply.channel.thread_id, "gateway → googlechat" ); - let token = self - .get_token() + let token = tokio::time::timeout_at(deadline, self.get_token()) .await + .map_err(|_| "googlechat token resolution timed out before sending any chunk".to_string())? .ok_or_else(|| "no credentials configured".to_string())?; let chunks = split_text(&reply.content.text, GOOGLE_CHAT_MESSAGE_LIMIT); if chunks.is_empty() { @@ -603,16 +613,27 @@ impl GoogleChatAdapter { let total = chunks.len(); let mut first_message_name = None; for (index, chunk) in chunks.into_iter().enumerate() { - match send_message( - &self.client, - &token, - &reply.channel.id, - reply.channel.thread_id.as_deref(), - chunk, - &self.api_base, + let result = tokio::time::timeout_at( + deadline, + send_message( + &self.client, + &token, + &reply.channel.id, + reply.channel.thread_id.as_deref(), + chunk, + &self.api_base, + ), ) .await - { + .map_err(|_| { + format!( + "chunk {}/{} timed out after {} successful chunk(s)", + index + 1, + total, + index + ) + })?; + match result { Ok(name) => { if first_message_name.is_none() { first_message_name = Some(name); @@ -2419,6 +2440,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn metadata_token_source_mints_chat_bot_token() { use wiremock::matchers::{header, method, path, path_regex}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2475,6 +2497,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn metadata_token_source_rejects_self_impersonation_before_token_request() { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2575,6 +2598,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn adc_refresh_failure_cooldown_prevents_queued_retry_storm() { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2710,6 +2734,7 @@ mod tests { } #[test] + #[ignore = "integration: exercises filesystem error handling"] fn from_parts_unreadable_key_file_with_use_adc_installs_adc() { // Regression: an unreadable/absent key FILE also yields token_cache=None // and must not suppress ADC. @@ -2819,14 +2844,37 @@ mod tests { } #[tokio::test] - async fn delivery_deadline_cancels_hung_operation() { - let err = within_delivery_deadline( - std::time::Duration::from_millis(1), - std::future::pending::>(), - ) - .await - .expect_err("pending delivery must time out"); - assert!(err.contains("delivery timed out"), "{err}"); + async fn delivery_deadline_includes_wait_for_serialization_lock() { + let adapter = GoogleChatAdapter::new(None, Some("token".into()), None); + let held = adapter.delivery_lock.lock().await; + let reply = GatewayReply { + schema: "openab.gateway.reply.v1".into(), + reply_to: "orig".into(), + platform: "googlechat".into(), + channel: ReplyChannel { + id: "spaces/TEST".into(), + thread_id: None, + }, + content: Content { + content_type: "text".into(), + attachments: vec![], + text: "hello".into(), + }, + command: None, + request_id: Some("req_queue".into()), + quote_message_id: None, + }; + + let err = adapter + .deliver_message_before( + &reply, + tokio::time::Instant::now() + std::time::Duration::from_millis(1), + ) + .await + .expect_err("queued delivery must time out before the held lock is released"); + drop(held); + assert!(err.contains("delivery queue timed out"), "{err}"); + assert!(err.contains("before sending any chunk"), "{err}"); } #[tokio::test] @@ -2837,6 +2885,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn adc_failure_does_not_fall_back_to_whitespace_static_token() { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2873,6 +2922,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn adc_takes_precedence_over_static_access_token() { use wiremock::matchers::{method, path, path_regex}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2925,6 +2975,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn metadata_token_source_rejects_blank_minted_token() { use wiremock::matchers::{method, path, path_regex}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -2975,6 +3026,7 @@ mod tests { } #[tokio::test] + #[ignore = "integration: uses a local HTTP server"] async fn handle_reply_edit_message_ignores_synthetic_unified_id() { use wiremock::matchers::method; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -3624,6 +3676,64 @@ mod tests { assert!(err.contains("500"), "{err}"); } + #[tokio::test] + #[ignore = "integration: delayed local HTTP response validates partial-timeout context"] + async fn multi_chunk_timeout_reports_successful_chunk_count() { + use wiremock::matchers::{method, path_regex}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path_regex("/spaces/.*/messages")) + .respond_with(ResponseTemplate::new(200).set_body_json( + serde_json::json!({"name": "spaces/TEST/messages/first_chunk"}), + )) + .up_to_n_times(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path_regex("/spaces/.*/messages")) + .respond_with( + ResponseTemplate::new(200) + .set_delay(std::time::Duration::from_millis(250)) + .set_body_json( + serde_json::json!({"name": "spaces/TEST/messages/late_chunk"}), + ), + ) + .mount(&server) + .await; + + let mut adapter = GoogleChatAdapter::new(None, Some("fake-token".into()), None); + adapter.api_base = server.uri(); + let reply = GatewayReply { + schema: "openab.gateway.reply.v1".into(), + reply_to: "orig".into(), + platform: "googlechat".into(), + channel: ReplyChannel { + id: "spaces/TEST".into(), + thread_id: None, + }, + content: Content { + content_type: "text".into(), + attachments: vec![], + text: "x".repeat(5000), + }, + command: None, + request_id: Some("req_partial_timeout".into()), + quote_message_id: None, + }; + + let err = adapter + .deliver_message_before( + &reply, + tokio::time::Instant::now() + std::time::Duration::from_millis(75), + ) + .await + .expect_err("second chunk must exceed the absolute deadline"); + assert!(err.contains("chunk 2/2 timed out"), "{err}"); + assert!(err.contains("after 1 successful chunk(s)"), "{err}"); + } + // --- Attachment parsing tests --- fn make_attachment( diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index c23f099be..adb2811bc 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -1077,11 +1077,19 @@ async fn handle_oab_connection(state: Arc, socket: axum::extract::ws:: } #[cfg(feature = "googlechat")] "googlechat" => { - if let Some(ref gc) = state_for_recv.google_chat { - gc.handle_reply(&reply, &state_for_recv.event_tx).await; - } else { - warn!("reply for googlechat but adapter not configured"); - } + // Do not await delivery inline: core's ack timer is already + // running, and the receive loop must keep draining replies. + // GoogleChatAdapter serializes sends internally; each spawned + // task's deadline includes its wait for that delivery lock. + let state = state_for_recv.clone(); + let reply = reply.clone(); + tokio::spawn(async move { + if let Some(ref gc) = state.google_chat { + gc.handle_reply(&reply, &state.event_tx).await; + } else { + warn!("reply for googlechat but adapter not configured"); + } + }); } #[cfg(feature = "wecom")] "wecom" => { From f59cbf9c09a7eadf1a90c5b0f7bbfab10ace3084 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:46:45 +0000 Subject: [PATCH 10/12] fix(googlechat): address group review nits --- config.toml.example | 1 + crates/openab-core/src/config.rs | 14 +++---- crates/openab-core/src/gateway.rs | 39 ++++++----------- .../openab-gateway/src/adapters/googlechat.rs | 42 ++++++++++++++++--- crates/openab-gateway/src/lib.rs | 30 +------------ docs/config-reference.md | 4 ++ docs/google-chat.md | 2 +- src/unified_adapter.rs | 41 ++++++++++-------- 8 files changed, 86 insertions(+), 87 deletions(-) diff --git a/config.toml.example b/config.toml.example index 1a2bc54c9..e5cf7df3d 100644 --- a/config.toml.example +++ b/config.toml.example @@ -125,6 +125,7 @@ allowed_channels = ["1234567890"] # ↑ omitted + non-empty list → auto- # access_token = "${GOOGLE_CHAT_ACCESS_TOKEN}" # static token alternative # use_adc = true # keyless ADC via GCE metadata + IAM Credentials; runtime SA impersonates a DISTINCT target (self-impersonation is prohibited). env: GOOGLE_CHAT_USE_ADC # adc_target_service_account = "chat-bot@project.iam.gserviceaccount.com" # required with use_adc; env: GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT +# Auth precedence: successfully loaded SA key > ADC > static access_token. # audience = "projects//..." # enables webhook JWT verification (L1) # webhook_path = "/webhook/googlechat" # env fallback: GOOGLE_CHAT_WEBHOOK_PATH # allow_all_users = false # env fallback: GOOGLE_CHAT_ALLOW_ALL_USERS diff --git a/crates/openab-core/src/config.rs b/crates/openab-core/src/config.rs index 7c744ecf0..fdd89dcb3 100644 --- a/crates/openab-core/src/config.rs +++ b/crates/openab-core/src/config.rs @@ -823,11 +823,11 @@ impl TelegramConfig { } } -/// `true` when env var == "1" or "true" (case-insensitive); default `false`. -/// Matches the legacy `TELEGRAM_TRUSTED_SOURCE_ONLY` semantics. +/// `true` when the trimmed env var is "1" or "true" (case-insensitive); +/// default `false`. Matches the legacy `TELEGRAM_TRUSTED_SOURCE_ONLY` semantics. fn env_flag_true_one(key: &str) -> bool { std::env::var(key) - .map(|v| v == "1" || v.eq_ignore_ascii_case("true")) + .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) .unwrap_or(false) } @@ -1271,11 +1271,9 @@ impl GoogleChatConfig { sa_key_json: opt_str(&self.sa_key_json, "GOOGLE_CHAT_SA_KEY_JSON"), sa_key_file: opt_str(&self.sa_key_file, "GOOGLE_CHAT_SA_KEY_FILE"), access_token: opt_str(&self.access_token, "GOOGLE_CHAT_ACCESS_TOKEN"), - use_adc: self.use_adc.unwrap_or_else(|| { - std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) - .unwrap_or(false) - }), + use_adc: self + .use_adc + .unwrap_or_else(|| env_flag_true_one("GOOGLE_CHAT_USE_ADC")), adc_target_service_account: opt_str( &self.adc_target_service_account, "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index bd704c5e0..e63416b56 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -44,7 +44,6 @@ fn platform_acks_writes(platform: &str) -> bool { EDIT_RESPONSE_PLATFORMS.contains(&platform) } - /// Platforms whose gateway adapters acknowledge normal send replies with a /// `GatewayResponse`. This capability is independent of cosmetic streaming: /// Google Chat is send-once, but its adapter reports API/auth failures and core @@ -60,32 +59,21 @@ fn platform_acks_replies(platform: &str) -> bool { fn reply_requires_ack(platform: &str, streaming: bool) -> bool { streaming || platform_acks_replies(platform) } -/// Gateway platforms whose messaging API cannot edit a message after it is sent. + +/// Platforms where cosmetic (typewriter) streaming is not viable, so replies +/// are forced send-once regardless of the configured `streaming` flag. /// -/// Cosmetic (typewriter) streaming works by posting a placeholder and then -/// repeatedly editing it in place with the growing text. On a platform with no -/// edit endpoint, each of those "edits" is delivered as a brand-new message -/// instead — so the user sees the same reply posted several times, each copy -/// longer than the last. Streaming is therefore force-disabled (send-once) for -/// these platforms regardless of the configured `streaming` flag. +/// Cosmetic streaming posts a placeholder and repeatedly edits it with growing +/// text. Without a usable edit path, those updates become duplicate messages: /// -/// LINE's Messaging API only exposes reply/push (no edit), so it lives here. -/// (The in-process unified adapter additionally hard-drops stray edit_message -/// commands in the LINE adapter itself — see `dispatch_line_reply`.) +/// - `line` / `lineworks`: no message-edit API. +/// - `googlechat`: editing requires a real message resource name, but unified +/// fallback IDs are synthetic; its per-space quota also makes rapid edits +/// unsuitable. See . /// -/// NOTE: like `EDIT_RESPONSE_PLATFORMS`, this is platform-identity standing in -/// for a *capability*. The right long-term model is a capability handshake at -/// gateway-connect time ("can this adapter edit messages?"); until that exists, -/// any new gateway platform that lacks a message-edit API MUST be added here. -/// Platforms where cosmetic (typewriter) streaming — a placeholder message -/// then rapid in-place edits — is not viable, so replies are forced send-once: -/// - `line` / `lineworks`: no message-edit API at all. -/// - `googlechat`: has an edit API, but the unified adapter's synthetic -/// `unified_` message id is not a valid resource name, so `patch` -/// rejects it with 400 INVALID_ARGUMENT before any edit applies; the -/// documented 1 write/sec-per-space quota (create + patch + delete -/// combined) further constrains high-frequency editing. -/// See . +/// NOTE: like `EDIT_RESPONSE_PLATFORMS`, this is platform identity standing in +/// for a capability. Replace it with a negotiated capability when available; +/// until then, add every platform that cannot support cosmetic edits here. const NON_STREAMING_PLATFORMS: &[&str] = &["line", "lineworks", "googlechat"]; /// Whether cosmetic streaming (placeholder + in-place edits) is possible on @@ -256,8 +244,7 @@ fn gateway_delivery_result(resp: GatewayResponse) -> Result { } else { Err(anyhow::anyhow!( "gateway reported failure: {}", - resp.error - .unwrap_or_else(|| "gateway reported failure".to_string()) + resp.error.unwrap_or_else(|| "unspecified error".to_string()) )) } } diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index 842e3d9d5..dfb97aa67 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -331,6 +331,12 @@ fn normalize_adc_target_service_account(value: Option) -> Result bool { + let safe_segment = |segment: &str| { + !segment.is_empty() + && !segment + .bytes() + .any(|b| matches!(b, b'?' | b'#' | b'&' | b'=' | b'%')) + }; let mut parts = name.split('/'); matches!( ( @@ -341,7 +347,7 @@ fn is_google_chat_message_name(name: &str) -> bool { parts.next(), ), (Some("spaces"), Some(space), Some("messages"), Some(message), None) - if !space.is_empty() && !message.is_empty() + if safe_segment(space) && safe_segment(message) ) } @@ -349,12 +355,12 @@ fn is_google_chat_message_name(name: &str) -> bool { pub struct GoogleChatAdapter { pub token_cache: Option, - pub metadata_source: Option, - pub access_token: Option, + pub(crate) metadata_source: Option, + pub(crate) access_token: Option, pub jwt_verifier: Option, - /// Serializes normal sends. Google Chat enforces a per-space write quota; - /// every caller starts its deadline before waiting on this lock so queueing - /// cannot outlive core's acknowledgement window. + /// Conservatively serializes normal sends across the adapter. Google Chat's + /// quota is per-space; every caller starts its deadline before waiting on + /// this lock so the current coarse queue cannot outlive core's ack window. delivery_lock: Mutex<()>, pub client: reqwest::Client, pub api_base: String, @@ -375,6 +381,27 @@ pub(crate) struct GoogleChatParts { pub adc_target_service_account: Option, } +impl GoogleChatParts { + /// Read the env-only credential contract once so standalone constructors + /// cannot drift on ADC parsing or target trimming. + pub(crate) fn from_env() -> Self { + Self { + sa_key_json: std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), + sa_key_file: std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), + access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), + audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), + use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") + .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) + .unwrap_or(false), + adc_target_service_account: std::env::var( + "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", + ) + .ok() + .filter(|s| !s.trim().is_empty()), + } + } +} + impl GoogleChatAdapter { /// Build an adapter from resolved parts (#1379): SA key JSON (inline wins /// over file path), optional static access token, optional JWT audience, @@ -2919,6 +2946,9 @@ mod tests { assert!(!is_google_chat_message_name("spaces/SP")); assert!(!is_google_chat_message_name("spaces/SP/messages/")); assert!(!is_google_chat_message_name("spaces/SP/messages/msg1/extra")); + assert!(!is_google_chat_message_name("spaces/SP/messages/msg1?x=1")); + assert!(!is_google_chat_message_name("spaces/SP/messages/msg1#fragment")); + assert!(!is_google_chat_message_name("spaces/SP/messages/msg%2F1")); } #[tokio::test] diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index adb2811bc..1653a4d10 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -195,20 +195,7 @@ impl AppState { .unwrap_or(false); if enabled { Some(adapters::googlechat::GoogleChatAdapter::from_parts( - adapters::googlechat::GoogleChatParts { - sa_key_json: std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), - sa_key_file: std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), - access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), - audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), - use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) - .unwrap_or(false), - adc_target_service_account: std::env::var( - "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", - ) - .ok() - .filter(|s| !s.trim().is_empty()), - }, + adapters::googlechat::GoogleChatParts::from_env(), )) } else { None @@ -764,20 +751,7 @@ pub async fn serve(config: ServeConfig) -> anyhow::Result<()> { info!(path = %googlechat_webhook_path, "googlechat adapter enabled"); app = app.route(&googlechat_webhook_path, post(adapters::googlechat::webhook)); Some(adapters::googlechat::GoogleChatAdapter::from_parts( - adapters::googlechat::GoogleChatParts { - sa_key_json: std::env::var("GOOGLE_CHAT_SA_KEY_JSON").ok(), - sa_key_file: std::env::var("GOOGLE_CHAT_SA_KEY_FILE").ok(), - access_token: std::env::var("GOOGLE_CHAT_ACCESS_TOKEN").ok(), - audience: std::env::var("GOOGLE_CHAT_AUDIENCE").ok(), - use_adc: std::env::var("GOOGLE_CHAT_USE_ADC") - .map(|v| v.trim() == "1" || v.trim().eq_ignore_ascii_case("true")) - .unwrap_or(false), - adc_target_service_account: std::env::var( - "GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT", - ) - .ok() - .filter(|s| !s.trim().is_empty()), - }, + adapters::googlechat::GoogleChatParts::from_env(), )) } else { None diff --git a/docs/config-reference.md b/docs/config-reference.md index 6a6d9033b..423f76541 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -232,6 +232,10 @@ Full first-class Google Chat section (config-first parity, #1379) — credential | `allow_all_users` | bool \| omit | `false` (deny-all) | Env: `GOOGLE_CHAT_ALLOW_ALL_USERS`. | | `allowed_users` | string[] | `[]` | User resource names (`users/`). Env: `GOOGLE_CHAT_ALLOWED_USERS`. | +Outbound auth precedence is: a successfully loaded service-account key, then +ADC, then the configured static `access_token`. ADC mint failures may fall back +to that static token and log a possible identity switch. + For GKE Workload Identity, set Helm value `agents..gateway.serviceAccountName` to an existing annotated Kubernetes ServiceAccount for the runtime GSA. An empty value falls back to diff --git a/docs/google-chat.md b/docs/google-chat.md index d4f044868..71ab2d200 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -268,7 +268,7 @@ Each field falls back to its `GOOGLE_CHAT_ALLOW_ALL_USERS` / `GOOGLE_CHAT_ALLOWE | `GOOGLE_CHAT_SA_KEY_JSON` | No | — | Service account key JSON string (enables auto-refresh) | | `GOOGLE_CHAT_SA_KEY_FILE` | No | — | Path to service account key JSON file (alternative to `SA_KEY_JSON`) | | `GOOGLE_CHAT_ACCESS_TOKEN` | No | — | Static OAuth2 access token (fallback, expires in 1 hour) | -| `GOOGLE_CHAT_USE_ADC` | No | `false` | Enable keyless ADC: attached runtime SA impersonates a distinct Chat-app target via IAM Credentials — see Option C | +| `GOOGLE_CHAT_USE_ADC` | No | `false` | Set to `true` or `1` to enable keyless ADC: attached runtime SA impersonates a distinct Chat-app target via IAM Credentials — see Option C | | `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT` | With ADC | — | Email of the dedicated Chat-app SA to impersonate. Required when `USE_ADC=true`; MUST differ from the runtime SA | | `GOOGLE_CHAT_WEBHOOK_PATH` | No | `/webhook/googlechat` | Webhook endpoint path | diff --git a/src/unified_adapter.rs b/src/unified_adapter.rs index 92344482c..e34e5fb27 100644 --- a/src/unified_adapter.rs +++ b/src/unified_adapter.rs @@ -10,6 +10,16 @@ use std::collections::HashMap; use std::sync::Arc; use tokio::sync::Mutex; +fn synthetic_unified_id() -> String { + format!( + "unified_{:x}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + ) +} + pub struct UnifiedGatewayAdapter { pub gw_state: Arc, /// Telegram reaction state (message_id -> emoji list) for add/remove_reaction @@ -81,6 +91,11 @@ impl UnifiedGatewayAdapter { gc.handle_reply(reply, &self.gw_state.event_tx).await; } else if reply.command.is_none() { return Err(anyhow!("googlechat adapter is not configured")); + } else { + tracing::warn!( + command = ?reply.command.as_deref(), + "googlechat command dropped: adapter is not configured" + ); } } #[cfg(feature = "wecom")] @@ -172,15 +187,10 @@ impl ChatAdapter for UnifiedGatewayAdapter { async fn send_message(&self, channel: &ChannelRef, content: &str) -> Result { let reply = self.build_reply(channel, content, None, None); - let message_id = self.dispatch_reply(&reply).await?.unwrap_or_else(|| { - format!( - "unified_{:x}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos() - ) - }); + let message_id = self + .dispatch_reply(&reply) + .await? + .unwrap_or_else(synthetic_unified_id); Ok(MessageRef { channel: channel.clone(), message_id, @@ -236,15 +246,10 @@ impl ChatAdapter for UnifiedGatewayAdapter { reply_to_message_id: &str, ) -> Result { let reply = self.build_reply(channel, content, None, Some(reply_to_message_id)); - let message_id = self.dispatch_reply(&reply).await?.unwrap_or_else(|| { - format!( - "unified_{:x}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos() - ) - }); + let message_id = self + .dispatch_reply(&reply) + .await? + .unwrap_or_else(synthetic_unified_id); Ok(MessageRef { channel: channel.clone(), message_id, From 3771053f4cbe32107b8b872db25b4ee8bda54c87 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:09:55 +0000 Subject: [PATCH 11/12] fix(googlechat): resolve frozen review blockers --- charts/openab/README.md | 2 +- charts/openab/templates/gateway.yaml | 3 +- .../tests/gateway_serviceaccount_test.yaml | 10 +- charts/openab/values.yaml | 5 +- .../openab-gateway/src/adapters/googlechat.rs | 131 +++++++++++++----- crates/openab-gateway/src/lib.rs | 81 +++++++++-- docs/config-reference.md | 7 +- docs/google-chat.md | 7 +- 8 files changed, 185 insertions(+), 61 deletions(-) diff --git a/charts/openab/README.md b/charts/openab/README.md index e7244cd87..2bcf05788 100644 --- a/charts/openab/README.md +++ b/charts/openab/README.md @@ -48,7 +48,7 @@ Each agent lives under `agents.`. | `stt.baseUrl` | STT API base URL. | `"https://api.groq.com/openai/v1"` | | `gateway.enabled` | Enable the gateway config block for webhook-based platforms. | `false` | | `gateway.deploy` | Deploy the gateway Deployment and Service. | `true` | -| `gateway.serviceAccountName` | Existing ServiceAccount attached to the gateway pod. Empty falls back to the per-agent `serviceAccountName`, then the chart-global value. Required for GKE Workload Identity when `gateway.googleChat.useAdc=true`; the chart does not create or annotate the ServiceAccount. | `""` | +| `gateway.serviceAccountName` | Existing ServiceAccount attached to the gateway pod. Empty preserves the Kubernetes default identity and does not inherit per-agent or chart-global values. Required for GKE Workload Identity when `gateway.googleChat.useAdc=true`; the chart does not create or annotate the ServiceAccount. | `""` | | `cron.usercronEnabled` | Enable user-provided cron configuration. | `false` | | `cronjobs` | Config-driven scheduled messages for an agent. | `[]` | | `persistence.enabled` | Enable persistent storage for auth and settings. | `true` | diff --git a/charts/openab/templates/gateway.yaml b/charts/openab/templates/gateway.yaml index c8dd95b04..7f45c6900 100644 --- a/charts/openab/templates/gateway.yaml +++ b/charts/openab/templates/gateway.yaml @@ -4,8 +4,7 @@ {{- $gwCfg := omit $cfg "nameOverride" }} {{- $d := dict "ctx" $ "agent" (printf "%s-gateway" $name) "cfg" $gwCfg }} {{- $agentD := dict "ctx" $ "agent" $name "cfg" $cfg }} -{{- $agentSvcAcct := default $.Values.serviceAccountName $cfg.serviceAccountName }} -{{- $gatewaySvcAcct := default $agentSvcAcct (($cfg.gateway).serviceAccountName) }} +{{- $gatewaySvcAcct := (($cfg.gateway).serviceAccountName) }} {{- $hasTeams := and (($cfg.gateway).teams).appId (($cfg.gateway).teams).appSecret }} {{- $hasTelegram := (($cfg.gateway).telegram).botToken }} {{- $hasLine := (($cfg.gateway).line).channelSecret }} diff --git a/charts/openab/tests/gateway_serviceaccount_test.yaml b/charts/openab/tests/gateway_serviceaccount_test.yaml index a63b1930d..2eea62b55 100644 --- a/charts/openab/tests/gateway_serviceaccount_test.yaml +++ b/charts/openab/tests/gateway_serviceaccount_test.yaml @@ -13,24 +13,22 @@ tests: - notExists: path: spec.template.spec.serviceAccountName - - it: falls back to the chart-global serviceAccountName + - it: does not inherit the chart-global serviceAccountName set: serviceAccountName: global-runtime documentIndex: 0 asserts: - - equal: + - notExists: path: spec.template.spec.serviceAccountName - value: global-runtime - - it: falls back to the per-agent serviceAccountName before the global value + - it: does not inherit per-agent or global serviceAccountName set: serviceAccountName: global-runtime agents.kiro.serviceAccountName: agent-runtime documentIndex: 0 asserts: - - equal: + - notExists: path: spec.template.spec.serviceAccountName - value: agent-runtime - it: gateway serviceAccountName overrides per-agent and global values set: diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index 9854240fa..218e3b3c0 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -395,8 +395,9 @@ agents: gateway: enabled: false # set to true + provide url to enable the [gateway] config block deploy: true # set to false to skip Gateway Deployment/Service (config-only mode) - # Existing Kubernetes ServiceAccount for the gateway pod. Empty falls back - # to agents..serviceAccountName, then chart-global serviceAccountName. + # Existing Kubernetes ServiceAccount for the gateway pod. Empty omits + # serviceAccountName and preserves the Kubernetes default identity; it + # does not inherit per-agent or chart-global serviceAccountName values. # Required on GKE when googleChat.useAdc uses Workload Identity. serviceAccountName: "" url: "" # e.g. ws://openab-gateway:8080/ws diff --git a/crates/openab-gateway/src/adapters/googlechat.rs b/crates/openab-gateway/src/adapters/googlechat.rs index dfb97aa67..03866cdb0 100644 --- a/crates/openab-gateway/src/adapters/googlechat.rs +++ b/crates/openab-gateway/src/adapters/googlechat.rs @@ -402,6 +402,34 @@ impl GoogleChatParts { } } +/// Emit one correlated normal-reply result when core supplied a request id. +/// Missing-adapter paths use the same contract as configured delivery failures, +/// so core never waits for an acknowledgement that cannot arrive. +pub(crate) fn emit_delivery_response( + event_tx: &tokio::sync::broadcast::Sender, + request_id: &Option, + result: Result, +) { + let Some(request_id) = request_id else { + return; + }; + let (success, message_id, error) = match result { + Ok(name) => (true, Some(name), None), + Err(error) => (false, None, Some(error)), + }; + let response = crate::schema::GatewayResponse { + schema: "openab.gateway.response.v1".into(), + request_id: request_id.clone(), + success, + thread_id: None, + message_id, + error, + }; + if let Ok(json) = serde_json::to_string(&response) { + let _ = event_tx.send(json); + } +} + impl GoogleChatAdapter { /// Build an adapter from resolved parts (#1379): SA key JSON (inline wins /// over file path), optional static access token, optional JWT audience, @@ -716,22 +744,8 @@ impl GoogleChatAdapter { } let result = self.deliver_message(reply).await; - if let Some(ref req_id) = reply.request_id { - let (success, message_id, error) = match result { - Ok(name) => (true, Some(name), None), - Err(e) => (false, None, Some(e)), - }; - let resp = crate::schema::GatewayResponse { - schema: "openab.gateway.response.v1".into(), - request_id: req_id.clone(), - success, - thread_id: None, - message_id, - error, - }; - if let Ok(json) = serde_json::to_string(&resp) { - let _ = event_tx.send(json); - } + if reply.request_id.is_some() { + emit_delivery_response(event_tx, &reply.request_id, result); } else if let Err(e) = result { error!(error = %e, "googlechat reply delivery failed without acknowledgement id"); } @@ -1042,6 +1056,8 @@ impl GoogleChatTokenCache { .and_then(|v| v.as_str()) .ok_or("missing private_key in SA key")? .to_string(); + jsonwebtoken::EncodingKey::from_rsa_pem(pkey.as_bytes()) + .map_err(|e| format!("invalid private_key in SA key: {e}"))?; Ok(Self { token: RwLock::new(None), sa_email: email, @@ -2425,6 +2441,12 @@ mod tests { // --- Token cache tests --- + const VALID_SA_KEY_JSON: &str = r#"{ + "type": "service_account", + "client_email": "test@test.iam.gserviceaccount.com", + "private_key": "-----BEGIN PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDdcQN1uyXnPVop\n76hhCUfZswb7/2AAqHPiikNp8Kkxx5R9Up/vGCEJDxbZ0iZy/oPVs27Fa3LtM+9D\n3p9qpbQLZLw3AhK3R/VoS4Ex0p6EkvcxqfQ4OolaJlDG+/myFEuZLqEs164DFUIJ\ntr4dygVdU+jFy6A+h7Q5T1QXArG4pR6Ap7DuBJhgEtvPqAn6U7JxAMEELcAZbhHU\nARjfiks2uFMaew1DKrH1qLzIj159ZmhgrXM/4Vc1lSgKMYyqUN3Kued05/DXd2rd\ngfpX7af9tyYsD6Lh1bwREJ4wDOYSewUZxj35mhZhdxHDlZa8wzXQdN8Z04eUghEL\nA+9whYa9AgMBAAECggEAEB0mNE3/Dxmu0vhml1EWkmftrS7DLKkVbbnD+BSUK1Qr\noQb/LmXiGYeokQcy1xFgRI+/Esyj21D5K0Yq8ZbHSED3VUVoWT/6QrGj4B1Efb84\nD7wfUmwoDJBXNnOlkujZK3dyMRIsznqgiJZstTw7MbRmbuZHbeVwHu9/3gDLL/Vp\nXET6KvhGbe5ssmBAefQhOsB+L0aKh4h0HuJdeD+Q7Xhc9gTKRZBwWs6xJgCUqLCr\n5XI7MqjMGDmDN8CImos8zOHCXCYJvIj5BHT1iVkdYn+VmeSDCh+hFcTxmVnSoYf9\nf7G14C160QeEnl0gGxE3yMqMvhI9tb9nXR9vRZjqnQKBgQD18YdRsZLhi9ZnZzXI\nMs1/BCslDnka6yt6q0jYlkzRn/mCyg9QzyFeQpD1fyoWVclRKnYbJygvA90ZkHCN\nXxn7M+0yYWxO9hF+ghK/bhGsVQE/nElLLumt5P8JH0rKB/NvTHpjvM4Q26A23mMp\n9/46gWEHpeIhJL852k2H+a9fZwKBgQDmfwEbVW9wLNDmYaG3z4su7B/n4DHWZrt/\nF5+XziZiEza2NIVFsa03ZaFPHRIK1m6ExDFnnbBd34CU0o3SMsxrTj92VEDb4LdF\nw9foEZnq0wMWT6y9qCZysYQCIJ/4Uy3Un98sbXc1HBJZ9lWq3baaeDttxvyhifNq\nPsmZtPzmOwKBgC3VDceelOWtPo5UgIRHW15BM50bPlxS2O5qPxAFqlkiO8gwyXvg\nrbI4K3Vkdj5lTDfw9sOGn4lraeeasC7YOypB+gD6gMmSN55gtQexhl+cE7h78nit\nTGTYmOJlT3Wo16e1E9XEWI5xr0CqXsZybZEPjTp0olhU1cH9OZeOYy0fAoGAUjp0\n1p+ABfC3BblGzCBKcw7hwwMERIyZzxlKYgm1P7/DAPVzpg1g0iZ7iZHBYgRloQ+s\n4F4tERAu+uiyl45vxsg/c6NTEB32w/i+CZhd5JwqucbqxS47qScTBP9Gknx6GSR/\npYXXxSailV1/6lj2T90ctmkKr0ZbhEep/B/JKQkCgYBx6jih7r21GZMrGLI4mN2H\nirN6VYrNklGBhgIBToJ9cp3iTApyr1pwYQSGAhjkA7ERS4UelhYOY6k466mBX+eR\nIUWbl2pgsjuySiq2WPEnjaI1pXoPGl5xDHVCEXH1+wUq4+iLCUuIeZ9mLRKY9xVT\nEE0jaWDa0qXlgQH+iAsJNw==\n-----END PRIVATE KEY-----\n" + }"#; + #[test] fn token_cache_rejects_invalid_json() { let result = GoogleChatTokenCache::new("not json"); @@ -2440,14 +2462,22 @@ mod tests { } #[test] - fn token_cache_accepts_valid_sa_key() { - let key = r#"{ - "type": "service_account", + fn token_cache_rejects_invalid_pem() { + let key = serde_json::json!({ "client_email": "test@test.iam.gserviceaccount.com", - "private_key": "-----BEGIN RSA PRIVATE KEY-----\nMIIBogIBAAJBALvRE+oCMiEhtfO5ufaVc9wGPUMgPGxmVFiMPC/NMxmCSiMGNO9h\nCOyByeF78QHp4gOW/lgVU8MJkv33hVMbOr0CAwEAAQJAD2k/cFR5MIkw1PFcm98K\n9MqYKGpJCmGBjFY0ek0FHoC14d/hpAGaoWMjNaAyjU/IbGv1fj8C5MfFRal0fV/L\nAQIhAP0T6FPJMm3O4bM18kMHnOP2+Y5kxMpVxCCjkVNH7D09AiEAvXEQJYwR+PFs\njDDhEm4VPmk+lKJoQlopj8TN5gQV8DECIBcXbU+LPWx4H+qRElhCB1B5a9mYmpY\nV6LFPnvSfHqNAiEAiNj5+A6E7WJ50il+5NG5yn7gXh8vNxdCYIw5qx6C2bECIBmW\nVGVRhSmNsmDMJFsGIdKJsnEXpizIVHtfpXsS4j9X\n-----END RSA PRIVATE KEY-----\n" - }"#; - let result = GoogleChatTokenCache::new(key); - assert!(result.is_ok()); + "private_key": "-----BEGIN PRIVATE KEY-----\nnot-a-real-key\n-----END PRIVATE KEY-----\n", + }) + .to_string(); + let error = match GoogleChatTokenCache::new(&key) { + Err(error) => error, + Ok(_) => panic!("expected invalid PEM to be rejected during construction"), + }; + assert!(error.contains("invalid private_key in SA key"), "{error}"); + } + + #[test] + fn token_cache_accepts_valid_sa_key() { + assert!(GoogleChatTokenCache::new(VALID_SA_KEY_JSON).is_ok()); } // --- Keyless ADC (MetadataTokenSource) tests --- @@ -2760,6 +2790,26 @@ mod tests { ); } + #[test] + fn from_parts_invalid_pem_with_use_adc_installs_adc() { + let key = serde_json::json!({ + "client_email": "sa@example.iam.gserviceaccount.com", + "private_key": "-----BEGIN PRIVATE KEY-----\nnot-a-real-key\n-----END PRIVATE KEY-----\n", + }) + .to_string(); + let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { + sa_key_json: Some(key), + use_adc: true, + adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), + ..Default::default() + }); + assert!(adapter.token_cache.is_none(), "invalid PEM must not load"); + assert!( + adapter.metadata_source.is_some(), + "invalid PEM must leave the configured ADC fallback selectable" + ); + } + #[test] #[ignore = "integration: exercises filesystem error handling"] fn from_parts_unreadable_key_file_with_use_adc_installs_adc() { @@ -2780,23 +2830,18 @@ mod tests { #[test] fn from_parts_loaded_key_suppresses_metadata_source() { - // A successfully loaded SA key wins outright at send time, so no ADC - // source is installed behind it (dead-at-runtime otherwise). - let key = serde_json::json!({ - "client_email": "sa@example.iam.gserviceaccount.com", - "private_key": "-----BEGIN PRIVATE KEY-----\nnot-a-real-key\n-----END PRIVATE KEY-----\n", - }) - .to_string(); + // A successfully loaded and PEM-validated SA key wins outright at send + // time, so no dead ADC source is installed behind it. let adapter = GoogleChatAdapter::from_parts(GoogleChatParts { - sa_key_json: Some(key), + sa_key_json: Some(VALID_SA_KEY_JSON.into()), use_adc: true, adc_target_service_account: Some("chat-bot@project.iam.gserviceaccount.com".into()), ..Default::default() }); - assert!(adapter.token_cache.is_some(), "valid key JSON → SA-key cache"); + assert!(adapter.token_cache.is_some(), "valid SA key loads cache"); assert!( adapter.metadata_source.is_none(), - "loaded SA key → ADC source not installed" + "loaded SA key suppresses ADC source" ); } @@ -3335,6 +3380,26 @@ mod tests { assert_eq!(name, Some("spaces/SP1/messages/msg123")); } + #[test] + fn emit_delivery_response_reports_missing_adapter_failure() { + let (event_tx, mut event_rx) = tokio::sync::broadcast::channel::(4); + emit_delivery_response( + &event_tx, + &Some("req_missing_adapter".into()), + Err("googlechat adapter is not configured".into()), + ); + + let response: GatewayResponse = + serde_json::from_str(&event_rx.try_recv().expect("expected failure response")).unwrap(); + assert_eq!(response.request_id, "req_missing_adapter"); + assert!(!response.success); + assert!(response.message_id.is_none()); + assert_eq!( + response.error.as_deref(), + Some("googlechat adapter is not configured") + ); + } + #[tokio::test] async fn handle_reply_sends_gateway_response_success() { use wiremock::{Mock, MockServer, ResponseTemplate}; diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index 1653a4d10..dccdda44b 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -954,6 +954,25 @@ async fn ws_handler( ws.on_upgrade(move |socket| handle_oab_connection(state, socket)) } +#[cfg(feature = "googlechat")] +fn spawn_googlechat_reply( + state: Arc, + reply: schema::GatewayReply, +) -> tokio::task::JoinHandle<()> { + tokio::spawn(async move { + if let Some(ref google_chat) = state.google_chat { + google_chat.handle_reply(&reply, &state.event_tx).await; + } else { + tracing::warn!("reply for googlechat but adapter not configured"); + adapters::googlechat::emit_delivery_response( + &state.event_tx, + &reply.request_id, + Err("googlechat adapter is not configured".into()), + ); + } + }) +} + async fn handle_oab_connection(state: Arc, socket: axum::extract::ws::WebSocket) { use axum::extract::ws::Message; use futures_util::{SinkExt, StreamExt}; @@ -1053,17 +1072,12 @@ async fn handle_oab_connection(state: Arc, socket: axum::extract::ws:: "googlechat" => { // Do not await delivery inline: core's ack timer is already // running, and the receive loop must keep draining replies. - // GoogleChatAdapter serializes sends internally; each spawned - // task's deadline includes its wait for that delivery lock. - let state = state_for_recv.clone(); - let reply = reply.clone(); - tokio::spawn(async move { - if let Some(ref gc) = state.google_chat { - gc.handle_reply(&reply, &state.event_tx).await; - } else { - warn!("reply for googlechat but adapter not configured"); - } - }); + // Dropping the JoinHandle detaches the delivery task; its deadline + // includes any wait for the adapter's delivery lock. + let _ = spawn_googlechat_reply( + state_for_recv.clone(), + reply.clone(), + ); } #[cfg(feature = "wecom")] "wecom" => { @@ -1115,6 +1129,51 @@ async fn handle_oab_connection(state: Arc, socket: axum::extract::ws:: info!("OAB client disconnected"); } +#[cfg(all(test, feature = "googlechat"))] +mod googlechat_dispatch_tests { + use super::{schema, spawn_googlechat_reply, AppState}; + use std::sync::Arc; + use tokio::sync::broadcast; + + #[tokio::test] + async fn missing_adapter_dispatch_emits_correlated_failure() { + let (event_tx, mut event_rx) = broadcast::channel::(4); + let state = Arc::new(AppState::test_default(event_tx)); + let reply = schema::GatewayReply { + schema: "openab.gateway.reply.v1".into(), + reply_to: "original".into(), + platform: "googlechat".into(), + channel: schema::ReplyChannel { + id: "spaces/TEST".into(), + thread_id: None, + }, + content: schema::Content { + content_type: "text".into(), + text: "hello".into(), + attachments: vec![], + }, + command: None, + request_id: Some("req_route_missing".into()), + quote_message_id: None, + }; + + spawn_googlechat_reply(state, reply).await.unwrap(); + + let response: schema::GatewayResponse = serde_json::from_str( + &event_rx + .try_recv() + .expect("missing adapter must emit a failure response"), + ) + .unwrap(); + assert_eq!(response.request_id, "req_route_missing"); + assert!(!response.success); + assert_eq!( + response.error.as_deref(), + Some("googlechat adapter is not configured") + ); + } +} + async fn health() -> &'static str { "ok" } diff --git a/docs/config-reference.md b/docs/config-reference.md index 423f76541..838ceacb6 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -238,9 +238,10 @@ to that static token and log a possible identity switch. For GKE Workload Identity, set Helm value `agents..gateway.serviceAccountName` to an existing annotated Kubernetes -ServiceAccount for the runtime GSA. An empty value falls back to -`agents..serviceAccountName`, then chart-global `serviceAccountName`; the -chart does not create or annotate the ServiceAccount. +ServiceAccount for the runtime GSA. An empty value omits `serviceAccountName` +and preserves the Kubernetes default identity; it does not inherit per-agent or +chart-global ServiceAccount values. The chart does not create or annotate the +ServiceAccount. --- diff --git a/docs/google-chat.md b/docs/google-chat.md index 71ab2d200..4cfb8a6f0 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -151,9 +151,10 @@ agents: The `openab-googlechat-runtime` KSA must carry the usual `iam.gke.io/gcp-service-account: openab-runtime@PROJECT.iam.gserviceaccount.com` -annotation and Workload Identity IAM binding. If `gateway.serviceAccountName` -is empty, the chart falls back to `agents..serviceAccountName`, then the -chart-global `serviceAccountName`. +annotation and Workload Identity IAM binding. Set +`gateway.serviceAccountName` explicitly to attach that KSA. An empty value +preserves the Kubernetes default identity and does not inherit per-agent or +chart-global ServiceAccount values. Precedence: if a configured SA key loads successfully, it wins and ADC is ignored. If a key is configured but fails to load, the adapter uses the configured ADC target and logs the identity switch. `GOOGLE_CHAT_USE_ADC=true` without `GOOGLE_CHAT_ADC_TARGET_SERVICE_ACCOUNT` fails closed (ADC is not installed). If ADC fails and a static token is explicitly configured, the adapter degrades to it with a warning that it may represent a different identity. From 0d142de33204d98169d7171926a35b9b3d68c38d Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:14:52 +0000 Subject: [PATCH 12/12] fix(gateway): detach googlechat delivery task --- crates/openab-gateway/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/openab-gateway/src/lib.rs b/crates/openab-gateway/src/lib.rs index dccdda44b..587cc15fe 100644 --- a/crates/openab-gateway/src/lib.rs +++ b/crates/openab-gateway/src/lib.rs @@ -1074,10 +1074,10 @@ async fn handle_oab_connection(state: Arc, socket: axum::extract::ws:: // running, and the receive loop must keep draining replies. // Dropping the JoinHandle detaches the delivery task; its deadline // includes any wait for the adapter's delivery lock. - let _ = spawn_googlechat_reply( + std::mem::drop(spawn_googlechat_reply( state_for_recv.clone(), reply.clone(), - ); + )); } #[cfg(feature = "wecom")] "wecom" => {