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 cbc1e8ed..8e77f0b5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -97,16 +97,23 @@ 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 // 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. diff --git a/src/lib.rs b/src/lib.rs index 6a835f11..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; @@ -171,23 +171,6 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result Result { tracing::warn!(%request_id, error = %e, "API key exchange failed"); - build_sts_error_response(&e) + sts_refusal(&e, request_id) } }, ) @@ -559,9 +560,27 @@ 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) +} + +/// 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, 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 @@ -668,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}" ) @@ -688,9 +711,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)?; @@ -740,7 +766,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"; @@ -757,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/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); diff --git a/tests/test_platform_trust.py b/tests/test_platform_trust.py index 46f1aa88..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 "