sentinel_one: fix error handling for first phase of threat_event agent data collection #20484
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,15 @@ | ||
| # newer versions go on top | ||
| - version: "2.11.3" | ||
| changes: | ||
| - description: Fix error handling for first phase of agent data collection. | ||
| type: bugfix | ||
| link: https://github.com/elastic/integrations/pull/20484 | ||
| - description: Fix handling of deleted threats. | ||
| type: bugfix | ||
| link: https://github.com/elastic/integrations/pull/20484 | ||
| - description: Fix global error handler message formatting. | ||
|
There was a problem hiding this comment. Severity: 🔵 Low The 2.11.2 changelog covers the three CEL/error-formatting fixes but not the ingest pipeline null-safety fix to Details
Recommendation: Add an entry to the 2.11.2 block: - version: "2.11.2"
changes:
- description: Fix error handling for first phase of agent data collection.
type: bugfix
link: https://github.com/elastic/integrations/pull/20484
- description: Fix handling of deleted threats.
type: bugfix
link: https://github.com/elastic/integrations/pull/20484
- description: Fix global error handler message formatting.
type: bugfix
link: https://github.com/elastic/integrations/pull/20484
- description: Fix null pointer error when converting file size for threat events without file information.
type: bugfix
link: https://github.com/elastic/integrations/pull/20484🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| type: bugfix | ||
| link: https://github.com/elastic/integrations/pull/20484 | ||
| - version: "2.11.2" | ||
| changes: | ||
| - description: Set agentless deployment mode `release` field to `ga`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| # Test that a 404 on a deleted threat's explore/events endpoint does not | ||
|
There was a problem hiding this comment. Severity: 🔵 Low The new script test covers the deleted-threat 404 path but nothing exercises the headline fix — preserving a phase-1 error when DetailsThe Neither test in this data stream asserts that behaviour. Recommendation: Add a script test whose mock returns a successful threats list first (so # s1-mock/config.yml
rules:
- path: /web/api/v2.1/threats
methods: [GET]
responses:
# 1st call: one threat, drains into an empty worklist.
- status_code: 200
body: |-
{"data":[{"id":"threat-001","threatInfo":{"updatedAt":"2024-01-15T10:01:00.000Z"}}],"pagination":{"nextCursor":null}}
# 2nd call: fails while cursor.worklist exists with an empty data list.
- status_code: 500
body: |-
{"errors":[{"code":5000000,"detail":"Internal error."}]}🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| # stall the input. The mock returns two threats: a deleted one (404 on | ||
| # explore/events) followed by a valid one (200 with events). The CEL | ||
| # program should skip the deleted threat and collect events from the | ||
| # valid one. | ||
|
|
||
| [!external_stack] skip 'Skipping external stack test.' | ||
| [!exec:jq] skip 'Skipping test requiring absent jq command' | ||
|
|
||
| use_stack -profile ${CONFIG_PROFILES}/${PROFILE} | ||
| install_agent -profile ${CONFIG_PROFILES}/${PROFILE} -network_name NETWORK_NAME | ||
| docker_up -profile ${CONFIG_PROFILES}/${PROFILE} -network ${NETWORK_NAME} s1-mock | ||
| add_package -profile ${CONFIG_PROFILES}/${PROFILE} | ||
| add_package_policy -profile ${CONFIG_PROFILES}/${PROFILE} test_config.yaml DATA_STREAM_NAME | ||
|
|
||
| # Wait for 2 threat event documents from the valid threat. The deleted | ||
| # threat's 404 produces a "retry" message that the pipeline drops. | ||
| get_docs -profile ${CONFIG_PROFILES}/${PROFILE} -want 2 -confirm 15s -timeout 5m ${DATA_STREAM_NAME} | ||
| cp stdout got_docs.json | ||
|
|
||
| # Verify both documents are real threat events, not error events. | ||
| exec jq '[.hits.hits[]._source | select(.sentinel_one.threat_event != null)] | length' got_docs.json | ||
| stdout '^2$' | ||
|
|
||
| # Verify no error events were indexed. | ||
| exec jq '[.hits.hits[]._source | select(.error.message != null)] | length' got_docs.json | ||
| stdout '^0$' | ||
|
|
||
| remove_package_policy -profile ${CONFIG_PROFILES}/${PROFILE} ${DATA_STREAM_NAME} | ||
| uninstall_agent -profile ${CONFIG_PROFILES}/${PROFILE} -timeout 1m | ||
| docker_down s1-mock | ||
|
|
||
| -- test_config.yaml -- | ||
| input: cel | ||
| vars: | ||
| url: http://s1-mock:8080 | ||
| api_token: test-api-token | ||
| data_stream: | ||
| vars: | ||
| interval: 30s | ||
| batch_size: 10 | ||
| preserve_original_event: true | ||
| enable_request_tracer: false | ||
| -- s1-mock/docker-compose.yml -- | ||
| version: '2.3' | ||
| services: | ||
| s1-mock: | ||
| image: docker.elastic.co/observability/stream:v0.20.0 | ||
| hostname: s1-mock | ||
| ports: | ||
| - 8080 | ||
| environment: | ||
| PORT: "8080" | ||
| volumes: | ||
| - ./config.yml:/config.yml | ||
| command: | ||
| - http-server | ||
| - --addr=:8080 | ||
| - --config=/config.yml | ||
| -- s1-mock/config.yml -- | ||
| rules: | ||
| # Threats list: returns two threats. deleted-threat-001 has been removed | ||
| # from SentinelOne; threat-002 is still valid. The deleted threat is | ||
| # first so the CEL program hits the 404 before reaching the valid one. | ||
| - path: /web/api/v2.1/threats | ||
| methods: [GET] | ||
| request_headers: | ||
| Authorization: | ||
| - "ApiToken test-api-token" | ||
| responses: | ||
| - status_code: 200 | ||
| headers: | ||
| Content-Type: | ||
| - "application/json" | ||
| body: |- | ||
| {"data":[{"id":"deleted-threat-001","threatInfo":{"threatName":"DeletedThreat","classification":"Malware","createdAt":"2024-01-15T10:00:00.000Z","updatedAt":"2024-01-15T10:01:00.000Z"}},{"id":"threat-002","threatInfo":{"threatName":"ValidThreat","classification":"Trojan","createdAt":"2024-01-15T11:00:00.000Z","updatedAt":"2024-01-15T11:01:00.000Z"}}],"pagination":{"nextCursor":null,"totalItems":2}} | ||
|
|
||
| # Explore events for deleted threat: 404 with SentinelOne error format. | ||
| - path: /web/api/v2.1/threats/deleted-threat-001/explore/events | ||
| methods: [GET] | ||
| request_headers: | ||
| Authorization: | ||
| - "ApiToken test-api-token" | ||
| responses: | ||
| - status_code: 404 | ||
| headers: | ||
| Content-Type: | ||
| - "application/json" | ||
| body: |- | ||
| {"code":4040010,"detail":"Threat ID deleted-threat-001 not found","title":"Requested resource was not found"} | ||
|
|
||
| # Explore events for valid threat: 200 with 2 events. | ||
| - path: /web/api/v2.1/threats/threat-002/explore/events | ||
| methods: [GET] | ||
| request_headers: | ||
| Authorization: | ||
| - "ApiToken test-api-token" | ||
| responses: | ||
| - status_code: 200 | ||
| headers: | ||
| Content-Type: | ||
| - "application/json" | ||
| body: |- | ||
| {"data":[{"id":"event-001","eventType":"DNS","createdAt":"2024-01-15T11:00:00.000Z","agentId":"agent-001","processName":"explorer.exe","pid":"1234","srcIp":"89.160.20.128","dstIp":"127.0.0.1","dstPort":53},{"id":"event-002","eventType":"NETWORK","createdAt":"2024-01-15T11:01:00.000Z","agentId":"agent-001","processName":"chrome.exe","pid":"5678","srcIp":"89.160.20.128","dstIp":"127.0.0.1","dstPort":443}],"pagination":{"nextCursor":null,"totalItems":2}} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,14 +39,14 @@ program: |- | |
| "sortBy": ["updatedAt"], | ||
| "sortOrder": ["asc"], | ||
| "updatedAt__gte": [ | ||
| state.?cursor.list_updated_at_gte.orValue( | ||
| state.?cursor.last_timestamp.orValue( | ||
|
There was a problem hiding this comment. Severity: 🔵 Low This PR drops the second DetailsOptional chaining propagates absence through the rest of the selection chain, so Recommendation: Drop the redundant "list_updated_at_gte": state.?cursor.list_updated_at_gte.orValue(
state.?cursor.last_timestamp.orValue(
(now - duration(state.initial_interval)).format(time_layout.RFC3339)
)
),Mirror the change in 🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| (now - duration(state.initial_interval)).format(time_layout.RFC3339) | ||
| ) | ||
| ) | ||
| ], | ||
| ?"siteIds": state.?site_ids.optMap(v, [string(v)]), | ||
| ?"cursor": state.?cursor.next_page.token.optMap(v, [v]), | ||
| }.format_query() | ||
| ).with( | ||
| { | ||
|
|
@@ -63,8 +63,8 @@ program: |- | |
| ?"token": (body.?pagination.nextCursor.orValue(null) != null) ? optional.of(body.pagination.nextCursor) : optional.none(), | ||
| }, | ||
| "fetch_more": body.?pagination.nextCursor.orValue(null) != null, | ||
| "list_updated_at_gte": state.?cursor.list_updated_at_gte.orValue( | ||
|
There was a problem hiding this comment. Severity: 🟠 High The list watermark Details
Consequences, all present in the current code:
The sibling Recommendation: Make the key optional in phase one and drop it when there is no next page, mirroring the "fetch_more": body.?pagination.nextCursor.orValue(null) != null,
?"list_updated_at_gte": (body.?pagination.nextCursor.orValue(null) != null) ?
optional.of(
state.?cursor.list_updated_at_gte.orValue(
state.?cursor.last_timestamp.orValue(
(now - duration(state.initial_interval)).format(time_layout.RFC3339)
)
)
)
:
optional.none(),
?"last_timestamp": state.?cursor.last_timestamp,🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| state.?cursor.last_timestamp.orValue( | ||
| (now - duration(state.initial_interval)).format(time_layout.RFC3339) | ||
| ) | ||
| ), | ||
|
|
@@ -90,7 +90,9 @@ program: |- | |
| } | ||
| ) | ||
| ) | ||
| ).as(state, has(state.?events.error) ? | ||
| state | ||
| : | ||
| state.with( | ||
| !has(state.?cursor.worklist) ? | ||
| state | ||
|
|
@@ -108,14 +110,20 @@ program: |- | |
| "Authorization": ["ApiToken " + state.api_token], | ||
| }, | ||
| } | ||
| ).do_request().as(resp, (resp.StatusCode == 200 || resp.StatusCode == 404) ? | ||
|
There was a problem hiding this comment. Severity: 🟡 Medium Folding 404 into the 200 branch sends the 404 body through DetailsAccepting 404 here routes the response into The sibling Splitting the branch also removes the two now-conditional expressions the shared branch needs - Recommendation: Restore ).do_request().as(resp, (resp.StatusCode == 200) ?
resp.Body.decode_json().as(body,
# ...unchanged 200 handling...
)
: (resp.StatusCode == 404) ?
{
"events": [{"message": "retry"}],
"want_more": state.?cursor.fetch_more.orValue(false) ?
state.cursor.fetch_more
:
size(state.cursor.worklist.data) > 1,
"cursor": {
"worklist": {"data": tail(state.cursor.worklist.data)},
"next_page": {
?"token": state.?cursor.next_page.token,
},
"next_chain": {},
"fetch_more": state.?cursor.fetch_more.orValue(false),
?"list_updated_at_gte": state.?cursor.list_updated_at_gte,
?"last_timestamp": state.cursor.worklist.data[0].?threatInfo.updatedAt.or(state.?cursor.last_timestamp),
},
}
:
# ...unchanged error branch...
)🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| ( | ||
| (resp.StatusCode == 200) ? | ||
| resp.Body.decode_json() | ||
| : | ||
| // Do not bother getting 404 body since it's not used. | ||
| {} | ||
| ).as(body, | ||
| (body.?pagination.nextCursor.orValue(null) != null).as(has_more_events, | ||
| { | ||
| "data": has_more_events ? state.cursor.worklist.data : tail(state.cursor.worklist.data), | ||
| }.as(new_worklist, | ||
| { | ||
| "events": (resp.StatusCode == 200 && has(body.data) && body.data.size() > 0) ? | ||
| body.data.map(e, | ||
| { | ||
| "message": e.encode_json(), | ||
|
|
@@ -136,7 +144,16 @@ program: |- | |
| ?"token": has_more_events ? optional.of(body.pagination.nextCursor) : optional.none(), | ||
| }, | ||
| "fetch_more": state.?cursor.fetch_more.orValue(false), | ||
| ?"list_updated_at_gte": (body.?pagination.nextCursor.orValue(null) != null) ? | ||
|
There was a problem hiding this comment. Severity: 🟡 Medium The new Details
Recommendation: Clear on the threats-list pagination state ( ?"list_updated_at_gte": state.?cursor.fetch_more.orValue(false) ?
optional.of(
state.?cursor.list_updated_at_gte.orValue(
state.?cursor.last_timestamp.orValue(
(now - duration(state.initial_interval)).format(time_layout.RFC3339)
)
)
)
:
optional.none(),Apply the identical change to the two rendered policy expectations ( 🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
There was a problem hiding this comment. Severity: 🟡 Medium The new Details
The two conditions are effectively inverted with respect to what the pin is for:
Concretely: after phase 1 returns page 1 of the threats list with a The sibling Recommendation: Gate the pin on the threats-list pagination state ( Remember to regenerate 🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
There was a problem hiding this comment. Severity: 🟡 Medium The new Details
Those two conditions are inverted with respect to when the value is read. When Concrete effect on the multi-page path: This change is also not described in the 2.11.3 changelog entries, which cover only the phase-1 error handling, the deleted-threat 404, and the on_failure message formatting. Recommendation: Gate the retention on the threats-list pagination flag ( ?"list_updated_at_gte": state.?cursor.fetch_more.orValue(false) ?
optional.of(
state.?cursor.list_updated_at_gte.orValue(
state.?cursor.last_timestamp.orValue(
(now - duration(state.initial_interval)).format(time_layout.RFC3339)
)
)
)
:
optional.none(),Apply the same edit to the rendered policy fixtures ( 🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
||
| optional.of( | ||
| state.?cursor.list_updated_at_gte.orValue( | ||
| state.?cursor.last_timestamp.orValue( | ||
| (now - duration(state.initial_interval)).format(time_layout.RFC3339) | ||
| ) | ||
| ) | ||
| ) | ||
| : | ||
| optional.none(), | ||
| ?"last_timestamp": has_more_events ? | ||
| state.?cursor.last_timestamp | ||
| : | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Severity: 🔵 Low
confidence: mediumpath: packages/sentinel_one/changelog.yml:4The changelog entry says "first phase of agent data collection" without naming the data stream; since this package also ships an
agentdata stream it reads as if that stream changed. Namethreat_eventexplicitly.Details
All three entries in 2.11.2 describe changes that are specific to the
threat_eventdata stream, but none of them says so. The first entry is the most ambiguous:packages/sentinel_one/data_stream/agent/exists, so "agent data collection" is naturally read as theagentdata stream rather than as Elastic Agent collection forthreat_event. The changelog is user-facing and is what an operator reads when deciding whether an upgrade affects them.Recommendation:
Name the data stream in each entry:
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills