feat(ansible): runs stay inside their source; controller-side vars are admin-only (#5034)

#5034 "Playbook runs: path and inventory checks in trigger_run, connection
vars admin-only, confirm enforced server-side, web actions audited".
Ansible runs inside the Steward container, so an operator who could name
any playbook path, set ansible_connection=local or ProxyCommand ssh args,
or put {{ lookup('pipe', …) }} in a value could run commands on Steward.

- steward/ansible/guards.py: check_playbook (the source must list it),
  check_inventory_scope (steward:* or an inventory file the source has),
  check_vars_for_role (ansible_* beyond host/port/user/password/become,
  and any Jinja in a value, are admin-only), changed_vars (an edit is
  held to the role only for what it changes).
- trigger_run applies the path checks, so every run path — web, MCP,
  alerts, schedules, plugins — gets them.
- Variables are checked by role on the web run form, host-page runs,
  schedule create/update, inventory group/target create/update, and MCP
  run_playbook / create_schedule / update_schedule.
- A steward:confirm playbook needs the confirmation ticked on the server
  side for a real (non-check) web run, as MCP already required.
- Audited: runs, cancel, schedule create/update/enable/disable/delete/
  run-now, playbook create/update/delete, inventory group and target
  create/update/delete (variable names only, never values). One
  audit_request helper serves the web routes.
- The CSRF form guard and the route guard now each test that they fail
  on a synthetic broken input (rule: a guard must be able to fail).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

steward/ansible/guards.py

@@ -0,0 +1,86 @@
+"""What a playbook run may be pointed at, and which variables only an admin
+may set.
+
+Ansible runs inside the Steward container, so a run is code on the Steward
+server unless it is held to playbooks its source lists and to variables that
+only shape what happens on the target. An operator may run playbooks and set
+their variables. Three things would let one run commands on Steward itself,
+and are refused for anyone but an admin:
+
+- a playbook or inventory path outside what the source lists;
+- an `ansible_*` variable that changes how or where Ansible connects
+  (`ansible_connection: local`, `ansible_ssh_common_args` with a
+  ProxyCommand, an interpreter or executable path);
+- a Jinja expression in any value (`{{ lookup('pipe', …) }}` is evaluated
+  on the controller when the value is used).
+
+Playbook files themselves are admin-only to write (ansible/routes.py).
+"""
+from __future__ import annotations
+
+from steward.ansible import sources as src_module
+from steward.models.users import UserRole
+
+# The ansible_* variables an operator may set: which address, port and user
+# to reach a target as, and whether to become another user there.
+OPERATOR_ANSIBLE_VARS = frozenset({
+    "ansible_host", "ansible_port", "ansible_user", "ansible_password",
+    "ansible_become", "ansible_become_user", "ansible_become_password",
+})
+_JINJA = ("{{", "{%", "{#")
+
+
+def check_playbook(source: dict, playbook_path: str) -> str | None:
+    """None when the source lists this playbook, else why it is refused."""
+    if playbook_path not in src_module.discover_playbooks(source["path"]):
+        return f"Source {source['name']!r} has no playbook {playbook_path!r}"
+    return None
+
+
+def check_inventory_scope(source: dict, scope: str) -> str | None:
+    """None for a Steward scope or an inventory file the source has, else why not."""
+    if scope.startswith("steward:"):
+        return None
+    if scope.startswith("repo:"):
+        parts = scope.split(":", 2)
+        path = parts[2] if len(parts) == 3 else ""
+        if path in src_module.discover_inventories(source["path"]):
+            return None
+        return f"Source {source['name']!r} has no inventory file {path!r}"
+    return "Invalid inventory_scope"
+
+
+def _has_jinja(value) -> bool:
+    if isinstance(value, str):
+        return any(mark in value for mark in _JINJA)
+    if isinstance(value, dict):
+        return any(_has_jinja(k) or _has_jinja(v) for k, v in value.items())
+    if isinstance(value, (list, tuple)):
+        return any(_has_jinja(v) for v in value)
+    return False
+
+
+def check_vars_for_role(variables: dict | None, role: UserRole | None) -> str | None:
+    """None when `role` may set these variables, else a message naming what
+    only an admin may set. role=None is treated as the least privileged."""
+    if role == UserRole.admin or not variables:
+        return None
+    names = sorted(n for n in variables
+                   if str(n).startswith("ansible_") and n not in OPERATOR_ANSIBLE_VARS)
+    if names:
+        return (f"Only an admin can set {', '.join(names)}: these change how or where "
+                "Ansible connects, and can run commands on the Steward server itself.")
+    templated = sorted(str(k) for k, v in variables.items() if _has_jinja(k) or _has_jinja(v))
+    if templated:
+        return (f"Only an admin can set a value containing {{{{ }}}}, {{% %}} or {{# #}} "
+                f"({', '.join(templated)}): Ansible evaluates it on the Steward server.")
+    return None
+
+
+
+def changed_vars(new: dict | None, old: dict | None) -> dict:
+    """The entries of `new` that are added or differ from `old`. An edit is
+    held to the editor's role only for what it changes, so an operator can
+    still edit a group whose connection vars an admin set."""
+    old = old or {}
+    return {k: v for k, v in (new or {}).items() if k not in old or old[k] != v}

steward/ansible/inventory_routes.py

@@ -3,11 +3,13 @@ from __future__ import annotations
 import uuid
 import yaml
 
-from quart import Blueprint, current_app, redirect, render_template, request, url_for
+from quart import Blueprint, current_app, redirect, render_template, request, session, url_for
 from sqlalchemy import select
 from sqlalchemy.orm import selectinload
 
+from steward.ansible import guards
 from steward.auth.middleware import require_role
+from steward.core.audit import audit_request as _audit
 from steward.models.ansible_inventory import AnsibleGroup, AnsibleTarget
 from steward.models.users import UserRole
 
@@ -30,6 +32,12 @@ def _parse_ansible_vars(raw: str) -> dict:
     return parsed
 
 
+def _vars_refusal(new: dict, old: dict | None = None) -> str | None:
+    """Why this editor may not make this vars change, or None (ansible/guards.py)."""
+    return guards.check_vars_for_role(guards.changed_vars(new, old),
+                                      UserRole(session.get("user_role", "viewer")))
+
+
 # ---------------------------------------------------------------------------
 # Groups
 # ---------------------------------------------------------------------------
@@ -59,10 +67,16 @@ async def groups_create():
     except ValueError as exc:
         return str(exc), 400
 
+    err = _vars_refusal(ansible_vars)
+    if err:
+        return err, 400
+
     group = AnsibleGroup(id=str(uuid.uuid4()), name=name, ansible_vars=ansible_vars)
     async with current_app.db_sessionmaker() as db:
         async with db.begin():
             db.add(group)
+    await _audit("ansible.group.created", "ansible_group", group.id,
+                 {"name": name, "vars": sorted(ansible_vars)})
     return redirect(url_for("ansible_inventory.groups_list"))
 
 
@@ -111,6 +125,9 @@ async def group_update(group_id: str):
             group = result.scalar_one_or_none()
             if group is None:
                 return "Group not found", 404
+            err = _vars_refusal(ansible_vars, group.ansible_vars)
+            if err:
+                return err, 400
 
             name = form.get("name", group.name).strip()
             if name:
@@ -123,7 +140,10 @@ async def group_update(group_id: str):
             ).scalars().all()
             target_map = {t.id: t for t in all_targets}
             group.targets = [target_map[tid] for tid in new_ids if tid in target_map]
+            detail = {"name": group.name, "vars": sorted(ansible_vars),
+                      "targets": sorted(t.name for t in group.targets)}
 
+    await _audit("ansible.group.updated", "ansible_group", group_id, detail)
     return redirect(url_for("ansible_inventory.group_detail", group_id=group_id))
 
 
@@ -136,8 +156,11 @@ async def group_delete(group_id: str):
                 select(AnsibleGroup).where(AnsibleGroup.id == group_id)
             )
             group = result.scalar_one_or_none()
-            if group is not None:
-                await db.delete(group)
+            if group is None:
+                return redirect(url_for("ansible_inventory.groups_list"))
+            name = group.name
+            await db.delete(group)
+    await _audit("ansible.group.deleted", "ansible_group", group_id, {"name": name})
     return redirect(url_for("ansible_inventory.groups_list"))
 
 
@@ -171,12 +194,18 @@ async def targets_create():
     except ValueError as exc:
         return str(exc), 400
 
+    err = _vars_refusal(ansible_vars)
+    if err:
+        return err, 400
+
     target = AnsibleTarget(
         id=str(uuid.uuid4()), name=name, address=address, ansible_vars=ansible_vars
     )
     async with current_app.db_sessionmaker() as db:
         async with db.begin():
             db.add(target)
+    await _audit("ansible.target.created", "ansible_target", target.id,
+                 {"name": name, "address": address, "vars": sorted(ansible_vars)})
     return redirect(url_for("ansible_inventory.targets_list"))
 
 
@@ -225,6 +254,9 @@ async def target_update(target_id: str):
             target = result.scalar_one_or_none()
             if target is None:
                 return "Target not found", 404
+            err = _vars_refusal(ansible_vars, target.ansible_vars)
+            if err:
+                return err, 400
 
             name = form.get("name", target.name).strip()
             address = form.get("address", target.address).strip()
@@ -238,7 +270,10 @@ async def target_update(target_id: str):
             all_groups_result = await db.execute(select(AnsibleGroup))
             group_map = {g.id: g for g in all_groups_result.scalars().all()}
             target.groups = [group_map[gid] for gid in new_group_ids if gid in group_map]
+            detail = {"name": target.name, "address": target.address, "vars": sorted(ansible_vars),
+                      "groups": sorted(g.name for g in target.groups)}
 
+    await _audit("ansible.target.updated", "ansible_target", target_id, detail)
     return redirect(url_for("ansible_inventory.target_detail", target_id=target_id))
 
 
@@ -251,6 +286,9 @@ async def target_delete(target_id: str):
                 select(AnsibleTarget).where(AnsibleTarget.id == target_id)
             )
             target = result.scalar_one_or_none()
-            if target is not None:
-                await db.delete(target)
+            if target is None:
+                return redirect(url_for("ansible_inventory.targets_list"))
+            name = target.name
+            await db.delete(target)
+    await _audit("ansible.target.deleted", "ansible_target", target_id, {"name": name})
     return redirect(url_for("ansible_inventory.targets_list"))

steward/ansible/routes.py

@@ -8,9 +8,10 @@ from quart import (
 )
 from sqlalchemy import select
 
-from steward.ansible import executor, sources as src_module
+from steward.ansible import executor, guards, sources as src_module
 from steward.ansible.inventory_gen import scope_labels
 from steward.auth.middleware import require_role
+from steward.core.audit import audit_request as _audit
 from steward.models.ansible import AnsibleRun, AnsibleRunStatus
 from steward.models.ansible_schedule import AnsibleSchedule
 from steward.models.users import User, UserRole
@@ -105,6 +106,30 @@ async def view_playbook(source_name: str, playbook_path: str):
     )
 
 
+def _caller_role() -> UserRole:
+    return UserRole(session.get("user_role", "viewer"))
+
+
+def _run_vars(params: dict | None, secret_vars: dict | None = None) -> dict:
+    return {**((params or {}).get("extra_vars_map") or {}), **(secret_vars or {})}
+
+
+def check_run_form(form, source: dict | None, playbook_path: str,
+                   params: dict | None, secret_vars: dict | None) -> str | None:
+    """The server-side gates a web run must pass (the form's own checks are
+    only a convenience): who may set these variables, and the playbook's
+    steward:confirm flag, which a real (non-check) run must have ticked."""
+    err = guards.check_vars_for_role(_run_vars(params, secret_vars), _caller_role())
+    if err:
+        return err
+    content = src_module.read_playbook(source["path"], playbook_path) if source else None
+    meta = src_module.discover_playbook_meta(content or "")
+    if meta["confirm"] and not (params or {}).get("check") and "confirmed" not in form:
+        return (f"{playbook_path} is flagged destructive by its author (steward:confirm). "
+                "Tick the confirmation to run it, or run it in check mode.")
+    return None
+
+
 def _parse_run_params(form) -> tuple[dict | None, dict | None, str | None]:
     """Build executor params + the unpersisted secret-vars dict from form fields.
 
@@ -235,6 +260,10 @@ async def create_run():
         return "playbook_path is required", 400
 
     params_or_none, secret_vars, err = _parse_run_params(form)
+    if err:
+        return err, 400
+    source = next((s for s in _get_sources() if s["name"] == source_name), None)
+    err = check_run_form(form, source, playbook_path, params_or_none, secret_vars)
     if err:
         return err, 400
 
@@ -251,6 +280,10 @@ async def create_run():
         return err, 404
     if err:
         return err, 400
+    await _audit("ansible.run.started", "ansible_run", run.id, {
+        "source": source_name, "playbook": playbook_path, "scope": inventory_scope,
+        "check": bool((params_or_none or {}).get("check")),
+        "vars": sorted(_run_vars(params_or_none, secret_vars))})
 
     return await render_template(
         "ansible/run_started.html",
@@ -323,6 +356,8 @@ async def cancel_run(run_id: str):
         return "Not found", 404
     if run.status in (AnsibleRunStatus.running, AnsibleRunStatus.queued):
         executor.cancel_run(run_id)
+        await _audit("ansible.run.cancelled", "ansible_run", run_id,
+                     {"playbook": run.playbook_path})
     return redirect(url_for("ansible.run_detail", run_id=run_id))
 
 
@@ -441,6 +476,15 @@ def _schedule_form_fields(form) -> tuple[dict | None, str | None]:
     }, None
 
 
+def _schedule_detail(fields: dict) -> dict:
+    """What a schedule audit row records: what runs, where, how often — and
+    which variables, by name only."""
+    return {"name": fields["name"], "source": fields["source_name"],
+            "playbook": fields["playbook_path"], "scope": fields["inventory_scope"],
+            "interval_seconds": fields["interval_seconds"],
+            "vars": sorted(_run_vars(fields["params"]))}
+
+
 @ansible_bp.post("/schedules")
 @require_role(UserRole.operator)
 async def create_schedule():
@@ -448,9 +492,14 @@ async def create_schedule():
     fields, err = _schedule_form_fields(form)
     if err:
         return err, 400
+    err = guards.check_vars_for_role(_run_vars(fields["params"]), _caller_role())
+    if err:
+        return err, 400
+    sched = AnsibleSchedule(enabled=True, **fields)
     async with current_app.db_sessionmaker() as db:
         async with db.begin():
-            db.add(AnsibleSchedule(enabled=True, **fields))
+            db.add(sched)
+    await _audit("ansible.schedule.created", "ansible_schedule", sched.id, _schedule_detail(fields))
     return redirect(url_for("ansible.schedules"))
 
 
@@ -466,8 +515,14 @@ async def update_schedule(schedule_id: str):
             sched = await db.get(AnsibleSchedule, schedule_id)
             if sched is None:
                 return "Not found", 404
+            err = guards.check_vars_for_role(
+                guards.changed_vars(_run_vars(fields["params"]), _run_vars(sched.params)),
+                _caller_role())
+            if err:
+                return err, 400
             for k, v in fields.items():
                 setattr(sched, k, v)
+    await _audit("ansible.schedule.updated", "ansible_schedule", schedule_id, _schedule_detail(fields))
     return redirect(url_for("ansible.schedules"))
 
 
@@ -480,6 +535,9 @@ async def toggle_schedule(schedule_id: str):
             if sched is None:
                 return "Not found", 404
             sched.enabled = not sched.enabled
+            enabled, name = sched.enabled, sched.name
+    await _audit("ansible.schedule.enabled" if enabled else "ansible.schedule.disabled",
+                 "ansible_schedule", schedule_id, {"name": name})
     return redirect(url_for("ansible.schedules"))
 
 
@@ -489,8 +547,11 @@ async def delete_schedule(schedule_id: str):
     async with current_app.db_sessionmaker() as db:
         async with db.begin():
             sched = await db.get(AnsibleSchedule, schedule_id)
-            if sched is not None:
-                await db.delete(sched)
+            if sched is None:
+                return redirect(url_for("ansible.schedules"))
+            name = sched.name
+            await db.delete(sched)
+    await _audit("ansible.schedule.deleted", "ansible_schedule", schedule_id, {"name": name})
     return redirect(url_for("ansible.schedules"))
 
 
@@ -519,6 +580,8 @@ async def run_schedule_now(schedule_id: str):
                 fresh.last_run_at = datetime.now(timezone.utc)
                 fresh.last_run_id = run.id if run else None
                 fresh.last_error = err
+    await _audit("ansible.schedule.run_now", "ansible_schedule", schedule_id,
+                 {"playbook": playbook_path, "run_id": run.id if run else None, "error": err})
     if run:
         return redirect(url_for("ansible.run_detail", run_id=run.id))
     return redirect(url_for("ansible.schedules"))
@@ -595,6 +658,9 @@ async def save_playbook():
             content=content, error=err,
         ), 400
 
+    await _audit("ansible.playbook.updated" if editing else "ansible.playbook.created",
+                 "ansible_playbook", f"{source_name}:{playbook_path}",
+                 {"source": source_name, "playbook": playbook_path, "bytes": len(content)})
     return redirect(url_for(
         "ansible.view_playbook", source_name=source_name, playbook_path=playbook_path))
 
@@ -606,9 +672,12 @@ async def delete_playbook():
     source = _editable_source((form.get("source_name", "") or "").strip())
     if source is None:
         return "Source is not editable", 400
-    ok, err = src_module.delete_playbook(source["path"], (form.get("playbook_path", "") or "").strip())
+    playbook_path = (form.get("playbook_path", "") or "").strip()
+    ok, err = src_module.delete_playbook(source["path"], playbook_path)
     if not ok:
         return err, 400
+    await _audit("ansible.playbook.deleted", "ansible_playbook", f"{source['name']}:{playbook_path}",
+                 {"source": source["name"], "playbook": playbook_path})
     return redirect(url_for("ansible.browse"))
 
 

steward/ansible/runner.py

@@ -10,7 +10,7 @@ import asyncio
 import uuid
 from datetime import datetime, timezone
 
-from steward.ansible import executor, sources as src_module
+from steward.ansible import executor, guards, sources as src_module
 from steward.ansible.inventory_gen import (
     fetch_scope_targets, generate_inventory, inventory_to_yaml,
 )
@@ -47,6 +47,13 @@ async def trigger_run(
     source = next((s for s in sources if s["name"] == source_name), None)
     if source is None:
         return None, None, "Source not found"
+    # Every run path (web, MCP, alerts, schedules, plugins) arrives here, so
+    # this is where a run is held to what its source lists (ansible/guards.py).
+    err = guards.check_playbook(source, playbook_path)
+    if err is None and inventory_content is None:
+        err = guards.check_inventory_scope(source, inventory_scope)
+    if err:
+        return None, source, err
 
     inventory_path: str | None = None
     if inventory_content is not None:
@@ -55,11 +62,8 @@ async def trigger_run(
         async with app.db_sessionmaker() as db:
             targets = await fetch_scope_targets(db, inventory_scope)
         inventory_content = inventory_to_yaml(generate_inventory(targets))
-    elif inventory_scope.startswith("repo:"):
-        parts = inventory_scope.split(":", 2)
-        inventory_path = parts[2] if len(parts) == 3 else ""
     else:
-        return None, source, "Invalid inventory_scope"
+        inventory_path = inventory_scope.split(":", 2)[2]
 
     run_id = str(uuid.uuid4())
     run = AnsibleRun(

steward/core/audit.py

@@ -38,3 +38,11 @@ async def log_audit(
                 ))
     except Exception:
         logger.exception("Failed to write audit event action=%r", action)
+
+
+async def audit_request(action: str, entity_type: str | None = None,
+                        entity_id: str | None = None, detail: dict | None = None) -> None:
+    """log_audit for the logged-in user of the current web request."""
+    from quart import current_app, session
+    await log_audit(current_app, session.get("user_id"), session.get("username", ""),
+                    action, entity_type=entity_type, entity_id=entity_id, detail=detail)

steward/hosts/routes.py

@@ -221,8 +221,11 @@ async def run_playbook(host_id: str):
 
     # Shared parser: discovered var__ fields → extra_vars_map, secret__ → secret_vars
     # (unpersisted), plus tags/check. No --limit — there's exactly one host.
-    from steward.ansible.routes import _parse_run_params
+    from steward.ansible.routes import _parse_run_params, _run_vars, check_run_form
     params_or_none, secret_vars, err = _parse_run_params(form)
+    if err:
+        return err, 400
+    err = check_run_form(form, source, playbook, params_or_none, secret_vars)
     if err:
         return err, 400
 
@@ -254,7 +257,9 @@ async def run_playbook(host_id: str):
 
     await log_audit(current_app, session.get("user_id"), session.get("username", ""),
                     "ansible.host_run", entity_type="host", entity_id=host.name,
-                    detail={"playbook": playbook, "source": source_name})
+                    detail={"playbook": playbook, "source": source_name, "run_id": run_id,
+                            "check": bool((params_or_none or {}).get("check")),
+                            "vars": sorted(_run_vars(params_or_none, secret_vars))})
     return redirect(url_for("ansible.run_detail", run_id=run_id))
 
 

steward/mcp/tools/automation.py

@@ -19,10 +19,10 @@ from datetime import timedelta
 from sqlalchemy import select
 from sqlalchemy.orm import selectinload
 
-from steward.ansible import executor, sources as src_module
+from steward.ansible import executor, guards, sources as src_module
 from steward.ansible.inventory_gen import scope_labels
 from steward.ansible.runner import trigger_run
-from steward.mcp._context import current_caller, mcp_app, public_url
+from steward.mcp._context import current_caller, current_caller_or_none, mcp_app, public_url
 from steward.models.alerts import AlertRule
 from steward.models.ansible import AnsibleRun, AnsibleRunStatus
 from steward.models.ansible_inventory import AnsibleGroup, AnsibleTarget
@@ -118,6 +118,17 @@ def split_vars(variables: list[dict], extra_vars: dict | None) -> tuple[dict, di
     return plain, secret
 
 
+def check_caller_vars(extra_vars: dict | None, previous: dict | None = None) -> None:
+    """Refuse variables this caller's role may not set (ansible/guards.py):
+    connection-type ansible_* names and Jinja in values are admin-only. On an
+    update only what changes is held to it."""
+    caller = current_caller_or_none()
+    err = guards.check_vars_for_role(guards.changed_vars(extra_vars, previous),
+                                     caller.role if caller else None)
+    if err:
+        raise ValueError(f"{err} Nothing was run or saved.")
+
+
 def build_params(plain: dict, limit: str, tags: str, check: bool) -> dict:
     params: dict = {}
     if plain:
@@ -437,6 +448,7 @@ async def run_playbook(source: str, playbook: str, scope: str = "all",
         raise ValueError(f"{playbook} is flagged destructive by its author (steward:confirm). "
                          "Run it with check=true first; to apply, pass check=false and "
                          "confirmed=true. Nothing was run.")
+    check_caller_vars(extra_vars)
     variables = src_module.discover_playbook_variables(content)
     extra_vars, filled = steward_defaults(s, variables, extra_vars)
     plain, secret = split_vars(variables, extra_vars)
@@ -564,6 +576,7 @@ async def create_schedule(name: str, source: str, playbook: str, interval_s: int
     """
     if not (name or "").strip():
         raise ValueError("a schedule needs a name.")
+    check_caller_vars(extra_vars)
     async with db_session() as db, db.begin():
         fields = await _schedule_fields(db, source, playbook, scope, extra_vars, limit, tags, check)
         sc = AnsibleSchedule(name=name.strip()[:128], interval_seconds=_interval(interval_s),
@@ -589,6 +602,8 @@ async def update_schedule(schedule: str, name: str | None = None, source: str |
     async with db_session() as db, db.begin():
         sc = pick(schedule, await _schedules(db), kind="schedule", list_tool="list_schedules")
         params = dict(sc.params or {})
+        if extra_vars is not None:
+            check_caller_vars(extra_vars, params.get("extra_vars_map"))
         cur_scope = sc.inventory_scope
         if cur_scope.startswith("repo:"):
             cur_scope = "inventory:" + cur_scope.rsplit(":", 1)[-1]