diff --git a/app/auth.py b/app/auth.py index 7fd02a0..47af2d1 100644 --- a/app/auth.py +++ b/app/auth.py @@ -275,7 +275,9 @@ def complete_login( or not isinstance(nonce, str) or not isinstance(next_path, str) or not isinstance(created_at, int) - or not hmac.compare_digest(expected_state, state) + or not hmac.compare_digest( + expected_state.encode("utf-8"), state.encode("utf-8") + ) or now < created_at or now - created_at > LOGIN_TRANSACTION_SECONDS ): diff --git a/app/central_auth.py b/app/central_auth.py index cf5369f..c3e99e9 100644 --- a/app/central_auth.py +++ b/app/central_auth.py @@ -57,6 +57,25 @@ class AuthTransactionError(CentralAuthError): """The browser callback did not match a live login transaction.""" +class CodeExchangeRejectedError(CentralAuthError): + """The auth service refused the code: expired, replayed, or superseded. + + This is the user's problem to retry, not an outage: a second tab, a slow + click, or a stale bookmark produces it. Callers should say "try again", + not "the service is unavailable". + """ + + +# Tolerated skew between this host's clock and the auth service's when +# checking the assertion expiry. The assertion lives five minutes. +CLOCK_SKEW_SECONDS = 60 + + +def _constant_time_equal(expected: str, actual: str) -> bool: + """compare_digest on str raises TypeError for non-ASCII; compare bytes.""" + return hmac.compare_digest(expected.encode("utf-8"), actual.encode("utf-8")) + + @dataclass(frozen=True) class AuthenticatedPrincipal: subject: str @@ -229,12 +248,19 @@ def exchange( try: with _open_token_request(request, self.timeout_seconds) as response: raw = response.read(MAX_TOKEN_RESPONSE_BYTES + 1) - except ( - urllib.error.HTTPError, - urllib.error.URLError, - TimeoutError, - OSError, - ) as exc: + except urllib.error.HTTPError as exc: + # 400 is the auth service's invalid_grant: the code was consumed, + # expired, or superseded by a newer /authorize for this browser. + # Anything else (401 invalid_client, 5xx) is a deployment or + # availability problem. + if exc.code == 400: + raise CodeExchangeRejectedError( + "The authentication service rejected the sign-in code" + ) from exc + raise CentralAuthError( + "The authentication service rejected the code exchange" + ) from exc + except (urllib.error.URLError, TimeoutError, OSError) as exc: raise CentralAuthError( "The authentication service rejected the code exchange" ) from exc @@ -250,13 +276,28 @@ def exchange( subject = payload.get("sub") email = payload.get("email") nonce = payload.get("nonce") + issuer = payload.get("iss") + audience = payload.get("aud") + expires_at = payload.get("exp") + now = int(time.time()) + # The response arrives over an authenticated TLS backchannel, so these + # checks defend against misconfiguration rather than an attacker: a + # response minted by a different issuer, for a different client, or + # replayed after its assertion window must not create a session. if ( not isinstance(subject, str) or not subject or len(subject) > 255 or not isinstance(email, str) or not isinstance(nonce, str) - or not hmac.compare_digest(nonce, expected_nonce) + or not _constant_time_equal(expected_nonce, nonce) + or not isinstance(issuer, str) + or issuer.rstrip("/") != self.issuer_url + or not isinstance(audience, str) + or audience != self.client_id + or isinstance(expires_at, bool) + or not isinstance(expires_at, int) + or expires_at + CLOCK_SKEW_SECONDS <= now ): raise CentralAuthError("The authentication response was invalid") try: diff --git a/app/main.py b/app/main.py index ebdc16d..0461dd5 100644 --- a/app/main.py +++ b/app/main.py @@ -33,6 +33,7 @@ AccessDeniedError, AuthTransactionError, CentralAuthError, + CodeExchangeRejectedError, verify_csrf_token, ) from app.config import settings @@ -616,6 +617,12 @@ def auth_callback( status_code=403, detail="Your account does not have access to this Explorer instance.", ) from exc + except CodeExchangeRejectedError as exc: + # A consumed, expired, or superseded code (for example a second tab + # that started its own sign-in) is a retry, not an outage. + raise HTTPException( + status_code=400, detail="Sign-in expired. Try again." + ) from exc except CentralAuthError as exc: raise HTTPException( status_code=502, detail="The authentication service is unavailable." diff --git a/scripts/bootstrap.sh b/scripts/bootstrap.sh index 71b7f2f..ff24cf4 100755 --- a/scripts/bootstrap.sh +++ b/scripts/bootstrap.sh @@ -237,7 +237,9 @@ EMAIL_S3_BUCKET="$EMAIL_S3_BUCKET" # AWS_SECRET_ACCESS_KEY= EOF chown "$APP_USER:$APP_USER" "$ENV_FILE" -chmod 0640 "$ENV_FILE" +# Owner-only: this file carries the session secret, the central-auth client +# secret, and any AWS keys. +chmod 0600 "$ENV_FILE" ok "env seeded at $ENV_FILE" # ── step 4b: optional encrypted config bundle ─────────────────────────── diff --git a/tests/test_central_auth.py b/tests/test_central_auth.py index 59b9b75..562d1ca 100644 --- a/tests/test_central_auth.py +++ b/tests/test_central_auth.py @@ -9,6 +9,8 @@ import json import sqlite3 import stat +import time +import urllib.error import urllib.parse import pytest @@ -18,6 +20,7 @@ CentralAuthClient, CentralAuthError, CentralAuthStore, + CodeExchangeRejectedError, csrf_token_for_session, verify_csrf_token, ) @@ -117,6 +120,9 @@ def read(self, _limit): "sub": "account-123", "email": "Alice@Example.com", "nonce": "expected-nonce", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, } ).encode() @@ -192,7 +198,16 @@ def __exit__(self, *args): return None def read(self, _limit): - return b'{"sub":"account-123","email":"alice@example.com","nonce":"wrong"}' + return json.dumps( + { + "sub": "account-123", + "email": "alice@example.com", + "nonce": "wrong", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, + } + ).encode() monkeypatch.setattr( "app.central_auth._open_token_request", lambda *_args: Response() @@ -210,3 +225,108 @@ def read(self, _limit): code_verifier="pkce-verifier", expected_nonce="expected-nonce", ) + + +def _client() -> CentralAuthClient: + return CentralAuthClient( + issuer_url="https://auth.example.com", + public_url="https://explorer.example.com", + client_id="explorer", + client_secret="a-random-client-secret-with-32-bytes", + ) + + +def _fake_response(payload: dict): + class Response: + def __enter__(self): + return self + + def __exit__(self, *_args): + return None + + def read(self, _limit): + return json.dumps(payload).encode() + + return Response() + + +def _valid_payload() -> dict: + return { + "sub": "account-123", + "email": "alice@example.com", + "nonce": "expected-nonce", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, + } + + +@pytest.mark.parametrize( + "mutation", + [ + {"aud": "lens"}, + {"iss": "https://other-auth.example.com"}, + {"exp": int(time.time()) - 120}, + {"exp": "soon"}, + {"exp": True}, + ], +) +def test_code_exchange_rejects_wrong_audience_issuer_or_expired( + monkeypatch, mutation +) -> None: + payload = _valid_payload() | mutation + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + with pytest.raises(CentralAuthError, match="invalid"): + _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + + +def test_code_exchange_tolerates_small_clock_skew(monkeypatch) -> None: + payload = _valid_payload() | {"exp": int(time.time()) - 10} + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + principal = _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + assert principal.subject == "account-123" + + +def test_non_ascii_nonce_from_issuer_is_rejected_not_crashed(monkeypatch) -> None: + payload = _valid_payload() | {"nonce": "expected-nonc\u00e9"} + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + with pytest.raises(CentralAuthError, match="invalid"): + _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + + +def test_http_400_from_token_endpoint_is_a_rejected_code(monkeypatch) -> None: + def refuse(*_args): + raise urllib.error.HTTPError( + "https://auth.example.com/token", 400, "Bad Request", {}, None + ) + + monkeypatch.setattr("app.central_auth._open_token_request", refuse) + with pytest.raises(CodeExchangeRejectedError): + _client().exchange(code="code", code_verifier="v" * 43, expected_nonce="n") + + +def test_http_401_from_token_endpoint_is_an_outage_not_a_retry(monkeypatch) -> None: + def refuse(*_args): + raise urllib.error.HTTPError( + "https://auth.example.com/token", 401, "Unauthorized", {}, None + ) + + monkeypatch.setattr("app.central_auth._open_token_request", refuse) + with pytest.raises(CentralAuthError) as excinfo: + _client().exchange(code="code", code_verifier="v" * 43, expected_nonce="n") + assert not isinstance(excinfo.value, CodeExchangeRejectedError) diff --git a/tests/test_central_auth_endpoints.py b/tests/test_central_auth_endpoints.py index 0e211b5..40d619a 100644 --- a/tests/test_central_auth_endpoints.py +++ b/tests/test_central_auth_endpoints.py @@ -17,6 +17,7 @@ CentralAuthClient, CentralAuthError, CentralAuthStore, + CodeExchangeRejectedError, ) @@ -191,6 +192,31 @@ def test_callback_fails_closed_when_code_exchange_fails(central_client) -> None: assert CENTRAL_AUTH_COOKIE_NAME not in client.cookies +def test_rejected_code_is_a_retry_not_an_outage(central_client) -> None: + client, _store = central_client + FakeAuthClient.exchange_error = CodeExchangeRejectedError("code consumed") + + response = complete_login(client) + + assert response.status_code == 400 + assert "Try again" in response.text + assert CENTRAL_AUTH_COOKIE_NAME not in client.cookies + + +def test_non_ascii_state_is_rejected_not_crashed(central_client) -> None: + client, _store = central_client + begin_login(client) + + response = client.get( + "/auth/callback", + params={"code": "irrelevant", "state": "\u00e9tat"}, + follow_redirects=False, + ) + + assert response.status_code == 400 + assert FakeAuthClient.exchanged_codes == [] + + def test_external_next_url_is_not_used_after_callback(central_client) -> None: client, _store = central_client