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

Return matched.atomic value from indicator #132509

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

Conversation

nkhristinin
Copy link
Contributor

@nkhristinin nkhristinin commented May 19, 2022

Summary

Switched back to put into matched.atomic value from threat indicator, not source event.

How to reproduce the bug:

PUT /threat-index
{
  "mappings": {
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "ip_range": {
        "type": "ip_range"
      }
    }
  }
}

PUT /source-index
{
  "mappings": {
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "ip": {
        "type": "ip"
      }
    }
  }
}

POST threat-index/_doc
{
  "@timestamp": ${timestamp},
  "ip_range": "127.0.0.1/30"
}

POST source-index/_doc 
{
  "@timestamp": ${timestamp},
  "ip": "127.0.0.2"
}

Create a rule like

Screenshot 2022-05-19 at 16 36 18

Then you should have alert with some threat enrichments which has matched.atomic - 127.0.0.2 which is a wrong:

Screenshot 2022-05-19 at 16 35 28

If you switch to this PR branch and send one more alert like

POST source-index/_doc 
{
  "@timestamp": ${timestamp},
  "ip": "127.0.0.3"
}

You should see matched.atomic value as 127.0.0.1/30

Checklist

Delete any items that are not applicable to this PR.

@nkhristinin nkhristinin marked this pull request as ready for review May 19, 2022 14:08
@nkhristinin nkhristinin requested a review from a team as a code owner May 19, 2022 14:08
@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 1, 2022

💔 Build Failed

Failed CI Steps

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

This one got lost in the shuffle, I think... If we can fix the file conflicts I think this will be good to go! Just let me know and I can approve.

@@ -113,11 +117,6 @@ export const createThreatSignals = async ({

logger.debug(buildRuleMessage(`Total indicator items: ${threatListCount}`));

const threatListConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we now have two different values for threatListConfig, can we give them distinguishing names? It's not obvious from this level how the results of getThreatListConfig() differ from the manually mapped version.

@@ -169,3 +170,27 @@ export const buildExecutionIntervalValidator: (interval: string) => () => void =
}
};
};

export const getThreatListConfig = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

A few unit tests showing how this works would be helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment release_note:fix Team:Security Solution Platform Security Solution Platform Team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants