aws-cloudformation-samples icon indicating copy to clipboard operation
aws-cloudformation-samples copied to clipboard

Boolean values converted to strings not handled properly

Open anderseknert opened this issue 2 years ago • 2 comments

While developing a new hook type to allow Open Policy Agent (OPA) to make decisions on infrastructure deployments via CloudFormation hooks, one of the challenges we faced was how boolean values in the YAML/JSON templates are converted to strings before they're presented to the hook. This behavior is surprising, and leads to seemingly correct policies that should deny a deployment instead allowing resource creation or modifications, as checking for "truthiness" will always return true (as both strings "true" and "false" are truthy).

While comparing our policies to the ones in this repository, I was surprised to find that these sample policies don't seem to take this into account.

While the main issue here is that this conversion is being done in the first place (and if there's an issue to track that, I'd love to know where I can find it!), the examples should at least be updated to take this into account, as they currently will allow resources to be created that aren't compliant with the hook policy deployed. To provide an example, the Python sample to block public access to S3 buckets will happily accept a template like the one provided below:

Resources:
  S3Bucket:
    Type: 'AWS::S3::Bucket'
    Properties:
      BucketName: !Sub 'mybucket-${AWS::Region}-${AWS::AccountId}'
      PublicAccessBlockConfiguration:
        BlockPublicAcls: false
        BlockPublicPolicy: false
        IgnorePublicAcls: false
        RestrictPublicBuckets: false

The corresponding handler code that checks for these values will pass as long as the values are set, even when set to false, as the converted "false" is truthy:

https://github.com/aws-cloudformation/aws-cloudformation-samples/blob/a3f2e64e66a3e33aa910cab9b34dcb4598bf5f92/hooks/python-hooks/s3-block-public-access/src/awssamples_s3blockpublicaccess_hook/handlers.py#L58-L64

I'm sure there are more examples like this, and while they can be fixed, it would be preferrable if the core issue—i.e. the conversion taking place—was addressed.

anderseknert avatar Apr 04 '22 11:04 anderseknert

Thank you, @anderseknert ! I took a look at the example above, and at the sample code. Whilst I included, in #77 , an update for that sample hook (together with other updates), I will bring your conversion-related feedback to the team. Thanks again!

mrinaudo-aws avatar Mar 15 '23 19:03 mrinaudo-aws

Much appreciated. Thanks @mrinaudo-aws 👍

anderseknert avatar Mar 15 '23 19:03 anderseknert