feat(sdk): out of box controls - part 2 - #247
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| r"\b(?:rm\s+-rf\s+(?:/|~|\$HOME)|sudo\s+rm\s+-rf|" | ||
| r"mkfs(?:\.[a-z0-9]+)?|dd\s+if=[^\s]+\s+of=/dev/[^\s]+|" | ||
| r"chmod\s+-R\s+777\s+/|chown\s+-R\s+[^|;&]*\s+/|" | ||
| r"shutdown\s+(?:-h\s+)?now|reboot)\b" |
There was a problem hiding this comment.
The shared trailing \b cannot match after / or ~, so rm -rf /, rm -rf ~, chmod -R 777 /, and chown -R root / pass this control. Please use explicit end/separator handling per alternative and add these cases to the test.
There was a problem hiding this comment.
Fixed.
| "anyOf": [ | ||
| { | ||
| "required": ["approved"], | ||
| "properties": {"approved": {"const": True}}, |
There was a problem hiding this comment.
Could we avoid treating a caller-controlled approved flag as authorization evidence? A prompt-injected agent can set or replay it, while tools without this field cannot complete the flow. This should use a trusted approval artifact or host workflow bound to the action; the outbound template below has the same issue.
There was a problem hiding this comment.
Addressed by removing caller-controlled approval flags as bypasses from both templates. Qualifying high-value and outbound actions now always steer to a trusted host workflow, which must bind its approval artifact to the exact action. Tests verify that both top-level and nested approval flags cannot bypass the controls.
| name="oob-sensitive-tool-requires-approved-role", | ||
| data=_leaf_control_payload( | ||
| description="Deny sensitive tool use when runtime context has an unapproved role.", | ||
| selector_path="context.user.role", |
There was a problem hiding this comment.
This fails open when context.user.role is missing: the selector returns None, and ListEvaluator returns matched=False before applying match_on="no_match". Please fail closed for missing/unknown roles and source the role from trusted principal context.
There was a problem hiding this comment.
Agreed. AC does not yet provide a trusted principal-role contract, so this control cannot securely distinguish authorized roles at the control layer. Making the selector fail closed would address missing values but would still trust caller-controlled context. We’ll remove this OOTB control for now and reintroduce it when RBAC/trusted principal context is supported.
| selector_path="name", | ||
| evaluator_name="list", | ||
| evaluator_config={ | ||
| "values": ["search", "web_search", "retrieve", "calculator"], |
There was a problem hiding this comment.
Exact bare names do not match integrations that qualify tool names. Google ADK emits names such as <agent>.web_search, so this denies an explicitly approved tool. Please normalize tool identity across integrations or match a canonical raw name, with an integration-level test.
There was a problem hiding this comment.
Addressed by adding a canonical step identity to the shared Step contract. Integrations can retain qualified names for registration while controls evaluate the integration-independent tool name. Google ADK now sends web_search as canonical identity for names such as writer.web_search; Strands is wired similarly. Added ADK integration coverage and fallback tests for unqualified names.
| try: | ||
| if namespace_key == default_out_of_box_namespace_key(): | ||
| return | ||
| if await _namespace_has_active_controls(db, namespace_key=namespace_key): |
There was a problem hiding this comment.
Checking for any active control makes partial seeding permanent: templates commit separately, so one transient failure leaves rows that cause every later request to return here. It also skips tenants with one custom control. Please check expected seed identities or always run the idempotent per-template pass.
There was a problem hiding this comment.
Fixed by removing the namespace-wide active-control check and always running the idempotent per-template seed pass. This allows partially seeded namespaces to recover and ensures a tenant’s custom controls do not suppress OOTB seeding. Also added regression coverage for both scenarios.
| return | ||
| if await _namespace_has_active_controls(db, namespace_key=namespace_key): | ||
| return | ||
| await seed_out_of_box_controls( |
There was a problem hiding this comment.
This path is reached from a GET authorized only with controls.read, but it commits Control and ControlVersion rows. Could seeding move to trusted tenant provisioning, or require controls.create resolved to the same namespace, so read-only callers cannot trigger writes?
There was a problem hiding this comment.
This write-on-GET behavior is intentional for OOTB controls. The Galileo All controls page only calls GET /api/v1/controls?cloned=false; there is currently no tenant-provisioning call, and the organization namespace is resolved from the existing controls.read authorization. We need the fixed, server-owned OOTB controls to appear on the first page load, regardless of agent or logstream attachment. The reconciliation is idempotent, uses immutable seed identities, and respects deletion tombstones; callers cannot provide or modify the seeded payloads. Standalone/open-source deployments are seeded at startup, while this path covers dynamically discovered organization namespaces without an additional authorization request.
namrataghadi-galileo
changed the title
2 participants
Summary
Scope
OUT_OF_BOX_CONTROL_TEMPLATESand expanded bootstrap/evaluator behavior tests.Risk and Rollout
server/src/agent_control_server/bootstrap/out_of_box_controls.py, or remove specific templates fromOUT_OF_BOX_CONTROL_TEMPLATES.Testing
make check(not run becauseuvdependency resolution hit private index/auth issues previously).venvChecklist