fix(auth): validate issuer, audience and expiry on the token response; tell users to retry a rejected code - #18
Merged
Conversation
…; tell users to retry a rejected code TL;DR: Review follow-ups on the central-auth client. Explorer now checks that the identity it receives was minted by the issuer it asked, for this client, and is still within its window; a consumed or superseded sign-in code tells the user to try again instead of blaming an outage; and two constant-time compares can no longer crash on non-ASCII input. Problem: - exchange() trusted sub, email and nonce from the token response and ignored iss, aud and exp. The response arrives over an authenticated TLS backchannel, so this was a misconfiguration risk rather than an attack path, but a response minted for another client or replayed after its window would still have created a session. - Every token-endpoint failure became a 502 "authentication service is unavailable". The auth service answers 400 invalid_grant for a consumed, expired, or superseded code, which happens whenever a user has two tabs sign in at once (the newer /authorize invalidates the older code). That is a retry, not an outage, and the message sent people looking for a problem that did not exist. - hmac.compare_digest raises TypeError on str arguments containing non-ASCII characters. The state value comes from the callback query string, so any visitor could turn the callback into a 500. The nonce compare had the same shape. - The bootstrap wrote .env with mode 0640 although it now holds the central-auth client secret alongside the session secret and AWS keys. Fix: - exchange() requires iss to equal the configured issuer origin, aud to equal the client id, and exp to be an integer not more than 60 seconds in the past (assertions live five minutes; the skew tolerance covers clock drift between hosts). Booleans are rejected as exp even though they are ints in Python. - New CodeExchangeRejectedError for HTTP 400 from the token endpoint; the callback maps it to 400 "Sign-in expired. Try again." 401 (invalid_client) and 5xx stay 502 because they are deployment or availability problems. - State and nonce compares operate on UTF-8 bytes. - bootstrap.sh writes .env with mode 0600. Tests: - New: wrong aud, wrong iss, expired exp, string exp, and boolean exp are all rejected; 10 seconds of skew is tolerated; a non-ASCII nonce is rejected rather than raising; HTTP 400 from the token endpoint is CodeExchangeRejectedError while 401 stays a plain CentralAuthError; the callback returns 400 with "Try again" for a rejected code and never exchanges a code when the state is non-ASCII. - Updated: the two unit-test fakes now return iss, aud and exp like the real service. - Ran: .venv/bin/python -m pytest -q (98 passed), ruff check, ruff format --check, bash -n and shellcheck on bootstrap.sh, update.sh and explorer-cli. (cherry picked from commit 5cb7fc3)
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.
TLDR
Review follow-ups on the central-auth client (#17), cherry-picked onto
mainbecause #17 merged while the review was in flight. Explorer now checks that the identity it receives was minted by the issuer it asked, for this client, and is still within its window; a consumed or superseded sign-in code tells the user to try again instead of blaming an outage; two constant-time compares can no longer crash on non-ASCII input; and the bootstrap writes.envowner-only.Problem
exchange()trustedsub,emailandnonceand ignorediss,audandexp. Over an authenticated TLS backchannel this is a misconfiguration risk rather than an attack path, but a response minted for another client or replayed after its window would still have created a session.invalid_grantfor a consumed, expired, or superseded code, which two tabs signing in at once produce routinely. That is a retry, not an outage.hmac.compare_digestraisesTypeErroronstrarguments containing non-ASCII characters.statecomes from the callback query string, so any visitor could turn the callback into a 500. The nonce compare had the same shape..envwas written 0640 although it now holds the client secret, the session secret, and AWS keys.Fix
exchange()requiresissto equal the configured issuer origin,audto equal the client id, andexpto be an integer not more than 60 seconds in the past (assertions live five minutes). Booleans are rejected asexp.CodeExchangeRejectedErrorfor HTTP 400 from the token endpoint; the callback maps it to 400 "Sign-in expired. Try again." 401 and 5xx stay 502.bootstrap.shwrites.envwith mode 0600.Tests
aud, wrongiss, expired, string and booleanexpare rejected; 10 seconds of skew is tolerated; a non-ASCII nonce is rejected rather than raising; HTTP 400 isCodeExchangeRejectedErrorwhile 401 is a plainCentralAuthError; the callback returns 400 with "Try again" for a rejected code and never exchanges a code when the state is non-ASCII.iss,audandexplike the real service.pytest -q(98 passed), ruff check, ruff format --check, bash -n and shellcheck on bootstrap.sh, update.sh and explorer-cli.Review notes and the items left for the owner are on #17.