fix(security): F01 (Astra P0) — KI-Tool-Ausführung ohne Freigabe verhindern
Check Cross-Plugin Imports / check (push) Waiting to run
Check Cross-Plugin Imports / check (push) Waiting to run
Vorher: _execute_tool (agent_loop.py) und der KI-Chat-Loop (stream_chat_comm) führten JEDES im Registry registrierte Tool aus, wenn das LLM dessen Namen lieferte — ohne Abgleich mit der angebotenen Liste, ohne required_permission-Check. Reproduktion (Astra): Nur audit_allowed angeboten, Modell nannte audit_restricted (system:admin) → Handler lief. Fix (fail-closed, an ALLEN Ausfuehrungspfaden): - _check_tool_access: (1) Allowlist — nur Tools die dem LLM angeboten wurden duerfen laufen; (2) required_permission gegen die AKTUELLEN User-Rechte (deny-first, Rechteentzug wirkt sofort, ohne Kontext = Ablehnung). Guard vor dry-run/approval/execute-Pfaden. - stream_chat_comm: gleicher Allowlist-Guard vor execute_tool_call. - run_react_loop/agent_runner/agent_stream/agent_routes reichen user_permissions durch (perm_ctx bzw. Session-User). - check_permission: Session-Kontexte tragen denied_permissions statt denied — beide Keys werden gelesen, Deny-Liste wird nie mehr ignoriert. Tests: test_agent_loop.py 18/18 (7 neue F01-Tests nach Astra-Abnahme: nicht angeboten → Handler null; fehlende Permission → abgewiesen; Fail-closed ohne Kontext; Deny-Liste session-shape; Rechteentzug mitten im Lauf wirkt auf naechste Aktion; dry-run guardet auch). ruff clean. Pre-existing-Beweis: permission_system_live-Failures reproduzieren sich ohne diesen Patch identisch (Plugin-Aktivierung in ephemeraler Test-DB, bekanntes Vorbestands-Finding).
This commit is contained in:
+101
-1
@@ -122,6 +122,10 @@ async def _execute_tool(
|
||||
"""Execute a single tool call via the registry.
|
||||
|
||||
Returns the tool result as a string, or an error message.
|
||||
|
||||
Note: access control (allowlist + required_permission) is enforced by
|
||||
``_check_tool_access`` in ``run_react_loop`` BEFORE any execution path
|
||||
(dry-run, approval, execute) is reached.
|
||||
"""
|
||||
tool = tool_registry.get(tool_name)
|
||||
if tool is None:
|
||||
@@ -135,6 +139,89 @@ async def _execute_tool(
|
||||
return f"Error: {exc}"
|
||||
|
||||
|
||||
def _extract_allowed_tool_names(tools: list[dict[str, Any]] | None) -> set[str]:
|
||||
"""Extract the tool names actually offered to the LLM.
|
||||
|
||||
Always returns a set (possibly empty) — empty means no tools were
|
||||
offered, so the caller fails closed on ANY tool call.
|
||||
"""
|
||||
if not tools:
|
||||
return set()
|
||||
names: set[str] = set()
|
||||
for t in tools:
|
||||
fn = (t or {}).get("function") or {}
|
||||
name = fn.get("name")
|
||||
if name:
|
||||
names.add(str(name))
|
||||
return names
|
||||
|
||||
|
||||
def _normalize_permissions(ctx: dict[str, Any] | None) -> dict[str, Any]:
|
||||
"""Normalize a user/agent context into the resolved-permissions shape
|
||||
expected by ``check_permission`` (permissions / denied / is_system_admin).
|
||||
|
||||
Session user contexts carry ``denied_permissions`` while resolved
|
||||
permission dicts use ``denied`` — both are accepted here.
|
||||
"""
|
||||
if not isinstance(ctx, dict):
|
||||
return {"permissions": [], "denied": [], "is_system_admin": False}
|
||||
return {
|
||||
"permissions": list(ctx.get("permissions", []) or []),
|
||||
"denied": list(ctx.get("denied", ctx.get("denied_permissions", [])) or []),
|
||||
"is_system_admin": bool(ctx.get("is_system_admin", False)),
|
||||
}
|
||||
|
||||
|
||||
def _check_tool_access(
|
||||
tool_registry: ToolRegistry,
|
||||
tool_name: str,
|
||||
allowed_tools: set[str] | None,
|
||||
user_permissions: dict[str, Any] | None,
|
||||
) -> str | None:
|
||||
"""F01 guard: enforce allowlist + required_permission before execution.
|
||||
|
||||
Must run before EVERY execution path (dry-run, approval, execute).
|
||||
Returns None when access is granted, otherwise an error observation.
|
||||
|
||||
Checks (fail-closed):
|
||||
1. Allowlist — the tool must be among the schemas actually offered to
|
||||
the LLM. A hallucinated/injected tool name never reaches a handler.
|
||||
2. required_permission — when the tool declares one, the acting user's
|
||||
CURRENT permissions must grant it. Without a permission context the
|
||||
call is rejected (deny list first, system admin bypass).
|
||||
"""
|
||||
tool = tool_registry.get(tool_name)
|
||||
if tool is None:
|
||||
return None # "not found" is handled by _execute_tool
|
||||
|
||||
# 1. Allowlist: only tools offered to the LLM may run.
|
||||
if allowed_tools is not None and tool_name not in allowed_tools:
|
||||
logger.warning(
|
||||
"F01 guard: tool '%s' is registered but NOT offered to this agent — rejected",
|
||||
tool_name,
|
||||
)
|
||||
return f"Error: Tool '{tool_name}' is not available to this agent"
|
||||
|
||||
# 2. required_permission: enforce against the user's CURRENT permissions.
|
||||
required = getattr(tool, "required_permission", None)
|
||||
if isinstance(required, str) and required:
|
||||
resolved = _normalize_permissions(user_permissions)
|
||||
from app.core.permissions import check_permission
|
||||
|
||||
if not check_permission(resolved, required):
|
||||
logger.warning(
|
||||
"F01 guard: tool '%s' requires '%s' which the acting user lacks — rejected",
|
||||
tool_name,
|
||||
required,
|
||||
)
|
||||
return (
|
||||
f"Error: Permission '{required}' required for tool '{tool_name}' "
|
||||
"and not granted to the acting user"
|
||||
)
|
||||
|
||||
return None
|
||||
|
||||
|
||||
async def run_react_loop(
|
||||
agent_definition: Any, # AgentDefinition from automation models
|
||||
messages: list[dict[str, Any]],
|
||||
@@ -151,6 +238,7 @@ async def run_react_loop(
|
||||
dry_run: bool = False,
|
||||
require_approval: bool = False,
|
||||
approval_tools: list[str] | None = None,
|
||||
user_permissions: dict[str, Any] | None = None,
|
||||
) -> ReActResult:
|
||||
"""Execute a ReAct loop: LLM reasoning → tool execution → repeat.
|
||||
|
||||
@@ -200,6 +288,10 @@ async def run_react_loop(
|
||||
"agent_name": getattr(agent_definition, "name", "Agent"),
|
||||
}
|
||||
|
||||
# F01 (Astra P0): allowlist — only tools actually offered to the LLM
|
||||
# may ever execute. Empty set = no tools offered = every call rejected.
|
||||
allowed_tool_names: set[str] = _extract_allowed_tool_names(tools)
|
||||
|
||||
# Audit helper — records every tool call in the audit log.
|
||||
async def _audit_tool_call(
|
||||
step_number: int,
|
||||
@@ -366,7 +458,15 @@ async def run_react_loop(
|
||||
args = {}
|
||||
logger.warning("Invalid JSON arguments for tool '%s': %s", tool_name, tc["arguments"])
|
||||
|
||||
if dry_run:
|
||||
# F01 (Astra P0): enforce allowlist + required_permission before
|
||||
# every execution path (dry-run, approval, execute). Fail-closed:
|
||||
# a hallucinated or injected tool name never reaches a handler.
|
||||
guard_error = _check_tool_access(
|
||||
tool_registry, tool_name, allowed_tool_names, user_permissions
|
||||
)
|
||||
if guard_error is not None:
|
||||
observation = guard_error
|
||||
elif dry_run:
|
||||
observation = json.dumps(
|
||||
{
|
||||
"dry_run": True,
|
||||
|
||||
@@ -20,7 +20,8 @@ import asyncio
|
||||
import json
|
||||
import logging
|
||||
import uuid
|
||||
from typing import TYPE_CHECKING, Any, AsyncGenerator
|
||||
from collections.abc import AsyncGenerator
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
from app.ai.agent_loop import ReActStep, run_react_loop
|
||||
|
||||
@@ -71,6 +72,7 @@ async def stream_react_loop(
|
||||
max_steps: int = 20,
|
||||
timeout_seconds: int = 300,
|
||||
trace_id: str | None = None,
|
||||
user_permissions: dict[str, Any] | None = None,
|
||||
) -> AsyncGenerator[str, None]:
|
||||
"""Run the ReAct loop and yield SSE-formatted events.
|
||||
|
||||
@@ -116,6 +118,7 @@ async def stream_react_loop(
|
||||
timeout_seconds=timeout_seconds,
|
||||
trace_id=trace_id,
|
||||
on_step=on_step,
|
||||
user_permissions=user_permissions, # F01: enforce at execution time
|
||||
)
|
||||
await queue.put(
|
||||
_sse(
|
||||
|
||||
Reference in New Issue
Block a user