diff --git a/RUNBOOK.md b/RUNBOOK.md index d556037..668b8de 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -24,7 +24,7 @@ The pods are pinned to `archlinux-k3s-3` (`.161`) via `nodeSelector`. | `MONGO_HOST` | `192.168.178.229` | Deployment manifest | | `MONGO_USER` | `root` | Deployment manifest | | `MONGO_PASS` | *(see below)* | Deployment manifest | -| `SECRET_KEY` | *(see below)* | Deployment manifest — Flask session signing key | +| `SECRET_KEY` | *(see below)* | Deployment manifest — Flask session signing key. **Required** | | `FLASK_ENV` | `production` | Dockerfile | | `FLASK_DEBUG` | `0` | Dockerfile | @@ -35,9 +35,37 @@ The pods are pinned to `archlinux-k3s-3` (`.161`) via `nodeSelector`. kubectl -n beer get deploy beer-deployment -o jsonpath='{.spec.template.spec.containers[0].env}' ``` -> The app refuses to start a *usable* session without a `SECRET_KEY`; the -> Dockerfile ships a placeholder so the container still boots if the variable -> is missing. Treat the placeholder as a misconfiguration. +> **The app refuses to start without a real `SECRET_KEY`.** There is no default +> and no placeholder: a missing or placeholder value raises +> `ImproperlyConfigured` and the container exits. A known session-signing key +> would let anyone forge a login cookie, so a misconfigured deployment must +> fail loudly rather than run insecurely. +> +> Generate one with: +> ```bash +> python -c 'import secrets; print(secrets.token_hex(32))' +> ``` + +### Moving the credentials into a Kubernetes Secret + +The live Deployment currently carries `MONGO_PASS` as a plain environment +value. Moving it to a Secret is the better shape and is **not** done here +because it would mean replacing the live hand-provisioned values — a change +that needs the operator's sign-off. When it is done: + +```yaml +env: + - name: MONGO_PASS + valueFrom: + secretKeyRef: + name: beer-db + key: mongo_pass + - name: SECRET_KEY + valueFrom: + secretKeyRef: + name: beer-app + key: secret_key +``` ## Deploying @@ -76,10 +104,31 @@ re-applying the manifest. kubectl -n beer get pods kubectl -n beer logs deploy/beer-deployment --tail=50 curl -s http://:30015/healthz # -> {"status":"ok"} +curl -s http://:30015/readyz # -> {"status":"ready"} ``` -`/healthz` never touches Mongo, so it proves the process is up but not that the -database is reachable. +`/healthz` is the **liveness** probe and never touches Mongo — it answers "is +the process up". A database outage must not make the kubelet restart a healthy +process. + +`/readyz` is the **readiness** probe and pings Mongo, returning 503 when the +database is unreachable. Wire it to the Service so a pod with no database is +pulled out of the endpoints rather than serving errors: + +```yaml +readinessProbe: + httpGet: + path: /readyz + port: 5000 + initialDelaySeconds: 5 + periodSeconds: 10 +livenessProbe: + httpGet: + path: /healthz + port: 5000 + initialDelaySeconds: 10 + periodSeconds: 20 +``` ## Data safety @@ -112,6 +161,16 @@ current gravity is above the assumed original gravity of 1012. Fixed in the current version: the standard `(OG - FG) × 131.25` formula is used, and a run with no known OG shows a dash instead of a figure. +### The pod will not start after a deploy + +The most likely cause is a missing `SECRET_KEY` — the app exits rather than +run with a default. Check the logs: + +```bash +kubectl -n beer logs deploy/beer-deployment --tail=20 +# ImproperlyConfigured: SECRET_KEY is not set. ... +``` + ### The chart on the run page is blank The chart library is served from `/static/js/chart.umd.js` — it is vendored in diff --git a/flask.Dockerfile b/flask.Dockerfile index e6cfa00..2cbd69c 100644 --- a/flask.Dockerfile +++ b/flask.Dockerfile @@ -15,9 +15,11 @@ ENV FLASK_ENV=production ENV FLASK_DEBUG=0 ENV PORT=5000 -# SECRET_KEY is supplied by the deployment (see RUNBOOK.md). The default here -# is only a placeholder so the container starts if it is missing. -ENV SECRET_KEY=change-me-in-the-deployment +# SECRET_KEY MUST be supplied by the deployment (see RUNBOOK.md). There is +# deliberately no default: the app refuses to start without a real one, so a +# misconfigured deployment fails loudly instead of running with a known +# session-signing key. +# ENV SECRET_KEY=... EXPOSE 5000 diff --git a/requirements-dev.txt b/requirements-dev.txt index 0d4ba38..777f65a 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,3 @@ -r requirements.txt -pytest -mongomock +pytest==9.2.0 +mongomock==4.3.0 diff --git a/requirements.txt b/requirements.txt index a4f6bce..ba57dab 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,8 @@ -flask -Flask-WTF -waitress -pymongo -requests +# Pinned to the versions the test suite runs against. The image is rebuilt +# on every merge, so an unpinned dependency can change behaviour underneath a +# deploy with no commit to show for it. +Flask==3.1.3 +Flask-WTF==1.3.0 +waitress==3.0.2 +pymongo==4.19.0 +requests==2.32.5 diff --git a/src/flask/app_factory.py b/src/flask/app_factory.py index ff54e9a..8cbfda3 100644 --- a/src/flask/app_factory.py +++ b/src/flask/app_factory.py @@ -10,12 +10,42 @@ from __future__ import annotations import os from pathlib import Path -from flask import Flask, g, redirect, request, session, url_for +from flask import (Flask, g, redirect, request, session, url_for) from flask_wtf import CSRFProtect __all__ = ["create_app"] +class ImproperlyConfigured(RuntimeError): + """Raised when the app is missing configuration it cannot safely default. + + Werkzeug 3 dropped its own ``ImproperlyConfigured``, and Flask has never + exported one, so the app defines its own. It is a ``RuntimeError`` so it + is not mistaken for an HTTP error the app should try to render a page for. + """ + + +def _require_secret_key() -> str: + """Return the Flask secret key, refusing to start without a real one. + + A known default secret key means anyone can forge a session cookie and + log in as the operator, so a missing or placeholder value must stop the + app rather than silently fall back. The one exception is the test + suite, which supplies its own key through the config. + """ + secret = os.getenv("SECRET_KEY", "").strip() + if not secret: + raise ImproperlyConfigured( + "SECRET_KEY is not set. Export a random value, e.g. " + "`export SECRET_KEY=$(python -c 'import secrets; print(secrets.token_hex(32))')`." + ) + if secret.lower() in {"change-me-in-the-deployment", "dev-secret-change-me"}: + raise ImproperlyConfigured( + "SECRET_KEY is still the placeholder value. Set a real secret key." + ) + return secret + + def create_app(config: dict | None = None) -> Flask: """Build and configure the Flask application.""" app = Flask( @@ -25,7 +55,7 @@ def create_app(config: dict | None = None) -> Flask: ) app.config.update( - SECRET_KEY=os.getenv("SECRET_KEY", "dev-secret-change-me"), + SECRET_KEY=_require_secret_key(), MONGO_HOST=os.getenv("MONGO_HOST", "127.0.0.1"), MONGO_USER=os.getenv("MONGO_USER", "root"), MONGO_PASS=os.getenv("MONGO_PASS", ""), @@ -101,9 +131,23 @@ def create_app(config: dict | None = None) -> Flask: @app.route("/healthz") def healthz(): - """Liveness probe used by Kubernetes. Never touches Mongo.""" + """Liveness probe. Never touches Mongo — it answers "is the process + up", not "is the app working". A database outage must not cause the + kubelet to restart a perfectly healthy process.""" return {"status": "ok"}, 200 + @app.route("/readyz") + def readyz(): + """Readiness probe. Checks that Mongo is actually reachable, so a + pod with no database is taken out of the Service's endpoints instead + of serving errors.""" + try: + repo = get_repo() + repo.client.admin.command("ping") + except Exception as exc: # noqa: BLE001 - any failure means not ready + return {"status": "not ready", "error": str(exc)}, 503 + return {"status": "ready"}, 200 + @app.route("/") def index(): if logged_in(): diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 58e8ad5..e1a2459 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -390,6 +390,13 @@ def days_between(start: date, end: date) -> int: return (end - start).days +def _fmt_decimal(points: int | None) -> str: + """Format gravity points as a decimal reading, e.g. ``1039`` -> ``1.039``.""" + if points is None: + return "" + return f"{points / 1000:.3f}" + + def run_summary(run: Run, today: date | None = None, og_override: int | None = None) -> dict: """A flat dict describing a run, ready for a template or a JSON response.""" @@ -405,8 +412,12 @@ def run_summary(run: Run, today: date | None = None, "fg_points": fg, "og_decimal": sg_points_to_decimal(og), "fg_decimal": sg_points_to_decimal(fg), + # Pre-formatted for the templates so no arithmetic happens in Jinja. + "og_decimal_str": _fmt_decimal(og), + "fg_decimal_str": _fmt_decimal(fg), "max_sg": run.max_sg(), "min_sg": run.min_sg(), + "min_sg_decimal": _fmt_decimal(run.min_sg()), "drop_points": run.gravity_drop_points(), "abv": run.abv_percent(), "projected_abv": run.projected_abv_percent(og_override=og_override), diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 165d55e..baa7bbd 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -22,6 +22,7 @@ from typing import Any from pymongo import MongoClient from pymongo.errors import PyMongoError +from werkzeug.security import check_password_hash, generate_password_hash from brewing import Run @@ -46,19 +47,50 @@ class BeerRepository: def user_check(self, username: str, password: str) -> bool: """Check the login credentials. - Historic rows store the password in plain text; new rows created by - :meth:`set_user_password` store a bcrypt-free salted SHA-256 digest. - Both are checked so existing accounts keep working. + Passwords are stored as a hash produced by + ``werkzeug.security.generate_password_hash`` — scrypt by default, so + the stored value carries its own salt and is deliberately slow to + compute. + + Legacy rows stored the password in plain text. Those are still + accepted, but a successful legacy login **migrates the row in place** + to a proper hash, so plaintext credentials disappear from the + database the first time each account is used. """ - doc = self.users.find_one({"username": username}) + if not username or not password: + return False + + doc = self.users.find_one({"username": str(username)}) if not doc: return False + stored = str(doc.get("password") or "") if not stored: return False + + if _is_hashed(stored): + return check_password_hash(stored, password) + + # Legacy plaintext password — accept it, then upgrade the row. if stored == password: + self._upgrade_password(doc, password) return True - return stored == _hash_password(password) + + return False + + def _upgrade_password(self, doc: dict, password: str) -> None: + """Replace a plaintext password with a proper hash.""" + try: + self.users.update_one( + {"_id": doc["_id"]}, + {"$set": { + "password": generate_password_hash(password), + "password_migrated_at": _now_iso(), + }}, + ) + except PyMongoError: + # A failed upgrade must not stop the login succeeding. + pass # -- readings ----------------------------------------------------------- # @@ -68,7 +100,7 @@ class BeerRepository: def reading_docs_for_run(self, beer_run_id: str) -> list[dict]: return list( - self.readings.find({"beer_run_id": str(beer_run_id)}).sort("date", 1) + self.readings.find({"beer_run_id": _as_id(beer_run_id)}).sort("date", 1) ) def runs(self) -> list[Run]: @@ -106,15 +138,15 @@ class BeerRepository: application used, so re-submitting the same day updates in place instead of creating a duplicate. Returns the ``_id`` of the document. """ - query = {"date": reading_date, "beer_run_id": str(beer_run_id)} + query = {"date": _as_date(reading_date), "beer_run_id": _as_id(beer_run_id)} update = { "$set": { "beer_run_id": str(beer_run_id), "beer_type": beer_type, "sg": str(sg), - "date": reading_date, + "date": _as_date(reading_date), "final_reading": "True" if final_reading else "False", - "comment": comment, + "comment": str(comment), } } result = self.readings.update_one(query, update, upsert=True) @@ -127,7 +159,7 @@ class BeerRepository: final: bool) -> int: """Flag (or unflag) a reading as the final reading of its run.""" result = self.readings.update_many( - {"date": reading_date, "beer_run_id": str(beer_run_id)}, + {"date": _as_date(reading_date), "beer_run_id": _as_id(beer_run_id)}, {"$set": {"final_reading": "True" if final else "False"}}, ) return result.modified_count @@ -136,7 +168,7 @@ class BeerRepository: def run_metadata(self, beer_run_id: str) -> dict: """The stored metadata for a run, or an empty dict.""" - doc = self.run_meta.find_one({"beer_run_id": str(beer_run_id)}) + doc = self.run_meta.find_one({"beer_run_id": _as_id(beer_run_id)}) if not doc: return {} doc.pop("_id", None) @@ -147,7 +179,7 @@ class BeerRepository: result: dict[str, dict] = {} for doc in self.run_meta.find(): doc.pop("_id", None) - result[str(doc.get("beer_run_id"))] = doc + result[_as_id(doc.get("beer_run_id"))] = doc return result def upsert_run_metadata(self, beer_run_id: str, **fields: Any) -> dict: @@ -159,7 +191,7 @@ class BeerRepository: payload = {k: v for k, v in fields.items() if v is not None} payload["updated_at"] = _now_iso() self.run_meta.update_one( - {"beer_run_id": str(beer_run_id)}, + {"beer_run_id": _as_id(beer_run_id)}, {"$set": payload, "$setOnInsert": {"created_at": _now_iso()}}, upsert=True, ) @@ -167,7 +199,7 @@ class BeerRepository: def delete_run_metadata(self, beer_run_id: str) -> bool: """Remove the metadata document for a run (readings are untouched).""" - return self.run_meta.delete_one({"beer_run_id": str(beer_run_id)}).deleted_count > 0 + return self.run_meta.delete_one({"beer_run_id": _as_id(beer_run_id)}).deleted_count > 0 # -- helpers ------------------------------------------------------------ # @@ -198,13 +230,41 @@ def _build_client() -> MongoClient: return MongoClient(uri, serverSelectionTimeoutMS=5000) -def _hash_password(password: str) -> str: - """Salted SHA-256 hex digest used for newly created users.""" - import hashlib - import secrets +def _as_id(value: object) -> str: + """Coerce a run id to a plain string. - salt = os.getenv("BEER_PASSWORD_SALT", "beer_data") - return hashlib.sha256(f"{salt}:{password}".encode()).hexdigest() + Every value that reaches a Mongo query is coerced first. Without this, a + caller passing a dict (e.g. ``{"$ne": ""}``) would be interpreted by + Mongo as a query operator rather than as a value — the classic NoSQL + injection route. + """ + if isinstance(value, str): + return value + if isinstance(value, (int, float)): + return str(value) + return "" + + +def _as_date(value: object) -> str: + """Coerce a reading date to a plain ``YYYY-MM-DD`` string.""" + if isinstance(value, str): + return value + return "" + + +def _is_hashed(stored: str) -> bool: + """True when *stored* looks like a werkzeug password hash. + + Werkzeug hashes are of the form ``method$salt$hash``, e.g. + ``scrypt:32768:8:1$abcdef...$0123...``. A legacy plaintext password + never contains a ``$`` in the right places, so the check is reliable. + """ + return "$" in stored and len(stored.split("$")) >= 3 + + +def hash_password(password: str) -> str: + """Hash a password for storage (scrypt via werkzeug).""" + return generate_password_hash(password) def _now_iso() -> str: diff --git a/src/flask/templates/brews.html b/src/flask/templates/brews.html index 57ad51d..9487482 100644 --- a/src/flask/templates/brews.html +++ b/src/flask/templates/brews.html @@ -94,7 +94,7 @@ {% if run.min_sg is not none %} {{ run.min_sg }} -
{{ '%.3f' % (run.min_sg / 1000) }}
+
{{ run.min_sg_decimal }}
{% else %}—{% endif %} diff --git a/src/flask/templates/run_detail.html b/src/flask/templates/run_detail.html index 7cd6f15..9d970b6 100644 --- a/src/flask/templates/run_detail.html +++ b/src/flask/templates/run_detail.html @@ -29,7 +29,7 @@ {% if summary.og_points is not none %}{{ summary.og_points }}{% else %}—{% endif %} {% if summary.og_decimal %} -
{{ '%.3f' % summary.og_decimal }}
+
{{ summary.og_decimal_str }}
{% endif %}
@@ -40,7 +40,7 @@ {% if summary.fg_points is not none %}{{ summary.fg_points }}{% else %}—{% endif %}
{% if summary.fg_decimal %} -
{{ '%.3f' % summary.fg_decimal }}
+
{{ summary.fg_decimal_str }}
{% endif %}
diff --git a/tests/conftest.py b/tests/conftest.py index d15fd27..f196fd5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,12 @@ if APP_DIR not in sys.path: sys.path.insert(0, APP_DIR) +@pytest.fixture(autouse=True) +def secret_key_env(monkeypatch): + """The app refuses to start without SECRET_KEY — give the tests one.""" + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + + @pytest.fixture() def mongo_client(): """An in-memory Mongo, shared by the repository the app builds.""" diff --git a/tests/test_app.py b/tests/test_app.py index 5ef5824..dd73020 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -26,6 +26,25 @@ def test_healthz_is_public(client): assert response.json == {"status": "ok"} +def test_readyz_checks_the_database(client): + response = client.get("/readyz") + assert response.status_code == 200 + assert response.json == {"status": "ready"} + + +def test_readyz_reports_not_ready_when_mongo_is_down(client, monkeypatch): + """A pod that cannot reach Mongo must not serve traffic.""" + from pymongo.errors import ServerSelectionTimeoutError + + def boom(*args, **kwargs): + raise ServerSelectionTimeoutError("no mongo") + + monkeypatch.setattr("mongo.repository.BeerRepository.__init__", + lambda self, **kw: (_ for _ in ()).throw(ServerSelectionTimeoutError("no mongo"))) + response = client.get("/readyz") + assert response.status_code == 503 + + def test_root_redirects_to_login(client): response = client.get("/") assert response.status_code == 302 @@ -179,3 +198,85 @@ def test_out_of_range_reading_is_saved_but_warned(logged_in, repo): assert b"outside the usual hydrometer range" in response.data # It is still stored — the warning is advisory, not a rejection. assert repo.readings.find_one({"beer_run_id": "12"}) is not None + + +# ------------------------------------------------------- security: passwords # + +def test_legacy_plaintext_password_is_accepted_and_migrated(repo): + """A legacy plaintext row still logs in, and is rewritten as a hash on + the way in so plaintext credentials disappear from the database.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check("beer", "beer_pass") is True + + doc = repo.users.find_one({"username": "beer"}) + assert doc["password"] != "beer_pass" + assert "$" in doc["password"] # a werkzeug hash + assert "password_migrated_at" in doc + + +def test_hashed_password_verifies(repo): + from mongo.repository import hash_password + + repo.users.insert_one({"username": "beer", "password": hash_password("beer_pass")}) + + assert repo.user_check("beer", "beer_pass") is True + assert repo.user_check("beer", "wrong") is False + + +def test_user_check_rejects_empty_credentials(repo): + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check("", "") is False + assert repo.user_check("beer", "") is False + assert repo.user_check("", "beer_pass") is False + + +def test_user_check_rejects_a_nosql_operator_payload(repo): + """A dict must never be able to act as a Mongo query operator.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check({"$ne": ""}, "anything") is False + assert repo.user_check({"$gt": ""}, {"$ne": ""}) is False + + +def test_run_id_cannot_be_used_as_an_operator(repo): + """A run id is coerced to a string before it reaches a query.""" + from mongo.repository import _as_id + + assert _as_id({"$ne": ""}) == "" + assert _as_id(20) == "20" + assert _as_id("20") == "20" + + +def test_the_app_refuses_to_start_without_a_secret_key(monkeypatch): + """A missing SECRET_KEY must stop the app, not fall back to a default.""" + import importlib + + import app_factory + + # Reload so the guard reads the environment as it is *now*. The exception + # class must be taken from the reloaded module — reloading rebinds it, so + # a class imported earlier would be a different object and pytest.raises + # would not match it. + monkeypatch.delenv("SECRET_KEY", raising=False) + importlib.reload(app_factory) + with pytest.raises(app_factory.ImproperlyConfigured): + app_factory.create_app() + + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + importlib.reload(app_factory) + + +def test_the_app_refuses_the_placeholder_secret_key(monkeypatch): + import importlib + + import app_factory + + monkeypatch.setenv("SECRET_KEY", "change-me-in-the-deployment") + importlib.reload(app_factory) + with pytest.raises(app_factory.ImproperlyConfigured): + app_factory.create_app() + + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + importlib.reload(app_factory)