Skip to content
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

[Defend Workflows] Defend Insights - Check existing Trusted Apps before creating an insight #207378

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

szwarckonrad
Copy link
Contributor

@szwarckonrad szwarckonrad commented Jan 21, 2025

This PR ensures that prepared insights are checked for an existing path and signer already added to the trusted app. If such insights are found, the execution is stopped. This resolves the issue of displaying insights to the user that have already been remediated.

@szwarckonrad szwarckonrad changed the title wip [Defend Workflows] Defend Insights - Check existing Trusted Apps before creating an insight Jan 22, 2025
@szwarckonrad szwarckonrad self-assigned this Jan 22, 2025
@szwarckonrad szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.18.0 Team:Defend Workflows “EDR Workflows” sub-team of Security Solution and removed backport:skip This commit does not require backporting labels Jan 22, 2025
@szwarckonrad szwarckonrad marked this pull request as ready for review January 22, 2025 18:14
@szwarckonrad szwarckonrad requested a review from a team as a code owner January 22, 2025 18:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@@ -127,7 +133,7 @@ export const WorkflowInsightsResults = ({
<EuiText size={'s'} color={'subdued'}>
{insight.message}
</EuiText>
<EuiText size={'xs'} color={'subdued'}>
<EuiText size={'xs'} color={'subdued'} css={'word-break: break-word'}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

longs paths were stretching out the container

@@ -31,6 +31,7 @@ export const useFetchInsights = ({ endpointId, onSuccess }: UseFetchInsightsConf
query: {
actionTypes: JSON.stringify([ActionType.Refreshed]),
targetIds: JSON.stringify([endpointId]),
size: 100,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it the es' default 10 is being used, we expect more results than that with certain AVs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why limit to 100?

@@ -57,6 +57,7 @@ export async function buildIncompatibleAntivirusWorkflowInsights(
const codeSignaturesHits = (
await esClient.search<FileEventDoc>({
index: FILE_EVENTS_INDEX_PATTERN,
size: eventIds.length,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also defaulting to 10 causing instances of paths with no matched signers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are eventIds always defined?

(entry.field === 'process.Ext.code_signature' && typeof entry.value === 'string')
) {
const sanitizedValue = (entry.value as string)
.replace(/[)(<>}{":\\]/gm, '\\$&')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙆‍♂️

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 21.3MB 21.3MB +335.0B

History

cc @szwarckonrad

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 left some question, but nothing crucial :) thanks!

@@ -31,6 +31,7 @@ export const useFetchInsights = ({ endpointId, onSuccess }: UseFetchInsightsConf
query: {
actionTypes: JSON.stringify([ActionType.Refreshed]),
targetIds: JSON.stringify([endpointId]),
size: 100,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why limit to 100?

@@ -57,6 +57,7 @@ export async function buildIncompatibleAntivirusWorkflowInsights(
const codeSignaturesHits = (
await esClient.search<FileEventDoc>({
index: FILE_EVENTS_INDEX_PATTERN,
size: eventIds.length,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are eventIds always defined?

(entry.field === 'process.Ext.code_signature' && typeof entry.value === 'string')
) {
const sanitizedValue = (entry.value as string)
.replace(/[)(<>}{":\\]/gm, '\\$&')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping you could add a comment explaining the regex a bit? :)

});

if (remediationExists) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be useful to add a logger here or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants