Netflix / security_monkey

Security Monkey monitors AWS, GCP, OpenStack, and GitHub orgs for assets and their changes over time.
Apache License 2.0
4.36k stars 800 forks source link

Increased ARN checking for check_ignore_list? #663

Open blakestoddard opened 7 years ago

blakestoddard commented 7 years ago

Hi!

Is there a particular reason that ARNs are ran through check_ignore_list on very limited basis when the watchers run? It seems like the ARN checks are limited to very few resources when they could be applied to a lot of other resources (such as ec2instances), which would make the Ignore List infinitely more useful as I could ignore stuff based on account.

If there is a reason, do you have any suggestions for other methods that I could try so that I could ignore particular resources from certain accounts that don't currently support having their ARN checked via check_ignore_list?

FWIW, I'm more than happy to do a PR with additional support for it if it's deemed a useful change.

scriptsrc commented 7 years ago

Hey @blakestoddard,

Each watcher decides what to send to check_ignore_list. Most send the item name. Managed Policies send both the name and the arn: https://github.com/Netflix/security_monkey/blob/develop/security_monkey/watchers/iam/managed_policy.py#L65

Are you requesting that the other watchers send both name and arn, instead of just name?

blakestoddard commented 7 years ago

Are you requesting that the other watchers send both name and arn, instead of just name?

Yep, that's exactly what I'm asking for. It seems like there are quite a number of other watchers that could send ARNs through, just like managed policies do.

scriptsrc commented 7 years ago

Maybe modify check_ignore_list to take a list instead of a str:

    def check_ignore_list(self, names=None):
        """
        See if the given item has a name flagging it to be ignored by security_monkey.
        """
        if not names:
            return False
        if type(names) is not list:
            names = list(names)
        for name in names:
            for result in self.ignore_list:
                # Empty prefix comes back as None instead of an empty string ...
                prefix = result.prefix or ""
                if name.lower().startswith(prefix.lower()):
                    app.logger.warn("Ignoring {}/{} because of IGNORELIST prefix {}".format(self.index, name, result.prefix))
                    return True

        return False

And then in the watchers we can collapse this:

                if self.check_ignore_list(policy.policy_name):
                    continue

                if self.check_ignore_list(policy.arn):
                    continue

Into this:

                if self.check_ignore_list([policy.arn, policy.policy_name]):
                    continue
scriptsrc commented 7 years ago

I wonder if we should also consider replacing prefixes with regular expressions.... Or allowing the user to choose which each one should be...

blakestoddard commented 7 years ago

The pass-in-an-array-and-iterate-over-it approach seems like a good path to take here.

As for adding regex support, it could be nice? I personally don't have a use for it and would much rather see per-account Ignore Lists (or the ability to specify that a particular ignore case only applies to one account and not universally), but having ARN ignore support in more places would take care of that use case.

To give an example of what I'm battling, we have our SecurityMonkey install monitoring our production and staging AWS accounts. Each morning, our staging databases are cycled based on a backup from the day before. Because there are instances going away and new ones being created, the email mass is strong and quite annoying. The current Ignore List implementation (sans ARN support) kind of screws us for monitoring production RDS instances since we give production and staging instances the same names for clarity and similarity, but have to ignore all instances with a given name to keep the email volume down. Sure, we could change the instance names, but I'd rather not. :)

phillycheeze commented 6 years ago

I've patched the method above to work with regular expressions in my project. I'll look into ignoring items per-account and figuring out a way to feed in more information from each watcher to make it more dynamic. If I can figure out a solution in the coming weeks, I'll submit a PR.

I'm not sure how user-involved this would be, but I imagine you'd have an initial input for the specific account it applies to or an "all" option. The prefix is re-labeled as "regex". And a third text-input for the key it applies to in the JSON (if blank, just use the technology default of name or arn).