fix: cache file permission window, export-env quoting, shell guardrail gap

README.md

@@ -250,9 +250,12 @@ recursive `s3 rm`, etc.).
 
 - Credentials are always short-lived (from AWS SSO's `GetRoleCredentials`),
   scoped to exactly the account/role requested, and expire on their own.
-- Nothing is ever written to a shell's exported `AWS_*` variables outside the
-  child process spawned by `exec`/`shell` — your parent shell's environment
-  is untouched.
+- `exec` and `shell` never touch the parent shell's own environment —
+  credentials exist only in the memory of the one child process/subshell
+  spawned for that command. `export-env` and `creds-process` are the
+  deliberate exceptions: printing credentials to stdout (for `eval` into
+  your *current* shell, or for AWS tooling's `credential_process` protocol)
+  is their whole point, not a leak — see the notes on them above.
 - `orgctl logout` clears every cached token/credential immediately.
 - Guardrails and the audit log are local-only conveniences, not a substitute
   for IAM permission boundaries, SCPs, or CloudTrail.

config/guardrails.example.yaml

@@ -5,6 +5,11 @@
 
 # Account IDs where ANY command via `orgctl exec`/`shell` is blocked outright.
 # Useful for a management/root account you never want ad-hoc commands against.
+# Does NOT cover `export-env` or `creds-process` — those are meant to work
+# non-interactively (wired into ~/.aws/config, a script, or CI), so blocking
+# them here would just break that use case rather than add friction to it.
+# If a protected account must never be reachable through orgctl at all,
+# don't add it to orgs.yaml (or don't wire creds-process for it).
 protected_account_ids:
   - "111111111111"
 

docs/THREAT_MODEL.md

@@ -22,7 +22,7 @@ What this tool has custody of, at some point, and what protecting it means:
 | Asset | Where it lives | What "protected" means |
 |---|---|---|
 | SSO access token | OS keychain (preferred) or `~/.orgctl/` cache file (fallback) | Not written to disk in plaintext when a keychain is available; 0600 permissions and expiry-checked reads when it isn't |
-| Short-lived role credentials (`AccessKeyId`/`SecretAccessKey`/`SessionToken`) | Same cache, keyed per account+role | Same as above, plus never exported outside the one child process/shell that requested them |
+| Short-lived role credentials (`AccessKeyId`/`SecretAccessKey`/`SessionToken`) | Same cache, keyed per account+role | Same as above, plus never exported outside the one child process/shell that requested them — except via `export-env`/`creds-process`, whose whole purpose is printing them to stdout for the caller's own use (see scenario 4) |
 | `orgs.yaml` (account registry) | `~/.orgctl/orgs.yaml` (or `ORGCTL_CONFIG`) | Contains account IDs and role names only — no secrets — but is still the map an attacker would want to see, and its contents drive which guardrails apply |
 | `guardrails.yaml` | `~/.orgctl/guardrails.yaml` (or `ORGCTL_GUARDRAILS`) | Governs which commands get blocked/confirmed — its integrity matters more than its confidentiality |
 | Local audit log | `~/.orgctl/audit.log` (or `ORGCTL_HOME`) | A record of what was run against which account, for the operator's own review; append-only in practice, not append-only *enforced* (see below) |
@@ -131,16 +131,28 @@ channel, not an ad-hoc copy.
 shell history, a subprocess the operator didn't intend to grant them to,
 or the parent shell's own environment.
 
-**Mitigation:** `exec_cmd._creds_to_env()` builds a **copy** of the
-environment (`os.environ.copy()`), so the parent shell's own `os.environ`
-is never mutated — credentials exist only in the memory of the one
-`subprocess.run()` child. `AWS_PROFILE` is explicitly stripped from that
-copy so a long-lived profile configured in the parent shell can't get
+**Mitigation:** for `exec`/`shell`, `exec_cmd._creds_to_env()` builds a
+**copy** of the environment (`os.environ.copy()`), so the parent shell's
+own `os.environ` is never mutated — credentials exist only in the memory of
+the one `subprocess.run()` child. `AWS_PROFILE` is explicitly stripped from
+that copy so a long-lived profile configured in the parent shell can't get
 picked up by mistake alongside the short-lived creds. `subprocess.run()` is
 called with a list (`command`), never `shell=True` with a joined string —
 so there's no shell-injection surface from account aliases, role names, or
 arguments containing shell metacharacters.
 
+`export-env` and `creds-process` are a deliberate exception to "stays in
+one child process" — printing credentials to stdout is their entire
+purpose (`eval`-ing into the *current* shell, or feeding AWS tooling's
+`credential_process` protocol), not a leak. What they still guard against:
+`export_env_lines()` quotes every value before interpolating it into the
+printed `export KEY=value` / `$env:KEY = "value"` line (`shlex.quote()` on
+POSIX, backtick/`"`/`$` escaping on PowerShell), so a value containing a
+shell metacharacter can't break out of its assignment when the caller
+`eval`s the output; `creds-process` writes human-readable errors and the
+login URL to stderr only, keeping stdout clean JSON for the AWS SDK/CLI to
+parse.
+
 **Residual risk:** once credentials are in a child process's environment,
 `orgctl` has no control over what that child process (or anything *it*
 spawns) does with them — a command that itself echoes `$AWS_SECRET_ACCESS_KEY`

src/orgctl/cache.py

@@ -66,6 +66,23 @@ def _lock_down(path: Path) -> None:
         pass  # e.g. Windows — best effort only
 
 
+def _write_locked_down(path: Path, data: str) -> None:
+    """Create/overwrite `path` with owner-only permissions in place, rather
+    than writing with the default (often world-readable) mode and chmod'ing
+    afterward — which leaves a brief window where a brand-new cache file,
+    containing a live token or credentials, is readable by anyone. Still
+    best-effort on Windows, where os.open()'s mode argument doesn't map onto
+    real ACLs (see _lock_down above) — the final _lock_down call here mainly
+    matters for the case the file already existed with looser permissions.
+    """
+    fd = os.open(path, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600)
+    try:
+        with os.fdopen(fd, "w") as f:
+            f.write(data)
+    finally:
+        _lock_down(path)
+
+
 def _path_for(key: str) -> Path:
     safe = "".join(c if c.isalnum() or c in "-_." else "_" for c in key)
     return cache_dir() / f"{safe}.json"
@@ -159,8 +176,7 @@ def put(key: str, value: dict) -> None:
                 pass  # fall back to file cache below
 
     p = _path_for(key)
-    p.write_text(json.dumps(value))
-    _lock_down(p)
+    _write_locked_down(p, json.dumps(value))
 
 
 def _keyring_delete(key: str) -> None:

src/orgctl/exec_cmd.py

@@ -7,6 +7,7 @@
 from __future__ import annotations
 
 import os
+import shlex
 import subprocess
 import sys
 import time
@@ -125,10 +126,29 @@ def spawn_shell(
     role: str | None,
     region: str | None = None,
     *,
+    gcfg: guardrails.GuardrailConfig | None = None,
     reason: str | None = None,
 ) -> int:
     account = resolve_account(cfg, account_alias_or_id)
     resolved_role = resolve_role(account, role)
+    gcfg = gcfg or guardrails.GuardrailConfig.load()
+
+    # There's no single "command" here to pattern-match against (this opens
+    # an interactive session), but the protected-account list is documented
+    # as covering both `exec` and `shell` — enforce that part of it.
+    block_reason = guardrails.check_protected_account(account.account_id, gcfg)
+    if block_reason:
+        audit.record(
+            action="shell",
+            account_id=account.account_id,
+            role=resolved_role,
+            result="blocked",
+            detail=block_reason,
+            reason=reason,
+        )
+        print(f"BLOCKED by guardrails: {block_reason}", file=sys.stderr)
+        return 2
+
     creds = get_role_credentials(sso_token, account.account_id, resolved_role)
     env = _creds_to_env(creds, region or cfg.default_region)
 
@@ -187,5 +207,15 @@ def export_env_lines(
         ("AWS_REGION", resolved_region),
     ]
     if powershell:
-        return "\n".join(f'$env:{k} = "{v}"' for k, v in pairs)
-    return "\n".join(f'export {k}="{v}"' for k, v in pairs)
+        return "\n".join(f"$env:{k} = {_powershell_quote(v)}" for k, v in pairs)
+    return "\n".join(f"export {k}={shlex.quote(v)}" for k, v in pairs)
+
+
+def _powershell_quote(value: str) -> str:
+    """Quote `value` for interpolation into a PowerShell double-quoted
+    string. AWS credentials/regions never actually contain these characters,
+    but the values still come from an external API response, so this is
+    defensive rather than provably unnecessary — same reasoning as using
+    shlex.quote() for the POSIX side above."""
+    escaped = value.replace("`", "``").replace('"', '`"').replace("$", "`$")
+    return f'"{escaped}"'

src/orgctl/guardrails.py

@@ -59,16 +59,30 @@ def load(cls, path: Path | None = None) -> GuardrailConfig:
 ]
 
 
-def check_command(command: list[str], account_id: str, cfg: GuardrailConfig) -> str | None:
-    """Return a block reason string if the command should be denied, else None."""
-    joined = " ".join(command)
-
+def check_protected_account(account_id: str, cfg: GuardrailConfig) -> str | None:
+    """Return a block reason if account_id is marked protected, else None.
+
+    Split out from check_command() so callers with no single "command" to
+    pattern-match against — spawn_shell(), which opens an interactive
+    session rather than running one command — can still enforce the
+    protected-account list, which guardrails.yaml documents as covering
+    both `exec` and `shell`.
+    """
     if account_id in cfg.protected_account_ids:
         return (
             f"Account {account_id} is marked protected in guardrails.yaml — "
             f"remove it there if this command is intentional."
         )
+    return None
 
+
+def check_command(command: list[str], account_id: str, cfg: GuardrailConfig) -> str | None:
+    """Return a block reason string if the command should be denied, else None."""
+    protected_reason = check_protected_account(account_id, cfg)
+    if protected_reason:
+        return protected_reason
+
+    joined = " ".join(command)
     for pattern in _BUILTIN_DENY + cfg.deny_patterns:
         if fnmatch.fnmatch(joined, pattern):
             return f"Command matches deny pattern: '{pattern}'"

tests/test_cache.py

@@ -1,5 +1,10 @@
+import os
+import stat
+import sys
 import time
 
+import pytest
+
 from orgctl import cache
 
 
@@ -57,3 +62,36 @@ def test_sso_token_key_respects_expiry(tmp_path, monkeypatch):
     monkeypatch.setenv("ORGCTL_HOME", str(tmp_path))
     cache.put("sso-token_expired", {"accessToken": "x", "expiresAt": time.time() - 10})
     assert cache.get("sso-token_expired") is None
+
+
+def test_put_creates_new_files_with_owner_only_mode_atomically(tmp_path, monkeypatch):
+    # Regression: the old implementation wrote with Path.write_text() (mode
+    # dictated by the process umask, often world-readable) and only
+    # restricted access with chmod() afterward — a real window during which
+    # a brand-new token/credentials file was readable by anyone. os.open()
+    # with O_CREAT and an explicit mode sets the permissions atomically at
+    # creation time instead.
+    monkeypatch.setenv("ORGCTL_HOME", str(tmp_path))
+    calls = []
+    real_open = os.open
+
+    def _spy_open(path, flags, mode=0o777):
+        calls.append((flags, mode))
+        return real_open(path, flags, mode)
+
+    monkeypatch.setattr(cache.os, "open", _spy_open)
+    cache.put("mykey", {"value": 1, "expiresAt": time.time() + 60})
+
+    assert calls, "expected cache.put() to create the file via os.open()"
+    flags, mode = calls[-1]
+    assert flags & os.O_CREAT
+    assert flags & os.O_WRONLY
+    assert mode == 0o600
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="POSIX permission bits don't apply on Windows")
+def test_put_file_has_owner_only_permissions_on_disk(tmp_path, monkeypatch):
+    monkeypatch.setenv("ORGCTL_HOME", str(tmp_path))
+    cache.put("mykey", {"value": 1, "expiresAt": time.time() + 60})
+    path = cache.cache_dir() / "mykey.json"
+    assert stat.S_IMODE(path.stat().st_mode) == 0o600

tests/test_exec_cmd.py

@@ -303,3 +303,44 @@ def _boom(creds, region):
     assert rc == 0
     assert len(fake_subprocess) == 1
     assert "policy pre-check failed to run" in capsys.readouterr().err
+
+
+def test_spawn_shell_blocks_protected_account(
+    cfg, fake_get_creds, fake_subprocess, tmp_path, capsys
+):
+    # Regression: guardrails.yaml documents protected_account_ids as
+    # blocking "ANY command via `orgctl exec`/`shell`", but spawn_shell()
+    # never called into guardrails at all — there's no single "command" to
+    # pattern-match against for an interactive session, but the
+    # protected-account list should still apply.
+    gcfg = guardrails.GuardrailConfig(protected_account_ids=["111111111111"])
+
+    rc = exec_cmd.spawn_shell(
+        cfg,
+        sso_token=None,
+        account_alias_or_id="prod",
+        role="admin",
+        gcfg=gcfg,
+    )
+
+    assert rc == 2
+    assert fake_get_creds == []
+    assert fake_subprocess == []
+    assert "BLOCKED by guardrails" in capsys.readouterr().err
+    assert _last_audit_entry(tmp_path)["result"] == "blocked"
+
+
+def test_spawn_shell_proceeds_when_account_not_protected(cfg, fake_get_creds, fake_subprocess):
+    gcfg = guardrails.GuardrailConfig(protected_account_ids=["999999999999"])
+
+    rc = exec_cmd.spawn_shell(
+        cfg,
+        sso_token=None,
+        account_alias_or_id="prod",
+        role="admin",
+        gcfg=gcfg,
+    )
+
+    assert rc == 0
+    assert fake_get_creds == [("111111111111", "admin")]
+    assert len(fake_subprocess) == 1

tests/test_export_env.py

@@ -0,0 +1,103 @@
+"""Tests for exec_cmd.export_env_lines(): the lines it prints are meant to be
+eval'd directly into the caller's shell, so a value containing a shell
+metacharacter must not let that value break out of its assignment.
+"""
+
+from __future__ import annotations
+
+import subprocess
+import sys
+
+import pytest
+
+from orgctl import exec_cmd
+from orgctl.config import Account, OrgConfig
+from orgctl.sso import SsoToken
+
+CFG = OrgConfig(
+    name="test",
+    sso_start_url="https://example.awsapps.com/start",
+    sso_region="us-east-1",
+    default_region="us-east-1",
+    accounts={
+        "prod": Account(alias="prod", account_id="111111111111", roles=["admin"]),
+    },
+)
+
+TOKEN = SsoToken(access_token="tok", expires_at=0, region="us-east-1", start_url=CFG.sso_start_url)
+
+
+@pytest.fixture(autouse=True)
+def _home(tmp_path, monkeypatch):
+    monkeypatch.setenv("ORGCTL_HOME", str(tmp_path))
+
+
+def _fake_creds(monkeypatch, **overrides):
+    creds = {
+        "AccessKeyId": "AKIAFAKE",
+        "SecretAccessKey": "fake-secret",
+        "SessionToken": "fake-token",
+        "Expiration": 9999999999000,
+        **overrides,
+    }
+    monkeypatch.setattr(exec_cmd, "get_role_credentials", lambda *a, **k: creds)
+    return creds
+
+
+def test_posix_lines_have_ordinary_values_unquoted(monkeypatch):
+    _fake_creds(monkeypatch)
+    lines = exec_cmd.export_env_lines(CFG, TOKEN, "prod", "admin")
+    assert "export AWS_ACCESS_KEY_ID=AKIAFAKE" in lines
+    assert "export AWS_SECRET_ACCESS_KEY=fake-secret" in lines
+    assert "export AWS_REGION=us-east-1" in lines
+
+
+def test_powershell_lines_have_ordinary_values_double_quoted(monkeypatch):
+    _fake_creds(monkeypatch)
+    lines = exec_cmd.export_env_lines(CFG, TOKEN, "prod", "admin", powershell=True)
+    assert '$env:AWS_ACCESS_KEY_ID = "AKIAFAKE"' in lines
+    assert '$env:AWS_REGION = "us-east-1"' in lines
+
+
+@pytest.mark.parametrize(
+    "malicious_token",
+    [
+        "abc$(rm -rf ~)def",
+        "abc`rm -rf ~`def",
+        "abc'; rm -rf ~; echo 'def",
+        'abc"; rm -rf ~; echo "def',
+    ],
+)
+def test_posix_export_line_is_safe_to_eval_even_with_shell_metacharacters(
+    monkeypatch, malicious_token
+):
+    # Regression: values were interpolated into `export KEY="value"` with no
+    # quoting/escaping. AWS credentials never actually contain these
+    # characters, but the values still come from an external API response,
+    # so the interpolation itself should not be a foot-gun. Prove it by
+    # actually eval-ing the generated line in a POSIX shell and checking the
+    # metacharacters were neutralized rather than executed.
+    if sys.platform == "win32":
+        pytest.skip("needs a POSIX shell to eval against")
+    _fake_creds(monkeypatch, SessionToken=malicious_token)
+    lines = exec_cmd.export_env_lines(CFG, TOKEN, "prod", "admin")
+
+    result = subprocess.run(
+        ["sh", "-c", f'{lines}\nprintf "%s" "$AWS_SESSION_TOKEN"'],
+        capture_output=True,
+        text=True,
+        check=True,
+    )
+    assert result.stdout == malicious_token
+
+
+def test_powershell_double_quotes_in_value_do_not_end_the_string(monkeypatch):
+    _fake_creds(monkeypatch, SessionToken='abc"; Remove-Item -Recurse C:\\; "def')
+    lines = exec_cmd.export_env_lines(CFG, TOKEN, "prod", "admin", powershell=True)
+    line = next(line_ for line_ in lines.splitlines() if "AWS_SESSION_TOKEN" in line_)
+    # The whole value stays inside one quoted string: exactly two unescaped
+    # (non-backtick-preceded) double quotes delimit it, at the very ends.
+    assert line.startswith('$env:AWS_SESSION_TOKEN = "')
+    assert line.endswith('"')
+    body = line[len('$env:AWS_SESSION_TOKEN = "') : -1]
+    assert '`"' in body  # the embedded quotes were escaped, not left bare