Conversation
…require exp, log successes An empty required_audiences accepted any audience and an empty subject_conditions skipped the subject check, so a role that omitted either — and both default to empty — trusted everything its issuer signed. Both now accept nothing; "any subject" is written "*", and static-config validation rejects a role with either list empty so the omission is reported at load rather than discovered at exchange. A token whose header carries typ must say JWT: access tokens (at+jwt) and other typed tokens are not identity tokens, whoever signed them. A token must carry exp, unless its issuer is listed in the new RoleConfig.allow_missing_exp_from — for a host's own long-lived tokens whose validity it tracks itself. A third-party token with no expiry is an indefinitely replayable credential its issuer never meant to issue. Successful exchanges are logged at info with issuer, subject, role and duration; before only failures were. Claim validation is factored out of verify_token into validate_claims, taking the clock, so each rule is tested without a key pair. BREAKING CHANGE: a role must set required_audiences and subject_conditions (use ["*"] to accept any subject); RoleConfig gains allow_missing_exp_from, which struct-literal constructors must add. Refs #143 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z
…n lacks resolve_scopes substituted an empty string for a missing or non-string claim. For a bucket that fails safe; for a prefix an empty string matches every key, so a missing claim silently granted the whole bucket — the opposite of what the doc comment promised. An unresolvable template is now an error at mint time, and mint_temporary_credentials returns Result. Closes #143 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z
JwtSigner::sign always dates the token itself: fresh jti, nbf, and exp at now plus the signer's ttl. A host that issues long-lived API keys with server-side revocation needs to choose those claims itself — a jti it stores to revoke against, and an exp the key's owner sets or leaves out. sign_claims signs exactly the object it is given, under the same key and kid, and adds nothing. sign is unchanged; both share the encoding and signing tail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z
|
Claude finished @alukach's task in 10s —— View job ✅ No blocking issues — safe to merge.
Non-blocking notes:
Simplify (ponytail)
💰 Estimated review cost: $0.10 · 0m09s · 4 turns |
3 tasks done
Config validation now rejects roles without required_audiences, so the worker failed to start on the preview deploy. GitHub roles use sts.amazonaws.com (what the smoke/integration tests request); the source.coop role uses https://data.staging.source.coop. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🚀 Latest commit deployed to https://multistore-proxy-pr-147.development-seed.workers.dev
|
alukach
added a commit
to source-cooperative/data.source.coop
that referenced
this pull request
Sep 29, 2026
…he platform (#234) One commit on `main`, now that #232 (ADR-014) is merged. It revises ADR-013 in place, since ADR-013 was never implemented, and it replaces an earlier draft that added an ADR-015 and then folded it back. Part of source-cooperative/source.coop#491. Records the decision that replaces #233, developmentseed/multistore#147 and the proxy half of source-cooperative/source.coop#570. ## What it records **ADR-013, revised: API keys are opaque secrets resolved by the platform.** A key is `sck_` + 32 random bytes (a fixed 47 characters, `^sck_[A-Za-z0-9_-]{43}$`), stored as a sha256 hash on the key record in source.coop and shown once. Nothing signs it. At `/.sts` the proxy accepts it as `WebIdentityToken` from a POST body only, trims and format-checks it locally, hashes it, and asks `POST /api/v1/service-account-keys/exchanges` for `{account_id, key_id, active}`, cached 60s positive and negative, failing closed; then mints session credentials through the same code as every other exchange. The cache-miss path is rate-limited by client IP. A stock AWS SDK does the exchange and the refresh itself from `AWS_WEB_IDENTITY_TOKEN_FILE`; the emergency stop for a leaked key is disabling the service account. **Why the first Decision was withdrawn** is recorded under Context, each point checkable against #233 and source.coop#570: the revocation lookup was never optional, so the signature verified what the lookup restates; a Worker cannot fetch its own JWKS, so the "no new path" benefit was gone; minting was a source.coop→Ory→proxy→source.coop cycle whose `/.keys` would sign any `jti` for a manager; `exp` in the token overrode the editable record; and non-expiring keys died on the second rotation of a signing key shared with outbound federation. The ADR's original Context and its rejected alternatives stand. **Amendments**, in ADR-014's house form: ADR-005 gains the one proxy-as-itself route, with the sentinel subject `urn:source:data-proxy` that fails both account-id grammars; ADR-014's first two bullets amending ADR-013 lose their `sub`/`jti` wording. The RFC index gains ADR-014's row, which #232 omitted. It also fixes a sentence in ADR-014's "How it authenticates" section, which the automated review caught: it said a key's subject is the service account's id, and now says a key names its account on the key record. **Alternatives** rejected with reasons: the proxy-signed JWT (the first Decision), source.coop-signed JWTs verified via a source.coop JWKS, the two-hop exchange (opaque key → short-lived Ory ID token → `/.sts`), the proxy's `get_credential` slot, Ory-native tokens, and long-lived Ory refresh tokens. ## The two-hop spike the ADR cites Run on 2026-09-25 against the staging Ory project, driving the headless authorization-code flow source.coop already uses for a person's proxy credentials (`getOryIdToken` on `main`) with a service-account-shaped subject and, as a control, a random UUID that matches no identity. Both returned an RS256 ID token from `https://auth.staging.source.coop` whose `sub` was exactly the subject given, with the default 3600s lifetime. The run used a throwaway confidential OAuth2 client created and deleted for the purpose. Conclusion: Ory Network will mint for a subject with no identity record, so the two-hop design needs no proxy change and remains available; the ADR rejects it for the first release on the client-side cost to HPC and VM users, not on feasibility. ## Review Five targeted reviews of the plan and the decision text (security, proxy implementer, source.coop implementer, ops and client experience, ADR and issue hygiene), each returning "approve with changes"; all changes are folded in. The ones that moved a decision: keys refused in the query string (invocation logs are on; GDAL sends STS as a GET and is routed through the CLI); the sentinel is a URN, not a slug; unknown keys answer `active: false` rather than 404 so the existing cache code applies; rate limiting is per IP on misses and ships with the branch; the revocation numbers distinguish writes (60s) from restricted reads (300s) per the proxy's caches; ADR-005 is amended, not merely depended on. ## What follows #235, the proxy PR replacing #233 (its exchange branch survives; minting, self-verification and the multistore pin do not), source.coop#570 reworked in place, multistore#147 closed, #231 rewritten, and the epic's Phase 5 items edited. ## PR Checklist - [x] This PR has **no** breaking changes. (Documentation only.) - [x] I have updated or added new tests to cover the changes in this PR. (None apply; no code.) - [x] This PR affects the [Source Cooperative Frontend & API](https://github.com/source-cooperative/source.coop), and I have opened issue/PR source-cooperative/source.coop#570 to track the change. (Existing PR, to be reworked to this ADR.) ## Related Issues #230, #231, #232, #233, #235; developmentseed/multistore#146, developmentseed/multistore#147; source-cooperative/source.coop#491, source-cooperative/source.coop#548, source-cooperative/source.coop#561, source-cooperative/source.coop#570, source-cooperative/source.coop#580. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
alukach
added a commit
to source-cooperative/data.source.coop
that referenced
this pull request
Sep 30, 2026
Supersedes #233. Implements the proxy half of API keys as recorded in the revised ADR-013 (#234, stacked on #232). Pairs with source-cooperative/source.coop#580 (into source-cooperative/source.coop#570's branch), which serves the route this calls. Closes #231. Part of source-cooperative/source.coop#491. ## What I'm changing A service account's API key is an opaque `sck_` secret, 30 random base62 characters and a six-character CRC-32 checksum of them (ADR-013, #242), that source.coop stores only as a SHA-256 hash. Nothing signs it. `/.sts` now accepts one as `WebIdentityToken` and resolves it by asking source.coop: - **Body only.** A key is read from the POST form body. One in the query string is refused before any lookup with `API key must be sent in the request body, not the URL (request id …)`, because Cloudflare logs request URLs. A JWT in the query string still goes to the STS route as before. - **Local checks first.** Trim surrounding whitespace, since every hand-made token file ends in a newline, then require exactly `sck_` + 36 base62 characters whose last six are the CRC-32 of the thirty before them (IEEE, as zlib computes it, written out in a few lines rather than a crate). Anything else with the `sck_` prefix is refused without a lookup. - **One lookup, cached.** `POST {SOURCE_API_URL}/api/v1/service-account-keys/exchanges` with `{"key_hash"}`, authenticated as the proxy itself (sentinel subject `urn:source:data-proxy`, the one route ADR-005 now lets the proxy call as itself). The route always answers 200: `{account_id, key_id, active: true}` or `{active: false}`. Both are cached for 60 seconds, so revocation takes effect within a minute and an unknown key costs one lookup a minute. A non-200 or network failure fails closed as a 500 `InternalError`, which SDKs retry, and caches nothing. - **One refusal for the key, and one for a mangled one.** A key that fails its shape or checksum was cut short or mistyped, and reads `InvalidIdentityToken: API key is malformed; check that it was copied whole (request id …)`; the format is public, so this reveals nothing, and it still counts against the rate limit. Unknown, revoked, expired and disabled all read `InvalidIdentityToken: API key was not accepted (request id …)`, with the id also in `x-amzn-requestid`. The id is in the message because SDKs show the user nothing else. The proxy logs one WARN line per refusal with the reason it knows (`malformed`, `inactive`, `query_string`, `rate_limited`) plus the key id and an 8-hex hash prefix; source.coop logs which of unknown, revoked, expired or disabled under the same request id. - **Minting.** Credentials for the named account under the `_default` role, sealed like every other session, with the same 900s floor, 3600s default and `STS_MAX_SESSION_DURATION_SECS` cap. `RoleArn`'s account segment is ignored, as for an Ory token. `ReadOnly`/`FullAccess` arrive with #221. - **Rate limit.** A new `KEY_EXCHANGE_LIMIT` ratelimit binding, 100 attempts a minute per client IP, in every wrangler config. A client exchanges about once a session, so a cluster behind one NAT stays far under it. A deployment without the binding logs an error and does not refuse traffic. **Decisions to flag** - **The limit applies to every `sck_` attempt, not only cache misses.** The cache sits inside the fetch helper, and at 100 a minute per IP the distinction makes no difference to legitimate traffic. ADR-013's wording is updated in #234 to match. - **No `<RequestId>` element in the STS error XML.** That lives in multistore-sts's `build_sts_error_response`; the header plus the message cover what SDKs surface, so no upstream change is needed now. - **`namespace_id`s `1001` (production), `1002` (staging) and `1003` (previews)** for the ratelimit binding. They only need to be unique within the Cloudflare account. - **CodeQL flags `key_hash` as weak password hashing (`src/keys.rs`), and it should be dismissed as a false positive.** The rule matches on the name: it takes the key for a password. A key is 256 random bits, so a slow hash or salt adds nothing, and the lookup is a key get, so there is no comparison to time. This is how GitHub stores its own tokens, and ADR-013 (#234) records it so nobody later "fixes" it to bcrypt. I have not dismissed the alert; that is the repo owner's call. - **Security Audit.** It failed here because `main`'s lockfile carried a rustls advisory (RUSTSEC-2026-0285). #238 fixed that on `main`, and this branch is rebased onto the fix. ## How I did it - `src/keys.rs` (new, wasm-free): `parse_api_key`, `looks_like_api_key`, `key_hash`, `KeyStanding`, `credentials_for`. - `src/lib.rs`: `api_key_exchange` runs after `ApiAuth` is built and before the router, and returns `None` for anything that isn't an `sck_` exchange so the STS route handles it unchanged. `exchange_api_key` does the lookup and minting; `key_refusal` builds the uniform refusal; `finish` adds CORS and the request-id headers to a pre-gateway response; `within_rate_limit` wraps the binding. - `src/source_api/auth.rs`: `PROXY_SELF_SUBJECT`, `ApiCaller` (anonymous, an account, or the proxy), `authorization_header_as_self`; `authorization_header` refuses the sentinel so no request can claim it. - `src/source_api/cache.rs`: `get_or_fetch_key_standing`; `cached_fetch` takes a method, an optional JSON body and an `ApiCaller`. The cache key is `{api_url}?key_hash={hash}`, so it is a URL, as the Cache API requires, and is scoped to the environment's API. - `src/sts.rs`: `default_role` factored out of `StsCredentialRegistry::new`. - `wrangler.toml`, `wrangler.preview.toml`: the binding per environment. `README.md`: a Bindings table and an API keys section. From #233 this keeps `finish`, the shape of `api_key_exchange`/`exchange_api_key`, `credentials_for` and the method argument to `cached_fetch`. It drops `/.keys`, minting, self-verification against the proxy's own JWKS, the `api_key` role, `chrono`/`rsa` and the `[patch.crates-io]` pin on multistore; developmentseed/multistore#147 is not needed. ## How to test it - `cargo test`: all suites, including the new `tests/keys.rs` (a key's shape, trimming `\n` and `\r\n`, rejection of short, long, wrong-case, mistyped, wrong-checksum, non-base62, checksum-less 47-character and JWT tokens, checksum vectors computed independently with Python's zlib (a CRC above 2^31, one with a leading zero), assembled with `concat!` so scanners don't flag the file, the SHA-256 test vector, and credentials sealed for the account within floor, default and cap). Run by the pre-commit hook, along with `cargo clippy --target wasm32-unknown-unknown -- -D warnings` and `cargo check --target wasm32-unknown-unknown`. - `pytest tests/test_api_keys.py` against `wrangler dev` and `tests/stub_api.py`, which gains the exchanges route keyed by the hash of fixed test keys, with a per-hash call counter. Nine tests, all passing locally with wrangler 3.114: a live key exchanges; **an unmodified boto3, configured only by `AWS_ROLE_ARN`, `AWS_WEB_IDENTITY_TOKEN_FILE` (a file holding the key and a trailing newline) and `AWS_ENDPOINT_URL_STS`, acquires credentials**; the second exchange within 60s never reaches the API; a trailing newline is harmless; unknown and revoked keys get byte-identical refusals apart from the id, and the refusal is cached; a key in the query string is refused without a lookup; malformed keys, including a mistyped one whose checksum fails, are refused without a lookup and with the malformed message; a wrong role is reported as such; an API 500 fails closed and is not cached. `test_control_plane.py` and `test_writes.py` still pass; their credentialed tests need CI's GitHub token and were skipped locally. The checksum commits (d6745e0, 30ad22f) were run by CI's Integration Tests job, which exchanges the new-format keys against the worker; it passed on both. - End to end, once source.coop#580 is on a deployment this preview points at: issue a key from a service account's page, save it to a file, then ```sh AWS_WEB_IDENTITY_TOKEN_FILE=./key AWS_ROLE_ARN=arn:aws:iam::000000000000:role/_default \ AWS_ENDPOINT_URL_STS=https://<preview>/.sts AWS_ENDPOINT_URL_S3=https://<preview> AWS_REGION=us-east-1 \ aws s3 ls s3://<owner>/<product>/ ``` Revoke the key and see the next exchange refused within 60s, then quote the printed request id to find the proxy's and source.coop's log lines. Not run here: it needs source-cooperative/source.coop#580 deployed. ## PR Checklist - [x] This PR has **no** breaking changes. (JWT exchanges at `/.sts` are unchanged; the new binding is additive.) - [x] I have updated or added new tests to cover the changes in this PR. - [x] This PR affects the [Source Cooperative Frontend & API](https://github.com/source-cooperative/source.coop), and I have opened issue/PR source-cooperative/source.coop#580 to track the change. ## Related Issues Closes #231. Supersedes #233. ADR: #234 (revises ADR-013, amends ADR-005). Route: source-cooperative/source.coop#580, into source-cooperative/source.coop#570. Epic: source-cooperative/source.coop#491. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #146 (same base branch); only the last commit is this PR.
What I'm changing
JwtSigner::signalways dates the token itself: a freshjti,nbf, andexpat now plus the signer's TTL. That is right for the proxy's own short-lived assertions and wrong for a host that issues long-lived API keys with server-side revocation, which is what source-cooperative/source.coop#491 needs the proxy to mint: the host chooses thejti(it stores it, and revokes against it) and theexp(the key's owner sets one, or leaves it out and relies on revocation — the case #146'sallow_missing_exp_fromexists for).sign_claims(&self, claims: &Value)signs exactly the object it is given, under the same key andkid, and adds nothing. A non-object is an error.signis unchanged; both share a privatesign_encodedtail.How I did it
crates/oidc-provider/src/jwt.rs—sign_claims, the sharedsign_encoded, and a test that the payload round-trips byte-for-byte with noexpadded and the header carries the signer'skid.Test plan
cargo fmtcargo test -p multistore-oidc-provider— 41 passedcargo clippy -p multistore-oidc-provider— clean🤖 Generated with Claude Code
https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z