From 87a15733c068ad4839e5b858236ac7272492fe85 Mon Sep 17 00:00:00 2001 From: Victor Kyriazakos Date: Tue, 14 Jul 2026 20:55:15 +0000 Subject: [PATCH] =?UTF-8?q?fix(monitoring):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20persist=20install=5Fid,=20drop=20dead=20config,=20single=20r?= =?UTF-8?q?edaction=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cut the leftover telemetry.* DEFAULT_CONFIG block (nothing reads it) and the legacy telemetry-key fallback in policy.py. - install_id: persist the minted UUID back to config.yaml on first use so service.instance.id survives gateway restarts (fail-open when the write is not possible); regression test covers the restart path. - Remove the no-op gateway_health_export.redaction config keys. Redaction is always-on by design and deliberately not configurable; status output now says so. - Collapse redaction to one unconditional secrets+PII scrub: drop the none/pii content modes (they served the dropped trajectories plane) and fold gateway_health.py's duplicate bearer/token/email/phone regex layer into agent/monitoring/redaction.py. --- agent/monitoring/gateway_health.py | 23 ++--- agent/monitoring/policy.py | 47 +++++++---- agent/monitoring/redaction.py | 83 +++++++++---------- hermes_cli/config.py | 59 ------------- hermes_cli/main.py | 6 +- .../gateway_health_export_probe.py | 1 - tests/monitoring/test_export_redaction.py | 55 +++++++----- .../monitoring/test_gateway_health_export.py | 26 +++++- 8 files changed, 137 insertions(+), 163 deletions(-) diff --git a/agent/monitoring/gateway_health.py b/agent/monitoring/gateway_health.py index 3404392c63..8ba31386e0 100644 --- a/agent/monitoring/gateway_health.py +++ b/agent/monitoring/gateway_health.py @@ -9,7 +9,6 @@ session history, audit records, or product analytics belong here. from __future__ import annotations import logging -import re from dataclasses import dataclass from typing import Any, Dict, List, Optional @@ -29,12 +28,6 @@ class GatewayHealthSnapshot: events: List[GatewayHealthEvent | GatewayDiagnosticEvent] -_EMAIL_RE = re.compile(r"[A-Za-z0-9._%+\-]+@[A-Za-z0-9.\-]+\.[A-Za-z]{2,}") -_BEARER_RE = re.compile(r"\bBearer\s+[A-Za-z0-9._~+\-/]+=*", re.IGNORECASE) -_TOKEN_RE = re.compile(r"\b(xox[baprs]-[A-Za-z0-9-]+|sk-[A-Za-z0-9_-]{8,}|gh[pousr]_[A-Za-z0-9_]{8,})\b") -_SECRET_LITERAL_RE = re.compile(r"\*{3,}") -_PHONE_RE = re.compile(r"(? bool: def redact_gateway_message(message: Any) -> str: - """Redact gateway diagnostic free text for customer-owned export.""" - text = str(message or "") + """Redact gateway diagnostic free text for operator-owned export. + + Single scrub path: everything goes through + ``agent.monitoring.redaction.redact_for_export`` (unconditional + secrets + PII), then is length-bounded. + """ try: from agent.monitoring.redaction import redact_for_export - redacted = redact_for_export(text, content_mode="pii") or "" + redacted = redact_for_export(str(message or "")) or "" except Exception: redacted = "[redaction-unavailable]" - redacted = _BEARER_RE.sub("[redacted]", redacted) - redacted = _TOKEN_RE.sub("[redacted]", redacted) - redacted = _SECRET_LITERAL_RE.sub("[redacted]", redacted) - redacted = re.sub(r"\bBearer\s+\[[^\]]+\]", "[redacted]", redacted, flags=re.IGNORECASE) - redacted = _EMAIL_RE.sub("[email]", redacted) - redacted = _PHONE_RE.sub("[phone]", redacted) return redacted[:500] diff --git a/agent/monitoring/policy.py b/agent/monitoring/policy.py index d07b90ea2c..1a42ce195c 100644 --- a/agent/monitoring/policy.py +++ b/agent/monitoring/policy.py @@ -8,31 +8,48 @@ collector. It carries no account identity and can be rotated by clearing from __future__ import annotations +import logging import uuid from typing import Any, Dict - -def _monitoring_cfg(config: Dict[str, Any]) -> Dict[str, Any]: - for key in ("monitoring", "telemetry"): # accept legacy telemetry.* keys - cfg = config.get(key) if isinstance(config, dict) else None - if isinstance(cfg, dict) and cfg.get("install_id"): - return cfg - cfg = config.get("monitoring") if isinstance(config, dict) else None - return cfg if isinstance(cfg, dict) else {} +logger = logging.getLogger(__name__) def ensure_install_id(config: Dict[str, Any]) -> str: - """Return a stable install id, minting one if the config slot is empty. + """Return a stable install id, minting and persisting one when empty. - Does not persist — the caller writes the returned value back to - config.yaml. Clearing ``monitoring.install_id`` (e.g. with - ``hermes config set monitoring.install_id ""``) mints anew on next call. + The id must survive gateway restarts (it becomes ``service.instance.id`` + on exported signals), so a freshly minted UUID is written back to + config.yaml immediately. The write is fail-open: if persisting fails + (read-only home, managed scope), the ephemeral id is still returned and + a new one is minted next start. + + Clearing ``monitoring.install_id`` (e.g. ``hermes config set + monitoring.install_id ""``) rotates the id on the next gateway start. """ - cfg = _monitoring_cfg(config) - existing = cfg.get("install_id") + mon = config.get("monitoring") if isinstance(config, dict) else None + existing = (mon or {}).get("install_id") if isinstance(mon, dict) else None if isinstance(existing, str) and existing.strip(): return existing - return str(uuid.uuid4()) + + minted = str(uuid.uuid4()) + try: + from hermes_cli.config import load_config, save_config + + fresh = load_config() + if isinstance(fresh, dict): + slot = fresh.setdefault("monitoring", {}) + if isinstance(slot, dict) and not str(slot.get("install_id") or "").strip(): + slot["install_id"] = minted + save_config(fresh) + except Exception: + logger.debug("install_id persist failed; using ephemeral id", exc_info=True) + # Keep the in-memory config consistent for this process either way. + if isinstance(config, dict): + config.setdefault("monitoring", {}) + if isinstance(config["monitoring"], dict): + config["monitoring"]["install_id"] = minted + return minted __all__ = [ diff --git a/agent/monitoring/redaction.py b/agent/monitoring/redaction.py index 7ea5524ccc..cf1564f017 100644 --- a/agent/monitoring/redaction.py +++ b/agent/monitoring/redaction.py @@ -1,77 +1,70 @@ """Redaction applied to monitoring data before egress. -Secrets are always redacted, on every export path; no setting disables this. -Wraps ``agent/redact.py::redact_sensitive_text(force=True)`` and fails CLOSED: -if the redactor cannot run, the raw string is never emitted. +One unconditional scrub, no modes, no knobs. Every string that leaves the +process passes through ``redact_for_export``: -``redact_for_export(text, content_mode="pii")`` additionally scrubs e-mail -addresses, phone numbers, and UUID-shaped identifiers — the gateway -diagnostics path always uses this mode, so log-derived messages leave the -process with secrets AND PII already removed. + * Secrets first — wraps ``agent/redact.py::redact_sensitive_text(force=True)`` + plus bearer/token-shape patterns, and fails CLOSED: if the redactor cannot + run, the raw string is never emitted. + * PII second — e-mail addresses, phone numbers, and UUID-shaped identifiers + are rewritten to ``[email]`` / ``[phone]`` / ``[id]``. + +There is deliberately no setting to weaken this. The monitoring plane is +content-free by design, so the only free text it carries (log-derived +diagnostic messages) is always fully scrubbed. """ from __future__ import annotations import re -from typing import Any, Dict, List, Optional +from typing import Optional -# Content-redaction strengths for any content that IS exported. -CONTENT_NONE = "none" # drop content entirely (structural telemetry only) -CONTENT_PII = "pii" # codec-aware PII redaction on exported content -CONTENT_MODES = {CONTENT_NONE, CONTENT_PII} +# ── secret shapes (belt-and-suspenders on top of agent/redact.py) ─────────── +_BEARER_RE = re.compile(r"\bBearer\s+[A-Za-z0-9._~+\-/]+=*", re.IGNORECASE) +_TOKEN_RE = re.compile( + r"\b(xox[baprs]-[A-Za-z0-9-]+|sk-[A-Za-z0-9_-]{8,}|gh[pousr]_[A-Za-z0-9_]{8,})\b" +) +_SECRET_LITERAL_RE = re.compile(r"\*{3,}") +_BEARER_RESIDUE_RE = re.compile(r"\bBearer\s+\[[^\]]+\]", re.IGNORECASE) -# ── PII patterns (applied only in CONTENT_PII mode, on content that is exported) ── +# ── PII shapes ─────────────────────────────────────────────────────────────── _EMAIL_RE = re.compile(r"[A-Za-z0-9._%+\-]+@[A-Za-z0-9.\-]+\.[A-Za-z]{2,}") # E.164-ish and common separators; conservative to avoid nuking code/IDs. _PHONE_RE = re.compile( r"(? Optional[str]: +def _secret_redact(text: str) -> str: """Always-on secret redaction. force=True so user config can't disable it.""" - if text is None: - return None try: from agent.redact import redact_sensitive_text - return redact_sensitive_text(str(text), force=True) + out = redact_sensitive_text(text, force=True) except Exception: # Fail CLOSED: if the redactor can't run, do not emit the raw string. return "[redaction-unavailable]" + out = _BEARER_RE.sub("[redacted]", out) + out = _TOKEN_RE.sub("[redacted]", out) + out = _SECRET_LITERAL_RE.sub("[redacted]", out) + out = _BEARER_RESIDUE_RE.sub("[redacted]", out) + return out -def _pii_redact(text: str) -> str: - text = _EMAIL_RE.sub("[email]", text) - text = _UUID_RE.sub("[id]", text) - text = _PHONE_RE.sub("[phone]", text) - return text - - -def redact_for_export( - text: Optional[str], - *, - content_mode: str = CONTENT_NONE, -) -> Optional[str]: - """Redact a single content string for export. - - Secrets are ALWAYS stripped. Then PII is stripped when content_mode is 'pii'. - Callers gate *whether content is exported at all* via telemetry.trajectories - (see ``content_export_enabled``); this function only scrubs content that the - caller has already decided to export. - """ - redacted = _secret_redact(text) - if redacted is None: +def redact_for_export(text: Optional[str]) -> Optional[str]: + """Scrub a string for egress: secrets, then PII. Unconditional.""" + if text is None: return None - if content_mode == CONTENT_PII: - redacted = _pii_redact(redacted) - return redacted + out = _secret_redact(str(text)) + out = _EMAIL_RE.sub("[email]", out) + out = _UUID_RE.sub("[id]", out) + out = _PHONE_RE.sub("[phone]", out) + return out __all__ = [ - "CONTENT_NONE", - "CONTENT_PII", - "CONTENT_MODES", "redact_for_export", ] diff --git a/hermes_cli/config.py b/hermes_cli/config.py index f0f6a43c31..bd07d7d6a4 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2175,60 +2175,6 @@ DEFAULT_CONFIG = { "redact_pii": False, # When True, hash user IDs and strip phone numbers from LLM context }, - # Telemetry & observability. Three settings, isolated from each other: - # local — full-fidelity local observability the user owns (real - # models, providers, tool names). Default ON. - # aggregate — opt-in metadata to Nous (no uploader ships yet). Default OFF. - # trajectories — content trajectories for training. Separate consent (not here yet). - # - # Enterprise locking: any telemetry.* key can be pinned by an administrator via - # the managed-scope layer (/etc/hermes/config.yaml), which wins over the user's - # value on a per-key basis. There is no telemetry-specific policy block — pin - # e.g. `telemetry.allow_aggregate: false` in managed scope to hard-forbid egress. - "telemetry": { - # Local telemetry: event log + SQLite index in state.db. The user's own data; - # never leaves the machine unless they export it or opt into aggregate metrics. - "local": True, - # Hard gate for aggregate metrics. When False, aggregate metrics are off - # regardless of consent_state. Intended to be pinned False by an administrator - # via managed scope on locked-down or air-gapped deployments. Default True - # (consent_state still governs the opt-in). - "allow_aggregate": True, - # Aggregate-metrics consent — the single source of truth for the opt-in: - # "unknown" (no choice made — never uploads), "local" (declined), - # "aggregate" (opted in). Set with `hermes config set telemetry.consent_state` - # or a managed-scope pin. Non-interactive installs sit at "unknown". - "consent_state": "unknown", - # Stable install identifier (aggregate metrics only). Empty string means "mint a - # fresh UUID on first use"; clear it to rotate. Never sent by local telemetry. - "install_id": "", - # Local event-log retention before rotation (days). Local telemetry only. - "retention_days": 90, - # Keep secret redaction on even at full local capture (a SIEM full of live - # credentials is a bigger attack target). Admin may override via managed scope. - "redact_secrets": True, - # Content redaction for exports / support bundles: "none" | "pii". - "content_redaction": "none", - # Trajectories: full message content / reasoning / raw tool args. - # Off by default. When enabled, full content becomes exportable to the - # configured destination — always secret-redacted, and PII-redacted per - # content_redaction. This is the consent gate for content export and is - # admin-pinnable via managed scope. It does not enable any upload. - "trajectories": { - "enabled": False, - }, - # Exporters. The OTLP exporter sends to a configured Collector endpoint; - # endpoint headers reference environment variable names rather than inline - # secrets. - "export": { - "otlp": { - "enabled": False, - "endpoint": None, - "headers_env": {}, # {"Authorization": "MY_OTLP_TOKEN_ENVVAR"} - }, - }, - }, - # Text-to-speech configuration # Each provider supports an optional `max_text_length:` override for the # per-request input-character cap. Omit it to use the provider's documented @@ -3107,11 +3053,6 @@ DEFAULT_CONFIG = { "service.name": "hermes-gateway", "deployment.environment": "production", }, - "redaction": { - "enabled": True, - "include_stack_summary": True, - "include_raw_stack": False, - }, }, # OTLP destination. headers_env maps header names to ENVIRONMENT # VARIABLE NAMES (never secret values); values are read from the diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 2cef49a220..3157d78870 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -14340,10 +14340,8 @@ def cmd_monitoring(args): print(f" Diagnostic events: {'on' if gh.get('diagnostic_events_enabled', True) else 'off'}") print(f" Warning/error logs: {'on' if gh.get('warning_error_events_enabled', True) else 'off'} " f"(interval {gh.get('logs_export_interval_seconds', 5)}s)") - red_raw = gh.get("redaction") - red: dict = red_raw if isinstance(red_raw, dict) else {} - print(f" Redaction: {'on' if red.get('enabled', True) else 'OFF'} " - f"(secrets/PII scrubbed in-process before egress)") + print(" Redaction: always on " + "(secrets/PII scrubbed in-process before egress; not configurable)") endpoint = otlp.get("endpoint") or "" if otlp.get("enabled") and endpoint: print(f" OTLP endpoint: {endpoint}") diff --git a/scripts/observability/gateway_health_export_probe.py b/scripts/observability/gateway_health_export_probe.py index 9b6de1b860..1635f46488 100644 --- a/scripts/observability/gateway_health_export_probe.py +++ b/scripts/observability/gateway_health_export_probe.py @@ -45,7 +45,6 @@ def main() -> None: "service.name": "hermes-gateway-smoke", "deployment.environment": "local-smoke", }, - "redaction": {"enabled": True, "include_raw_stack": False}, }, "export": { "otlp": { diff --git a/tests/monitoring/test_export_redaction.py b/tests/monitoring/test_export_redaction.py index 0a5beaff90..1dede385a5 100644 --- a/tests/monitoring/test_export_redaction.py +++ b/tests/monitoring/test_export_redaction.py @@ -1,56 +1,69 @@ -"""Export redaction pipeline tests — the security-critical layer. +"""Export redaction tests — the security-critical layer. Invariants: - * Secrets ALWAYS stripped, every export path, no flag disables it. + * One unconditional scrub: secrets AND PII, no modes, no knobs. * Fails CLOSED: if the redactor can't run, the raw string is never emitted. - * PII (emails, phones, UUID-shaped ids) stripped in 'pii' mode — the mode - the gateway diagnostics path always uses. + * Structure (subsystem names, error codes) survives; free-text PII does not. """ from __future__ import annotations +from unittest import mock + import agent.monitoring.redaction as R -def test_secret_always_stripped_in_none_mode(): +def test_secret_key_always_stripped(): fake_key = "sk-ant-api03-" + "A" * 24 # constructed to dodge literal-scrubbers - text = f"calling with key {fake_key} and moving on" - out = R.redact_for_export(text, content_mode=R.CONTENT_NONE) + out = R.redact_for_export(f"calling with key {fake_key} and moving on") assert out is not None assert fake_key not in out -def test_secret_always_stripped_in_pii_mode(): - fake_token = "ghp_" + "0123456789abcdef" * 2 + "0123" - text = f"token {fake_token} leaked" - out = R.redact_for_export(text, content_mode=R.CONTENT_PII) +def test_token_shapes_stripped(): + ghp = "ghp_" + "0123456789abcdef" * 2 + "0123" + slack = "xoxb-" + "123456789012-abcdefABCDEF" + out = R.redact_for_export(f"token {ghp} and {slack} leaked") assert out is not None - assert fake_token not in out + assert ghp not in out + assert slack not in out + assert "[redacted]" in out + + +def test_bearer_header_stripped(): + out = R.redact_for_export("Authorization: Bearer abc.def-ghi_jkl") + assert out is not None + assert "abc.def-ghi_jkl" not in out def test_none_passthrough(): - assert R.redact_for_export(None, content_mode=R.CONTENT_NONE) is None + assert R.redact_for_export(None) is None -def test_pii_mode_strips_email_phone_uuid(): +def test_pii_always_stripped(): text = ("reach alice@example.com or +1 415 555 0100, " "install 123e4567-e89b-12d3-a456-426614174000") - out = R.redact_for_export(text, content_mode=R.CONTENT_PII) + out = R.redact_for_export(text) assert out is not None assert "alice@example.com" not in out assert "426614174000" not in out assert "[email]" in out assert "[id]" in out + assert "[phone]" in out -def test_none_mode_keeps_ordinary_words(): - out = R.redact_for_export("just ordinary words", content_mode=R.CONTENT_NONE) - assert out == "just ordinary words" +def test_ordinary_words_survive(): + assert R.redact_for_export("just ordinary words") == "just ordinary words" -def test_pii_mode_preserves_non_pii_structure(): - text = "platform.slack entered fatal after auth_failed" - out = R.redact_for_export(text, content_mode=R.CONTENT_PII) +def test_structure_preserved(): + out = R.redact_for_export("platform.slack entered fatal after auth_failed") assert out is not None assert "platform.slack" in out assert "auth_failed" in out + + +def test_fails_closed_when_redactor_unavailable(): + with mock.patch("agent.redact.redact_sensitive_text", side_effect=RuntimeError): + out = R.redact_for_export("secret sauce sk-live-key") + assert out == "[redaction-unavailable]" diff --git a/tests/monitoring/test_gateway_health_export.py b/tests/monitoring/test_gateway_health_export.py index 8c1a9ef851..3fe8ff190b 100644 --- a/tests/monitoring/test_gateway_health_export.py +++ b/tests/monitoring/test_gateway_health_export.py @@ -14,8 +14,8 @@ def test_default_config_keeps_gateway_health_export_disabled(): assert cfg["warning_error_events_enabled"] is True assert cfg["export_interval_seconds"] == 60 assert cfg["logs_export_interval_seconds"] == 5 - assert cfg["redaction"]["enabled"] is True - assert cfg["redaction"]["include_raw_stack"] is False + # Redaction is always-on and deliberately NOT configurable. + assert "redaction" not in cfg def test_gateway_health_snapshot_maps_runtime_status_to_low_cardinality_metrics(): @@ -340,3 +340,25 @@ def test_gateway_diagnostic_log_handler_never_raises_on_malformed_record(): ) handler.emit(record) + + +def test_install_id_persists_across_calls(tmp_path, monkeypatch): + """A minted install id must survive restarts (service.instance.id continuity).""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + (tmp_path / "config.yaml").write_text("{}\n") + + import hermes_cli.config as cfg_mod + from agent.monitoring.policy import ensure_install_id + + first = ensure_install_id(cfg_mod.load_config()) + assert first and first != "unknown" + # Persisted: a fresh load (simulating a new gateway process) returns the same id. + second = ensure_install_id(cfg_mod.load_config()) + assert second == first + assert first in (tmp_path / "config.yaml").read_text() + + +def test_install_id_existing_value_wins(monkeypatch): + from agent.monitoring.policy import ensure_install_id + + assert ensure_install_id({"monitoring": {"install_id": "keep-me"}}) == "keep-me"