diff --git a/steward/app.py b/steward/app.py index 6551942..630f926 100644 --- a/steward/app.py +++ b/steward/app.py @@ -155,6 +155,9 @@ def create_app( for _name in ("metric_label", "metric_about", "metric_unit", "metric_value", "source_label", "cert_left", "field_label"): app.jinja_env.globals[_name] = getattr(_fmt, _name) + # Which plugin settings fields are credentials (a password box, never echoed). + from .core.settings import is_plugin_secret as _is_plugin_secret + app.jinja_env.globals["is_plugin_secret"] = _is_plugin_secret from .alerts.service import rule_condition as _rule_condition app.jinja_env.globals["rule_condition"] = _rule_condition from .models.monitors import MONITOR_TYPE_LABELS as _type_labels diff --git a/steward/core/settings.py b/steward/core/settings.py index 3d4844d..5a90bc0 100644 --- a/steward/core/settings.py +++ b/steward/core/settings.py @@ -204,6 +204,11 @@ def _decode(value: Any, key: str = "") -> Any: exact setting that needs re-entering. """ from steward.core.crypto import decrypt_secret, is_encrypted + if key.startswith(PLUGIN_PREFIX) and isinstance(value, dict): + name = key[len(PLUGIN_PREFIX):] + return {f: decrypt_secret(v, context=plugin_secret_key(name, f)) + if is_plugin_secret(f) and is_encrypted(v) else v + for f, v in value.items()} return decrypt_secret(value, context=key) if is_encrypted(value) else value @@ -260,6 +265,38 @@ def source_token_key(name: str) -> str: return f"{SOURCES_KEY}[{name}].http_token" +# A plugin's settings are one dict under plugin.; a field whose name +# says it holds a credential is sealed inside it, field by field. The same +# test decides that the settings page shows it as a password box. +PLUGIN_PREFIX = "plugin." +_PLUGIN_SECRET_WORDS = ("password", "secret", "token") + + +def is_plugin_secret(field: str) -> bool: + """Whether a plugin config field holds a credential: unifi's password, + any *_secret or *_token.""" + return any(w in field for w in _PLUGIN_SECRET_WORDS) + + +def seal_plugin_secrets(cfg: Any) -> tuple[Any, int]: + """Encrypt each plaintext secret field of one plugin's settings dict; + returns (cfg, how many).""" + from steward.core.crypto import encrypt_secret, is_encrypted + if not isinstance(cfg, dict): + return cfg, 0 + sealed, count = dict(cfg), 0 + for field, value in cfg.items(): + if is_plugin_secret(field) and isinstance(value, str) and value and not is_encrypted(value): + sealed[field] = encrypt_secret(value) + count += 1 + return sealed, count + + +def plugin_secret_key(name: str, field: str) -> str: + """How a plugin's secret is named in logs and the undecryptable-secrets banner.""" + return f"{PLUGIN_PREFIX}{name}.{field}" + + # ───────────────────────────────────────────────────────────────────────────── # Async helpers (use inside request handlers / scheduled tasks) # ───────────────────────────────────────────────────────────────────────────── @@ -281,6 +318,8 @@ async def set_setting(session: AsyncSession, key: str, value: Any) -> None: if key in SECRET_KEYS and isinstance(value, str) and value: from steward.core.crypto import encrypt_secret to_store = encrypt_secret(value) + elif key.startswith(PLUGIN_PREFIX): + to_store, _ = seal_plugin_secrets(value) result = await session.execute( select(AppSetting).where(AppSetting.key == key) ) @@ -569,6 +608,12 @@ def migrate_plaintext_secrets(db_url: str) -> int: if sealed: row.value_json = json.dumps(sources) converted += sealed + for row in (await session.execute(select(AppSetting).where( + AppSetting.key.startswith(PLUGIN_PREFIX)))).scalars(): + cfg, sealed = seal_plugin_secrets(json.loads(row.value_json)) + if sealed: + row.value_json = json.dumps(cfg) + converted += sealed finally: await engine.dispose() return converted @@ -605,6 +650,13 @@ def scan_undecryptable_secrets(db_url: str) -> list[str]: key = source_token_key(src.get("name", "?")) if _is_undecryptable(src.get("http_token"), key): bad.append(key) + for row in (await session.execute(select(AppSetting).where( + AppSetting.key.startswith(PLUGIN_PREFIX)))).scalars(): + cfg = json.loads(row.value_json) + for field, value in (cfg.items() if isinstance(cfg, dict) else ()): + key = plugin_secret_key(row.key[len(PLUGIN_PREFIX):], field) + if is_plugin_secret(field) and _is_undecryptable(value, key): + bad.append(key) finally: await engine.dispose() return bad diff --git a/steward/settings/routes.py b/steward/settings/routes.py index 1862397..e9c0b70 100644 --- a/steward/settings/routes.py +++ b/steward/settings/routes.py @@ -15,6 +15,7 @@ from steward.core.settings import ( 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, seal_source_tokens, + is_plugin_secret, ) settings_bp = Blueprint("settings", __name__, url_prefix="/settings") @@ -830,6 +831,12 @@ async def save_plugin_detail(name: str): old_enabled = to_plugins_cfg(old_settings).get(name, {}).get("enabled", False) plugin_cfg = _build_plugin_cfg_from_form(plugin, form) + # A secret is never sent back to the browser, so its box arrives blank + # unless someone typed a new one: blank keeps the saved value. + old_cfg = to_plugins_cfg(old_settings).get(name, {}) + for field, value in list(plugin_cfg.items()): + if is_plugin_secret(field) and value == "" and old_cfg.get(field): + plugin_cfg[field] = old_cfg[field] async with current_app.db_sessionmaker() as db: async with db.begin(): diff --git a/steward/templates/settings/plugin_detail.html b/steward/templates/settings/plugin_detail.html index af6f346..5da3f37 100644 --- a/steward/templates/settings/plugin_detail.html +++ b/steward/templates/settings/plugin_detail.html @@ -108,10 +108,11 @@ {% else %}
- + value="{% if secret %}{% else %}{{ plugin._config.get(cfg_key, cfg_default) }}{% endif %}" + {% if secret %}autocomplete="new-password" placeholder="{{ 'kept; type to change' if plugin._config.get(cfg_key) else 'none saved' }}"{% endif %}>
{% endif %} diff --git a/tests/core/test_plugin_secrets.py b/tests/core/test_plugin_secrets.py new file mode 100644 index 0000000..4382bab --- /dev/null +++ b/tests/core/test_plugin_secrets.py @@ -0,0 +1,34 @@ +"""A plugin's credentials are sealed inside its settings dict (#5671): the +UniFi password was stored in plain text, so a backup would have carried it.""" +from steward.core.crypto import init_crypto, is_encrypted +from steward.core.settings import _decode, is_plugin_secret, seal_plugin_secrets + + +def setup_function(): + init_crypto("unit-test-secret-key") + + +def test_which_fields_are_credentials(): + assert is_plugin_secret("password") and is_plugin_secret("client_secret") + assert is_plugin_secret("api_token") + assert not is_plugin_secret("username") and not is_plugin_secret("host") + + +def test_sealing_touches_only_plaintext_secrets_and_reading_opens_them(): + cfg = {"enabled": True, "host": "https://10.0.0.1", "username": "admin", + "password": "hunter2", "poll_interval_seconds": 60} + sealed, n = seal_plugin_secrets(cfg) + assert n == 1 and is_encrypted(sealed["password"]) + assert {k: v for k, v in sealed.items() if k != "password"} == { + k: v for k, v in cfg.items() if k != "password"} + again, n2 = seal_plugin_secrets(sealed) # idempotent + assert n2 == 0 and again == sealed + assert _decode(sealed, "plugin.unifi") == cfg + assert seal_plugin_secrets({"password": ""}) == ({"password": ""}, 0) + assert seal_plugin_secrets("not a dict") == ("not a dict", 0) + + +def test_only_plugin_settings_are_opened(): + # A dict under a non-plugin key is left alone: only plugin.* is opened. + sealed, _ = seal_plugin_secrets({"password": "x"}) + assert _decode(sealed, "thresholds.cpu") == sealed diff --git a/tests/integration/test_plugin_secrets.py b/tests/integration/test_plugin_secrets.py new file mode 100644 index 0000000..5193820 --- /dev/null +++ b/tests/integration/test_plugin_secrets.py @@ -0,0 +1,91 @@ +"""Integration: a plugin's password is ciphertext in app_settings, reads back +as itself, is sealed at startup if it was stored in plain text, and is kept +when the settings form is saved with its box blank (#5671). Requires +STEWARD_DATABASE_URL.""" +from __future__ import annotations + +import asyncio +import json +import os +import time +import uuid + +import pytest + +pytestmark = pytest.mark.integration + + +@pytest.fixture +def app(): + if not os.environ.get("STEWARD_DATABASE_URL"): + pytest.skip("needs Postgres") + from steward.app import create_app + return create_app(testing=False) + + +def test_a_plugin_password_is_sealed_read_back_and_kept_on_a_blank_save(app): + from sqlalchemy import delete, select + + from steward.core.crypto import is_encrypted + from steward.core.settings import get_setting, migrate_plaintext_secrets, set_setting + from steward.models.settings import AppSetting + from steward.models.users import User, UserRole + + async def raw(key): + async with app.db_sessionmaker() as s: + row = (await s.execute(select(AppSetting).where(AppSetting.key == key))).scalar_one() + return json.loads(row.value_json) + + async def go(): + tag = uuid.uuid4().hex[:8] + cfg = {"enabled": False, "host": "https://10.9.9.1", "username": "admin", + "password": f"pw-{tag}", "site": "default", "verify_ssl": False, + "poll_interval_seconds": 60, "top_clients": 10} + async with app.db_sessionmaker() as s: + async with s.begin(): + await set_setting(s, "plugin.unifi", cfg) + stored = await raw("plugin.unifi") + assert is_encrypted(stored["password"]) and stored["username"] == "admin" + async with app.db_sessionmaker() as s: + assert (await get_setting(s, "plugin.unifi"))["password"] == f"pw-{tag}" + + # A row written before this fix, in plain text, is sealed at startup. + async with app.db_sessionmaker() as s: + async with s.begin(): + row = (await s.execute(select(AppSetting).where(AppSetting.key == "plugin.unifi"))).scalar_one() + row.value_json = json.dumps(cfg) + assert not is_encrypted((await raw("plugin.unifi"))["password"]) + await asyncio.to_thread(migrate_plaintext_secrets, app.config["DATABASE_URL"]) + assert is_encrypted((await raw("plugin.unifi"))["password"]) + + # Saving the settings page with the password box blank keeps it. + async with app.db_sessionmaker() as s: + async with s.begin(): + user = User(username=f"ps-{tag}", email=f"ps-{tag}@test", password_hash="x", + role=UserRole.admin, is_active=True) + s.add(user) + uid = user.id + client = app.test_client() + async with client.session_transaction() as sess: + sess.update(user_id=uid, user_role="admin", username=f"ps-{tag}", + _csrf="test-token", login_at=time.time()) + resp = await client.post("/settings/plugins/unifi/", form={ + "csrf_token": "test-token", "plugin.unifi.host": "https://10.9.9.1", + "plugin.unifi.username": "admin", "plugin.unifi.password": "", + "plugin.unifi.site": "default", "plugin.unifi.poll_interval_seconds": "120", + "plugin.unifi.top_clients": "10"}) + assert resp.status_code in (200, 302, 303), await resp.get_data(as_text=True) + async with app.db_sessionmaker() as s: + after = await get_setting(s, "plugin.unifi") + assert after["password"] == f"pw-{tag}" and after["poll_interval_seconds"] == 120 + assert is_encrypted((await raw("plugin.unifi"))["password"]) + + page = await (await client.get("/settings/plugins/unifi/")).get_data(as_text=True) + assert f"pw-{tag}" not in page and "kept; type to change" in page + + async with app.db_sessionmaker() as s: + async with s.begin(): + await s.execute(delete(AppSetting).where(AppSetting.key == "plugin.unifi")) + await s.execute(delete(User).where(User.id == uid)) + + asyncio.run(go())