feat(platform): add portable cybersecurity plugin framework - #9
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 352c69d1f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lowered = content.lower() | ||
| for function in profile["unsupported_functions"]: | ||
| _require( | ||
| f"{str(function).lower()}(" not in lowered, | ||
| f"{context}: unsupported function {function}", |
There was a problem hiding this comment.
When authored KQL contains an unknown field or invalid operator, validate_hunt() still succeeds because this check only rejects a small list of unsupported functions; for example, appending | extend X = DefinitelyNotARealField or | definitely_not_a_kql_operator 123 is accepted. This contradicts specs/sentinel-hunt-workbench.spec.md:28-29 and lets package checks and release-report static gates pass for KQL that cannot compile, so validate the query against the declared profile/operator grammar or stop reporting that gate as passed.
AGENTS.md reference: AGENTS.md:L156-L157
Useful? React with 👍 / 👎.
| if type_name == "duration": | ||
| text = _string(value, name, 16) | ||
| if not _DURATION.fullmatch(text): | ||
| raise ContentError(f"parameter {name} must be a bounded KQL duration such as 30m or 2h") | ||
| return text |
There was a problem hiding this comment.
When a caller supplies a value such as 999999d, this branch accepts and renders it as a supposedly bounded correlation_window; the regex limits only digit count, not elapsed time. If an analyst executes the rendered hunt, such a window can create extremely broad joins and excessive query cost, contrary to the package's bounded-parameter contract, so parse the unit and enforce an explicit maximum duration.
AGENTS.md reference: AGENTS.md:L101-L102
Useful? React with 👍 / 👎.
| if stage.get("aad_tenant_field"): | ||
| lines.append(f"| where tostring({stage['aad_tenant_field']}) == tostring(tenant_id)") |
There was a problem hiding this comment.
In a workspace containing telemetry for more than one tenant, most stages never compare the source table's TenantId with the requested tenant_id; instead, every row is stamped with the same constant TenantScope, so the later join cannot prevent cross-tenant correlations. All tables in the shipped Sentinel Analytics profile declare TenantId, but this filter runs only for the few stages with an aad_tenant_field, allowing unrelated tenant events to be reported as one scoped hunt result.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| _stage("target_logon", "DeviceLogonEvents", "TimeGenerated", "target_device", "tostring(DeviceId)", ["AccountObjectId", "DeviceId", "LogonId", "ReportId"], ["DeviceId", "ReportId", "TimeGenerated"], join_in="tostring(AccountObjectId)", join_out="tostring(AccountObjectId)", predicate="isnotempty(DeviceId)", evidence="target-device logon"), | ||
| _stage("remote_execution", "DeviceProcessEvents", "TimeGenerated", "process", "tostring(ProcessUniqueId)", ["AccountObjectId", "DeviceId", "ProcessUniqueId", "ReportId"], ["DeviceId", "ReportId", "TimeGenerated"], join_in="tostring(AccountObjectId)", join_out="tostring(ProcessUniqueId)", predicate="isnotempty(ProcessUniqueId)", evidence="process execution"), |
There was a problem hiding this comment.
When the same account logs on to multiple devices, H11 joins the target logon to process execution only by AccountObjectId, so a logon on device A can be correlated with a process on device B and presented as lateral movement on the target. The hunt hypothesis explicitly requires execution on the logged-on target device; carry DeviceId through this hop and join on both account and device to avoid unsupported correlations.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| _stage("dns_resolution", "DnsEvents", "TimeGenerated", "domain", "tostring(Name)", ["Name", "Computer"], ["Name", "Computer", "TimeGenerated"], join_in="tostring(Name)", join_out="tostring(Computer)", predicate="isnotempty(Name)", evidence="DNS resolution history"), | ||
| _stage("originating_process", "DeviceProcessEvents", "TimeGenerated", "device", "tostring(DeviceId)", ["DeviceId", "ProcessUniqueId", "FileName", "ReportId"], ["DeviceId", "ReportId", "TimeGenerated"], join_in="tostring(DeviceId)", predicate="isnotempty(ProcessUniqueId)", evidence="candidate originating process"), |
There was a problem hiding this comment.
For H06, the DNS stage emits DnsEvents.Computer as Join2, while the process stage consumes DeviceProcessEvents.DeviceId; these represent a device name and an immutable Defender device ID respectively, so normal records will not join and the advertised process-attribution hunt will miss matches. Use DeviceName on the process side or add an explicit, validated name-to-ID mapping before this correlation.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| "$PYTHON", | ||
| "plugins/logging-telemetry/security-logging-advisor/skills/repository-context/scripts/collect-repository-context.py", | ||
| "plugins/logging-telemetry/security-logging-advisor" |
There was a problem hiding this comment.
When users run the newly advertised python3 -m cops demo security-logging-advisor, this command invokes the collector with the package path and the resulting JSON includes repository_path as the absolute checkout path via os.path.abspath. That evidence is intended for analyst or model review and can disclose usernames and machine-specific directory structure when shared, so emit a repository-relative/redacted label instead of the absolute path.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
| def _remove_markers(content: str, markers: tuple[str, ...]) -> str: | ||
| lines = [line for line in content.splitlines() if not any(marker in line for marker in markers)] | ||
| return "\n".join(lines) + ("\n" if content.endswith("\n") else "") |
There was a problem hiding this comment.
Every purported semantic mutation removes only a // huntwb:... marker line, leaving the executable KQL unchanged; validate_hunt() then catches the missing required comment and the report records the mutation as detected. Consequently the advertised 144/144 critical-mutation result does not show that removing time filters, weakening joins, or changing ordering would be detected, so each mutation must alter the corresponding executable clause before calculating the mutation score.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| "objective": "Correlate spray-like authentication evidence to later account activity without treating shared egress or a successful sign-in as proof of compromise.", | ||
| "entities": ["account", "source_ip", "cloud_resource"], | ||
| "stages": [ | ||
| _stage("authentication_failures", "SigninLogs", "TimeGenerated", "account", "tostring(UserId)", ["UserId", "ResultType", "IPAddress", "Id"], ["Id"], join_out="tostring(UserId)", aad_tenant_field="AADTenantId", predicate='ResultType != "0"', evidence="failed authentication"), |
There was a problem hiding this comment.
H01's first stage accepts any single failed sign-in and immediately joins it to later activity by UserId; it never groups failures by source IP, counts distinct targeted accounts, or applies a spray threshold. A user mistyping one password before a successful sign-in can therefore satisfy the advertised password-spray chain, while genuinely distributed spray evidence is not established, so aggregate the failure stage into a bounded multi-account/source pattern before correlating downstream activity.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
1 participant
Summary
copsoperator CLIValidation
.venv/bin/python scripts/agent/check.pygit diff origin/main...HEAD --checkValidation boundaries