-
Notifications
You must be signed in to change notification settings - Fork 590
fix(runner): narrow the runner environment and drop subscription auto-upload #5298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
161 changes: 161 additions & 0 deletions
161
hosting/kubernetes/helm/tests/test_runner_secret_absence.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| # /// script | ||
| # requires-python = ">=3.11" | ||
| # dependencies = ["PyYAML>=6"] | ||
| # /// | ||
| """Rendered-chart regression guard for the runner's narrow environment. | ||
|
|
||
| The agent runner must run with a deliberately narrow environment (interface.md sections 2, 9, | ||
| and the runner-selfhosting-cleanup design): a local harness process shares the runner container, | ||
| so anything on the runner's process environment is readable from /proc by user code. This test | ||
| renders the Helm chart and asserts the runner Deployment's container env contains ONLY runner and | ||
| provider-registry variables — never the platform's database, auth, crypt, license, Redis, object | ||
| store, or unrelated provider secrets, and never a static AGENTA_API_KEY. It is the guard that keeps | ||
| a future `agenta.commonEnv` include (or any broad env block) from silently re-widening the runner. | ||
|
|
||
| Run: uv run hosting/kubernetes/helm/tests/test_runner_secret_absence.py | ||
| Requires the `helm` binary on PATH. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import yaml | ||
|
|
||
| CHART_DIR = Path(__file__).resolve().parents[1] | ||
|
|
||
| # Minimum values needed for the chart to render (URLs are required by the chart's own guard). | ||
| BASE_ARGS = [ | ||
| "--set", | ||
| "agenta.webUrl=https://agenta.example.com", | ||
| "--set", | ||
| "agenta.apiUrl=https://agenta.example.com/api", | ||
| "--set", | ||
| "agenta.servicesUrl=https://agenta.example.com/services", | ||
| "--set", | ||
| "agentRunner.enabled=true", | ||
| ] | ||
|
|
||
| TOKEN_ARGS = [ | ||
| "--set", | ||
| "agentRunner.auth.tokenSecretRef.name=agenta-runner", | ||
| "--set", | ||
| "agentRunner.auth.tokenSecretRef.key=token", | ||
| ] | ||
|
|
||
| # Exact names the runner container env must NEVER contain. | ||
| FORBIDDEN_EXACT = { | ||
| "AGENTA_AUTH_KEY", | ||
| "AGENTA_CRYPT_KEY", | ||
| "AGENTA_LICENSE", | ||
| "AGENTA_API_KEY", | ||
| "OPENAI_API_KEY", | ||
| "ANTHROPIC_API_KEY", | ||
| "GEMINI_API_KEY", | ||
| "COHERE_API_KEY", | ||
| "MISTRAL_API_KEY", | ||
| } | ||
|
|
||
| # Prefixes the runner container env must NEVER contain (databases, cache, object store). | ||
| FORBIDDEN_PREFIXES = ( | ||
| "POSTGRES_", | ||
| "REDIS_", | ||
| "AGENTA_REDIS_", | ||
| "AGENTA_STORE_", | ||
| ) | ||
|
|
||
| # Names the runner container env MUST contain (its own configuration). | ||
| REQUIRED = { | ||
| "AGENTA_RUNNER_PORT", | ||
| "AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS", | ||
| } | ||
|
|
||
|
|
||
| def render(extra_args: list[str]) -> list[dict]: | ||
| result = subprocess.run( | ||
| [ | ||
| "helm", | ||
| "template", | ||
| "runner-env-test", | ||
| str(CHART_DIR), | ||
| *BASE_ARGS, | ||
| *extra_args, | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| return [doc for doc in yaml.safe_load_all(result.stdout) if doc] | ||
|
|
||
|
|
||
| def runner_container_env_names(docs: list[dict]) -> list[str]: | ||
| """The env var NAMES on the runner Deployment's `runner` container.""" | ||
| for doc in docs: | ||
| if doc.get("kind") != "Deployment": | ||
| continue | ||
| labels = doc.get("metadata", {}).get("labels", {}) | ||
| if labels.get("app.kubernetes.io/component") != "runner": | ||
| continue | ||
| containers = doc["spec"]["template"]["spec"]["containers"] | ||
| runner = next(c for c in containers if c["name"] == "runner") | ||
| return [entry["name"] for entry in runner.get("env", [])] | ||
| raise AssertionError("no runner Deployment found in the rendered chart") | ||
|
|
||
|
|
||
| def check(names: list[str], *, expect_token: bool) -> list[str]: | ||
| failures: list[str] = [] | ||
| present = set(names) | ||
|
|
||
| for forbidden in sorted(FORBIDDEN_EXACT): | ||
| if forbidden in present: | ||
| failures.append(f"runner env must not contain {forbidden}") | ||
|
|
||
| for name in names: | ||
| for prefix in FORBIDDEN_PREFIXES: | ||
| if name.startswith(prefix): | ||
| failures.append(f"runner env must not contain {name} (prefix {prefix})") | ||
|
|
||
| for required in sorted(REQUIRED): | ||
| if required not in present: | ||
| failures.append(f"runner env must contain {required}") | ||
|
|
||
| if expect_token and "AGENTA_RUNNER_TOKEN" not in present: | ||
| failures.append( | ||
| "runner env must contain AGENTA_RUNNER_TOKEN when auth.tokenSecretRef is set" | ||
| ) | ||
| if not expect_token and "AGENTA_RUNNER_TOKEN" in present: | ||
| failures.append( | ||
| "runner env must not contain AGENTA_RUNNER_TOKEN when auth.tokenSecretRef is unset" | ||
| ) | ||
|
|
||
| return failures | ||
|
|
||
|
|
||
| def main() -> int: | ||
| failures: list[str] = [] | ||
|
|
||
| # Default deployment (no token ref): the narrow env, no runner token. | ||
| names = runner_container_env_names(render([])) | ||
| failures += check(names, expect_token=False) | ||
|
|
||
| # Token configured: AGENTA_RUNNER_TOKEN appears, everything else stays narrow. | ||
| names_with_token = runner_container_env_names(render(TOKEN_ARGS)) | ||
| failures += check(names_with_token, expect_token=True) | ||
|
|
||
| if failures: | ||
| print("FAIL: runner environment is not narrow:", file=sys.stderr) | ||
| for line in failures: | ||
| print(f" - {line}", file=sys.stderr) | ||
| return 1 | ||
|
|
||
| print( | ||
| "OK: runner Deployment env is narrow (no platform secrets, provider keys, or API key)." | ||
| ) | ||
| print(f" default env: {sorted(names)}") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add
CLAUDE_CONFIG_DIRto theenvironmentblock.The comment instructs users to "override the defaults above" for both
PI_CODING_AGENT_DIRandCLAUDE_CONFIG_DIR. However, unlikePI_CODING_AGENT_DIR,CLAUDE_CONFIG_DIRis missing from theenvironmentsection above. Since therunnerservice lacks anenv_filedirective, Docker Compose will drop this variable if set on the host environment or a global.envfile, breaking the documented feature.💻 Proposed fix
Declare
CLAUDE_CONFIG_DIRin theenvironmentsection of therunnerservice (around line 293):AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER: ${AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER:-local} PI_CODING_AGENT_DIR: ${PI_CODING_AGENT_DIR:-/pi-agent} + CLAUDE_CONFIG_DIR: ${CLAUDE_CONFIG_DIR:-} AGENTA_RUNNER_DAYTONA_API_KEY: ${AGENTA_RUNNER_DAYTONA_API_KEY:-}