-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution][Endpoint] Change SentinelOne response actions to use agent.id
instead of observer.serial_number
#189535
[Security Solution][Endpoint] Change SentinelOne response actions to use agent.id
instead of observer.serial_number
#189535
Conversation
…gent-id-for-sentinelone-events
…gent-id-for-sentinelone-events
…gent-id-for-sentinelone-events
…ID_FIELDS` object
/ci |
/ci |
…gent-id-for-sentinelone-events
Looks like the only ResponseOps owned change is the snapshot showing the change of a parameter name for the connector. Just double-checking, for Backward Compatibility / Zero Down Time (BWC/ZDT) concerns, requests to run these connectors is always from the UX, and they are never queued to be run in task manager (for instance, an action on an alert). As long as that's true, the param name change shouldn't have any BWC / ZDT issues (like a queued connector execution would). |
Hi @pmuellr , Thank you for the review... Re:
That is correct. The connector and associated changes are only used from the UI/API in Kibana. Not queued from an action on an alert) |
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.
ResponseOps changes LGTM
Only change was a connector parameter name change (for ResponseOps), which can be a BWC/ZDT issue if a connector is run as an alerting action. However, this connector is not used an an alerting action, only executed via UX. So, it shouldn't have any BWC/ZDT issues.
…entinelone-events' into task/olm-9304-use-agent-id-for-sentinelone-events
…gent-id-for-sentinelone-events
…gent-id-for-sentinelone-events # Conflicts: # x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/formatted_field.tsx
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.
Thanks for the improvements! I left some comments and one idea - let me know what you think 👍
crowdstrike: 'device.id', | ||
endpoint: ['agent.id'], | ||
sentinel_one: [ | ||
'sentinel_one.alert.agent.id', |
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.
I wonder if this wouldn't make more sense to make this change on the integration side, to map all of these fields to unified 'device.id', as in CrowdStrike ? What do you think @paul-tavares ?
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.
Yeah, that would be great if there was a single place in the ES doc that contained the Agent's ID (which the device.id
seems to be it (here)), but I'm not sure how feasible that is. And it would also put a dependency on the external integration to have those changes done first before we could shift over to using it.
Since this change is required in order for us to be able to officially GA the SentinelOne functionality (RBAC is the other required feature), I'm thinking we use this approach of keeping a list of multiple field and maybe adjust again in the future if we ever get the integrations to support a common field. is that cool with you?
I'll take a note to bring this up with the Security integrations team to see what they think.
return fieldAgentType as ResponseActionAgentType; | ||
} | ||
} | ||
return 'endpoint'; |
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.
Do we need a defaulted endpoint
I guess it would end up being returned from the if condition
anyway right?
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.
I'm not sure I follow - can you clarify what you mean by "...returned from the if condition
..."????
FYI: This utility should return a ResponseAcitonAgentType
thus endpoint
here insures that.
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.
Wouldn't this return endpoint
if there is agent.id
field?
if (fieldValues.includes(agentIdEcsField)) {
return fieldAgentType as ResponseActionAgentType;
}
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.
@tomsonpl ,
I might be missing something, but your assumption that agentIdEcsField
(argument to the utility function) is actually a valid agent id field is not correct. agentIdEcsField
is a string
so we don't know if it will match or not against one of the supported agent id fields - thus why we have the return 'endpoint'
as the default return value.
label: 'Agent status', | ||
}, | ||
// Fields used in support of Response Actions | ||
...Object.values(RESPONSE_ACTIONS_ALERT_AGENT_ID_FIELDS) |
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.
Q: Is this order dependant? What would happen if there's device.id (crowdstrike field) first, and then agent.id ? Would former get overwritten by the latter?
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.
re: "is this order dependent?"
I'm not sure. I'm not completely clear how this data structure is used in the real code (not the mock here).
This change was a refactor to ensure the prior code remained supported.
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.
Code looks good to me. I've a few questions but no blockers. Tested it out as well and it works as expected.
...Object.values(RESPONSE_ACTIONS_ALERT_AGENT_ID_FIELDS) | ||
.flat() |
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.
You may want to export and use SUPPORTED_ALERT_FIELDS
here and all other instances where you're deriving a flat list from RESPONSE_ACTIONS_ALERT_AGENT_ID_FIELDS
.
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.
Will do. Thanks
@@ -144,6 +150,7 @@ describe('useHighlightedFields', () => { | |||
}); | |||
|
|||
it('should return sentinelone agent id field if data is s1 alert', () => { | |||
const agentIdField = RESPONSE_ACTIONS_ALERT_AGENT_ID_FIELDS.sentinel_one[0]; |
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.
Q. Does this test work only for sentinel_one.alert.agent.id
or should this test be updated so it tests for all the sentinel_one field values?
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.
I can change it to loop through all fields and validate them.
import linuxSvg from './logos/linux.svg'; | ||
import windowsSvg from './logos/windows.svg'; | ||
import macosSvg from './logos/macos.svg'; | ||
|
||
export type Platform = 'macos' | 'linux' | 'windows'; | ||
export type Platform = SupportedHostOsType; |
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.
We can just substitute SupportedHostOsType
instead of Platform
wherever Platform
is used.
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.
Yeah, we can. I did not do it in this PR in order for it to not get too big, but I will create a follow up PR to do that.
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.
Lgtm 👍 , thanks!
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.
Code looks good from investigations side 🚀 .
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Security Solution impacts
PR updates the SentinelOne response actions to:
sentinel_one.[data_type].agent.id
field to identify the host IDlogs-sentinel_one.alert*
index*.agent.id
field:logs-sentinel_one.alert*
logs-sentinel_one.threat*
logs-sentinel_one.activity*
logs-sentinel_one.agent*
observable.serial_number
field (the field used prior to this PR to identify the agent id in the SentinelOne document) should update the rule to use one of the new fields (see screen capture below)get-file
command (this was just release 2 weeks ago to serverless).*.alert*
and*.threat*
indexesprocesses
for SentinelOne to NOT display a Zip file passcode for the download (not needed)Connector impacts
agentId
as an argument instead ofagentUUID
Rule definition showing use of two of the SentinelOne data sources:
Checklist