From 26d74d8ec72f93b51698ebfd537b831a04187c63 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Wed, 30 Sep 2026 23:27:41 -0700 Subject: [PATCH 1/6] fix(sts): trim a platform token before reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A token file written with `jq -r … > file` ends in a newline, which SDKs send as-is and the signature's base64 decode rejects with a 400. Co-Authored-By: Claude Opus 5.5 --- src/lib.rs | 5 ++++- tests/test_platform_trust.py | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 6a835f11..43f183c7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -688,9 +688,12 @@ async fn platform_exchange( api_auth: &ApiAuth, request_id: &str, ) -> Option<(u16, String)> { - let sts = try_parse_sts_request(parts.query.as_deref()) + let mut sts = try_parse_sts_request(parts.query.as_deref()) .or_else(|| try_parse_sts_request(parts.form_body.as_deref()))? .ok()?; + // SDKs send a token file's contents as-is, and `jq -r … > file` ends it in + // a newline, which the signature segment's base64 decode rejects. + sts.web_identity_token = sts.web_identity_token.trim().to_string(); let (header, claims) = platform::unverified(&sts.web_identity_token)?; let issuer = claims.get("iss")?.as_str()?; let audiences = config.platform_issuers.get(issuer)?; diff --git a/tests/test_platform_trust.py b/tests/test_platform_trust.py index 46f1aa88..7ea93976 100644 --- a/tests/test_platform_trust.py +++ b/tests/test_platform_trust.py @@ -115,6 +115,13 @@ def test_a_trusted_workflow_gets_credentials_that_act_as_the_account(): assert subjects[f"/api/v1/products/{WRITE_ACCOUNT}/{product}"] == TRUST_ACCOUNT +@needs_token +def test_a_token_read_from_a_file_may_end_in_a_newline(): + """SDKs send AWS_WEB_IDENTITY_TOKEN_FILE's contents untrimmed.""" + resp = exchange(ID_TOKEN + "\n", as_account(TRUST_ACCOUNT)) + assert resp.status_code == 200, resp.text[:300] + + @needs_token def test_an_account_that_does_not_trust_the_workflow_refuses_it(): untrusting = as_account("ci-tests--someone-else") From 5ae932d6104981378bda5405a2d3e4a3f6893ac6 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Wed, 30 Sep 2026 23:28:17 -0700 Subject: [PATCH 2/6] perf(sts): cache a refused trust under the trust answer's own key The separate refused key cost an extra Cache API match before every lookup, and the delete of the positive entry never found anything after a 403. A refusal is now stored as {"trusted":false} under the same key for REFUSED_TRUST_CACHE_SECS: one Cache API read per exchange. Co-Authored-By: Claude Opus 5.5 --- src/source_api/cache.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/source_api/cache.rs b/src/source_api/cache.rs index 9c764cef..a06d8b07 100644 --- a/src/source_api/cache.rs +++ b/src/source_api/cache.rs @@ -57,10 +57,9 @@ const KEY_STANDING_CACHE_SECS: u32 = 60; // 1 minute /// reason: a trust that is removed should stop minting quickly (ADR-014). const TRUST_CACHE_SECS: u32 = 60; // 1 minute -/// A refusal from the trusts route, cached under its own key: long enough that -/// replaying one token its account does not trust costs about one lookup per -/// 10 seconds, however many addresses it comes from, and short enough that a -/// trust just added works within seconds. +/// A refusal from the trusts route: long enough that replaying one token its +/// account does not trust costs about one lookup per 10 seconds per data +/// center, and short enough that a trust just added works within seconds. const REFUSED_TRUST_CACHE_SECS: u32 = 10; // ── Public cache functions ───────────────────────────────────────── @@ -220,7 +219,7 @@ pub async fn get_or_fetch_key_standing( /// assume-role call. Asked as the account itself. The route says yes with a /// 200, cached for `TRUST_CACHE_SECS` like every 200, and no with a 403 (a 401 /// for an account it cannot resolve), which `cached_fetch` leaves uncached and -/// this caches for `REFUSED_TRUST_CACHE_SECS` under a key of its own. +/// this caches as `{"trusted":false}` for `REFUSED_TRUST_CACHE_SECS`. pub async fn get_or_fetch_trust( api_base_url: &str, account: &str, @@ -241,11 +240,6 @@ pub async fn get_or_fetch_trust( utf8_percent_encode(issuer, PATH_SEGMENT), utf8_percent_encode(subject, PATH_SEGMENT), ); - let refused_key = format!("{cache_key}&refused"); - let cache = worker::Cache::default(); - if matches!(cache.get(&refused_key, false).await, Ok(Some(_))) { - return Err(ProxyError::AccessDenied); - } let body = serde_json::json!({ "issuer": issuer, "subject": subject }).to_string(); let answer = cached_fetch::( &cache_key, @@ -260,17 +254,21 @@ pub async fn get_or_fetch_trust( .await; let trusted = match answer { Ok(answer) => answer.trusted, - Err(ProxyError::AccessDenied) => false, + Err(ProxyError::AccessDenied) => { + let cache = worker::Cache::default(); + let refused = r#"{"trusted":false}"#; + cache_put(&cache, &cache_key, refused, REFUSED_TRUST_CACHE_SECS).await; + false + } Err(e) => return Err(e), }; - if !trusted { - // A 200 saying no was cached like any 200: drop it, so a no is held - // for `REFUSED_TRUST_CACHE_SECS` whichever way the route said it. - let _ = cache.delete(cache_key.as_str(), false).await; - cache_put(&cache, &refused_key, "{}", REFUSED_TRUST_CACHE_SECS).await; - return Err(ProxyError::AccessDenied); + // ponytail: a 200 saying no (which the route never sends) is held for + // TRUST_CACHE_SECS like any 200; still refused, only slower to flip to yes. + if trusted { + Ok(()) + } else { + Err(ProxyError::AccessDenied) } - Ok(()) } /// The trusts route's answer. Its status already says yes (200) or no (403); From e06a99367f80b02cc78d1ac6b7f4e96f10cee818 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Wed, 30 Sep 2026 23:28:55 -0700 Subject: [PATCH 3/6] fix(sts): carry the request id in every exchange refusal Platform-token refusals and the API key path's non-key errors came from build_sts_error_response without it, and SDKs show only the message. Co-Authored-By: Claude Opus 5.5 --- src/lib.rs | 15 ++++++++++----- tests/test_platform_trust.py | 4 +++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 43f183c7..3012e8d9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -551,7 +551,7 @@ async fn api_key_exchange( // API being unreachable, which fails closed as a 500 the SDK retries. Err(e) => { tracing::warn!(%request_id, error = %e, "API key exchange failed"); - build_sts_error_response(&e) + sts_refusal(&e, request_id) } }, ) @@ -559,9 +559,14 @@ async fn api_key_exchange( /// `InvalidIdentityToken`, with the request id in the message. fn key_refusal(message: &str, request_id: &str) -> (u16, String) { - build_sts_error_response(&ProxyError::InvalidOidcToken(with_request_id( - message, request_id, - ))) + sts_refusal(&ProxyError::InvalidOidcToken(message.into()), request_id) +} + +/// `build_sts_error_response`, with the request id in the message. +fn sts_refusal(e: &ProxyError, request_id: &str) -> (u16, String) { + let (status, xml) = build_sts_error_response(e); + let message_end = format!("{}", with_request_id("", request_id)); + (status, xml.replacen("", &message_end, 1)) } /// `message` with the request id, if there is one: SDKs show a user the @@ -743,7 +748,7 @@ async fn exchange_platform_token( } = token; let failed = |e: ProxyError| { tracing::warn!(%request_id, %issuer, error = %e, "platform token exchange failed"); - build_sts_error_response(&e) + sts_refusal(&e, request_id) }; let not_authorized = || { let message = "Not authorized to perform sts:AssumeRoleWithWebIdentity"; diff --git a/tests/test_platform_trust.py b/tests/test_platform_trust.py index 7ea93976..2cc6c486 100644 --- a/tests/test_platform_trust.py +++ b/tests/test_platform_trust.py @@ -83,7 +83,9 @@ def test_a_forged_token_is_refused_before_any_trust_lookup(): before = trust_lookups(TRUST_ACCOUNT) resp = exchange(forged(), as_account(TRUST_ACCOUNT)) assert resp.status_code == 400 - assert sts_fields(resp)["Code"] == "InvalidIdentityToken" + fields = sts_fields(resp) + assert fields["Code"] == "InvalidIdentityToken" + assert fields["Message"].endswith(f"(request id {RAY})") assert trust_lookups(TRUST_ACCOUNT) == before From 56cb2ba74431e29e956e6554be59ce49dce418a1 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Wed, 30 Sep 2026 23:29:19 -0700 Subject: [PATCH 4/6] fix(config): never let PLATFORM_ISSUERS claim the person issuer The platform path takes its issuers' tokens ahead of the STS route, so an entry for AUTH_ISSUER would refuse every person exchange. Drop it at load with an error. Co-Authored-By: Claude Opus 5.5 --- src/config.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index cbc1e8ed..36ccfcfa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -103,10 +103,15 @@ fn build_config(env: &Env) -> AppConfig { // Platform identity providers (GitHub Actions, say), each with its own // audiences. Unset trusts none. - let platform_issuers = env + let mut platform_issuers = env .var("PLATFORM_ISSUERS") .map(|v| crate::platform::parse_issuers(&v.to_string())) .unwrap_or_default(); + // The platform path claims its issuers' tokens ahead of the STS route, so + // an entry for the person issuer would refuse every person exchange. + if platform_issuers.remove(&auth_issuer).is_some() { + tracing::error!(issuer = %auth_issuer, "PLATFORM_ISSUERS names AUTH_ISSUER; ignoring that entry"); + } // Ceiling for client-requested DurationSeconds on /.sts. Unset → 3600 (1h), // matching multistore's own default so behavior is unchanged until raised. From 5813cc35e019f340131d3edfabd93874c23344bc Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Wed, 30 Sep 2026 23:29:51 -0700 Subject: [PATCH 5/6] fix(sts): limit the AUTH_AUDIENCE 501 to the person route The fail-closed check ran before the API-key and platform short-circuits, so an empty AUTH_AUDIENCE also disabled exchanges that carry their own audience checks (platform) or none at all (keys). Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/config.rs | 6 ++++-- src/lib.rs | 35 ++++++++++++++++++----------------- wrangler.preview.toml | 2 +- wrangler.toml | 2 +- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 6c4a0227..6b20a90f 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,7 @@ Set in `wrangler.toml` or via the Cloudflare dashboard: | `SOURCE_API_URL` | `https://source.coop` | Source Cooperative API base URL | | `LOG_LEVEL` | `WARN` | Tracing level (`TRACE`, `DEBUG`, `INFO`, `WARN`, `ERROR`) | | `AUTH_ISSUER` | `https://auth.source.coop` | The person issuer trusted for `/.sts` token exchange; its tokens act as their own subject | -| `AUTH_AUDIENCE` | — | Comma-separated OAuth client ID(s) that `/.sts` subject tokens must be issued to (`aud` claim); a token is accepted if it matches any. Unset = `/.sts` token exchange is disabled (returns 501) | +| `AUTH_AUDIENCE` | — | Comma-separated OAuth client ID(s) that `/.sts` subject tokens must be issued to (`aud` claim); a token is accepted if it matches any. Unset = person-token exchange at `/.sts` is disabled (returns 501); API keys and platform tokens still exchange | | `PLATFORM_ISSUERS` | — | JSON object from each platform issuer URL to the audiences its tokens must carry, such as `{"https://token.actions.githubusercontent.com": ["https://data.source.coop"]}`. An issuer with no audience is refused. Unset = no platform issuer is trusted | | `OIDC_PROVIDER_ISSUER` | `https://data.source.coop` | Issuer URL for minted JWTs and OIDC discovery | | `OIDC_PROVIDER_KID` | `data-proxy-1` | Key ID for the active signing key | diff --git a/src/config.rs b/src/config.rs index 36ccfcfa..8e77f0b5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -97,8 +97,10 @@ fn build_config(env: &Env) -> AppConfig { if auth_audiences.is_empty() { // Fail closed: without an audience restriction, an ID token minted for // ANY OAuth client of AUTH_ISSUER could be exchanged for a user's - // credentials, so /.sts is disabled entirely (returns 501) until set. - tracing::warn!("AUTH_AUDIENCE not set: /.sts token exchange is disabled (returns 501)"); + // credentials, so its exchange is disabled (returns 501) until set. + tracing::warn!( + "AUTH_AUDIENCE not set: person-token exchange at /.sts is disabled (returns 501)" + ); } // Platform identity providers (GitHub Actions, say), each with its own diff --git a/src/lib.rs b/src/lib.rs index 3012e8d9..9d6f8b28 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -171,23 +171,6 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result Result Date: Wed, 30 Sep 2026 23:34:09 -0700 Subject: [PATCH 6/6] fix(sts): escape caller text in exchange error bodies RoleArn and a token's kid or alg reached the of an application/xml response unescaped, through multistore's build_sts_error_response: markup there ran as XHTML on the proxy origin, and an & made the body unparseable for SDKs. The API-key and platform paths now build their errors here with the message escaped. The person route's errors still come from multistore's builder. Co-Authored-By: Claude Opus 5.5 --- src/lib.rs | 32 ++++++++++++++++++++++++-------- tests/test_platform_trust.py | 9 +++++++++ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9d6f8b28..515db29b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -40,7 +40,7 @@ use multistore_oidc_provider::{HttpExchange, OidcCredentialProvider, OidcProvide use multistore_path_mapping::{MappedRegistry, PathMapping}; use multistore_sts::jwks::JwksCache; use multistore_sts::route_handler::StsRouterExt; -use multistore_sts::{build_sts_error_response, build_sts_response, try_parse_sts_request}; +use multistore_sts::{build_sts_response, try_parse_sts_request}; use object_path::{extract_path_segments, is_keyless_write, mapped_copy_source}; use std::sync::OnceLock; use sts::StsCredentialRegistry; @@ -563,11 +563,24 @@ fn key_refusal(message: &str, request_id: &str) -> (u16, String) { sts_refusal(&ProxyError::InvalidOidcToken(message.into()), request_id) } -/// `build_sts_error_response`, with the request id in the message. +/// The STS error for `e`, mapped as multistore's `build_sts_error_response` +/// maps it, with the request id in the message. Built here because that one +/// writes the message unescaped, and it can carry a caller's `RoleArn` or a +/// token's `kid`. fn sts_refusal(e: &ProxyError, request_id: &str) -> (u16, String) { - let (status, xml) = build_sts_error_response(e); - let message_end = format!("{}", with_request_id("", request_id)); - (status, xml.replacen("", &message_end, 1)) + let (status, code, message) = match e { + ProxyError::RoleNotFound(r) => ( + 400, + "MalformedPolicyDocument", + format!("role not found: {r}"), + ), + ProxyError::InvalidOidcToken(m) => (400, "InvalidIdentityToken", m.clone()), + ProxyError::InvalidRequest(m) => (400, "InvalidParameterValue", m.clone()), + ProxyError::AccessDenied => (403, "AccessDenied", "access denied".to_string()), + _ => (500, "InternalError", "internal error".to_string()), + }; + let message = with_request_id(&message, request_id); + (status, sts_error_xml(code, &message)) } /// `message` with the request id, if there is one: SDKs show a user the @@ -674,9 +687,13 @@ fn throttled() -> (u16, String) { ) } -/// An STS-shaped error body with a code or message `build_sts_error_response` -/// does not produce. +/// An STS-shaped error body. The message is escaped: it can carry text the +/// caller sent, and the body is served as XML. fn sts_error_xml(code: &str, message: &str) -> String { + let message = message + .replace('&', "&") + .replace('<', "<") + .replace('>', ">"); format!( "\n{code}{message}" ) @@ -766,7 +783,6 @@ async fn exchange_platform_token( config.sts_max_session_duration_secs, ) .ok_or_else(|| failed(ProxyError::RoleNotFound(sts.role_arn.clone())))?; - // No angle brackets in the message: the STS error body carries it unescaped. let account = sts::account(&sts.role_arn).ok_or_else(|| { failed(ProxyError::InvalidRequest( "RoleArn must name the account to act as: arn:aws:iam::ACCOUNT:role/ROLE".into(), diff --git a/tests/test_platform_trust.py b/tests/test_platform_trust.py index 2cc6c486..9e84f480 100644 --- a/tests/test_platform_trust.py +++ b/tests/test_platform_trust.py @@ -63,6 +63,15 @@ def test_a_platform_token_must_name_the_account_it_acts_as(): assert "RoleArn must name the account" in sts_fields(resp)["Message"] +def test_a_refusal_escapes_what_the_caller_sent(): + """RoleArn is echoed in the message; markup in it stays text.""" + role_arn = 'arn:aws:iam::ab--cd:role/&' + resp = exchange(forged(), role_arn) + assert resp.status_code == 400 + assert "