StyraInc / opa-aws-cloudformation-hook

AWS Cloudformation Hook for OPA-powered infrastructure policy enforcement
Apache License 2.0
35 stars 5 forks source link

handlers.py: Exception caught sequence item 0: expected str instance, dict found #39

Open pauly4it opened 2 years ago

pauly4it commented 2 years ago

After setting up the CloudFormation hook and creating a stack with an S3 bucket with public access, the following was logged to CloudWatch for the hook:

Exception caught sequence item 0: expected str instance, dict found
Traceback (most recent call last):
  File "/var/task/cloudformation_cli_python_lib/hook.py", line 273, in __call__
    raise error
  File "/var/task/cloudformation_cli_python_lib/hook.py", line 262, in __call__
    caller_sess, request, invocation_point, callback, type_configuration
  File "/var/task/cloudformation_cli_python_lib/hook.py", line 100, in _invoke_handler
    return handler(session, request, callback_context, type_configuration)
  File "/var/task/styra_opa_hook/handlers.py", line 127, in pre_handler
    return opa_query(request, session, type_configuration, action)
  File "/var/task/styra_opa_hook/handlers.py", line 97, in opa_query
    message = " | ".join(body["violations"])
TypeError: sequence item 0: expected str instance, dict found

For reference, the OPA agent returned the following:

{"allow":false,"violations":[{"allowed":false,"message":"public access not blocked for bucket testing"}]}
anderseknert commented 2 years ago

Yeah, violations is currently assumed to be a set of strings only. While we could extend this to support more response formats, I'm not sure what the added value in this case would be? 🙂 That allowed is false is already implied by the presence of violations, so also adding that to each violation entry does not seems a little redundant.

One thing that might be useful though is if we had a case for adding other types of metadata to a violation, like severity or something like that. But whatever we'd go with, we'd need to document properly what works and what doesn't.

anderseknert commented 2 years ago

Regardless of that — we could definitely print something more informational than a stacktrace here. Just something like the below would be an improvement:

Unrecognized format in OPA response.
Expected: {"allow": <boolean>, "violations": <array<string>>}
Got: {"allow": <boolean>, "violations": <array<object>>}