fix: a collector that will not talk to us is a message, not a traceback - #48
Merged
Conversation
Two of the three profiles hardcoded failure semantics into the `event:system`
login path:
checkpoint.py ext["act"] = "Reject" (unconditional)
ext["auth_status"] = "Failed Login" (unconditional)
paloalto.py ext["PanOSEventID"] = "auth-fail" (unconditional)
The engine only ever sends `status="success"` down this path, from REP-018's
lateral movement chain. So every admin login Replicant produced on those two
vendors was internally contradictory: a Reject whose own `msg` read
"Administrator ... logged in successfully".
This is not cosmetic. REP-018's detection use case keys on *successful*
administrative logins moving host to host, so the events were wrong in exactly
the field a correlation rule matches, on exactly the technique that needs them.
An engineer tuning against this would have concluded their rule was broken.
Palo Alto was the worse of the two: `PanOSStatus` beside it carried the truth,
so the record claimed success one key over from an id that said auth-fail.
FortiGate is the oracle and was always right, carrying `status` into
`FTNTFGTstatus` and into the event name. Both fixes follow a precedent already
in the same two files: the VPN handlers (`checkpoint.py` _vpn, `paloalto.py`
_globalprotect) branch on `is_fail` correctly. The system handlers simply did not.
The guard is parametrized over every vendor rather than written per profile, so
a fourth vendor cannot reintroduce it quietly, and it asserts both directions:
success must carry no failure verdict, and failure must still read as one. A fix
that always says success is the same bug mirrored.
Observed to fail first, naming the exact offenders:
success rendered with failure semantics: {'act': 'Reject', 'auth_status': 'Failed Login'}
with FortiGate passing in the same run, which is what identified it as a
per-profile defect rather than a shared one.
799 py. Golden-line tests unaffected. black, ruff, mypy clean.
Found by an end-to-end review, reproduced before fixing.
Three defects that compound into one bad operator experience. 1. The connect timeout governed every send. `connect()` set `settimeout(connect_timeout)` (5s) on the tcp and tls sockets and never reset it, so `sendall` mid-run ran under the same ceiling and a collector applying ordinary backpressure raised socket.timeout. The tls branch had it too, which is easy to miss by reading: settimeout is applied to the raw socket BEFORE wrap_socket and survives onto the SSLSocket. Now a separate `send_timeout` (30s) takes over once connected. Deliberately not None: blocking forever trades a spurious timeout for a run that hangs with no way out. 2. `socket.timeout` IS `TimeoutError` and a subclass of `OSError`, and every run handler caught only `(RuntimeError, NotImplementedError)`. So defect 1 produced a raw traceback after a run that had looked healthy. OSError added to four handlers in cli/app.py and two in cli/menu.py. 3. `Orchestrator.send_test` is annotated `-> bool` and did not return one. `__enter__` calls `connect()`, so a refused collector escaped the contract and BOTH callers' failure branches were unreachable: the "could not reach the collector" message was already written and could never print. Two sites the review did not name, found by reproducing rather than reading: - `cli/menu.py` `_run_scenario` had no try/except at all, unlike its `_run_technique` sibling. Adding OSError to the tuples the review listed would have left the menu's scenario path still crashing. - The same shape one level down in `send_test`, above. Also fixes F-13 from the 2026-08 security review, which was visible in the output while verifying this. `drift_days` is now-minus-event-time, so a positive value means historical, and the line read "%d days from now". It described the default historical anchor as future-dated: the opposite of the truth, in the first thing an operator checks when a SIEM shows nothing. `stale_anchor_warning` in settings.py has always phrased this correctly; this line did not. Verified by running the CLI against a closed port, not only in tests: run refused: [Errno 61] Connection refused where it previously emitted a traceback. All new guards were observed to fail first. 806 py. black, ruff, mypy clean. Findings #4 and #5 of the end-to-end review, plus F-13. Reproduced before fixing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects that compound into one bad operator experience.
The connect timeout governed every send.
connect()setsettimeout(connect_timeout)(5s) on the tcp and tls sockets and never resetit, so
sendallmid-run ran under the same ceiling and a collector applyingordinary backpressure raised socket.timeout. The tls branch had it too, which
is easy to miss by reading: settimeout is applied to the raw socket BEFORE
wrap_socket and survives onto the SSLSocket. Now a separate
send_timeout(30s) takes over once connected. Deliberately not None: blocking forever
trades a spurious timeout for a run that hangs with no way out.
socket.timeoutISTimeoutErrorand a subclass ofOSError, and every runhandler caught only
(RuntimeError, NotImplementedError). So defect 1produced a raw traceback after a run that had looked healthy. OSError added
to four handlers in cli/app.py and two in cli/menu.py.
Orchestrator.send_testis annotated-> booland did not return one.__enter__callsconnect(), so a refused collector escaped the contract andBOTH callers' failure branches were unreachable: the "could not reach the
collector" message was already written and could never print.
Two sites the review did not name, found by reproducing rather than reading:
cli/menu.py_run_scenariohad no try/except at all, unlike its_run_techniquesibling. Adding OSError to the tuples the review listed wouldhave left the menu's scenario path still crashing.
send_test, above.Also fixes F-13 from the 2026-08 security review, which was visible in the
output while verifying this.
drift_daysis now-minus-event-time, so a positivevalue means historical, and the line read "%d days from now". It described the
default historical anchor as future-dated: the opposite of the truth, in the
first thing an operator checks when a SIEM shows nothing.
stale_anchor_warningin settings.py has always phrased this correctly; this line did not.
Verified by running the CLI against a closed port, not only in tests:
run refused: [Errno 61] Connection refused
where it previously emitted a traceback. All new guards were observed to fail
first.
806 py. black, ruff, mypy clean.
Findings #4 and #5 of the end-to-end review, plus F-13. Reproduced before fixing.