aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.63k stars 3.91k forks source link

wafv2: CfnWebACL Rules property has incorrect CloudFormation schema #6056

Closed thibaut-pro closed 4 years ago

thibaut-pro commented 4 years ago

link to reference doc page: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-wafv2.CfnWebACL.html

I tried instantiating a simple WAF ACL with the following code:

    const acl = new waf.CfnWebACL(this, 'ACL', {
      defaultAction: {
        allow: true,
      },
      scope: 'CLOUDFRONT',
      visibilityConfig: {
        cloudWatchMetricsEnabled: true,
        metricName: 'waf', // TODO add a stage suffix
        sampledRequestsEnabled: false,
      },
      rules: {
        rules: [
          { name: 'AWS-AWSManagedRulesAmazonIpReputationList' },
          { name: 'AWS-AWSManagedRulesCommonRuleSet' },
          { name: 'AWS-AWSManagedRulesKnownBadInputsRuleSet' },
          { name: 'AWS-AWSManagedRulesSQLiRuleSet' },
          { name: 'AWS-AWSManagedRulesLinuxRuleSet' },
        ],
      },
    });

The code compiles but cdk deploy fails with an ACL Internal Failure that doesn't provide any helpful context.

Can you update the document with a working starter example?


This is a 📕 documentation issue

hoegertn commented 4 years ago

You might want to look at: https://twitter.com/hoegertn/status/1222960907929706499

There are some spec errors in the CFN spec for WAFv2

Once they are resolved, CDK will work.

richardhboyd commented 4 years ago

I think this type of feature is better suited for the cdk-examples repository.

thibaut-pro commented 4 years ago

It would be nice to have a cdk-example for it. That said, a simple example in the documentation overview similar to https://docs.aws.amazon.com/cdk/api/latest/docs/aws-lambda-readme.html is also always helpful.

RachelleJanssen commented 4 years ago

First, try to create a simple ACL and make sure you use @aws-cdk/aws-wafv2 and not @aws-cdk/aws-waf (this took me half an hour to figure out). I was able to create an ACL without rules.

However, I just found out that ACLs with the CLOUDFRONT scope need to be created in the us-east-1 region. Before I was getting errors on the scope property. When you create REGIONAL scoped ACLs it's all fine, but CLOUDFRONT scope is global and somehow only works with us-east-1. This is quite a limitation because I can't roll-out the stack containing this ACL in any other region.

To see it work, go to your bin folder. There should be a file which contains the following. In the stack properties is a property called region. Set that to us-east-1

#!/usr/bin/env node
import "source-map-support/register";
import * as cdk from "@aws-cdk/core";
import { AwsCdkAclStack } from "../lib/aws-cdk-acl-stack";

const app = new cdk.App();
new AwsCdkAclStack(app, "MyStack", { stackName: "MyStack", env: { region: "us-east-1" } });

I really hope this gets resolved soon but it seems like an underlying issue with cloudformation, not CDK (unless CDK can somehow reroute CLOUDFRONT scoped ACLs to use the us-east-1 region). I'm hoping to roll-out and duplicate an entire solution using CDK soon.

RachelleJanssen commented 4 years ago

@hoegertn I'm not sure if it's related to spec errors. It seems that by removing the extra wrapped "Rules" property from the cdk.output I'm able to create the stack using the cdk.output file with the correct rule.

@thibaut-singlefile I reverse engineered the ACL configuration by creating one manually and downloading it has JSON (button on the top right of the ACL detail screen). I copy pasted one of the rules into the array and, like I said, edited the cdk.output file. Then it worked to recreate the exact ACL.

Scope set to CLOUDFRONT Still only works in us-east-1 though...

Edit: sorry, you're right. Looks like most of the code is generated based on the AWS CloudFormation Resource Specification.

thibaut-pro commented 4 years ago

@RachelleJanssen that's great news! Can you post your working configuration here?

RachelleJanssen commented 4 years ago

@thibaut-singlefile I see the managed rules you wanted to put on the ACL, so I created one manually and reverse-engineered the config to CDK (I assume you use typescript). Please keep in mind that CDK has a bug that outputs the Rules: { Rules: [] } wrapper, which caused your internal failure. I opened a bug report here

Like mentioned above, keep in mind that for the time being you need to fix the output manually.

You won't be able to successfully run cdk commands with this anymore, because the synthesized template.json will put Rules: { Rules: [] } back in whenever you run deploy, synth or diff

Go to cdk.output and edit the .template.json file. Remove the Rules { } wrapper around the Rules: [ ]. Then go to the cloudformation console, and create a stack by uploading the .template.json file. CDK and the CLI have some issues with Cloudfront being global. I tested this in us-east-1

Take a look at the rule properties and the output it produces. You might want to tweak a few things.

const acl = new wafv2.CfnWebACL(this, "ACL2", {
  defaultAction: {
    allow: true,
  },
  scope: "CLOUDFRONT",
  visibilityConfig: {
    cloudWatchMetricsEnabled: true,
    metricName: "waf", // tODO add a stage suffix
    sampledRequestsEnabled: false,
  },
  rules: {
    rules: [
      {
        name: "AWS-AWSManagedRulesCommonRuleSet",
        priority: 0,
        statement: {
          managedRuleGroupStatement: {
            vendorName: "AWS",
            name: "AWSManagedRulesCommonRuleSet"
          }
        },
        overrideAction: {
          none: {}
        },
        visibilityConfig: {
          sampledRequestsEnabled: true,
          cloudWatchMetricsEnabled: true,
          metricName: "AWS-AWSManagedRulesCommonRuleSet"
        }
      },
      {
        name: "AWS-AWSManagedRulesAmazonIpReputationList",
        priority: 1,
        statement: {
          managedRuleGroupStatement: {
            vendorName: "AWS",
            name: "AWSManagedRulesAmazonIpReputationList"
          }
        },
        overrideAction: {
          none: {}
        },
        visibilityConfig: {
          sampledRequestsEnabled: true,
          cloudWatchMetricsEnabled: true,
          metricName: "AWS-AWSManagedRulesAmazonIpReputationList"
        }
      },
      {
        name: "AWS-AWSManagedRulesKnownBadInputsRuleSet",
        priority: 2,
        statement: {
          managedRuleGroupStatement: {
            vendorName: "AWS",
            name: "AWSManagedRulesKnownBadInputsRuleSet"
          }
        },
        overrideAction: {
          none: {}
        },
        visibilityConfig: {
          sampledRequestsEnabled: true,
          cloudWatchMetricsEnabled: true,
          metricName: "AWS-AWSManagedRulesKnownBadInputsRuleSet"
        }
      },
      {
        name: "AWS-AWSManagedRulesLinuxRuleSet",
        priority: 3,
        statement: {
          managedRuleGroupStatement: {
            vendorName: "AWS",
            name: "AWSManagedRulesLinuxRuleSet"
          }
        },
        overrideAction: {
          none: {}
        },
        visibilityConfig: {
          sampledRequestsEnabled: true,
          cloudWatchMetricsEnabled: true,
          metricName: "AWS-AWSManagedRulesLinuxRuleSet"
        }
      },
      {
        name: "AWS-AWSManagedRulesSQLiRuleSet",
        priority: 4,
        statement: {
          managedRuleGroupStatement: {
            vendorName: "AWS",
            name: "AWSManagedRulesSQLiRuleSet"
          }
        },
        overrideAction: {
          none: {}
        },
        visibilityConfig: {
          sampledRequestsEnabled: true,
          cloudWatchMetricsEnabled: true,
          metricName: "AWS-AWSManagedRulesSQLiRuleSet"
        }
      }
    ],
  },
});
thibaut-pro commented 4 years ago

Thank you for the detailed steps. Manually uploading the modified output template to Cloudformation worked for me. I'll wait for @aws-cdk/aws-wafv2 to be updated to switch to using CDK for this.

RachelleJanssen commented 4 years ago

@SomayaB Please note that the question is about documentation, but the underlying issue is an unresolved bug (hence the bug report, which was flagged as a duplicate and got closed)

rix0rrr commented 4 years ago

For everyone affected by this, using the Escape Hatch mechanism (which applies any time the CloudFormation template the CDK generates is not the one you would like to see) is probably your only solution to tide you over until this issue gets resolved upstream by CloudFormation and we import the new schema.

rix0rrr commented 4 years ago

If someone feels like it and it takes too long to get traction from CloudFormation, we can also patch the schema locally. See here for an example:

https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/cfnspec/spec-source/500_IoT1Click_patch_PlacementTemplate_DeviceTemplates.json

If someone is willing to try their hand at that, I would be in favor of accepting that. We would need some tracking information in there though so we can remove the patch when it's no longer necessary. A reference to a bug report on the CloudFormation repository would be enough.

shivlaks commented 4 years ago

@rix0rrr - I was able to sync with a couple folks from the WAF team yesterday and they're in the midst of amending the resource specification. I think we should hold off on putting in a patch unless there are delays in importing the updated spec

jgeurts commented 4 years ago

This is a hack, but maybe you'll find it as an alternative to the escape hatch. This class rips through the generated cloudfront template and fixes the object array wrappers:

import * as cdk from '@aws-cdk/core';
import * as waf from '@aws-cdk/aws-wafv2';

interface Props {
  [key: string]: object | object[] | boolean | number | string | undefined;
}

/**
 * Fixes the CloudFormation output to change array wrappers like `Rules: { Rules: [] }` to `Rules: []`.
 */
export class FixedWebACL extends waf.CfnWebACL {
  protected renderProperties(properties: Props): Props {
    if (!cdk.canInspect(properties)) {
      return properties;
    }

    const originalBrokenSyntax = super.renderProperties(properties);

    const fixedSyntax: Props = {};
    // Fix properties that are objects when they should be arrays
    for (const [key, value] of Object.entries(originalBrokenSyntax)) {
      fixedSyntax[key] = this.fixObjectsWithNestedArrays(key, value);
    }

    return fixedSyntax;
  }

  private isObjectWithNestedArray(key: string, value: object | boolean | number | string | undefined): boolean {
    if (typeof value === 'undefined' || value === null) {
      return false;
    }

    if (typeof value === 'object') {
      const valueKeys = Object.keys(value);
      return valueKeys && valueKeys.length === 1 && valueKeys[0] === key;
    }

    return false;
  }

  private fixObjectsWithNestedArrays(key: string, value: object | object[] | boolean | number | string | undefined): object | object[] | boolean | number | string | undefined {
    if (typeof value === 'undefined' || value === null) {
      return value;
    }

    if (Array.isArray(value)) {
      const updatedArray: (object | object[] | boolean | number | string | undefined)[] = [];
      for (const item of value) {
        updatedArray.push(this.fixObjectsWithNestedArrays('__na__', item));
      }

      return updatedArray;
    }

    if (typeof value === 'object') {
      if (this.isObjectWithNestedArray(key, value)) {
        return this.fixObjectsWithNestedArrays('__na__', (value as Props)[key]);
      }

      const updatedValue: Props = {};
      for (const [childKey, childValue] of Object.entries(value)) {
        updatedValue[childKey] = this.fixObjectsWithNestedArrays(childKey, childValue);
      }

      return updatedValue;
    }

    return value;
  }
}

While it worked for my use case, your mileage might vary. Also, this is purely a hack until the underlying CF schema can be fixed.

RachelleJanssen commented 4 years ago

@jgeurts this is the solution is similar to what I wanted to post, because I did an accidental git reset --hard. Thanks for posting :)

grifx commented 4 years ago

@shivlaks FYI, there is a similar issue with the CfnIPSet addresses property:

const ipSet = new CfnIPSet(this, "office-ip-set", {
  addresses: {
    ipAddresses: ["1.2.3.4/32"]
  },
  ipAddressVersion: "IPv4",
  name: "FromOfficeIpAddress",
  scope: "CLOUDFRONT"
});
RachelleJanssen commented 4 years ago

I can confirm that the Rules: { Rules: [ ] } wrapper problem has been resolved by release 1.26 and I'm able to deploy the code without the escape hatch, happy days!

shivlaks commented 4 years ago

@grifx - a bevy of CloudFormation spec changes were introduced in #6424 and that's now been released in 1.26.0 - Is this still an issue?

thibaut-pro commented 4 years ago

I can also confirm that I was able to instantiate the WAF ACL with AWS CDK using 1.26. Here is the code that worked for me:

const acl = new wafv2.CfnWebACL(this, "ACL2", {
    defaultAction: {
        allow: { allow: true },
    },
    scope: "CLOUDFRONT",
    visibilityConfig: {
        cloudWatchMetricsEnabled: true,
        metricName: "waf",
        sampledRequestsEnabled: false,
    },
    rules: [
        {
            name: 'AWS-AWSManagedRulesAmazonIpReputationList',
            priority: 0,
            statement: {
                managedRuleGroupStatement: {
                    vendorName: 'AWS',
                    name: 'AWSManagedRulesAmazonIpReputationList',
                },
            },
            overrideAction: {
                none: {},
            },
            visibilityConfig: {
                sampledRequestsEnabled: true,
                cloudWatchMetricsEnabled: true,
                metricName: 'Staging-AWS-AWSManagedRulesAmazonIpReputationList',
            },
        },
        {
            name: 'AWS-AWSManagedRulesCommonRuleSet',
            priority: 1,
            statement: {
                managedRuleGroupStatement: {
                    vendorName: 'AWS',
                    name: 'AWSManagedRulesCommonRuleSet',
                },
            },
            overrideAction: {
                none: {},
            },
            visibilityConfig: {
                sampledRequestsEnabled: true,
                cloudWatchMetricsEnabled: true,
                metricName: 'Staging-AWS-AWSManagedRulesCommonRuleSet',
            },
        },
        {
            name: 'AWS-AWSManagedRulesKnownBadInputsRuleSet',
            priority: 2,
            statement: {
                managedRuleGroupStatement: {
                    vendorName: 'AWS',
                    name: 'AWSManagedRulesKnownBadInputsRuleSet',
                },
            },
            overrideAction: {
                none: {},
            },
            visibilityConfig: {
                sampledRequestsEnabled: true,
                cloudWatchMetricsEnabled: true,
                metricName: 'Staging-AWS-AWSManagedRulesKnownBadInputsRuleSet',
            },
        },
        {
            name: 'AWS-AWSManagedRulesLinuxRuleSet',
            priority: 3,
            statement: {
                managedRuleGroupStatement: {
                    vendorName: 'AWS',
                    name: 'AWSManagedRulesLinuxRuleSet',
                },
            },
            overrideAction: {
                none: {},
            },
            visibilityConfig: {
                sampledRequestsEnabled: true,
                cloudWatchMetricsEnabled: true,
                metricName: 'Staging-AWS-AWSManagedRulesLinuxRuleSet',
            },
        },
        {
            name: 'AWS-AWSManagedRulesSQLiRuleSet',
            priority: 4,
            statement: {
                managedRuleGroupStatement: {
                    vendorName: 'AWS',
                    name: 'AWSManagedRulesSQLiRuleSet',
                },
            },
            overrideAction: {
                none: {},
            },
            visibilityConfig: {
                sampledRequestsEnabled: true,
                cloudWatchMetricsEnabled: true,
                metricName: 'Staging-AWS-AWSManagedRulesSQLiRuleSet',
            },
        },
    ],
});
praddc commented 4 years ago

I'm sure it's just something I'm doing, but now that the original error is resolved, is anyone getting the following error now when trying this in Python:

Wafdev Error reason: Your statement has multiple values set for a field that requires exactly one value., field: RULE, parameter: Rule (Service: Wafv2, Status Code: 400, Request ID: e558fb83-afa0-484c-9831-9f8b06ac6c2b)

Code to create:

rule_sql = wafv2.CfnWebACL.RuleProperty(name='AWS-AWSManagedRulesSQLiRuleSet',
                                            statement=wafv2.CfnWebACL.StatementOneProperty(
                                                managed_rule_group_statement=wafv2.CfnWebACL.ManagedRuleGroupStatementProperty(
                                                    vendor_name='AWS',
                                                    name='AWSManagedRulesSQLiRuleSet'
                                                )
                                            ),
                                            visibility_config=wafv2.CfnWebACL.VisibilityConfigProperty(
                                                sampled_requests_enabled=True,
                                                cloud_watch_metrics_enabled=True,
                                                metric_name='AWS-AWSManagedRulesSQLiRuleSet'
                                            ),
                                            priority=0,
                                            )

my_waf = wafv2.CfnWebACL(scope, 'DevWaf' + environment,
                         name='DevWaf' + environment,
                         scope='REGIONAL',
                         default_action=wafv2.CfnWebACL.DefaultActionProperty(
                             allow={
                                 'Type': 'ALLOW'
                             }
                         ),
                         visibility_config=wafv2.CfnWebACL.VisibilityConfigProperty(
                             cloud_watch_metrics_enabled=True,
                             metric_name='DevWafMetric' + environment,
                             sampled_requests_enabled=True
                         ),
                         rules=[
                             rule_sql
                         ]
                         )

Which seems to output the correct JSON to the stack template:

"DevWafdev": {
  "Type": "AWS::WAFv2::WebACL",
  "Properties": {
    "DefaultAction": {
      "Allow": {
        "Type": "ALLOW"
      }
    },
    "Scope": "REGIONAL",
    "VisibilityConfig": {
      "CloudWatchMetricsEnabled": true,
      "MetricName": "CtWafMetricdev",
      "SampledRequestsEnabled": true
    },
    "Name": "DevWafdev",
    "Rules": [
      {
        "Name": "AWS-AWSManagedRulesSQLiRuleSet",
        "Priority": 0,
        "Statement": {
          "ManagedRuleGroupStatement": {
            "Name": "AWSManagedRulesSQLiRuleSet",
            "VendorName": "AWS"
          }
        },
        "VisibilityConfig": {
          "CloudWatchMetricsEnabled": true,
          "MetricName": "AWS-AWSManagedRulesSQLiRuleSet",
          "SampledRequestsEnabled": true
        }
      }
    ]
  },
  "Metadata": {
    "aws:cdk:path": "InfraStack/DevWafdev"
  }
},

(I'm using version 1.26 of all CDK packages, boto3 v1.12.8, and botocore v1.15.8)

RachelleJanssen commented 4 years ago

@praddc the error sounds familiar. Unfortunately, I currently don't have access to my machine so I can't reproduce it. But I believe to remember having the same error. The new spec changed the "defaultAction" field, or at least how it's validated. Instead of allow: { allow: true } I believe it should be allow: {}. I discovered this in the cloudformation documentation. Go to the examples and you'll see what I mean: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-wafv2-webacl.html#aws-resource-wafv2-webacl--examples

When you go into the CDK code, you'll see that the field has optional parameters. You have to pick one and assign an empty object to it. think the logic behind it is "if the property is set then that is the choice that was made. A nicer solution would be with an enum of course like defaultAction: DefaultActions.ALLOW.

Give it a try and I'll edit this post when I get to testing it.

praddc commented 4 years ago

@RachelleJanssen A good catch, and thank you for the comment, but sadly it's still giving the same error even after I fixed that. Let me know if you figure out anything else that looks off. Thanks!

(new resulting template:)

    "DevWafdev": {
      "Type": "AWS::WAFv2::WebACL",
      "Properties": {
        "DefaultAction": {
          "Allow": {}
        },
        "Scope": "REGIONAL",
        "VisibilityConfig": {
          "CloudWatchMetricsEnabled": true,
          "MetricName": "DevWafMetricdev",
          "SampledRequestsEnabled": true
        },
        "Name": "DevWafdev",
        "Rules": [
          {
            "Name": "AWS-AWSManagedRulesSQLiRuleSet",
            "Priority": 0,
            "Statement": {
              "ManagedRuleGroupStatement": {
                "Name": "AWSManagedRulesSQLiRuleSet",
                "VendorName": "AWS"
              }
            },
            "VisibilityConfig": {
              "CloudWatchMetricsEnabled": true,
              "MetricName": "AWS-AWSManagedRulesSQLiRuleSet",
              "SampledRequestsEnabled": true
            }
          }
        ]
      },
      "Metadata": {
        "aws:cdk:path": "InfraStack/DevWafdev"
      }
    },
rix0rrr commented 4 years ago

I'm going to close this issue as it looks like the remaining discussion points aren't about actionable things involving the CDK code base: it's about L1s and what values to plug into CloudFormation to get the result you want, which is the kind of question that belongs on Stack Overflow.

tmo-trustpilot commented 4 years ago

@RachelleJanssen A good catch, and thank you for the comment, but sadly it's still giving the same error even after I fixed that. Let me know if you figure out anything else that looks off. Thanks!

I've discovered you get the error "Your statement has multiple values set for a field that requires exactly one value., field: RULE, parameter: Rule" is returned if you are missing the OverrideAction on the ManagedRuleGroupStatement. The field is needed even if you don't want to override it, you specify "none" as per the docs.

OverrideAction:
  None: {}

Clearly a CF issue because the cdk types can't enforce this the way they are, but this GH Issue is the main result when searching for that error message so hopefully this helps someone.

praddc commented 4 years ago

@tmo-trustpilot This fixed my issue. Thank you very, very much!

jonrau1 commented 4 years ago

@tmo-trustpilot this fixed my issue in CFN native without CDK. The docs are not clear about having to include OverrideAction like at all

ralovely commented 4 years ago

Given this page is the first (and only, really) coming up in Google for the error

Your statement has multiple values set for a field that requires exactly one value., field: RULE, parameter: Rule"

I would add to @tmo-trustpilot 's comment above:

Hope this helps someone.

mirkop-mattr commented 4 years ago

Hope this helps someone.

Thanks a lot @ralovely . This did help me while debugging the same issue using terraform.

sarbajitdutta commented 3 years ago

I cannot seem to pass none: {} for overrideAction using cdk

                  RuleProperty.builder()
                    .name("AWSManagedRulesLinuxRuleSet")
                    .priority(1)
                    .overrideAction(OverrideActionProperty.builder().none("{}").build())
                    .visibilityConfig(VisibilityConfigProperty.builder()
                        .cloudWatchMetricsEnabled(true)
                        .sampledRequestsEnabled(true)
                        .metricName("cdk-cbt-api-AWSManagedRulesLinuxRuleSetMetric")
                        .build())
                    .statement(StatementProperty.builder()
                        .managedRuleGroupStatement(ManagedRuleGroupStatementProperty.builder()
                            .vendorName("AWS")
                            .name("AWSManagedRulesLinuxRuleSet")
                            .excludedRules(Arrays.asList())
                            .build())
                        .build())
                    .build();

Gives me error:

Caused by: software.amazon.jsii.JsiiException: Resolution error: Supplied properties not correct for "CfnWebACLProps"
  rules: element 0: supplied properties not correct for "RuleProperty"
    overrideAction: supplied properties not correct for "OverrideActionProperty"
      none: "{}" should be an 'object'.
skinny85 commented 3 years ago

@sarbajitdutta can you specify what resource are you trying to construct? Is this for WAF V1, or V2?

In other words, we need more context to help you than just that one snippet 🙂.

sarbajitdutta commented 3 years ago

@skinny85 Its WAFv2. This is where the code is code I finally did it by using JSONObject - .overrideAction(OverrideActionProperty.builder().none(new JSONObject()).build()) I want to see how can it be done natively without using 3rd party libraries.

skinny85 commented 3 years ago

Can you try .overrideAction(OverrideActionProperty.builder().none(new HashMap<String, Object>()).build()), and let me know if it works?

phrastnik commented 3 years ago

Just stumbled across this thread and can confirm that ....lder().none(new HashMap<String, Object>())...... works.