From faa605ceadb4845ab8bc5626bbcfeb1f20a4410a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 6 Oct 2026 11:14:37 -0400 Subject: [PATCH] feat(secrets): webhook URL and git tokens sealed; run output masks its secrets (#5037) #5037 "Secrets at rest and in run output: webhook URL and git token encrypted, askpass via env, secret values scrubbed from logs". - webhook.url joins SECRET_KEYS; existing values are sealed at startup by migrate_plaintext_secrets. Settings -> Notifications no longer echoes the URL: it names the saved host, keeps it when left blank, and has a Remove box. - Git source http_token is sealed inside ansible.sources on save and at startup, opened in get_sources, and counted by the undecryptable-secrets scan as ansible.sources[].http_token. - The askpass script holds no token: git runs it with STEWARD_GIT_TOKEN in its environment and printf prints it as data. - The executor masks every secret the run is handed (become/vault passwords, bootstrap passwords, sensitive run vars, and the per-host agent tokens the host_agent plugin now passes as redact) in each line before it is broadcast, logged or stored. - Bundled "Write agent config" tasks are no_log; a test fails if any bundled task templating steward_token lacks it. Co-Authored-By: Claude Opus 5.5 --- plugins/host_agent/routes.py | 2 + .../ansible/bundled/host_agent/install.yml | 2 + .../ansible/bundled/host_agent/provision.yml | 2 + steward/ansible/executor.py | 48 ++++++++++++++- steward/ansible/runner.py | 6 +- steward/ansible/sources.py | 38 ++++++++---- steward/core/settings.py | 43 +++++++++++++ steward/settings/routes.py | 14 +++-- steward/templates/settings/notifications.html | 14 ++++- tests/core/test_git_askpass.py | 61 +++++++++++++++---- tests/core/test_run_scrub.py | 57 +++++++++++++++++ 11 files changed, 252 insertions(+), 35 deletions(-) create mode 100644 tests/core/test_run_scrub.py diff --git a/plugins/host_agent/routes.py b/plugins/host_agent/routes.py index f5c2221..118f075 100644 --- a/plugins/host_agent/routes.py +++ b/plugins/host_agent/routes.py @@ -959,6 +959,7 @@ async def deploy_via_ansible(): playbook_path="host_agent/install.yml", inventory_content=inventory_content, inventory_scope=scope, + redact=list(tokens.values()), # extra-vars outrank the playbook's `vars:` defaults; token stays per-host. params={"extra_vars_map": {"steward_url": url, "agent_interval": interval}}, triggered_by=session.get("user_id"), @@ -1036,6 +1037,7 @@ async def provision_via_ansible(): playbook_path="host_agent/provision.yml", inventory_content=inventory_content, inventory_scope=scope, + redact=list(tokens.values()), # extra_vars_map → a JSON -e @file: highest precedence (beats play vars) # and space-safe (steward_pubkey carries a comment with a space). params={"extra_vars_map": { diff --git a/steward/ansible/bundled/host_agent/install.yml b/steward/ansible/bundled/host_agent/install.yml index ce50939..92af713 100644 --- a/steward/ansible/bundled/host_agent/install.yml +++ b/steward/ansible/bundled/host_agent/install.yml @@ -61,6 +61,8 @@ notify: restart steward-agent - name: Write agent config + # The content carries steward_token; --diff and a failure would print it. + no_log: true ansible.builtin.copy: dest: /etc/steward-agent.conf owner: root diff --git a/steward/ansible/bundled/host_agent/provision.yml b/steward/ansible/bundled/host_agent/provision.yml index 21b62ac..bd9bc83 100644 --- a/steward/ansible/bundled/host_agent/provision.yml +++ b/steward/ansible/bundled/host_agent/provision.yml @@ -117,6 +117,8 @@ notify: restart steward-agent - name: Write agent config + # The content carries steward_token; --diff and a failure would print it. + no_log: true ansible.builtin.copy: dest: /etc/steward-agent.conf owner: root diff --git a/steward/ansible/executor.py b/steward/ansible/executor.py index ac8667e..4199ff3 100644 --- a/steward/ansible/executor.py +++ b/steward/ansible/executor.py @@ -293,6 +293,45 @@ def cancel_run(run_id: str) -> bool: # ── Run execution ───────────────────────────────────────────────────────────── +# ── Secret scrubbing ────────────────────────────────────────────────────────── +# Ansible echoes values back: a failed task prints its args, -v prints vars, +# --diff prints templated files. Each line is masked before it is broadcast, +# written to the log artifact or stored on the run, so a secret the run was +# handed never leaves it. +MASK = "***" +_MIN_SECRET_LEN = 4 # masking "on" or "1" would mangle the output and hide nothing + + +def _strings(value) -> list[str]: + if isinstance(value, str): + return [value] + if isinstance(value, dict): + return [s for v in value.values() for s in _strings(v)] + if isinstance(value, (list, tuple, set)): + return [s for v in value for s in _strings(v)] + return [] + + +def run_secrets(creds: dict | None, connection: dict | None, + secret_vars: dict | None, redact: list[str] | None) -> list[str]: + """Every secret value this run is handed: the global become/vault + passwords, the bootstrap passwords, the sensitive run variables and any + the caller names (a per-host agent token).""" + creds, connection = creds or {}, connection or {} + values = [creds.get("become_password"), creds.get("vault_password"), + connection.get("password"), connection.get("become_password"), + *_strings(secret_vars), *(redact or [])] + found = {v for v in values if isinstance(v, str) and len(v) >= _MIN_SECRET_LEN} + return sorted(found, key=len, reverse=True) # longest first: no partial masks + + +def scrub(line: str, secrets: list[str]) -> str: + for value in secrets: + if value in line: + line = line.replace(value, MASK) + return line + + async def start_run( app: "Quart", run_id: str, @@ -303,6 +342,7 @@ async def start_run( inventory_content: str | None = None, connection: dict | None = None, secret_vars: dict | None = None, + redact: list[str] | None = None, ) -> None: """Execute ansible-playbook as a subprocess and update the DB run row. @@ -315,10 +355,12 @@ async def start_run( never persisted on the AnsibleRun row, never placed on argv, and not logged. secret_vars are run-time playbook variables flagged sensitive; like connection they reach the subprocess (merged into the extra-vars file) but - are never persisted. + are never persisted. Every value in run_secrets() — redact names more — is + masked in each output line before it goes anywhere. """ from steward.models.ansible import AnsibleRunStatus + secrets = run_secrets(app.config.get("ANSIBLE", {}), connection, secret_vars, redact) _run_lines[run_id] = [] db_output = "" truncated = False @@ -432,7 +474,7 @@ async def start_run( assert proc.stdout is not None async for raw_line in proc.stdout: - line = raw_line.decode(errors="replace").rstrip("\n\r") + line = scrub(raw_line.decode(errors="replace").rstrip("\n\r"), secrets) _broadcast(run_id, line) if logf is not None: logf.write(line + "\n") @@ -478,7 +520,7 @@ async def start_run( final_status = "cancelled" if run_id in _cancelled else "failed" # Surface WHY in the run output instead of an empty "failed" (e.g. ENOSPC # when the temp dir can't be created — the run never reaches Ansible). - msg = f"[run error] {type(exc).__name__}: {exc}" + msg = scrub(f"[run error] {type(exc).__name__}: {exc}", secrets) _broadcast(run_id, msg) if len(failures) < _FAILURE_CAP: failures.append(msg[:500]) diff --git a/steward/ansible/runner.py b/steward/ansible/runner.py index 6606596..0d1eacc 100644 --- a/steward/ansible/runner.py +++ b/steward/ansible/runner.py @@ -28,6 +28,7 @@ async def trigger_run( inventory_content: str | None = None, connection: dict | None = None, secret_vars: dict | None = None, + redact: list[str] | None = None, ): """Resolve inventory for the scope, persist an AnsibleRun, and launch it. @@ -39,7 +40,8 @@ async def trigger_run( first-contact provisioning — passed to the executor but deliberately NOT stored on the AnsibleRun row (params), so the password never lands in the DB. secret_vars (sensitive run-time playbook variables) are likewise passed - through to the executor but never persisted. + through to the executor but never persisted. redact names further values + to mask in the run's output (a bespoke inventory's per-host tokens). Returns (run, source, error): on success error is None; on failure run is None and error is a short human-readable reason. """ @@ -85,7 +87,7 @@ async def trigger_run( executor.start_run( app, run_id, playbook_path, inventory_path or "", source["path"], params or None, inventory_content, - connection=connection, secret_vars=secret_vars, + connection=connection, secret_vars=secret_vars, redact=redact, ) ) task.add_done_callback( diff --git a/steward/ansible/sources.py b/steward/ansible/sources.py index 9c0cc6e..b1919ca 100644 --- a/steward/ansible/sources.py +++ b/steward/ansible/sources.py @@ -108,6 +108,13 @@ def is_editable_source(source: dict) -> bool: return source.get("type") == "local" and source.get("name") != BUILTIN_SOURCE_NAME +def _open_token(src: dict) -> str: + """The source's git token in the clear (it is stored sealed).""" + from steward.core.crypto import decrypt_secret + from steward.core.settings import source_token_key + return decrypt_secret(src.get("http_token") or "", context=source_token_key(src.get("name", "?"))) + + def get_sources(ansible_cfg: dict) -> list[dict]: """Return resolved source list from ansible config section. @@ -149,7 +156,7 @@ def get_sources(ansible_cfg: dict) -> list[dict]: "url": src.get("url"), "branch": src.get("branch", "main"), "pull_interval_seconds": int(src.get("pull_interval_seconds", 3600)), - "http_token": src.get("http_token", "") if src_type == "git" else "", + "http_token": _open_token(src) if src_type == "git" else "", }) return result @@ -323,11 +330,25 @@ def validate_playbook_yaml(content: str) -> tuple[bool, str | None]: return True, None +# The askpass script holds no secret: git runs it with the token in its +# environment, and printf prints it as data, so no character in a token can +# become shell syntax. +ASKPASS_TOKEN_ENV = "STEWARD_GIT_TOKEN" +ASKPASS_SCRIPT = ( + "#!/bin/sh\n" + 'case "$1" in\n' + " Username*) printf '%s\\n' oauth2 ;;\n" + f" Password*) printf '%s\\n' \"${ASKPASS_TOKEN_ENV}\" ;;\n" + "esac\n" +) + + async def git_pull(source: dict) -> None: """Clone the git repo if absent; pull if already present. - When source['http_token'] is set, injects credentials via a temporary - GIT_ASKPASS script so they never touch .git/config or process args. + When source['http_token'] is set, git asks a temporary GIT_ASKPASS script, + which prints the token from the child's environment, so it never touches + .git/config, process args or a file. """ path = Path(source["path"]) token = source.get("http_token", "") @@ -337,18 +358,11 @@ async def git_pull(source: dict) -> None: if token: fd, askpass_path = tempfile.mkstemp(prefix="steward-askpass-", suffix=".sh") try: - script = ( - "#!/bin/sh\n" - 'case "$1" in\n' - ' Username*) echo "oauth2" ;;\n' - f' Password*) echo "{token}" ;;\n' - "esac\n" - ) - os.write(fd, script.encode()) + os.write(fd, ASKPASS_SCRIPT.encode()) finally: os.close(fd) os.chmod(askpass_path, stat.S_IRWXU) - env = {**os.environ, "GIT_ASKPASS": askpass_path} + env = {**os.environ, "GIT_ASKPASS": askpass_path, ASKPASS_TOKEN_ENV: token} try: if not (path / ".git").exists(): diff --git a/steward/core/settings.py b/steward/core/settings.py index 8e17c75..aad7d41 100644 --- a/steward/core/settings.py +++ b/steward/core/settings.py @@ -160,6 +160,9 @@ SECRET_KEYS: set[str] = { "ansible.ssh_private_key", "ansible.become_password", "ansible.vault_password", + # A webhook URL carries its own credential (Discord, Slack and ntfy put the + # token in the path). + "webhook.url", } @@ -201,6 +204,31 @@ def _is_undecryptable(stored: Any, key: str = "") -> bool: ) +# Git sources live as a list under one setting, so their tokens are sealed +# field by field rather than through SECRET_KEYS. +SOURCES_KEY = "ansible.sources" + + +def seal_source_tokens(sources: Any) -> tuple[Any, int]: + """Encrypt each source's plaintext http_token; returns (sources, how many).""" + from steward.core.crypto import encrypt_secret, is_encrypted + if not isinstance(sources, list): + return sources, 0 + sealed, count = [], 0 + for src in sources: + token = src.get("http_token") if isinstance(src, dict) else None + if isinstance(token, str) and token and not is_encrypted(token): + src = {**src, "http_token": encrypt_secret(token)} + count += 1 + sealed.append(src) + return sealed, count + + +def source_token_key(name: str) -> str: + """How a source's token is named in logs and the undecryptable-secrets banner.""" + return f"{SOURCES_KEY}[{name}].http_token" + + # ───────────────────────────────────────────────────────────────────────────── # Async helpers (use inside request handlers / scheduled tasks) # ───────────────────────────────────────────────────────────────────────────── @@ -507,6 +535,13 @@ def migrate_plaintext_secrets(db_url: str) -> int: if isinstance(val, str) and val and not is_encrypted(val): row.value_json = json.dumps(encrypt_secret(val)) converted += 1 + row = (await session.execute( + select(AppSetting).where(AppSetting.key == SOURCES_KEY))).scalar_one_or_none() + if row is not None: + sources, sealed = seal_source_tokens(json.loads(row.value_json)) + if sealed: + row.value_json = json.dumps(sources) + converted += sealed finally: await engine.dispose() return converted @@ -535,6 +570,14 @@ def scan_undecryptable_secrets(db_url: str) -> list[str]: for row in result.scalars(): if _is_undecryptable(json.loads(row.value_json), row.key): bad.append(row.key) + row = (await session.execute( + select(AppSetting).where(AppSetting.key == SOURCES_KEY))).scalar_one_or_none() + sources = json.loads(row.value_json) if row is not None else [] + for src in sources if isinstance(sources, list) else []: + if isinstance(src, dict): + key = source_token_key(src.get("name", "?")) + if _is_undecryptable(src.get("http_token"), key): + bad.append(key) finally: await engine.dispose() return bad diff --git a/steward/settings/routes.py b/steward/settings/routes.py index 6478eb7..b0d7632 100644 --- a/steward/settings/routes.py +++ b/steward/settings/routes.py @@ -11,7 +11,7 @@ from steward.core.settings import ( get_all_settings, set_setting, delete_setting, get_stored_plugin_names, find_orphaned_plugins, to_smtp_cfg, to_webhook_cfg, to_ansible_cfg, to_plugins_cfg, - to_oidc_cfg, to_ldap_cfg, to_thresholds_cfg, to_access_cfg, DEFAULTS, + to_oidc_cfg, to_ldap_cfg, to_thresholds_cfg, to_access_cfg, DEFAULTS, seal_source_tokens, ) settings_bp = Blueprint("settings", __name__, url_prefix="/settings") @@ -208,7 +208,10 @@ async def save_thresholds(): async def notifications(): async with current_app.db_sessionmaker() as db: settings = await get_all_settings(db) - return await render_template("settings/notifications.html", settings=settings) + from urllib.parse import urlsplit + webhook_host = urlsplit(settings.get("webhook.url") or "").hostname or "" + return await render_template("settings/notifications.html", settings=settings, + webhook_host=webhook_host) @settings_bp.post("/notifications/") @@ -226,7 +229,10 @@ async def save_notifications(): if form.get("smtp.password"): await set_setting(db, "smtp.password", form.get("smtp.password")) await set_setting(db, "smtp.recipients", recipients) - await set_setting(db, "webhook.url", form.get("webhook.url", "")) + if "webhook.remove" in form: + await set_setting(db, "webhook.url", "") + elif form.get("webhook.url", "").strip(): + await set_setting(db, "webhook.url", form.get("webhook.url").strip()) await set_setting(db, "webhook.template", form.get("webhook.template", "")) await _reload_app_config() await log_audit(current_app, session.get("user_id"), session.get("username", ""), @@ -246,7 +252,7 @@ async def _get_ansible_sources() -> list[dict]: async def _save_ansible_sources(sources: list[dict]) -> None: async with current_app.db_sessionmaker() as db: async with db.begin(): - await set_setting(db, "ansible.sources", sources) + await set_setting(db, "ansible.sources", seal_source_tokens(sources)[0]) await _reload_app_config() diff --git a/steward/templates/settings/notifications.html b/steward/templates/settings/notifications.html index ee8ebf9..4cbb834 100644 --- a/steward/templates/settings/notifications.html +++ b/steward/templates/settings/notifications.html @@ -56,10 +56,18 @@

Webhook

- - + + +

+ Stored encrypted and never shown again: a webhook URL usually holds its own token.

+ {% if webhook_host %} +
+ + +
+ {% endif %}
diff --git a/tests/core/test_git_askpass.py b/tests/core/test_git_askpass.py index 6fb9c53..20d7330 100644 --- a/tests/core/test_git_askpass.py +++ b/tests/core/test_git_askpass.py @@ -77,9 +77,9 @@ async def test_git_pull_creates_askpass_when_token_set(tmp_path): @pytest.mark.asyncio -async def test_git_pull_askpass_contains_token(tmp_path): - """The askpass script content includes the token before cleanup.""" - from steward.ansible.sources import git_pull +async def test_git_pull_askpass_reads_the_token_from_its_environment(tmp_path): + """The askpass script holds no token; git's environment carries it.""" + from steward.ansible.sources import ASKPASS_TOKEN_ENV, git_pull repo_path = tmp_path / "repo" (repo_path / ".git").mkdir(parents=True) @@ -89,17 +89,19 @@ async def test_git_pull_askpass_contains_token(tmp_path): "path": str(repo_path), "url": "https://git.example.com/org/repo.git", "branch": "main", - "http_token": "secret-token", + "http_token": 'tok"$(id)`x`', } - script_content_captured = {} + captured = {} async def fake_subprocess(*args, **kwargs): - askpass = kwargs.get("env", {}).get("GIT_ASKPASS", "") + env = kwargs.get("env", {}) + askpass = env.get("GIT_ASKPASS", "") if askpass and os.path.exists(askpass): - script_content_captured["content"] = Path(askpass).read_text() + captured["content"] = Path(askpass).read_text() mode = stat.S_IMODE(os.stat(askpass).st_mode) - script_content_captured["executable"] = bool(mode & stat.S_IXUSR) + captured["executable"] = bool(mode & stat.S_IXUSR) + captured["token"] = env.get(ASKPASS_TOKEN_ENV) proc = MagicMock() proc.returncode = 0 proc.communicate = AsyncMock(return_value=(b"", b"")) @@ -108,9 +110,46 @@ async def test_git_pull_askpass_contains_token(tmp_path): with patch("asyncio.create_subprocess_exec", side_effect=fake_subprocess): await git_pull(source) - assert "secret-token" in script_content_captured.get("content", "") - assert "oauth2" in script_content_captured.get("content", "") - assert script_content_captured.get("executable") is True + assert "tok" not in captured["content"] + assert "oauth2" in captured["content"] + assert captured["executable"] is True + assert captured["token"] == 'tok"$(id)`x`' + + +def test_askpass_prints_a_hostile_token_as_data(tmp_path): + """Run the real script: shell syntax in a token comes back verbatim.""" + import subprocess + from steward.ansible.sources import ASKPASS_SCRIPT, ASKPASS_TOKEN_ENV + + script = tmp_path / "askpass.sh" + script.write_text(ASKPASS_SCRIPT) + script.chmod(0o700) + token = 'a"b$(touch pwned)`id` c\\' + env = {"PATH": os.environ.get("PATH", ""), ASKPASS_TOKEN_ENV: token} + out = subprocess.run([str(script), "Password for 'https://x'"], env=env, cwd=tmp_path, + capture_output=True, text=True, check=True).stdout + assert out == token + "\n" + assert not (tmp_path / "pwned").exists() + user = subprocess.run([str(script), "Username for 'https://x'"], env=env, + capture_output=True, text=True, check=True).stdout + assert user == "oauth2\n" + + +def test_sealed_tokens_open_in_get_sources(tmp_path, monkeypatch): + from steward.ansible.sources import get_sources + from steward.core import crypto + from steward.core.settings import seal_source_tokens + + monkeypatch.setattr(crypto, "_fernet", crypto._make_fernet("unit-test-key")) + stored, count = seal_source_tokens([ + {"name": "repo", "type": "git", "url": "https://x/r.git", "http_token": "mytoken123"}, + {"name": "plain", "type": "git", "url": "https://x/p.git"}, + ]) + assert count == 1 and crypto.is_encrypted(stored[0]["http_token"]) + assert seal_source_tokens(stored) == (stored, 0) # sealing twice changes nothing + src = next(s for s in get_sources({"cache_dir": str(tmp_path), "sources": stored}) + if s["name"] == "repo") + assert src["http_token"] == "mytoken123" @pytest.mark.asyncio diff --git a/tests/core/test_run_scrub.py b/tests/core/test_run_scrub.py new file mode 100644 index 0000000..e5521c4 --- /dev/null +++ b/tests/core/test_run_scrub.py @@ -0,0 +1,57 @@ +"""A run's secrets never reach its output: each line is masked before it is +broadcast, written to the log artifact or stored on the run.""" +from steward.ansible.executor import MASK, run_secrets, scrub + + +def test_run_secrets_gathers_everything_the_run_is_handed(): + secrets = run_secrets( + {"become_password": "become-pw", "vault_password": "vault-pw", "ssh_user": "steward"}, + {"user": "root", "password": "boot-pw"}, + {"db": {"password": "nested-pw"}, "api_keys": ["key-one"], "port": 5432}, + ["agent-token-abc"], + ) + assert set(secrets) == {"become-pw", "vault-pw", "boot-pw", "nested-pw", "key-one", + "agent-token-abc"} + assert "steward" not in secrets and "root" not in secrets + + +def test_short_values_are_not_masked(): + assert run_secrets(None, None, {"flag": "on", "n": "1"}, None) == [] + + +def test_scrub_masks_every_occurrence_longest_first(): + secrets = run_secrets(None, None, {"a": "hunter2", "b": "hunter2-extended"}, None) + line = 'fatal: [h]: FAILED! => {"msg": "hunter2-extended / hunter2"}' + assert scrub(line, secrets) == f'fatal: [h]: FAILED! => {{"msg": "{MASK} / {MASK}"}}' + + +def test_scrub_leaves_clean_lines_alone(): + assert scrub("ok: [web1]", ["agent-token-abc"]) == "ok: [web1]" + + +def test_bundled_tasks_that_template_the_agent_token_are_no_log(): + """Every bundled task whose args mention steward_token sets no_log + (assert-style checks excepted: they print a fixed fail_msg).""" + import pathlib + import yaml + import steward.ansible + + def missing(plays) -> list[str]: + out = [] + for play in plays or []: + for task in (play.get("tasks") or []) + (play.get("handlers") or []): + body = {k: v for k, v in task.items() if k not in ("name", "no_log")} + if "ansible.builtin.assert" in body or "assert" in body: + continue + if "steward_token" in yaml.safe_dump(body) and not task.get("no_log"): + out.append(task.get("name", "?")) + return out + + # The check can fail. + assert missing([{"tasks": [{"name": "leaky", "copy": {"content": "{{ steward_token }}"}}]}]) == ["leaky"] + + bundled = pathlib.Path(steward.ansible.__file__).parent / "bundled" + files = sorted(bundled.rglob("*.yml")) + assert files + leaks = [f"{f.name}: {t}" for f in files for t in missing(yaml.safe_load(f.read_text()))] + assert not leaks, leaks