Repository navigation
[New Integration] added SpecterOps BloodHound Enterprise integration - #21063
omkarj-metron wants to merge 13 commits into
Conversation
ReviewersBuildkite won't run for external contributors automatically; you need to add a comment:
NOTE: https://github.com/elastic/integrations/blob/main/.buildkite/pull-requests.json contains all those details. |
|
/test |
efd6
left a comment
There was a problem hiding this comment.
Please add system tests and pipeline test expectations.
There was a problem hiding this comment.
This needs a test expectation file.
There was a problem hiding this comment.
Added test-finding-attack-path.log-expected.json covering both sample documents (PascalCase attack-path payload and the nested camelCase variant).
| @@ -0,0 +1 @@ | |||
| {"id":"broken", "Finding": | |||
There was a problem hiding this comment.
Removed it. The JSON processor uses ignore_failure: true, so a truncated line does not exercise pipeline error handling and was not a useful fixture.
There was a problem hiding this comment.
Please celfmt this file. You can install the program with go install github.com/elastic/celfmt/cmd/celfmt@latest.
There was a problem hiding this comment.
Formatted with celfmt -agent.
| ).as(uri, | ||
|
|
||
| "".as(body_data, |
There was a problem hiding this comment.
Finding collection only uses GET, but BloodHound’s HMAC signature still covers the request body. body_data is bound to "" once so the same empty body is used for both the signature and request.Body. Added a comment above the program to make that explicit.
| on_failure: | ||
| - set: | ||
| field: error.message | ||
| value: "{{ _ingest.on_failure_message }}" |
There was a problem hiding this comment.
| on_failure: | |
| - set: | |
| field: error.message | |
| value: "{{ _ingest.on_failure_message }}" | |
| - set: | |
| tag: set_event_kind_92954dfa | |
| field: event.kind | |
| value: pipeline_error | |
| if: ctx.error?.message != null | |
| - append: | |
| tag: append_tags_9fe66b2c | |
| field: tags | |
| value: preserve_original_event | |
| allow_duplicates: false | |
| if: ctx.error?.message != null | |
| on_failure: | |
| - set: | |
| tag: set_event_kind_72b1902f | |
| field: event.kind | |
| value: pipeline_error | |
| - append: | |
| tag: append_tags_279d5a5c | |
| field: tags | |
| value: preserve_original_event | |
| allow_duplicates: false | |
| - append: | |
| tag: append_error_message_fb774d38 | |
| field: error.message | |
| value: >- | |
| Processor '{{{ _ingest.on_failure_processor_type }}}' | |
| {{#_ingest.on_failure_processor_tag}}with tag '{{{ _ingest.on_failure_processor_tag }}}' | |
| {{/_ingest.on_failure_processor_tag}}failed with message '{{{ _ingest.on_failure_message }}}' |
| "embeddableConfig": { | ||
| "enhancements": {}, | ||
| "attributes": { | ||
| "title": "[Logs BloodHound Enterprise] Records by severity", |
There was a problem hiding this comment.
| "title": "[Logs BloodHound Enterprise] Records by severity", | |
| "title": "Records by severity", |
| "embeddableConfig": { | ||
| "enhancements": {}, | ||
| "attributes": { | ||
| "title": "[Logs BloodHound Enterprise] Records by domain", |
There was a problem hiding this comment.
| "title": "[Logs BloodHound Enterprise] Records by domain", | |
| "title": "Records by domain", |
| "embeddableConfig": { | ||
| "enhancements": {}, | ||
| "attributes": { | ||
| "title": "[Logs BloodHound Enterprise] Records by category", |
There was a problem hiding this comment.
| "title": "[Logs BloodHound Enterprise] Records by category", | |
| "title": "Records by category", |
| "embeddableConfig": { | ||
| "enhancements": {}, | ||
| "attributes": { | ||
| "title": "[Logs BloodHound Enterprise] Top attack path findings", |
There was a problem hiding this comment.
| "title": "[Logs BloodHound Enterprise] Top attack path findings", | |
| "title": "Top attack path findings", |
| } | ||
| } | ||
| }, | ||
| { |
There was a problem hiding this comment.
How does this differ from "Records by category"?
There was a problem hiding this comment.
It didn’t, both panels bucketed event.action (finding type), one as a donut and one as a bar chart. Removed the duplicate “Records by category” panel and kept “Top attack path findings”.
…treams, enhancing debugging capabilities. Removed obsolete LICENSE.txt file. Updated changelog to reflect review feedback and initial release details. Introduced new Docker deployment configurations and test files for improved integration testing.
…nditional tags for event kind and error messages.
…updated expected log output.
|
Hi @efd6, |
|
|
||
| #### finding | ||
|
|
||
| This is the `finding` dataset. Optional raw BloodHound attack-path finding documents collected via CEL. |
There was a problem hiding this comment.
| This is the `finding` dataset. Optional raw BloodHound attack-path finding documents collected via CEL. | |
| This is the `finding` dataset. Optional raw BloodHound attack-path finding documents. |
(users should not care how they get their data)
| @@ -0,0 +1,4 @@ | |||
| fields: | |||
| "@timestamp": "2026-07-24T14:50:02.760Z" | |||
There was a problem hiding this comment.
| "@timestamp": "2026-07-24T14:50:02.760Z" |
We don't need this for pipeline tests.
There was a problem hiding this comment.
This is a massive CEL program. Do we need to do all this work here? Work at the edge should really only be things that are not doable in the ingest pipeline. Please simplify it to the minimal amount of work that is absolutely needed.
Use CEL to collect data, traverse API steps where necessary, and split events from documents. Do not use it for general purpose document manipulation.
There was a problem hiding this comment.
Agreed. The finding program now only collects data: walk domains → types → paginated details, HMAC-sign BloodHound requests (that has to stay in CEL), and split each detail row into an event. Field mapping and enrichment moved to the ingest pipeline.
| ignore_missing: true | ||
| - set: | ||
| field: message | ||
| value: "{{json.title}}" |
There was a problem hiding this comment.
| value: "{{json.title}}" | |
| value: "{{{json.title}}}" |
(use triple-stache throughout; we do not need HTML escaping)
| value: >- | ||
| Processor '{{{ _ingest.on_failure_processor_type }}}' | ||
| {{#_ingest.on_failure_processor_tag}}with tag '{{{ _ingest.on_failure_processor_tag }}}' | ||
| {{/_ingest.on_failure_processor_tag}}failed with message '{{{ _ingest.on_failure_message }}}' No newline at end of file |
| description: Fixed CEL loop budget for finding collection. | ||
| required: true | ||
| show_user: false | ||
| default: "500000" |
There was a problem hiding this comment.
Please don't do this. Is there a reason that you believe this is necessary?
There was a problem hiding this comment.
Removed from the finding stream.
| @@ -0,0 +1,6 @@ | |||
| fields: | |||
| "@timestamp": "2026-07-24T14:50:02.760Z" | |||
There was a problem hiding this comment.
| "@timestamp": "2026-07-24T14:50:02.760Z" |
There was a problem hiding this comment.
Same concern here, but more so. This program is functionally unmaintainable.
There was a problem hiding this comment.
Understood, this one cannot follow the same “collect and split only” pattern as finding. health_check is Case & Alert Sync: it has to POST /api/cases, bulk-index Security alerts, and attach those alerts. Ingest pipelines cannot make those calls, so the walk has to stay in CEL.
What we changed: troubleshooting event mapping lives in the ingest pipeline, not in CEL. The program is a two-pass orchestrator (cases_only then alerts) so Kibana cases are created before alert attachment. We did try a thinner CEL here; it stopped creating Security Cases, so we kept the working sync. Happy to discuss a follow-up if there is an Elastic-supported way to do case/alert writes outside CEL or otherwise we will park it for our next iteration.
| interval: 10s | ||
| preserve_original_event: true | ||
| assert: | ||
| hit_count: 1 |
There was a problem hiding this comment.
If pagination is part of the API we want to test that at least one pagination is handled, this means that we must have at least 2 here (preferably we have state, middle and end, so at least 3). This applies to the other data stream.
| @@ -0,0 +1,9 @@ | |||
| # newer versions go on top | |||
| - version: "1.0.0" | |||
There was a problem hiding this comment.
Let's wait a bit before we release as GA. I'd suggest "0.1.0".
…g the initial release. Adjusted changelog entries and manifest versioning. Enhanced documentation for finding and health check data streams, including improved descriptions and pagination settings. Modified API response structures and update test configurations to align with new data handling.
|
Hi @efd6, |
|
Hello @efd6, |
|
Hi @efd6, following up on this, just wanted to see if you have a moment to review so we can fast-track it? |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
| (current_step == 1) ? | ||
| ( | ||
| (has(resp.StatusCode) && int(resp.StatusCode) == 401) ? | ||
| state.with({"events": [{"error": "BloodHound authentication failed (invalid token_id or token_key)", "_step": 1, "_status_code": int(resp.StatusCode), "_raw": string(resp.Body), "story": "Sync stopped because BloodHound API authentication failed. Please verify token_id and token_key."}], "step": 1, "want_more": false}) |
There was a problem hiding this comment.
Severity: 🟠 High confidence: high path: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:373
health_check emits bare maps (no message key), so the ingest pipeline never parses them and error events are rejected by the mapping; wrap each step event in message so the input, pipeline and fixtures describe the same document.
Details
Every events entry in this program is a plain map such as {"error": "...", "_step": 1, "_raw": ..., "story": ...} or {"_step": 2, "story": ...} (lines 373, 445, 514, 597, 637, 909, 1039, 1307 and every other branch). The CEL input publishes each map's keys as top-level document fields and only sets message when the map contains a message key. The pipeline (health_check/elasticsearch/ingest_pipeline/default.yml lines 19-39) renames message to event.original, runs json into bhe.sync and renames bhe.sync._step; with no message none of that runs, so in production bhe.sync.* is never populated and event.outcome is always success. The handwritten fixtures test-health-check-sync-step.log and test-health-check-sync-error.log are NDJSON lines that the test runner feeds as message, so the pipeline tests pass against a shape the input never produces. Error branches are worse: error is a top-level string, but fields/ecs.yml declares error.message, so error is mapped as an object and those documents fail with a mapper_parsing_exception; before that, the pipeline's terminate condition ctx.error?.message != null dereferences a String in Painless and throws, routing the event through on_failure. With dynamic: true on the index template the root-level _raw/_body response bodies are also mapped as ad-hoc fields.
Recommendation:
Serialise each step event into message, as the finding stream already does, and keep hard-failure errors in the input's structured error shape:
"events": [{"message": {"_step": 1, "_uri": uri, "story": "..."}.encode_json()}]
"events": {"error": {"code": string(resp.StatusCode), "message": "GET " + uri + ": " + string(resp.Body)}}, "want_more": false
If top-level fields are preferred instead, convert the fixtures to the .json {"events": [...]} form with the fields at top level and rework the pipeline; either way fixtures and producing source must agree.
Also in: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:445, packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:997, packages/bloodhound_enterprise/data_stream/health_check/_dev/test/pipeline/test-health-check-sync-step.log:1, packages/bloodhound_enterprise/data_stream/health_check/_dev/test/pipeline/test-health-check-sync-error.log:1
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| "Body": body_data, | ||
| } | ||
| ).do_request().as(resp, | ||
| bytes( |
There was a problem hiding this comment.
Severity: 🟠 High confidence: high path: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:363
HTTP status is only checked in step 1; a failed response in steps 2-9 is parsed as {} and treated as success, which can delete every existing case or create duplicates. Check resp.StatusCode before interpreting the body in each step.
Details
Lines 363-368 replace any non-JSON body with {} and only the current_step == 1 branch (lines 372-375) inspects resp.StatusCode. Every other step derives its next state from parsed_body alone, so a 5xx, 429, HTML error page or a Kibana {"statusCode":401,...} body is indistinguishable from an empty success. Traced consequences: (a) a transient error on /available-types (step 3) takes the No types found branches at lines 669-687, which do not add that domain/zone's keys to all_current_keys; the final branch at line 689 then computes stale_ids from all_current_keys, so every existing case for that domain/zone is queued and step 9 issues DELETE /api/cases?ids=... for each. (b) a failed /api/cases/_find (step 2, line 497) yields no cases key, so existing_bh_cases is empty and step 6 POSTs a duplicate case for every finding. (c) a failed /details call (step 5) is recorded as No instances found. Because this stream performs destructive writes against Kibana, swallowing failures is not safe by default.
Recommendation:
Gate each step on the status code and stop the run with a structured error before interpreting the body:
.do_request().as(resp,
(int(resp.StatusCode) < 200 || int(resp.StatusCode) >= 300) ?
state.with({
"events": {"error": {"code": string(resp.StatusCode), "message": method + " " + uri + ": " + string(resp.Body)}},
"step": 1,
"want_more": false,
})
:
resp.Body.decode_json().as(parsed_body, ...)
)
In particular, never compute stale_ids or enter step 9 unless every discovery request in steps 2-3 succeeded.
Also in: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:669, packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:497
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| required: false | ||
| show_user: false | ||
| default: | | ||
| verification_mode: none |
There was a problem hiding this comment.
Severity: 🟠 High confidence: high path: packages/bloodhound_enterprise/manifest.yml:146
The ssl var ships verification_mode: none as its live default, disabling TLS certificate verification for the BloodHound, Kibana and Elasticsearch connections that carry API keys; comment the hint out as sibling packages do.
Details
The input-level ssl yaml var (lines 139-146) defaults to verification_mode: none and both stream templates render it under resource.ssl whenever it is non-empty (health_check/agent/stream/cel.yml.hbs line 21-23, finding/agent/stream/cel.yml.hbs line 11-13). Every policy created from this package therefore sends the BloodHound signature headers and the Kibana API key over connections whose server certificate is not validated, unless the operator notices a hidden (show_user: false) advanced setting. Sibling packages such as packages/bitsight/manifest.yml line 65 keep the same hint commented out (#verification_mode: none) so the secure default applies.
Recommendation:
Keep the hint but comment it out so the default is an empty, secure configuration. If the local elastic-package stack needs relaxed verification, set it in the system test config instead.
| verification_mode: none | |
| #verification_mode: none |
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| field: ecs.version | ||
| value: "9.3.0" | ||
| - json: | ||
| field: message |
There was a problem hiding this comment.
Severity: 🟠 High confidence: high path: packages/bloodhound_enterprise/data_stream/finding/elasticsearch/ingest_pipeline/default.yml:8
The finding pipeline never preserves event.original: it parses message directly and later overwrites message, so the raw document is lost and preserve_original_event is a no-op. Add the standard rename/remove pair, parse from event.original, and declare event.original in ecs.yml.
Details
Lines 7-10 run json on message, and lines 35-39 and 209-213 overwrite message with a rendered summary. event.original is never set anywhere in this pipeline, although the CEL template exposes the preserve_original_event toggle (which adds the tag) and fields/ecs.yml does not declare event.original either. The sibling health_check pipeline (lines 19-34) implements the rename message -> event.original / remove message / parse event.original sequence, so the two streams are inconsistent. Because json also has ignore_failure: true, a malformed document silently becomes an event.kind: alert record with no fields and no original payload to triage.
Recommendation:
Mirror the health_check opening and parse from event.original:
- rename:
field: message
tag: rename_message_to_event_original
target_field: event.original
ignore_missing: true
if: ctx.event?.original == null
- remove:
field: message
tag: remove_message
ignore_missing: true
if: ctx.event?.original != null
- json:
field: event.original
target_field: json
tag: parse_json
if: ctx.event?.original != nullAdd - name: event.original with external: ecs to fields/ecs.yml, and drop ignore_failure: true on the json processor so parse failures reach the pipeline-level on_failure handler.
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| ignore_missing: true | ||
| - rename: | ||
| field: json.status | ||
| target_field: event.action |
There was a problem hiding this comment.
Severity: 🟠 High confidence: medium path: packages/bloodhound_enterprise/data_stream/finding/elasticsearch/ingest_pipeline/default.yml:33
Two consecutive unconditional rename processors share a target (json.Finding/json.status -> event.action, json.severity/json.Severity -> log.level); if both source keys are present the second rename throws and the document becomes a pipeline_error. Guard the second rename, and move the finding_type fallback before json.Finding is renamed away.
Details
rename fails with field [event.action] already exists when the target is populated. Lines 27-34 rename json.Finding to event.action and then unconditionally rename json.status to event.action; lines 54-61 do the same for both casings of severity into log.level. The handwritten fixture never carries both keys on one line, so the pipeline test passes, but a details row that includes status alongside Finding (or both severity casings) aborts the pipeline. Separately, lines 44-49 copy json.Finding into bloodhound_enterprise.finding_type after json.Finding has already been renamed at line 28, so that fallback can never fire.
Recommendation:
Make the second rename conditional on the target still being empty and move the fallback before the rename:
- set:
field: bloodhound_enterprise.finding_type
copy_from: json.Finding
tag: set_finding_type_from_finding
ignore_empty_value: true
if: ctx.json?.finding_type == null && ctx.json?.Finding != null
- rename:
field: json.Finding
target_field: event.action
tag: rename_finding_to_event_action
ignore_missing: true
- rename:
field: json.status
target_field: event.action
tag: rename_status_to_event_action
ignore_missing: true
if: ctx.event?.action == null
- rename:
field: json.Severity
target_field: log.level
tag: rename_severity_to_log_level
ignore_missing: true
if: ctx.log?.level == nullAlso in: packages/bloodhound_enterprise/data_stream/finding/elasticsearch/ingest_pipeline/default.yml:60, packages/bloodhound_enterprise/data_stream/finding/elasticsearch/ingest_pipeline/default.yml:46
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| type: boolean | ||
| description: Whether the relationship is inherited. | ||
| - name: remediation | ||
| type: keyword |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/data_stream/finding/fields/fields.yml:39
bloodhound_enterprise.remediation holds free-text remediation prose but is mapped as keyword, so long values are not indexed; map it as match_only_text.
Details
The pipeline writes json.remediation.summary or the whole json.remediation string into this field (default.yml lines 140-143 and 204-208). Remediation guidance is prose that can exceed the default keyword ignore_above, in which case the value is stored in _source but not indexed. The field is not a low-cardinality identifier.
Recommendation:
- name: remediation
type: match_only_text
description: Summary of remediation steps recommended by BloodHound Enterprise.🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| elasticsearch_url: http://{{Hostname}}:{{Port}} | ||
| data_stream: | ||
| vars: | ||
| interval: 10s |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/data_stream/health_check/_dev/test/system/test-default-config.yml:15
interval: 10s sits under data_stream.vars in the health_check system test, but health_check declares interval only at the package input level, so the value is not applied; move it to the top-level vars block.
Details
data_stream/health_check/manifest.yml declares only enable_request_tracer, preserve_original_event, tags and processors as stream vars. interval (default 1h) is declared at the policy-template input level in the root manifest.yml (line 83). Test values are resolved against declared variables at the matching level, so the policy uses the 1h default and the config misrepresents what is exercised. The test still runs because the first CEL execution happens immediately. (If the vars move to stream level per the shadowing finding, this placement becomes correct.)
Recommendation:
vars:
base_url: http://{{Hostname}}:{{Port}}
token_id: test-token-id
token_key: test-token-key
selected_environment: EXAMPLE.CORP
bhe_zones: "Tier Zero"
kibana_url: http://{{Hostname}}:{{Port}}
kibana_api_key: test-kibana-key
elasticsearch_url: http://{{Hostname}}:{{Port}}
interval: 10s
data_stream:
vars:
preserve_original_event: true🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| description: "Collect BloodHound Enterprise attack-path findings into Elastic Security Cases and Alerts for investigation and remediation tracking." | ||
| type: integration | ||
| categories: | ||
| - custom |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/manifest.yml:10
The package is categorised only as custom, so it will not appear under the Security category in the Integrations catalog; add security.
Details
The integration produces Security Cases and Alerts and ships a Security Solution-tagged dashboard, but categories lists only custom. Sibling packages owned by the same team (e.g. packages/bitwarden/manifest.yml) use security.
Recommendation:
categories:
- security🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| multi: true | ||
| show_user: false | ||
| default: | ||
| - bloodhound_integration-health_check |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/bloodhound_enterprise/data_stream/health_check/manifest.yml:34
The health_check default tag uses the wrong package name (bloodhound_integration-health_check); use bloodhound_enterprise-health_check to match the finding stream and the package.
Details
The finding stream tags events bloodhound_enterprise-finding (finding manifest line 47) and the package name is bloodhound_enterprise, but the health_check stream defaults to bloodhound_integration-health_check. The inconsistent prefix makes the tag useless for filtering by package.
Recommendation:
Use the package name as the tag prefix.
| - bloodhound_integration-health_check | |
| - bloodhound_enterprise-health_check |
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| /packages/bitdefender @elastic/security-service-integrations @elastic/sit-crest-contractors | ||
| /packages/bitsight @elastic/security-service-integrations @elastic/sit-crest-contractors | ||
| /packages/bitwarden @elastic/security-service-integrations @elastic/sit-crest-contractors | ||
| /packages/bloodhound_enterprise @elastic/security-service-integrations @elastic/sit-crest-contractors |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: .github/CODEOWNERS:174
The CODEOWNERS entry is inserted out of alphabetical order (before blacklens); move it after the blacklens line.
Details
The surrounding block is sorted (bitsight, bitwarden, blacklens, bluecoat at lines 172-176). bloodhound_enterprise was added between bitwarden and blacklens rather than between blacklens and bluecoat.
Recommendation:
/packages/bitwarden @elastic/security-service-integrations @elastic/sit-crest-contractors
/packages/blacklens @elastic/security-service-integrations @elastic/sit-crest-contractors
/packages/bloodhound_enterprise @elastic/security-service-integrations @elastic/sit-crest-contractors
/packages/bluecoat @elastic/integration-experience
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
…ed integration type to 'security', and improved manifest documentation. Adjusted health check and finding data streams for better clarity and functionality, including new fields and enhanced error handling. Updated README for deployment instructions and clarified agent support. Improved test configurations and JSON structures for consistency.
|
Hi @jamiehynds, |
| (int(resp.StatusCode) < 200 || int(resp.StatusCode) >= 300) ? | ||
| state.with({ | ||
| "events": {"error": {"code": string(resp.StatusCode), "message": method + " " + uri + ": " + string(resp.Body)}}, | ||
| "step": current_step, |
There was a problem hiding this comment.
Severity: 🟠 High confidence: medium path: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:370
A non-2xx response keeps step at the failing step, so any persistent 4xx (404 on a stale case delete, 400 on a case payload, 401 after key rotation) re-issues the same request every interval and discovery never runs again; reset to step 1 (at least for non-5xx/429) and clear the per-run position keys.
Details
The status gate at lines 367-372 returns state.with({"events": {"error": ...}, "step": current_step, "want_more": false}). Because this program keeps its whole position in plain state keys (step, finding_index, instance_index, process_findings, cases_to_delete, delete_index, existing_bh_cases), all of which persist across runs, the next interval re-executes exactly the same request with the same state. For a transient 5xx that is a reasonable retry; for a non-transient status there is no exit: a DELETE in step 9 that 404s because a user already removed the case, a POST in step 6 rejected with 400, or a 401 after key rotation each emit one error event per interval indefinitely, and steps 1-3 (domain discovery, case listing, stale detection) never run again until the agent restarts. The earlier recommendation on this gate (prior finding 293d6ed87b6c4354) was "step": 1; the current code deviates without handling the non-recoverable case. The finding stream has the same shape: its error branches at lines 220-241 do not touch step/type_index/skip, so a persistently failing /details call for one type stalls that stream on the same request.
Recommendation:
Restart from discovery when a request fails, or resume in place only for 5xx/429. Later steps are idempotent with respect to a fresh discovery pass (cases keyed by title, alerts by _id), so a restart is safe:
(int(resp.StatusCode) < 200 || int(resp.StatusCode) >= 300) ?
state.with({
"events": {"error": {"code": string(resp.StatusCode), "message": method + " " + uri + ": " + string(resp.Body)}},
"step": (int(resp.StatusCode) >= 500 || int(resp.StatusCode) == 429) ? current_step : 1,
"process_findings": [],
"cases_to_delete": [],
"all_current_keys": [],
"pending_alert_ids": [],
"pending_alert_indices": [],
"want_more": false,
})
Apply the same reset ("step": 1, "types": [], "type_index": 0, "skip": 0) in the two error branches of the finding program.
Also in: packages/bloodhound_enterprise/data_stream/finding/agent/stream/cel.yml.hbs:240
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| - drop_event: | ||
| when: | ||
| equals: | ||
| retry: true |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/bloodhound_enterprise/data_stream/finding/agent/stream/cel.yml.hbs:267
The fixed drop_event processor is indented two spaces while {{processors}} is rendered at column 0, so any user-supplied Processors value produces a sequence with mixed indentation that fails to parse; put the fixed entry at column 0 like the other CEL templates in the repo.
Details
Lines 263-270 render processors: followed by - drop_event: (two-space indent) and then {{processors}} at column 0. Fleet substitutes a root-level yaml-type variable by dumping the parsed value in place, which for a list emits - processor: items at column 0. YAML requires every entry of one block sequence to share the same indentation, so as soon as a user supplies any processor the rendered input is invalid and the stream fails to start; with the var empty the template parses, which is why the default system test does not catch it. Every other template in the repository that combines a fixed drop_event with user processors (for example packages/o365/data_stream/audit/agent/stream/cel.yml.hbs lines 591-600, and the rapid7_insightvm, proofpoint_essentials and microsoft_exchange_online_message_trace CEL templates) places the fixed entry at column 0; this file is the only one in packages/ that indents it.
Recommendation:
Dedent the fixed processor to column 0 so both halves of the sequence line up with the rendered user processors.
| - drop_event: | |
| when: | |
| equals: | |
| retry: true | |
| - drop_event: | |
| when: | |
| equals: | |
| retry: true |
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| @@ -0,0 +1,2 @@ | |||
| {"_step":5,"info":"Finished instance collection for finding type 'T0GenericAll' in domain 'EXAMPLE.CORP' and BHE zone 'Tier Zero'. Total instances collected: 3.","domains_after_filter":2} | |||
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/bloodhound_enterprise/data_stream/health_check/_dev/test/pipeline/test-health-check-sync-step.log:1
The health_check pipeline fixtures are hand-written documents the CEL program never emits (bh_cases_found, domains_after_filter, a returned 401 error string) while real step events carry story, _uri, _domain, case_key and the counters that no fixture exercises; rebuild the fixtures from actual step events and drop the two never-emitted field declarations.
Details
The health_check CEL program emits every step event as a message map built from _step, story, and step-specific keys (_uri, _domain, case_id, case_key, case_part, finding_count, stale_count, pending, attached_count, case_total_alerts, deleted_case_id). A grep of the package shows bh_cases_found and domains_after_filter appear only in the two fixtures, fields/fields.yml (lines 23 and 41) and the generated README, never in agent/stream/cel.yml.hbs; the string returned 401 likewise exists only in the error fixture, since HTTP failures are now returned as the input's structured error.message (which the pipeline terminates on) rather than as a step event. Conversely story is emitted by every step branch and appears in no fixture, and the _uri -> uri and _domain -> domain renames in the pipeline (lines 40-49) have no input that exercises them. The pipeline test therefore validates a document shape the stream never produces and leaves the real contract unverified.
Recommendation:
Replace the fixture lines with step events the program can actually emit, for example:
{"_step":1,"_uri":"/api/v2/available-domains","story":"Authentication succeeded for base_url https://example.bloodhoundenterprise.io. Selected environment input provided: 'EXAMPLE.CORP', so fetching data only for matching environments. BloodHound Enterprise zones input provided: 'Tier Zero', so fetching data only for matching zones. Fetched 1 domains and retained 1 after applying filters."}
{"_step":3,"finding_count":1,"stale_count":0,"story":"Finished discovery with 1 findings to process (creating missing cases first, then attaching alerts) and 0 stale cases prepared for cleanup."}
{"info":"No types found, advancing tag","_step":3,"_domain":"EXAMPLE.CORP","story":"No finding types were returned for domain 'EXAMPLE.CORP' current zone; advancing to the next tag."}
{"_step":7,"_uri":"/.alerts-security.alerts-default/_bulk","pending":1,"info":"Bulk indexed alerts","story":"Bulk indexed 1 alerts and attaching them to the case."}
For the error fixture use an in-band error event the program emits, e.g. {"error":"Case resolution failed","_step":6,"story":"Case creation or lookup failed, so sync skipped this finding and moved on."}. Then regenerate the expected output with elastic-package test pipeline --data-streams health_check --generate, and remove bh_cases_found / domains_after_filter from fields.yml unless the program is changed to emit them.
Also in: packages/bloodhound_enterprise/data_stream/health_check/_dev/test/pipeline/test-health-check-sync-error.log:1
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| ( | ||
| !state.existing_case_titles.exists(title, | ||
| title == f._unique_key || title.startsWith(f._unique_key + " [") | ||
| ) || state.existing_bh_cases.exists(c, |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: medium path: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:587
Findings whose case already has at least one alert are filtered out of process_findings, so principals that appear after the first successful sync are never attached and the case is never updated; drop the totalAlerts == 0 gate (switching the bulk op to create so existing alerts are not overwritten) or document the limitation.
Details
Step 3 (lines 582-592) queues a finding only when no case title matches its key, or when the matching case reports totalAlerts missing or 0. Once a case holds a single attached alert that finding type is excluded from process_findings on every later cycle, so steps 4-8 never run for it again. BloodHound findings are long-lived and gain and lose principals over time; with this gate the integration captures only the principals present on the first successful sync and silently ignores later ones. That contradicts the README ("creates and updates Kibana Security Cases ... attaches Security Alerts for at-risk principals") and the PR description ("creates/updates Elastic Security Cases"). Note that simply removing the gate while keeping the {"index": {"_id": ...}} bulk action (line 260) would re-index every existing alert each cycle and reset kibana.alert.workflow_status to open on alerts an analyst has closed or acknowledged.
Recommendation:
Process every current finding each cycle, use a create bulk action so already-indexed alerts are left untouched (a 409 per existing id is filtered out by the status < 300 check in step 7), and only attach the ids that were newly created:
).filter(f,
!state.process_findings.exists(existing, existing._unique_key == f._unique_key)
)
{"create": {"_id": alert_id}}.encode_json() + "\n" + { ... }.encode_json()
If request volume on large tenants is the concern, compare the fetched instance count with the case's totalAlerts before entering step 7 instead of skipping the finding entirely. Otherwise state plainly in the README that only the first sync populates alerts for a finding and later principals are not added.
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| - security | ||
| conditions: | ||
| kibana: | ||
| version: "^9.3.0" |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: medium path: packages/bloodhound_enterprise/manifest.yml:13
The Kibana constraint ^9.3.0 drops the whole 8.19 line without an identifiable feature that needs it; use the new-package default ^8.19.0 || ^9.1.0 or name the concrete 9.3-only dependency in the README.
Details
New packages are expected to ship with conditions.kibana.version: "^8.19.0 || ^9.1.0" unless a used feature requires a newer stack. The README (line 13) justifies ^9.3.0 only as "CEL features used by the Case & Alert Sync program", but nothing in either template is 9.3-specific: optional-field syntax (state.?step.orValue), trim_right, format_query, hmac, base64, encode_json, now.format and request().with().do_request() are all used by packages/zoom (^8.19.2 || ^9.1.2) and packages/xm_cyber (^8.18.0 || ^9.1.0); max_executions is used by the xm_cyber CEL templates on ^8.18.0; format_version: 3.5.x ships in packages/system_otel with ^8.18.0 || ^9.0.0; and deployment_modes.agentless.release: beta appears in ti_abusech, temporal and others. The Kibana Cases and Elasticsearch _bulk APIs the program calls exist throughout 8.x. Dropping 8.19 narrows the installable audience for no verified reason.
Recommendation:
Either widen the constraint to the repository default, or document in the README and PR which specific CEL function, input option, or Kibana Cases/alerts API behaviour requires 9.3:
conditions:
kibana:
version: "^8.19.0 || ^9.1.0"
elastic:
subscription: "basic"🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| - set: | ||
| field: source.domain | ||
| tag: set_source_domain_from_domain_sid | ||
| copy_from: json.DomainSID |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/data_stream/finding/elasticsearch/ingest_pipeline/default.yml:129
source.domain falls back to json.FromEnvironmentID and json.DomainSID, which are identifiers rather than domain names; keep the SID under bloodhound_enterprise.* instead of mixing it into an ECS domain-name field.
Details
The chain at lines 108-131 first copies json.domain_name / json.Environment (names) into source.domain, then falls back to json.FromEnvironmentID and json.DomainSID. A SID such as S-1-5-21-... is an identifier, not a domain name, so documents carrying only DomainSID would populate source.domain with a value that does not match the name-based values in other documents, breaking aggregations and any by-domain breakdown. The finding CEL program always injects domain_name from the parent step (cel.yml.hbs line 185), so the two identifier fallbacks are dead for the shipped collector; if they are meant for externally produced documents, the identifier should be preserved separately.
Recommendation:
Drop the two identifier fallbacks for source.domain and keep the SID as a vendor field (declare it in fields.yml):
- rename:
field: json.DomainSID
target_field: bloodhound_enterprise.domain_sid
tag: rename_domain_sid
ignore_missing: true🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| if: ctx.bloodhound_enterprise?.health_check?.error == null | ||
| on_failure: | ||
| - append: | ||
| field: error.message |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/bloodhound_enterprise/data_stream/health_check/elasticsearch/ingest_pipeline/default.yml:78
The two append processors in the health_check on_failure block have no tag, unlike every other processor in the package; add tags so failures are attributable, matching the finding pipeline.
Details
The error.message append (line 77) and the tags append (line 87) in the health_check pipeline-level on_failure block carry no tag, while the equivalent block in the finding pipeline tags both (append_error_message, append_preserve_original_event). The package otherwise tags every processor.
Recommendation:
Add tag: append_error_message to the first append and tag: append_preserve_original_event to the second, mirroring the finding pipeline's on_failure block.
| field: error.message | |
| field: error.message | |
| tag: append_error_message |
Also in: packages/bloodhound_enterprise/data_stream/health_check/elasticsearch/ingest_pipeline/default.yml:88
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| - name: interval | ||
| type: text | ||
| title: "Polling Interval" | ||
| description: Fixed efficient default for dashboard collection. |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/bloodhound_enterprise/data_stream/finding/manifest.yml:13
The finding stream's interval description says it is a default for dashboard collection, but the stream's own description and the README say the dashboard reads Security alerts, not this stream; describe what the variable actually controls.
Details
The variable description reads "Fixed efficient default for dashboard collection." The stream description on line 7 and the README both state the Attack Path dashboard uses alerts created by Case & Alert Sync and that this stream is optional raw collection. The text is user-visible in Fleet and contradicts the stream it belongs to; it also calls the value "fixed" although it is an editable text var.
Recommendation:
Describe the polling interval for raw finding collection instead.
| description: Fixed efficient default for dashboard collection. | |
| description: Time to wait between polls of the BloodHound Enterprise findings API for raw finding documents (e.g. 6h, 12h). |
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| @@ -0,0 +1,58 @@ | |||
| title: BloodHound Enterprise Sync | |||
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/data_stream/health_check/manifest.yml:1
The primary Case & Alert Sync stream is named health_check, which misdescribes its purpose and cannot be renamed later without a breaking change; pick a descriptive dataset name before the first release.
Details
The data stream directory, and therefore the permanent dataset bloodhound_enterprise.health_check, the index pattern logs-bloodhound_enterprise.health_check-* and the default tag, is health_check, while its title is "BloodHound Enterprise Sync" and the README calls it "the primary stream" that performs case creation, alert indexing and stale-case deletion. Dataset names are part of the published contract: renaming after release is a breaking-change that invalidates saved searches and README references. The PR checklist itself notes leftover scaffold naming is still being reconciled, so this is the cheapest moment to fix it.
Recommendation:
Rename the directory to something that describes the stream (for example case_sync or sync) and update the matching references: fields/base-fields.yml (event.dataset), the default tags value, the _dev/test fixtures and system test, the README {{fields}} and Discover references, and the pipeline test directory. No code suggestion is offered because the change spans a directory rename.
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| { | ||
| "_instances": ( | ||
| has(state.sync_mode) && string(state.sync_mode) == "cases_only" | ||
| ) ? [] : merged_instances, |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/bloodhound_enterprise/data_stream/health_check/agent/stream/cel.yml.hbs:814
process_findings keeps every finding's full _instances array in state for the rest of the sync and never clears it after its alerts are attached, so persisted state grows with the sum of all instances across all finding types; clear _instances when advancing to the next finding.
Details
Step 5 stores the merged details rows in _instances on the current finding (lines 812-814) when sync_mode is alerts. Every transition to the next finding (for example lines 1056-1071, 1137-1152, 1189-1204, 1240-1255) advances finding_index without mapping process_findings to empty the finished finding's _instances; the only places _instances is reset are lines 814 (cases_only mode) and 868 (no instances). The CEL input persists the full returned state after every evaluation, and each state.with copies it, so on a tenant with many finding types and large instance counts the per-step cost and registry writes grow for the whole run instead of staying bounded by one finding. Whether this has been sized against a realistically large tenant could not be determined from the source.
Recommendation:
When a finding's attachment is finished and finding_index advances, drop its instances, for example in each such branch:
"process_findings": state.process_findings.map(f,
(f._unique_key == cur_finding._unique_key) ? f.with({"_instances": []}) : f
),
Alternatively keep only _instance_count in state and re-page the details endpoint in step 7 instead of buffering all rows.
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
|
Thanks @omkarj-metron. @efd6 are there any outstanding issues or can we proceed with merge? |
Proposed commit message
Add a new SpecterOps BloodHound Enterprise Fleet integration (
bloodhound_enterprisev1.0.0).WHAT:
health_check) that polls BloodHound Enterprise, creates/updates Elastic Security Cases, attaches Security Alerts for at-risk principals, and deletes stale casesfindingdata stream for raw attack-path finding documents (disabled by default)WHY:
Checklist
changelog.ymlfile.Author's Checklist
packages/bloodhound_enterprise/elastic-package checkpasseshealth_checkandfindingsample_event.jsongenerated_dev/build/docs/README.mdlink:updated to this PR URL after openlogstream) removed if not intentional