-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] add defend insights elastic assistant tool #198676
Conversation
1b32df0
to
e29f793
Compare
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
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.
Left some feedback that I would I would like to hear back on. Note that I focused only on files under security solution.
Also - why is there no feature flag for this code? With out one it just be available in serverless with the next release after it is merged to main
- is that ok?
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/errors.ts
Outdated
Show resolved
Hide resolved
...security_solution/server/assistant/tools/defend_insights/get_events/get_file_events_query.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/get_events/index.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/index.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/errors.ts
Show resolved
Hide resolved
...s/security_solution/server/assistant/tools/defend_insights/prompts/incompatible_antivirus.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/index.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/index.ts
Outdated
Show resolved
Hide resolved
Thanks for the review @paul-tavares. Responded directly on each one. Will update code for all of the ones I reacted with 👍.
That's an oversight by me. It's technically an internal API but we should still add a feature flag. Will update. |
x-pack/plugins/security_solution/server/assistant/tools/defend_insights/index.ts
Show resolved
Hide resolved
.../packages/kbn-elastic-assistant-common/impl/schemas/defend_insights/common_attributes.gen.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/assistant/tools/index.ts
Outdated
Show resolved
Hide resolved
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 @joeypoon!
LGTM 🚀
105700e
to
c789fa3
Compare
e3bceca
to
86812fa
Compare
Hey @joeypoon 👋 so exciting to see the cool stuff here! However, given this PR has +++4.5k lines, it's very difficult to actually get a solid understanding and therefore do a review. |
I agree with you in concept but unfortunately this one is really hard to split up. In fact, this is already split up and there will be quite a few more PRs following this 😅. Happy to walk through it with you if it helps but breaking this one up any smaller was unfortunately not feasible. |
/** | ||
* Enables the Defend Insights feature | ||
*/ | ||
defendInsights: false, |
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 adding this. I searched around and I don't actually see it being used... Where is it checked?
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.
Each of the 3 API handlers check for it like this. We don't actually use it in security_solution
plugin yet but will for following PRs.
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.
@joeypoon - Thats strange (to me). So somehow this elastic_assistant
plugin has access to our internal features? The code you pointed to is a bit hard to follow. Do you know how where it is actually is checking the feature flag is enabled/disabled?
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're checking for it at the beginning of all 3 new API handlers (in elastic_assistant) and that definition for those is actually here. The definition in x-pack/plugins/security_solution/common/experimental_features.ts
isn't actually being used right now and is just prep for next PRs to have less conflicts.
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. ok. I'm just a bit confused how the FF from security solution will be used in another plugin.
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. ok. I'm just a bit confused how the FF from security solution will be used in another plugin.
It's not, it will be used within the security solution plugin. It's only introduced here since it's the first PR being merged for the overall feature.
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 changes. I left a question regarding the feature flag, but am approving and you can just answer it when you can
* Adds a new elastic AI assistant tool, Defend Insights * this tool provides Elastic Defend configuration insights * initially it will only support incompatible antivirus detection * additional Defend Insight types can be added by simply registering a type, prompt, and context query * this is to be internally used to provide actionable Endpoint Insights such as suggesting Trusted App entries for detected incompatible antiviruses
eb9ab83
to
0059617
Compare
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 👍 Left a few questions, but just out of curiosity. Thanks!
response: estypes.SearchResponse<EsDefendInsightSchema> | ||
): DefendInsightsResponse[] => { | ||
return response.hits.hits | ||
.filter((hit) => hit._source !== undefined) |
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.
Just a thought:
Since _source: true
is included in the request, wouldn’t every document already have a _source
field?
I’m wondering if this filter is still necessary in that case.
.filter((hit) => hit._source !== undefined) | ||
.map((hit) => { | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
const insightSchema = hit._source!; |
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.
Ah, now I see - you’re ensuring _source
is always present. Just to clarify, does it actually go missing sometimes, or is this primarily to satisfy TypeScript?
const defendInsight: DefendInsightsResponse = { | ||
timestamp: insightSchema['@timestamp'], | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
id: hit._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’m not sure if using the bang
(!) operator in these places is the best approach. Wouldn’t it be clearer and safer to define the response type more explicitly instead?
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.
SearchResponse
is defined at ES level and both _id
and _source
are defined as optional. To your point though, we can do a null check instead.
createdAt: insightSchema.created_at, | ||
updatedAt: insightSchema.updated_at, | ||
lastViewedAt: insightSchema.last_viewed_at, | ||
users: |
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.
Why not just:
users: insightSchema.users ?? [];
?
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.
Honestly, I copied this over from attack discovery and just changed the necessary keys 😅. My impression though is that we want to ensure we don't potentially leak additional fields? and maybe also just because new objects.
Starting backport for target branches: 7.17, 8.15, 8.16, 8.18, 8.x https://github.com/elastic/kibana/actions/runs/11910742167 |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
|
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ic#198676) ### Summary Adds the new Defend Insights Elastic Assistant tool. This assistant tool provides Elastic Defend configuration insights. For this initial PR, only incompatible antivirus detection is supported. Telemetry is collected for success and error events. For incompatible antivirus detection, Defend Insights will review the last 200 file events for the given endpoint and output suspected antiviruses. Improvements such as customizable event count and date range will come in the future. This PR does not include any UI, that will come in a separate PR. 3 internal APIs for interacting with Defend Insights are provided here: - `POST /defend_insights` for creating a new Defend Insight - `GET /defend_insights/{id}` for getting a Defend Insight - `GET /defend_insights` for getting multiple Defend Insights - available optional query params: - `size` - default 10 - `ids` - `connector_id` - `type` - `incompatible_antivirus` - `status` - `running`, `completed`, `failed`, `canceled` - `endpoint_ids` This initial implementation does not include the LangGraph/output chunking upgrades seen in Attack Discovery due to time constraints. We'll look to make this upgrade in a future PR. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) (cherry picked from commit efc0568) # Conflicts: # .github/CODEOWNERS
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…198676) (#201104) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] add defend insights elastic assistant tool (#198676)](#198676) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Joey F. Poon","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-19T10:07:42Z","message":"[Security Solution] add defend insights elastic assistant tool (#198676)\n\n### Summary\r\nAdds the new Defend Insights Elastic Assistant tool. This assistant tool\r\nprovides Elastic Defend configuration insights. For this initial PR,\r\nonly incompatible antivirus detection is supported. Telemetry is\r\ncollected for success and error events.\r\n\r\nFor incompatible antivirus detection, Defend Insights will review the\r\nlast 200 file events for the given endpoint and output suspected\r\nantiviruses. Improvements such as customizable event count and date\r\nrange will come in the future.\r\n\r\nThis PR does not include any UI, that will come in a separate PR. 3\r\ninternal APIs for interacting with Defend Insights are provided here:\r\n- `POST /defend_insights` for creating a new Defend Insight\r\n- `GET /defend_insights/{id}` for getting a Defend Insight\r\n- `GET /defend_insights` for getting multiple Defend Insights\r\n\t- available optional query params:\r\n\t\t- `size` - default 10\r\n\t\t- `ids`\r\n\t\t- `connector_id`\r\n\t\t- `type` - `incompatible_antivirus`\r\n\t\t- `status` - `running`, `completed`, `failed`, `canceled`\r\n\t\t- `endpoint_ids`\r\n\r\nThis initial implementation does not include the LangGraph/output\r\nchunking upgrades seen in Attack Discovery due to time constraints.\r\nWe'll look to make this upgrade in a future PR.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)","sha":"efc0568e014105637332533e37491f074ec8fe2b","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Defend Workflows","backport:all-open","v8.18.0"],"number":198676,"url":"https://github.com/elastic/kibana/pull/198676","mergeCommit":{"message":"[Security Solution] add defend insights elastic assistant tool (#198676)\n\n### Summary\r\nAdds the new Defend Insights Elastic Assistant tool. This assistant tool\r\nprovides Elastic Defend configuration insights. For this initial PR,\r\nonly incompatible antivirus detection is supported. Telemetry is\r\ncollected for success and error events.\r\n\r\nFor incompatible antivirus detection, Defend Insights will review the\r\nlast 200 file events for the given endpoint and output suspected\r\nantiviruses. Improvements such as customizable event count and date\r\nrange will come in the future.\r\n\r\nThis PR does not include any UI, that will come in a separate PR. 3\r\ninternal APIs for interacting with Defend Insights are provided here:\r\n- `POST /defend_insights` for creating a new Defend Insight\r\n- `GET /defend_insights/{id}` for getting a Defend Insight\r\n- `GET /defend_insights` for getting multiple Defend Insights\r\n\t- available optional query params:\r\n\t\t- `size` - default 10\r\n\t\t- `ids`\r\n\t\t- `connector_id`\r\n\t\t- `type` - `incompatible_antivirus`\r\n\t\t- `status` - `running`, `completed`, `failed`, `canceled`\r\n\t\t- `endpoint_ids`\r\n\r\nThis initial implementation does not include the LangGraph/output\r\nchunking upgrades seen in Attack Discovery due to time constraints.\r\nWe'll look to make this upgrade in a future PR.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)","sha":"efc0568e014105637332533e37491f074ec8fe2b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198676","number":198676,"mergeCommit":{"message":"[Security Solution] add defend insights elastic assistant tool (#198676)\n\n### Summary\r\nAdds the new Defend Insights Elastic Assistant tool. This assistant tool\r\nprovides Elastic Defend configuration insights. For this initial PR,\r\nonly incompatible antivirus detection is supported. Telemetry is\r\ncollected for success and error events.\r\n\r\nFor incompatible antivirus detection, Defend Insights will review the\r\nlast 200 file events for the given endpoint and output suspected\r\nantiviruses. Improvements such as customizable event count and date\r\nrange will come in the future.\r\n\r\nThis PR does not include any UI, that will come in a separate PR. 3\r\ninternal APIs for interacting with Defend Insights are provided here:\r\n- `POST /defend_insights` for creating a new Defend Insight\r\n- `GET /defend_insights/{id}` for getting a Defend Insight\r\n- `GET /defend_insights` for getting multiple Defend Insights\r\n\t- available optional query params:\r\n\t\t- `size` - default 10\r\n\t\t- `ids`\r\n\t\t- `connector_id`\r\n\t\t- `type` - `incompatible_antivirus`\r\n\t\t- `status` - `running`, `completed`, `failed`, `canceled`\r\n\t\t- `endpoint_ids`\r\n\r\nThis initial implementation does not include the LangGraph/output\r\nchunking upgrades seen in Attack Discovery due to time constraints.\r\nWe'll look to make this upgrade in a future PR.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)","sha":"efc0568e014105637332533e37491f074ec8fe2b"}},{"branch":"8.18","label":"v8.18.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
…ic#198676) ### Summary Adds the new Defend Insights Elastic Assistant tool. This assistant tool provides Elastic Defend configuration insights. For this initial PR, only incompatible antivirus detection is supported. Telemetry is collected for success and error events. For incompatible antivirus detection, Defend Insights will review the last 200 file events for the given endpoint and output suspected antiviruses. Improvements such as customizable event count and date range will come in the future. This PR does not include any UI, that will come in a separate PR. 3 internal APIs for interacting with Defend Insights are provided here: - `POST /defend_insights` for creating a new Defend Insight - `GET /defend_insights/{id}` for getting a Defend Insight - `GET /defend_insights` for getting multiple Defend Insights - available optional query params: - `size` - default 10 - `ids` - `connector_id` - `type` - `incompatible_antivirus` - `status` - `running`, `completed`, `failed`, `canceled` - `endpoint_ids` This initial implementation does not include the LangGraph/output chunking upgrades seen in Attack Discovery due to time constraints. We'll look to make this upgrade in a future PR. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
…ic#198676) ### Summary Adds the new Defend Insights Elastic Assistant tool. This assistant tool provides Elastic Defend configuration insights. For this initial PR, only incompatible antivirus detection is supported. Telemetry is collected for success and error events. For incompatible antivirus detection, Defend Insights will review the last 200 file events for the given endpoint and output suspected antiviruses. Improvements such as customizable event count and date range will come in the future. This PR does not include any UI, that will come in a separate PR. 3 internal APIs for interacting with Defend Insights are provided here: - `POST /defend_insights` for creating a new Defend Insight - `GET /defend_insights/{id}` for getting a Defend Insight - `GET /defend_insights` for getting multiple Defend Insights - available optional query params: - `size` - default 10 - `ids` - `connector_id` - `type` - `incompatible_antivirus` - `status` - `running`, `completed`, `failed`, `canceled` - `endpoint_ids` This initial implementation does not include the LangGraph/output chunking upgrades seen in Attack Discovery due to time constraints. We'll look to make this upgrade in a future PR. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
Summary
Adds the new Defend Insights Elastic Assistant tool. This assistant tool provides Elastic Defend configuration insights. For this initial PR, only incompatible antivirus detection is supported. Telemetry is collected for success and error events.
For incompatible antivirus detection, Defend Insights will review the last 200 file events for the given endpoint and output suspected antiviruses. Improvements such as customizable event count and date range will come in the future.
This PR does not include any UI, that will come in a separate PR. 3 internal APIs for interacting with Defend Insights are provided here:
POST /defend_insights
for creating a new Defend InsightGET /defend_insights/{id}
for getting a Defend InsightGET /defend_insights
for getting multiple Defend Insightssize
- default 10ids
connector_id
type
-incompatible_antivirus
status
-running
,completed
,failed
,canceled
endpoint_ids
This initial implementation does not include the LangGraph/output chunking upgrades seen in Attack Discovery due to time constraints. We'll look to make this upgrade in a future PR.
Checklist
For maintainers