diff --git a/backend/src/cyclone/secrets.py b/backend/src/cyclone/secrets.py index bec83c4..f09cefc 100644 --- a/backend/src/cyclone/secrets.py +++ b/backend/src/cyclone/secrets.py @@ -14,11 +14,26 @@ Setup (one-time, by the operator): Verification: security find-generic-password -s cyclone -a sftp.gainwell.password -w + +SP25: the lookup now resolves the secret in three tiers, in this order: + + 1. Plain env var named exactly ```` — highest priority. + Lets a Linux server or Docker container pass the MFT password + via ``CYCLONE_SFTP_PASSWORD=...`` without touching the Keychain. + Trailing/leading whitespace (including the ``\\n`` that .env + files often leave at EOF) is stripped; an empty value is + treated as absent. + 2. macOS Keychain via ``keyring`` — unchanged from SP9. Kept as a + fallback so a macOS workstation that prefers + ``security add-generic-password`` works without env-var exports. + 3. ``None`` — caller decides what to do (``SftpClient._connect`` + raises a precise ``RuntimeError`` on real-mode auth). """ from __future__ import annotations import logging +import os from typing import Optional log = logging.getLogger(__name__) @@ -37,23 +52,44 @@ except ImportError: def get_secret(name: str) -> Optional[str]: - """Fetch a secret from macOS Keychain by name. + """Fetch a secret by name. Three-tier lookup: env var, Keychain, None. Args: - name: The Keychain account (e.g. "sftp.gainwell.password"). + name: The secret name. Matches both the env-var name and the + Keychain account (e.g. ``"sftp.gainwell.password"`` / + ``CYCLONE_SFTP_PASSWORD`` would NOT match — the env var + name is the bare secret name, not the + ``CYCLONE_`` form). Operators who want + the env-var shortcut set the var named exactly ````. + + For the Gainwell MFT password, the spec uses + ``CYCLONE_SFTP_PASSWORD`` as the env var and the + ``sftp.gainwell.password`` Keychain account as the + fallback. If we want them to match by name (one source of + truth), we map via the ``_ENV_NAME_FOR`` table at the + bottom of this module. Returns: - The secret string, or None if the entry is missing or keyring - is not installed. + The secret string (stripped), or ``None`` if no tier produced + a value. """ - if not _HAS_KEYRING: - log.warning("keyring not installed; get_secret(%r) returning None", name) - return None - try: - return keyring.get_password(SERVICE_NAME, name) - except Exception as exc: # noqa: BLE001 (Keychain can raise anything) - log.warning("Keychain get_secret(%r) failed: %s", name, exc) - return None + env_name = _ENV_NAME_FOR.get(name, name) + raw = os.environ.get(env_name) + if raw is not None: + stripped = raw.strip() + if stripped: + log.debug("Secret %r resolved from env var %r", name, env_name) + return stripped + # Empty env var — treat as absent and fall through. + + if _HAS_KEYRING: + try: + value = keyring.get_password(SERVICE_NAME, name) + if value is not None: + return value + except Exception as exc: # noqa: BLE001 (Keychain can raise anything) + log.warning("Keychain get_secret(%r) failed: %s", name, exc) + return None def set_secret(name: str, value: str) -> bool: @@ -77,3 +113,13 @@ def has_keyring() -> bool: """True if the ``keyring`` library is importable (regardless of whether the Keychain entry actually exists).""" return _HAS_KEYRING + + +# Mapping from Keychain account name (used in ``SftpBlock.auth``) to +# the operator-facing env var. Keeping this table here means callers +# don't have to remember the difference between +# ``sftp.gainwell.password`` (Keychain account) and +# ``CYCLONE_SFTP_PASSWORD`` (env var). +_ENV_NAME_FOR: dict[str, str] = { + "sftp.gainwell.password": "CYCLONE_SFTP_PASSWORD", +} diff --git a/backend/tests/test_secrets_envvar.py b/backend/tests/test_secrets_envvar.py new file mode 100644 index 0000000..82b016e --- /dev/null +++ b/backend/tests/test_secrets_envvar.py @@ -0,0 +1,100 @@ +"""SP25 — env-var fallback in cyclone.secrets.get_secret(). + +The scheduler's ``SftpClient._connect`` calls ``get_secret(name)`` to +fetch the MFT password. Today the lookup is macOS-Keychain-only via +the ``keyring`` library. SP25 adds a plain env-var tier in front of +the Keychain so the same code path serves a Linux server or Docker +container with no platform-specific glue. +""" +from __future__ import annotations + +import importlib + +import pytest + + +@pytest.fixture +def secrets_module(monkeypatch): + """Reload cyclone.secrets with a clean keyring state per test.""" + # Force a fresh import so module-level keyring cache is clean. + import cyclone.secrets as secrets_mod + importlib.reload(secrets_mod) + yield secrets_mod + importlib.reload(secrets_mod) + + +def test_env_var_wins_over_keychain(monkeypatch, secrets_module): + """Env var present AND Keychain has an entry → env var returned.""" + monkeypatch.setenv("cyclone.test.password", "from-env") + monkeypatch.setattr( + secrets_module.keyring, "get_password", + lambda service, name: "from-keychain", + ) + assert secrets_module.get_secret("cyclone.test.password") == "from-env" + + +def test_env_var_strips_trailing_newline(monkeypatch, secrets_module): + """A trailing newline (common in .env files) is stripped.""" + monkeypatch.setenv("cyclone.test.password", "s3cret\n") + assert secrets_module.get_secret("cyclone.test.password") == "s3cret" + + +def test_env_var_strips_leading_and_trailing_whitespace(monkeypatch, secrets_module): + monkeypatch.setenv("cyclone.test.password", " s3cret \n") + assert secrets_module.get_secret("cyclone.test.password") == "s3cret" + + +def test_env_var_set_keychain_absent(monkeypatch, secrets_module): + monkeypatch.setenv("cyclone.test.password", "from-env") + monkeypatch.setattr( + secrets_module.keyring, "get_password", lambda service, name: None, + ) + assert secrets_module.get_secret("cyclone.test.password") == "from-env" + + +def test_env_var_absent_keychain_present(monkeypatch, secrets_module): + monkeypatch.delenv("cyclone.test.password", raising=False) + monkeypatch.setattr( + secrets_module.keyring, "get_password", + lambda service, name: "from-keychain", + ) + assert secrets_module.get_secret("cyclone.test.password") == "from-keychain" + + +def test_both_absent_returns_none(monkeypatch, secrets_module): + monkeypatch.delenv("cyclone.test.password", raising=False) + monkeypatch.setattr( + secrets_module.keyring, "get_password", lambda service, name: None, + ) + assert secrets_module.get_secret("cyclone.test.password") is None + + +def test_keyring_library_missing_falls_through_to_none(monkeypatch, secrets_module): + """If keyring is not installed at all, get_secret still returns None + instead of raising ImportError.""" + monkeypatch.setenv("cyclone.test.password", "from-env") + monkeypatch.setattr(secrets_module, "_HAS_KEYRING", False) + # Even if keyring is missing, the env-var tier still wins. + assert secrets_module.get_secret("cyclone.test.password") == "from-env" + + +def test_empty_env_var_treated_as_absent(monkeypatch, secrets_module): + """An env var set to '' should not propagate — fall through to Keychain/None.""" + monkeypatch.setenv("cyclone.test.password", "") + monkeypatch.setattr( + secrets_module.keyring, "get_password", + lambda service, name: "from-keychain", + ) + assert secrets_module.get_secret("cyclone.test.password") == "from-keychain" + + +def test_gainwell_env_var_mapping(monkeypatch, secrets_module): + """The Keychain account ``sftp.gainwell.password`` maps to the env + var ``CYCLONE_SFTP_PASSWORD`` via the internal name table. Without + this, an operator setting ``CYCLONE_SFTP_PASSWORD`` on a Linux box + would not get the MFT password.""" + monkeypatch.setenv("CYCLONE_SFTP_PASSWORD", "real-mft-password") + monkeypatch.setattr( + secrets_module.keyring, "get_password", lambda service, name: None, + ) + assert secrets_module.get_secret("sftp.gainwell.password") == "real-mft-password"