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

Adding module to manage both trusted and threat IP sets in AWS Guardduty #40851

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@slapula
Contributor

slapula commented May 29, 2018

SUMMARY

This is a module to manage IP lists in AWS GuardDuty. It handles both IP sets (Amazon's name not mine, here I'm referring to these as trusted IP sets) and threat intel sets. The integration tests utilize the guardduty_detector module which I submitted in #40837.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

guardduty_ip_list

ANSIBLE VERSION
ansible 2.4.1.0
  config file = None
  configured module search path = [u'/Users/asmith/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/asmith/.pyenv/versions/2.7.13/lib/python2.7/site-packages/ansible
  executable location = /Users/asmith/.pyenv/versions/2.7.13/bin/ansible
  python version = 2.7.13 (default, Sep  8 2017, 15:16:38) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)]
@mattclay

This comment has been minimized.

Member

mattclay commented Jun 8, 2018

CI failure in integration tests:

2018-05-29 20:45:56 ERROR! no action detected in task. This often indicates a misspelled module name, or incorrect module path.
2018-05-29 20:45:56 
2018-05-29 20:45:56 The error appears to have been in '/root/ansible/test/integration/targets/guardduty_ip_list/tasks/main.yml': line 143, column 5, but may
2018-05-29 20:45:56 be elsewhere in the file depending on the exact syntax problem.
2018-05-29 20:45:56 
2018-05-29 20:45:56 The offending line appears to be:
2018-05-29 20:45:56 
2018-05-29 20:45:56   always:
2018-05-29 20:45:56   - name: destroy GuardDuty detector
2018-05-29 20:45:56     ^ here

@mattclay mattclay added the ci_verified label Jun 8, 2018

@ansibot ansibot added the stale_ci label Jun 8, 2018

@slapula

This comment has been minimized.

Contributor

slapula commented Jun 8, 2018

This error is expected since these tests rely on a GuardDuty detector being enabled on the AWS account. I have submitted the missing module here #40837.

@s-hertel s-hertel self-requested a review Jun 12, 2018

@s-hertel

Thanks for the PR @slapula! I'll work on updating CI for your pull requests this week.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
def ip_list_exists(client, module, result):
try:
if module.params.get('list_type') == 'trusted':
sets = client.list_ip_sets(

This comment has been minimized.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
result['trusted_set_id'] = i
return True
if module.params.get('list_type') == 'threat':
sets = client.list_threat_intel_sets(

This comment has been minimized.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
if ti_set['Name'] == module.params.get('name'):
result['threat_set_id'] = i
return True
except ClientError:

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

Rather than mask all ClientErrors, this should handle the specific message(s) that occur/if they occur when the set doesn't exist yet with is_boto3_error_code from ansible.module_utils.aws.core and fail for exceptions that actually indicate a problem:

except is_boto3_error_code('TheListIsntFoundError'):
    return False
except (BotoCoreError, ClientError) as e:  # pylint: disable=duplicate-except
    module.fail_json_aws(e, msg="Unable to list {0} sets".format(module.params.get('list_type')))
lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
return False
def create_ip_list(client, module, params, result):

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

It looks like the ID is the only important thing to return here. Changed will always be True for this code path, so you can set it where this gets called.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
IpSetId=i
)
if ti_set['Name'] == module.params.get('name'):
result['trusted_set_id'] = i

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

I feel like passing around a reference to a dict complicates things unnecessarily and makes the code less obvious. That isn't a big deal when the module is so small, but if more options are introduced and the module grows it can become more of a pain. We recently removed this pattern from ec2_group and I'd prefer not to add it in new places. It looks like the meaningful information this function should provide is either to return the set ID or None.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
return result
def update_ip_list(client, module, params, result):

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

It looks like this doesn't need result as a parameter and could just take a set_id instead. result['changed'] = True could be set where this is called since that's always the case for this code path. It doesn't look like this function needs to return anything.

lib/ansible/modules/cloud/amazon/guardduty_ip_list.py Outdated
return result
def delete_ip_list(client, module, result):

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This could use the same things as my comment about update_ip_list().

@slapula

This comment has been minimized.

Contributor

slapula commented Jul 2, 2018

Thanks @s-hertel ! Bear with me, I am aware that there are a number of improvements I can make across the board with my open PRs. There are things I learned while working on my later PRs that I should and will include in my earlier PRs. If you see something say something but keep in mind that I am going to be circling back on all of them to make sure they are as merge ready as they can be.

@slapula slapula force-pushed the slapula:aws_guardduty_ip_list_module branch to 882c304 Jul 6, 2018

@slapula

This comment has been minimized.

Contributor

slapula commented Jul 6, 2018

@s-hertel I just updated this module with your recommendations. I also renamed it to follow the standard.

@ansibot ansibot added the stale_ci label Jul 16, 2018

@ryansb

Please add in retries (aws_retry should make this pretty easy).

module: aws_guardduty_ip_list
short_description: Manage Trusted/Threat IP list for AWS GuardDuty.
description:
- Manage a Trusted/Threat IP list assigned to a AWS GuardDuty detector.

This comment has been minimized.

@ryansb

ryansb Jul 18, 2018

Contributor

*an AWS ...

options:
name:
description:
- A user-friendly name that is displayed in all finding generated by activity that involves

This comment has been minimized.

@ryansb

ryansb Jul 18, 2018

Contributor

*findings

'enabled': dict(type='bool', default=True),
'detector_id': dict(type='str', required=True),
'format': dict(type='str', choices=['TXT', 'STIX', 'OTX_CSV', 'ALIEN_VAULT', 'PROOF_POINT', 'FIRE_EYE']),
'location': dict(type='str', required=True),

This comment has been minimized.

@ryansb

ryansb Jul 18, 2018

Contributor

Is there any case where this wouldn't be an S3 URI?

)
for i in sets:
for s in i['ThreatIntelSetIds']:
ti_set = client.get_threat_intel_set(

This comment has been minimized.

@ryansb

ryansb Jul 18, 2018

Contributor

I think these looped calls need to have retries enabled. It seems likely that with all these in close succession there would be issues with the AWS API rate limits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment