From f42b76aa340025e7b567dee274f7663e84cae127 Mon Sep 17 00:00:00 2001 From: AhmadHammad21 Date: Sat, 30 May 2026 00:27:09 +0300 Subject: [PATCH] fix(security): distinguish unset vs empty cloud-accounts contextvar (fail-closed) The product had a cross-tenant credential leak: a registered org that hadn't connected an AWS account in Settings -> Cloud Accounts could still investigate AWS because the agent silently fell back to the platform's own AWS credentials. Root cause in the OSS resolver: current_cloud_accounts() collapsed None and {} into the same thing (`return _current_accounts.get() or {}`), so the resolver couldn't distinguish "OSS self-host, contextvar never set -> use ambient creds" (correct) from "product tenant, contextvar explicitly set to {} -> fail closed" (also needed). With this collapsed semantics, even if the product DID always call set_current_cloud_accounts([]), the resolver still returned _base_session(). Tri-state semantics on the per-task contextvar: - None -> OSS / self-host; resolve_session falls back to ambient host creds. - {} -> product tenant with no Cloud Account connected; resolve_session raises RuntimeError instead of leaking platform creds. - {p:a} -> per-provider resolution (existing behavior; unchanged). Changes: - providers/aws/credentials.py: * current_cloud_accounts() returns Optional[dict] directly (no collapsing). * account_for_provider / get_current_cloud_account tolerate None. * resolve_session() branches on None vs {} vs has-aws-key. - tools/bash_tool.py: same distinction at the credential injection block. None -> ambient (run_env=None); {} -> block any cloud binary; else -> existing. - tests/test_tools/test_credentials.py: three new tests pinning the behavior, plus a small fix to test_bash_uses_both_clouds_when_both_connected so it matches on basename (shutil.which resolves tokens[0] to an absolute path). --- .../tests/test_tools/test_credentials.py | 56 ++++++++++++++++++- .../providers/aws/credentials.py | 40 +++++++++---- .../src/opendevops_core/tools/bash_tool.py | 34 +++++++---- 3 files changed, 107 insertions(+), 23 deletions(-) diff --git a/apps/backend/tests/test_tools/test_credentials.py b/apps/backend/tests/test_tools/test_credentials.py index e38949f..d4a0d53 100644 --- a/apps/backend/tests/test_tools/test_credentials.py +++ b/apps/backend/tests/test_tools/test_credentials.py @@ -317,6 +317,60 @@ def test_bash_uses_both_clouds_when_both_connected(monkeypatch): cred.reset_current_cloud_account(token) assert r_aws["success"] and r_az["success"] - env_by_bin = dict(runs) + # runs[N][0] is the absolute exe path (shutil.which resolved it); match on basename + # so the test works regardless of where aws/az live (or .exe / .cmd on Windows). + import os as _os + + env_by_bin = {_os.path.basename(t).split(".")[0]: env for t, env in runs} assert env_by_bin["aws"]["AWS_ACCESS_KEY_ID"] == "AKIAFAKE" # AWS account creds assert env_by_bin["az"]["AZURE_SUBSCRIPTION_ID"] == "sub-1" # Azure account env + + +# ── Tri-state contextvar: None (ambient) vs {} (explicit empty / fail closed) ── + + +def test_current_cloud_accounts_is_none_when_unset(): + """OSS / self-host default: contextvar was never set → callers see None, not {}.""" + assert cred.current_cloud_accounts() is None + + +def test_resolve_session_ambient_when_contextvar_unset(monkeypatch): + """OSS default behavior is preserved: with no contextvar set, resolve_session falls back + to _base_session() (the host's ambient env/profile creds).""" + sentinel = object() + monkeypatch.setattr(cred, "_base_session", lambda: sentinel) + cred._session_cache.clear() + # contextvar is at its default (None); no setter was called. + assert cred.resolve_session() is sentinel + + +def test_resolve_session_fails_closed_on_explicit_empty(): + """Product safety net: when the contextvar is explicitly set to {} (e.g. tenant with no + Cloud Account connected), resolve_session must NOT fall back to platform creds — it raises. + """ + cred._session_cache.clear() + token = cred.set_current_cloud_accounts([]) # builds {} -> fail-closed signal + try: + with pytest.raises(RuntimeError, match="no AWS account connected"): + cred.resolve_session() + finally: + cred.reset_current_cloud_account(token) + + +def test_bash_aws_blocked_on_explicit_empty_accounts(monkeypatch): + """Mirror of the resolver test at the bash layer: with {} on the contextvar, bash `aws` + is denied without touching subprocess (so the platform's `aws` CLI can't be used either).""" + from opendevops_core.tools import bash_tool + + def _no_run(*a, **k): # subprocess must not execute + raise AssertionError("subprocess.run must not run when accounts is explicitly empty") + + monkeypatch.setattr(bash_tool.subprocess, "run", _no_run) + token = cred.set_current_cloud_accounts([]) + try: + res = bash_tool.run_bash_command("aws sts get-caller-identity") + finally: + cred.reset_current_cloud_account(token) + + assert res["success"] is False and res.get("blocked") is True + assert "no cloud account" in res["error"].lower() diff --git a/apps/core/src/opendevops_core/providers/aws/credentials.py b/apps/core/src/opendevops_core/providers/aws/credentials.py index 93ae392..a331042 100644 --- a/apps/core/src/opendevops_core/providers/aws/credentials.py +++ b/apps/core/src/opendevops_core/providers/aws/credentials.py @@ -29,9 +29,15 @@ from opendevops_core.config import settings -# Map of provider -> active cloud account for this request (or None = env/profile fallback). +# Map of provider -> active cloud account for this request. # An org can have several connected clouds active at once (e.g. AWS + Azure); each cloud's # tools resolve credentials against that provider's account independently. +# +# Three states matter: +# None -> contextvar never set in this task. OSS / self-host => ambient env/profile creds. +# {} -> explicitly set to empty (e.g. product tenant with no Cloud Account connected) => +# fail closed; AWS/Azure tools deny instead of using the platform's creds. +# {p:a} -> active accounts; resolve per-provider. _current_accounts: contextvars.ContextVar[dict[str, dict] | None] = contextvars.ContextVar( "current_cloud_accounts", default=None ) @@ -56,18 +62,21 @@ def reset_current_cloud_account(token: contextvars.Token) -> None: _current_accounts.reset(token) -def current_cloud_accounts() -> dict[str, dict]: - """The provider->account map active for this task ({} when none).""" - return _current_accounts.get() or {} +def current_cloud_accounts() -> dict[str, dict] | None: + """Return the provider->account map active for this task, or None when the contextvar + was never set (OSS / self-host => ambient fallback). Callers must distinguish ``None`` + (ambient) from ``{}`` (explicitly empty => fail closed).""" + return _current_accounts.get() def account_for_provider(provider: str) -> dict | None: - return current_cloud_accounts().get(provider) + accts = _current_accounts.get() + return accts.get(provider) if accts else None def get_current_cloud_account() -> dict | None: """Back-compat single-account accessor: prefer AWS, else any active account, else None.""" - accts = current_cloud_accounts() + accts = _current_accounts.get() if not accts: return None return accts.get("aws") or next(iter(accts.values())) @@ -176,15 +185,24 @@ def _access_key_session(account: dict) -> boto3.Session: def resolve_session() -> boto3.Session: - """The boto3 session for the active AWS account, or the base session when nothing is set.""" - accts = current_cloud_accounts() - if not accts: + """The boto3 session for the active AWS account. + + Tri-state on the per-task contextvar: + * Unset (None) — OSS / self-host => fall back to the host's ambient creds. + * Explicitly empty ({}) — product tenant with no Cloud Account connected => + fail closed; do NOT fall back to platform creds (cross-tenant leak). + * Has an "aws" entry — assume role / use access key. + * Has entries but no "aws" — Azure-only / other; AWS access denied. + """ + accts = _current_accounts.get() + if accts is None: return _base_session() # OSS / unscoped -> ambient env/profile creds account = accts.get("aws") if account is None: - # Fail closed: the org has connected clouds but no AWS account, so AWS access must NOT - # fall back to the platform's base credentials (that would leak the platform account). + # Fail closed: either the contextvar was explicitly set to {} (product tenant with no + # connected accounts) or the org has only non-AWS clouds connected. Either way, AWS + # access must NOT fall back to the platform's base credentials. raise RuntimeError( "AWS is not available for this organization (no AWS account connected)." ) diff --git a/apps/core/src/opendevops_core/tools/bash_tool.py b/apps/core/src/opendevops_core/tools/bash_tool.py index f0c63a2..6812cd4 100644 --- a/apps/core/src/opendevops_core/tools/bash_tool.py +++ b/apps/core/src/opendevops_core/tools/bash_tool.py @@ -246,10 +246,12 @@ def run_bash_command(command: str) -> dict[str, Any]: logger.info("bash_tool RUN | cmd={!r}", command) # Resolve credentials per command, against the org's connected account for that command's - # cloud. An org can have multiple clouds active at once (AWS + Azure); each binary uses its - # own provider's account. If the org has connected clouds but none for this binary's provider, - # fail closed (don't fall back to the platform's ambient creds). No connected accounts at all - # (OSS / self-host) -> ambient env (the host's `aws`/`az` login). + # cloud. Tri-state on the contextvar (set in product chat.py; never set in OSS): + # None -> ambient mode (OSS / self-host); run_env stays None, subprocess inherits host env. + # {} -> product tenant with no Cloud Account connected; deny all cloud binaries + # (do NOT fall back to platform creds — that would be a cross-tenant leak). + # {p:a} -> per-provider resolution: each binary uses its own provider's account, fail + # closed when an org has connected clouds but not the one this binary needs. accounts = current_cloud_accounts() binary = tokens[0] run_env = None @@ -257,7 +259,13 @@ def run_bash_command(command: str) -> dict[str, Any]: def _blocked(msg: str) -> dict[str, Any]: return {"success": False, "output": "", "error": msg, "command": command, "blocked": True} - if binary == "aws": + cloud_binary = binary in ("aws", "az", "kubectl") + if accounts == {} and cloud_binary: + return _blocked( + f"{binary} is not available — no cloud account is connected for this organization." + ) + + if accounts and binary == "aws": if accounts.get("aws") is not None: try: frozen = resolve_session().get_credentials().get_frozen_credentials() @@ -273,18 +281,22 @@ def _blocked(msg: str) -> dict[str, Any]: except Exception as e: # noqa: BLE001 - fail closed; do not leak platform creds logger.error("bash_tool: could not resolve org AWS creds: {}", e) return _blocked(f"Could not resolve organization AWS credentials: {e}") - elif accounts: - return _blocked("AWS is not available for this organization (no AWS account connected).") - elif binary == "az": + else: + return _blocked( + "AWS is not available for this organization (no AWS account connected)." + ) + elif accounts and binary == "az": if accounts.get("azure") is not None: try: run_env = azure_cli_env(accounts["azure"]) except Exception as e: # noqa: BLE001 - fail closed logger.error("bash_tool: could not resolve org Azure creds: {}", e) return _blocked(f"Could not resolve organization Azure credentials: {e}") - elif accounts: - return _blocked("Azure is not available for this organization (no Azure account connected).") - elif binary == "kubectl" and accounts.get("azure") is not None: + else: + return _blocked( + "Azure is not available for this organization (no Azure account connected)." + ) + elif accounts and binary == "kubectl" and accounts.get("azure") is not None: # AKS: kubectl must read the per-org kubeconfig written by `az aks get-credentials` # into the org's isolated config dir. try: