From ab06a45412205ef9e81581f97b6b7ee2580a99bd Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 6 Oct 2026 19:15:20 -0400 Subject: [PATCH 1/4] docs: reconcile RBAC comment and SECURITY.md with the shipped role model The RBAC comment described the user role as "may edit only its own account and cannot create or delete accounts", omitting that the role may upload programs, start/stop the PLC, debug and read status/logs. docs/SECURITY.md still listed Role-based access control under Future Improvements, though the feature shipped. Both drifted from the published capability table and misled external reviewers into reading other endpoints as RBAC gaps. Rewrite the comment to describe both roles the way the docs site does and remove the stale Future Improvements bullet. No behaviour change. Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_011taWuGDLF7eEJdmYjTLgoy --- docs/SECURITY.md | 1 - webserver/restapi.py | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index ac1efd9f..7e02f3ee 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -423,7 +423,6 @@ Planned security enhancements: - Database encryption at rest - Certificate management UI - Two-factor authentication -- Role-based access control ## Compliance Considerations diff --git a/webserver/restapi.py b/webserver/restapi.py index 10e38d67..cfa23c76 100644 --- a/webserver/restapi.py +++ b/webserver/restapi.py @@ -135,10 +135,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) From 92bf096a11efee042e917919f9742ffa62aee1ae Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 6 Oct 2026 19:17:35 -0400 Subject: [PATCH 2/4] fix(users): serialize first-admin bootstrap to close a TOCTOU race Concurrent unauthenticated POST /api/create-user calls on an empty database all passed the User.query.first() check and all committed with role=admin, because the check and the commit were not atomic. The set_password step (PBKDF2 at 600,000 iterations) widens the window to ~120 ms on fast hardware and more on an SBC. Serialize the bootstrap branch of create_user with a threading.Lock and re-check inside the lock. Insert a UNIQUE sentinel row (BootstrapMarker) in the same transaction as the first User, and catch IntegrityError on commit as a losing racer. The lock is primary; the sentinel is defense in depth for the lock-bypass case (future multi-process workers). The sentinel table is created by db.create_all on an upgraded device before any new bootstrap call reaches the lock, so legacy devices with users already present stay on the authenticated branch and never touch BootstrapMarker. Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_011taWuGDLF7eEJdmYjTLgoy --- webserver/restapi.py | 76 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 12 deletions(-) diff --git a/webserver/restapi.py b/webserver/restapi.py index cfa23c76..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 @@ -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() From afcedee8ed88a637ee19cc8b138a97079dd172ea Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 6 Oct 2026 19:21:02 -0400 Subject: [PATCH 3/4] test(users): regression test for the first-admin bootstrap race Two tests under tests/pytest/restapi: - test_concurrent_bootstrap_creates_exactly_one_admin fires five concurrent create-user POSTs released by a Barrier on an empty database and asserts exactly one 201, four 401s, one row in the users table with role=admin and one row in bootstrap_marker. Deterministic across 20 consecutive runs. - test_bootstrap_sentinel_blocks_a_second_bootstrap_after_user_deletion exercises the belt-and-suspenders path: delete every user outside the API, confirm the sentinel row survives, and confirm a second unauthenticated create-user is refused with 401. The threading test disposes the engine pool on teardown so no pooled connection carries a stale schema-inspection cache into the next test. Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_011taWuGDLF7eEJdmYjTLgoy --- tests/pytest/restapi/test_create_user_race.py | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 tests/pytest/restapi/test_create_user_race.py 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 From 0db007f11087618bd6ef493ad5487b207b138253 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 7 Oct 2026 15:34:32 -0400 Subject: [PATCH 4/4] docs: mention bootstrap_marker and the full "delete restapi.db" recovery path The TOCTOU fix added a one-row sentinel in restapi.db so that clearing only the users table cannot reopen the first-user bootstrap window. Say so where an operator would look for recovery steps. Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_011taWuGDLF7eEJdmYjTLgoy --- docs/SECURITY.md | 4 ++++ docs/TROUBLESHOOTING.md | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 7e02f3ee..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 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