diff --git a/README.md b/README.md index 817df3d..21e6492 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ This project is designed to make **ComfyUI a reliable automation target** with a - Runtime profile startup hardening is fail-closed in hardened mode - Connector ingress is fail-closed in public/hardened posture when platform allowlists are missing, with synchronized startup gate, deployment-profile check, Security Doctor posture, and startup audit visibility - Admin write actions are protected by an explicit **Admin Token** boundary +- Optional local secret-manager integration supports 1Password CLI with explicit enable + command allowlist fail-closed controls while keeping frontend surfaces secret-blind - Webhook ingress is **deny-by-default** until authentication is configured - Encrypted webhook ingress is **fail-closed** on signature/decrypt/app-id validation failures - Bridge worker ingress enforces device token auth, scope checks, and idempotency handling @@ -91,6 +92,17 @@ Deployment profiles and hardening checklists:
+Optional local secret-manager baseline for safer key sourcing + +- Added a pluggable backend secret-provider chain for API keys (`env -> optional 1Password CLI -> encrypted server store -> none`) so operators can keep runtime keys out of plaintext deployment config where needed. +- Added fail-closed 1Password guardrails requiring explicit enablement, executable allowlist, command path validation, and bounded lookup timeout behavior. +- Added regression coverage for precedence resolution, allowlist/failure fallback behavior, and no-secret-leak logging expectations. +- Completed full verification gate pass on `dev` (detect-secrets, pre-commit, backend unit suites, adversarial/retry/real-backend lanes, and frontend Playwright E2E). + +
+ +
+ Today’s implementation roundup across frontend quality, planner contracts, and connector security baselines - Completed the frontend quality bundle by stabilizing canonical style ownership, adding baseline frontend unit coverage, and expanding regression coverage for Library/Approvals/admin-console parity. diff --git a/docs/adr/ADR-0001-config-surface-unification.md b/docs/adr/ADR-0001-config-surface-unification.md index b995d05..c3ae957 100644 --- a/docs/adr/ADR-0001-config-surface-unification.md +++ b/docs/adr/ADR-0001-config-surface-unification.md @@ -55,4 +55,3 @@ Phase 2+ (future follow-ups): - Rejected due to blast radius and rollback difficulty. 2. Keep dual systems and patch ad hoc: - Rejected due to ongoing precedence drift and maintenance cost. - diff --git a/docs/release/config_secrets_contract.md b/docs/release/config_secrets_contract.md index a62b7f7..4a5d05b 100644 --- a/docs/release/config_secrets_contract.md +++ b/docs/release/config_secrets_contract.md @@ -1,8 +1,8 @@ # OpenClaw Config & Secrets Contract (v1) > **Status**: normative -> **Version**: 1.0.3 -> **Date**: 2026-03-01 +> **Version**: 1.0.4 +> **Date**: 2026-03-07 This document defines the authoritative configuration contract for OpenClaw. It enumerates all supported environment variables, their precedence rules, and security classifications. @@ -31,6 +31,24 @@ Controls the core LLM client used by nodes (Planner, Refiner, etc.). | `OPENCLAW_LLM_BASE_URL` | No | Provider default | Override base URL (crucial for local/compatible providers). | | `OPENCLAW_LLM_TIMEOUT`| No | `120` | Request timeout in seconds. | +Optional local secret-manager path (S11, disabled by default): + +| Variable | Required | Default | Description | +| :--- | :--- | :--- | :--- | +| `OPENCLAW_1PASSWORD_ENABLED` | No | `0` | Enables optional 1Password CLI provider for API key resolution. | +| `OPENCLAW_1PASSWORD_ALLOWED_COMMANDS` | **Yes (when enabled)** | - | Comma-separated executable allowlist. Empty allowlist with enabled provider is fail-closed. | +| `OPENCLAW_1PASSWORD_CMD` | No | `op` | 1Password CLI executable name/path. Must match allowlist entry. | +| `OPENCLAW_1PASSWORD_VAULT` | **Yes (when enabled)** | - | Vault name used to resolve secret references. | +| `OPENCLAW_1PASSWORD_ITEM_TEMPLATE` | No | `openclaw/{provider}` | Item path template. Must contain `{provider}` placeholder. | +| `OPENCLAW_1PASSWORD_FIELD` | No | `api_key` | Item field name containing the key value. | +| `OPENCLAW_1PASSWORD_TIMEOUT_SEC` | No | `5` | CLI lookup timeout (bounded, fail-closed on timeout). | + +Lookup precedence for provider keys: +1. Provider-specific env key (`OPENCLAW_*`, legacy `MOLTBOT_*`) +2. Generic env key (`OPENCLAW_LLM_API_KEY`, legacy aliases) +3. Optional 1Password provider (if enabled and allowlist-valid) +4. Encrypted server-side secret store (`secrets.enc.json`) + **SSRF Protection:** | Variable | Default | Description | diff --git a/docs/release/feature_flags.md b/docs/release/feature_flags.md index 54ff17f..f687f18 100644 --- a/docs/release/feature_flags.md +++ b/docs/release/feature_flags.md @@ -23,6 +23,7 @@ Users should audit these flags before deploying to a public or untrusted network | `OPENCLAW_BRIDGE_ENABLED` | `0` | **High** | Enables the sidecar bridge for remote orchestration. Requires `OPENCLAW_BRIDGE_DEVICE_TOKEN` (and in public posture also mTLS + device allowlist controls). | | `OPENCLAW_ALLOW_ANY_PUBLIC_LLM_HOST` | `0` | **High** | Bypasses the known-host allowlist for LLM `base_url`. Allows SSRF to public IPs. | | `OPENCLAW_ALLOW_INSECURE_BASE_URL` | `0` | **Critical** | Allows HTTP (non-HTTPS) or private IP `base_url` for LLM. Risk of internal network scanning (SSRF). | +| `OPENCLAW_1PASSWORD_ENABLED` | `0` | **Medium** | Enables optional local 1Password CLI key lookup. Requires explicit command allowlist (`OPENCLAW_1PASSWORD_ALLOWED_COMMANDS`) and vault config; fail-closed when misconfigured. | | `OPENCLAW_LOG_TRUNCATE_ON_START` | `0` | **Low** | Operational log hygiene toggle. If `1`, truncates active `openclaw.log` at startup (once per process). | | `OPENCLAW_CONNECTOR_DISCORD_TOKEN` | *None* | **Medium** | Presence enables Discord Bot gateway. | | `OPENCLAW_CONNECTOR_LINE_CHANNEL_SECRET` | *None* | **Medium** | Presence enables LINE webhook listener. Requires a public HTTPS endpoint. | diff --git a/docs/security_key_lifecycle_sop.md b/docs/security_key_lifecycle_sop.md index 370cf5a..c36a797 100644 --- a/docs/security_key_lifecycle_sop.md +++ b/docs/security_key_lifecycle_sop.md @@ -8,6 +8,9 @@ This SOP defines operational procedures for three security-critical materials: Use this runbook for routine rotation, emergency revocation, and disaster recovery. +Scope note: +- If `S11` optional 1Password provider is enabled, provider API keys can be sourced from local 1Password instead of `secrets.enc.json`. In that mode, this SOP still governs the local encryption key lifecycle for any secrets that remain in server-side store. + ## 1. State Artifacts All paths are relative to `OPENCLAW_STATE_DIR` (legacy fallback: `MOLTBOT_STATE_DIR`): @@ -22,6 +25,12 @@ All paths are relative to `OPENCLAW_STATE_DIR` (legacy fallback: `MOLTBOT_STATE_ Optional startup log hygiene for incident drills: - Set `OPENCLAW_LOG_TRUNCATE_ON_START=1` before restart when you need a clean `openclaw.log` timeline. +Optional `S11` local secret-manager settings (if used): +- `OPENCLAW_1PASSWORD_ENABLED=1` +- `OPENCLAW_1PASSWORD_ALLOWED_COMMANDS=` +- `OPENCLAW_1PASSWORD_CMD=op` +- `OPENCLAW_1PASSWORD_VAULT=` + ## 2. Global Rules 1. Always take a timestamped backup before any lifecycle operation. diff --git a/services/config_layers.py b/services/config_layers.py index acd74d1..bc54283 100644 --- a/services/config_layers.py +++ b/services/config_layers.py @@ -154,4 +154,3 @@ def resolve_layered_config( sources[key] = source return effective, sources - diff --git a/services/providers/keys.py b/services/providers/keys.py index 1bd3a06..ea6318d 100644 --- a/services/providers/keys.py +++ b/services/providers/keys.py @@ -1,12 +1,13 @@ """ LLM Provider API Key Management. -R16: Key lookup policy per provider with env var precedence. +R16/S11: Key lookup policy per provider with pluggable secret-provider chain. """ import logging import os from typing import Optional +from ..secret_providers import resolve_provider_secret from .catalog import PROVIDER_CATALOG, get_provider_info logger = logging.getLogger("ComfyUI-OpenClaw.services.providers.keys") @@ -23,53 +24,16 @@ def get_api_key_for_provider(provider: str) -> Optional[str]: """ Get API key for a specific provider. - Precedence (S25): + Precedence (S11/S25): 1. Provider-specific env var (preferred: OPENCLAW_*; legacy: MOLTBOT_*) - 2. Generic key (OPENCLAW_LLM_API_KEY, MOLTBOT_LLM_API_KEY, CLAWDBOT_LLM_API_KEY) - 3. Server secret store (server-side persistence) + 2. Generic env key (OPENCLAW_LLM_API_KEY, MOLTBOT_LLM_API_KEY, CLAWDBOT_LLM_API_KEY) + 3. Optional 1Password CLI provider (explicit opt-in + allowlisted command) + 4. Server secret store (encrypted server-side persistence) Returns None if no key found (acceptable for local providers). """ - provider_info = get_provider_info(provider) - - # Try provider-specific key first - if provider_info and provider_info.env_key_name: - candidates = [] - if provider_info.env_key_name.startswith("MOLTBOT_"): - candidates.append( - provider_info.env_key_name.replace("MOLTBOT_", "OPENCLAW_", 1) - ) - candidates.append(provider_info.env_key_name) - for env_name in candidates: - key = os.environ.get(env_name) - if key: - return key - - # Fall back to generic keys - for env_name in GENERIC_KEY_NAMES: - key = os.environ.get(env_name) - if key: - return key - - # S25: Fall back to server secret store (best-effort; empty store = no effect). - try: - from ..secret_store import get_secret_store - - store = get_secret_store() - - # Try provider-specific secret - key = store.get_secret(provider) - if key: - return key - - # Try generic secret - key = store.get_secret("generic") - if key: - return key - except Exception as e: - logger.debug(f"S25: Failed to check secret store (non-fatal): {e}") - - return None + key, _source = resolve_provider_secret(provider) + return key def requires_api_key(provider: str) -> bool: @@ -100,7 +64,7 @@ def get_all_configured_keys() -> dict: """ result = {} - # Check server secret store (best-effort) + # Check server secret store status (best-effort) store_status = {} try: from ..secret_store import get_secret_store @@ -112,19 +76,45 @@ def get_all_configured_keys() -> dict: for provider_id, info in PROVIDER_CATALOG.items(): if info.env_key_name: - key = os.environ.get(info.env_key_name) + key = None + provider_candidates = [] + if info.env_key_name.startswith("MOLTBOT_"): + provider_candidates.append( + info.env_key_name.replace("MOLTBOT_", "OPENCLAW_", 1) + ) + provider_candidates.append(info.env_key_name) - # Determine source source = None - if key: - source = "env" - elif provider_id in store_status: + for env_name in provider_candidates: + value = os.environ.get(env_name) + if value: + key = value + source = "env" + break + + if key is None: + for env_name in GENERIC_KEY_NAMES: + value = os.environ.get(env_name) + if value: + key = value + source = "env" + break + + if key is None and ( + provider_id in store_status or "generic" in store_status + ): source = "server_store" + if key is None and source is None: + resolved, resolved_source = resolve_provider_secret(provider_id) + if resolved: + key = resolved + source = resolved_source + result[provider_id] = { "env_var": info.env_key_name, - "configured": key is not None or provider_id in store_status, - "masked": mask_api_key(key) if key else None, + "configured": key is not None or source is not None, + "masked": mask_api_key(key) if key and source == "env" else None, "source": source, } else: @@ -135,13 +125,29 @@ def get_all_configured_keys() -> dict: "source": "local", } - # Add generic secret if stored - if "generic" in store_status: + # Add generic key status for diagnostics. + generic_key = None + generic_source = None + for env_name in GENERIC_KEY_NAMES: + value = os.environ.get(env_name) + if value: + generic_key = value + generic_source = "env" + break + if generic_key is None and "generic" in store_status: + generic_source = "server_store" + if generic_key is None and generic_source is None: + resolved, resolved_source = resolve_provider_secret("generic") + if resolved: + generic_key = resolved + generic_source = resolved_source + + if generic_source is not None: result["generic"] = { "env_var": "OPENCLAW_LLM_API_KEY", "configured": True, - "masked": None, # Never expose stored secrets - "source": "server_store", + "masked": mask_api_key(generic_key) if generic_source == "env" else None, + "source": generic_source, } return result diff --git a/services/secret_providers.py b/services/secret_providers.py new file mode 100644 index 0000000..be07bb9 --- /dev/null +++ b/services/secret_providers.py @@ -0,0 +1,301 @@ +""" +S11 secret provider chain. + +Provides deterministic API-key resolution via pluggable providers: +- env +- optional 1Password CLI +- encrypted server-side secret store +- default none +""" + +from __future__ import annotations + +import logging +import os +import re +import subprocess +from pathlib import Path +from typing import Optional, Protocol + +try: + from .config_layers import GENERIC_LLM_API_KEY_ENV_KEYS, get_preferred_env_value +except ImportError: + from services.config_layers import ( # type: ignore + GENERIC_LLM_API_KEY_ENV_KEYS, + get_preferred_env_value, + ) + +try: + from .providers.catalog import get_provider_info +except ImportError: + from services.providers.catalog import get_provider_info # type: ignore + +logger = logging.getLogger("ComfyUI-OpenClaw.services.secret_providers") + +_TRUTHY = {"1", "true", "yes", "on"} +_PROVIDER_ID_RE = re.compile(r"^[a-z0-9][a-z0-9._-]{0,63}$") +_VAULT_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$") +_FIELD_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$") +_ITEM_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]{0,127}$") + + +def _is_truthy(value: Optional[str]) -> bool: + if value is None: + return False + return value.strip().lower() in _TRUTHY + + +def _env_value( + primary: str, legacy: str, default: Optional[str] = None +) -> Optional[str]: + value, _used_legacy = get_preferred_env_value(primary, legacy) + if value is None: + return default + return value + + +class SecretProvider(Protocol): + source: str + + def get_secret(self, provider: str) -> Optional[str]: ... + + +class EnvSecretProvider: + source = "env" + + def _provider_env_candidates(self, provider: str) -> list[str]: + info = get_provider_info(provider) + if not info or not info.env_key_name: + return [] + + candidates: list[str] = [] + if info.env_key_name.startswith("MOLTBOT_"): + candidates.append(info.env_key_name.replace("MOLTBOT_", "OPENCLAW_", 1)) + candidates.append(info.env_key_name) + return candidates + + def get_secret(self, provider: str) -> Optional[str]: + # Provider-specific first + for env_name in self._provider_env_candidates(provider): + value = os.environ.get(env_name) + if value: + return value + + # Generic fallback + for env_name in GENERIC_LLM_API_KEY_ENV_KEYS: + value = os.environ.get(env_name) + if value: + return value + return None + + +class OnePasswordSecretProvider: + source = "onepassword" + + def _enabled(self) -> bool: + return _is_truthy( + _env_value("OPENCLAW_1PASSWORD_ENABLED", "MOLTBOT_1PASSWORD_ENABLED", "0") + ) + + def _command(self) -> str: + command = _env_value("OPENCLAW_1PASSWORD_CMD", "MOLTBOT_1PASSWORD_CMD", "op") + return str(command or "").strip() + + def _allowed_commands(self) -> set[str]: + raw = _env_value( + "OPENCLAW_1PASSWORD_ALLOWED_COMMANDS", + "MOLTBOT_1PASSWORD_ALLOWED_COMMANDS", + "", + ) + return { + token.strip().lower() + for token in str(raw or "").split(",") + if token and token.strip() + } + + def _vault(self) -> str: + return str( + _env_value("OPENCLAW_1PASSWORD_VAULT", "MOLTBOT_1PASSWORD_VAULT", "") or "" + ).strip() + + def _item_template(self) -> str: + return str( + _env_value( + "OPENCLAW_1PASSWORD_ITEM_TEMPLATE", + "MOLTBOT_1PASSWORD_ITEM_TEMPLATE", + "openclaw/{provider}", + ) + or "" + ).strip() + + def _field(self) -> str: + return str( + _env_value("OPENCLAW_1PASSWORD_FIELD", "MOLTBOT_1PASSWORD_FIELD", "api_key") + or "" + ).strip() + + def _timeout_sec(self) -> float: + raw = _env_value( + "OPENCLAW_1PASSWORD_TIMEOUT_SEC", "MOLTBOT_1PASSWORD_TIMEOUT_SEC", "5" + ) + try: + value = float(raw or "5") + except (TypeError, ValueError): + value = 5.0 + return max(1.0, min(value, 30.0)) + + def is_available(self) -> bool: + if not self._enabled(): + return False + + allowed = self._allowed_commands() + if not allowed: + # CRITICAL: explicit allowlist is required when 1Password provider is enabled. + logger.warning( + "S11: 1Password enabled but OPENCLAW_1PASSWORD_ALLOWED_COMMANDS is empty; fail-closed." + ) + return False + + command = self._command() + if not command: + logger.warning("S11: 1Password command is empty; fail-closed.") + return False + command_name = Path(command).name.lower() + if command_name not in allowed: + logger.warning( + "S11: 1Password command '%s' not in allowlist; fail-closed.", + command_name, + ) + return False + + vault = self._vault() + field = self._field() + template = self._item_template() + if not vault or not _VAULT_RE.fullmatch(vault): + logger.warning("S11: 1Password vault is missing/invalid; fail-closed.") + return False + if not field or not _FIELD_RE.fullmatch(field): + logger.warning("S11: 1Password field is invalid; fail-closed.") + return False + if "{provider}" not in template: + logger.warning( + "S11: 1Password item template must include '{provider}'; fail-closed." + ) + return False + return True + + def _build_ref(self, provider: str) -> Optional[str]: + if not _PROVIDER_ID_RE.fullmatch(provider): + logger.warning( + "S11: Invalid provider id for 1Password lookup; fail-closed." + ) + return None + template = self._item_template() + item = template.format(provider=provider) + if not item or not _ITEM_RE.fullmatch(item) or ".." in item: + logger.warning("S11: 1Password item name is invalid; fail-closed.") + return None + return f"op://{self._vault()}/{item}/{self._field()}" + + def _read_ref(self, ref: str, provider: str) -> Optional[str]: + command = self._command() + args = [command, "read", ref] + try: + completed = subprocess.run( + args, + capture_output=True, + text=True, + shell=False, + timeout=self._timeout_sec(), + check=False, + ) + except FileNotFoundError: + logger.warning("S11: 1Password command not found; fail-closed.") + return None + except subprocess.TimeoutExpired: + logger.warning( + "S11: 1Password lookup timed out for provider '%s'; fail-closed.", + provider, + ) + return None + except Exception as exc: + logger.warning( + "S11: 1Password lookup error for provider '%s' (%s); fail-closed.", + provider, + type(exc).__name__, + ) + return None + + if completed.returncode != 0: + logger.warning( + "S11: 1Password lookup failed for provider '%s' (exit=%s); fail-closed.", + provider, + completed.returncode, + ) + return None + + value = (completed.stdout or "").strip() + return value or None + + def get_secret(self, provider: str) -> Optional[str]: + if not self.is_available(): + return None + + provider_ref = self._build_ref(provider) + if provider_ref: + value = self._read_ref(provider_ref, provider) + if value: + return value + + if provider != "generic": + generic_ref = self._build_ref("generic") + if generic_ref: + return self._read_ref(generic_ref, "generic") + return None + + +class ServerStoreSecretProvider: + source = "server_store" + + def get_secret(self, provider: str) -> Optional[str]: + try: + from .secret_store import get_secret_store + except ImportError: + from services.secret_store import get_secret_store # type: ignore + + try: + store = get_secret_store() + value = store.get_secret(provider) + if value: + return value + if provider != "generic": + return store.get_secret("generic") + except Exception as exc: + logger.debug( + "S25/S11: secret store lookup failed (non-fatal): %s", + exc, + ) + return None + + +def get_secret_providers() -> list[SecretProvider]: + """Return key-resolution providers in deterministic order.""" + return [ + EnvSecretProvider(), + OnePasswordSecretProvider(), + ServerStoreSecretProvider(), + ] + + +def resolve_provider_secret(provider: str) -> tuple[Optional[str], Optional[str]]: + """ + Resolve a provider secret through the configured provider chain. + + Returns: + (secret, source) where source in {"env","onepassword","server_store"} or None. + """ + for resolver in get_secret_providers(): + secret = resolver.get_secret(provider) + if secret: + return secret, resolver.source + return None, None diff --git a/tests/test_r139_config_layers.py b/tests/test_r139_config_layers.py index fda17fa..79eb611 100644 --- a/tests/test_r139_config_layers.py +++ b/tests/test_r139_config_layers.py @@ -24,7 +24,10 @@ class TestR139ConfigLayers(unittest.TestCase): value, used_legacy = get_preferred_env_value( "OPENCLAW_LLM_PROVIDER", "MOLTBOT_LLM_PROVIDER", - env={"OPENCLAW_LLM_PROVIDER": "openai", "MOLTBOT_LLM_PROVIDER": "anthropic"}, + env={ + "OPENCLAW_LLM_PROVIDER": "openai", + "MOLTBOT_LLM_PROVIDER": "anthropic", + }, ) self.assertEqual(value, "openai") self.assertFalse(used_legacy) @@ -106,4 +109,3 @@ class TestR139ConfigLayers(unittest.TestCase): if __name__ == "__main__": unittest.main() - diff --git a/tests/test_s11_secret_providers.py b/tests/test_s11_secret_providers.py new file mode 100644 index 0000000..3e24349 --- /dev/null +++ b/tests/test_s11_secret_providers.py @@ -0,0 +1,136 @@ +""" +S11 secret-provider chain tests. +""" + +import os +import shutil +import tempfile +import unittest +from unittest.mock import patch + +from services.providers.keys import get_all_configured_keys, get_api_key_for_provider +from services.secret_store import get_secret_store + + +class TestS11SecretProviderChain(unittest.TestCase): + def setUp(self): + self._orig_env = dict(os.environ) + self._tmpdir = tempfile.mkdtemp() + # Keep environment deterministic per test. + for key in list(os.environ.keys()): + if key.startswith("OPENCLAW_") or key.startswith("MOLTBOT_"): + del os.environ[key] + get_secret_store(state_dir=self._tmpdir).clear_all() + + def tearDown(self): + os.environ.clear() + os.environ.update(self._orig_env) + shutil.rmtree(self._tmpdir, ignore_errors=True) + + def test_env_precedence_over_onepassword_and_store(self): + os.environ["OPENCLAW_OPENAI_API_KEY"] = "sk-env-priority" + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "vlt" + + store = get_secret_store(state_dir=self._tmpdir) + store.set_secret("openai", "sk-store") + + with patch("services.secret_providers.subprocess.run") as mock_run: + key = get_api_key_for_provider("openai") + + self.assertEqual(key, "sk-env-priority") + mock_run.assert_not_called() + + def test_onepassword_lookup_success(self): + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "teamvault" + + with patch("services.secret_providers.subprocess.run") as mock_run: + mock_run.return_value = type( + "R", + (), + {"returncode": 0, "stdout": "sk-op-123\n", "stderr": ""}, + )() + key = get_api_key_for_provider("openai") + + self.assertEqual(key, "sk-op-123") + args = mock_run.call_args.args[0] + self.assertEqual(args[:2], ["op", "read"]) + self.assertIn("op://teamvault/openclaw/openai/api_key", args) + + def test_onepassword_allowlist_fail_closed_falls_back_to_store(self): + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "safeop" + os.environ["OPENCLAW_1PASSWORD_CMD"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "teamvault" + + store = get_secret_store(state_dir=self._tmpdir) + store.set_secret("openai", "sk-store-only") + + with patch("services.secret_providers.subprocess.run") as mock_run: + key = get_api_key_for_provider("openai") + + self.assertEqual(key, "sk-store-only") + mock_run.assert_not_called() + + def test_onepassword_failure_falls_back_to_generic_store(self): + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "teamvault" + + store = get_secret_store(state_dir=self._tmpdir) + store.set_secret("generic", "sk-generic-store") + + with patch("services.secret_providers.subprocess.run") as mock_run: + mock_run.return_value = type( + "R", + (), + {"returncode": 1, "stdout": "", "stderr": "unauthorized"}, + )() + key = get_api_key_for_provider("openai") + + self.assertEqual(key, "sk-generic-store") + self.assertGreaterEqual(mock_run.call_count, 1) + + def test_onepassword_error_log_does_not_leak_secret_value(self): + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "teamvault" + + leaked = "sk-should-not-appear" + with patch("services.secret_providers.subprocess.run") as mock_run: + mock_run.return_value = type( + "R", + (), + {"returncode": 1, "stdout": "", "stderr": f"bad key {leaked}"}, + )() + with self.assertLogs( + "ComfyUI-OpenClaw.services.secret_providers", level="WARNING" + ) as logs: + key = get_api_key_for_provider("openai") + + self.assertIsNone(key) + self.assertNotIn(leaked, "\n".join(logs.output)) + + def test_configured_keys_reports_onepassword_source_without_value(self): + os.environ["OPENCLAW_1PASSWORD_ENABLED"] = "1" + os.environ["OPENCLAW_1PASSWORD_ALLOWED_COMMANDS"] = "op" + os.environ["OPENCLAW_1PASSWORD_VAULT"] = "teamvault" + + def _resolver(*_args, **_kwargs): + return type( + "R", (), {"returncode": 0, "stdout": "sk-op-xyz", "stderr": ""} + )() + + with patch("services.secret_providers.subprocess.run", side_effect=_resolver): + info = get_all_configured_keys() + + self.assertTrue(info["openai"]["configured"]) + self.assertEqual(info["openai"]["source"], "onepassword") + self.assertIsNone(info["openai"]["masked"]) + + +if __name__ == "__main__": + unittest.main()