Bound the API key Argon2 scan and drop the media token expiry raise - #326
Draft
arsfeld wants to merge 4 commits into
Draft
Bound the API key Argon2 scan and drop the media token expiry raise#326arsfeld wants to merge 4 commits into
arsfeld wants to merge 4 commits into
Conversation
refresh_media_token/3 called DateTime.from_unix!/1 on claims["exp"], which raises on a claim set with no usable expiry. The mutation is public, so a surprising claim set would crash the request rather than return an error. The expiry now flows through expires_at_from_claims/1, the same helper refresh_access_token/3 uses, and a missing expiry is reported like every other failure. MediaToken.create_token/2 passes an explicit ttl so Guardian always writes an integer exp today; this closes the hazard before that assumption changes rather than after. No rate limiting here: MediaToken.refresh_token/1 is an HMAC verify plus one primary-key lookup, with no Argon2 in the path, so the amplification argument that justified rate limiting refresh_access_token/3 does not apply.
Keys created between the api_keys table landing (20251104023006) and the key_prefix column landing (20251226020220) have a NULL prefix, and it cannot be backfilled because the prefix is not recoverable from an Argon2 hash. Verification is about to narrow by prefix, which makes these keys stop authenticating. Revoking them first means the operator sees a revoked key in the admin UI rather than one that fails for no stated reason. The statement skips rows that already carry a revoked_at so a manual revocation keeps its original timestamp, and down/0 is deliberately a no-op because rollback cannot tell this migration's revocations apart from the operator's.
verify_api_key/1 loaded every row of api_keys with its user preloaded and ran Argon2 against each one until a match. A wrong key therefore cost one full Argon2 pass per active key, which turns API authentication into a CPU amplifier, and the full-table load ran on successful requests too. key_prefix is already stored, populated by the only creation path, and indexed, and it is derivable from the presented key. Narrowing by it makes a wrong key with an unrecognised prefix cost zero Argon2 passes and a recognised one cost exactly one. Keys predating the key_prefix column match nothing and were revoked by the preceding migration. IP rate limiting in ApiAuth is unchanged; this removes the amplification factor underneath it.
… comment Review round 1 found three issues in the prefix-less key revocation work (514bf82): - The two new tests bound the timestamp as a query parameter using the `?` placeholder, which is exqlite syntax. Postgrex requires `$1`, so both tests were a syntax error on the Postgres CI job. Rewritten to interpolate the timestamp as a literal, matching the migration's own statement byte-for-byte and requiring no placeholder syntax at all. - Because the tests previously bound a parameter instead of interpolating a literal, they never actually exercised the migration's real statement. They now do. - The migration comment and commit message for 514bf82 claimed the revoked key becomes visible "in the admin UI." There is no admin UI for API keys, only the GraphQL `apiKeys` query. Reworded to say the key's state is inspectable via the API and explicable in release notes instead. Also relaxes `ApiKey.key_prefix` from non-null to nullable in the GraphQL schema (lib/mydia_web/schema/common_types.ex). Legacy rows have a NULL key_prefix, so a client selecting `keyPrefix` on one of the keys this migration revokes hit a non-null violation and could not see `revokedAt`, the one field that would explain why the key stopped working. No client depends on the non-null guarantee.
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
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.
Two follow-ups deliberately left out of scope in #301, plus the review fixes they turned up.
API keys created before 2025-12-26 stop working and are marked revoked. Those keys predate the
key_prefixcolumn and have it NULL, and the prefix cannot be backfilled because it is not recoverable from an Argon2 hash. Verification now matches on prefix, so they can no longer authenticate. The remedy is to issue a new key.This needs a release note. Mydia has no admin UI for API keys, so the release note is the only channel that reaches an operator wondering why their key stopped working. It should name the date boundary and the remedy.
What changed
verify_api_key/1narrowed to an indexed prefix lookup. It previously loaded every row ofapi_keyswith its user preloaded and ran Argon2 against each until one matched. A wrong key therefore cost one full Argon2 pass per active key, which turns API authentication into a CPU amplifier, and the full-table load ran on successful requests too.key_prefixis already stored, populated by the only creation path, indexed, and derivable from the presented key, so narrowing by it gives:The everyday win is arguably the success path, which no longer loads the whole table. Rate limiting in
ApiAuthis unchanged; this removes the amplification factor underneath it.The hash check is untouched. Prefix matching is an additional filter, never a substitute: a key still has to pass
Argon2.verify_pass, and revocation and expiry are still enforced.refresh_media_token/3reports a missing expiry instead of raising. It calledDateTime.from_unix!/1onclaims["exp"]. It is a public unauthenticated mutation, so a raise would crash the request. This is defensive only:MediaToken.create_token/2passes an explicit ttl, so Guardian always writes an integerexpand it cannot raise today. No client-visible change on any existing path; all five existing error strings are byte-identical.ApiKey.keyPrefixrelaxed fromnon_null(:string)to:string. Found in review. Legacy rows have it NULL, so a client selectingkeyPrefixhit a non-null violation on precisely the rows the migration revokes, hiding the keys an operator would need to diagnose. This is a latent bug independent of the rest of this PR.Migration ordering
The migration lands before the query narrowing, so no commit leaves a key silently failing without having been marked revoked.
At runtime there is no window either:
Ecto.Migratoris child #3 in the supervision tree and the endpoint starts later, so migrations complete before any request is served. The exception is a devmix phx.serverrun where migrations are skipped, and there the key is equally dead, just without the recorded reason. It self-heals on the next migrate.down/0is deliberately a no-op. Rollback cannot distinguish keys this migration revoked from keys the operator revoked by hand, and un-revoking on rollback is the wrong default for a security action. Note thatmix ecto.rollbackwill therefore report success while doing nothing.Known trade-off
A prefix miss returns in microseconds while a prefix hit costs a full Argon2 pass, so the endpoint now discloses whether a given 8-character prefix exists. That is inherent to the zero-pass goal, constant time and zero-pass are incompatible. Across a 62^8 space behind per-IP rate limiting it is uninteresting, but it is a deliberate choice rather than an oversight.
Verification
✓ Precommit passed(compile, unused deps, format, credo --strict, tests); full suite6217 tests, 0 failures, 34 skipped.22 tests, 0 failureson the API key file against a real PG 17.10 instance. Review round 1 caught tests using the exqlite?placeholder, which is a Postgrex syntax error and would have gone red on thetest-postgresCI job; they now use a placeholder-free literal that also exercises the migration's actual statement rather than a different bound-parameter path.CardigannTesttiming flake (clean on isolated re-run, 15 tests 0 failures) and twoMediaImportTestfailures from the documented deep-worktree-pathvarchar(255)tmp_dir hazard. Neither touches any file this PR changes.Note on commit messages
The body of 514bf82 says revocation makes the key visible "in the admin UI". That is wrong, there is no admin UI for API keys, and the claim came from the design doc. Review caught it. The migration comment is corrected in 09293ba; the commit message is left as-is rather than rewriting history.