diff --git a/docs/SECURITY.md b/docs/SECURITY.md index ac1efd9f..a63f4fc2 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -288,6 +288,10 @@ docker run -p 8443:8443 ... # All interfaces - User accounts - Hashed passwords - Session data +- `bootstrap_marker`: one-row sentinel inserted alongside the first admin + so a lost `users` table cannot reopen the first-user bootstrap window. + Full recovery of first-user bootstrap therefore requires removing the + entire `restapi.db` file, not clearing individual tables. **Protection:** - File system permissions @@ -423,7 +427,6 @@ Planned security enhancements: - Database encryption at rest - Certificate management UI - Two-factor authentication -- Role-based access control ## Compliance Considerations diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 2e84ef60..1c87aa6f 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -198,11 +198,16 @@ The OpenPLC Editor handles self-signed certificates automatically. If you're usi ```bash ls -la /var/run/runtime/restapi.db ``` -2. Reset database (WARNING: deletes all users): +2. Reset database (WARNING: deletes all users and reopens the first-user + bootstrap window): ```bash sudo rm /var/run/runtime/restapi.db sudo ./start_openplc.sh ``` + The runtime holds a `bootstrap_marker` row inside `restapi.db` to prevent + a second admin being created once the first exists. Clearing individual + tables will leave this sentinel in place and keep bootstrap closed; + full recovery requires removing the entire `restapi.db` file. 3. Check .env file exists: ```bash ls -la /var/run/runtime/.env diff --git a/tests/pytest/restapi/test_create_user_race.py b/tests/pytest/restapi/test_create_user_race.py new file mode 100644 index 00000000..28b12880 --- /dev/null +++ b/tests/pytest/restapi/test_create_user_race.py @@ -0,0 +1,103 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Autonomy® + +"""Regression test for the first-admin bootstrap TOCTOU race. + +Before the fix, N concurrent POST /api/create-user calls on an empty database +all passed the User.query.first() check and all committed with role=admin. +The fix serializes the bootstrap branch with a threading.Lock, re-checks +inside the lock, and writes a UNIQUE sentinel row in the same transaction +as the first user. This test fires N concurrent requests and asserts exactly +one admin lands. +""" + +import threading + +from webserver import restapi + + +def _race_create_user(app, n: int) -> list[tuple[str, int]]: + """Fire ``n`` concurrent create-user POSTs, released by a shared barrier. + + Each thread owns its own test client. The barrier lines every thread up + before the simultaneous release so the contention window is as tight as + the host can produce. + """ + results: list[tuple[str, int]] = [] + results_lock = threading.Lock() + release = threading.Barrier(n) + + def post(index: int) -> None: + username = f"racer{index}" + client = app.test_client() + release.wait(timeout=10) + resp = client.post( + "/api/create-user", + json={"username": username, "password": "race-test-pw-12"}, + ) + with results_lock: + results.append((username, resp.status_code)) + + threads = [threading.Thread(target=post, args=(i,)) for i in range(n)] + for t in threads: + t.start() + for t in threads: + t.join() + return results + + +def test_concurrent_bootstrap_creates_exactly_one_admin(app): + """Five concurrent POSTs on an empty DB produce one admin and four refusals.""" + try: + results = _race_create_user(app, n=5) + + successes = [r for r in results if r[1] == 201] + refusals = [r for r in results if r[1] != 201] + + assert len(successes) == 1, ( + f"expected exactly one 201, got {len(successes)} out of {len(results)}: {results}" + ) + # Losing racers see "a user exists" inside the lock and return 401. A + # lock bypass that reached the commit would land on IntegrityError + # and also 401. + for username, status in refusals: + assert status == 401, f"losing racer {username} got {status}, expected 401" + + with app.app_context(): + users = restapi.User.query.all() + assert len(users) == 1 + assert users[0].role == restapi.ADMIN_ROLE + markers = restapi.BootstrapMarker.query.all() + assert len(markers) == 1, "sentinel row must land in the same transaction" + finally: + # The worker threads each opened a pooled connection; dispose the pool + # so no stale schema-inspection cache survives into the next test. + with app.app_context(): + restapi.db.engine.dispose() + + +def test_bootstrap_sentinel_blocks_a_second_bootstrap_after_user_deletion(app): + """Even if the user table is wiped later, the sentinel keeps bootstrap closed. + + This exercises the belt-and-suspenders path: the lock's re-check reads BOTH + User and BootstrapMarker, so a device whose users were deleted outside the + API cannot be re-bootstrapped without authentication. + """ + client = app.test_client() + first = client.post( + "/api/create-user", + json={"username": "founder", "password": "pw-abcd-1234"}, + ) + assert first.status_code == 201, first.get_json() + + with app.app_context(): + restapi.User.query.delete() + restapi.db.session.commit() + assert restapi.User.query.count() == 0 + assert restapi.BootstrapMarker.query.count() == 1 + + second = client.post( + "/api/create-user", + json={"username": "pretender", "password": "pw-abcd-1234"}, + ) + assert second.status_code == 401 diff --git a/webserver/restapi.py b/webserver/restapi.py index 10e38d67..2d012121 100644 --- a/webserver/restapi.py +++ b/webserver/restapi.py @@ -4,6 +4,7 @@ import base64 import json import os +import threading from typing import Callable, Optional from flask import Blueprint, Flask, current_app, jsonify, request @@ -18,6 +19,7 @@ from flask_sqlalchemy import SQLAlchemy from sqlalchemy import inspect as sa_inspect from sqlalchemy import text as sa_text +from sqlalchemy.exc import IntegrityError from werkzeug.security import check_password_hash, generate_password_hash import webserver.config @@ -135,10 +137,10 @@ def restapi_capabilities(): jwt_blacklist = set() -# Role-based access control. For now there are exactly two roles: ``admin`` -# (may manage every account) and ``user`` (may edit only its own account and -# cannot create or delete accounts). Enforcement lives server-side in the -# endpoints below — the editor UI mirrors it but is never the boundary. +# RBAC: two roles. ``admin`` manages accounts and retrieves projects. +# ``user`` operates the PLC (upload, start/stop, debug, status/logs) and +# edits its own account. Enforcement is server-side; the editor UI mirrors +# it but is never the boundary. ADMIN_ROLE = "admin" USER_ROLE = "user" ROLES = (ADMIN_ROLE, USER_ROLE) @@ -186,6 +188,25 @@ def to_dict(self): return {"id": self.id, "username": self.username, "role": self.role} +class BootstrapMarker(db.Model): # type: ignore[name-defined] + """One-row sentinel inserted alongside the first admin. + + Primary key is implicitly ``UNIQUE``, so a second concurrent writer that + races past ``_bootstrap_lock`` collides on commit and the create-user + handler catches it as ``IntegrityError``. The row is written in the same + transaction as the first ``User``, so either both land or neither does. + """ + + __tablename__ = "bootstrap_marker" + id: int = db.Column(db.Integer, primary_key=True) + + +# Serializes the unauthenticated bootstrap branch of create-user. Only one +# thread runs the check / hash / commit sequence at a time; the authoritative +# re-check inside the lock closes the window the TOCTOU race opened. +_bootstrap_lock = threading.Lock() + + def admin_count() -> int: """Number of accounts holding the admin role (used by last-admin guards).""" return User.query.filter_by(role=ADMIN_ROLE).count() @@ -351,19 +372,19 @@ def create_user(): logger.error("Error checking for users: %s", e) return jsonify({"msg": f"User creation error: {e}"}), 401 - # Bootstrap: with no users yet, anyone may create the FIRST account and it - # is always an admin (someone has to be able to manage accounts). Once any - # user exists, only an authenticated admin may create further accounts. + # First account on an empty DB is created unauthenticated and is always + # admin. The bootstrap branch is serialized so two racing callers cannot + # both land as admin. See _bootstrap_first_admin. if not users_exist: - role = ADMIN_ROLE - else: - if verify_jwt_in_request(optional=True) is None: - return jsonify({"msg": "Authentication required"}), 401 - if not (current_user and current_user.is_admin()): - return jsonify({"msg": "Admin privileges required"}), 403 - role = (request.get_json() or {}).get("role", USER_ROLE) - if role not in ROLES: - return jsonify({"msg": f"Invalid role. Must be one of: {', '.join(ROLES)}"}), 400 + return _bootstrap_first_admin() + + if verify_jwt_in_request(optional=True) is None: + return jsonify({"msg": "Authentication required"}), 401 + if not (current_user and current_user.is_admin()): + return jsonify({"msg": "Admin privileges required"}), 403 + role = (request.get_json() or {}).get("role", USER_ROLE) + if role not in ROLES: + return jsonify({"msg": f"Invalid role. Must be one of: {', '.join(ROLES)}"}), 400 data = request.get_json() username = data.get("username") @@ -384,6 +405,37 @@ def create_user(): return jsonify({"msg": "User created", "id": user.id, "role": user.role}), 201 +def _bootstrap_first_admin(): + """Create the first admin on an empty database, serialized against races. + + Called from ``create_user`` only when the pre-lock check sees no users. + Inside the lock an authoritative re-check against ``User`` and + ``BootstrapMarker`` closes the TOCTOU window; the UNIQUE PK on + ``BootstrapMarker`` catches a lock bypass as ``IntegrityError``. + """ + with _bootstrap_lock: + if User.query.first() is not None or BootstrapMarker.query.first() is not None: + return jsonify({"msg": "Authentication required"}), 401 + + data = request.get_json() or {} + username = data.get("username") + password = data.get("password") + if not username or not password: + return jsonify({"msg": "Missing username or password"}), 400 + + user = User(username=username, role=ADMIN_ROLE) + user.set_password(password) + db.session.add(user) + db.session.add(BootstrapMarker(id=1)) + try: + db.session.commit() + except IntegrityError: + db.session.rollback() + return jsonify({"msg": "Authentication required"}), 401 + + return jsonify({"msg": "User created", "id": user.id, "role": user.role}), 201 + + # verify existing users individually @restapi_bp.route("/get-user-info/", methods=["GET"]) @jwt_required()