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

Fix for ELB policy attribute string values. #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for ELB policy attribute string values. #50

wants to merge 1 commit into from

Conversation

PyScott
Copy link

@PyScott PyScott commented Oct 4, 2017

This fixes the false positives reported by the ELB Policy watcher. Boto3 returns these policy attribute values as strings, and these values are generally string "true" or string "false". In Python, bool("false") is actually True. This is the same code that SecurityMonkey used in versions 8.0 and before.

https://github.com/Netflix/security_monkey/blob/v0.8.0/security_monkey/watchers/elb.py#L87
http://boto3.readthedocs.io/en/latest/reference/services/elb.html#ElasticLoadBalancing.Client.describe_load_balancer_policies

@mikegrima
Copy link
Contributor

@MonkeySecurity is this good to go, or has the ELB policy auditor fixed this?

Generally speaking, we would like CloudAux to preserve (there are exceptions) the data that boto would return.

@mikegrima
Copy link
Contributor

@PyScott I haven't reviewed this in a while. Can you verify if the latest version of Security Monkey addresses this?

@PyScott
Copy link
Author

PyScott commented Jun 1, 2018

@mikegrima Apologies for the delay. I've checked out the latest version of SecurityMonkey/cloudaux and the issue remains. My test load balancer with SSLv3 disabled is reporting as having it enabled in SM.

Instead of my original approach, what do you think about replacing L31-L36 to look like this:

ret['protocols']['sslv2'] = attributes.get('Protocol-SSLv2') == 'true'

It looks like this comparison is already handled similarly on L51.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants