Jwilkes new aws pack

[capanw]

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: link to the issue

Description

We spoke with @michalgold, @altmannyarden about a new pack for AWS Enrichment and Remediation playbooks. This needs to be a new pack, because it includes multiple integration commands from multiple AWS packs including EC2 and IAM. In the future, we will add support for other AWS packs. This is my first submission. Please, let me know if you have any questions.

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]

All committers have signed the CLA.

[capanw]

Hello,

we are seeing the following error

[ERROR]: Packs/AWS-Enrichment-Remediation/README.md: [RM108] - Error in readme image: got HTTP response code 404, reason = Not Found The following image link seems to be broken, please repair it: ![AWS - Security Group Remediation](https://raw.githubusercontent.com/demisto/content/master/Packs/AWS-Enrichment-Remediation/doc_files/AWS_-_Security_Group_Remediation.png)

We think this is what the final file path should be. Any suggestions?

Thanks

[idovandijk]

Thanks for your contribution. I reviewed the playbooks. Listing my comments with numbers below so you can reference them if you have any question:

AWS - Enrichment

1. The IP is mandatory for this playbook, so let's change the input to mandatory:
   
   Additionally it would be good to verify the input exists before continuing with the playbook. You can do it in the same condition that checks if the integration is enabled.
2. Question - will an Owner ID always be returned? If not sure please also add a validation for it before we query using the Owner ID:
   
3. It's best practice to add the sub-keys of the outputs to the playbook outputs as well:
   
   For example AWS.IAM.Users will work just fine, but tasks that want to grab the outputs of the playbook and use them will not know that there's a AWS.IAM.Users.UserName. So let's define all of them :)

AWS - Security Group Remediation

4. The VPC ID is also mandatory here, please change the input to mandatory. Also add a verification for the input.
5. Are you sure that's a good filter? Name=group-name,Values=Remediation-Security-Group. What if there is already a security group with a different name?
6. Please add the security group name (AKA Remediation-Security-Group) as a playbook input, and use the input in the tasks.

General

7. Please use the Demisto SDK to calculate the pack dependencies for this pack. Since this pack has a mandatory dependency on other AWS packs we need to make sure that the dependency is defined as mandatory in the pack_metadata.json file.

Finally - after making adjustments, please provide possible time slots for a short demo of 15-20 minutes. Just to see the playbooks running in an incident. Let me know if you have any issues or concerns!

[johnnywilkes]

@idovandijk , thanks for you review.
Do you have an answer to our question: #21729 (comment)

for Question 7: is there a demisto-sdk command for this?

[johnnywilkes]

I am going to wait for my coworker @capanw to be back from FTO Monday to start addressing these concerns. Thank you for the feedback @idovandijk

[idovandijk]

@idovandijk , thanks for you review. Do you have an answer to our question: #21729 (comment)

for Question 7: is there a demisto-sdk command for this?

Use a relative path (../doc_files/image.png)

[capanw]

@idovandijk , thanks for you review. Do you have an answer to our question: #21729 (comment)
for Question 7: is there a demisto-sdk command for this?

Use a relative path (../doc_files/image.png)

Hello, we tried to use the relative path and got the following error message:

- Issues with unique files in pack: AWS-Enrichment-Remediation [ERROR]: /Users/callu/github_ext/content-JW-Fork/Packs/AWS-Enrichment-Remediation/README.md: [RM108] - Error in readme image: Detected the following image relative path: ![AWS - Security Group Remediation](doc_files/AWS_-_Security_Group_Remediation.png). Relative paths are not supported in pack README files. See https://xsoar.pan.dev/docs/integrations/integration-docs#images for further info on how to add images to pack README files.

[capanw]

Hello @idovandijk,

For questions 1 and 4 we understand the importance of input validation. However, when a subplaybook input is mandatory, this means it will fail if the supplied input isn’t available. Therefore, it would seem you would have to do something like a SetIfEmpty to make sure this doesn’t happen. Instead it might be better to do a check in the parent that references the subplaybook. What do you think? Let me know if you would prefer to setup a quick meeting to discuss.

[capanw]

Thanks for your contribution. I reviewed the playbooks. Listing my comments with numbers below so you can reference them if you have any question:

AWS - Enrichment

1. The IP is mandatory for this playbook, so let's change the input to mandatory:
   
   Additionally it would be good to verify the input exists before continuing with the playbook. You can do it in the same condition that checks if the integration is enabled.
2. Question - will an Owner ID always be returned? If not sure please also add a validation for it before we query using the Owner ID:
   
3. It's best practice to add the sub-keys of the outputs to the playbook outputs as well:
   
   For example AWS.IAM.Users will work just fine, but tasks that want to grab the outputs of the playbook and use them will not know that there's a AWS.IAM.Users.UserName. So let's define all of them :)

AWS - Security Group Remediation

4. The VPC ID is also mandatory here, please change the input to mandatory. Also add a verification for the input.
5. Are you sure that's a good filter? Name=group-name,Values=Remediation-Security-Group. What if there is already a security group with a different name?
6. Please add the security group name (AKA Remediation-Security-Group) as a playbook input, and use the input in the tasks.

General

7. Please use the Demisto SDK to calculate the pack dependencies for this pack. Since this pack has a mandatory dependency on other AWS packs we need to make sure that the dependency is defined as mandatory in the pack_metadata.json file.

Finally - after making adjustments, please provide possible time slots for a short demo of 15-20 minutes. Just to see the playbooks running in an incident. Let me know if you have any issues or concerns!

Hello @idovandijk,

For question 3: We had a discussion on this and for the automation that we are using, it has about 40 of those sub-keys for the output and felt like its an overwhelming number of sub-keys. Hence, we have added just the one's that are using them. what do you think? Thank you

[capanw]

Thanks for your contribution. I reviewed the playbooks. Listing my comments with numbers below so you can reference them if you have any question:

AWS - Enrichment

1. The IP is mandatory for this playbook, so let's change the input to mandatory:
   
   Additionally it would be good to verify the input exists before continuing with the playbook. You can do it in the same condition that checks if the integration is enabled.
2. Question - will an Owner ID always be returned? If not sure please also add a validation for it before we query using the Owner ID:
   
3. It's best practice to add the sub-keys of the outputs to the playbook outputs as well:
   
   For example AWS.IAM.Users will work just fine, but tasks that want to grab the outputs of the playbook and use them will not know that there's a AWS.IAM.Users.UserName. So let's define all of them :)

AWS - Security Group Remediation

4. The VPC ID is also mandatory here, please change the input to mandatory. Also add a verification for the input.
5. Are you sure that's a good filter? Name=group-name,Values=Remediation-Security-Group. What if there is already a security group with a different name?
6. Please add the security group name (AKA Remediation-Security-Group) as a playbook input, and use the input in the tasks.

General

7. Please use the Demisto SDK to calculate the pack dependencies for this pack. Since this pack has a mandatory dependency on other AWS packs we need to make sure that the dependency is defined as mandatory in the pack_metadata.json file.

Finally - after making adjustments, please provide possible time slots for a short demo of 15-20 minutes. Just to see the playbooks running in an incident. Let me know if you have any issues or concerns!

Hello @idovandijk,

For question 3: We had a discussion on this and for the automation that we are using, it has about 40 of those sub-keys for the output and felt like its an overwhelming number of sub-keys. Hence, we have added just the one's that are using them. what do you think? Thank you

Hello, for question 2, there should always be an owner ID returned as every security group will have the owner id information. We have not come across a situation where there is no owner associated to a SG. Thanks

[idovandijk]

@capanw if there is an overwhelming amount of subkeys for the outputs then you can keep just the ones you believe may possibly be used outside of the playbook, that's OK.
Regarding the response for question 2 - got it :)
Let me know when done with the modifications, thanks.

[idovandijk]

@rshunim can you please help them with the pack README file error?

[capanw]

@capanw if there is an overwhelming amount of subkeys for the outputs then you can keep just the ones you believe may possibly be used outside of the playbook, that's OK. Regarding the response for question 2 - got it :) Let me know when done with the modifications, thanks.

Hello @idovandijk, thanks for your review. We are working on the suggested changes for Question 1 and 4.

[capanw]

Thanks for your contribution. I reviewed the playbooks. Listing my comments with numbers below so you can reference them if you have any question:

AWS - Enrichment

1. The IP is mandatory for this playbook, so let's change the input to mandatory:
   
   Additionally it would be good to verify the input exists before continuing with the playbook. You can do it in the same condition that checks if the integration is enabled.
2. Question - will an Owner ID always be returned? If not sure please also add a validation for it before we query using the Owner ID:
   
3. It's best practice to add the sub-keys of the outputs to the playbook outputs as well:
   
   For example AWS.IAM.Users will work just fine, but tasks that want to grab the outputs of the playbook and use them will not know that there's a AWS.IAM.Users.UserName. So let's define all of them :)

AWS - Security Group Remediation

4. The VPC ID is also mandatory here, please change the input to mandatory. Also add a verification for the input.
5. Are you sure that's a good filter? Name=group-name,Values=Remediation-Security-Group. What if there is already a security group with a different name?
6. Please add the security group name (AKA Remediation-Security-Group) as a playbook input, and use the input in the tasks.

General

7. Please use the Demisto SDK to calculate the pack dependencies for this pack. Since this pack has a mandatory dependency on other AWS packs we need to make sure that the dependency is defined as mandatory in the pack_metadata.json file.

Finally - after making adjustments, please provide possible time slots for a short demo of 15-20 minutes. Just to see the playbooks running in an incident. Let me know if you have any issues or concerns!

Hello @idovandijk,

For question 3: We had a discussion on this and for the automation that we are using, it has about 40 of those sub-keys for the output and felt like its an overwhelming number of sub-keys. Hence, we have added just the one's that are using them. what do you think? Thank you

Hello @idovandijk,

Thank you for all the suggestions. Here are the updates on the questions:

Question 1: We made changes to Mandatory field and updated our sub-Playbook to include the input checks.
Question 2: No changes for this as we always expect OwnerID.
Question 3: We have added just the ones we think are needed.
Question 4: We made changes to Mandatory field and updated our sub-Playbook to include the input checks.
Question 5: We made changes, so that the user has an option to over ride the default value of "Remediation-Security-Group"
Question 6: Added this as an input
Question 7: Updated our pack_metadata.json file to include dependencies

Please let me know if you have any further questions.

Thanks

[idovandijk]

Thank you for making the changes and recording the demo. The PR is ready to be merged. Have a nice week.

[rshunim]

Hi Chait,
Thank you very much for your contribution!

As I see, the image link seem to be broken, so try my suggetions