From 7f005717f01beb72238746ecf87772b080fae237 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Fri, 25 Sep 2026 13:35:03 -0700 Subject: [PATCH 1/3] feat(sts): exchange opaque API keys at /.sts by hash lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-013 as revised (#234): a service account's API key is an opaque `sck_` secret that source.coop stores as a SHA-256 hash. When `/.sts` receives one as `WebIdentityToken`, the proxy trims and format-checks it locally, hashes it, and asks `POST {SOURCE_API_URL}/api/v1/service-account-keys/exchanges` whether it is active and for which account, authenticated as itself with the sentinel subject `urn:source:data-proxy`. The answer is cached for 60 seconds, inactive answers included; an API failure fails closed with a 500 and caches nothing. It then mints credentials under the `_default` role for the account the API names, through the STS crate's minting and sealing. A key is accepted only from a POST form body. One in the query string is refused before any lookup, with a message saying why, because Cloudflare logs request URLs. Every refusal of the key itself reads "API key was not accepted (request id …)" with the id also in `x-amzn-requestid`; the reason is logged once under that id. Attempts are rate-limited per client IP by a new `KEY_EXCHANGE_LIMIT` ratelimit binding in every wrangler config; a deployment without it logs an error and does not refuse traffic. `ApiAuth` gains `authorization_header_as_self` and refuses the sentinel as an on-behalf-of subject; `cached_fetch` takes a method, an optional JSON body and an `ApiCaller`. `sts::default_role` is factored out so the key exchange mints under the same role. Supersedes #233: nothing signs a key, so `/.keys`, self-verification against the proxy's own JWKS, the `api_key` role and the multistore pin are gone. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --- Cargo.toml | 4 + README.md | 10 ++ src/keys.rs | 82 +++++++++++++++++ src/lib.rs | 198 ++++++++++++++++++++++++++++++++++++++++ src/source_api/auth.rs | 60 +++++++++++- src/source_api/cache.rs | 81 +++++++++++++--- src/sts.rs | 30 ++++-- tests/keys.rs | 106 +++++++++++++++++++++ tests/stub_api.py | 47 ++++++++++ tests/test_api_keys.py | 133 +++++++++++++++++++++++++++ wrangler.preview.toml | 7 ++ wrangler.toml | 16 ++++ 12 files changed, 746 insertions(+), 28 deletions(-) create mode 100644 src/keys.rs create mode 100644 tests/keys.rs create mode 100644 tests/test_api_keys.py diff --git a/Cargo.toml b/Cargo.toml index 472e6b39..e5f80da3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,10 @@ path = "tests/object_path.rs" name = "fixtures" path = "tests/fixtures.rs" +[[test]] +name = "keys" +path = "tests/keys.rs" + [dependencies] # Multistore multistore = { version = "0.7.2", features = ["azure", "gcp"] } diff --git a/README.md b/README.md index 2699e72e..381912bf 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,16 @@ Set in `wrangler.toml` or via the Cloudflare dashboard: | `OIDC_PROVIDER_KID` | `data-proxy-1` | Key ID for the active signing key | | `OIDC_PROVIDER_KID_PREVIOUS` | — | Key ID for the previous key (during rotation) | +### Bindings + +| Binding | Kind | Description | +| -------------------- | ----------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| `KEY_EXCHANGE_LIMIT` | `ratelimit` | Per-client-IP limit on API-key exchanges at `/.sts` (ADR-013). Declared under `[[unsafe.bindings]]` in every `wrangler*.toml`; a deployment without it logs an error and exchanges without a limit | + +### API keys + +A service account's API key (ADR-013) is an opaque `sck_` secret that source.coop stores as a hash. It is presented at `/.sts` as `WebIdentityToken`, from a POST form body only — a key in the URL is refused, because the URL is logged. The proxy trims and format-checks it, hashes it, and asks `POST {SOURCE_API_URL}/api/v1/service-account-keys/exchanges` for its standing as itself (subject `urn:source:data-proxy`), caching the answer for 60 seconds; then it mints credentials for the account the API names, exactly as it would for an ID token. Every refusal of the key reads `API key was not accepted (request id …)`; the reason is in the log under that id. + ### Secrets **GitHub environment secrets are the source of truth.** The deploy workflow diff --git a/src/keys.rs b/src/keys.rs new file mode 100644 index 00000000..7e06074f --- /dev/null +++ b/src/keys.rs @@ -0,0 +1,82 @@ +//! API keys (ADR-013): opaque secrets a service account presents at `/.sts` +//! in place of an OIDC token. Nothing here verifies a signature — there is +//! none. The key's standing lives in source.coop, keyed by the key's SHA-256, +//! and this module is the wasm-free half of the exchange: recognising a key, +//! hashing it, and sealing credentials for the account the API names. + +use multistore::error::ProxyError; +use multistore::types::{RoleConfig, TemporaryCredentials}; +use multistore_sts::sts::mint_temporary_credentials; +use multistore_sts::TokenKey; +use serde::Deserialize; +use sha2::{Digest, Sha256}; + +/// Every key starts with this, followed by 32 random bytes in base64url: +/// a fixed 47 characters, the pattern secret scanners are given. +pub const API_KEY_PREFIX: &str = "sck_"; +const API_KEY_LEN: usize = 47; + +/// The token, trimmed, if it has exactly a key's shape; `None` for anything +/// else — a JWT, a truncated key, the wrong case — so the JWT path or a local +/// refusal takes it without a lookup. Whitespace is trimmed first because +/// every hand-made token file ends in a newline, and some SDKs send it. +pub fn parse_api_key(token: &str) -> Option<&str> { + let key = token.trim(); + (key.len() == API_KEY_LEN + && key.starts_with(API_KEY_PREFIX) + && key[API_KEY_PREFIX.len()..] + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')) + .then_some(key) +} + +/// Whether the token so much as looks like a key — the prefix alone. Used to +/// refuse a key sent where it would be logged, before checking anything else. +pub fn looks_like_api_key(token: &str) -> bool { + token.trim_start().starts_with(API_KEY_PREFIX) +} + +/// Hex SHA-256 of a key: the record's key in source.coop, and all the proxy +/// ever sends of it. +pub fn key_hash(key: &str) -> String { + Sha256::digest(key.as_bytes()) + .iter() + .map(|b| format!("{b:02x}")) + .collect() +} + +/// The Source API's answer for a presented key +/// (`POST /api/v1/service-account-keys/exchanges`): whether it may be +/// exchanged and, if so, for whom. Unknown, revoked, expired and disabled all +/// come back inactive and unnamed, so nothing distinguishes them here. +#[derive(Debug, Clone, Deserialize)] +pub struct KeyStanding { + pub active: bool, + #[serde(default)] + pub account_id: Option, + #[serde(default)] + pub key_id: Option, +} + +/// Credentials for an account the API has vouched for, sealed the way the +/// STS route seals every session, for the duration the client asked for +/// within the role's cap. The floor and default are AWS's and multistore's. +pub fn credentials_for( + role: &RoleConfig, + account_id: &str, + duration_seconds: Option, + token_key: &TokenKey, +) -> Result { + let duration = duration_seconds + .unwrap_or(3600) + .clamp(900, role.max_session_duration_secs); + let mut creds = mint_temporary_credentials( + role, + account_id, + duration, + "STSPRXY", + &serde_json::json!({}), + ); + creds.session_token = token_key.seal(&creds)?; + Ok(creds) +} diff --git a/src/lib.rs b/src/lib.rs index e9f8d42e..d3199bb0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,19 +12,23 @@ mod authz; mod backend_auth; mod config; mod handlers; +mod keys; mod location; mod object_path; mod pagination; mod source_api; mod sts; +use crate::config::AppConfig; use crate::source_api::{ApiAuth, SourceCoopRegistry}; use analytics::log_analytics; use handlers::{AccountListHandler, IndexHandler}; use multistore::api::response::ErrorResponse; +use multistore::error::ProxyError; use multistore::proxy::{GatewayResponse, ProxyGateway}; use multistore::route_handler::{ProxyResult, RequestInfo}; use multistore::router::Router; +use multistore::types::TemporaryCredentials; use multistore_cf_workers::{ collect_js_body, GatewayResponseExt, NoopCredentialRegistry, RequestParts, WorkerBackend, WorkerSubscriber, @@ -35,6 +39,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 object_path::{extract_path_segments, is_keyless_write, mapped_copy_source}; use std::sync::OnceLock; use sts::StsCredentialRegistry; @@ -231,6 +236,16 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result(headers: &'a http::HeaderMap, name: &str) -> &'a st .unwrap_or("") } +// ── API keys ──────────────────────────────────────────────────────── + +/// A response answered before the gateway, with the CORS headers and the +/// request id every gateway response carries — as `x-request-id`, and as +/// `x-amzn-requestid`, the header AWS SDKs read it from. +fn finish((status, xml): (u16, String), request_id: &str) -> web_sys::Response { + let response = + add_cors(GatewayResponse::Response(ProxyResult::xml(status, xml)).into_web_sys()); + if !request_id.is_empty() { + let _ = response.headers().set("x-request-id", request_id); + let _ = response.headers().set("x-amzn-requestid", request_id); + } + response +} + +/// The rate-limiter binding for API-key exchanges, keyed by client IP. +const KEY_EXCHANGE_LIMIT: &str = "KEY_EXCHANGE_LIMIT"; + +/// The API-key exchange, if this request is one: `None` when it is not an +/// `AssumeRoleWithWebIdentity` carrying an `sck_` key, so the STS route takes +/// it. A key is accepted from the form body only — Cloudflare logs the URL — +/// and the refusal for one in the query string says so, because that is the +/// one mistake a user can fix. +async fn api_key_exchange( + config: &AppConfig, + parts: &RequestParts, + env: &Env, + api_auth: &ApiAuth, + request_id: &str, +) -> Option<(u16, String)> { + if let Some(parsed) = try_parse_sts_request(parts.query.as_deref()) { + let is_key = parsed + .as_ref() + .is_ok_and(|sts| keys::looks_like_api_key(&sts.web_identity_token)); + if !is_key { + return None; // a token in the query string is the STS route's + } + tracing::warn!(%request_id, reason = "query_string", "API key exchange refused"); + return Some(key_refusal( + "API key must be sent in the request body, not the URL", + request_id, + )); + } + let sts = try_parse_sts_request(parts.form_body.as_deref())?.ok()?; + if !keys::looks_like_api_key(&sts.web_identity_token) { + return None; + } + + // Every attempt costs a lookup for a distinct key, so the flood to bound is + // distinct junk keys from one place. Legitimate exchanges are rare — once + // per session — so even a cluster behind one NAT stays well under the limit. + let client_ip = header_str(&parts.headers, "cf-connecting-ip"); + if !within_rate_limit(env, client_ip).await { + tracing::warn!(%request_id, reason = "rate_limited", "API key exchange refused"); + return Some(( + 429, + sts_error_xml( + "Throttling", + "too many API key exchanges from this address; retry later", + ), + )); + } + + Some( + match exchange_api_key(config, &sts, api_auth, request_id).await { + Ok(creds) => build_sts_response(&creds), + // One answer for every refusal of the key itself — unknown, revoked, + // expired, disabled, malformed. `exchange_api_key` has logged why. + Err(ProxyError::InvalidOidcToken(_)) => { + key_refusal("API key was not accepted", request_id) + } + // A bad role is about the request, not the key; anything else is the + // 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) + } + }, + ) +} + +/// `InvalidIdentityToken` with the request id in the message: SDKs show a +/// user the message and nothing else, and the id is what finds the log line. +fn key_refusal(message: &str, request_id: &str) -> (u16, String) { + let message = if request_id.is_empty() { + message.to_string() + } else { + format!("{message} (request id {request_id})") + }; + build_sts_error_response(&ProxyError::InvalidOidcToken(message)) +} + +/// Hash the key, ask source.coop for its standing, and mint for the account +/// it names. A refused key is logged here, once, and returned as +/// `InvalidOidcToken`. The proxy knows only "malformed" or "inactive": which +/// of unknown, revoked, expired or disabled is in source.coop's log under the +/// same request id. +async fn exchange_api_key( + config: &AppConfig, + sts: &multistore_sts::request::StsRequest, + api_auth: &ApiAuth, + request_id: &str, +) -> Result { + let Some(key) = keys::parse_api_key(&sts.web_identity_token) else { + tracing::warn!(%request_id, reason = "malformed", "API key exchange refused"); + return Err(ProxyError::InvalidOidcToken("malformed".into())); + }; + if !sts::is_default_role(&sts.role_arn) { + return Err(ProxyError::RoleNotFound(sts.role_arn.clone())); + } + let key_hash = keys::key_hash(key); + let standing = source_api::cache::get_or_fetch_key_standing( + &config.api_base_url, + &key_hash, + api_auth, + request_id, + ) + .await + .map_err(|e| match e { + // The route answers 200 for any well-formed hash; a 404 means the API + // does not serve it, which is a deployment mismatch, not an unknown key. + ProxyError::BucketNotFound(_) => { + ProxyError::Internal("key standing route not found".into()) + } + e => e, + })?; + let key_id = standing.key_id.as_deref().unwrap_or(""); + let hash_prefix = &key_hash[..8]; + let account_id = match (standing.active, standing.account_id) { + (true, Some(account_id)) => account_id, + _ => { + tracing::warn!(%request_id, key_id, hash_prefix, "API key is not active"); + return Err(ProxyError::InvalidOidcToken("inactive".into())); + } + }; + let role = sts::default_role( + config.auth_issuer.clone(), + config.auth_audiences.clone(), + config.sts_max_session_duration_secs, + ); + let creds = keys::credentials_for( + &role, + &account_id, + sts.duration_seconds, + &config.session_token_key, + )?; + tracing::info!(%request_id, key_id, %account_id, "API key exchanged"); + Ok(creds) +} + +/// Whether `client_ip` may make another exchange attempt now. A missing +/// binding is a deployment error, logged as such; it does not refuse traffic. +async fn within_rate_limit(env: &Env, client_ip: &str) -> bool { + let key = if client_ip.is_empty() { + "unknown" + } else { + client_ip + }; + match env.rate_limiter(KEY_EXCHANGE_LIMIT) { + Ok(limiter) => match limiter.limit(key.to_string()).await { + Ok(outcome) => outcome.success, + Err(e) => { + tracing::warn!("rate limiter call failed: {e}"); + true + } + }, + Err(_) => { + tracing::error!( + "{KEY_EXCHANGE_LIMIT} binding is not configured; API-key exchanges are unlimited" + ); + true + } + } +} + +/// An STS-shaped error body for a status `build_sts_error_response` has no +/// variant for. +fn sts_error_xml(code: &str, message: &str) -> String { + format!( + "\n{code}{message}" + ) +} + // ── CORS ──────────────────────────────────────────────────────────── fn add_cors(resp: web_sys::Response) -> web_sys::Response { diff --git a/src/source_api/auth.rs b/src/source_api/auth.rs index f803c622..41da850c 100644 --- a/src/source_api/auth.rs +++ b/src/source_api/auth.rs @@ -1,5 +1,29 @@ use multistore_oidc_provider::jwt::JwtSigner; +/// The subject the proxy signs with when it calls the API as itself rather +/// than on behalf of an account (ADR-013, amending ADR-005). Only the API-key +/// standing lookup accepts it: the API-key exchange happens before anything +/// names an account. A URN, so no account id can ever equal it, and refused +/// as an on-behalf-of subject so no request can claim it. +pub(crate) const PROXY_SELF_SUBJECT: &str = "urn:source:data-proxy"; + +/// Who an API request is made as. +#[derive(Clone, Copy, Debug)] +pub(crate) enum ApiCaller<'a> { + /// No credentials: the API answers as it would any stranger. + Anonymous, + /// On behalf of an account the proxy has authenticated. + Account(&'a str), + /// The proxy itself. + Proxy, +} + +impl<'a> From> for ApiCaller<'a> { + fn from(subject: Option<&'a str>) -> Self { + subject.map_or(ApiCaller::Anonymous, ApiCaller::Account) + } +} + /// How the proxy authenticates to the Source Cooperative API. #[derive(Clone)] pub(crate) struct ApiAuth { @@ -20,12 +44,38 @@ impl ApiAuth { /// Build the `Authorization` header value for an API request on behalf of /// `subject`. /// - /// Returns `None` if signing fails. The signing key is parsed and validated - /// once at startup (`JwtSigner::from_pem`, which panics on a bad key), so a - /// runtime failure here is very unlikely. When it does happen the error is - /// logged and the caller falls through to an unauthenticated request, which - /// the API surfaces as `AccessDenied` (403) rather than a 500. + /// Returns `None` if signing fails, or if `subject` is the proxy's own + /// sentinel — that is `authorization_header_as_self`'s to sign, never a + /// caller's to claim. The signing key is parsed and validated once at + /// startup (`JwtSigner::from_pem`, which panics on a bad key), so a + /// runtime signing failure is very unlikely. When it does happen the + /// error is logged and the caller falls through to an unauthenticated + /// request, which the API surfaces as `AccessDenied` (403) rather than a + /// 500. pub fn authorization_header(&self, subject: &str) -> Option { + if subject == PROXY_SELF_SUBJECT { + tracing::error!("refusing to sign an on-behalf-of assertion as the proxy itself"); + return None; + } + self.sign(subject) + } + + /// The `Authorization` header value for a request the proxy makes as + /// itself. See `PROXY_SELF_SUBJECT`. + pub fn authorization_header_as_self(&self) -> Option { + self.sign(PROXY_SELF_SUBJECT) + } + + /// The header for `caller`, or `None` when the request goes out anonymous. + pub fn authorization_header_for(&self, caller: ApiCaller<'_>) -> Option { + match caller { + ApiCaller::Anonymous => None, + ApiCaller::Account(subject) => self.authorization_header(subject), + ApiCaller::Proxy => self.authorization_header_as_self(), + } + } + + fn sign(&self, subject: &str) -> Option { match self.signer.sign(subject, &self.issuer, &self.audience, &[]) { Ok(token) => Some(format!("Bearer {}", token)), Err(e) => { diff --git a/src/source_api/cache.rs b/src/source_api/cache.rs index a3499843..5cb7743a 100644 --- a/src/source_api/cache.rs +++ b/src/source_api/cache.rs @@ -3,7 +3,9 @@ //! Each public function caches one API call type with its own TTL. //! Adjust the `*_CACHE_SECS` constants to tune per-datatype expiry. +use super::auth::ApiCaller; use super::types::{DataConnection, SourceProduct, SourceProductList}; +use crate::keys::KeyStanding; use multistore::error::ProxyError; use percent_encoding::{utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC}; @@ -44,6 +46,12 @@ const PRODUCT_LIST_CACHE_SECS: u32 = 60; // 1 minute /// so a revoked grant should stop taking effect quickly. const PERMISSIONS_CACHE_SECS: u32 = 60; // 1 minute +/// A presented API key's standing (`/service-account-keys/exchanges`). The +/// permissions TTL, for the same reason: it gates access, so a revoked key +/// should stop being exchangeable quickly (ADR-013). Inactive answers are +/// cached too — an unknown key costs one lookup a minute, not one a request. +const KEY_STANDING_CACHE_SECS: u32 = 60; // 1 minute + // ── Public cache functions ───────────────────────────────────────── /// Fetch a single product's metadata, cached for `PRODUCT_CACHE_SECS`. @@ -65,10 +73,12 @@ pub async fn get_or_fetch_product( cached_fetch( &cache_key, &api_url, + "GET", + None, PRODUCT_CACHE_SECS, api_auth, request_id, - subject, + subject.into(), ) .await } @@ -94,10 +104,12 @@ pub async fn get_or_fetch_permissions( cached_fetch( &cache_key, &api_url, + "GET", + None, PERMISSIONS_CACHE_SECS, api_auth, request_id, - Some(subject), + ApiCaller::Account(subject), ) .await } @@ -125,10 +137,12 @@ pub async fn get_or_fetch_data_connection( cached_fetch( &cache_key, &api_url, + "GET", + None, DATA_CONNECTION_CACHE_SECS, api_auth, request_id, - subject, + subject.into(), ) .await } @@ -150,10 +164,42 @@ pub async fn get_or_fetch_product_list( cached_fetch( &cache_key, &api_url, + "GET", + None, PRODUCT_LIST_CACHE_SECS, api_auth, request_id, - subject, + subject.into(), + ) + .await +} + +/// A presented API key's standing, cached for `KEY_STANDING_CACHE_SECS`, +/// looked up by the key's hash — the key itself never leaves the proxy. +/// Asked as the proxy itself: nothing names an account before the answer. +/// A POST, because the API records the use as it answers; the route answers +/// 200 for any well-formed hash, inactive ones included, so the cache holds +/// every answer alike. +pub async fn get_or_fetch_key_standing( + api_base_url: &str, + key_hash: &str, + api_auth: &crate::ApiAuth, + request_id: &str, +) -> Result { + let api_url = format!("{}/api/v1/service-account-keys/exchanges", api_base_url); + // The Cache API keys on URLs; the base URL scopes the entry to this + // environment's API, and the hash is the only thing that varies. + let cache_key = format!("{api_url}?key_hash={key_hash}"); + let body = serde_json::json!({ "key_hash": key_hash }).to_string(); + cached_fetch( + &cache_key, + &api_url, + "POST", + Some(&body), + KEY_STANDING_CACHE_SECS, + api_auth, + request_id, + ApiCaller::Proxy, ) .await } @@ -181,15 +227,18 @@ fn cache_key_with_subject(api_url: &str, subject: Option<&str>) -> String { } /// Generic cache-or-fetch: check the Cache API, return cached JSON on hit, -/// otherwise fetch from `api_url`, store in cache with the given TTL, and -/// return the deserialized result. +/// otherwise fetch from `api_url` with `method` (and a JSON `body`, if any), +/// store in cache with the given TTL, and return the deserialized result. +#[allow(clippy::too_many_arguments)] async fn cached_fetch( cache_key: &str, api_url: &str, + method: &str, + body: Option<&str>, ttl_secs: u32, api_auth: &crate::ApiAuth, request_id: &str, - subject: Option<&str>, + caller: ApiCaller<'_>, ) -> Result { let span = tracing::info_span!( "cached_fetch", @@ -219,17 +268,21 @@ async fn cached_fetch( // ── Cache miss — fetch from API ──────────────────────────── span.record("cache_hit", false); let init = web_sys::RequestInit::new(); - init.set_method("GET"); + init.set_method(method); let req_headers = web_sys::Headers::new() .map_err(|e| ProxyError::Internal(format!("headers build failed: {:?}", e)))?; // Only authenticate to the API when we have an identified caller. // Anonymous proxy requests hit the API without credentials. - if let Some(subj) = subject { - if let Some(auth_value) = api_auth.authorization_header(subj) { - req_headers - .set("Authorization", &auth_value) - .map_err(|e| ProxyError::Internal(format!("header set failed: {:?}", e)))?; - } + if let Some(auth_value) = api_auth.authorization_header_for(caller) { + req_headers + .set("Authorization", &auth_value) + .map_err(|e| ProxyError::Internal(format!("header set failed: {:?}", e)))?; + } + if let Some(body) = body { + req_headers + .set("content-type", "application/json") + .map_err(|e| ProxyError::Internal(format!("header set failed: {:?}", e)))?; + init.set_body(&wasm_bindgen::JsValue::from_str(body)); } if !request_id.is_empty() { let _ = req_headers.set("x-request-id", request_id); diff --git a/src/sts.rs b/src/sts.rs index da49bc1e..4290aa9d 100644 --- a/src/sts.rs +++ b/src/sts.rs @@ -36,19 +36,31 @@ impl StsCredentialRegistry { max_session_duration_secs: u64, ) -> Self { Self { - default_role: RoleConfig { - role_id: "_default".to_string(), - name: "Default".to_string(), - trusted_oidc_issuers: vec![oidc_issuer], - required_audiences, - subject_conditions: vec![], - allowed_scopes: vec![], // unlimited - max_session_duration_secs, - }, + default_role: default_role(oidc_issuer, required_audiences, max_session_duration_secs), } } } +/// The `_default` role: trusts `oidc_issuer` for tokens minted for one of +/// `required_audiences`, with no scope restriction. Shared with the API-key +/// exchange, which mints under the same role once the API has named the +/// account (`keys::credentials_for`). +pub(crate) fn default_role( + oidc_issuer: String, + required_audiences: Vec, + max_session_duration_secs: u64, +) -> RoleConfig { + RoleConfig { + role_id: "_default".to_string(), + name: "Default".to_string(), + trusted_oidc_issuers: vec![oidc_issuer], + required_audiences, + subject_conditions: vec![], + allowed_scopes: vec![], // unlimited + max_session_duration_secs, + } +} + impl CredentialRegistry for StsCredentialRegistry { async fn get_credential( &self, diff --git a/tests/keys.rs b/tests/keys.rs new file mode 100644 index 00000000..7543dd88 --- /dev/null +++ b/tests/keys.rs @@ -0,0 +1,106 @@ +//! Native unit tests for the wasm-free half of the API-key exchange (`keys`), +//! included via `#[path]` like `tests/sts.rs`. The Cache API and the lookup +//! itself are wasm-only and are covered by `tests/test_api_keys.py`. + +#[path = "../src/keys.rs"] +mod keys; +#[path = "../src/sts.rs"] +mod sts; + +use keys::*; +use multistore_sts::TokenKey; + +const KEY: &str = "sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + +// ── recognising a key ────────────────────────────────────────────── + +#[test] +fn a_well_formed_key_is_a_key() { + assert_eq!(parse_api_key(KEY), Some(KEY)); + let mixed = "sck_Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09A"; + assert_eq!(mixed.len(), 47); + assert_eq!(parse_api_key(mixed), Some(mixed)); +} + +#[test] +fn surrounding_whitespace_is_trimmed() { + // Every hand-made token file ends in a newline; some SDKs send it. + assert_eq!(parse_api_key(&format!("{KEY}\n")), Some(KEY)); + assert_eq!(parse_api_key(&format!("{KEY}\r\n")), Some(KEY)); + assert_eq!(parse_api_key(&format!(" {KEY} ")), Some(KEY)); +} + +#[test] +fn anything_else_is_not_a_key() { + assert_eq!(parse_api_key(&KEY[..46]), None, "too short"); + assert_eq!(parse_api_key(&format!("{KEY}a")), None, "too long"); + assert_eq!( + parse_api_key(&KEY.replace("sck_", "SCK_")), + None, + "wrong case" + ); + assert_eq!(parse_api_key(&KEY.replace('a', "+")), None, "not base64url"); + assert_eq!( + parse_api_key("eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ4In0.sig"), + None, + "a JWT" + ); + assert_eq!(parse_api_key(""), None); +} + +#[test] +fn the_prefix_alone_marks_a_key_for_refusal() { + assert!(looks_like_api_key("sck_anything")); + assert!(looks_like_api_key(" sck_anything")); + assert!(!looks_like_api_key("eyJ.a.b")); + assert!(!looks_like_api_key("SCK_anything")); +} + +// ── hashing ──────────────────────────────────────────────────────── + +#[test] +fn the_hash_is_hex_sha256_of_the_key() { + // Computed independently: sha256("sck_" + "a" * 43). + assert_eq!( + key_hash(KEY), + "079124300599a6ace561d0554a60dccf90edb04d486b55062bba42ff230d4f5f" + ); + assert_eq!(key_hash(KEY).len(), 64); +} + +// ── minting ──────────────────────────────────────────────────────── + +fn token_key() -> TokenKey { + TokenKey::from_base64("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=").unwrap() +} + +fn role(cap: u64) -> multistore::types::RoleConfig { + sts::default_role("https://auth.example.test".into(), vec!["aud".into()], cap) +} + +#[test] +fn credentials_are_sealed_for_the_account_within_floor_and_cap() { + let key = token_key(); + let creds = credentials_for(&role(43_200), "acme--nightly-sync", None, &key).unwrap(); + assert_eq!(creds.source_identity, "acme--nightly-sync"); + assert_eq!(creds.assumed_role_id, "_default"); + assert!(creds.access_key_id.starts_with("STSPRXY")); + // Sealed: the session token unseals to these credentials. + let unsealed = key.unseal(&creds.session_token).unwrap().unwrap(); + assert_eq!(unsealed.source_identity, "acme--nightly-sync"); + + let now = chrono_now(); + let default = credentials_for(&role(43_200), "a", None, &key).unwrap(); + assert!((default.expiration.timestamp() - now - 3600).abs() <= 2); + let floored = credentials_for(&role(43_200), "a", Some(1), &key).unwrap(); + assert!((floored.expiration.timestamp() - now - 900).abs() <= 2); + let capped = credentials_for(&role(3_600), "a", Some(86_400), &key).unwrap(); + assert!((capped.expiration.timestamp() - now - 3600).abs() <= 2); +} + +fn chrono_now() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() as i64 +} diff --git a/tests/stub_api.py b/tests/stub_api.py index 994fcbf9..ce08b3bc 100644 --- a/tests/stub_api.py +++ b/tests/stub_api.py @@ -21,6 +21,7 @@ SOURCE_API_URL in .dev.vars. """ +import hashlib import json import os from http.server import BaseHTTPRequestHandler, HTTPServer @@ -112,9 +113,55 @@ def _fixture(name): ) +# ── API keys ─────────────────────────────────────────────────────── +# Opaque keys the proxy resolves by SHA-256 at POST +# /api/v1/service-account-keys/exchanges (ADR-013), as itself. The stub keys +# its answers on the hash of each constant, so test_api_keys.py presents the +# key and never the hash — exactly what the proxy is meant to send. A counter +# per hash lets the tests prove the proxy's 60s standing cache is doing its +# job: the second exchange of a key must not reach here. +LIVE_KEY = "sck_" + "L" * 43 +REVOKED_KEY = "sck_" + "R" * 43 +UNKNOWN_KEY = "sck_" + "U" * 43 +ERR_500_KEY = "sck_" + "E" * 43 +KEY_ACCOUNT = "ci-tests--nightly-sync" + + +def _hash(key): + return hashlib.sha256(key.encode()).hexdigest() + + +KEY_STANDINGS = { + _hash(LIVE_KEY): (200, {"account_id": KEY_ACCOUNT, "key_id": "k-live", "active": True}), + _hash(REVOKED_KEY): (200, {"active": False}), + _hash(ERR_500_KEY): (500, {}), +} +KEY_EXCHANGE_COUNTS = {} + + class Handler(BaseHTTPRequestHandler): + def do_POST(self): + path = self.path.split("?")[0] + if path != "/api/v1/service-account-keys/exchanges": + return self._send(404, b"{}") + # The proxy authenticates as itself; the stub cannot verify the + # signature, but a missing header would mean the proxy sent nothing. + if not self.headers.get("Authorization", "").startswith("Bearer "): + return self._send(401, b"{}") + length = int(self.headers.get("content-length") or 0) + try: + key_hash = json.loads(self.rfile.read(length))["key_hash"] + except (ValueError, KeyError, TypeError): + return self._send(400, b"{}") + KEY_EXCHANGE_COUNTS[key_hash] = KEY_EXCHANGE_COUNTS.get(key_hash, 0) + 1 + status, body = KEY_STANDINGS.get(key_hash, (200, {"active": False})) + self._send(status, json.dumps(body).encode()) + def do_GET(self): path = self.path.split("?")[0] + # Test-only: how many times each key's standing was asked for. + if path == "/_stub/key-exchange-counts": + return self._send(200, json.dumps(KEY_EXCHANGE_COUNTS).encode()) if path == f"/api/v1/products/{WRITE_ACCOUNT}/{ERR_500_PRODUCT}": return self._send(500, b"{}") if path == f"/api/v1/products/{WRITE_ACCOUNT}/{ERR_BAD_JSON_PRODUCT}": diff --git a/tests/test_api_keys.py b/tests/test_api_keys.py new file mode 100644 index 00000000..05170499 --- /dev/null +++ b/tests/test_api_keys.py @@ -0,0 +1,133 @@ +"""API-key exchange at /.sts (ADR-013), against the stub Source API. + +A key is opaque: the proxy hashes it and asks the stub for its standing, as +itself, then mints credentials for the account the stub names. These tests +pin the parts that only run in the worker — the form-body-only rule, the +uniform refusal, the 60s standing cache, and fail-closed on an API error — +by counting how often each key's standing reaches the stub. +""" + +import re +import xml.etree.ElementTree as ET + +import requests + +from stub_api import ERR_500_KEY, KEY_ACCOUNT, LIVE_KEY, REVOKED_KEY, UNKNOWN_KEY, _hash + +PROXY_URL = "http://localhost:8787" +STUB_URL = "http://localhost:9000" + +# The worker takes its request id from `cf-ray`, which Cloudflare sets on +# every real request and `wrangler dev` does not; the tests supply one. +RAY = "ci-ray-0001" +REQUEST_ID = re.compile(r"\(request id [^)]+\)") + + +def exchange(key, *, in_query=False, role="arn:aws:iam::000000000000:role/_default"): + params = {"Action": "AssumeRoleWithWebIdentity", "RoleArn": role, "WebIdentityToken": key} + headers = {"cf-ray": RAY} + if in_query: + return requests.post(f"{PROXY_URL}/.sts", params=params, headers=headers) + return requests.post(f"{PROXY_URL}/.sts", data=params, headers=headers) + + +def lookups(key): + return requests.get(f"{STUB_URL}/_stub/key-exchange-counts").json().get(_hash(key), 0) + + +def sts_fields(resp): + return {el.tag.rpartition("}")[2]: el.text for el in ET.fromstring(resp.text).iter()} + + +def test_a_live_key_is_exchanged_for_credentials_of_its_account(): + resp = exchange(LIVE_KEY) + assert resp.status_code == 200, resp.text[:300] + fields = sts_fields(resp) + assert fields["AccessKeyId"].startswith("STSPRXY") + assert fields["SessionToken"] + assert fields["Expiration"] + # The account lives inside the sealed session token; the response names + # only the role. That the token was sealed for KEY_ACCOUNT is pinned by + # tests/keys.rs, which can unseal it. + assert fields["AssumedRoleId"] == "_default" + + +def test_a_stock_sdk_acquires_credentials_from_a_token_file_holding_the_key(tmp_path, monkeypatch): + """The point of the design: an unmodified SDK, configured by environment + alone, with the key saved to a file the way a user saves it — trailing + newline and all.""" + import boto3 + + token_file = tmp_path / "key" + token_file.write_text(LIVE_KEY + "\n") + for var in ("AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_SESSION_TOKEN", "AWS_PROFILE"): + monkeypatch.delenv(var, raising=False) + monkeypatch.setenv("AWS_CONFIG_FILE", str(tmp_path / "config")) + monkeypatch.setenv("AWS_SHARED_CREDENTIALS_FILE", str(tmp_path / "credentials")) + monkeypatch.setenv("AWS_ROLE_ARN", "arn:aws:iam::000000000000:role/_default") + monkeypatch.setenv("AWS_WEB_IDENTITY_TOKEN_FILE", str(token_file)) + monkeypatch.setenv("AWS_ENDPOINT_URL_STS", f"{PROXY_URL}/.sts") + monkeypatch.setenv("AWS_REGION", "us-east-1") + + creds = boto3.Session().get_credentials().get_frozen_credentials() + assert creds.access_key.startswith("STSPRXY") + assert creds.token + + +def test_the_standing_is_cached_so_a_second_exchange_never_reaches_the_api(): + exchange(LIVE_KEY) + before = lookups(LIVE_KEY) + assert exchange(LIVE_KEY).status_code == 200 + assert lookups(LIVE_KEY) == before, "second exchange within the TTL asked the API again" + + +def test_a_trailing_newline_in_the_token_file_is_harmless(): + assert exchange(LIVE_KEY + "\n").status_code == 200 + + +def test_unknown_and_revoked_keys_are_refused_alike_with_a_request_id(): + answers = {name: exchange(key) for name, key in [("unknown", UNKNOWN_KEY), ("revoked", REVOKED_KEY)]} + for name, resp in answers.items(): + assert resp.status_code == 400, name + fields = sts_fields(resp) + assert fields["Code"] == "InvalidIdentityToken", name + assert fields["Message"] == f"API key was not accepted (request id {RAY})", name + assert resp.headers.get("x-amzn-requestid") == RAY, name + # Nothing in the body says which it was. + assert REQUEST_ID.sub("", answers["unknown"].text) == REQUEST_ID.sub("", answers["revoked"].text) + # Refusals are cached too: an unknown key costs one lookup a minute. + before = lookups(UNKNOWN_KEY) + exchange(UNKNOWN_KEY) + assert lookups(UNKNOWN_KEY) == before + + +def test_a_key_in_the_query_string_is_refused_before_any_lookup(): + before = lookups(LIVE_KEY) + resp = exchange(LIVE_KEY, in_query=True) + assert resp.status_code == 400 + assert "request body" in sts_fields(resp)["Message"] + assert lookups(LIVE_KEY) == before + + +def test_a_malformed_key_is_refused_locally(): + before = sum(requests.get(f"{STUB_URL}/_stub/key-exchange-counts").json().values()) + for bad in ["sck_tooshort", "sck_" + "x" * 44, "SCK_" + "L" * 43]: + resp = exchange(bad) + assert resp.status_code == 400, bad + assert sts_fields(resp)["Code"] == "InvalidIdentityToken", bad + assert sum(requests.get(f"{STUB_URL}/_stub/key-exchange-counts").json().values()) == before + + +def test_a_wrong_role_is_reported_as_such(): + resp = exchange(LIVE_KEY, role="arn:aws:iam::000000000000:role/nope") + assert resp.status_code == 400 + assert sts_fields(resp)["Code"] == "MalformedPolicyDocument" + + +def test_an_api_failure_fails_closed_and_is_not_cached(): + first = exchange(ERR_500_KEY) + assert first.status_code == 500 + assert sts_fields(first)["Code"] == "InternalError" + before = lookups(ERR_500_KEY) + exchange(ERR_500_KEY) + assert lookups(ERR_500_KEY) == before + 1, "a failed lookup was cached" diff --git a/wrangler.preview.toml b/wrangler.preview.toml index 0f13699f..323e9f6f 100644 --- a/wrangler.preview.toml +++ b/wrangler.preview.toml @@ -47,3 +47,10 @@ dataset = "source_data_proxy_staging" [[services]] binding = "PUBLIC_LOG_STREAM" service = "public-log-stream-staging" + +# API-key exchange rate limit, per client IP; see wrangler.toml. +[[unsafe.bindings]] +name = "KEY_EXCHANGE_LIMIT" +type = "ratelimit" +namespace_id = "1003" +simple = { limit = 100, period = 60 } diff --git a/wrangler.toml b/wrangler.toml index 14b970db..6b10f65d 100644 --- a/wrangler.toml +++ b/wrangler.toml @@ -67,6 +67,16 @@ dataset = "source_data_proxy_production" binding = "PUBLIC_LOG_STREAM" service = "public-log-stream" +# API-key exchanges at /.sts, per client IP. Every attempt costs the Source +# API one lookup for a distinct key, so this bounds a flood of junk keys from +# one place; a legitimate client exchanges about once a session, so even a +# cluster behind one NAT stays far under it. See src/lib.rs `api_key_exchange`. +[[unsafe.bindings]] +name = "KEY_EXCHANGE_LIMIT" +type = "ratelimit" +namespace_id = "1001" +simple = { limit = 100, period = 60 } + [env.staging] routes = [ {pattern = "data.staging.coolnewgeo.com/*", zone_name = "coolnewgeo.com"}, @@ -90,6 +100,12 @@ dataset = "source_data_proxy_staging" binding = "PUBLIC_LOG_STREAM" service = "public-log-stream-staging" +[[env.staging.unsafe.bindings]] +name = "KEY_EXCHANGE_LIMIT" +type = "ratelimit" +namespace_id = "1002" +simple = { limit = 100, period = 60 } + [env.staging.observability] enabled = true head_sampling_rate = 1 From d6745e03dd105630afa37b52193d39f94ee09db9 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Tue, 29 Sep 2026 16:24:13 -0700 Subject: [PATCH 2/3] feat(sts): check an API key's CRC-32 checksum before looking it up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A key is now `sck_`, 30 random base62 characters and a six-character base62 CRC-32 of those 30 — 40 characters, GitHub's own token layout, as source.coop now issues them. The proxy checks the shape and the checksum before the rate limit's lookup, so a key that was cut short or mistyped costs nothing and is refused as malformed ("API key is malformed; check that it was copied whole"), a message that reveals nothing since the format is public. Every other refusal still reads "API key was not accepted". The CRC-32 is the IEEE one zlib computes, written out in a few lines rather than adding a crate. The unit tests pin it to vectors computed independently with Python's zlib, including a CRC above 2^31 and one whose checksum keeps a leading zero; the integration stub builds its keys the same way. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --- README.md | 2 +- src/keys.rs | 47 ++++++++++++++++++++++++++++++++---------- src/lib.rs | 11 ++++++++-- tests/keys.rs | 37 ++++++++++++++++++++++++++------- tests/stub_api.py | 21 +++++++++++++++---- tests/test_api_keys.py | 9 ++++++-- 6 files changed, 100 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 381912bf..f502c343 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,7 @@ Set in `wrangler.toml` or via the Cloudflare dashboard: ### API keys -A service account's API key (ADR-013) is an opaque `sck_` secret that source.coop stores as a hash. It is presented at `/.sts` as `WebIdentityToken`, from a POST form body only — a key in the URL is refused, because the URL is logged. The proxy trims and format-checks it, hashes it, and asks `POST {SOURCE_API_URL}/api/v1/service-account-keys/exchanges` for its standing as itself (subject `urn:source:data-proxy`), caching the answer for 60 seconds; then it mints credentials for the account the API names, exactly as it would for an ID token. Every refusal of the key reads `API key was not accepted (request id …)`; the reason is in the log under that id. +A service account's API key (ADR-013) is an opaque `sck_` secret that source.coop stores as a hash. It is presented at `/.sts` as `WebIdentityToken`, from a POST form body only — a key in the URL is refused, because the URL is logged. The proxy trims it and checks its shape and checksum (the last six characters are a CRC-32 of the thirty random ones before them, in base62), hashes it, and asks `POST {SOURCE_API_URL}/api/v1/service-account-keys/exchanges` for its standing as itself (subject `urn:source:data-proxy`), caching the answer for 60 seconds; then it mints credentials for the account the API names, exactly as it would for an ID token. A key that fails its shape or checksum was cut short or mistyped, and is refused as such without a lookup; every other refusal of the key reads `API key was not accepted (request id …)`, and the reason is in the log under that id. ### Secrets diff --git a/src/keys.rs b/src/keys.rs index 7e06074f..33eb4eb8 100644 --- a/src/keys.rs +++ b/src/keys.rs @@ -11,25 +11,50 @@ use multistore_sts::TokenKey; use serde::Deserialize; use sha2::{Digest, Sha256}; -/// Every key starts with this, followed by 32 random bytes in base64url: -/// a fixed 47 characters, the pattern secret scanners are given. +/// Every key starts with this, followed by 30 random base62 characters and +/// the six-character checksum of those 30: a fixed 40 characters, the pattern +/// secret scanners are given. pub const API_KEY_PREFIX: &str = "sck_"; -const API_KEY_LEN: usize = 47; +const API_KEY_LEN: usize = 40; +const BASE62: &[u8; 62] = b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; -/// The token, trimmed, if it has exactly a key's shape; `None` for anything -/// else — a JWT, a truncated key, the wrong case — so the JWT path or a local -/// refusal takes it without a lookup. Whitespace is trimmed first because -/// every hand-made token file ends in a newline, and some SDKs send it. +/// The token, trimmed, if it has exactly a key's shape and its checksum +/// holds; `None` for anything else — a JWT, a truncated or mistyped key, the +/// wrong case — so the JWT path or a local refusal takes it without a lookup. +/// Whitespace is trimmed first because every hand-made token file ends in a +/// newline, and some SDKs send it. pub fn parse_api_key(token: &str) -> Option<&str> { let key = token.trim(); + let rest = key.strip_prefix(API_KEY_PREFIX)?; (key.len() == API_KEY_LEN - && key.starts_with(API_KEY_PREFIX) - && key[API_KEY_PREFIX.len()..] - .bytes() - .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')) + && rest.bytes().all(|b| b.is_ascii_alphanumeric()) + && rest.as_bytes()[30..] == checksum(&rest.as_bytes()[..30])) .then_some(key) } +/// A key's last six characters: the CRC-32 of the thirty before them (IEEE, +/// as zlib computes it), in base62, most significant digit first. +fn checksum(body: &[u8]) -> [u8; 6] { + let mut crc = !0u32; + for &b in body { + crc ^= u32::from(b); + for _ in 0..8 { + crc = if crc & 1 == 1 { + (crc >> 1) ^ 0xEDB8_8320 + } else { + crc >> 1 + }; + } + } + let mut n = !crc; + let mut digits = [b'0'; 6]; + for digit in digits.iter_mut().rev() { + *digit = BASE62[(n % 62) as usize]; + n /= 62; + } + digits +} + /// Whether the token so much as looks like a key — the prefix alone. Used to /// refuse a key sent where it would be logged, before checking anything else. pub fn looks_like_api_key(token: &str) -> bool { diff --git a/src/lib.rs b/src/lib.rs index d3199bb0..94554a32 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -533,8 +533,15 @@ async fn api_key_exchange( Some( match exchange_api_key(config, &sts, api_auth, request_id).await { Ok(creds) => build_sts_response(&creds), - // One answer for every refusal of the key itself — unknown, revoked, - // expired, disabled, malformed. `exchange_api_key` has logged why. + // A key that fails its shape or checksum was cut short or mistyped, + // which the user can fix; saying so reveals nothing, since the + // format is public and no lookup was made. + Err(ProxyError::InvalidOidcToken(reason)) if reason == "malformed" => key_refusal( + "API key is malformed; check that it was copied whole", + request_id, + ), + // One answer for every other refusal of the key — unknown, revoked, + // expired, disabled. `exchange_api_key` has logged why. Err(ProxyError::InvalidOidcToken(_)) => { key_refusal("API key was not accepted", request_id) } diff --git a/tests/keys.rs b/tests/keys.rs index 7543dd88..44f86ce1 100644 --- a/tests/keys.rs +++ b/tests/keys.rs @@ -10,16 +10,20 @@ mod sts; use keys::*; use multistore_sts::TokenKey; -const KEY: &str = "sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +// Keys whose checksums were computed independently, with Python's zlib.crc32. +const KEY: &str = "sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1yLcDB"; // ── recognising a key ────────────────────────────────────────────── #[test] fn a_well_formed_key_is_a_key() { assert_eq!(parse_api_key(KEY), Some(KEY)); - let mixed = "sck_Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09A"; - assert_eq!(mixed.len(), 47); + // Every character class, and a CRC above 2^31. + let mixed = "sck_0123456789ABCDEFGHIJabcdefghij4Us3aw"; assert_eq!(parse_api_key(mixed), Some(mixed)); + // A CRC below 62^5, whose checksum keeps its leading zero. + let padded = "sck_000000000000000000000000000001010Ohw"; + assert_eq!(parse_api_key(padded), Some(padded)); } #[test] @@ -32,14 +36,33 @@ fn surrounding_whitespace_is_trimmed() { #[test] fn anything_else_is_not_a_key() { - assert_eq!(parse_api_key(&KEY[..46]), None, "too short"); + assert_eq!(parse_api_key(&KEY[..39]), None, "too short"); assert_eq!(parse_api_key(&format!("{KEY}a")), None, "too long"); assert_eq!( parse_api_key(&KEY.replace("sck_", "SCK_")), None, "wrong case" ); - assert_eq!(parse_api_key(&KEY.replace('a', "+")), None, "not base64url"); + assert_eq!( + parse_api_key(&KEY.replacen('a', "b", 1)), + None, + "a mistyped character" + ); + assert_eq!( + parse_api_key("sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1yLcDC"), + None, + "a mistyped checksum" + ); + assert_eq!( + parse_api_key("sck_aaaaaaaaaaaaaaaaaaaaaaaaaa-_aa1yLcDB"), + None, + "not base62" + ); + assert_eq!( + parse_api_key("sck_Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09Ab-_09A"), + None, + "the checksum-less 47-character format" + ); assert_eq!( parse_api_key("eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ4In0.sig"), None, @@ -60,10 +83,10 @@ fn the_prefix_alone_marks_a_key_for_refusal() { #[test] fn the_hash_is_hex_sha256_of_the_key() { - // Computed independently: sha256("sck_" + "a" * 43). + // Computed independently: sha256 of KEY. assert_eq!( key_hash(KEY), - "079124300599a6ace561d0554a60dccf90edb04d486b55062bba42ff230d4f5f" + "613aab548f220de88af7132782834dd7af6ec9ded8df4a7b19840545280968db" ); assert_eq!(key_hash(KEY).len(), 64); } diff --git a/tests/stub_api.py b/tests/stub_api.py index ce08b3bc..664b83c8 100644 --- a/tests/stub_api.py +++ b/tests/stub_api.py @@ -24,6 +24,7 @@ import hashlib import json import os +import zlib from http.server import BaseHTTPRequestHandler, HTTPServer from pathlib import Path @@ -120,10 +121,22 @@ def _fixture(name): # key and never the hash — exactly what the proxy is meant to send. A counter # per hash lets the tests prove the proxy's 60s standing cache is doing its # job: the second exchange of a key must not reach here. -LIVE_KEY = "sck_" + "L" * 43 -REVOKED_KEY = "sck_" + "R" * 43 -UNKNOWN_KEY = "sck_" + "U" * 43 -ERR_500_KEY = "sck_" + "E" * 43 +BASE62 = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + + +def _key(char): + """A well-formed key: 30 of `char`, then their CRC-32 in six base62 digits.""" + body, n, checksum = char * 30, zlib.crc32((char * 30).encode()), "" + for _ in range(6): + n, digit = divmod(n, 62) + checksum = BASE62[digit] + checksum + return "sck_" + body + checksum + + +LIVE_KEY = _key("L") +REVOKED_KEY = _key("R") +UNKNOWN_KEY = _key("U") +ERR_500_KEY = _key("E") KEY_ACCOUNT = "ci-tests--nightly-sync" diff --git a/tests/test_api_keys.py b/tests/test_api_keys.py index 05170499..bfccfea5 100644 --- a/tests/test_api_keys.py +++ b/tests/test_api_keys.py @@ -111,10 +111,15 @@ def test_a_key_in_the_query_string_is_refused_before_any_lookup(): def test_a_malformed_key_is_refused_locally(): before = sum(requests.get(f"{STUB_URL}/_stub/key-exchange-counts").json().values()) - for bad in ["sck_tooshort", "sck_" + "x" * 44, "SCK_" + "L" * 43]: + mistyped = LIVE_KEY[:4] + "M" + LIVE_KEY[5:] + for bad in ["sck_tooshort", LIVE_KEY[:-1], LIVE_KEY + "x", mistyped]: resp = exchange(bad) assert resp.status_code == 400, bad - assert sts_fields(resp)["Code"] == "InvalidIdentityToken", bad + fields = sts_fields(resp) + assert fields["Code"] == "InvalidIdentityToken", bad + assert fields["Message"] == f"API key is malformed; check that it was copied whole (request id {RAY})", bad + # The wrong case is no key at all, so the JWT path refuses it. + assert exchange(LIVE_KEY.replace("sck_", "SCK_")).status_code == 400 assert sum(requests.get(f"{STUB_URL}/_stub/key-exchange-counts").json().values()) == before From 30ad22ff0801f20a8f6335261a3af18a4eed75c6 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Tue, 29 Sep 2026 16:44:08 -0700 Subject: [PATCH 3/3] test(sts): assemble test keys so secret scanners don't flag them Once `sck_` keys are registered with GitHub's secret scanning, any whole key in a public repository is reported to source.coop and may trip push protection. The unit tests' keys are now concatenated from pieces, as the integration stub and the Source CLI's tests already build theirs at run time. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --- tests/keys.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/keys.rs b/tests/keys.rs index 44f86ce1..e7414609 100644 --- a/tests/keys.rs +++ b/tests/keys.rs @@ -10,8 +10,9 @@ mod sts; use keys::*; use multistore_sts::TokenKey; -// Keys whose checksums were computed independently, with Python's zlib.crc32. -const KEY: &str = "sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1yLcDB"; +// Keys whose checksums were computed independently, with Python's zlib.crc32, +// each assembled with `concat!` so that secret scanners don't flag this file. +const KEY: &str = concat!("sck_", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "1yLcDB"); // ── recognising a key ────────────────────────────────────────────── @@ -19,10 +20,10 @@ const KEY: &str = "sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1yLcDB"; fn a_well_formed_key_is_a_key() { assert_eq!(parse_api_key(KEY), Some(KEY)); // Every character class, and a CRC above 2^31. - let mixed = "sck_0123456789ABCDEFGHIJabcdefghij4Us3aw"; + let mixed = concat!("sck_", "0123456789ABCDEFGHIJabcdefghij", "4Us3aw"); assert_eq!(parse_api_key(mixed), Some(mixed)); // A CRC below 62^5, whose checksum keeps its leading zero. - let padded = "sck_000000000000000000000000000001010Ohw"; + let padded = concat!("sck_", "000000000000000000000000000001", "010Ohw"); assert_eq!(parse_api_key(padded), Some(padded)); } @@ -49,7 +50,7 @@ fn anything_else_is_not_a_key() { "a mistyped character" ); assert_eq!( - parse_api_key("sck_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1yLcDC"), + parse_api_key(concat!("sck_", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "1yLcDC")), None, "a mistyped checksum" );