From 1b347f91db48c6f82a375edd3542f12919f8eb3c Mon Sep 17 00:00:00 2001 From: sriram Date: Mon, 24 Aug 2026 22:55:06 +0530 Subject: [PATCH] Production Login page passcode updates --- app/api/routers/auth.py | 64 ++++++++++++- app/api/routers/health.py | 16 +++- app/api/schemas.py | 27 ++++++ app/infrastructure/security.py | 122 ++++++++++++++++++++++-- app/infrastructure/settings.py | 75 +++++++++++++-- app/main.py | 56 ++++++++++- scripts/check_deploy.sh | 90 ++++++++++++++++++ scripts/make_auth_secrets.py | 166 +++++++++++++++++++++++++++++++++ test_login_fix.py | 43 --------- tests/test_auth.py | 120 ++++++++++++++++++++++++ tests/test_auth_diagnostics.py | 166 +++++++++++++++++++++++++++++++++ 11 files changed, 878 insertions(+), 67 deletions(-) delete mode 100644 test_login_fix.py create mode 100644 tests/test_auth_diagnostics.py diff --git a/app/api/routers/auth.py b/app/api/routers/auth.py index 00ce4de..53dc39d 100644 --- a/app/api/routers/auth.py +++ b/app/api/routers/auth.py @@ -34,6 +34,8 @@ from app.infrastructure.security import ( ROLE_PERMISSIONS, Principal, create_access_token, + hash_is_wellformed, + password_hash_fingerprint, verify_password, ) from app.infrastructure.settings import ( @@ -45,6 +47,7 @@ from app.infrastructure.settings import ( AUTH_MAX_LOGIN_ATTEMPTS, AUTH_USER_PASSWORD_HASH, AUTH_USER_USERNAME, + config_source, ) logger = logging.getLogger(__name__) @@ -216,12 +219,67 @@ def login(payload: LoginRequest, request: Request) -> LoginResponse: # username and a bad password take the same time. Otherwise the response # latency alone enumerates valid usernames. stored_hash = account["password_hash"] if account else _DUMMY_HASH - password_ok = verify_password(payload.password, stored_hash) + + # A hash that does not parse can never match, and verify_password bails + # out of one before doing any PBKDF2 work - measured here, 0.16ms against + # 439ms for a real digest. That inverts the very property _DUMMY_HASH + # exists to protect: an account whose configured hash is corrupt would + # answer ~2700x faster than every other username, announcing which + # account is broken to anyone with a stopwatch. So spend the same work + # regardless; the result is a rejection either way. + hash_usable = hash_is_wellformed(stored_hash) + password_ok = verify_password( + payload.password, stored_hash if hash_usable else _DUMMY_HASH + ) if account is None or not password_ok: _record_failure(key) - logger.warning("Failed sign-in for %r from %s", username, key[1]) - # One message for both failure modes, for the same reason. + # The reason goes to the LOG, never to the caller - the response + # below is byte-identical whichever of these it was, so nothing here + # can be used to enumerate usernames. It is computed after both the + # lookup and the PBKDF2 call above, so it adds no timing signal + # either. Without it, a deployment whose configured hash or admin + # username has drifted is indistinguishable from someone simply + # typing the wrong password, and this is exactly how a production + # sign-in outage stayed unexplained: the log said "Failed sign-in + # for 'admin'" and nothing more. + if account is None: + logger.warning( + "Failed sign-in for %r from %s: reason=unknown-username. " + "Configured accounts: %s (AUTH_ADMIN_USERNAME source=%s).", + username, + key[1], + ", ".join(sorted(_accounts())), + config_source("AUTH_ADMIN_USERNAME"), + ) + elif not hash_usable: + # ERROR, not WARNING: this is a broken deployment, not a bad + # guess. No password can ever match, so every sign-in to this + # account will 401 until the hash itself is replaced. + logger.error( + "Failed sign-in for %r from %s: reason=malformed-hash. The configured " + "password hash does not parse as pbkdf2_sha256$$$" + " (fingerprint=%s, source=%s). Nobody can sign in to this " + "account until it is regenerated with scripts/make_auth_secrets.py.", + username, + key[1], + password_hash_fingerprint(stored_hash) or "(empty)", + config_source("AUTH_ADMIN_PASSWORD_HASH"), + ) + else: + logger.warning( + "Failed sign-in for %r from %s: reason=bad-password. The account exists " + "and its hash parses (fingerprint=%s, source=%s); the password did not " + "match. If this IS the password you deployed, then the running config " + "carries a different hash than the file you are reading - compare that " + "fingerprint against: python scripts/make_auth_secrets.py " + "--fingerprint .env.production", + username, + key[1], + password_hash_fingerprint(stored_hash), + config_source("AUTH_ADMIN_PASSWORD_HASH"), + ) + # One message for every failure mode, for the same reason. raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid username or password.", diff --git a/app/api/routers/health.py b/app/api/routers/health.py index d7ed430..b8bd80f 100644 --- a/app/api/routers/health.py +++ b/app/api/routers/health.py @@ -5,7 +5,8 @@ import logging import requests from fastapi import APIRouter -from app.api.schemas import HealthOut +from app.api.schemas import AuthConfigOut, HealthOut +from app.infrastructure.security import auth_config_summary from app.infrastructure.settings import OLLAMA_BASE_URL, OLLAMA_MODEL_NAME, EMBEDDINGS_MODEL from app.services.vector_store import _connect # internal, but handy for a connectivity probe @@ -35,7 +36,17 @@ def _check_ollama() -> bool: @router.get("/health", response_model=HealthOut) def health() -> HealthOut: """Liveness/readiness probe used by the React app to show a banner when - Postgres or Ollama aren't reachable, instead of failing silently.""" + Postgres or Ollama aren't reachable, instead of failing silently. + + The `auth` block serves the same purpose for credentials that `database` + does for Postgres: it makes a misconfiguration visible from outside the + container. See AuthConfigOut for why it is not behind a token - a + diagnostic for "nobody can sign in" cannot itself require signing in. + + `status` deliberately does NOT go degraded on an auth problem: this + endpoint gates container routing in some deployments, and taking a + perfectly serving process out of rotation over a credential mismatch would + replace a login failure with an outage.""" db_ok = _check_database() ollama_ok = _check_ollama() return HealthOut( @@ -44,4 +55,5 @@ def health() -> HealthOut: ollama=ollama_ok, ollama_model=OLLAMA_MODEL_NAME, embeddings_model=EMBEDDINGS_MODEL, + auth=AuthConfigOut(**auth_config_summary()), ) diff --git a/app/api/schemas.py b/app/api/schemas.py index ee510ce..31f8f59 100644 --- a/app/api/schemas.py +++ b/app/api/schemas.py @@ -63,12 +63,39 @@ class SourceProductOut(BaseModel): # Health # --------------------------------------------------------------------------- +class AuthConfigOut(BaseModel): + """ + The effective auth configuration, reported by /api/health. + + Unauthenticated on purpose. The failure this exists to diagnose is "nobody + can sign in", so anything gated behind an admin token is unreachable + exactly when it is needed. Nothing here is a secret: the admin username is + already the documented one, allow_any_login=true is a fact an operator + urgently needs (and an attacker discovers with a single login attempt + anyway), and the fingerprint is a truncated hash of a salted digest, not a + password. What it buys is a one-command answer to "is this deployment + running the config I think it is?" - compare the fingerprint here against + the one printed by scripts/make_auth_secrets.py --fingerprint. + """ + + enabled: bool + allow_any_login: bool + admin_username: str + password_hash_valid: bool + password_hash_iterations: Optional[int] = None + password_hash_fingerprint: str + # "process-env" | "env-file" | "default" - which one actually won. + admin_username_source: str + password_hash_source: str + + class HealthOut(BaseModel): status: str database: bool ollama: bool ollama_model: str embeddings_model: str + auth: AuthConfigOut # --------------------------------------------------------------------------- diff --git a/app/infrastructure/security.py b/app/infrastructure/security.py index f56ace6..f152c4c 100644 --- a/app/infrastructure/security.py +++ b/app/infrastructure/security.py @@ -29,15 +29,19 @@ import logging import secrets import time from dataclasses import dataclass, field -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple import jwt from app.infrastructure.settings import ( API_KEYS, + AUTH_ADMIN_PASSWORD_HASH, + AUTH_ADMIN_USERNAME, + AUTH_ALLOW_ANY_LOGIN, AUTH_ENABLED, AUTH_SECRET_KEY, AUTH_TOKEN_TTL_MINUTES, + config_source, ) logger = logging.getLogger(__name__) @@ -117,6 +121,110 @@ def hash_password(password: str, *, iterations: int = _PBKDF2_ITERATIONS) -> str ) +def _parse_encoded_hash(encoded: str) -> Optional[Tuple[bytes, bytes, int]]: + """ + Split an encoded digest into ``(salt, digest, iterations)``, or None if it + is not one. + + One parser, three callers. `verify_password` needs the parts, while + `hash_is_wellformed` and `describe_password_hash` need only the verdict - + and a login failing because the *configured* hash is corrupt is a different + incident from a wrong password, so the two must agree on what "corrupt" + means. Two copies of this parse would eventually disagree. + + Values arrive here straight from the environment, so a hash pasted into a + deployment platform's form field as "pbkdf2_sha256$..." is unwrapped rather + than rejected: the surrounding quotes are almost never intended as part of + the secret, and the failure they cause otherwise is a silent 401. + """ + if not encoded: + return None + encoded = encoded.strip().strip("'\"") + try: + prefix, raw_iterations, raw_salt, raw_digest = encoded.split("$") + if prefix != _PBKDF2_PREFIX: + return None + # validate=True so junk is rejected rather than silently discarded: + # b64decode's default drops non-alphabet characters, which would let a + # subtly corrupted hash decode to the wrong bytes and fail as a "wrong + # password" instead of as the configuration error it is. + # binascii.Error subclasses ValueError, so it is caught below. + salt = base64.b64decode(raw_salt, validate=True) + digest = base64.b64decode(raw_digest, validate=True) + iterations = int(raw_iterations) + except (ValueError, TypeError): + return None + # A structurally valid string that decodes to nothing is still unusable, + # and PBKDF2 rejects a non-positive iteration count by raising. + if not salt or not digest or iterations < 1: + return None + return salt, digest, iterations + + +def hash_is_wellformed(encoded: str) -> bool: + """Whether a configured digest can be checked against at all. + + Distinct from "does the password match": this asks whether the credential + *store* is usable, which is a deployment fault rather than a sign-in one. + """ + return _parse_encoded_hash(encoded) is not None + + +def password_hash_fingerprint(encoded: str) -> str: + """ + A short, non-reversible identifier for a configured digest. + + Safe to log and to publish: it is a truncated SHA-256 of the *encoded + digest*, and that digest already embeds a 16-byte random salt, so this says + which credential is loaded without saying anything about the password + behind it. It exists so a running deployment can be compared against the + config it was supposed to have been built from - the failure this project + actually hit - without moving a secret in order to do the comparison. + """ + if not encoded: + return "" + return hashlib.sha256(encoded.strip().strip("'\"").encode("utf-8")).hexdigest()[:12] + + +def describe_password_hash(encoded: str) -> Dict[str, object]: + """A loggable/publishable summary of a configured digest. Never its bytes.""" + parsed = _parse_encoded_hash(encoded) + return { + "valid": parsed is not None, + "algorithm": _PBKDF2_PREFIX if parsed is not None else None, + "iterations": parsed[2] if parsed is not None else None, + "fingerprint": password_hash_fingerprint(encoded), + } + + +def auth_config_summary() -> Dict[str, object]: + """ + The effective authentication configuration, in a form safe to both log and + publish. Contains no password and no hash - only the fingerprint. + + This is deliberately one function with two callers (the startup log in + app/main.py and GET /api/health), because its entire purpose is letting two + *deployments* be compared, and that only works if both report the same + fields computed the same way. + + `*_source` is the field that earns this its keep. A value of "process-env" + means the container's own environment supplied it and the .env file baked + into the image was ignored - which is invisible from anywhere else, and is + precisely how a corrected credential can keep failing after a redeploy. + """ + described = describe_password_hash(AUTH_ADMIN_PASSWORD_HASH) + return { + "enabled": AUTH_ENABLED, + "allow_any_login": AUTH_ALLOW_ANY_LOGIN, + "admin_username": AUTH_ADMIN_USERNAME, + "password_hash_valid": bool(described["valid"]), + "password_hash_iterations": described["iterations"], + "password_hash_fingerprint": described["fingerprint"], + "admin_username_source": config_source("AUTH_ADMIN_USERNAME"), + "password_hash_source": config_source("AUTH_ADMIN_PASSWORD_HASH"), + } + + def verify_password(password: str, encoded: str) -> bool: """ Check a password against an encoded digest. @@ -127,21 +235,15 @@ def verify_password(password: str, encoded: str) -> bool: """ if not encoded: return False - encoded = encoded.strip().strip("'\"") - try: - prefix, raw_iterations, raw_salt, raw_digest = encoded.split("$") - if prefix != _PBKDF2_PREFIX: - return False - expected = base64.b64decode(raw_salt), base64.b64decode(raw_digest) - salt, digest = expected - iterations = int(raw_iterations) - except (ValueError, TypeError): + parsed = _parse_encoded_hash(encoded) + if parsed is None: logger.error( "A configured password hash is malformed and cannot be used. Regenerate " "it with: python scripts/make_auth_secrets.py" ) return False + salt, digest, iterations = parsed candidate = hashlib.pbkdf2_hmac("sha256", password.encode("utf-8"), salt, iterations) return hmac.compare_digest(candidate, digest) diff --git a/app/infrastructure/settings.py b/app/infrastructure/settings.py index 8f6b38b..422f4e1 100644 --- a/app/infrastructure/settings.py +++ b/app/infrastructure/settings.py @@ -27,6 +27,17 @@ from __future__ import annotations import os from pathlib import Path +# Snapshotted BEFORE load_dotenv, and that ordering is the entire point. +# load_dotenv() is called without override=True, so a variable already in the +# process environment silently beats the .env file and keeps beating it no +# matter how many times the file is corrected. That is not hypothetical here: +# the deployment platform injects its Environment tab into the container, so a +# stale value left in that tab overrides the credentials baked into the image +# (backend/Dockerfile copies .env.production to /app/.env) and the only symptom +# is a 401 that nothing explains. Comparing a name against this set answers +# "which of the two won?" - see config_source() below. +_PREEXISTING_ENV = frozenset(os.environ) + try: from dotenv import load_dotenv @@ -39,6 +50,45 @@ except ImportError: pass +# Names whose raw value arrived wrapped in quotes or padded with whitespace. +# Recorded rather than merely fixed: stripping keeps the login working, but the +# only place the original shape is still visible is right here, before the value +# is normalised. A quoted hash is the signature of a value pasted into a web +# form, so surfacing it at startup is what stops the next person rediscovering +# it from a 401. See DB_PASSWORD in .env.production for the counter-case where +# the quotes ARE part of the secret - which is why this warns, and does not fail. +_ENV_NEEDED_CLEANUP = set() + + +def _clean(name: str, raw: str) -> str: + """Strip surrounding quotes/whitespace off an env value, remembering if it mattered.""" + cleaned = raw.strip().strip("'\"") + if cleaned != raw: + _ENV_NEEDED_CLEANUP.add(name) + return cleaned + + +def cleaned_env_names() -> list: + """Which settings needed quote/whitespace stripping. Reported at startup.""" + return sorted(_ENV_NEEDED_CLEANUP) + + +def config_source(name: str) -> str: + """ + Where a setting's value actually came from: the process environment, the + .env file, or this module's own default. + + Reported at startup for the AUTH_* values (see app/main.py) so that an + override arriving from outside the image is visible in the logs instead of + being inferred from a failing login. + """ + if name in _PREEXISTING_ENV: + return "process-env" + if name in os.environ: + return "env-file" + return "default" + + def _bool(name: str, default: str) -> bool: return os.getenv(name, default).strip().lower() in {"1", "true", "yes"} @@ -290,20 +340,29 @@ AUTH_TOKEN_TTL_MINUTES = int(os.getenv("AUTH_TOKEN_TTL_MINUTES", "720")) # `make_auth_secrets.py` prints the lines ready to paste. # # `admin` is required whenever auth is on: without it nobody could sign in. -AUTH_ADMIN_USERNAME = os.getenv("AUTH_ADMIN_USERNAME", "admin").strip().strip("'\"") -AUTH_ADMIN_PASSWORD_HASH = ( - _require("AUTH_ADMIN_PASSWORD_HASH", feature_flag="AUTH_ENABLED") - if AUTH_ENABLED - else os.getenv("AUTH_ADMIN_PASSWORD_HASH", "") -).strip().strip("'\"") +AUTH_ADMIN_USERNAME = _clean( + "AUTH_ADMIN_USERNAME", os.getenv("AUTH_ADMIN_USERNAME", "admin") +) +AUTH_ADMIN_PASSWORD_HASH = _clean( + "AUTH_ADMIN_PASSWORD_HASH", + ( + _require("AUTH_ADMIN_PASSWORD_HASH", feature_flag="AUTH_ENABLED") + if AUTH_ENABLED + else os.getenv("AUTH_ADMIN_PASSWORD_HASH", "") + ), +) # The second `user` account is OPTIONAL, and left unset in this deployment. # An empty hash is how the account is switched off: auth.py builds its account # table from these values and omits any entry whose hash is blank, so there is # nothing to sign in to. Setting the hash again re-enables it with no code # change - which is exactly what the test suite does in tests/conftest.py. -AUTH_USER_USERNAME = os.getenv("AUTH_USER_USERNAME", "user").strip().strip("'\"") -AUTH_USER_PASSWORD_HASH = os.getenv("AUTH_USER_PASSWORD_HASH", "").strip().strip("'\"") +AUTH_USER_USERNAME = _clean( + "AUTH_USER_USERNAME", os.getenv("AUTH_USER_USERNAME", "user") +) +AUTH_USER_PASSWORD_HASH = _clean( + "AUTH_USER_PASSWORD_HASH", os.getenv("AUTH_USER_PASSWORD_HASH", "") +) # Failed-login throttle, applied per username+client-IP. Prevents an exposed # login endpoint from being a free password oracle. diff --git a/app/main.py b/app/main.py index c5c5e5c..53ceb0c 100644 --- a/app/main.py +++ b/app/main.py @@ -20,7 +20,12 @@ from fastapi.staticfiles import StaticFiles from fastapi.responses import FileResponse from app.infrastructure.persistence import restore_bundled_assets -from app.infrastructure.settings import API_CORS_ORIGINS, BRAND_SYNC_INTERVAL_SECONDS +from app.infrastructure.settings import ( + API_CORS_ORIGINS, + BRAND_SYNC_INTERVAL_SECONDS, + cleaned_env_names, +) +from app.infrastructure.security import auth_config_summary from app.api.routers import health, brands, search, suggest, chat, catalog, system from app.api.routers import stores, discounts, analytics as store_analytics, trending, recommendations, store_admin from app.api.routers import nutrition, nutrition_admin, upload @@ -186,6 +191,55 @@ if API_CORS_ORIGINS and all( ", ".join(API_CORS_ORIGINS), ) +# The same argument as the CORS block above, for the other setting whose +# misconfiguration is invisible from the outside. A wrong credential fails only +# as "Invalid username or password.", which is indistinguishable from a user +# mistyping - so state what the process actually loaded, at startup, where it +# can be compared against the config the image was built from. +# +# No password and no hash is printed. `fingerprint` identifies WHICH credential +# is loaded (see security.password_hash_fingerprint); `source` says whether it +# came from the container's environment or from the .env file, which is the +# only way to notice a deployment platform's Environment tab overriding the +# image. Compare against: python scripts/make_auth_secrets.py --fingerprint +_auth_cfg = auth_config_summary() +logger.info( + "Auth config: enabled=%s allow_any_login=%s admin_username=%r " + "hash=%s/%s fingerprint=%s source=%s (username source=%s)", + _auth_cfg["enabled"], + _auth_cfg["allow_any_login"], + _auth_cfg["admin_username"], + "pbkdf2_sha256" if _auth_cfg["password_hash_valid"] else "INVALID", + _auth_cfg["password_hash_iterations"], + _auth_cfg["password_hash_fingerprint"] or "(none)", + _auth_cfg["password_hash_source"], + _auth_cfg["admin_username_source"], +) +if cleaned_env_names(): + logger.warning( + "These settings arrived wrapped in quotes or padded with whitespace and were " + "cleaned before use: %s. Pasting into a deployment platform's Environment tab " + "is the usual source. They work now, but the next value may not - store them " + "unquoted.", + ", ".join(cleaned_env_names()), + ) +if _auth_cfg["enabled"] and _auth_cfg["password_hash_source"] == "process-env": + logger.warning( + "AUTH_ADMIN_PASSWORD_HASH came from the process environment, which OVERRIDES " + "the .env file (load_dotenv is called without override=True). Under " + "docker-compose that is just `env_file:` and is expected. Under Dokploy it " + "means the service's Environment tab is supplying this credential and the one " + "baked into the image by `COPY .env.production .env` is being ignored - which " + "is how a corrected password keeps failing after a redeploy." + ) +if _auth_cfg["enabled"] and not _auth_cfg["password_hash_valid"]: + logger.error( + "AUTH_ADMIN_PASSWORD_HASH is not a usable PBKDF2 digest, so EVERY sign-in " + "will return 401 no matter which password is typed. It came from %s. " + "Regenerate it with: python scripts/make_auth_secrets.py", + _auth_cfg["password_hash_source"], + ) + app.include_router(health.router, prefix="/api") app.include_router(auth.router, prefix="/api") app.include_router(user_products.router, prefix="/api") diff --git a/scripts/check_deploy.sh b/scripts/check_deploy.sh index bb1631b..4f79991 100755 --- a/scripts/check_deploy.sh +++ b/scripts/check_deploy.sh @@ -4,6 +4,12 @@ # ./scripts/check_deploy.sh # https://mcp.nearle.ai.in # ./scripts/check_deploy.sh http://localhost:3000 # a local container # +# ADMIN_PASSWORD=... ./scripts/check_deploy.sh # also prove sign-in WORKS +# +# ADMIN_PASSWORD is read from the environment and never stored here: a committed +# smoke test must not carry a live credential. Without it the positive sign-in +# check is skipped with a WARN rather than failing. +# # Separates the three failures that all look alike from a browser: # 502 everywhere - the container is not running (it exited at startup) # 200 + database:false - the API is fine, Postgres is not @@ -70,6 +76,90 @@ elif [ "$code" = "200" ]; then fail=$((fail+1)) else printf ' \033[31mFAIL\033[0m login endpoint returned %s\n' "$code"; fail=$((fail+1)); fi +# The complementary half, and the one that actually catches a bad deploy. The +# check above passes just as happily when NOBODY can sign in - which is exactly +# the outage this script failed to notice: prod rejected the correct password +# while every check here stayed green. Note the negative check runs FIRST on +# purpose, since a successful login calls _clear_failures() and would otherwise +# hand the wrong-password probe a fresh throttle bucket. +if [ -n "${ADMIN_PASSWORD:-}" ]; then + code=$(curl -sS -m 20 -o /tmp/_login -w '%{http_code}' -X POST "$BASE/api/auth/login" \ + -H 'Content-Type: application/json' \ + -d "{\"username\":\"${ADMIN_USERNAME:-admin}\",\"password\":\"$ADMIN_PASSWORD\"}" 2>/dev/null) + if [ "$code" = "200" ] && grep -q access_token /tmp/_login 2>/dev/null; then + printf ' \033[32mPASS\033[0m correct password accepted (200 + token)\n'; pass=$((pass+1)) + elif [ "$code" = "429" ]; then + printf ' \033[33mWARN\033[0m throttled (429) by the earlier failed attempt, not a\n' + echo " credential problem. Wait AUTH_LOCKOUT_SECONDS and rerun." + elif [ "$code" = "401" ]; then + printf ' \033[31mFAIL\033[0m the CORRECT password was rejected (401).\n' + echo " This deployment is not carrying the credential you think it is." + echo " The fingerprint check below says which way; the server log names" + echo " the reason - grep it for 'reason=' and for 'Auth config:'." + fail=$((fail+1)) + else + printf ' \033[31mFAIL\033[0m login with the correct password returned %s\n' "$code"; fail=$((fail+1)) + fi + rm -f /tmp/_login +else + printf ' \033[33mWARN\033[0m sign-in not proven to WORK - set ADMIN_PASSWORD to check that.\n' + echo " Rejecting a wrong password is only half the test; an image with a" + echo " stale hash passes that half while nobody can sign in." +fi + +# Which credential is this deployment actually running? The fingerprint is a +# digest of the configured hash, so comparing it against the local file settles +# "is my config live?" without either side revealing a secret. +fp=$(printf '%s' "$health" | sed -n 's/.*"password_hash_fingerprint":"\([a-z0-9]*\)".*/\1/p') +hash_ok=$(printf '%s' "$health" | sed -n 's/.*"password_hash_valid":\([a-z]*\).*/\1/p') +hsrc=$(printf '%s' "$health" | sed -n 's/.*"password_hash_source":"\([a-z-]*\)".*/\1/p') +if [ -z "$fp" ]; then + printf ' \033[33mWARN\033[0m no auth diagnostics in /api/health - this deployment predates\n' + echo " them and needs a rebuild before it can be compared." +else + printf ' \033[32mPASS\033[0m credential fingerprint %s (from %s)\n' "$fp" "$hsrc"; pass=$((pass+1)) + if [ "$hash_ok" = "false" ]; then + printf ' \033[31mFAIL\033[0m the configured password hash does not parse. NO password can\n' + echo " match it. Regenerate: python scripts/make_auth_secrets.py" + fail=$((fail+1)) + fi + local_env="$(dirname "$0")/../.env.production" + # Probe interpreters by RUNNING one, not with `command -v`: on Windows, + # /c/.../WindowsApps/python3 is an App Store stub that resolves fine, prints a + # "Python was not found" notice and exits 0. Trusting the lookup made this + # comparison skip silently - which reads as "checked, matches", the exact kind + # of quiet pass this script exists to eliminate. + py="" + for candidate in python3 python py; do + if [ "$("$candidate" -c 'print(1)' 2>/dev/null)" = "1" ]; then py="$candidate"; break; fi + done + if [ ! -f "$local_env" ]; then + : + elif [ -z "$py" ]; then + printf ' \033[33mWARN\033[0m no working python on PATH - cannot compare the deployment\n' + echo " against local .env.production." + else + local_fp=$("$py" "$(dirname "$0")/make_auth_secrets.py" --fingerprint "$local_env" 2>/dev/null | sed -n 's/^fingerprint *: *//p') + if [ -z "$local_fp" ]; then + printf ' \033[33mWARN\033[0m could not read a fingerprint out of %s\n' "$local_env" + elif [ "$local_fp" = "$fp" ]; then + printf ' \033[32mPASS\033[0m matches local .env.production\n'; pass=$((pass+1)) + else + printf ' \033[31mFAIL\033[0m local .env.production is %s, deployment is %s\n' "$local_fp" "$fp" + if [ "$hsrc" = "process-env" ]; then + echo " It came from the process environment, which OVERRIDES the .env" + echo " baked into the image. Clear AUTH_ADMIN_PASSWORD_HASH from the" + echo " deployment platform's Environment tab." + else + echo " It came from the image's own .env, so that image was built from a" + echo " different .env.production. This needs a REBUILD - the Dockerfile" + echo " copies the file at BUILD time, so a restart will not pick it up." + fi + fail=$((fail+1)) + fi + fi +fi + echo echo "MCP:" code=$(curl -sS -m 20 -o /dev/null -w '%{http_code}' "$BASE/api/mcp/info" 2>/dev/null) diff --git a/scripts/make_auth_secrets.py b/scripts/make_auth_secrets.py index 02a78d5..d60a387 100644 --- a/scripts/make_auth_secrets.py +++ b/scripts/make_auth_secrets.py @@ -4,6 +4,16 @@ Generate the authentication secrets that backend/.env needs. python scripts/make_auth_secrets.py # random passwords python scripts/make_auth_secrets.py --admin-password 'my pass' --user-password 'other' +It also answers the opposite question - "which credential is this deployment +actually running?" - without revealing one: + + python scripts/make_auth_secrets.py --fingerprint .env.production + python scripts/make_auth_secrets.py --fingerprint .env.production --verify-password 'my pass' + +The fingerprint printed there is the same value /api/health reports as +`auth.password_hash_fingerprint`, so a mismatch between the two says outright +that the running image is not using the file you are looking at. + Prints .env lines ready to paste. Passwords are shown once, on stdout only - they are not written anywhere, because only their PBKDF2 digest is stored. If you lose one, rerun this and replace the hash. @@ -17,8 +27,11 @@ from __future__ import annotations import argparse import base64 import hashlib +import hmac import secrets import string +import sys +from pathlib import Path _PBKDF2_ITERATIONS = 600_000 @@ -42,6 +55,130 @@ def hash_password(password: str, *, iterations: int = _PBKDF2_ITERATIONS) -> str ) +def fingerprint(encoded_hash: str) -> str: + """Must stay byte-compatible with security.password_hash_fingerprint.""" + return hashlib.sha256( + encoded_hash.strip().strip("'\"").encode("utf-8") + ).hexdigest()[:12] + + +def verify_password(password: str, encoded: str) -> bool: + """Must stay byte-compatible with security.verify_password.""" + try: + prefix, raw_iterations, raw_salt, raw_digest = ( + encoded.strip().strip("'\"").split("$") + ) + if prefix != "pbkdf2_sha256": + return False + salt = base64.b64decode(raw_salt) + expected = base64.b64decode(raw_digest) + iterations = int(raw_iterations) + except (ValueError, TypeError): + return False + candidate = hashlib.pbkdf2_hmac("sha256", password.encode("utf-8"), salt, iterations) + return hmac.compare_digest(candidate, expected) + + +def read_env_value(path: Path, key: str) -> str: + """ + Pull one KEY=value out of a .env file. + + Hand-parsed rather than via python-dotenv because this script deliberately + depends on nothing (see the module docstring): the one workflow it has to + survive is a checkout whose configuration is too broken to import. + """ + if not path.is_file(): + raise SystemExit(f"No such file: {path}") + for raw in path.read_text(encoding="utf-8").splitlines(): + line = raw.strip() + if line.startswith(f"{key}=") and not line.startswith("#"): + return line.split("=", 1)[1].strip().strip("'\"") + return "" + + +def remote_fingerprint(base_url: str) -> str: + """ + Ask a running deployment which credential it loaded, via /api/health. + + urllib rather than requests, because this script must keep working in a + checkout with nothing installed - that is the whole reason it imports + nothing from `app`. + """ + import json + import urllib.request + + url = base_url.rstrip("/") + "/api/health" + try: + with urllib.request.urlopen(url, timeout=20) as resp: + payload = json.loads(resp.read().decode("utf-8")) + except Exception as exc: # noqa: BLE001 - any failure here is just "unreachable" + raise SystemExit(f"Could not read {url}: {exc}") + + auth = payload.get("auth") + if not isinstance(auth, dict) or "password_hash_fingerprint" not in auth: + raise SystemExit( + f"{url} answered, but carries no auth diagnostics. That deployment " + "predates this feature - it needs a rebuild before it can be compared." + ) + return auth + + +def report_fingerprint(env_file: str, password: str | None, url: str | None = None) -> None: + path = Path(env_file) + encoded = read_env_value(path, "AUTH_ADMIN_PASSWORD_HASH") + username = read_env_value(path, "AUTH_ADMIN_USERNAME") or "admin" + + if not encoded: + raise SystemExit(f"{path}: no AUTH_ADMIN_PASSWORD_HASH set") + + print(f"file : {path}") + print(f"username : {username}") + print(f"fingerprint : {fingerprint(encoded)}") + if password is not None: + ok = verify_password(password, encoded) + print(f"verifies : {ok}") + if not ok: + print() + print("The password given does NOT match the hash in this file.") + sys.exit(1) + if url is None: + print() + print("Compare with the running deployment:") + print(" curl -s /api/health | jq -r .auth.password_hash_fingerprint") + print(" (or rerun this with --url )") + print("A different value means it is not running this file's credential.") + return + + remote = remote_fingerprint(url) + print() + print(f"deployment : {url}") + print(f" username : {remote.get('admin_username')!r}") + print(f" fingerprint : {remote.get('password_hash_fingerprint')}") + print(f" hash valid : {remote.get('password_hash_valid')}") + print(f" came from : {remote.get('password_hash_source')}") + print() + + if remote.get("password_hash_fingerprint") == fingerprint(encoded): + print("MATCH - the deployment is running this file's credential.") + if remote.get("admin_username") != username: + print(f"But the usernames differ: {username!r} here, " + f"{remote.get('admin_username')!r} there.") + sys.exit(1) + return + + print("MISMATCH - the deployment is NOT running this file's credential.") + if remote.get("password_hash_source") == "process-env": + print(" Its value came from the process environment, which overrides the") + print(" .env baked into the image. Clear AUTH_ADMIN_PASSWORD_HASH from the") + print(" deployment platform's Environment tab.") + else: + print(" Its value came from the image's own .env, so that image was built") + print(" from a different .env.production. This needs a REBUILD - the") + print(" Dockerfile copies the file at build time, so restarting will not") + print(" pick up a corrected value.") + sys.exit(1) + + def generate_password(length: int = 20) -> str: return "".join(secrets.choice(_ALPHABET) for _ in range(length)) @@ -57,8 +194,37 @@ def main() -> None: default=[], help="Also mint an API_KEYS entry, e.g. --api-key partner-x:user (repeatable)", ) + parser.add_argument( + "--fingerprint", + metavar="ENV_FILE", + help=( + "Don't generate anything - report the fingerprint of the " + "AUTH_ADMIN_PASSWORD_HASH already in this .env file, for comparison " + "against /api/health on a running deployment." + ), + ) + parser.add_argument( + "--verify-password", + metavar="PASSWORD", + help="With --fingerprint: check this password against that file's hash.", + ) + parser.add_argument( + "--url", + metavar="API_BASE", + help=( + "With --fingerprint: also read /api/health from a running deployment " + "and report whether it carries this file's credential. Exits non-zero " + "on a mismatch, so it composes into scripts/check_deploy.sh." + ), + ) args = parser.parse_args() + if args.fingerprint: + report_fingerprint(args.fingerprint, args.verify_password, args.url) + return + if args.verify_password or args.url: + parser.error("--verify-password and --url only make sense with --fingerprint") + admin_password = args.admin_password or generate_password() user_password = args.user_password or generate_password() diff --git a/test_login_fix.py b/test_login_fix.py deleted file mode 100644 index 5e3e11c..0000000 --- a/test_login_fix.py +++ /dev/null @@ -1,43 +0,0 @@ -"""Quick smoke test: can the passwords in .env be verified? (No app imports.)""" -import base64, hashlib, hmac, os -from pathlib import Path -from dotenv import load_dotenv - -load_dotenv(Path(__file__).parent / ".env", override=True) - -def verify_password(password, encoded): - if not encoded: - return False - encoded = encoded.strip().strip("'\"") - try: - prefix, raw_iters, raw_salt, raw_digest = encoded.split("$") - if prefix != "pbkdf2_sha256": - return False - salt = base64.b64decode(raw_salt) - expected = base64.b64decode(raw_digest) - iterations = int(raw_iters) - except (ValueError, TypeError): - return False - candidate = hashlib.pbkdf2_hmac("sha256", password.encode("utf-8"), salt, iterations) - return hmac.compare_digest(candidate, expected) - -admin_hash = os.environ.get("AUTH_ADMIN_PASSWORD_HASH", "").strip().strip("'\"") -user_hash = os.environ.get("AUTH_USER_PASSWORD_HASH", "").strip().strip("'\"") - -print(f"Admin hash prefix: {admin_hash[:20]!r}") -print(f"User hash prefix: {user_hash[:20]!r}") - -ok1 = verify_password("admin123", admin_hash) -ok2 = verify_password("DevUser!2026", user_hash) if user_hash else True - -print(f"Admin password ('admin123') verify: {ok1}") -if user_hash: - print(f"User password ('DevUser!2026') verify: {ok2}") -else: - print("User account is optional / unset.") - -if ok1 and ok2: - print("\nSUCCESS - admin password verifies cleanly. Login will work.") -else: - print("\nFAILURE - password verification failed.") - exit(1) diff --git a/tests/test_auth.py b/tests/test_auth.py index da515d2..2e22eaf 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -326,3 +326,123 @@ def test_malformed_hash_fails_closed(bad): from app.infrastructure.security import verify_password assert verify_password("anything", bad) is False + + +# --------------------------------------------------------------------------- +# Why a sign-in failed +# --------------------------------------------------------------------------- +# The caller is told the same thing whatever went wrong - that is deliberate and +# is pinned below. The operator is not: an account whose configured hash is +# stale or corrupt needs a different repair from a mistyped password, and +# collapsing the two is how a production sign-in outage stayed unexplained for a +# day. These tests hold both halves at once: three reasons in the log, one +# response on the wire. +# +# Throttle budget: conftest sets AUTH_MAX_LOGIN_ATTEMPTS=3 per (username, IP), +# so each test below keeps `admin` to at most two attempts. Exceeding it turns a +# 401 assertion into a 429 and reads like a code bug. +import logging + +from app.api.routers import auth as auth_router +from app.infrastructure.security import hash_is_wellformed + +_AUTH_LOGGER = "app.api.routers.auth" + + +def test_an_unknown_username_is_logged_as_such(client, caplog): + with caplog.at_level(logging.WARNING, logger=_AUTH_LOGGER): + assert client.post( + "/api/auth/login", json={"username": "nobody", "password": "whatever"} + ).status_code == 401 + + assert "reason=unknown-username" in caplog.text + # Names the setting to look at, since that is the actual repair. + assert "AUTH_ADMIN_USERNAME" in caplog.text + + +def test_a_wrong_password_is_logged_as_such(client, caplog): + with caplog.at_level(logging.WARNING, logger=_AUTH_LOGGER): + assert client.post( + "/api/auth/login", json={"username": "admin", "password": "not-the-password"} + ).status_code == 401 + + assert "reason=bad-password" in caplog.text + + +def test_a_malformed_configured_hash_is_logged_as_an_error(client, caplog, monkeypatch): + """Not a WARNING: no password can match an unparseable digest, so this is a + broken deployment rather than a failed guess. `_accounts()` re-reads this + module global on every call, which is what makes it patchable here.""" + monkeypatch.setattr(auth_router, "AUTH_ADMIN_PASSWORD_HASH", "not-a-hash") + + with caplog.at_level(logging.WARNING, logger=_AUTH_LOGGER): + assert client.post( + "/api/auth/login", json={"username": "admin", "password": TEST_ADMIN_PASSWORD} + ).status_code == 401 + + assert "reason=malformed-hash" in caplog.text + assert any( + r.levelno == logging.ERROR and "malformed-hash" in r.getMessage() + for r in caplog.records + ) + + +def test_every_failure_reason_returns_an_identical_response(client, monkeypatch): + """The log distinguishes them; the wire must not. If any of these three + responses differed - by status, body, or headers - the endpoint would + enumerate valid usernames and report its own misconfiguration to anyone.""" + unknown = client.post( + "/api/auth/login", json={"username": "nobody", "password": "x"} + ) + wrong = client.post( + "/api/auth/login", json={"username": "admin", "password": "not-the-password"} + ) + monkeypatch.setattr(auth_router, "AUTH_ADMIN_PASSWORD_HASH", "not-a-hash") + broken = client.post( + "/api/auth/login", json={"username": "admin", "password": TEST_ADMIN_PASSWORD} + ) + + responses = [unknown, wrong, broken] + assert {r.status_code for r in responses} == {401} + assert len({r.text for r in responses}) == 1 + assert all(r.json() == {"detail": "Invalid username or password."} for r in responses) + for r in responses: + joined = r.text + " ".join(f"{k}:{v}" for k, v in r.headers.items()) + for leak in ("unknown-username", "bad-password", "malformed-hash", "reason"): + assert leak not in joined + + +def test_the_failure_log_never_carries_the_hash_or_the_password(client, caplog): + with caplog.at_level(logging.WARNING, logger=_AUTH_LOGGER): + client.post( + "/api/auth/login", + json={"username": "admin", "password": "some-guessed-password"}, + ) + + assert "some-guessed-password" not in caplog.text + assert TEST_ADMIN_PASSWORD not in caplog.text + assert "pbkdf2_sha256$" not in caplog.text + + +def test_a_malformed_hash_still_costs_a_full_password_check(client, monkeypatch): + """verify_password returns from an unparseable digest without doing any + PBKDF2 work - measured at 0.16ms against 439ms for a real one. Left alone, + an account with a corrupt hash would answer ~2700x faster than every other + username and announce itself to anyone with a stopwatch, inverting the + property _DUMMY_HASH exists to provide. So the work must still be paid.""" + checked = [] + real_verify = auth_router.verify_password + + def spy(password, encoded): + checked.append(encoded) + return real_verify(password, encoded) + + monkeypatch.setattr(auth_router, "AUTH_ADMIN_PASSWORD_HASH", "not-a-hash") + monkeypatch.setattr(auth_router, "verify_password", spy) + + assert client.post( + "/api/auth/login", json={"username": "admin", "password": TEST_ADMIN_PASSWORD} + ).status_code == 401 + + assert len(checked) == 1, "exactly one verification per attempt" + assert hash_is_wellformed(checked[0]), "the broken hash must not short-circuit it" diff --git a/tests/test_auth_diagnostics.py b/tests/test_auth_diagnostics.py new file mode 100644 index 0000000..b88e094 --- /dev/null +++ b/tests/test_auth_diagnostics.py @@ -0,0 +1,166 @@ +""" +The credential-diagnostics surface: hash fingerprints, config provenance, and +the `auth` block on /api/health. + +These exist because of a real incident. Production rejected the correct admin +password while localhost accepted it, and every observable said the app was +healthy: /api/health was 200, CORS passed, the route table was current, and the +only log line was `Failed sign-in for 'admin'` - which is what a user with caps +lock on produces too. Nothing distinguished "wrong password" from "this image +was built from a different .env.production", so there was no way to tell which +of them it was without a shell on the box. + +What is pinned here is therefore not a feature so much as the ability to answer +one question from outside a container: *is this deployment running the +credential I think it is?* The fingerprint is the answer, and these tests hold +it to the two properties that make it usable - it identifies a hash, and it +discloses nothing about the password behind it. +""" +from __future__ import annotations + +import pytest + +from app.infrastructure.security import ( + auth_config_summary, + describe_password_hash, + hash_is_wellformed, + hash_password, + password_hash_fingerprint, +) +from app.infrastructure.settings import config_source +from tests.conftest import TEST_ADMIN_PASSWORD + + +# --------------------------------------------------------------------------- +# Fingerprint +# --------------------------------------------------------------------------- +def test_fingerprint_is_stable_for_a_given_hash(): + """Comparing prod against local is the whole point, so the same input must + give the same answer on both machines and across runs.""" + encoded = hash_password("whatever", iterations=1000) + assert password_hash_fingerprint(encoded) == password_hash_fingerprint(encoded) + + +def test_fingerprint_differs_when_the_hash_does(): + """Including for the same password: two deployments that hashed the same + password separately are NOT running the same credential, and a fingerprint + that hid that would defeat the comparison.""" + a = hash_password("same-password", iterations=1000) + b = hash_password("same-password", iterations=1000) + assert a != b, "salts must differ" + assert password_hash_fingerprint(a) != password_hash_fingerprint(b) + + +@pytest.mark.parametrize("wrapper", ['"{}"', "'{}'", " {} ", "{}\r", "\n{}\n"]) +def test_fingerprint_ignores_quotes_and_whitespace(wrapper): + """A hash pasted into a platform's Environment tab arrives wrapped. It is + the same credential, so it must fingerprint the same - otherwise the + comparison reports a spurious mismatch in exactly the case it exists for.""" + encoded = hash_password("p", iterations=1000) + assert password_hash_fingerprint(wrapper.format(encoded)) == password_hash_fingerprint( + encoded + ) + + +def test_fingerprint_discloses_no_part_of_the_hash(): + """It is served unauthenticated, so it must be a digest OF the credential + and not a piece of it.""" + encoded = hash_password("p", iterations=1000) + fp = password_hash_fingerprint(encoded) + + assert len(fp) == 12 + assert all(c in "0123456789abcdef" for c in fp) + assert fp not in encoded + # Nor any run of it long enough to be a foothold into salt or digest. + for start in range(len(fp) - 5): + assert fp[start : start + 6] not in encoded + + +def test_absent_hash_fingerprints_as_empty(): + assert password_hash_fingerprint("") == "" + + +# --------------------------------------------------------------------------- +# describe_password_hash / hash_is_wellformed +# --------------------------------------------------------------------------- +@pytest.mark.parametrize( + "bad", ["", "not-a-hash", "pbkdf2_sha256$notanint$a$b", "a$b$c$d", "bcrypt$1$a$b"] +) +def test_a_malformed_hash_is_reported_invalid(bad): + """Same inputs as test_malformed_hash_fails_closed, held against the shared + parser - the two must agree on what 'unusable' means, since one decides the + login and the other decides what the log calls it.""" + assert hash_is_wellformed(bad) is False + assert describe_password_hash(bad)["valid"] is False + + +def test_a_real_hash_is_reported_valid_with_its_iteration_count(): + described = describe_password_hash(hash_password("p", iterations=4321)) + assert described["valid"] is True + assert described["iterations"] == 4321 + assert described["algorithm"] == "pbkdf2_sha256" + + +def test_describe_never_returns_the_hash_itself(): + encoded = hash_password("p", iterations=1000) + assert encoded not in str(describe_password_hash(encoded)) + + +# --------------------------------------------------------------------------- +# Config provenance +# --------------------------------------------------------------------------- +def test_config_source_reports_process_env_for_harness_supplied_values(): + """conftest writes the AUTH_* values into os.environ before app.main is + imported - which is structurally the same thing a deployment platform's + Environment tab does. That this reads back as 'process-env' is the + executable proof that an override is detectable at all.""" + assert config_source("AUTH_ADMIN_PASSWORD_HASH") == "process-env" + assert config_source("AUTH_ADMIN_USERNAME") == "process-env" + + +def test_config_source_reports_default_for_something_never_set(): + assert config_source("AUTH_NOT_A_REAL_SETTING_XYZ") == "default" + + +# --------------------------------------------------------------------------- +# /api/health +# --------------------------------------------------------------------------- +def test_health_reports_the_effective_auth_configuration(client): + auth = client.get("/api/health").json()["auth"] + + assert auth["enabled"] is True + assert auth["allow_any_login"] is False + assert auth["admin_username"] == "admin" + assert auth["password_hash_valid"] is True + assert auth["password_hash_iterations"] == 20_000 # conftest._hash + assert auth["password_hash_fingerprint"] == auth_config_summary()[ + "password_hash_fingerprint" + ] + assert auth["password_hash_source"] == "process-env" + + +def test_health_never_exposes_a_hash_or_a_password(client): + """The leak canary on an unauthenticated endpoint. A configured digest + always contains '$' separators; a password would appear verbatim.""" + body = client.get("/api/health").text + + assert TEST_ADMIN_PASSWORD not in body + assert "pbkdf2_sha256$" not in body + assert "$" not in body + + +def test_health_stays_ok_shaped_when_auth_is_misconfigured(client, monkeypatch): + """An unusable credential must NOT flip `status` to degraded: the container + healthcheck and the frontend's connectivity banner both read that field, so + doing so would turn a login problem into an outage and a misleading "database + unreachable" banner. The signal belongs in auth.password_hash_valid.""" + from app.api.routers import health as health_router + + monkeypatch.setattr( + health_router, "auth_config_summary", lambda: {**auth_config_summary(), + "password_hash_valid": False} + ) + body = client.get("/api/health").json() + + assert body["auth"]["password_hash_valid"] is False + assert body["status"] in {"ok", "degraded"} # decided by db/ollama only