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

[Pack: Recorded Future Intelligence][v1.4.0] Recorded Future playbook alerts v1 0 #25103

Conversation

recordedfuture-simonhornestedt
Copy link
Contributor

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: N/A

Description

Introduce new Recorded Future feature Playbook alerts as a separate integration into the Recorded Future content pack, some minor settings update to the Recorded Future v2 Integration.

Screenshots

Paste here any images that will help the reviewer

Minimum version of Cortex XSOAR

  • 6.0.0
  • 6.1.0
  • 6.2.0
  • 6.5.0

Does it break backward compatibility?

  • Yes
    • Further details:
  • No

Must have

  • Tests
  • Documentation

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2023

CLA assistant check
All committers have signed the CLA.

@content-bot content-bot added the Contribution Thank you! Contributions are always welcome! label Mar 6, 2023
@content-bot content-bot changed the base branch from master to contrib/recordedfuture-simonhornestedt_recordedfuture-playbook-alerts-v1_0 March 6, 2023 16:43
@content-bot content-bot requested a review from yucohen March 6, 2023 16:43
@content-bot
Copy link
Collaborator

Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @yucohen will know he can start review the proposed changes.

@edik24
Copy link
Contributor

edik24 commented Mar 6, 2023

Hi @recordedfuture-simonhornestedt can you please fill out the contribution Registration form and sign the CLA?

@recordedfuture-simonhornestedt recordedfuture-simonhornestedt changed the title Recordedfuture playbook alerts v1 0 [Pack: Recorded Future Intelligence][v1.4.0] Recorded Future playbook alerts v1 0 Mar 7, 2023
@content-bot content-bot added Contribution Form Filled Whether contribution form filled or not. Partner labels Mar 7, 2023
@edik24 edik24 self-assigned this Mar 7, 2023
@recordedfuture-simonhornestedt recordedfuture-simonhornestedt marked this pull request as ready for review March 7, 2023 14:28
@recordedfuture-simonhornestedt recordedfuture-simonhornestedt force-pushed the recordedfuture-playbook-alerts-v1_0 branch from 70c4328 to aaeb286 Compare March 7, 2023 15:10
@yucohen yucohen requested a review from efelmandar March 8, 2023 05:52
@recordedfuture-simonhornestedt recordedfuture-simonhornestedt force-pushed the recordedfuture-playbook-alerts-v1_0 branch from 32ae319 to dc9a16a Compare March 8, 2023 08:39
@recordedfuture-simonhornestedt recordedfuture-simonhornestedt force-pushed the recordedfuture-playbook-alerts-v1_0 branch from b12f435 to d956159 Compare March 9, 2023 14:10
@efelmandar
Copy link
Contributor

Hi @recordedfuture-simonhornestedt and thank you for your contribution, I reviewed your changes and found some points I want to discuss please see them below:

  1. The new incident types you added do not use auto-extract, which is not recommended in most use cases. I suggest configuring auto-extract to all or a list to some incident fields to leverage auto-extract and auto-enrichment in playbooks.

  2. In the Recorded Future Domain Abuse playbook - before the Create issue in JIRA (task no. 1) there is no check that JIRA integration is configured and available.

  3. In the Recorded Future Playbook Alert Details playbook - Why not use the Mapping tab in the task to map all the output fields to the corresponding incident fields to show it better in the layout?

  4. In the Recorded Future Vulnerability playbook, as before tasks Create issue in JIRA and Enrich CVE as Note (tasks no. 1 & no. 16) there is no check that the integration is enabled and configured.

  5. The two playbooks, Recorded Future Vulnerability and Recorded Future Domain Abuse are very similar and only differ by a couple of tasks which creates a little bit of duplication and probably can be merged into one playbook.
    As this might not fit every use case please consider this change or let me know if you think otherwise.

Overall the changes look good, and once these points are addressed we can move forward

@recordedfuture-simonhornestedt
Copy link
Contributor Author

recordedfuture-simonhornestedt commented Mar 13, 2023

Hi @efelmandar thanks for your feedback, I reached out to the person who created our playbooks/incident types from our Professional services team for some thoughts on your bullet points, this is his thoughts and reasoning to the choices:

  1. we never use auto extract for our incident types because we always have a playbook for it (auto extract and enrichment doesn’t make sense for our alerts)
  2. we don’t necessarily need to check for the Jira integration as this is a playbook template. we could remove it and just have a manual step to create an incident in an incident management platform or keep as 1 example and just mark it skip on error
  3. our outputs don’t easily map to fields and with our output being dynamic with no standard it’s virtually imposing
  4. Jira issue is already started in 2., we don’t need to check for enrichment because the customer will have our integration configured (once again we could make this a skip if error task)
  5. we can’t merge them, doesn’t make sense … they are similar but once again the customer is supposed to copy and add steps to them as they are template playbooks

Any additional thoughts or suggestions with having this information?

@efelmandar
Copy link
Contributor

@recordedfuture-simonhornestedt I appreciate your response regarding my points and can understand why it is reasonable to not make the changes in this case. Thanks again your contributions are valued and appreciated.

…laybook-alerts-v1_0' into recordedfuture-playbook-alerts-v1_0
Copy link
Contributor

@yucohen yucohen left a comment

Choose a reason for hiding this comment

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

@recordedfuture-simonhornestedt Thanks for the contribution! Great work!
Please see my comments :)
Also, we can delete the pipfile and pipfile.lock files

for screenshot_data in incident_json["panel_evidence_summary"][
"screenshots"
]:
file_name = f'{screenshot_data.get("image_id").replace("img:","")}.png'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file_name = f'{screenshot_data.get("image_id").replace("img:","")}.png'
file_name = f'{screenshot_data.get("image_id", "").replace("img:","")}.png'

In the case where no screenshots, the replace method won't fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"screenshots"
]:
file_name = f'{screenshot_data.get("image_id").replace("img:","")}.png'
file_data = screenshot_data.get("base64")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file_data = screenshot_data.get("base64")
file_data = screenshot_data.get("base64", "")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- display: API Token
name: token
defaultvalue: ""
type: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

lets change it to type 9. see this article

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@recordedfuture-simonhornestedt
Copy link
Contributor Author

Removed pipfile and updated the dockerimage tag so that should be the last of the changes requested

@yucohen yucohen added docs-approved ForceMerge Forcing the merge of the PR despite the build status labels Mar 19, 2023
@yucohen yucohen removed request for esharf and ilaner March 19, 2023 11:35
@yucohen yucohen merged commit 22a590c into demisto:contrib/recordedfuture-simonhornestedt_recordedfuture-playbook-alerts-v1_0 Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Form Filled Whether contribution form filled or not. Contribution Thank you! Contributions are always welcome! docs-approved ForceMerge Forcing the merge of the PR despite the build status Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants