mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-12 13:52:15 +00:00
fix(egress): close GOOGLE_API_KEY coverage gap + config/mappings write TOCTOU
Two correctness gaps surfaced in the review thread (texasich) that survived the prior rounds: - GOOGLE_API_KEY was warn-only while GEMINI_API_KEY was fail-closed, despite both authenticating the same generativelanguage LLM endpoint (auth.py treats them as interchangeable). An operator with only GOOGLE_API_KEY set + fail_on_uncovered_providers got false coverage. Added it to _LLM_SPECIFIC_NON_BEARER_PROVIDERS. - write_proxy_config / write_mappings chmod'd AFTER os.replace, leaving the token-bearing files briefly world-readable under a slack umask (the 0o700 state dir mitigates but same-uid race remained). chmod the temp file BEFORE the atomic replace, matching the CA-key write path. Tests: assert GOOGLE_API_KEY in blocked tier; assert proxy.yaml + mappings.json land at 0o600.
This commit is contained in:
parent
ad978ed962
commit
ae1bfa1806
2 changed files with 32 additions and 2 deletions
|
|
@ -187,6 +187,13 @@ _LLM_SPECIFIC_NON_BEARER_PROVIDERS: Tuple[str, ...] = (
|
|||
"ANTHROPIC_API_KEY",
|
||||
"AZURE_OPENAI_API_KEY",
|
||||
"GEMINI_API_KEY",
|
||||
# GOOGLE_API_KEY is an interchangeable alias for GEMINI_API_KEY in
|
||||
# Hermes (auth.py keys Google on both; the native Gemini adapter
|
||||
# accepts either) and authenticates the same generativelanguage
|
||||
# LLM endpoint. It belongs in the fail-closed tier too — otherwise
|
||||
# an operator with only GOOGLE_API_KEY set who enables
|
||||
# fail_on_uncovered_providers gets a false sense of coverage.
|
||||
"GOOGLE_API_KEY",
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -1155,8 +1162,13 @@ def write_proxy_config(config: Dict) -> Path:
|
|||
tmp_path = state / ".proxy.yaml.tmp"
|
||||
with open(tmp_path, "w", encoding="utf-8") as f:
|
||||
yaml.safe_dump(config, f, default_flow_style=False, sort_keys=False)
|
||||
# Tighten perms on the temp file BEFORE the atomic replace so the
|
||||
# final path is never briefly world-readable under a slack umask
|
||||
# (the config embeds proxy token values). chmod-after-replace would
|
||||
# leave a TOCTOU window; the 0o700 state dir mitigates but same-uid
|
||||
# processes could still race.
|
||||
os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR)
|
||||
os.replace(tmp_path, out)
|
||||
os.chmod(out, stat.S_IRUSR | stat.S_IWUSR)
|
||||
return out
|
||||
|
||||
|
||||
|
|
@ -1184,8 +1196,11 @@ def write_mappings(mappings: List[TokenMapping]) -> Path:
|
|||
tmp_path = state / ".mappings.json.tmp"
|
||||
with open(tmp_path, "w", encoding="utf-8") as f:
|
||||
json.dump(payload, f, indent=2)
|
||||
# chmod before the atomic replace — see write_proxy_config. The
|
||||
# mappings file holds proxy token values, so close the TOCTOU window
|
||||
# rather than chmod-ing after the file is already at its final path.
|
||||
os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR)
|
||||
os.replace(tmp_path, out)
|
||||
os.chmod(out, stat.S_IRUSR | stat.S_IWUSR)
|
||||
return out
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -392,6 +392,16 @@ def test_write_proxy_config_serializes_yaml(hermes_home, tmp_path):
|
|||
text = out.read_text(encoding="utf-8")
|
||||
assert "tunnel_listen" in text
|
||||
assert f"ca_cert: {ca_crt}" in text
|
||||
# The rendered config embeds proxy token values — it must land at
|
||||
# 0o600, and (TOCTOU) must never be transiently world-readable
|
||||
# between the atomic replace and the chmod. We chmod the temp file
|
||||
# before the replace, so the final file is 0o600 from first byte.
|
||||
import os as _os
|
||||
mode = _os.stat(out).st_mode & 0o777
|
||||
assert mode == 0o600, f"proxy.yaml perms {oct(mode)}, expected 0o600"
|
||||
mappings_out = ip.write_mappings([_sample_mapping()])
|
||||
mmode = _os.stat(mappings_out).st_mode & 0o777
|
||||
assert mmode == 0o600, f"mappings.json perms {oct(mmode)}, expected 0o600"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -1360,6 +1370,7 @@ def test_blocked_providers_subset_of_uncovered(hermes_home, monkeypatch):
|
|||
monkeypatch.setenv("AWS_ACCESS_KEY_ID", "AKIA-test")
|
||||
monkeypatch.setenv("GOOGLE_APPLICATION_CREDENTIALS", "/etc/gcp.json")
|
||||
monkeypatch.setenv("GEMINI_API_KEY", "g-test")
|
||||
monkeypatch.setenv("GOOGLE_API_KEY", "g-test-alias")
|
||||
|
||||
uncovered = set(ip.discover_uncovered_providers())
|
||||
blocked = set(ip.discover_blocked_providers())
|
||||
|
|
@ -1376,6 +1387,10 @@ def test_blocked_providers_subset_of_uncovered(hermes_home, monkeypatch):
|
|||
# LLM-specific providers ARE blocked.
|
||||
assert "ANTHROPIC_API_KEY" in blocked
|
||||
assert "GEMINI_API_KEY" in blocked
|
||||
# GOOGLE_API_KEY is an alias for GEMINI_API_KEY (same generativelanguage
|
||||
# LLM endpoint) — it must be in the fail-closed tier, not warn-only,
|
||||
# or fail_on_uncovered_providers gives false coverage.
|
||||
assert "GOOGLE_API_KEY" in blocked
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue