fix None type scenario in get_actions_for_service

[reetasingh]

What does this PR do?

Address NoneType error raised as part of issue #239

What gif best describes this PR or how it makes you feel?

Completion checklist

• Additions and changes have unit tests
• Unit tests, Pylint, security testing, and Integration tests are passing.. GitHub actions does this automatically
• The pull request has been appropriately labeled using the provided PR labels

[reetasingh]

@kmcquade please review , this is my first PR for this repo. Hoping to make more contributions soon

[reetasingh]

Python 3.9.0 (tags/v3.9.0:9cf6752, Oct  5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>>
>>> from policy_sentry.querying.actions import get_actions_for_service
>>>
>>>
>>> def example():
...     actions = get_actions_for_service('abc')  # Then you can leverage any method that requires access to the database.
...     for action in actions:
...         print(action)
...
>>> if __name__ == '__main__':
...     example()
...
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in example
  File "C:\Users\rakes\git\policy_sentry\policy_sentry\querying\actions.py", line 26, in get_actions_for_service
    service_prefix_data = get_service_prefix_data(service_prefix)
  File "C:\Users\rakes\git\policy_sentry\policy_sentry\shared\iam_data.py", line 27, in get_service_prefix_data
    raise Exception(f"Service prefix {service_prefix} not found.")
Exception: Service prefix abc not found.
>>>

[kmcquade]

Hey @reetasingh! Thanks for making a PR.

I think that you might have made the fix in the wrong file.

  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/command/query.py", line 84, in action_table
    query_action_table(name, service, access_level, condition, wildcard_only, fmt)
  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/command/query.py", line 163, in query_action_table
    output = get_actions_for_service(service)
  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/querying/actions.py", line 28, in get_actions_for_service
    for item in service_prefix_data["privileges"]:
TypeError: 'NoneType' object is not subscriptable

The above indicates two different options to investigate:

1. policy_sentry.shared.iam_data.get_service_prefix_data
2. policy_sentry.querying.actions.get_actions_for_service

get_service_prefix_data

The section you chose - policy_sentry.shared.iam_data.get_service_prefix_data - is working as intended, actually. Some background:

I use Policy Sentry as a library, as do several others that I'm aware of - most notably in Cloudsplaining. Cloudsplaining pulls live AWS IAM Policies for analysis. Sometimes these IAM Policies reference AWS Services that have not yet been included in the IAM Definition that we store at policy_sentry/shared/data/iam-definition.json. Our IAM definition gets updated once a month.

But for Cloudsplaining, we need to make sure that if a service does not exist, that Policy Sentry just logs the issue and does not break the scan by throwing an exception. I've encountered this issue a few times - where Policy Sentry gets a new version at the beginning of the month, and AWS releases 4 new service prefixes within that same month, before we have time to release a new version of Policy Sentry. That would break the scan.

So, given the above - get_service_prefix_data is working as intended.

get_actions_for_service

I think this is the one that actually needs fixing. The function below looks like it will still try to add an object of type None to the list of results. I think that is what causes the issue I mentioned.

policy_sentry/policy_sentry/querying/actions.py

Lines 26 to 30 in a8502ce

	service_prefix_data = get_service_prefix_data(service_prefix)
	results = []
	for item in service_prefix_data["privileges"]:
	results.append(f"{service_prefix}:{item}")
	return results

I think this means you would have to change your PR to be focused on get_actions_for_service instead of get_service_prefix_data. Might want to just start with a fresh PR though and close this one - that should be cleaner.

[reetasingh]

service_prefix_data["privileges"]

Thanks for the background @kmcquade
Now i understand why raising an exception is not feasible here.

actually, service_prefix_data["privileges"] is the one which is failing because service_prefix_data is None and we are trying to find privileges in a None type

do you want me to add a check for not None before calling service_prefix_data["privileges"]

also, given that service_prefix_data["privileges"] is being called in multiple places, that check should be everywhere where we are using service_prefix_data["privileges"]

[kmcquade]

Yeah that works! Thank you!

[reetasingh]

@kmcquade addressed the review comments

[kmcquade]

Good stuff. Thanks!