feat(mesh): security hardening — authentication, validation, rate limits, lockout, audit integrity

[cagataycali]

Mesh Security Hardening

Closes the 11 attack vectors identified in the strands_robots mesh pentest scope (PR #101). Adds a single security boundary between the wire and the Mesh API; tightens IoT policies, audit integrity, CA pinning, and the LLM-facing tool. Five rounds of review feedback applied; 667 mesh tests pass, ruff + mypy clean, no public-API breaks.

1. Threat surface (before)

flowchart TB
    subgraph LAN["Zenoh LAN multicast"]
        A1["Any device on Wi-Fi"]
        A2["Compromised IoT camera"]
    end
    subgraph CLOUD["AWS IoT Core"]
        B1["Stolen robot cert"]
        B2["Leaked claim cert"]
        B3["Misconfigured operator"]
    end
    subgraph LLM["LLM agent context"]
        C1["Injected prompt"]
        C2["Poisoned RAG document"]
    end

    R[("Robot fleet<br/>cmd / state / camera")]:::target

    A1 -->|"1. publish to /cmd"| R
    A1 -->|"2. replay captured msg"| R
    A1 -->|"3. fake policy_host=evil"| R
    LAN <-->|"4. dual-path double-exec"| CLOUD
    B1 -->|"6. tamper audit log"| R
    B1 -->|"9. spoof presence"| R
    A1 -->|"10. flood /cmd"| R
    B1 -->|"11. CA-substitution MITM"| R
    A1 -->|"7. capture camera URLs"| R
    A1 -->|"8. command after E-stop"| R
    C1 -->|"5. broadcast emergency_stop"| R
    C2 --> LLM

    classDef target fill:#f8d7da,stroke:#721c24,color:#000
    classDef attack fill:#f8d7da,stroke:#721c24,color:#000
    class R target
    class A1,A2,B1,B2,B3,C1,C2 attack

Loading

11 vectors, 3 attacker tiers (LAN device, stolen cert, prompt injection). Single shared LAN with no auth, IoT policy wildcards, no replay protection, no audit signing, no LLM HITL.

2. Solution architecture

flowchart LR
    subgraph WIRE["Wire (Zenoh / IoT MQTT / Bridge)"]
        W1["Signed envelope<br/>HMAC-SHA256"]
        W2["Replay nonce<br/>+freshness window"]
        W3["Bridge dedup<br/>cross-transport"]
    end
    subgraph BOUNDARY["mesh/security.py - single boundary"]
        S1["sign_envelope /<br/>verify_envelope"]
        S2["validate_command<br/>action+kwarg allowlist"]
        S3["consume_peer_token<br/>per-sender bucket"]
        S4["LockoutError<br/>structured rejection"]
    end
    subgraph MESH["Mesh API"]
        M1["_put_signed<br/>(public alias: publish)"]
        M2["_dispatch + _exec_cmd"]
        M3["_estop_lockout<br/>fleet-wide"]
        M4["audit log<br/>per-record HMAC + seq"]
    end
    subgraph LLM["LLM tool"]
        L1["robot_mesh"]
        L2["Strands SDK<br/>HITL interrupt"]
        L3["per-action rate limit"]
    end

    WIRE --> BOUNDARY
    BOUNDARY --> MESH
    LLM --> BOUNDARY

    classDef defended fill:#d4edda,stroke:#155724,color:#000
    classDef boundary fill:#cfe2ff,stroke:#084298,color:#000
    class W1,W2,W3,M1,M2,M3,M4,L1,L2,L3 defended
    class S1,S2,S3,S4 boundary

Loading

Every outbound message goes through Mesh._put_signed (public alias publish for cross-module callers). Every inbound message goes through verify_envelope + validate_command. The audit log gains per-record HMAC + per-peer monotonic seq counter persisted to a sidecar with flock-protected cross-process safety.

3. Hardening matrix

#	Vector	Mitigation	Strict-mode guarantee
1	Unauthenticated LAN command publish	HMAC envelope on every publish	Forged commands rejected at verify_envelope
2	Replay of captured signed message	Per-message uuid4 nonce + 60s past / 5s forward window	Replay rejected; nonce cache scoped per-peer
3	Command injection via policy_host / policy_provider / policy_type / model_path / pretrained_name_or_path / server_address	All six gated by validate_command allowlists	Off-allowlist values rejected client-side AND server-side
4	Bridge double-execution	_CommandDeduplicator keyed by envelope nonce	Same envelope on Zenoh + IoT delivers exactly once
5	LLM-driven fleet actuation	tool_context.interrupt(...) HITL approval for emergency_stop / broadcast; per-action sliding-window rate limit	Operator must explicitly approve y / yes; declined approval does NOT consume a rate slot
6	Audit log tampering	Per-record HMAC + per-peer seq + sidecar persistence + O_NOFOLLOW symlink defence + bounded log rotation + flock-protected cross-process counter	verify_audit_integrity reports ok=False on any tamper, deletion, signing degrade, or symlink swap
7	Camera frame disclosure	Presigned URL TTL 60s default (1h hard cap); STRANDS_MESH_CAMERA_DISABLED kill switch; /ref topic carries signed envelope	Capturing /ref requires PSK in strict mode; URL expires before bulk-exfil window
8	Emergency-stop bypass	Persistent _estop_lockout engaged on fleet-wide safety/estop broadcast; only status / resume permitted; resume requires constant-time-compared override code; wire response is a single generic shape (no oracle leakage)	Receivers AND issuer enter lockout; resume returns {"status": "ok"} or {"status": "error", "error": "resume rejected"} only
9	Presence spoofing	Reuses HMAC envelope; verifier rejects unsigned presence in strict mode	Same as #1
10	Per-sender DoS via thread-spawn flood	TokenBucket per sender (default 20/60s, capped 1000 burst); registry self-prunes	_on_cmd rejects starved senders before spawning exec thread
11	CA-substitution MITM	SHA-256 pin (Amazon Root CA1) + 64 KiB body cap + socket.setdefaulttimeout per-recv deadline + on-disk re-use always raw-checked (break-glass applies only to download path)	Rogue CA detected at fetch AND at every subsequent re-use

4. New module: mesh/security.py

flowchart TD
    subgraph PUB["Public API"]
        F1["sign_envelope(payload)"]
        F2["verify_envelope(env, scope=peer_id)"]
        F3["validate_command(cmd)"]
        F4["consume_peer_token(sender)"]
        F5["enforce_peer_rate_limit(sender)"]
        F6["is_safe_policy_host /<br/>is_safe_policy_provider /<br/>is_safe_policy_type /<br/>is_safe_model_path /<br/>is_safe_server_address"]
    end
    subgraph EXC["Exception hierarchy"]
        E1["SecurityError"]
        E2["AuthenticationError"]
        E3["AuthorizationError"]
        E4["ValidationError"]
        E5["LockoutError"]
        E6["RateLimitError"]
    end
    E1 --> E2
    E1 --> E3
    E1 --> E4
    E1 --> E5
    E1 --> E6

    classDef api fill:#cfe2ff,stroke:#084298,color:#000
    classDef exc fill:#fff3cd,stroke:#856404,color:#000
    class F1,F2,F3,F4,F5,F6 api
    class E1,E2,E3,E4,E5,E6 exc

Loading

Single home for HMAC primitives, command schema, rate limiting, allowlists. Mesh._put_signed is the only outbound publish path; verify_envelope is the only inbound entry. The structured exception types feed typed audit events (response_hijack_rejected, command_rejected_lockout, validation_error, rate_limit_drop).

5. Permissive vs strict mode

flowchart LR
    A[STRANDS_MESH_PSK]
    B[STRANDS_MESH_REQUIRE_AUTH]

    A -- "unset" --> P[Permissive]
    A -- "set" --> S{REQUIRE_AUTH?}
    S -- "false" --> M[Mixed: signs out,<br/>accepts unsigned in]
    S -- "true" --> SS[Strict: rejects unsigned]

    P -. "WARNING once" .-> P
    M -.-> M
    SS --> R[Production]

    classDef warn fill:#fff3cd,stroke:#856404,color:#000
    classDef ok fill:#d4edda,stroke:#155724,color:#000
    classDef stop fill:#f8d7da,stroke:#721c24,color:#000
    class P,M warn
    class SS,R ok

Loading

• Permissive (default): zero-config for dev. Outgoing is enveloped but unsigned; incoming accepts legacy bare dicts. One-time WARNING logged. Safety handlers (_on_safety_estop / _on_safety_resume) refuse to act on remote events so a LAN attacker cannot weaponise fleet-wide DoS.
• Mixed: PSK set but REQUIRE_AUTH unset. Out-going is signed; verifier still accepts unsigned legacy peers during a fleet rollout window.
• Strict: PSK + REQUIRE_AUTH=true. Verifier rejects every un-enveloped or HMAC-mismatched message. This is the production posture. Documented residual risk: in permissive mode the replay-cache prune walk is fillable by unauthenticated peers; strict mode closes that surface entirely.

6. Operator quick-start

# Production posture - fail-closed.
export STRANDS_MESH_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_REQUIRE_AUTH=true
export STRANDS_MESH_AUDIT_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_OVERRIDE_CODE=$(openssl rand -hex 16)

# Recommended hardening knobs.
export STRANDS_MESH_PEER_RATE=20/60
export STRANDS_MESH_REPLAY_WINDOW=60
export STRANDS_MESH_AUDIT_MAX_BYTES=104857600   # 100 MiB rotation cap
export STRANDS_MESH_AUDIT_MAX_FILES=5
export STRANDS_MESH_CAMERA_PRESIGN_TTL=60       # 1 minute, max 3600
export STRANDS_MESH_DEDUP_TTL=120

# Tighten allowlists (per-deployment).
export STRANDS_MESH_POLICY_HOST_ALLOW=vla.internal,10.0.0.0/24
export STRANDS_MESH_HF_REPO_ALLOW=my-org,trusted-team/specific-model
export STRANDS_MESH_POLICY_TYPE_ALLOW=my_custom_policy

# Forensic verification.
python -c 'from strands_robots.mesh.audit import verify_audit_integrity; print(verify_audit_integrity())'
python -c 'from strands_robots.mesh.iot.provision import verify_ca_pin; from pathlib import Path; print(verify_ca_pin(Path.home() / ".strands_robots/iot/AmazonRootCA1.pem"))'

Variable	Purpose	Default	Hard cap
STRANDS_MESH_PSK	HMAC-signing key for envelopes	unset (permissive)	—
STRANDS_MESH_REQUIRE_AUTH	Reject unsigned envelopes	false	—
STRANDS_MESH_REPLAY_WINDOW	Past-tolerance seconds	60	600
STRANDS_MESH_PEER_RATE	Per-sender <count>/<seconds>	20/60	burst 1000
STRANDS_MESH_AUDIT_PSK	Audit-log signing key	unset	—
STRANDS_MESH_AUDIT_MAX_BYTES	Audit log rotation cap	100 MiB	10 GiB
STRANDS_MESH_AUDIT_MAX_FILES	Rotated audit copies kept	5	100
STRANDS_MESH_OVERRIDE_CODE	Resume-lockout code	unset	—
STRANDS_MESH_DEDUP_TTL	Bridge dedup window seconds	120	—
STRANDS_MESH_CAMERA_PRESIGN_TTL	Presigned URL lifetime	60	3600
STRANDS_MESH_CAMERA_DISABLED	Kill switch	false	—
STRANDS_MESH_DISABLE_CA_PIN	Break-glass for re-encoding proxy	false	download path only
STRANDS_MESH_POLICY_HOST_ALLOW	Extra hosts/CIDRs for VLA target	unset	—
STRANDS_MESH_POLICY_TYPE_ALLOW	Extra policy_type / policy_provider values	unset	—
STRANDS_MESH_HF_REPO_ALLOW	Extra HF org prefixes	unset	—
STRANDS_MESH_BRIDGE_TOPICS_PREFIX	Bridge filter prefix-match opt-in	response	—

7. LLM-tool surface

flowchart LR
    LLM["LLM agent"] -->|"robot_mesh(emergency_stop)"| T[robot_mesh]
    T -->|"_rate_limit_check<br/>(NO record yet)"| RL{slot available?}
    RL -- "no" --> R1[reject - audit]
    RL -- "yes" --> HITL["tool_context.interrupt"]
    HITL --> OP[Operator out-of-band]
    OP -- "y / yes / approve" --> A[approved]
    OP -- "anything else" --> D[declined]
    D -->|"NO slot consumed - R3-3"| LLM
    A -->|"_rate_limit_record"| EX[mesh.broadcast]

    classDef warn fill:#fff3cd,stroke:#856404,color:#000
    classDef ok fill:#d4edda,stroke:#155724,color:#000
    classDef bad fill:#f8d7da,stroke:#721c24,color:#000
    class T,HITL,EX ok
    class OP,A warn
    class R1,D bad

Loading

Approval is delivered out-of-band of the LLM's tool-argument flow, so prompt injection cannot smuggle approval. Only y / yes / approve / approved (case- and whitespace-insensitive, exact-match after strip().lower()) approve. Declined approvals do not consume rate-limit slots — three nuisance LLM prompts an operator declines do not lock out a real emergency_stop (R3-3).

8. Audit integrity flow

flowchart TD
    EV["Safety event"] --> NS["_next_seq peer_id"]
    NS -- "_SEQ_LOCK" --> FL["fcntl.flock seq lockfile"]
    FL --> RL["reload sidecar"]
    RL --> INC["increment + persist"]
    INC --> SR["_sign_record"]
    SR -->|"PSK snapshot ok"| OK["JSON line + fsync"]
    SR -->|"PSK degrade detected R4-2"| DROP["AuditPSKDegradedError - dropped"]
    OK --> RT["rotate if size > cap"]
    RT --> PUB["publish_safety_event<br/>signed envelope"]

    VA["verify_audit_integrity"] --> RD["read records"]
    RD --> VS["per-record HMAC verify"]
    VS --> GD["per-peer gap detection<br/>tamper-aware cursor"]
    GD --> RPT["report ok / bad_signature / missing_sig / sequence_gaps"]

    classDef ok fill:#d4edda,stroke:#155724,color:#000
    classDef warn fill:#fff3cd,stroke:#856404,color:#000
    class NS,FL,RL,INC,SR,OK,RT,PUB,VA,RD,VS,GD,RPT ok
    class DROP warn

Loading

Covers tampering of in-progress records (per-record HMAC), deletion of records (sequence gaps), restart-and-renumber (sidecar persistence + cross-process flock), env-var unset mid-run (PSK snapshot + AuditPSKDegradedError), symlink swap (O_NOFOLLOW + lstat reject), unbounded growth (rotation cap), tampered seq masking real gaps (bad_signature records do not advance per-peer cursor).

9. Backwards compatibility

• No PSK = permissive mode. Existing zero-config Zenoh-LAN setups keep working; outgoing carries an envelope but no signature; incoming accepts legacy bare dicts. One-time WARNING.
• API: only break is robot_mesh's confirm: bool parameter, replaced by the Strands SDK interrupt. Callers that previously passed confirm=True now omit it; the framework delivers the operator response.
• Bridge filter: STRANDS_MESH_BRIDGE_TOPICS is now exact-match by default; deployments that customised it and relied on the old prefix-walk MUST migrate to the new STRANDS_MESH_BRIDGE_TOPICS_PREFIX env var.
• Default STRANDS_MESH_POLICY_TYPE_ALLOW: includes the LeRobot policy class names (act, diffusion, tdmpc, etc.). Custom providers must be added explicitly.

10. Test summary

• 667 mesh tests pass, 2 skipped. Was 440 baseline before hardening.
• 8 new test files, 8 existing files updated, +250 regression tests across 5 review rounds.
• ruff check: clean. ruff format --check: clean. mypy on the 7 hardened modules: clean.
• tests/mesh/test_security_regressions.py carries one regression test per round-of-review fix; future refactors that re-introduce a flaw fail loudly.
• tests/mesh/test_pentest_findings.py carries 20 adversarial regression tests covering 16 pentest attack surfaces (5 vulnerabilities found and fixed during autonomous Phase-4 pentest, 11 verified-defended); each test docstring describes the threat model inline.

11. Files changed

Module	Role	Lines
strands_robots/mesh/security.py	new — single security boundary	955
strands_robots/mesh/audit.py	per-record HMAC + seq + rotation + flock	760
strands_robots/mesh/core.py	signed envelope chokepoint + lockout + publish alias	1298
strands_robots/mesh/sensors.py	sensor publishes routed through _put_signed	474
strands_robots/mesh/transport/bridge_transport.py	cross-transport dedup + exact-match filter	(lines unchanged)
strands_robots/mesh/iot/provision.py	CA pin + thing-name regex + per-recv timeout	764
strands_robots/mesh/iot/camera_offload.py	uses public Mesh.publish	268
strands_robots/tools/robot_mesh.py	HITL interrupt + per-action rate limit	515

Plus CHANGELOG.md with per-round details and README.md env-var matrix. Cycle-by-cycle pentest evidence lives in the commit messages on this branch and in the inline docstrings of tests/mesh/test_pentest_findings.py.

12. Follow-ups (NOT in this PR; tracked on the project board)

• CA pin time-bomb: surface verify_ca_pin via a mesh-doctor CLI subcommand or accept-multiple-pins list so AWS root rotation doesn't require a flag-day deployment.
• _on_cmd consumes the per-sender token before checking the lockout state; during a fleet-wide e-stop, valid signed cmds from a peer that hasn't yet heard the broadcast burn through their bucket and write command_rejected_lockout audit entries. Worth a fast-path early return on lockout to keep the audit log signal-to-noise high during incidents.

[yinsong1986]

Summary

Large, well-organised security pass on the mesh layer. The new mesh/security.py cleanly factors HMAC enveloping, replay protection, command validation, and per-sender rate limiting; the audit log gains sequence numbers + per-record HMAC + a verify_audit_integrity walker; the bridge transport now collapses cross-transport duplicates; IoT policies tighten Receive scope; the Amazon Root CA1 download is pinned and size-capped; and the robot_mesh LLM tool routes fleet-wide actions through a Strands SDK interrupt and a per-action sliding-window rate limiter. Tests are substantial (+143 across 8 new files) and existing tests have been threaded through the envelope shape.

Overall direction is sound and the threat-model framing is helpful, but a few concrete bugs in the multi-peer / single-process execution model leaked through, and the PR description oversells two of the mitigations relative to what the diff actually does. None of these block merging the broad security improvement, but they should be addressed before this is relied on as the production baseline.

Concerns

• Process-global state vs. multi-peer-per-process semantics. Both _NONCE_CACHE (security.py) and _SEQ_COUNTER (audit.py) are process-global, but the surrounding logic implicitly assumes one peer per process. In test fixtures and Simulation() configurations multiple Mesh peers commonly share a process; the resulting failure modes are visible (broadcast deduped to a single receiver as a "replay"; verify_audit_integrity reports phantom gaps). Two of the inline comments below pin specific instances. A regression test that constructs ≥2 Mesh instances in one process and exercises broadcast + audit verification would catch both.

• PR description / code semantics drift on the E-stop "persistent lockout". emergency_stop() only sets self._estop_lockout on the originating peer. Receivers process the broadcast {action: stop} and call r.stop_task() but do not subscribe to strands/safety/estop or engage their own lockout, so threat #8 ("after E-stop, follow-up commands could still execute") is only closed for the issuer. If that's intentional (single-issuer model), the description should say so; if not, receivers need to engage _estop_lockout on the safety/estop topic as well. There's no test pinning the receiver-side behaviour either way.

• Two outbound publish paths bypass _put_signed. Lines core.py:356 (_state_loop → strands/<peer>/state) and core.py:857 (publish_step → strands/<peer>/stream) call put(...) directly, contradicting the PR description bullet ("Heartbeat, e-stop, resume, camera, state, and broadcast publishes all flow through _put_signed"). State is enumerated in OperatorObserveFleet, so under STRANDS_MESH_REQUIRE_AUTH=true operators will reject every state update — and conversely the state/stream channels remain spoofable from any unauthenticated LAN peer. Either route them through _put_signed too, or note the carve-out in the description and the security.py module docstring.

• _dispatch returns a no-error error dict during lockout. When the lockout is engaged, {"error": "command rejected"} is returned (core.py:642) but _exec_cmd then publishes that as type="response" rather than type="error". Callers waiting on the RPC will see a successful-shaped response with an error field rather than the structured error path. Minor, but it makes the audit picture slightly less crisp.

What's good

• The __all__ in mesh/security.py is clean (no underscore exports), and exception hierarchy is well-shaped.
• Constant-time compares in the right places (envelope HMAC, override code).
• The CA pin verifies on every load including the already-downloaded path — refusing to use a mismatched on-disk file rather than silently overwriting it is the right call.
• validate_command is centralised, regex-allowlisted, and called both client-side (in robot_mesh) and server-side (in _exec_cmd) — defence in depth as AGENTS.md PR-#92 learnings prescribe.
• Bridge dedup keys on the full 256-bit SHA-256 fingerprint (no birthday-attack truncation) and isolates by (topic_key, ident).
• Thing-name regex and IoT policy scope tightening are textbook.

Verification suggestions

hatch run test -- tests/mesh/ -x
# Repro for the multi-peer nonce issue:
python -c "
import os; os.environ['STRANDS_MESH_PSK']='k'
from strands_robots.mesh import security as s
env = s.sign_envelope({'sender_id':'a','command':{'action':'status'}})
print(s.verify_envelope(env))           # ok
print(s.verify_envelope(env))           # AuthenticationError: replay
# Same envelope reaching two peers in one process: second peer's verify will raise.
"

[yinsong1986]

Per-peer gap detection is broken because _SEQ_COUNTER is process-monotonic, not per-peer. If two Mesh peers share one process and write interleaved events (peer-A:1, peer-B:2, peer-A:3), the per-peer walker sees [1, 3] for peer-A and reports a phantom gap — even though no record was deleted. The new test_sequence_per_process_not_per_peer (test_audit_integrity.py:48) actually proves the counter is process-global, but the test_sequence_gap_detected case only exercises a single peer so the bug doesn't surface.

Either maintain _SEQ_COUNTER per-peer (a dict[peer_id, int] keyed by peer_id argument under _SEQ_LOCK) so within-peer adjacency is meaningful, or change the gap-detection to test global monotonicity instead of per-peer adjacency. As written, verify_audit_integrity() will report ok: False against any healthy multi-peer process — which makes the forensic guarantee actively misleading.

Add a regression test with two distinct peer_ids interleaved that asserts sequence_gaps == [] on the clean path.

[yinsong1986]

_NONCE_CACHE is a single process-global dict, so the second verify_envelope call on the same envelope — from a different peer in the same process — will raise AuthenticationError("replay detected"). This collides with the broadcast pattern: every peer subscribes to strands/broadcast and runs _on_cmd → verify_envelope on the same payload. In any in-process multi-peer config (test harnesses, Simulation() with multiple robots) only the first peer to receive a broadcast will accept it; every other peer will drop it as a replay.

The cleanest fix is to scope the cache by recipient, e.g. expose _record_nonce(nonce, now, scope: str) and have Mesh._on_cmd pass self.peer_id so each peer maintains its own (or a shared) nonce window. Worth a regression test that constructs two Mesh instances in one process, broadcasts from a third, and asserts both peers' _on_cmd ran (rather than one being silently dropped).

The _PEER_RATE_LIMITS dict has the same multi-peer-per-process surface but the failure mode there is benign (shared budget); the nonce cache failure mode is functional message loss.

[yinsong1986]

The persistent lockout only protects the originating peer: self._estop_lockout is local, no Mesh subscribes to strands/safety/estop, and the broadcast {action: stop} only triggers r.stop_task() on receivers. So in a fleet, after E-stop the receivers complete their stop and then accept the very next command on cmd — which is exactly the threat the PR description claims this closes (#8: "after E-stop, follow-up commands could still execute … only status/resume permitted").

Either (a) subscribe to strands/safety/estop in start() and have its handler call self._estop_lockout.set() on receivers, with _resume_lockout driven by a corresponding strands/safety/resume (already published at core.py:958) — at which point the lockout is fleet-wide; or (b) downgrade the description to make the single-issuer scope explicit and document the receiver-side behaviour.

A test that broadcasts E-stop from peer-A and then sends {action: status} and {action: execute, ...} to peer-B, asserting only status succeeds, would pin whichever semantics you commit to.

[yinsong1986]

When the lockout is engaged, _dispatch returns {"error": "command rejected"} but _exec_cmd (above, line 605) then wraps it as type="response" with result={"error": "command rejected"}. RPC callers will see a normal-shaped response and have to peek at result.error to distinguish — and the rejected-cmd audit-log path is in _exec_cmd's ValidationError branch, not here, so a lockout-rejected command is not recorded in the audit log even though it's the most operationally interesting rejection.

Consider raising a dedicated exception (e.g. LockoutError in the security module) from _dispatch and handling it in _exec_cmd symmetrically with ValidationError: emit type="error" on the response topic and log_safety_event("command_rejected_lockout", ...) so the forensic record includes E-stop-bypass attempts.

[yinsong1986]

urllib.request.urlopen(_AMAZON_ROOT_CA1_URL) has no timeout= argument, so a stalled captive portal or hostile proxy can hang provision_robot() / provision_operator() indefinitely. The size cap on the body (line 664) only kicks in once data starts flowing.

Add timeout=15 (or similar) to the urlopen call and surface a clear RuntimeError on socket.timeout. Cheap, and aligns with the rest of the threat-model framing — a network adversary that can cause a hang has a foothold for downstream issues even if they can't substitute the CA bytes.

[cagataycali]

Thanks for the thorough review @yinsong1986. Pushed 03bcaa5 addressing every finding. Summary below; each item has a regression test in tests/mesh/test_security_regressions.py to pin the fix.

Multi-peer-per-process correctness

Nonce cache (process-global vs scoped)

You were right — _NONCE_CACHE was a single dict keyed by nonce alone, so the second peer in a multi-peer process rejected every broadcast as a replay. Repro reproduced exactly as you described.

Fix: cache key is now (scope, nonce) and verify_envelope(envelope, scope=...) accepts an optional scope. Mesh._on_cmd, _on_response, _on_presence, plus the two new safety handlers all pass scope=self.peer_id. Default scope is "" so existing single-verifier callers (and tests) keep working without changes. Regression test: test_multi_peer_broadcast_each_peer_accepts_independently.

Audit sequence counter (process-global vs per-peer)

Same structural problem in audit.py. _SEQ_COUNTER was process-global, so when two peers interleaved writes the per-peer adjacency check in verify_audit_integrity reported phantom gaps even on a clean log.

Fix: _SEQ_COUNTERS: dict[str, int] keyed by peer_id. Each peer has its own monotonic counter; per-peer adjacency is now genuine. _next_seq(peer_id) updated, single caller updated, regression test test_audit_seq_per_peer_no_phantom_gaps confirms verify_audit_integrity() returns ok=True, sequence_gaps=[] on an interleaved-writers log. The previous test_sequence_per_process_not_per_peer (which actually documented the buggy behaviour) is renamed to test_sequence_per_peer and rewritten to assert the correct invariant.

E-stop fleet-wide lockout

Correct call — the PR description over-claimed. The local _estop_lockout was issuer-only; receivers stopped the current task and then accepted the next command on cmd.

Fix: Mesh.start() now subscribes to strands/safety/estop and strands/safety/resume and routes them to two new handlers _on_safety_estop / _on_safety_resume. Receivers engage / clear their own _estop_lockout in response to verified events; in strict mode unsigned safety events are rejected so an unauthenticated LAN peer cannot trigger a fleet-wide lockout. Regression tests: test_safety_estop_topic_engages_remote_lockout (signed estop → receiver lockout engaged → signed resume → cleared) and test_unsigned_estop_rejected_in_strict_mode.

State / stream paths now signed

You caught two outbound paths I'd missed: _state_loop (strands/<peer>/state) and publish_step (strands/<peer>/stream) called raw put(...). Both now route through _put_signed. Regression test: test_state_loop_publish_goes_through_signed_envelope verifies the published payload is a versioned envelope with a nonce.

Structured lockout rejection

Good point on the response shape and the audit miss. Fixed:

• New LockoutError exception class in mesh/security.py (subclass of SecurityError, exported).
• _dispatch raises LockoutError("command rejected") instead of returning a dict.
• _exec_cmd handles LockoutError symmetrically with ValidationError — emits type="error" on the response topic and writes a command_rejected_lockout audit entry that includes the sender and action.
• The wire response stays generic (literal "command rejected", no timing fields).

Regression test test_lockout_raises_lockouterror_and_audits exercises the full path end-to-end.

CA download timeout

Fair callout. urlopen(_AMAZON_ROOT_CA1_URL, timeout=15) with an inline comment explaining why. Existing M4 tests still pass.

CodeQL findings

• test(mesh): test PKI helpers + shared conftest (1/9 of #195 split) #220, mesh(audit): tamper-evident audit log (HMAC + per-peer seq + rotation) (4/9 of #195 split) #221 — the two except Exception: pass blocks in core.py:559,601 (audit-log fallbacks) are now except Exception as audit_exc: logger.debug("... audit log unavailable: %s", audit_exc) with a comment explaining audit failures must never propagate up into the cmd handler.
• [mesh] Security hardening (Zenoh-native): umbrella tracker for split of #195 #219 — _PSK_WARNED: hoisted global _PSK_WARNED to the top of sign_envelope so static analysers see the name at function scope. The original code was correct (CodeQL's dataflow couldn't follow the conditional global); the restructure is purely cosmetic for the analyser's benefit.

Test summary after the fix

• tests/mesh/test_security_regressions.py → 40/40 (was 34, +6 review regressions).
• Total mesh tests: 609 passed, 2 skipped (was 603).
• ruff check, ruff format --check, mypy on the seven hardened modules: all clean.

Verification

Your proposed repro now produces the right behaviour:

>>> import os; os.environ['STRANDS_MESH_PSK']='k'
>>> from strands_robots.mesh import security as s
>>> env = s.sign_envelope({'sender_id':'a','command':{'action':'status'}})
>>> s.verify_envelope(env, scope="peer-a")   # peer-a accepts
{'sender_id': 'a', 'command': {'action': 'status'}}
>>> s.verify_envelope(env, scope="peer-b")   # peer-b ALSO accepts (was the bug)
{'sender_id': 'a', 'command': {'action': 'status'}}
>>> s.verify_envelope(env, scope="peer-a")   # same peer twice = real replay
strands_robots.mesh.security.AuthenticationError: replay detected for nonce …

Ready for another look when you have a moment.

[yinsong1986]

Summary

Large, well-scoped security hardening of the mesh layer: HMAC-signed envelopes with replay protection, action allowlist, per-sender token bucket, persistent e-stop lockout, audit-record HMAC + sequence numbers, cross-transport dedup, IoT policy scope tightening, Amazon Root CA1 pinning with a 64 KiB body cap, and a Strands SDK interrupt for fleet-wide LLM-tool actions. +143 tests (603 mesh tests passing). The changes are mostly additive and the permissive-mode fallback preserves zero-config back-compat.

Overall direction is sound. A handful of real gaps below.

What's good

• Single chokepoint (Mesh._put_signed) for outbound signing — easy to audit. Drops on signing failure when PSK is configured rather than silently downgrading.
• Replay cache is scoped ((scope, nonce) tuple) so multiple peers in one process don't false-positive on broadcasts.
• Constant-time compare for HMAC and override-code paths.
• Public verify_audit_integrity() is the right primitive for forensic checks.
• Bridge dedup uses full 256-bit fingerprints (no birthday-attack truncation) and shares one cache between Zenoh+IoT subscribers.
• _validate_thing_name defence-in-depth before AWS calls and filesystem interpolation matches the AGENTS.md learning on sanitising LLM-controllable inputs.
• Per-action sliding-window rate limit on the LLM tool, plus a real Strands SDK interrupt (not a confirm: bool).
• Operator IoT policy is now correctly explicit-per-suffix; previously a wildcard Receive scoped every operator credential to the entire fleet.

Concerns

• AGENTS.md violation: every STRANDS_* env var must be documented in README.md in the same PR. This PR introduces 11 new env vars (STRANDS_MESH_PSK, STRANDS_MESH_REQUIRE_AUTH, STRANDS_MESH_REPLAY_WINDOW, STRANDS_MESH_POLICY_HOST_ALLOW, STRANDS_MESH_PEER_RATE, STRANDS_MESH_AUDIT_PSK, STRANDS_MESH_OVERRIDE_CODE, STRANDS_MESH_DEDUP_TTL, STRANDS_MESH_CAMERA_PRESIGN_TTL, STRANDS_MESH_CAMERA_DISABLED, STRANDS_MESH_DISABLE_CA_PIN) and git diff upstream/main...HEAD -- README.md CHANGELOG.md is empty. Per AGENTS.md > Review Learnings (#86) > 'Env Vars': "Document every env var in README.md… The list is the single source of truth for users." Also a missing CHANGELOG entry for what is effectively the security headline of the release.

• Sensor + safety-event publishes still use raw put(), not _put_signed. strands_robots/mesh/sensors.py (unchanged in this PR) publishes pose / health / imu / odom / lidar / hand / map-info AND publish_safety_event via plain put(). The PR description claims "Heartbeat, e-stop, resume, camera, state, and broadcast publishes all flow through _put_signed" — true, but publish_safety_event (called from Mesh.emergency_stop itself, core.py:995) writes to strands/{peer}/safety/event, which is the topic the IoT bootstrap rule mirrors into the DynamoDB audit table (strands_robots/mesh/iot/bootstrap.py:339). Means an attacker (or any unsigned-mode peer) can inject fake safety events into the cloud forensic table even with strict-mode signing turned on. Worth either a follow-up issue or routing safety events through _put_signed in this PR.

• Audit seq is in-memory per process — sequence-gap detection cannot survive a restart. _SEQ_COUNTERS is a plain dict reset to empty on import (audit.py:85). After a process restart, peer X's seq starts at 1 again. A compromised process that deletes records and restarts produces a clean verify_audit_integrity() output. The threat model document calls out "compromised process can edit/truncate the log" — restart-and-renumber is a special case of truncation that the current design doesn't catch. Either persist the counter alongside the log file or document the limitation in verify_audit_integrity's docstring ("gap detection is per-process; restarts cannot be distinguished from deletion").

• Permissive-mode safety/resume will accept un-authenticated lockout clears. When STRANDS_MESH_PSK is unset (the documented default for back-compat), _on_safety_resume only checks the legacy un-enveloped path and clears the lockout. So the e-stop lockout — the headline safety control of this PR — is effectively soft in the default config. The PR header should make this explicit ("e-stop lockout is only secure with STRANDS_MESH_PSK + STRANDS_MESH_REQUIRE_AUTH=true") and the Mesh.emergency_stop() docstring should note the same so operators don't think they're protected when they're not.

• Bare except Exception: clauses in new code. Several added in this PR violate the AGENTS.md > Review Learnings (#86) > 'Exception Clauses Must Be Narrow' rule (e.g. tools/robot_mesh.py:257 around tool_context.interrupt, core.py:660 around dispatch). They mask programming errors as "interrupt unavailable" / "dispatch error".

Verification suggestions

# Confirm all sensor/safety-event publishes do (or don't) go via _put_signed:
rg -n 'put\(f?"strands/' strands_robots/mesh/

# Sequence-gap survives a restart? (it shouldn't, today)
STRANDS_MESH_AUDIT_PSK=test python -c '
from strands_robots.mesh import audit
audit.log_safety_event("e","p",{"i":1}); audit.log_safety_event("e","p",{"i":2})
'
STRANDS_MESH_AUDIT_PSK=test python -c '
from strands_robots.mesh import audit
# delete record 2 by hand…
print(audit.verify_audit_integrity())  # gap detection should report (2,3) — does it?
'

# Spot-check that no host paths or emojis crept into wire strings:
rg -nP '[^\x00-\x7F]' strands_robots/mesh/security.py strands_robots/mesh/audit.py

Run the new tests and a smoke of the robot_mesh tool against a 2-peer Zenoh setup with STRANDS_MESH_PSK=... set on one side and unset on the other to confirm the asymmetric strict/permissive interop matches the intended threat model.

[yinsong1986]

Permissive-mode lockout clear is unauthenticated. _on_safety_resume only validates the envelope — it does not check the override code (the comment at L799-801 says "the publisher already validated the operator override code locally; we trust the signed envelope"). When STRANDS_MESH_PSK is unset (the documented permissive-mode default for back-compat), verify_envelope accepts un-enveloped legacy payloads and the only thing this handler then does is self._estop_lockout.clear(). Result: the headline e-stop lockout fix is effectively soft in the default zero-config Zenoh-LAN configuration — any peer on the LAN can publish strands/safety/resume and clear every other peer's lockout.

At minimum, document this prominently in Mesh.emergency_stop()'s docstring and the README configuration section ("e-stop persistent lockout requires STRANDS_MESH_PSK + STRANDS_MESH_REQUIRE_AUTH=true"). Better: refuse to clear lockout when neither PSK nor strict-auth is configured (if not _security.psk_configured() and not _security.auth_required(): return).

[yinsong1986]

Sequence-gap detection cannot survive a process restart. _SEQ_COUNTERS (audit.py:85) is an in-memory dict that resets on every import, so each process restart starts every peer's seq back at 1. A compromised process that deletes records and restarts yields a clean verify_audit_integrity() result, defeating the "records-were-deleted" half of the docstring's threat model.

Two options:

1. Persist the counter — write _SEQ_COUNTERS to a sidecar file under audit_log_path().parent and reload it on first _next_seq call. Then the seq is monotonic across restarts.
2. Document the limitation — update both this function's docstring and the module docstring to call out that gap detection is per-process and restarts cannot be distinguished from deletion. Rename the field to proc_seq so downstream readers don't read more guarantee into it than there is.

Also minor: bad_signature records still update last_seq_by_peer on L312, so a tampered record's seq value can mask a real gap on the next legit record. Should continue after counting it as bad.

[yinsong1986]

Public verify_ca_pin() silently returns True when the break-glass env var is set. verify_ca_pin(ca_path) (L695) calls _verify_ca_bytes(body), which short-circuits to return True when STRANDS_MESH_DISABLE_CA_PIN=true (L689-691). The PR description and docstring describe verify_ca_pin as the public helper for ops scripts to spot-check pinned CAs in the field — but with the env var set, it returns True for any file content, including a tampered or rogue CA. That's the exact case ops would be checking for.

Split the two paths: keep the env-var bypass in _ensure_ca (so provisioning continues to work behind a re-encoding proxy), but have verify_ca_pin always do the raw hash compare so a forensic tool sees the truth. Or, at minimum, log the WARNING from inside verify_ca_pin so a human running the helper notices the bypass.

[yinsong1986]

Bare except Exception masks programming errors as "interrupt unavailable". AGENTS.md > Review Learnings (#86) > 'Exception Clauses Must Be Narrow' explicitly forbids except Exception for non-recovery code paths. The intent here is to detect "the host is calling this tool directly without an interrupt-capable context" — but as written this also swallows AttributeError from typos in tool_context.interrupt(...) arguments, TypeError from a future SDK breaking change, etc., reporting them all as "interrupt unavailable" instead of surfacing the real bug.

Fix: narrow to whatever the SDK actually raises in the no-interrupt-context path (NotImplementedError, RuntimeError, or a Strands-specific subclass — check strands.types.tools for the contract). Same critique applies to the catch-all at core.py:660 (except Exception as exc around self._dispatch(cmd)) — leak str(exc) of internal errors onto the wire as {"error": "<exc>"}.

[yinsong1986]

emergency_stop() publishes the safety event via raw put(), bypassing the signed envelope. publish_safety_event (defined in strands_robots/mesh/sensors.py:448, unchanged in this PR) calls put(f"strands/{self.peer_id}/safety/event", event) directly — it doesn't go through _put_signed. That topic is then mirrored into a DynamoDB audit table by the IoT bootstrap rule (strands_robots/mesh/iot/bootstrap.py:339).

Net effect: even with strict-mode signing turned on (PSK + STRANDS_MESH_REQUIRE_AUTH=true), an attacker on the LAN can publish unsigned strands/<spoofed_peer>/safety/event payloads and pollute the cloud audit trail. The very emergency_stop action this PR is hardening uses the unsigned path for its own audit emission.

Fix: route publish_safety_event through _put_signed (sensors.py is the right place; the mixin already has self.peer_id, just needs self._put_signed(...) from the Mesh side). Additionally, this is one of seven put(...) call sites in sensors.py that are still unsigned — pose / IMU / odom / lidar / hand / health are arguably privacy-sensitive too. Consider an inline TODO or a follow-up issue if scope-discipline matters more than completeness here.

[cagataycali]

Thanks for the second pass @yinsong1986. Pushed c93855e addressing every finding from review #2. Each fix has a regression test in tests/mesh/test_security_regressions.py.

CRITICAL #1 — sensor + safety-event publishes now signed

You were right: publish_safety_event and the seven sensor topics in sensors.py (pose/imu/odom/lidar/hand/health/map-info) all called transport.put() directly, bypassing _put_signed. Since safety/event is mirrored to a DynamoDB audit table by the IoT bootstrap rule, an unsigned LAN peer could pollute the cloud forensic trail even with strict-mode signing on. Fix: all eight put(...) sites in sensors.py now route through self._put_signed. Camera /ref publishes in iot/camera_offload.py likewise. Regression: test_publish_safety_event_uses_signed_envelope and test_camera_ref_uses_signed_envelope.

CRITICAL #2 — permissive-mode safety handlers refuse remote events

Good catch — in permissive mode the receiver previously accepted any unsigned strands/safety/resume and cleared the lockout, so any LAN peer could silently undo every peer's e-stop. Fix: _on_safety_estop and _on_safety_resume now refuse to act when neither PSK nor strict-auth is configured. Local Mesh.emergency_stop() / _resume_lockout(code) still work for local operators; only the LAN fan-out is gated. Documented in Mesh.emergency_stop()'s docstring and the new Mesh security section in README.md. Regression: test_safety_handlers_refuse_when_unauthenticated.

HIGH #3 — env vars + CHANGELOG

Clean AGENTS.md miss. README's Configuration table gains 11 new STRANDS_MESH_* entries (PSK, REQUIRE_AUTH, REPLAY_WINDOW, POLICY_HOST_ALLOW, PEER_RATE, AUDIT_PSK, OVERRIDE_CODE, DEDUP_TTL, CAMERA_PRESIGN_TTL, CAMERA_DISABLED, DISABLE_CA_PIN). New 'Mesh security' subsection walks through permissive vs strict modes and the e-stop authentication policy. CHANGELOG.md gains the full Unreleased entry covering the security headline.

HIGH #4 — audit sequence counters survive restart

_SEQ_COUNTERS was an in-memory dict reset on every import — restart-and-renumber gave a clean verify_audit_integrity() against a deleted-record log. Fix: counters now persist to a sidecar mesh_audit.seq.json next to the audit log, atomically rewritten via temp-file + os.replace, reloaded on first _next_seq call. Writes are fail-soft (WARNING on OSError) so audit persistence cannot break the safety code path. Regression: test_seq_counter_persists_across_simulated_restart.

MEDIUM #5 — narrow exception clause

tools/robot_mesh.py:257 was except Exception, swallowing programming errors as 'interrupt unavailable'. Narrowed to except RuntimeError (the actual SDK signal for no-agent-context). InterruptException continues to propagate so the approval-pause flow works. Regression: test_robot_mesh_interrupt_propagates_programming_errors.

MEDIUM #6 — verify_ca_pin() always tells the truth

Split the two paths: _verify_ca_bytes (provisioning) keeps the STRANDS_MESH_DISABLE_CA_PIN break-glass for re-encoding proxies; verify_ca_pin (forensic) always does the raw hash compare so a tampered CA can never appear valid to ops scripts. New _hash_matches_pin helper centralises the pure check. Regression: test_verify_ca_pin_ignores_break_glass_envvar.

LOW #7 — bad_signature records do not advance per-peer cursor

verify_audit_integrity used to update last_seq_by_peer even for records that failed signature verification, so an attacker could forge a record's seq to mask a real gap. Bad records now continue before the cursor update. Regression: test_bad_signature_does_not_mask_gap.

CodeQL #219 — _PSK_WARNED hoist

Refactored the conditional global into a dedicated _warn_psk_unset_once() helper. sign_envelope no longer touches module state directly, satisfying CodeQL's dataflow analysis.

Static-analysis cleanup

While threading _put_signed through the sensor mixin, mypy started flagging it as undefined on SensorLoopsMixin. Added a typed stub (def _put_signed(self, ...) -> None: ...) on the mixin so the type-checker sees the contract; at runtime Mesh._put_signed shadows it via MRO (verified by test_state_loop_publish_goes_through_signed_envelope).

Test summary

• tests/mesh/test_security_regressions.py: 49/49 passing (was 40, +9 review regressions).
• Full mesh suite: 624 passed, 2 skipped (was 618).
• hatch run lint (ruff + format + mypy): all clean — Success: no issues found in 192 source files.

Ready for another look.

[yinsong1986]

Summary

This is a substantial security-hardening pass on the mesh layer: HMAC-signed envelopes with replay protection, command validation + policy_host allowlist, per-sender token-bucket rate limit, persistent e-stop lockout, signed audit log with sequence-counter sidecar, Amazon Root CA1 SHA-256 pin, IoT topic-policy scope tightening, cross-transport dedup, and a HITL interrupt for the LLM tool. The PR description, threat model, and test coverage (+143 tests) are all very strong and the permissive-mode back-compat story is well-thought-out.

The big-ticket primitives look correct. The two threads I'd push back on are (a) one of the eleven advertised attack vectors ("command injection via attacker-controlled policy_host") is only partially closed, and (b) some interaction issues between the new layers (rate limit vs. interrupt; broad exceptions wrapping the new structured-error paths). Inline comments below.

What's good

• Clean module boundary in mesh/security.py; _put_signed as the single signing chokepoint is the right shape.
• Per-peer scope argument on verify_envelope correctly handles multi-peer-in-one-process broadcast replays.
• Asymmetric freshness window (60 s past, 5 s forward) and constant-time compare_digest everywhere it matters.
• Audit-log sidecar persistence across restarts is exactly what defeats the "compromised process renumbers from 1" attack — nicely thought through.
• _ensure_ca re-verifies the pin on every load (not just download), and verify_ca_pin deliberately ignores the break-glass env var. Good split.
• Lockout response is a generic "command rejected" with no leak of duration / engaged state to the remote.
• IoT policy scope tightening is the right kind of change — least-privilege topic patterns instead of strands/* wildcards.
• 8 new test files; behavioural fixes pinned by regression tests (the AGENTS.md pattern).

Concerns

• Threat-vector #3 is not fully closed. validate_command enforces a policy_host allowlist, but the receiver's _dispatch (core.py:711-715, pre-existing context — not in this diff) forwards model_path, pretrained_name_or_path, server_address, policy_type from the wire straight into _execute_task_sync / start_task with no validation. An authenticated peer (or a leaked PSK) can therefore still steer a robot at an arbitrary HuggingFace model identifier — including one that triggers trust_remote_code paths if STRANDS_TRUST_REMOTE_CODE=1. The PR's threat-vector table claims "command injection via attacker-controlled policy_host" is mitigated; in practice the same class of attack is reachable via these untriaged kwargs. Worth either (a) extending validate_command to allowlist these fields too, or (b) explicitly documenting that model_path/pretrained_name_or_path over the mesh requires a separate trust boundary.
• _warn_psk_unset_once is one-shot for the process lifetime. That's the right UX choice for production but it means an env-var rotation that drops the PSK at runtime will publish unsigned messages silently after the first warning. Acceptable, but worth a sentence in the docstring.
• No backward-compatibility migration note for the audit-log sidecar. Existing deployments will see a fresh mesh_audit.seq.json appear next to the audit log; the file gets 0o600. Worth flagging in the CHANGELOG that the sidecar is process-monotonic and that running multiple processes against the same STRANDS_MESH_AUDIT_DIR will produce sequence gaps that verify_audit_integrity reports as suspicious — that's by design but it will confuse first encounters.
• Scope discipline is good — every change traces back to one of the eleven listed vectors. No drive-by edits I noticed.

Verification suggestions

# Strict-mode round trip with a real PSK + replay rejection.
STRANDS_MESH_PSK=test-psk STRANDS_MESH_REQUIRE_AUTH=true \
    hatch run pytest tests/mesh/test_security.py tests/mesh/test_security_regressions.py -x

# Confirm verify_audit_integrity flags a tampered record + a deleted record.
STRANDS_MESH_AUDIT_PSK=audit-psk hatch run pytest tests/mesh/test_audit_integrity.py -x

# Bridge dedup with both transports active.
hatch run pytest tests/mesh/test_bridge_dedup.py -x

# Spot-check the e-stop lockout path can survive a process restart with the
# sidecar — kill the test process between an estop and a resume and confirm
# the seq counters resume from where they left off.

[yinsong1986]

This bare except Exception: is exactly the pattern AGENTS.md prohibits (Review Learnings (#86) > 'Exception Clauses Must Be Narrow'). It also shipping str(exc) straight onto the response topic at line 669 leaks internal exception detail to a remote (potentially attacker-controlled) caller — the equivalent LockoutError and ValidationError paths above were carefully designed to emit generic / sanitised text. Suggest narrowing to the specific dispatch failures you actually want to surface (AttributeError, KeyError, TypeError, etc.) and substituting a static "dispatch error" message on the wire while keeping exc only in the local log.

[yinsong1986]

bucket.consume(1.0) is called while still holding _PEER_RATE_LOCK, which serialises every per-sender rate-limit check across the whole process — defeating the point of having per-bucket locks. The TokenBucket is already thread-safe internally, so the registry lock should be released before consuming:

with _PEER_RATE_LOCK:
    bucket = _PEER_RATE_LIMITS.get(sender_id)
    if bucket is None:
        bucket = TokenBucket(capacity=burst, rate_per_s=rate_per_s)
        _PEER_RATE_LIMITS[sender_id] = bucket
    # GC pass...
return bucket.consume(1.0)   # outside the registry lock

No correctness bug — but at high command volumes across many peers this becomes a single-threaded chokepoint where all of _on_cmd queues up.

[yinsong1986]

_rate_limit_check runs before the HITL interrupt, so a declined approval still consumes a slot in the sliding-window bucket. With emergency_stop capped at 3/min, an operator who declines three accidental LLM-triggered prompts within a minute has now locked the agent out of issuing a real emergency stop until the window rolls. That's the opposite of the intended safety property — the rate limit exists to bound LLM-driven nuisance, not to inhibit a genuine emergency.

Suggest: only add to _RATE_HISTORY after the interrupt is approved (or when the action doesn't require an interrupt). If you want to bound declined-prompt spam, do it with a separate, narrower budget.

[yinsong1986]

When STRANDS_MESH_DISABLE_CA_PIN=true is set and a CA file already exists on disk, _verify_ca_bytes returns True unconditionally — so a rogue CA written by an earlier compromised provisioning run is silently re-used on every subsequent invocation, with only a WARNING in the log. The break-glass intent is to allow proxy environments to download a re-encoded cert; preserving an arbitrary on-disk file across restarts is a strictly worse posture.

Consider keeping the break-glass for the download path (lines 670–681) but always doing the raw _hash_matches_pin check on the existing-file branch — failed pin → RuntimeError regardless of the env var. Operators who set the override deliberately can still re-download once and proceed.

[yinsong1986]

validate_command checks policy_host, policy_port, instruction length, duration, and steps, but _dispatch at mesh/core.py:711-715 (pre-existing) also reads model_path, pretrained_name_or_path, server_address, policy_type from the same command dict and forwards them to _execute_task_sync / start_task. A peer that has authenticated (or leaked PSK) can therefore set pretrained_name_or_path="attacker-controlled-org/evil-model" and bypass the spirit of the policy_host allowlist entirely.

Since this lives in the validator's per-action schema, it would be cheap to add (e.g. allowlist regex on pretrained_name_or_path, allowlist of policy_type, and either reject or apply is_safe_policy_host to server_address). Threat-vector #3 in the PR description claims this is closed; in practice the equivalent attack is still open via these fields.

[yinsong1986]

Summary

This is a substantial security-hardening pass on the mesh layer: HMAC-signed envelopes with replay protection, command/host/model-path validation, per-sender token-bucket rate limiting, persistent emergency-stop lockout, signed audit log with sequence numbers, AWS IoT policy scope tightening, Amazon Root CA1 SHA-256 pinning, presigned-URL TTL clamping, bridge-mode cross-transport dedup, and a HITL-interrupt gate on the LLM-facing robot_mesh tool. The threat model is documented, env vars are catalogued in README, and the new modules are well-scoped (security.py is a clean home for the auth/validation/rate-limit primitives).

What's good

• Threat-vector → mitigation mapping in the PR description is concrete and traceable.
• Strict / permissive split is principled: permissive preserves zero-config dev, strict (STRANDS_MESH_PSK + STRANDS_MESH_REQUIRE_AUTH) is fully fail-closed; the safety-handler refusal in permissive mode (_on_safety_estop/_on_safety_resume) is exactly the right call to prevent fleet-wide DoS via unauthenticated estop spam.
• Constant-time compares (hmac.compare_digest) are used consistently for both signature verification and the override code.
• Round-3 split of _rate_limit_check / _rate_limit_record so that declined HITL approvals don't burn emergency_stop slots is a thoughtful safety property — tests/mesh/test_robot_mesh_security.py should pin that as a regression.
• 143 new tests, including round-trip coverage for audit integrity, replay attacks, dedup, CA pin, and IoT policy scope. Lint and mypy clean.
• AGENTS.md compliance is generally strong: structured error returns from the tool, no _-prefixed exports, no host paths in tests, lockout response intentionally generic to avoid leakage.

Concerns

Three summary-level items beyond the inline comments:

1. Permissive-mode wire format is permanently lossy for replay protection. When STRANDS_MESH_PSK is unset, verify_envelope still records nonces but accepts unsigned envelopes — a LAN attacker on a permissive-mode mesh can mint fresh envelopes with new nonces all day, so the replay window only protects against literal-payload replay, not forged commands. The README says this; a one-line explicit warning in security.py's module docstring would help operators not over-trust the permissive path.
2. Validation is at the receiver only for tell/send/broadcast payloads — the round-3 client-side validation in tools/robot_mesh.py is good, but a direct mesh.send(target, {...}) call from a non-tool caller skips it. That's by design (validation runs again at the receiver) but if a future refactor moves validate_command out of _exec_cmd, there's no compile-time guarantee the receive-side path stays gated. Worth a comment in _exec_cmd explicitly stating that this is the only chokepoint.
3. STRANDS_MESH_DISABLE_CA_PIN semantic split (download honors, on-disk does not) is correct but operationally subtle. When AWS rotates the root CA, every existing deployment hard-fails until an operator manually deletes AmazonRootCA1.pem and sets the override for a single re-fetch. There's no early-warning hook (e.g. a cron-friendly verify_ca_pin check) and the pin's date in the comment is the only signal that a rotation is overdue. Consider exposing a mesh.iot.provision.ca_pin_age_days() or similar so ops dashboards can flag it before the first failed provisioning run.

Verification suggestions

hatch run test -- tests/mesh/ -k 'security or audit_integrity or bridge_dedup or iot_ca_pin or iot_policy_scope or robot_mesh_security or camera_acl' -q

# Manual: replay-attack smoke
STRANDS_MESH_PSK=secret python -c "
from strands_robots.mesh.security import sign_envelope, verify_envelope
e = sign_envelope({'a': 1})
print(verify_envelope(e, scope='x'))            # ok
try: verify_envelope(e, scope='x')              # replay
except Exception as ex: print('replay caught:', ex)
"

# Manual: audit integrity after a forced corruption
STRANDS_MESH_AUDIT_PSK=k python - <<'PY'
import os, json, tempfile, pathlib
os.environ['STRANDS_MESH_AUDIT_DIR'] = tempfile.mkdtemp()
from strands_robots.mesh.audit import log_safety_event, audit_log_path, verify_audit_integrity
for i in range(3): log_safety_event('t', 'p', {'i': i})
p = audit_log_path()
lines = p.read_text().splitlines()
rec = json.loads(lines[1]); rec['payload']['i'] = 99
lines[1] = json.dumps(rec, sort_keys=True, separators=(',', ':'))
p.write_text('\n'.join(lines) + '\n')
print(verify_audit_integrity())   # expect bad_signature == 1, ok == False
PY

[yinsong1986]

policy_provider is not validated even though it selects which policy implementation runs.

Round 3 added gates for policy_type, pretrained_name_or_path, model_path, and server_address, but mesh/core.py:723 does:

policy_provider = cmd.get("policy_provider", "mock")
...
r._execute_task_sync(instruction, policy_provider, policy_port, policy_host, duration, **extra)

policy_provider is the registry key the executor uses to pick the policy class — it is the attacker-controlled selector, more so than policy_type. An authenticated peer (or a leaked PSK) can still steer a robot to whatever provider the registry supports by setting policy_provider to an arbitrary string, even though policy_type is now allowlisted. This contradicts the round-3 commit message claim that threat-vector #3 is "only fully closed once these four fields are gated."

Mirror the is_safe_policy_type allowlist for policy_provider (or merge them — the two registries overlap), and add a regression test in tests/mesh/test_security.py that fails on pre-fix code (per AGENTS.md > Review Learnings (#85) > "Pin regression tests for reviewed fixes").

[yinsong1986]

_sign_record re-reads STRANDS_MESH_AUDIT_PSK on every record, so the audit log silently degrades from signed to unsigned if the env var is unset mid-run.

def _sign_record(record):
    psk = _audit_psk()    # os.getenv(...) every call
    if psk is None:
        return None
    ...

The documented threat model is "a compromised process cannot delete records or evade gap detection." But a process that can clear its own env briefly can write a run of unsigned forgeries, then re-set the PSK; verify_audit_integrity reports them as missing_sig (not bad_signature) and ok is computed as bad_signature == 0 and not gaps — i.e. a log full of unsigned forgeries still reports ok=True when a PSK is configured at audit time but the log itself was written without one. That defeats the integrity guarantee for any reader that only checks result['ok'].

Two fixes worth considering:

1. Cache the PSK presence at module load (or first call) so an env-var unset mid-run is logged loudly via logger.error and signing continues with the cached key (or the process refuses to write further records).
2. Fold missing_sig > 0 and psk_present into the ok calculation at line 428 of audit.py so verification fails closed when a PSK is configured but records are unsigned.

[yinsong1986]

The seq sidecar is not fsync'd, but the audit log is — durability mismatch on crash.

The audit log write at line 298 calls os.fsync(fh.fileno()), but _persist_seq_counters writes the sidecar via os.replace(tmp, sidecar) with no fsync on the rename or the temp file. After a power loss the audit log can have records with seq=N while the sidecar's last-flushed state is seq=N−k. On restart _load_seq_counters picks up the stale N−k, the next event writes seq=N−k+1, and the audit log now contains duplicate seq values for the same peer.

verify_audit_integrity's gap detection then mis-reports — last_seq_by_peer advances to whatever the most recent record claims, so a duplicated seq looks like an out-of-order record but not necessarily a gap. The integrity guarantee that the PR description claims ("a compromised process cannot delete records and renumber from 1") is weaker than advertised when the failure mode is just an unclean shutdown.

Add os.fsync() on the temp fd before os.replace, and ideally fsync the parent directory afterwards on POSIX — or accept the limitation and document it in the module docstring next to the "counters persist to a sidecar" claim.

[yinsong1986]

The pinned CA hash is a hardcoded ticking time bomb with no proactive surface for ops.

_AMAZON_ROOT_CA1_SHA256 = "2c43952ee9e000ff2acc4e2ed0897c0a72ad5fa72c3d934e81741cbd54f05bd1"
# Last verified 2026-05.

When AWS rotates the root, every deployment hard-fails on the next provisioning run. Round 3 also (correctly) made the on-disk re-use path immune to STRANDS_MESH_DISABLE_CA_PIN, so the only recovery is "manually delete ~/.strands_robots/iot/AmazonRootCA1.pem and re-run with the override." The combination — silent expiry + no early warning + manual recovery — is operationally fragile.

Consider one of:

• Surface verify_ca_pin(path) in a way that fleet-monitoring can call (e.g. a mesh-doctor CLI subcommand) so dashboards can flag a mismatch before a provisioning run fails.
• Allow a list of acceptable pinned hashes (_AMAZON_ROOT_CA1_SHA256_PINS = [...]) so a rotation can ship as a code change without a flag day for every deployment that hasn't refreshed yet.
• Emit a logger.warning when the pin's age (encoded near the constant or in a sidecar metadata file) crosses 12 months, so the comment isn't the only signal.

Not blocking, but worth tracking as a follow-up issue on the project board.

[yinsong1986]

_on_cmd consumes a per-sender token before checking the lockout state; during emergency-stop, valid signed cmds from a peer burn through their bucket and fill the audit log with command_rejected_lockout entries.

The order in _on_cmd is: verify envelope → consume token → spawn thread → _dispatch raises LockoutError → _exec_cmd audits command_rejected_lockout. While the lockout is engaged, a peer that did not yet hear the safety/estop will keep dispatching commands at its normal rate; each one consumes a _security.consume_peer_token slot AND writes an audit entry, so the audit log can grow at the peer's send rate × STRANDS_MESH_PEER_RATE until the peer realizes the fleet is locked out.

Not a security blocker (the rate limiter still bounds total work), but a fast-path early return for the lockout case before the token consume — or at least skipping the audit write for the second-and-subsequent rejection from the same sender within a short window — would prevent the lockout's audit-log volume from drowning the very forensic trail operators rely on during an incident. Worth a follow-up issue on the project board (AGENTS.md > Project Dashboard rule).

[yinsong1986]

Summary

Large, well-scoped security hardening of the mesh layer. The PR adds a single mesh/security.py boundary that owns HMAC-SHA256 envelope signing, replay protection, command/host allowlisting, and per-sender token-bucket rate limiting; routes every outbound publish through Mesh._put_signed; tightens IoT policies (scoped Receive instead of strands/* wildcard); pins Amazon Root CA1 with size-cap + break-glass; adds bridge cross-transport dedup; gives the audit log per-record HMACs + per-peer monotonic seq counters with sidecar persistence; and replaces the LLM-tool's confirm: bool with a proper Strands SDK HITL interrupt. Test coverage is genuinely strong (+143 tests, 603 passing) and round 2/3 review feedback is documented in CHANGELOG.

What's good

• Single security boundary in one module — reviewable in isolation. Permissive vs strict modes, env-var matrix, and trust model are spelled out in the module docstring.
• HITL interrupt over a confirm: bool is the right call against prompt injection.
• Per-record HMAC + per-peer seq + tamper-aware cursor advancement in verify_audit_integrity (audit.py:492) is a careful design — a tampered seq value can't mask a downstream gap.
• Symlink + O_NOFOLLOW defence on the audit log (audit.py:356-363) is the right depth for the threat model.
• Round-3 fixes for _PEER_RATE_LOCK release-before-bucket.consume() and the rate-limit check/record split (declined HITL no longer consumes a slot) are well-reasoned.
• CHANGELOG documents the round-3 fixes and PENTEST_FINDINGS.md captures the cycle-by-cycle pentest evidence.

Concerns

• PR is behind upstream/main. Diff-vs-main shows phantom deletions in strands_robots/simulation/ and tests/simulation/ because PR #192 (closes #191) merged after this branch was cut. Vs upstream/main the diff is clean (sim files unchanged), but the PR will need a rebase before merge so the GitHub diff renders correctly and CI is on a current base.
• Two custom exception classes are dead code. RateLimitError and AuthorizationError are defined and exported in __all__ but never raised anywhere in the tree (grep confirms). AGENTS.md "No dead code" rule applies.
• PENTEST_FINDINGS.md placeholder reference. The doc still references commit <this-commit> literally on line 56 — replace with the actual SHA before merging or drop the placeholder.
• Audit-write amplification under flood. Every safety event is now a double synchronous write (audit log line + seq sidecar via os.replace + chmod). Token bucket caps inbound floods at 20/60s/sender by default, but STRANDS_MESH_PEER_RATE can go up to 1000 burst, and rate-limit-drops themselves are audited. Worth confirming sidecar persistence doesn't become the bottleneck on hot-event paths — see inline.
• Permissive-mode replay cache fillable by unauthenticated peers. In permissive mode, verify_envelope records nonces for any well-shaped envelope from any peer (security.py:460). Cache eviction handles overflow but the prune path holds _NONCE_LOCK for an O(n) walk under attack — a noise vector the strict-mode design avoids. Worth at least documenting the residual risk.

Verification suggestions

• Confirm gh pr view 194 --json mergeStateStatus shows mergeable / not behind, and rebase if not.
• Spot-check the audit hot path under load: for i in $(seq 1 1000); do python -c 'from strands_robots.mesh.audit import log_safety_event; log_safety_event("test", "p1", {})'; done and time the difference with STRANDS_MESH_AUDIT_PSK set vs unset.
• grep -nP '[^\x00-\x7F]' strands_robots/mesh/*.py to confirm no orphan combining marks slipped in (PR text uses box-drawing dashes in module docstrings — those are fine in source comments but worth a sweep).
• grep -rn "RateLimitError\|AuthorizationError" strands_robots tests to confirm the dead-code finding before deciding whether to wire them up or drop them.

[yinsong1986]

Dead code — defined and exported but never raised.

RateLimitError (here) and AuthorizationError (line 172) are both in the __all__ list at the bottom of the file but git grep RateLimitError strands_robots tests and git grep AuthorizationError strands_robots tests return only the definition + export sites — no raise anywhere in the tree, no test references.

The per-sender token bucket in _on_cmd returns False on starvation rather than raising, and RateLimitError is never thrown from the rate-limit path. AuthorizationError has no consumers at all.

AGENTS.md > Key Conventions #10: "No dead code — if it's not called and not part of base class, delete it."

Either wire them into the dispatch chain (e.g. raise RateLimitError from consume_peer_token and catch it at the boundary so the audit message becomes structured) or drop both classes and trim the exports.

[yinsong1986]

Permissive-mode replay cache is fillable by any unauthenticated peer.

In permissive mode (no PSK, STRANDS_MESH_REQUIRE_AUTH unset), this branch unconditionally records the nonce of any well-shaped envelope from any LAN peer. An attacker can publish 10k+ envelopes with random nonces and force the prune path at _record_nonce (security.py:313-324) to hold _NONCE_LOCK for an O(n) walk on every subsequent inbound message — a low-cost DoS plus a brief window where legitimate replay protection is degraded as the prune drops the oldest 20%.

In strict mode the HMAC check above gates entry to _record_nonce, so this surface is closed. In permissive mode, the README markets back-compat as "zero-config Zenoh-LAN setups keep working unchanged" without acknowledging this side-effect.

Mitigations to consider:

• Skip nonce recording entirely in permissive mode and rely on the freshness window (the asymmetry between past tolerance and forward skew already bounds the replay envelope), OR
• Document the residual risk in the module docstring under "Operating modes" so operators know permissive mode is dev-only, not just unauthenticated-but-otherwise-safe.

Either is fine; the current state silently degrades under attack.

[yinsong1986]

Audit-write amplification: every safety event triggers a synchronous sidecar tmp + os.replace + chmod cycle.

_next_seq is called inside the record-build path of log_safety_event (audit.py:334), so every audited event (rate-limit drops, command rejections, lockout rejections, every LLM-tool action, the _publish_cameras_once debug-stream — anything that calls log_safety_event) does:

1. Acquire _SEQ_LOCK
2. os.open + json.dump + os.replace + chmod on the sidecar
3. Release _SEQ_LOCK, then
4. Acquire _WRITE_LOCK and os.fsync the audit line

That's 2× synchronous writes per event. Under bursts — e.g. an attacker probing actions at the configured STRANDS_MESH_PEER_RATE ceiling (which _MAX_PEER_RATE_BURST allows up to 1000), each rate-limit drop is itself audited at audit.py line 579 — the sidecar becomes the hot path.

The regression-test rule (AGENTS.md > Review Learnings #85: "Cache expensive JSON parsing" / hot-loop discipline) suggests batching: persist the sidecar at most once per N events or once per second, with a final flush in an atexit hook. The on-disk counters can lose at most that many seconds of seq state on a hard kill, which is acceptable given that verify_audit_integrity already detects gaps.

If intentional: please add a comment justifying per-event persistence, and consider a regression test asserting acceptable throughput at, say, 100 events/sec.

[yinsong1986]

The urlopen(...) call has a 15s connect/handshake timeout, but resp.read(...) has no per-read deadline.

A slow-loris responder (or hostile proxy) that establishes the TLS handshake quickly but then dribbles bytes can hold this read open for arbitrary wall-clock time. The 64 KiB cap bounds memory but not time.

Fix: pass a socket.setdefaulttimeout(...) for the duration of the call, or use socket.socket.settimeout on the underlying socket via a custom opener, so the read itself observes a deadline. A ~30s wall-clock cap is plenty for a 1.4 KiB cert.

This is the same threat-model concern that motivated the size cap — they belong together.

[yinsong1986]

Defence-in-depth: _expected_responders[turn] = target accepts whatever target the caller passes, including the BROADCAST_RESPONDER sentinel.

The sentinel is "<broadcast>\x00" and init_mesh's peer_id regex (line 1224) rejects NUL, so a real peer can't collide. Still, a caller-supplied target here bypasses the responder check at _on_response if it ever happens to equal the sentinel — a defensive if target == BROADCAST_RESPONDER: raise ValueError(...) here would make the contract explicit and protect against a future refactor that loosens the peer-id rules.

Low-priority hardening, not a current bug. Worth a one-line guard given how recently D1 was discovered.

[yinsong1986]

Summary

This is a substantial security-hardening pass on the mesh layer: HMAC-signed envelopes with replay protection, action/policy_host/model-path allowlists, per-sender token-bucket rate limiting, persistent e-stop lockout, signed audit log with sequence numbers and a verify_audit_integrity helper, cross-transport bridge dedup, IoT policy scope tightening, and Amazon Root CA1 SHA-256 pinning. The threat model in the description maps cleanly to specific code, and most fixes ship with regression tests under tests/mesh/ (especially test_pentest_findings.py).

The round-1/2/3 review-feedback iterations and the Phase-4 pentest commit show genuine adversarial review of the diff before posting. Nice.

What's good

• Threat-model traceability. Each control is tied to a numbered attack vector and to a regression test in tests/mesh/test_pentest_findings.py (e.g. test_p4_a2_bridge_topic_filter_exact_match_only, test_p4_d1_response_hijack_rejected_for_point_to_point). This matches AGENTS.md → "Pin regression tests for reviewed fixes".
• Permissive-mode back-compat is explicit. No-PSK → envelope-without-sig passes through; bare legacy dicts pass through. Strict mode is opt-in. Existing zero-config Zenoh-LAN setups won't break.
• Symmetric receiver/client validation. validate_command runs both in _exec_cmd (server side) and in tools/robot_mesh.py::tell|send|broadcast (client side). Defence in depth.
• HITL via SDK interrupt, not a confirm: bool. The shift to tool_context.interrupt(...) for emergency_stop and broadcast is the correct move — it routes the approval out-of-band of the LLM's tool-argument flow, so prompt injection cannot smuggle a flipped boolean. The whitelist of canonical affirmatives is right.
• Cross-process gap-detection awareness. The seq sidecar exists specifically to defeat the "crash and renumber from 1" evasion. Even given the concern below, this is more thought than most audit logs get.
• CA pinning split. verify_ca_pin() (forensic, never honours break-glass) vs _verify_ca_bytes() (provisioning, honours STRANDS_MESH_DISABLE_CA_PIN) is exactly the right two-API design; the on-disk re-use path correctly does not honour the break-glass.

Concerns

• Description claims rotation that isn't here. The CHANGELOG diff in the PR description references a Phase-4 / Cycle 6 / E1 "audit-log flood DoS" fix with _DEFAULT_LOG_MAX_BYTES, STRANDS_MESH_AUDIT_MAX_BYTES, STRANDS_MESH_AUDIT_MAX_FILES env vars. None of those identifiers exist in strands_robots/mesh/audit.py at HEAD 5872dbe, none are referenced in README.md, and PENTEST_FINDINGS.md only goes through Cycle 5. Either the rotation work needs to land in this PR, or the description / CHANGELOG / PENTEST_FINDINGS.md need to drop the E1 references so the PR matches what merges. Audit-log unbounded growth (line-rate rate_limit_drop events from a flood-attacking peer is the obvious driver) is a real concern — worth landing the rotation rather than dropping the claim.
• Scope is genuinely large for one PR. +6653/-106 across 29 files. This is justifiable given the threat-model story, but it makes per-line review hard and the round-1→round-3 commit history shows real review issues caught only iteratively. Future security-hardening PRs would benefit from being split (e.g. envelope auth, audit integrity, IoT-side hardening could each be their own PR).
• STRANDS_MESH_OVERRIDE_CODE constant-time compare uses raw bytes of differing lengths. hmac.compare_digest(expected.encode(), provided.encode()) short-circuits on length-mismatch in compare_digest's implementation (it's still constant-time relative to the shorter input, which is a known nuance of the function). Not a bug, but worth a comment explaining the property given the docstring's emphasis on constant time.
• tools/robot_mesh.py:161: except Exception: pass in _audit_tool_action. Auditing failures are silently swallowed (no logger.debug even). For a PR whose primary motivation is forensic integrity, the audit path itself losing events without trace is the wrong default. Suggest at least logger.debug("[robot_mesh] audit failed: %s", exc).

Verification suggestions

hatch run test tests/mesh/   # 603 mesh tests should pass per the description
hatch run lint               # ruff check, ruff format --check, mypy

# Spot-check the round-3 hardening of cross-mesh model-path injection:
python -c "
from strands_robots.mesh.security import validate_command, ValidationError
try:
    validate_command({'action':'execute','instruction':'go','pretrained_name_or_path':'attacker/evil-repo'})
    print('FAIL: attacker repo accepted')
except ValidationError as e:
    print('OK:', e)
"

# Spot-check the response-hijack regression directly:
hatch run test tests/mesh/test_pentest_findings.py::test_p4_d1_response_hijack_rejected_for_point_to_point -v

[yinsong1986]

D1 hardening is asymmetric — the receive-side turn_id fallback is still truncated. This call (and the matching broadcast at line 944) correctly upgrade to the full 128-bit uuid.uuid4().hex. But _exec_cmd still has turn = data.get("turn_id") or uuid.uuid4().hex[:8] (head line 594) for the case where an inbound command omits turn_id — 32 bits, the exact birthday-collision / predictability surface D1 set out to remove. It's pre-existing code, but the same threat-model that motivated the change here applies there: a peer that can deliver a turn_id-less command can predict the response topic the receiver publishes on with non-trivial probability.

Either generate a full uuid.uuid4().hex in _exec_cmd too (cheap, no downside), or have validate_command require turn_id and reject turn_id-less commands outright. The D1 regression test (test_p4_d1_turn_id_full_uuid4_entropy) only covers the outbound path — a companion test for _exec_cmd would pin this.

[yinsong1986]

Cross-module reach into a _-prefixed method. mesh._put_signed(...) is being called from a different module. The PR's own comment (lines 248–251) acknowledges this is the right channel because transport.put is the unsigned path — which is exactly the case for promoting _put_signed to a public name (e.g. Mesh.put_signed or Mesh.publish).

AGENTS.md → Review Learnings (#86) → "Public API Hygiene":

Never recommend a _method in user-facing docstrings or error messages. … Promote it (rename _dispatch_action → dispatch_action) or add public shorthands … before merging.

The same applies to module-to-module calls: every consumer is now coupled to a private name and the next refactor that renames _put_signed silently breaks the offloader. Cheap fix: add Mesh.publish(key, payload) as the public alias and have camera_offload.py use that.

[yinsong1986]

The seq sidecar isn't safe across concurrent processes. The threat model for seq is to defeat "compromised process truncates and renumbers from 1". But the design assumes a single writer:

1. _load_seq_counters() runs once per process (_AUDIT_STATE.seq_loaded gate).
2. Each _next_seq increments the in-memory counter and os.replaces the sidecar.
3. Two processes A and B both start with sidecar at seq=10. A advances to 15, persists. B (which loaded 10 earlier and has been writing in parallel) is at 12 in memory and persists 12 — sidecar rolls back from 15 to 12.
4. A's next write reads from in-memory (16), persists 16 — the gap between B's 12 and A's 16 is invisible because gap detection in verify_audit_integrity is per-peer_id, but two processes hosting the same peer_id (multi-Mesh test harness, Simulation re-runs, supervised restarts) collide.

This is the same threat model #6 in the PR description ("a compromised process cannot delete records and renumber from 1 to evade gap detection") — only with two cooperating writers instead of restart.

Fix options: (a) load sidecar under _SEQ_LOCK every _next_seq call (slow but correct), (b) flock(2) the sidecar across processes around the load+increment+persist sequence, or (c) document explicitly that audit-log integrity assumes a single writer per peer_id and add a startup check that errors out if two processes share an STRANDS_MESH_AUDIT_DIR.

[yinsong1986]

_resume_lockout leaks lockout state to remote callers. The PR description states the lockout response is "intentionally generic ({"error": "command rejected"}) — no leakage of duration or active flag to a remote caller". That's true for _dispatch's LockoutError path (line 715), but _resume_lockout itself returns three distinct results:

• {"status": "noop", "lockout": False} — lockout NOT engaged
• {"status": "error", "error": "override code not configured"} — lockout engaged, server has no code
• {"status": "error", "error": "invalid override code"} — lockout engaged, code wrong
• {"status": "ok", "lockout_elapsed_s": <float>} — lockout cleared (also leaks duration)

And _dispatch line 717–718 publishes that dict back as the RPC result. So an authenticated peer (or anyone in permissive mode) can probe action: resume with arbitrary codes to: (a) detect whether the lockout is engaged at all, (b) detect whether STRANDS_MESH_OVERRIDE_CODE is configured, (c) on success, learn the lockout duration. (a) and (b) are oracles for an attacker planning a follow-up attack; (c) confirms when an operator unlocked.

Fix: collapse all four cases into a single generic {"status": "ok"} (success) or {"status": "error", "error": "resume rejected"} (everything else) on the wire response. Keep the structured detail in the local publish_safety_event audit record (already happening) where forensics can use it.

[yinsong1986]

Bare except Exception is forbidden by repo conventions. The only failure modes for this parsing block are ValueError (int() / float() of a non-number) and AttributeError (no real path here since os.getenv always returns str | None and "x".split("/", 1) always returns a list). Use except ValueError: or at most except (ValueError, IndexError):.

AGENTS.md → Review Learnings (#86) → "Exception Clauses Must Be Narrow":

except Exception is forbidden for non-recovery code paths. Use the smallest superset of expected exception types.

Landing this in security-critical code makes it a particularly visible bad-pattern — and it could mask a future bug in _MAX_PEER_RATE_BURST clamping logic by silently falling back to the default (20, 60) instead of surfacing the parse failure.

[yinsong1986]

Summary

This is a large, ambitious security-hardening PR for the mesh layer: HMAC-signed envelopes with replay protection, command validation + allowlists, per-sender token-bucket rate limiting, persistent e-stop lockout, signed audit log with sequence numbers, bridge cross-transport dedup, IoT policy scoping, CA pinning, and HITL interrupt for fleet-wide actions in robot_mesh. The CHANGELOG is detailed, env vars are documented in README, and test coverage is broad (143 new tests across 8 files). The diff shape matches the description; permissive vs strict mode is a sensible default since it preserves zero-config dev behaviour.

The round-by-round reviewer feedback that's already been addressed (R1–R4) is encouraging — most of the obviously-bad patterns are gone. What's left are higher-order issues that are worth surfacing before merge.

What's good

• Wire-format envelope is versioned, canonical, constant-time-compared, and per-scope replay-cached. The asymmetric freshness window (60s past, 5s forward) is the right shape.
• Audit-log integrity is well thought through: per-peer monotonic seq with sidecar persistence, O_NOFOLLOW + symlink rejection, atomic temp+rename+fsync, rotation, and verify_audit_integrity that does NOT advance the cursor on bad-sig records (so seq tampering can't mask gaps).
• _dispatch lockout uses a generic command rejected string and a typed LockoutError — the wire response intentionally doesn't leak duration or active flag.
• The HITL interrupt for emergency_stop and broadcast is the right pattern — out-of-band approval that an injected prompt cannot smuggle.
• Bridge dedup uses full SHA-256 (not a truncated fingerprint) and the exact-vs-prefix split for filter entries is the right post-mortem fix for the cycle-2 finding.
• Tests use monkeypatch.setenv rather than direct os.environ[...] mutation. PENTEST_FINDINGS.md is detailed and links each cycle to a regression test.

Concerns

• Scope creep — .gitignore adds system_prompt.prompt, which is a session/tooling artifact unrelated to mesh security. Either drop it or move it to a separate hygiene PR.
• Permissive mode is the default, and several mitigations advertised by this PR are gated on strict mode (PSK + REQUIRE_AUTH). The _on_safety_estop/_on_safety_resume handlers explicitly return early in permissive mode; per-sender rate limiting keys on attacker-controlled sender_id; the replay cache itself is publicly fillable. The README does call this out, but the table at the top of the description ("11 attack vectors closed") reads as if all 11 are closed by default. Consider making strict mode the default in a v0.x bump, or at minimum tightening the table to indicate which mitigations are conditional on the operator opting in.
• _socket.setdefaulttimeout mutation in _ensure_ca is process-global. While the try/finally restores the prior value, any other thread doing socket I/O during the CA download window observes the mutated default — provisioning is rare so this is small, but it's worth a comment or a switch to a per-request timeout.
• Test surface gap — there's no regression test that pre-existing peers (no PSK) can interop with upgraded peers (PSK set on the other side) in both directions. The compatibility claim in the PR description is the riskiest behavioural promise; one explicit interop test would pin it.
• PENTEST_FINDINGS.md is committed at +423 lines — useful reference, but consider whether it belongs in the repo permanently or in docs/ / a separate ops repo. As-is it's a maintenance burden (every future security finding adds to it).

Verification suggestions

• Run the full mesh test suite under both modes: STRANDS_MESH_PSK=$(openssl rand -hex 32) STRANDS_MESH_REQUIRE_AUTH=true hatch run pytest tests/mesh -q and the same without those env vars set.
• Smoke test the HITL interrupt path manually: invoke agent.tool.robot_mesh(action="emergency_stop") from a Strands session and confirm the SDK actually pauses for operator approval (not just that the unit test mocks the interrupt).
• python -c "from strands_robots.mesh.audit import verify_audit_integrity; print(verify_audit_integrity())" against a real audit log produced by a brief test run — confirm ok=True on a fresh log and ok=False after a manual edit.
• Bridge dedup cross-transport: hard to exercise without IoT credentials, but the unit tests in test_bridge_dedup.py cover the deduplicator directly.

[yinsong1986]

Scope creep: system_prompt.prompt looks like a local tooling artifact (opencode / Claude Code session prompt) and has no connection to mesh security hardening. Either drop this line from this PR or split the gitignore tweaks into a separate housekeeping PR — keeping the security PR's diff focused on its stated purpose makes the CHANGELOG entry honest and the future bisect easier.

[yinsong1986]

The per-sender token bucket keys on sender_id, which is a field inside the (authenticated) payload — but in permissive mode (no PSK, the default) any LAN attacker can mint envelopes with a fresh sender_id per message and never collide with a bucket. The DoS-mitigation property of consume_peer_token evaporates exactly when it's most needed. With a PSK configured this is fine because forging a sender_id requires the key.

Two options that would close this:

1. In permissive mode, fall back to keying on the Zenoh transport's source identifier (sample-source) rather than the trusted-payload sender_id. Or:
2. Document loudly that the per-sender rate limit is a strict-mode-only control; in permissive mode it's a courtesy, not a defence.

The README's "Mesh security" section is honest about the lockout being soft in permissive mode — the same caveat belongs on the rate-limiter, both in the docstring of consume_peer_token and in the threat-vector table in the PR description (#10 reads as fully closed).

[yinsong1986]

The lockout-response design comment at line 711-715 says "the wire response is intentionally generic so a remote caller cannot use it to map the lockout window" — but validate_command runs before the lockout check in _dispatch. While the lockout is engaged, a malformed or out-of-allowlist command from an authenticated peer returns the rich "validation: <reason>" string, while a well-formed command returns the generic "command rejected". An attacker probing whether lockout is active just sends a known-bad action and watches the response shape.

Fix: either run the lockout check before validate_command (so any inbound non-status/resume action gets the same generic string while locked out), or normalize the validation error reply to the same generic string when _estop_lockout.is_set(). The audit log can keep the rich reason — that's the correct operator channel.

[yinsong1986]

socket.setdefaulttimeout() is process-global, not request-scoped. While the try/finally correctly restores _prev_default, any other thread in the process doing socket I/O during the CA download window observes the mutated 15s default — mostly harmless during one-shot provisioning, but it makes the function's blast radius larger than its docstring implies, and it can interact badly with library code (boto3, requests, zenoh) that assumes the default is None.

A per-request timeout would avoid the mutation entirely. The R4-4 problem (urlopen's timeout= only covers connect+handshake) is real, but the better fix is to wrap the response in a streaming read with an explicit per-recv deadline, or to set the timeout on the underlying socket via the urllib opener's handler chain rather than setdefaulttimeout. At minimum, the docstring should warn that this function temporarily mutates a process-global.

[yinsong1986]

Permissive-mode replay-cache fillability is acknowledged in the module docstring (R4-6) as an accepted limitation, but it's worth pinning: in permissive mode (the default), the verifier still calls _record_nonce for every well-shaped envelope from any LAN peer. An unauthenticated attacker can flood unique 16-char nonces and force the GC walk under _NONCE_LOCK to run on every cmd handler call once the cap is hit. While memory is bounded at 10k entries, the GC pass is O(n) on a hot lock that every _on_cmd / _on_response / _on_presence waits on.

Two defences worth considering even for permissive mode:

1. Skip _record_nonce entirely when no PSK is configured AND auth_required() is false — replay protection without authentication is not a meaningful defence anyway, since an attacker who can spoof one nonce can spoof a fresh one.
2. Add a per-scope rate cap on cache writes (e.g. 1000 inserts/min/scope) so a flood is dropped before the GC walk runs.

At minimum, add a regression test that demonstrates the GC behaviour under a synthetic flood — there's currently no test that pins this pathology.

[yinsong1986]

Summary

Large, well-scoped security hardening of the mesh layer: HMAC-signed wire envelopes with per-message nonces and a freshness window, command-schema validation with allowlists for policy_host / policy_provider / policy_type / model_path / pretrained_name_or_path / server_address, per-record audit HMAC + per-peer monotonic seq with flock-protected sidecar persistence and O_NOFOLLOW rotation, fleet-wide e-stop lockout with a constant-time-compared override code and a single generic wire-response shape, cross-transport bridge dedup, exact-match bridge filter (with a separate prefix-allowlist for response/<turn>), CA-pin + body-cap + per-recv timeout on Amazon Root CA1 fetch, and an HITL tool_context.interrupt gate plus per-action sliding-window rate limit on the LLM-facing robot_mesh tool. The PR description is unusually thorough; the threat-model matrix maps cleanly to the diff.

What's good

• Single security boundary in mesh/security.py with a clean exception hierarchy (SecurityError/AuthenticationError/AuthorizationError/ValidationError/RateLimitError/LockoutError). Every outbound publish goes through Mesh._put_signed; every inbound goes through verify_envelope. That's the right shape.
• hmac.compare_digest for HMAC and override-code comparison — no naive == anywhere I could spot.
• Pentest evidence committed in PENTEST_FINDINGS.md and pinned via tests/mesh/test_pentest_findings.py is exactly the "every reviewed fix gets a regression test" pattern AGENTS.md > Review Learnings (#85) prescribes. The R5 pass also addresses the prior round's responder-binding turn-id truncation symmetrically on the receive side.
• Bridge filter exact/prefix split is the right fix for the cloud-pollution attack; _should_bridge also rejects .. traversal segments on the prefix path.
• R5-4 generic resume response removes a real differential-response oracle, with structured detail kept in the local audit record. Good design.
• Sim-by-default / fail-closed posture preserved: permissive-mode _on_safety_estop / _on_safety_resume refuse to act on remote events, so a permissive deployment cannot be DoS'd into a fleet-wide lockout by an unauthenticated LAN peer. The README now spells this out.

Concerns

• Pre-1.0 break documented as confirm: bool removal in robot_mesh is sound, but the bridge-filter migration (STRANDS_MESH_BRIDGE_TOPICS exact-match by default with a separate _PREFIX env var) is a silent semantics change for any operator who customised the old prefix-walk. The CHANGELOG notes it but a deprecation warning at runtime when an operator-set STRANDS_MESH_BRIDGE_TOPICS would have prefix-matched under the old logic but won't now would catch this in production.
• Hardcoded CA-pin SHA-256 (provision.py:71) is a time-bomb on AWS root rotation: every robot on the fleet starts failing _ensure_ca simultaneously the day AWS publishes a new AmazonRootCA1.pem and the old one ages out of the canonical URL. The PR description notes a follow-up to surface verify_ca_pin via a mesh-doctor CLI or accept multiple pins, but the surface here is fragile enough that it deserves to land before broad fleet rollout, not after. (Inline below as well.)
• Permissive-mode replay cache fillability (R4-6): documented as known residual risk, and that's defensible — but the docstring should be louder. verify_envelope records nonces in permissive mode for any well-formed envelope from any peer, and the cap is only 10k entries with an O(n) sorted prune. A LAN attacker can keep this hot path busy. Strict mode closes it but most dev/demo deployments will run permissive.
• mock / lerobot_local policy_type allowlist semantics: is_safe_policy_provider and is_safe_policy_type share _DEFAULT_POLICY_TYPES. That's reasonable for now, but policy_provider is a registry key (e.g. mock -> MockPolicy) while policy_type is a class family (e.g. act, diffusion); collapsing them onto one allowlist works only because the current registry happens to use the same string for both. A future split (e.g. registering a "openvla" provider that exposes the pi0 policy class) would silently allow either string in either field. Worth a comment on the helper, or split the env vars.
• No public mesh-doctor / verify CLI: verify_audit_integrity and verify_ca_pin are only reachable via the inline python -c snippets in the README. Operators running an incident response under stress will copy-paste those wrong. A small CLI subcommand would be a tiny add and dramatically reduce mistakes during a real lockout-and-forensics drill.

Verification suggestions

• Run the new pentest-regression suite explicitly and confirm zero skips: hatch run test -- tests/mesh/test_pentest_findings.py tests/mesh/test_security_regressions.py -v. The two existing skips in the global suite should NOT include any of these new modules.
• Re-run with STRANDS_MESH_PSK and STRANDS_MESH_REQUIRE_AUTH=true set in the environment, then with both unset, and diff the test counts — strict-mode-only tests should be additive, not replacing permissive-mode behaviour.
• Smoke-test the _resume_lockout generic-response surface by hand: trigger an e-stop, then probe _resume_lockout("") (lockout engaged, code unset), _resume_lockout("wrong") (lockout engaged, code wrong), and _resume_lockout(correct) after clearing the env, and confirm the wire response is byte-for-byte identical for the three failure shapes.
• Verify CA-pin behaviour against the actual current AmazonRootCA1: python -c 'from strands_robots.mesh.iot.provision import verify_ca_pin; from pathlib import Path; print(verify_ca_pin(Path.home() / ".strands_robots/iot/AmazonRootCA1.pem"))'. If False, the pinned hash needs refresh BEFORE merge; if the file doesn't exist locally, run _ensure_ca once against a clean dir to validate the download path.

[yinsong1986]

The try/except OSError here is convoluted and partially fail-open. The try body raises OSError only on the symlink-rejection path, the except OSError: handler then re-checks is_symlink() and conditionally re-raises — but if the second is_symlink() returns False (TOCTOU race, e.g. attacker replaced the symlink with a real file between checks), the original rejection OSError is silently swallowed and _ensure_paths proceeds to path.touch() at line 618, which does follow symlinks.

The O_NOFOLLOW open at line 689 is the real defence, but path.touch() runs before that and creates the TOCTOU window (and on Windows, where O_NOFOLLOW is 0, the static check here is the only line of defence). Two suggestions:

1. Drop the wrapping try/except OSError — is_symlink() doesn't raise on a non-existent file, it returns False, so there's nothing to catch. Just if path.is_symlink(): raise OSError(...).
2. Replace path.touch() at line 618 with os.open(path, O_WRONLY | O_CREAT | O_NOFOLLOW, 0o600) then immediately close — the file creation step itself should refuse to follow a symlink.

Also: a regression test that races a symlink-swap between _ensure_paths and log_safety_event would pin this; the existing test_p4_f2_audit_log_symlink_swap_blocked only covers the post-creation case.

[yinsong1986]

socket.setdefaulttimeout(15.0) is a process-global side effect — for the duration of the urlopen call, every other socket created on every other thread in this process inherits the 15s timeout. If _ensure_ca is invoked from a long-running provisioning workflow that is concurrently doing other socket work (Zenoh session, IoT MQTT keepalive, anything boto3-driven), those sockets briefly pick up a foreign default and may behave subtly differently — most visibly as new connections capping at 15s rather than no-timeout, but on rare occasions as recv deadlines surfacing in unrelated code paths.

The intent (per-recv deadline on the CA download) is right, but the mechanism leaks. Two cleaner alternatives:

• Use urllib.request.build_opener(...) with a custom HTTPSHandler whose https_open uses socket.create_connection(..., timeout=15.0) — the timeout sticks to that one socket only.
• Or wrap the response in a socket.timeout-aware reader yourself (resp.fp._sock.settimeout(15.0) works on CPython but is fragile).

AGENTS.md > Review Learnings (#85) > "Document the concurrency contract" applies — at minimum, if you keep setdefaulttimeout, the docstring should call out that this is process-global so callers know not to invoke _ensure_ca concurrently with latency-sensitive work.

[yinsong1986]

Hardcoding _AMAZON_ROOT_CA1_SHA256 = "2c4395..." is a fail-closed time-bomb. The day AWS rotates AmazonRootCA1 (and removes the old PEM from https://www.amazontrust.com/repository/AmazonRootCA1.pem), every robot in the fleet that re-runs _ensure_ca — for example after a re-provision, an STRANDS_MESH_AUDIT_DIR migration, or any mass redeploy — starts raising RuntimeError("AmazonRootCA1 ... failed pin check") simultaneously. The break-glass STRANDS_MESH_DISABLE_CA_PIN=true only applies to the download path (correctly!), so operators have no way to recover except editing source.

The PR description acknowledges this as a follow-up tracked on the project board, but the cost asymmetry is large enough that I'd suggest landing one of these in this PR:

• Accept a list of pinned SHA-256 hashes (_AMAZON_ROOT_CA1_PINS = ("2c4395...", ) and check body in pins). Future rotations land a 2-pin tuple in advance and the old pin gets removed in a later PR after rollout.
• Or expose a STRANDS_MESH_CA_PINS env var (comma-separated) so a fleet-ops operator can ship a new pin via configuration without a code change.

The pure form here is the right security posture; the operational form needs an upgrade path.

[yinsong1986]

policy_provider validation has a hole when an authenticated peer sends policy_provider: null explicitly:

provider_value = cmd.get("policy_provider", "mock")  # cmd["policy_provider"] = None  ->  provider_value = None
if provider_value and not is_safe_policy_provider(...):  # short-circuits, validation skipped
    raise ValidationError(...)
if provider_value:                                    # also skipped
    out["policy_provider"] = ...

So out["policy_provider"] keeps the explicit None (since out = dict(cmd) at line 718 copied it). Back at core.py:737, cmd.get("policy_provider", "mock") returns None (the key exists), and None is forwarded into r._execute_task_sync(instruction, None, ...). The downstream behaviour depends on the executor — likely a TypeError, but it could also silently route to a default provider that bypasses the gate's intent.

Fix: treat the "policy_provider" in cmd case as "value present, must be a non-empty string in the allowlist":

if "policy_provider" in cmd:
    value = cmd["policy_provider"]
    if not isinstance(value, str) or not is_safe_policy_provider(value):
        raise ValidationError(...)
    out["policy_provider"] = value.strip().lower()
else:
    out["policy_provider"] = "mock"

This matches the pattern already used for model_path / pretrained_name_or_path / server_address. A regression test sending {"action": "execute", ..., "policy_provider": None} should fail on pre-fix code.

[yinsong1986]

except Exception: pass is the AGENTS.md anti-pattern ("Exception Clauses Must Be Narrow" + "Fail-fast with strict=True"). The audit log call is best-effort, which is fine — but a swallowed exception with no log line means a broken audit path silently disappears every safety-significant tool action.

The rest of this PR uses the right pattern (e.g. core.py:583-587):

except Exception as audit_exc:
    logger.debug("[mesh] %s: audit log unavailable: %s", self.peer_id, audit_exc)

Applying that here keeps the "audit must never crash safety" contract while still leaving a debug-level breadcrumb for operators investigating "why don't I see my LLM tool actions in the audit log?". One-line fix; no behaviour change for the happy path.

[yinsong1986]

Summary

Large, well-scoped hardening of the mesh wire and tool surface: HMAC-signed envelopes, replay protection, command validation allowlists, per-sender token-bucket rate limiting, per-record audit-log HMAC + per-peer monotonic seq with cross-process flock, CA-pin re-checked on every on-disk re-use, IoT policy scope tightening, HITL interrupt for emergency_stop / broadcast. Diff is +7103 / -107 across 27 files but the bulk is the new mesh/security.py (~960 lines), mesh/audit.py rewrite (+693), and a substantial test suite (5 new test files, ~3500 lines of new tests). PR title/description match the diff; no scope creep into unrelated modules.

What's good

• One real security boundary (mesh/security.py) + a single _put_signed chokepoint for outbound publishes. This is the right shape — every audit/HITL/rate-limit/dedup/lockout finding has one place to live.
• Strong test discipline: regressions for every reviewed fix, threat-model docstrings on tests/mesh/test_pentest_findings.py, and a test_security_regressions.py that pins each round's behavioural change.
• Audit-log threat model is genuinely thorough: per-record HMAC, per-peer seq, sidecar persistence, O_NOFOLLOW + lstat symlink defence, rotation cap, fsync of tmp + parent dir, cross-process fcntl.flock, PSK-degrade detection. The verify_audit_integrity reporting (bad_signature cursor not advancing, missing_sig while psk_present) shows the threat model was actually walked.
• AGENTS.md compliance: no host paths committed, no emojis in user-facing strings, no _method recommendations in docstrings (the Mesh.publish public alias for _put_signed is exactly the R5-2 hygiene rule), narrow-except clauses (R5-5), CHANGELOG.md gets a per-round detail entry, README.md gets the env-var matrix.
• Backwards-compat story is coherent: permissive mode keeps zero-config Zenoh-LAN demos working; strict mode is documented as the production posture; STRANDS_MESH_BRIDGE_TOPICS_PREFIX opt-in for the prefix-walk migration.

Concerns

• Audit gap on remote-triggered safety events. _on_safety_estop and _on_safety_resume set/clear _estop_lockout on signed remote events, but neither calls log_safety_event / publish_safety_event. The local-issuer paths (emergency_stop() and _resume_lockout()) both do. Given that the entire PR's audit-integrity story (per-record HMAC, seq, gap detection) is built around forensic recoverability of safety events, the receiver side dropping these to a logger.critical line breaks the model. See inline comment.
• Permissive-mode replay protection has a documented hole, but legacy bare-dict payloads have NO replay protection at all. verify_envelope returns un-enveloped legacy dicts unchanged in permissive mode (if "v" not in envelope or "payload" not in envelope: return envelope). The module docstring's R4-6 callout covers nonce-cache fillability for enveloped messages but does not call out that bare dicts skip freshness AND nonce checks entirely. An attacker on a permissive LAN can replay any legacy cmd indefinitely. Worth adding to the permissive-mode caveats in the README.
• Audit entries on validation/lockout/rate-limit rejection are amplifiable under flood. R4-7 acknowledges audit-write amplification in the module docstring, but each invalid cmd from one peer also triggers _put_signed of an error response. The token bucket caps per-sender input rate but each rejected cmd produces a synchronous double-write (audit + signed wire publish) that runs to completion. This is the same shape as the PR's own #12 follow-up about lockout fast-path; worth expanding that follow-up issue to cover the validation-rejection path too.
• Magnitude. ~960 lines of new security code + ~3500 lines of tests in one PR. The maintainers' usual review-ability ceiling is much smaller. This is fine here only because the seven "hardened modules" each have a clear, separable responsibility and the test suite pins per-round behaviour. Future hardening work should split.

Verification suggestions

# 1. Strict-mode round trip: prove fleet-wide e-stop fans out and audits.
export STRANDS_MESH_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_REQUIRE_AUTH=true
export STRANDS_MESH_AUDIT_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_OVERRIDE_CODE=$(openssl rand -hex 16)
hatch run pytest tests/mesh/ -x

# 2. Confirm verify_audit_integrity reports remote-triggered estops:
#    Spin two Mesh peers in one process, have one call emergency_stop(),
#    then verify_audit_integrity() and inspect the JSONL — the receiver
#    side has no record of its lockout engagement. (This is the inline
#    audit-gap concern.)

# 3. Permissive-mode replay sanity: capture a legacy bare-dict cmd on
#    the wire (no PSK), replay it 60s later, verify it executes a second
#    time. Documents the residual risk concretely.

# 4. CodeQL findings since branch creation — the PR notes #226/#227/#228
#    as comment-fixes; check the Security tab for any new alerts on the
#    +693 lines of audit.py.

[yinsong1986]

Audit gap on remote-triggered safety events. This handler sets _estop_lockout on a signed remote estop but never calls log_safety_event / publish_safety_event. Compare with emergency_stop() (line ~1140) which records event_type="emergency_stop" and the local _resume_lockout() which records resume_ok / resume_denied. The whole PR's audit-integrity story (per-record HMAC, per-peer seq, gap detection) is premised on every safety transition being walkable post-incident. A receiver going into lockout because some other peer broadcast safety/estop is exactly the event a forensic reader needs.

Same applies to _on_safety_resume two methods below (line 929) — clears the lockout with only logger.warning, no audit record.

Suggested fix: emit publish_safety_event("remote_estop_engaged", severity="critical", payload={"sender_id": sender, "trigger": "remote"}) (and the symmetric remote_resume_applied in _on_safety_resume) so verify_audit_integrity walkers see both ends of the lockout window.

Add a regression test under tests/mesh/test_pentest_findings.py — currently no test in this PR exercises the _on_safety_estop audit path (grep shows zero call sites for log_safety_event inside either _on_safety_estop or _on_safety_resume). Per AGENTS.md > Review Learnings (#85) > "Pin regression tests for reviewed fixes", this needs a test that fails on the current code.

[yinsong1986]

Legacy bare-dict payloads bypass freshness AND replay protection in permissive mode. This early-return for un-enveloped messages skips the ts window check, the nonce minimum-length check, and _record_nonce. The module-docstring R4-6 callout (lines 51-66) covers nonce-cache fillability for enveloped messages but does not flag that legacy bare dicts have no replay protection at all — an attacker who captures a legacy cmd envelope on a permissive LAN can replay it indefinitely.

Two options:

1. Apply replay protection to legacy bare dicts too: synthesize a content-fingerprint nonce (sha256 of canonical bytes) before this early return so at least content-replay is caught.
2. Document this explicitly in the module docstring's known-limitations section (and in the README's "Mesh security" section) so operators running permissive mode in untrusted environments understand the gap.

Option 2 is the smaller change but option 1 closes the actual surface. At minimum, the README's permissive-mode warning should be sharpened from "legacy unsigned messages still accepted" to "legacy unsigned messages still accepted with NO replay protection".

[yinsong1986]

Raise-then-immediately-catch is a code smell. The try: raise AuthorizationError(...) except AuthorizationError as exc: logger.debug(...) pattern (lines 844-849) consumes traceback machinery solely to drive a debug log line. The comment says "Raise into the local handler so a future refactor that adds a wider error pipeline can plug in" but that's speculative refactor scaffolding — YAGNI.

Replace with a direct logger.debug("[mesh] %s: response_hijack_rejected: responder=%r expected=%r", ...). The structured log_safety_event("response_hijack_rejected", ...) call above (lines 826-839) already gives forensic readers the typed event; the AuthorizationError exception class is exported in __all__ but only constructed here. If a future refactor genuinely needs to chain on this, it can re-introduce the raise at that point with concrete intent.

[yinsong1986]

Per-sender token consumed before lockout check. _on_cmd calls consume_peer_token(sender_id) and then spawns _exec_cmd which checks _estop_lockout inside _dispatch. During a fleet-wide e-stop, every signed cmd from a peer that has not yet processed the estop broadcast burns a token from its bucket and produces a command_rejected_lockout audit entry (line 666) PLUS a signed error response on the wire — exactly when the audit log most needs signal-to-noise.

The PR description (#12) lists this as a known follow-up, which is fine — but the same shape applies to validate_command rejections (line 622): a peer with one stale config can flood the audit log + wire with command_rejected entries up to its full bucket rate. Worth either expanding the follow-up issue to cover both rejection paths or adding a fast-path early-return at the top of _on_cmd that drops to DEBUG-only logging when the lockout is engaged.

[yinsong1986]

out["policy_provider"] lowercased — but the dispatcher uses it as a registry key. validate_command lowercases the provider name (line 797: out["policy_provider"] = str(provider_value).strip().lower()) and _dispatch (core.py:737) reads it back. The allowlist comparison in is_safe_policy_provider is also case-insensitive, so the round-trip is consistent within the security module — but the receiver-side dispatch passes this lowercased string straight into r._execute_task_sync(instruction, policy_provider, ...) and the policy registry's lookup is presumably case-sensitive (LeRobot policy classes are conventionally lowercase too, so this is probably a no-op in practice).

Worth a one-line regression test confirming that a caller passing policy_provider="Lerobot" (mixed case) still resolves to the same policy as policy_provider="lerobot" — silent case-coercion in the security layer that the dispatch layer doesn't expect would be the kind of subtle data-integrity drift AGENTS.md > Review Learnings (#85) > "Match schema and data keys" warns about. Either lock in the lowercase contract with a test or stop lowercasing here and let the registry do the comparison.

[yinsong1986]

Summary

This PR adds an HMAC-signed envelope security boundary in front of the mesh wire (mesh/security.py), per-record audit-log HMAC + per-peer sequence numbers + bounded rotation, CA pin hardening for IoT provisioning, and an HITL/rate-limited surface for the robot_mesh LLM tool. It is a large change (+7600/-109 across 27 files) that maps cleanly onto the 11-vector threat matrix in the description, and most of the code reads as carefully thought-out — multiple review rounds are clearly visible in the inline R*-N comments.

My review is COMMENTED only — a maintainer should make the final verdict. I am not blocking, but I have one likely real bug (audit rotation off-by-one) and three substantive design concerns where the implementation does not quite match what the PR description / README claim.

What's good

• Concrete threat matrix with one regression test per finding (tests/mesh/test_security_regressions.py) — exactly the "Pin regression tests for reviewed fixes" rule from AGENTS.md > Review Learnings (#85).
• HITL approval is delivered out-of-band of the LLM tool args (tool_context.interrupt), so prompt injection cannot smuggle approval. Declined approval correctly does NOT consume a rate-limit slot (R3-3).
• CA pin promoted to a tuple + env-var-extensible (R7-3) avoids the flag-day rotation foot-gun. verify_ca_pin deliberately ignores STRANDS_MESH_DISABLE_CA_PIN so forensic scripts can't be silenced.
• Per-record HMAC + per-peer seq + cross-process flock on the sidecar (R5-3) closes the obvious tamper-then-restart attack on the audit log.
• Wide-except clauses are individually justified inline (audit best-effort path, dispatch error sanitisation), and the comments cite AGENTS.md > "Exception Clauses Must Be Narrow" as the rule they're consciously deviating from.
• Generic wire response on _resume_lockout (R5-4) closes the four-shape oracle leak.

Concerns

• PR description vs. implementation drift on validation reach. The threat-matrix row #3 says off-allowlist values are "rejected client-side AND server-side." In practice validate_command is only invoked client-side from tools/robot_mesh.py; programmatic Mesh.send(target, cmd) and Mesh.broadcast(cmd) skip it entirely (see inline comment on core.py:967). The receiver does enforce, so this is defence-in-depth, not a hole — but the description and the README's "send / broadcast payloads are validated through validate_command before leaving the agent" oversell what ships.
• Fleet-wide safety/resume does not require the override code on receivers (see inline on core.py:927). Anyone holding the PSK can clear every peer's lockout without ever proving knowledge of STRANDS_MESH_OVERRIDE_CODE. This may be intentional ("PSK is the trust root"), but the threat matrix row #8 reads as if the override code protects the fleet-wide surface — it only protects the local issuer's gate.
• Audit-log rotation cascade keeps max_files - 1 rotated copies, not max_files (see inline on audit.py:270). The README documents the env var as "Maximum number of rotated audit log copies kept" — the implementation under-counts by one. The existing test in test_pentest_findings.py only checks an upper bound of 3 files total with max_files=3, so it would not have caught this.
• No CHANGELOG entry for the breaking-ish wire-format change. Bridge filter behaviour change is mentioned, but the new envelope wire format itself is a one-way roll-out (a v1 envelope from a new peer will fail verify_envelope in old peers that don't know the shape — old _on_cmd will just fail to find expected keys and silently drop). Worth a one-paragraph upgrade note for fleets that mix old + new versions during rollout.
• Ambition vs. test depth on multi-process flock. _next_seq correctness under genuine multi-process contention (vs. multi-Mesh in one process) is not exercised by the test suite — there is documentation of the flock contract but no test that forks two processes against a shared STRANDS_MESH_AUDIT_DIR and asserts no duplicate seq values. Given the threat model explicitly calls this out as a hardening goal, a multiprocessing.Process-based regression test would be a high-value follow-up.

Verification suggestions

# Reproduce the rotation off-by-one (audit.py:270):
export STRANDS_MESH_AUDIT_DIR=/tmp/audit_rotcheck && rm -rf $STRANDS_MESH_AUDIT_DIR && \
  STRANDS_MESH_AUDIT_MAX_BYTES=2048 STRANDS_MESH_AUDIT_MAX_FILES=5 \
  python -c "from strands_robots.mesh.audit import log_safety_event; \
  [log_safety_event('flood', 'p', {'i': i, 'pad': 'x'*200}) for i in range(2000)]" && \
  ls $STRANDS_MESH_AUDIT_DIR/mesh_audit*
# Expected per docs: mesh_audit.jsonl + .1 .. .5 (six files). Observed: .1 .. .4 only (five files).

# Reproduce the resume bypass (core.py:927) — any PSK holder can fan-out a resume
# without override code; see test_security_regressions.py for the local pattern,
# then verify a remote peer with a SIGNED envelope but NO override clears its lockout.

# Confirm Mesh.send doesn't validate (core.py:967):
python -c "from strands_robots.mesh import security as s; \
  print(s.validate_command({'action': 'execute', 'instruction': 'go', \
  'pretrained_name_or_path': 'evil-corp/backdoor'}))" \
  # raises ValidationError
# But mesh.send(target, {...same dict...}) just publishes it; receiver rejects.

[yinsong1986]

Off-by-one in the rotation cascade — keeps max_files - 1 rotated copies, not max_files.

With max_files=N, the loop iterates n in [N-1, N-2, ..., 1]. For n = N-1 the predicate n + 1 > max_files - 1 evaluates to N > N - 1 (always true), so the file at .{N-1} is unlink-ed instead of being renamed to .{N}. The result: rotated suffixes only ever go up to .{N-1}, and mesh_audit.jsonl.{N} never exists.

The README documents STRANDS_MESH_AUDIT_MAX_FILES as "Maximum number of rotated audit log copies kept" — operators reading that and setting STRANDS_MESH_AUDIT_MAX_FILES=5 will get four rotated copies plus the active log, not five. Real disk-budget impact is small, but the discrepancy itself is the bug.

The existing test (tests/mesh/test_pentest_findings.py::test_p4_e1_audit_log_size_bounded_by_rotation) only asserts an upper bound (<= max_bytes * max_files * 1.5 total) and "at least one rotated", so it cannot catch under-rotation.

Fix: change the predicate to n + 1 > max_files (or equivalently, drop the discard branch and always os.replace, then unlink any file at .{max_files + 1} after the cascade). Add a regression test that sets max_files=5, floods until at least 5 rotations have occurred, and asserts .1 through .5 all exist.

[yinsong1986]

STRANDS_MESH_OVERRIDE_CODE does not protect the fleet-wide resume — only the local issuer's gate.

_resume_lockout (line 1152) checks the override code on the issuing peer, then publishes a strands/safety/resume envelope (line 1217) whose payload is {peer_id, t, lockout_elapsed_s} — no override code on the wire. _on_safety_resume here just verifies the signed envelope and clears the lockout. Net effect: any peer that holds the PSK can broadcast a safety/resume and clear every other peer's lockout without ever proving knowledge of the override code.

This contradicts the threat-matrix row #8 in the PR description ("resume requires constant-time-compared override code"). It is internally consistent (PSK is presumably the higher-trust root), but a reader of the PR description would assume the override code is the second factor for fleet-wide resume; in practice it is only a second factor for the local CLI / RPC path.

Options:

1. Document this explicitly in _on_safety_resume's docstring and in the README's Mesh-security section: "PSK alone is sufficient to fan-out a resume; the override code is a local-issuer gate, not a fleet-wide gate."
2. Or include the override code (or an HMAC over the override code + nonce) in the resume payload and re-verify on receive. This costs nothing because the receiver already has the env var and it's already constant-time-compared in _resume_lockout.

Either is acceptable; the current state with neither is the worst case.

[yinsong1986]

Mesh.send / Mesh.broadcast skip validate_command, so client-side validation only protects callers that go through tools/robot_mesh.py.

Programmatic mesh users (third-party integrations, tests, scripts that import Mesh directly) publish whatever dict is passed in. The receiver does call validate_command on the inbound side (_exec_cmd line 607), so this is defence-in-depth not a security hole — but the PR description (matrix row #3, "rejected client-side AND server-side") and the README ("send/broadcast payloads are validated through validate_command before leaving the agent") oversell what ships.

Suggested fix — wrap the existing publish with the validator and a single error path:

# in send() and broadcast(), right before _put_signed:
try:
    cmd = _security.validate_command(cmd)
except _security.ValidationError as exc:
    return {"status": "error", "error": f"validation: {exc}"}

This matches the AGENTS.md > #92 review-learning rule "Validate before subprocess interpolation" — the same defence-in-depth principle applies to the wire boundary. If the maintainers prefer to keep the LLM-tool-only validation contract, the docs and PR description should be edited to match.

[yinsong1986]

HITL approval is requested before the command JSON is parsed and validated.

For broadcast, the operator sees the raw command string in the reason dict at the moment of approval. If the JSON is malformed or the command fails validate_command (lines 430-442), the action is rejected after the operator has already approved — so the audit trail records operator approved (line 328) for an action that never actually ran. Worse, the post-approval _rate_limit_record (line 327) consumes a slot for a command that was never sent; subsequent legitimate emergency-stops then face an inflated rate-limit window.

Suggested ordering:

1. Parse + validate_command first (refuse early on bad JSON / out-of-policy).
2. Then surface the validated command to the interrupt prompt.
3. Then record the slot on approval.

This also gives the operator the post-validation command to review (with normalised policy_provider, etc.) instead of raw LLM output, which is closer to what they'd want to approve anyway. Minor compared to the other findings, but easy to fix.

[yinsong1986]

policy_provider defaults to "mock" when key is absent — even for start actions targeting real hardware.

R4-1's fix correctly closed the explicit-None hole, but the else: out["policy_provider"] = "mock" fallback is now reachable from any execute/start command that doesn't carry the key. On a real-hardware peer running lerobot_local, this silently routes to the mock provider unless every caller remembers to set it.

Pre-PR the receiver-side _dispatch had its own cmd.get("policy_provider", "mock") default (still present at core.py:737), so there's no regression — but this is the right place to surface the issue, because the validator is the new single source of truth. Two cleaner options:

1. Make the key required for execute/start (raise ValidationError when absent). Forces explicit choice on every caller.
2. Plumb the default from the robot's actual provider rather than hard-coding mock here.

Low priority — call this out as a maintenance note rather than a blocker. The current code is internally consistent and matches the legacy default.

[cagataycali]

Round 8 — Red Team Findings Fixed (R8-2 through R8-7)

Commit: 1c03678 on security/pentest-hardening

Addresses 6 findings from autonomous red-team analysis (issue cagataycali/strands-gtc-nvidia#339). R8-1 was confirmed NOT a bug (IoT/bridge envelope integrity is correct by design).

---

Changes

Finding	Severity	Fix	Regression Test
R8-2	HIGH	_on_safety_estop / _on_safety_resume now emit publish_safety_event audit records (remote_estop_engaged / remote_resume_applied)	test_r8_2_remote_estop_audited, test_r8_2_remote_resume_audited
R8-3	LOW	Rotation loop iterates range(max_files, 0, -1) and discards at n+1 > max_files (was max_files-1)	test_r8_3_audit_rotation_keeps_exact_max_files
R8-4	MED-HIGH	Resume envelope now carries HMAC proof-of-override-code; receivers verify before clearing lockout	test_r8_4_resume_without_proof_rejected, test_r8_4_resume_with_valid_proof_accepted
R8-5	MED	Legacy bare-dict payloads now get content-fingerprint nonce (SHA-256); literal replay blocked	test_r8_5_legacy_bare_dict_replay_blocked
R8-6	LOW-MED	Mesh.send() / Mesh.broadcast() now call validate_command() before _put_signed	test_r8_6_mesh_send_validates_client_side, test_r8_6_mesh_broadcast_validates_client_side
R8-7	NONE	Replaced raise-then-catch scaffolding in _on_response with logger.debug	test_r8_7_on_response_no_raise_catch_scaffolding

Test Results

689 passed, 2 skipped in 26.70s

All existing tests continue to pass. 9 new regression tests added in tests/mesh/test_security_regressions.py.

Files Changed

• strands_robots/mesh/core.py — R8-2, R8-4, R8-6, R8-7
• strands_robots/mesh/security.py — R8-5
• strands_robots/mesh/audit.py — R8-3
• tests/mesh/test_security_regressions.py — +9 regression tests
• CHANGELOG.md — Round 8 entry

Known Limitations (documented)

• R8-5: Legacy bare-dict replay protection catches literal-payload replays only. Forged-timestamp variants are not caught (fundamental limitation of permissive mode). Strict mode closes this surface entirely.
• R8-4: When STRANDS_MESH_OVERRIDE_CODE is NOT configured, resume proof verification is skipped (backward compatible with the existing trust-PSK model).

@yinsong1986 — ready for review when CI is green.

[cagataycali]

Round 8 — force-pushed c6d20c0 (consolidates two parallel R8 efforts)

Two agents worked on round-8 in parallel from 4d9d3e6:

• 1c03678 (red-team agent): R8-2..R8-7 with 9 regression tests
• c6d20c0 (this agent): R8-1..R8-10 with 18 regression tests — strict superset

Force-pushed c6d20c0 since it covers everything in 1c03678 plus additional fixes that the red-team agent didn't address.

What's now on the branch

ID	Severity	Finding	Test
R8-1	HIGH	Receiver-side _on_safety_estop / _on_safety_resume now write audit records via publish_safety_event (remote_estop_engaged, remote_resume_applied)	test_r8_1_remote_estop_engages_audit_record, test_r8_1_remote_resume_writes_audit_record
R8-2	HIGH	Legacy bare-dict payloads now have content-fingerprint replay protection ("L:" + sha256(_canonical_bytes))	test_r8_2_legacy_bare_dict_replay_blocked, test_r8_2_distinct_legacy_dicts_both_pass
R8-3	LOW	Removed dead AuthorizationError raise-then-catch scaffolding; class deleted entirely	test_r8_3_authorization_error_class_is_gone, test_r4_8_supersedes_authorization_error_deleted_per_r8_3
R8-4	BUG	Audit-log rotation off-by-one: predicate corrected from n+1 > max_files-1 to n+1 > max_files. MAX_FILES=5 now genuinely keeps 5 rotated copies.	test_r8_4_rotation_reaches_max_files
R8-5	HIGH	Fleet-wide resume now requires HMAC-of-override-code proof in the envelope. Receivers without STRANDS_MESH_OVERRIDE_CODE configured FAIL CLOSED.	test_r8_5_remote_resume_requires_override_proof, test_r8_5_remote_resume_rejects_wrong_override, test_r8_5_receiver_without_override_code_fails_closed
R8-6	MED	Mesh.send and Mesh.broadcast now run client-side validate_command before publishing. Bad cmd never reaches _put_signed.	test_r8_6_mesh_send_validates_client_side, test_r8_6_mesh_broadcast_validates_client_side
R8-7	MED	robot_mesh broadcast tool now parses + validates the command JSON BEFORE raising the HITL interrupt. Operators approve the post-validation form. Declined broadcasts no longer consume rate slots.	test_r8_7_broadcast_validates_before_hitl_interrupt, test_r8_7_declined_broadcast_does_not_consume_rate_slot
R8-10	CodeQL #229	Deleted unused _AMAZON_ROOT_CA1_SHA256 alias. Internal code references the tuple directly.	test_r8_10_amazon_root_ca1_sha256_alias_is_gone
R5-defensive	polish	Belt-and-suspenders BROADCAST_RESPONDER guard at the _expected_responders[turn] = target assignment site (in addition to the public Mesh.send entry-point guard)	test_r5_defensive_send_broadcast_responder_sentinel_rejected_at_assignment
R7-policy-lc	polish	Locked policy_provider lowercase contract with regression test	test_r7_policy_provider_lowercase_contract
R5-flood	polish	Pinned permissive-mode nonce-cache flood eviction behaviour (cap=10 000, GC drops oldest 20%)	test_r5_permissive_replay_cache_flood_bounded

Stats

• 697 mesh tests passing (was 680 at R7 baseline; +17 R8)
• 108 regression tests in test_security_regressions.py (was 49)
• ruff check, ruff format --check, mypy strands_robots/mesh/ all clean

R8-1 false-alarm note (red-team finding)

A red-team analysis hypothesised that the IoT/bridge transport might mutate the signed envelope and break strict-mode HMAC verification. Independently verified as a FALSE ALARM: _canonical_bytes operates on the deserialised dict (not wire bytes), so transport serialization with default JSON separators is irrelevant. The receiver does json.loads(raw) before HMAC verification, recomputing _canonical_bytes on the same logical dict. No test added for this hypothesis since there's no bug to pin.

Trade-off note on R8-5

Took Option A (HMAC proof in envelope) over Option B (docs-only). The asymmetry is now explicit and documented: receivers without STRANDS_MESH_OVERRIDE_CODE configured fail closed and refuse remote resumes. Operators must distribute the override code to every peer for fleet-wide remote resume to work. Symmetric with how PSK + override_code already work in tandem on the local path.

@yinsong1986 ready for another look.

[yinsong1986]

Summary

This PR adds a single security boundary (mesh/security.py) between the wire and the Mesh API: HMAC-SHA256 signed envelopes with replay protection, a command-validation allowlist, per-sender token-bucket rate limiting, an emergency-stop fleet lockout with override-code proof, an integrity-checked audit log (per-record HMAC + per-peer monotonic seq + cross-process flock + sidecar persistence + O_NOFOLLOW symlink defence + size-bounded rotation), HITL approval for fleet-wide LLM tool actions, IoT policy scoping, CA pin pluralization, and bridge-transport cross-transport dedup. The implementation is thorough, the threat model is clearly documented, and the eight rounds of review-feedback regression tests are exactly the pattern AGENTS.md > Review Learnings (#85) > 'Pin regression tests for reviewed fixes' calls for.

What's good

• mesh/security.py is a single home for HMAC primitives, schema, allowlists, and rate limiting. The exception hierarchy (SecurityError -> Authentication/Validation/RateLimit/Lockout) maps cleanly onto typed audit events.
• Permissive vs strict mode is explicit, documented, and the receiver-side safety handlers (_on_safety_estop/_on_safety_resume) refuse to act in permissive mode rather than silently fanning out a LAN-attacker DoS. Good fail-closed default.
• R8-5's HMAC-of-(override_code, proof_nonce) on the resume envelope correctly closes the gap that the override code only protected the local issuer pre-fix.
• Wide regression-test surface (697 tests, +250 vs baseline) with one test per round-of-review fix and one test per pentest finding. test_security_regressions.py and test_pentest_findings.py are exactly the regression-pinning rigour the repo asks for.
• Public API hygiene: Mesh.publish was promoted as the cross-module callable so iot/camera_offload.py no longer couples to a private _put_signed. Matches AGENTS.md > Review Learnings (#86) > Public API Hygiene.
• Audit log rotation, per-record HMAC, per-peer seq, and O_NOFOLLOW are textbook forensic-trail engineering. The asymmetric tolerance (60s past, 5s forward) on freshness is the correct lean.

Concerns

• The PR description's section 12 already flags two real follow-up items (rate-limit consumed before lockout check; CA-pin time bomb). The CA-pin one was partially addressed by R7-3 (STRANDS_MESH_CA_PINS env) but operators still need a mesh-doctor surface to stage rotations safely. Worth tracking on the project board before this lands.
• 11 new STRANDS_MESH_* env vars in one PR. Documented in README and CHANGELOG, but operators with existing deployments will need a config audit. Consider a mesh.config.dump() helper (or mesh-doctor config) to make the active configuration visible at runtime — the Round-4 known limitations block in security.py's docstring already tells operators that permissive-mode replay protection has caveats they need to think about; making the active mode discoverable closes that loop.
• The _canonical_bytes helper exists in BOTH mesh/security.py (line 349) and mesh/audit.py (line 518) with slightly different semantics (audit's version excludes the sig key). Two implementations of essentially the same primitive in adjacent modules is a future-bug surface — if a refactor changes one's separator/sort behaviour and not the other, signature verification silently breaks. Pull the shared helper into a single home (e.g. security._canonical_bytes(payload, *, exclude=())) and have audit.py call it.
• One small docstring drift: _resume_lockout's docstring (core.py:1268) still shows the legacy {"status": "error", "error": "<reason>"} shape in its Returns section even though R5-4 narrowed every error path to the single generic {"status": "error", "error": "resume rejected"} form. Future readers will be misled.

Verification suggestions

# 1. Strict-mode end-to-end smoke (PSK + REQUIRE_AUTH + override + audit signing)
export STRANDS_MESH_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_REQUIRE_AUTH=true
export STRANDS_MESH_AUDIT_PSK=$(openssl rand -hex 32)
export STRANDS_MESH_OVERRIDE_CODE=$(openssl rand -hex 16)
hatch run test tests/mesh/

# 2. Confirm rotation actually reaches max_files=5 on disk (R8-4 fix)
rm -f ~/.strands_robots/mesh_audit.jsonl*
export STRANDS_MESH_AUDIT_MAX_BYTES=2048 STRANDS_MESH_AUDIT_MAX_FILES=5
python -c "
from strands_robots.mesh.audit import log_safety_event
for i in range(2000):
    log_safety_event('test', 'p', {'i': i, 'pad': 'x'*200})
"
ls -1 ~/.strands_robots/mesh_audit.jsonl* | wc -l   # should print 6 (active + .1..5)

# 3. Pentest-style negative tests: forge a response with another peer's responder_id
#    on a point-to-point send, and confirm it lands in the audit log as
#    'response_hijack_rejected' rather than being delivered to the caller.
hatch run test tests/mesh/test_pentest_findings.py -k response_hijack -v

# 4. Permissive-mode replay surface (R4-6) — flood unique nonces and confirm
#    the cache stays bounded but does NOT lose protection for legitimate
#    in-window replays. Already covered by test_security_regressions.py but
#    a manual run with `-s` makes the GC walk visible.
hatch run test tests/mesh/test_security_regressions.py -k flood -v -s

[yinsong1986]

Inbound sender_id is unvalidated and flows directly into the response topic key.

sender_id arrives from a successfully-verified envelope, so the peer holds the PSK — but init_mesh enforces _PEER_ID_PATTERN (^[a-zA-Z0-9][a-zA-Z0-9._\-]{0,127}$) only on the LOCAL peer's id at construction, never on inbound sender_ids. A PSK-holding peer can claim any string as sender_id (including "*", "../broadcast", "safety/estop", or another legitimate peer's id) and this line interpolates it into strands/{sender}/response/{turn}.

Consequences within the PSK trust boundary:

1. Audit log spoofing: command_rejected, command_rejected_lockout, rate_limit_drop all record sender = data.get("sender_id"). A forensic walker cannot distinguish a real peer's misbehaviour from an attacker's framing.
2. Topic-pattern abuse: a sender_id containing / or Zenoh wildcard chars publishes the response (and the validation-error/lockout-error envelopes at lines 611, 655, 693) on a non-canonical topic — a way to flood arbitrary strands/*/response/* topics or to publish into strands/safety/... paths that other peers' subscribers may pick up.

Fix: validate sender_id against _PEER_ID_PATTERN (or a tighter equivalent) immediately after verify_envelope returns in _on_cmd/_on_response/_on_presence and reject on mismatch, with an audit entry. Pin a regression test (test_inbound_sender_id_rejected_on_wildcard_chars etc.) per AGENTS.md > Review Learnings (#85) > 'Pin regression tests for reviewed fixes'.

[yinsong1986]

Public method raises while holding _rpc_lock, violating the documented return contract.

Mesh.send() is documented to return error dicts (see lines 1014, 1031). Two lines above, the same defensive check on target == BROADCAST_RESPONDER returns {"status": "error", "error": "..."} cleanly. This belt-and-suspenders inner copy raise ValueError(...) at line 1050:

1. Breaks the API contract — callers expecting dict get an exception.
2. Raises while still holding _rpc_lock (we're inside with self._rpc_lock:). The lock releases via context-manager unwind, fine — but the popped state at lines 1048-1049 leaves no opportunity for the dispatcher's finally block to fire because the with self._rpc_lock exit clears the state we just popped, and the outer try/finally (lines 1053-1060) is never entered.
3. Inconsistent with the public guard above which returns rather than raising.

Match the public guard's contract: change raise ValueError(...) to a clean dict return after popping state, e.g. return {"status": "error", "error": "send: target may not equal BROADCAST_RESPONDER or contain NUL"}. AGENTS.md > Review Learnings (#85) > 'Error Handling Contracts': "Return error dicts, never raise. ... Never raise exceptions that bypass the structured response."

[yinsong1986]

except OSError as oe: raise oe is dead-code masquerading as defence.

This pattern is identical to no try/except at all — OSError propagates the same way. The accompanying comment ("ELOOP under O_NOFOLLOW = symlink -> retry without it is NOT what we want; just re-raise") tells the reader the intent, but the code itself adds nothing. Three concerns:

1. AGENTS.md > Key Conventions feat: training pipeline, GPU backends, dataset recording, and observability #10: 'No dead code'.
2. The wider except Exception block at line 723 below catches the re-raised OSError and tries to os.close(fd) — but if os.open raised, fd was never assigned, so try: os.close(fd) will trigger UnboundLocalError, masking the original symlink-rejection exception with a noisy traceback.
3. AGENTS.md > Review Learnings (feat: Robot() factory + top-level lazy imports #86) > Exception Clauses Must Be Narrow: 'except Exception is forbidden for non-recovery code paths'. The clause at line 723 catches everything to clean up fd, but the only error paths that need it are from os.fdopen itself.

Delete lines 706-711 entirely (the os.open will raise naturally). Tighten the outer except Exception to except OSError to match what os.fdopen can actually raise during open.

[yinsong1986]

Inconsistent clamp behaviour between MAX_BYTES and MAX_FILES env vars.

_resolve_log_max_bytes (line 213) emits a WARNING when an operator-supplied value exceeds _LOG_MAX_BYTES_CAP. This sibling helper silently coerces a STRANDS_MESH_AUDIT_MAX_FILES=200 to 100 with no log line. An operator who set 200 expecting 200 rotations of forensic history will silently lose the last 100. Same goes for the < 1 -> 1 clamp at line 232, and the ValueError parse path at line 230 which falls through to the default with no warning.

Fix: mirror the MAX_BYTES helper's WARNING-on-clamp and WARNING-on-invalid-parse pattern so clamp / fallback events are visible to operators investigating 'why is my retention only 5 files?' This is small but matters for audit log forensics — silent clamps undermine the trust the rest of the module is trying to build.

[yinsong1986]

R8-2 closes the literal-replay surface for legacy bare dicts but the residual risk is wider than the inline comment suggests.

The content-fingerprint nonce protects against a captured legacy bare dict being replayed while the entry is still in _NONCE_CACHE. But:

1. The nonce-cache GC (line 327-337) drops the oldest 20% on overflow and the entire cache turns over after _replay_window_s() for normal traffic. A LAN attacker who flood-fills the cache with throwaway envelopes (R4-6, already documented as a known limitation) can specifically target the eviction of a captured legacy dict to make its fingerprint fall out of the cache, then replay it.
2. The fingerprint covers the entire dict including any embedded timestamp, but legacy dicts by definition predate envelope ts, so freshness is not enforced and the captured payload remains semantically valid for as long as the underlying command is.

This is acknowledged inline ("residual risk operators must accept"), but operators reading the README's 'Permissive (default)' guidance currently see only the one-time WARNING. Recommend escalating the README's permissive-mode bullet to call out replay-after-eviction explicitly, and consider gating the legacy bare-dict acceptance on an explicit STRANDS_MESH_ALLOW_LEGACY_BARE=true env var so operators rolling out a fleet must opt into the residual risk rather than inherit it via permissive defaults.

[yinsong1986]

Summary

This PR rewires the mesh security boundary onto Zenoh built-ins: _zenoh_config.py emits namespace + scouting + transport caps + downsampling + low_pass_filter + mTLS + ACL; _acl_config.py ships a default-deny role policy; security.py keeps only the payload-semantic allowlists. The audit-log hardening (per-record HMAC, per-peer seq, sidecar + flock, rotation) and the HITL-interrupt path in tools/robot_mesh.py are real strengths. Test count is genuinely impressive.

The direction is correct — leaning on transport-level identity is strictly better than a hand-rolled HMAC envelope. But the actual implementation has gaps that the docstrings claim are closed; flagging the highest-impact ones inline.

What's good

• Splitting wire auth (_zenoh_config/_acl_config) from payload validation (security.py) is the right factoring; validate_command's allowlists are exactly the post-ACL surface.
• Audit log: per-record HMAC + per-peer monotonic seq + cross-process flock + symlink defence + rotation. The verify_audit_integrity cursor not advancing on bad_signature records is a thoughtful touch.
• HITL interrupt + _rate_limit_check/_rate_limit_record split in robot_mesh is the right shape — declined approvals not consuming slots is the safety property that matters.
• _resume_lockout's generic {"status": "ok"} / {"status": "error", "error": "resume rejected"} shape (R5-4) avoids leaking oracle-shaped errors. Constant-time compare is correct.
• CA pin re-use path correctly refuses to honour STRANDS_MESH_DISABLE_CA_PIN on the on-disk path; per-recv timeout via custom HTTPSHandler (R7-2) is the right fix for the process-global setdefaulttimeout foot-gun.

Concerns

High: PR description is significantly out of sync with the code

The PR description on GitHub (sections 1–8, the operator quick-start matrix, the threat-vector matrix) is written against the previous iteration of this PR. It still claims:

• STRANDS_MESH_PSK and STRANDS_MESH_REQUIRE_AUTH are the operator knobs ("production posture: PSK + REQUIRE_AUTH=true").
• sign_envelope / verify_envelope are the inbound/outbound chokepoints ("every outbound message goes through Mesh._put_signed … verify_envelope is the only inbound entry").
• Permissive vs Mixed vs Strict tri-state replay protection.
• HMAC envelope on every publish closes vector #1.

None of that matches 7113742. The current code has no PSK, no envelope, no sign_envelope/verify_envelope, no permissive/strict modes — those were all replaced with mTLS + ACL. The CHANGELOG entry under "Final refactor" describes the new shape; the PR description prose did not get the same update. A reviewer relying on the description (matrix, mermaid diagrams, env-var table, threat coverage section §3) will look for code that doesn't exist and miss the actual properties of what shipped.

A single rewrite of the PR description on GitHub aligning with the CHANGELOG's "Final refactor" section would un-block reviewers materially.

Medium: scope creep — simulation deletions in a mesh-security PR

The diff also removes 359 lines from tests/simulation/mujoco/test_recording_synchronous.py, 138 lines from tests/simulation/test_policy_runner_benchmark.py, 55 lines from strands_robots/simulation/policy_runner.py, 25 lines from strands_robots/simulation/base.py, and 217 lines from strands_robots/simulation/mujoco/rendering.py. None of this is mesh-security work, and the PR title is feat(mesh): security hardening. Either split into a separate "chore: revert simulation X" PR or document the deletions in the description with a reason. As-is they read like an accidental rebase artefact.

Medium: sender_id is read from the payload, never bound to the cert CN

See inline on core.py:570. The new model relies on Zenoh mTLS to bind identity at the link, but the application code still trusts the payload's sender_id field for response routing and audit attribution. Between two authorised operators (op-1 / op-2) this is forgeable — the entire responder_id defence in _on_response only catches one half of the lateral-mischief surface.

Minor: stale references in code comments / docstrings

strands_robots/mesh/transport/bridge_transport.py:238,285 still references mesh.security.sign_envelope which no longer exists. After the rewrite the dedup code only ever exercises the fingerprint path. Worth a sweep for sign_envelope, verify_envelope, STRANDS_MESH_PSK, permissive, strict mode across the surviving comments — anything left over describes a previous architecture.

Verification suggestions

# Sanity check the new mesh security path with an actual zenoh wheel:
pip install eclipse-zenoh
hatch run test tests/mesh/test_zenoh_config.py tests/mesh/test_acl_config.py tests/mesh/test_session_config.py

# Walk every doc / comment that still references the removed surface:
rg -n 'sign_envelope|verify_envelope|STRANDS_MESH_PSK|REQUIRE_AUTH|permissive mode|strict mode' \
  strands_robots/ README.md

# Spot-check that AUTH_MODE=none really skips ACL and TLS in the emitted config:
STRANDS_MESH_AUTH_MODE=none python -c \
  'from strands_robots.mesh.session import _build_config; c = _build_config(); print(c.get_json("transport/link/tls"), c.get_json("access_control"))'

# Confirm the simulation deletions either belong here (with a stated reason)
# or split them into their own PR:
git diff upstream/main -- strands_robots/simulation tests/simulation

[yinsong1986]

sender_id is read from the payload (data.get("sender_id", "")), not from the mTLS-bound peer identity that the docstring above promises ("the sender's cert CN is bound to the link"). There is no code anywhere in _on_cmd / _exec_cmd / _on_response that extracts the cert CN from the Zenoh sample and compares it with sender_id.

Consequences between two ACL-authorised operators (op-1, op-2):

• op-1 publishes a cmd with sender_id="op-2". The cmd dispatches; rkey = f"strands/op-2/response/{turn}" (line 591) routes the response to op-2's response key. ACL may deny that publish, but the dispatch already executed.
• The audit log's command_rejected / command_rejected_lockout / rate_limit_drop records (and _audit_tool_action calls in tools/robot_mesh.py) all attribute the action to the forged sender_id, so post-incident attribution is broken.
• _on_response's responder_id check (L797-829) only protects the response-receiving side. The command-issuing side is unprotected — op-1 can issue cmds claiming to be op-2, which is the more interesting attack surface.

The fix is to either pull the CN off the Zenoh sample (sample.attachment or whatever the binding API is in the installed zenoh wheel) and overwrite sender_id with the authenticated value, or reject any payload whose sender_id does not match the link CN. Without one of these, the docstring's "identity is bound at the TLS handshake" claim is doc-only.

[yinsong1986]

_put_signed is now a one-line put(key, payload) wrapper that does no signing. The docstring at L1317-1318 says "_put_signed is preserved for callsite stability; a future refactor may rename it to _publish" — i.e., the docstring acknowledges the name no longer matches the semantics.

AGENTS.md > Review Learnings (#86) > Public API Hygiene > "Match docstrings to semantics" calls this out: keeping a misleading name across a refactor is exactly the case the convention exists to prevent. There are 12+ in-class call sites and one external caller (mesh/iot/camera_offload.py via the publish alias). Renaming _put_signed → _put (or _publish) is mechanical and wouldn't break the Mesh.publish public alias added in this PR.

Leaving this for a follow-up bakes the misleading name into every audit hit on this file (grep _put_signed will keep returning hits about "signed" publishes that aren't signed). Better to rename now while the rewrite context is fresh.

[yinsong1986]

STRANDS_MESH_AUTH_MODE=none is a single env-flip that disables both the mTLS terminator (tls_block, link_protocols_block) and the ACL (acl_block). The only mitigation is a logger.warning(...) here.

Unlike the previous PSK envelope, there is no application-layer fallback when auth_mode=none. _on_cmd / _on_safety_estop / _on_safety_resume all assume the wire is authenticated and never re-check auth_mode — they just parse JSON and dispatch. So in none mode any device on the LAN can publish on strands_robots/safety/estop and DoS the entire fleet (vector #5 / #8 in the PR description's matrix), and the README's "Mesh security" section claims this is mitigated.

This also breaks the previous PR description's claim of "sim-by-default" / fail-safe defaults — flipping a single env var on a production host now silently turns off every defence. Two options:

1. Make the docstring + README explicit that none is only safe on a network where every host is trusted (not just "a development network"), and document the regression vs the prior PSK-permissive mode in the threat matrix.
2. Keep a lightweight application-layer auth path (HMAC envelope on a separate key) as a fallback when auth_mode=none — reuses the deleted mesh/identity.py work that this PR retired.

Option 1 is cheaper but the README needs to stop calling these surfaces "mitigated" when AUTH_MODE=none.

[yinsong1986]

The hand-rolled JSON5 preprocessor (_strip_json5_comments, _quote_unquoted_keys, _strip_trailing_commas — together ~150 lines of string-walker + regex code) is parsing operator-supplied config that feeds Zenoh's ACL. A subtle bug here silently downgrades security.

Concrete issues with _quote_unquoted_keys (L134-176):

• The context check at L170 (out[-1] in "{,\n \t") inspects the previous output char. Comments are stripped in an earlier pass, so what's in out[-1] may be the residue of where the comment ended, not a syntactically meaningful token. A key right after a stripped block comment is fragile.
• key_re = re.compile(r"([A-Za-z_][A-Za-z0-9_]*)\s*:") will mis-fire on URL-shaped strings if any future ACL field accepts them outside quotes — e.g. an unquoted key_expr: foo:bar (the JSON5 spec allows the value to be unquoted only for booleans/numbers, but operators may try and the preprocessor would silently mangle it).
• No round-trip test: the test suite checks that valid input produces valid output, but there's no fuzz / property-based check that _json5_to_json(x) is equivalent to x for arbitrary valid JSON5.

The project already pulls json5 indirectly via several deps; adding pyjson5 as a hard dep for this module (or pinning it as an extra) replaces 150 lines of foot-gun with pyjson5.loads(raw). Alternative: tighten the loader to accept strict JSON only and require operators to keep ACL files JSON-clean. The current path's correctness depends on assumptions that aren't covered by tests.

[yinsong1986]

Summary

This is the round-9 rewrite that drops the application-layer HMAC envelope and leans on Zenoh's mTLS + ACL + downsampling + low_pass_filter for wire authentication, identity, replay protection, and DoS bounds. What survives is payload validation (validate_command + allowlists), audit-log HMAC + per-peer seq counters with flock cross-process safety, the operator override-code second factor on _resume_lockout, the responder-id binding in _on_response, the bridge transport dedup, and the IoT CA-pin hardening. The architectural pivot is sound: Zenoh's primitives are stronger than what was hand-rolled, the resulting tree is ~1030 net lines smaller, and the new _zenoh_config.py / _acl_config.py / built-in default-deny ACL are clean and testable.

What's good

• The pivot itself: deleting identity.py (505 lines) plus 700+ lines from security.py in favour of mTLS at the link layer is the right call. Application-layer HMAC envelopes are a smell when the underlying transport can do mutual TLS.
• _zenoh_config.py and _acl_config.py are small, pure builder modules with strict env-var clamps and clear failure modes; the default-deny ACL with robot-* / op-* / audit-* CN globs matches what an operator would write by hand.
• Audit-log hardening is genuinely thorough: per-record HMAC, per-peer monotonic seq, sidecar persistence with fcntl.flock lockfile (R5-3), O_NOFOLLOW create + symlink reject, bounded rotation, AuditPSKDegradedError to catch mid-run PSK unset (R4-2), tamper-aware cursor advance in verify_audit_integrity, and the STRANDS_MESH_OVERRIDE_CODE second factor with constant-time compare and uniform error responses (R5-4) on _resume_lockout.
• The R8-7 split between rate-limit check vs record so a declined HITL approval does not consume a slot is a real safety property, not just hygiene — three nuisance LLM prompts that an operator declines should not lock out a real emergency_stop.
• Test count: 29 mesh test files, including dedicated test_validate_command.py (44 cases), test_zenoh_config.py (28), test_acl_config.py (13), test_session_config.py (18 integration), test_audit_integrity.py, test_iot_ca_pin.py, test_pentest_findings.py. No host paths leaked.
• AGENTS.md compliance is generally good: Mesh.publish is the public alias for _put_signed (PR#86 > Public API Hygiene), the inline # noqa: BLE001 on audit_tool_action's wide catch is documented, env-var typos raise loud ValueError, all STRANDS_MESH_* vars are documented in README.

Concerns

• Scope and reviewability. The diff is +6600/-112 across 35 files, on a security-critical surface, and went through nine review rounds with significant architectural pivots. The CHANGELOG entry mixes 'pre-rewrite' rounds with the post-rewrite description and explicitly warns that the earlier round entries reference code that no longer exists (_NONCE_CACHE, identity.py, sign_envelope, STRANDS_MESH_PSK). That is honest, but it leaves the diff documentation in an inconsistent state at merge time — see one inline comment about the bridge dedup module still referring to mesh.security.sign_envelope in 2026.
• auth_mode="none" is operator-toggleable, ungated, and silently degrades fleet safety. A single env var flip removes mTLS + the ACL — and the receiver-side safety handlers (_on_safety_estop) have no fallback gate, so any LAN peer can engage a fleet-wide lockout. The current mitigation is one WARNING log line at session-open. AGENTS.md > Safety Defaults > 'Reject invalid modes loudly' + 'Sim-by-default' is the relevant pattern: production deployments should not be one typo away from broadcast-deauth on the safety topic. Worth at least an explicit refusal to combine auth_mode=none with STRANDS_ROBOT_MODE=real.
• Asymmetric trust between estop and resume. _on_safety_resume requires HMAC(STRANDS_MESH_OVERRIDE_CODE, proof_nonce) (a real second factor); _on_safety_estop requires nothing beyond ACL. So in mTLS mode, any operator-class cert can fan out a fleet-wide DoS without operator confirmation; in auth_mode=none, anyone can. The PR's own follow-up section calls out a related signal-to-noise gap (_on_cmd token-bucket-then-lockout ordering) but does not flag this asymmetry. Inline comment below.
• Reader-side false negative in verify_audit_integrity. When a verifier has no PSK but records are signed, the function continues past the per-peer cursor update. The result is ok=True and empty sequence_gaps even on a tampered log. Ops scripts using the public boolean as a 'integrity-OK' signal need either a separate verifiable: bool field or ok=False when signed > 0 and not psk_present.
• Bridge transport _CommandDeduplicator documents a defunct interface. The docstring and inline comments describe an envelope nonce path set by mesh.security.sign_envelope — a function the rewrite deleted. The fingerprint fallback works fine; only the documentation drifted, but that path is now untested against real production payloads.
• No regression test pinning the _on_safety_estop receiver-side behaviour. The audit story (remote_estop_engaged) is added in R8-1 and the docstring says the handler engages the lockout on receipt, but no test exercises the receive-side path or the trust assumption it embeds. Per AGENTS.md PR#85 > 'Pin regression tests for reviewed fixes'.

Verification suggestions

# Confirm the test count + clean lint claim from the PR description.
hatch run test tests/mesh -q
hatch run lint

# Smoke-test session config under both auth modes; verify that
# auth_mode=none WARNING is loud and that mTLS rejects a missing-CA setup.
STRANDS_MESH_AUTH_MODE=none python -c 'from strands_robots.mesh.session import _build_config; _build_config()'
STRANDS_MESH_AUTH_MODE=mtls python -c 'from strands_robots.mesh.session import _build_config; _build_config()'  # expect ValueError

# Audit log forensic round-trip — write events, tamper one, verify reports it.
python - <<'PY'
import os, tempfile, json, pathlib
os.environ['STRANDS_MESH_AUDIT_DIR'] = tempfile.mkdtemp()
os.environ['STRANDS_MESH_AUDIT_PSK'] = 'x' * 64
from strands_robots.mesh.audit import log_safety_event, audit_log_path, verify_audit_integrity
for i in range(3):
    log_safety_event('test', 'peer-a', {'i': i})
p = audit_log_path()
lines = p.read_text().splitlines()
# Tamper line 1 (flip a byte in payload).
lines[1] = lines[1].replace('"i":1', '"i":99', 1)
p.write_text('\n'.join(lines) + '\n')
print(verify_audit_integrity())  # expect ok=False, bad_signature=1
PY

# CA pin re-verify path on a stored cert.
python -c 'from strands_robots.mesh.iot.provision import verify_ca_pin; from pathlib import Path; print(verify_ca_pin(Path.home() / ".strands_robots/iot/AmazonRootCA1.pem"))'

[yinsong1986]

Asymmetric trust between estop and resume. _on_safety_resume requires HMAC(STRANDS_MESH_OVERRIDE_CODE, proof_nonce) and refuses if the local code is unset (real second factor). This handler engages a fleet-wide lockout on the strength of ACL alone. Two consequences:

1. The docstring claim 'authorisation lives at the Zenoh ACL: only peers in the operator_peer subject can publish on safety/** keys' is false when STRANDS_MESH_AUTH_MODE=none — that mode is operator-toggleable via env var and bypasses the ACL entirely, leaving any LAN peer able to fan out a fleet-wide DoS by publishing on strands/safety/estop.
2. Even in mTLS mode, any cert in the operator_peer subject can DoS the entire fleet without operator confirmation — including a stolen / leaked operator cert. _on_safety_resume's second factor explicitly defends against this for resume; estop has no parallel.

Minimum: weaken the docstring to be honest about the auth_mode=none case. Better: require an HMAC(override_code, nonce) proof on remote estop too, and have receivers without the override code fail closed (matching _on_safety_resume lines 895–902). Then the PR's claim of 'fleet-wide DoS via signed estop is mitigated' actually holds end-to-end.

AGENTS.md > Review Learnings (#86) > Safety Defaults > 'Reject invalid modes loudly'.

[yinsong1986]

Reader-side false negative when a verifier reads a signed log without the PSK.

Upstream (line ~824–827), when record.get('sig') is not None and psk is None, the loop continues — signed is incremented but bad_signature is not, AND last_seq_by_peer is never updated for that record. So an ops script running verify_audit_integrity() from a host that does not have STRANDS_MESH_AUDIT_PSK set sees ok=True and sequence_gaps=[] even on a tampered or truncated log. The psk_present field is False so a careful caller can detect the case, but the public boolean (ok) is the obvious signal and it is wrong.

Suggest one of:

• Set ok = False (and add a verifiable field) whenever signed > 0 and not psk_present. The current ok semantics already capture 'I checked everything I could check'; downgrading on unverifiable-signed-records is the natural extension.
• Or, raise / log loudly when called without a PSK against a signed log so an ops script cannot accidentally pass the boolean upstream.

A regression test that writes signed records, then calls verify_audit_integrity() from a fresh process with no PSK, and asserts ok=False would pin this.

[yinsong1986]

auth_mode=none degrades silently with one WARNING line. AGENTS.md > Review Learnings (#86) > 'Sim-by-default' / 'Reject invalid modes loudly' applies here — the production posture (mTLS + ACL) is one env-var flip away from being completely off, and there is no production guard.

Suggested hardening, all small:

• Refuse the combination STRANDS_MESH_AUTH_MODE=none + STRANDS_ROBOT_MODE=real with a ValueError at session open. Dev / lab runs in sim mode keep working; real-robot deployments cannot accidentally drop authentication.
• Document in the module docstring (and the README's auth_mode=none section) that this mode disables the entire wire-side defence stack and that the receiver-side safety handlers (_on_safety_estop etc.) trust the ACL that this mode disables.
• Consider an STRANDS_MESH_ALLOW_INSECURE=true ack to make the operator opt in twice, mirroring STRANDS_TRUST_REMOTE_CODE and STRANDS_MESH_DISABLE_CA_PIN (which already follow this pattern).

The WARNING is easily lost in startup noise; a structured second-factor opt-in would be much harder to set by mistake.

[yinsong1986]

Documentation drift after the rewrite. Both the module-level comment (lines 235–242) and this docstring describe an 'envelope nonce' set by mesh.security.sign_envelope — a function this PR's rewrite explicitly deleted. Production payloads no longer carry a top-level nonce key, so the if isinstance(nonce, str) and len(nonce) >= 8 branch is now unreachable on real traffic; only the test fixtures still produce it.

The fingerprint fallback (sender_id + turn_id + cmd) keeps the dedup correct, so this is doc / dead-branch hygiene rather than a functional bug, but per AGENTS.md > Review Learnings (#86) > 'Reference module names, not [removed APIs]' and > Key Conventions #10 ('No dead code'):

• Either drop the n:nonce branch and the test fixtures that rely on it, OR re-introduce a producer that emits a nonce on outbound _put_signed calls — choose one.
• Update the docstring + module comment to describe what the code actually does post-rewrite. Right now a future maintainer reading this will go looking for sign_envelope and find nothing.

[yinsong1986]

No regression test pins this receiver-side behaviour. R8-1 added the remote_estop_engaged audit record (CHANGELOG line near 'R8-1') and the docstring documents the lockout-on-receipt contract, but tests/mesh/test_pentest_findings.py only exercises the bridge-filter safety/estop topic at line 46/58, never the handler. grep -rn '_on_safety_estop\|remote_estop' tests/mesh/ returns no matches.

AGENTS.md > Review Learnings (#85) > 'Pin regression tests for reviewed fixes' applies: each behavioural fix in this PR should have a test that fails on the pre-fix code. Without one, a future refactor that — for example — drops the publish_safety_event call inside this branch (because 'no test asserts on it') will silently break the forensic story for the most operationally important event.

Minimal coverage: synthesize a fake sample with peer_id=other-x, dispatch to _on_safety_estop, assert _estop_lockout.is_set() is True and the audit log gained a remote_estop_engaged record. Pair with the symmetric resume case to also pin the override-code asymmetry called out separately.

[yinsong1986]

Summary

This is a large, careful piece of work — eight rounds of review feedback show in the comments and the audit-integrity story (per-record HMAC + per-peer monotonic seq + cross-process flock + sidecar persistence + O_NOFOLLOW) is genuinely thoughtful. The shift to leaning on Zenoh's built-in transport/link/tls + access_control + downsampling + low_pass_filter rather than a hand-rolled HMAC envelope is the right call and removes ~1700 lines of bespoke crypto code.

My main concerns are (a) a description-vs-implementation gap in the default ACL that materially weakens what the README promises, (b) what looks like an unintended scope creep that would silently revert PR #192, and (c) a few correctness gaps in the audit-integrity story that the test suite likely doesn't catch.

What's good

• Strong defence-in-depth around validate_command — payload-level allowlists for policy_host, policy_type, pretrained_name_or_path, and a separate hard cap on instruction length, duration, and step counts.
• Audit log gets per-record HMAC, per-peer monotonic seq, cross-process flock on the sidecar, O_NOFOLLOW on the audit file, bounded rotation. The record_is_bad → don't advance per-peer cursor logic in verify_audit_integrity is the right call (avoids the "forged seq masks a real gap" failure mode).
• HITL interrupt for emergency_stop / broadcast is delivered out-of-band from the LLM tool-arg flow, and a declined approval correctly does not consume a rate-limit slot (the R3-3 / _rate_limit_check vs _rate_limit_record split is a nice touch).
• Per-action sliding-window rate limit on the LLM tool, plus client-side validate_command on Mesh.send / Mesh.broadcast so the same gate runs on programmatic callers, not just on the LLM path.
• Generous regression-test surface: tests/mesh/test_pentest_findings.py, test_redteam_zenoh.py (live mTLS+ACL), test_audit_integrity.py, test_validate_command.py, test_zenoh_config.py, test_acl_config.py. Most reviewed fixes are pinned to a named test (R8-4 rotation, R7-1 symlink, R5-3 flock, etc.) per AGENTS.md > #85 Review Learnings > "Pin regression tests for reviewed fixes".

Concerns

• Unintended revert of PR #192 (scope creep / merge hazard). The PR branched off d7fd2fb, before #192 (synchronous camera recording) was merged into main. The diff now removes on_frame= from SimEngine.evaluate_benchmark, removes start_cameras_recording_synchronous from the rendering mixin, and deletes both tests/simulation/mujoco/test_recording_synchronous.py (359 lines) and tests/simulation/test_policy_runner_benchmark.py (138 lines). Neither the PR title nor any of the eight "round-of-review" CHANGELOG entries mention undoing #192. This needs a rebase against current main before merge — otherwise we silently break #191's fix the moment this lands.
• Default ACL contradicts the README's threat-vector matrix. README §"Authorisation: ACL on cert Common Name" and the threat-vector table promise role-separated robot-* (cannot publish on */cmd) and op-* (cannot publish on telemetry topics). mesh._acl_config.default_acl() actually emits key_exprs: ["**"] for both put and declare_subscriber in a single any_authenticated_peer subject. Any peer with a CA-signed cert can publish anywhere. The role-separated ACL only exists if the operator hand-writes STRANDS_MESH_ACL_FILE. See inline comment.
• Documentation drift in CHANGELOG. CHANGELOG line 92 says STRANDS_MESH_NAMESPACE defaults to strands_robots. The actual code default in _zenoh_config.DEFAULT_NAMESPACE is "strands" and the README agrees with the code. CHANGELOG should match.
• PENTEST.md is committed but reads like an internal scoping document. It has TODO admin links ("Talos engagement: TODO", "Threat models: TODO", "Docs: TODO") and references a pentest of an earlier PR (#101) — not this PR's hardening. If the goal is to ship pentest scope alongside the fix, the doc needs to land in a state the maintainers actually want public; if it's a working doc, it probably belongs in the same bucket as RETIRE.md / .autonomous/ (see .gitignore change).
• .gitignore change is malformed. -e on a line by itself isn't a valid gitignore directive — it looks like a stray heredoc artefact. Cosmetic but visible.

Verification suggestions

# 1. Confirm the simulation revert hazard:
git diff upstream/main..610ae840 -- strands_robots/simulation/ tests/simulation/
# Expect: removal of on_frame, start_cameras_recording_synchronous,
# and two whole test files. None of this is mentioned in the PR body.

# 2. Empirically confirm the default ACL is permissive:
python -c "
from strands_robots.mesh._acl_config import default_acl
import json
print(json.dumps(default_acl('strands'), indent=2))
"
# Expect: a single any_authenticated_peer subject with key_exprs ['**']
# for both put and declare_subscriber — i.e. role separation only if the
# operator supplies STRANDS_MESH_ACL_FILE.

# 3. Confirm verify_audit_integrity doesn't read rotated logs:
STRANDS_MESH_AUDIT_DIR=/tmp/audit-test \
STRANDS_MESH_AUDIT_PSK=test \
STRANDS_MESH_AUDIT_MAX_BYTES=1024 \
STRANDS_MESH_AUDIT_MAX_FILES=2 \
python -c "
from strands_robots.mesh.audit import log_safety_event, verify_audit_integrity, read_audit_log
for i in range(50):
    log_safety_event('emergency_stop', 'p1', {'i': i, 'pad': 'x'*200})
print('records visible to verifier:', len(read_audit_log()))
print('verify result:', verify_audit_integrity())
"
# Expect: only the active log is read, so seq cursor on p1 starts mid-stream
# and gap-detection has nothing to compare against pre-rotation events.

[yinsong1986]

Default ACL is permissive, not the role-separated default the README promises.

The README's "Authorisation: ACL on cert Common Name" section and the threat-vector matrix (Valid robot-* cert tries to publish on */cmd → Mitigated) describe a robot_publish_telemetry / operator_publish_cmds split. default_acl() actually returns one any_authenticated_peer subject with key_exprs: ["**"] for both put and declare_subscriber. Any peer holding a CA-signed cert — including a robot-* cert — can publish on <ns>/*/cmd and <ns>/safety/**.

The role-separated ACL only kicks in if the operator hand-writes STRANDS_MESH_ACL_FILE. The docstring at L222–236 acknowledges this ("Why permissive default rather than default-deny") but the README does not, and the threat-vector table is presented as the production posture.

Two paths out:

1. Make the default ACL match the README — emit the robot_publish_telemetry / operator_publish_cmds rules with cert-CN globs the way the example file documents (knowing CN globs don't actually work, this means enumeration, which means there is no usable default — option 2 is honest about that).
2. Update the README to say "the default ACL admits any authenticated peer; role separation requires STRANDS_MESH_ACL_FILE" and remove the rows from the threat-vector table that depend on the role-separated ACL being on.

Either way, the description-vs-implementation drift here is the kind of thing AGENTS.md > #86 review-learnings > "Match docstrings to semantics" calls out as a blocking concern.

[yinsong1986]

verify_audit_integrity() only reads the active log, but the writer rotates. read_audit_log() (called here when records is None) opens mesh_audit.jsonl only — it never reads mesh_audit.jsonl.1 .. .N. After even one rotation, the per-peer last_seq_by_peer cursor here starts at whatever seq the active log opens with (likely >>1 because the sidecar persists across rotations). So:

• An attacker who can publish safety events can flood-rotate to push their pre-tamper records into a file the verifier never opens — gap-detection becomes a non-tool against pre-rotation evidence.
• Even without an attacker, a normal-operations rotation would produce a verify_audit_integrity report with no gap evidence covering the rotated window — exactly the half of the audit history operators most need for a post-incident walk.

Fix: have read_audit_log (or a new read_full_audit_log) walk the rotated suffixes in order (.N ... .1 ... active) when verifying integrity. The gap-detection cursor is per-peer so the merged stream stays meaningful. Add a regression test that rotates the log mid-write and asserts no spurious gaps.

[yinsong1986]

Bare except Exception on the safety hot-path violates AGENTS.md > #86 "Exception Clauses Must Be Narrow".

Both _on_safety_estop (here) and _on_safety_resume (L890) wrap payload.to_bytes().decode() + json.loads(...) in except Exception: return. The attempted operations can fail with AttributeError, UnicodeDecodeError, json.JSONDecodeError, or TypeError. Catching Exception also swallows KeyboardInterrupt-adjacent failures, programming bugs in sample.payload.to_bytes(), and any future signature change.

The failure mode that matters here: a peer-side bug (TypeError in to_bytes due to a Zenoh API change, say) silently turns the local lockout into a no-op — the peer never engages its e-stop and the operator has no way to tell. AGENTS.md is explicit that catch-all except Exception with a return (no log) is a forbidden pattern unless gated behind an opt-out parameter.

Fix: except (AttributeError, UnicodeDecodeError, json.JSONDecodeError, TypeError) as exc: logger.warning("[safety] %s: malformed estop payload: %s", self.peer_id, exc); return. Same for _on_safety_resume.

[yinsong1986]

_AUDIT_STATE.psk_was_present snapshot is set outside the write lock — TOCTOU race.

_sign_record reads + initialises _AUDIT_STATE.psk_was_present on first call. The caller log_safety_event calls _sign_record(record) at L666 before acquiring _WRITE_LOCK at L681. Two threads on a fresh process can both observe snapshot is None simultaneously, both write psk_was_present = True, no harm in this case — but the race generalises: on a process that started with no PSK, a second thread that arrives after the env was set will observe psk_was_present = False and never trigger the AuditPSKDegradedError upgrade path the next time the env is unset.

The R4-2 hardening here is specifically about catching a process that briefly clears the env to write forgeries. Setting the snapshot under a lock is cheap and removes a real (if narrow) attack window. Either move _sign_record inside the with _WRITE_LOCK: block at L681 or use a dedicated _PSK_SNAPSHOT_LOCK.

[yinsong1986]

except RuntimeError may also swallow InterruptException on some SDK versions — verify the exception hierarchy.

The comment at L340-344 is correct in intent ("InterruptException MUST propagate up"). It's worth confirming that the strands.types.tools InterruptException (or whatever the SDK actually raises to pause the loop) is not a RuntimeError subclass — if it is, this except RuntimeError will catch the pause-the-loop signal, return an _err(...), and the gate fails open (the action never runs, but the operator is also never asked).

A quick assert not isinstance(InterruptException, type) or not issubclass(InterruptException, RuntimeError) regression test, plus widening the comment to name the exception explicitly, would lock this in. Today's behaviour relies on the SDK's exception class hierarchy not changing — that's a fragile contract for a HITL gate.

[cagataycali]

Superseded by #195

After this PR's eight rounds of review iterations, the design that
emerged is materially different from the one originally proposed:
the application-layer HMAC envelope is gone, replaced by Zenoh's
native mTLS + ACL + downsampling + low_pass_filter primitives.
That's a clean architectural pivot, but it leaves this PR's
description, CHANGELOG, and threat-vector matrix all out of sync
with the code.

Additionally, this PR was branched from d7fd2fb — predating #192
(synchronous camera recording, merged into main after this branch
opened). The diff therefore silently reverts #192's
start_cameras_recording_synchronous and deletes
tests/simulation/mujoco/test_recording_synchronous.py (359 lines)

• tests/simulation/test_policy_runner_benchmark.py (138 lines).
  Reviewer @yinsong1986 caught the regression in the most recent
  review.

#195 is the clean rebase: branched off current main so #192 stays
intact, every reviewer concern from this PR's final review folded
in (default ACL contradiction, CHANGELOG namespace mismatch, audit
verifier blind to rotated logs, PENTEST.md scoping doc dropped),
single commit with one fresh PR description carrying all the
mermaid diagrams that documented the design here.

Closing in favour of #195. The commit history on the
security/pentest-hardening branch stays available for forensic
review of the round-by-round review iterations that got us here.