developmentseed / developmentseed/multistore

sts: fail closed on empty trust fields, check token type, require exp, log successes

Open
#143 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
19
Forks
1
PR merge metrics
No merged PRs in 30d

Description

Four small hardening items in `multistore-sts`. None is reachable from the Source Cooperative deployment today, because it serves a single hardcoded Role — and all four become reachable the moment Roles are user-authored.

**Today**
- An empty `required_audiences` accepts **any** audience (`jwks.rs`, `audience_allowed` returns true on an empty list).
- An empty `subject_conditions` skips the subject check entirely.
- Both carry `#[serde(default)]`, so a Role that omits either field silently trusts everything from its issuer. (`trusted_oidc_issuers` is also defaulted but correctly fails closed.)
- There is no token-type (`typ`) check.
- A token with no `exp` is accepted — the expiry check is inside `if let Some(exp)`. This matters for tokens from *third-party* issuers, where a missing expiry means an indefinitely replayable credential the issuer never intended.
- Successful exchanges are never logged; only errors are.

**Do**
- Make an empty audience list and an empty subject-condition list mean **deny**.
- Add a token-type check.
- Reject tokens with no `exp` **from issuers other than the proxy itself**. See the caveat below — a blanket rule would break a legitimate case.
- Log successful exchanges.

**Also worth fixing here**
`resolve_scopes` substitutes `{claim}` templates into scope prefixes, and a missing claim resolves to an empty string. For `bucket` that fails safe, but for `prefixes` an empty string matches every key — so a missing claim grants **full-bucket access**. The doc comment claims it fails safe. Treat an unresolved placeholder as a hard error at mint time.

> [!IMPORTANT]
> **Do not require `exp` unconditionally.** A host may legitimately mint its own long-lived tokens whose validity is tracked server-side rather than in the claim — Source Cooperative's API keys work this way, with a nullable `expires_at` on a revocation record the exchange already reads. A blanket rule rejects exactly those.
>
> Scope the requirement to tokens the host did not mint: require `exp` when `iss` is not the host's own issuer, or exempt a token carrying a host-recognised type claim. Whichever shape it takes, the exemption should be explicit rather than a side effect of configuration.

**Done when**
Released, and the dependency bumped in `source-cooperative/data.source.coop`.

---

Part of source-cooperative/source.coop#491.

Contributor guide

Open the contributing guide

Research direction

Begin in jwks.rs, especially audience_allowed, and trace the token exchange and resolve_scopes entry points. Review the existing validation and logging tests, if present, before covering empty trust fields, token type, issuer-scoped expiry, unresolved scope claims, and successful exchanges. Done means the hardening behavior is tested, released, and the dependency is bumped in source-cooperative/data.source.coop.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
authentication, backend-api-design, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.