aws-cloudformation / cloudformation-coverage-roadmap

The AWS CloudFormation Public Coverage Roadmap
https://aws.amazon.com/cloudformation/
Creative Commons Attribution Share Alike 4.0 International
1.11k stars 54 forks source link

WAFv2 Bugfix/Feature #331

Closed jamiepmullan closed 4 years ago

jamiepmullan commented 4 years ago

I am developing an application via CDK (but using the Cloudformation objects as WAF isn't implemented properly yet).

The bug

  1. I am experiencing a bug where I get the error Internal Failure with the following definition:
new CfnRuleGroup(this, "rulegroup", {
      name: "raterule",
      description: "RateRule",
      rules: {
        rules: [
          {
            name: "RuleOne",
            priority: 1,
            action: {
              allow: true
            },
            statement: {
              rateBasedStatement: {
                aggregateKeyType: "IP",
                limit: 2010
              }
            }
          }
        ]
      },
      scope: "REGIONAL"
    } 
);

Unfortunately doesn't give me a lot more information and checked over this a few times with the docs. Is there any gotcha's that might affect this from working?

The Feature?

  1. I've been trying to use WAFv2 so that I can associate that with an API Gateway instance (without having to deploy a Cloudfront instance to just do this). I've looked at the docs, but I can't find the equivalent to CfnWebACLAssociation. I know its in the documentation here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/ReleaseHistory.html Any ideas on when this will be fulfilled to use it?

  2. Along from this, when I was trying out the waf-regional originally (instead of wafv2), when I called CfnWebACLAssociation it always came back with the error:

    The referenced item does not exist. (Service: AWSWAFRegional; Status Code: 400; Error Code: WAFNonexistentItemException;

    Is this going to be fixed? As its a really not scalable as we need to put workarounds in to assign the WAF ACL to the API gateway. This is mentioned in this ticket too: https://github.com/aws-cloudformation/aws-cloudformation-coverage-roadmap/issues/60#issuecomment-518211227

Let me know if you want me to clarify anything.

luiseduardocolon commented 4 years ago

The line in release history is incorrect: "The following resource was added: AWS::WAFv2::WebACLAssociation" This is not part of WAFV2 but part of WAVRegional. We have alerted our docs team to fix this.

jamiepmullan commented 4 years ago

@luiseduardocolon - Thanks, are there any plans to add the feature so that WAF ACL, Rules, and association can be done in Cloudformation? (and not fail like regional WAF does at the moment).

luiseduardocolon commented 4 years ago

@jamiepmullan I don't know of any short-term plans for this. I'd like this to become a formal coverage request (as in, it is already possible via APIs but not via CloudFormation) and then try to get as much attention (+1s) as possible to help us drive attention to it.

dveijck commented 4 years ago

@jamiepmullan I've been fiddling around with CDK and WAFv2 in a similar way like you and ran into the same Internal Failure error.

The problem lies in the nested rules statement:

new CfnRuleGroup(this, "rulegroup", {
      ...
      rules: {
        rules: [ // <-- here is your problem
          ...
        ]
      },
      scope: "REGIONAL"
    } 
);

whereas CloudFormation wants this:

new CfnRuleGroup(this, "rulegroup", {
      ...
      rules: [
        ... // This is how it should have been
      ],
      scope: "REGIONAL"
    } 
);

I was able to work around this by creating my own RuleProperty interface and add a RuleProperty[] to the CfnWebACL as a property override. The Typescript interfaces get converted to the correct yaml (but note the PascalCase in the example below). You should be able to do the same for the CfnRuleGroup.

For example something like this should work:

export class MyWafv2Stack extends cdk.Stack {

  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    ...
    const rules: RuleProperty[] = []; // <-- this is my 'own' RuleProperty definition that mimicks the CDK generated resources
    rules.push(
      {
        Name: "RuleOne", // Note the PascalCase for all the properties
        Priority: 1,
        Action: {
          Allow: {} // This is also different wrt your question
        },
        Statement: {
          RateBasedStatement: {
            AggregateKeyType: "IP",
            Limit: 2010
          }
        }
      }
    );
    const cfnRuleGroup = new CfnRuleGroup(
      this, 
      "rulegroup", 
      {
        name: "raterule", // No need for PascalCase here as this is just the CDK generate stuff that works correctly
        description: "RateRule",
        scope: "REGIONAL"
      }
    );
    cfnRuleGroup.addPropertyOverride("Rules", rules); // <-- and this is where I add the property override for the rules

    ...
  }

}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-rule.html
 * Note the PascalCase
 */
interface RuleProperty { // Here is my 'own' RuleProperty definition
  Action?: RuleActionProperty // Need to provide my 'own' definition for this class as well
  Name: string,
  OverrideAction?: OverrideActionProperty, // Need to provide my 'own' definition for this class as well
  Priority: number,
  Statement: StatementOneProperty, // Need to provide my 'own' definition for this class as well
  VisibilityConfig: VisibilityConfigProperty // Need to provide my 'own' definition for this class as well
}

interface RuleActionProperty {
  ...
}
... // etc.

The huge drawback is however that you need to mimick all the classes that CDK does generate for you. You could implement only the parts that you need .

I'm not sure if the problem is with the CloudFormation documentation and spec or the way CDK generates the Cfn* resources from this spec. However IMHO the CloudFormation WAFv2 spec is confusing in the way it seems to declare the nested resources in multiple layers. For example the same issue exists when defining a GeoMatchStatement with respect to CountryCodes (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-geomatchstatement.html).

Sorry for the long post, but hope the example helps to clarify it for you or anybody who comes upon this issue.

dveijck commented 4 years ago

@jamiepmullan as with respect to your second question, you could use CDK's AwsCustomResource to call the WAFV2 SDK service and associate the WebACL to an API Gateway. This is one of the ways to implement a custom resource when CloudFormation doesn't support AWS functionality (yet).

Have a closer look at:

jamiepmullan commented 4 years ago

Thanks @dveijck - useful, will give it a go!

Saw is starting to release more WAFv2 resources in Cloudformation, so hopefully this will all be released soon: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/AWS_WAFv2.html

jamiepmullan commented 4 years ago

@dveijck - Finally got around to trying this out... unfortunately its not what it appears... You're still met with the error: Error reason: A reference in your rule statement is not valid., field: RATE_BASED_STATEMENT, parameter: RateBasedStatement Tried few different formats and that but still breaking! Hopefully this will be fixed... Just tried v1.23.0 and still experiencing the issue

dveijck commented 4 years ago

@jamiepmullan sad to hear you didn't get this to work. I did a quick experiment where I modified my own WAFv2 WebACL to see if I could roll out the RateBasedStatement. It did roll out successfully, so maybe there is another minor issue somewhere. Note that I'm still on CDK 1.20.0 (I wouldn't expect that to make a difference in this case) and that I didn't try it with a CfnRuleGroup (but looking at the CloudFormation docs the 'Rules' have the same structure (apart from managed rules related properties)).

My code looks like this (disclaimer: I had to cut and paste some of the stuff from my own code so hopefully that didn't introduce any errors):

export class MyWafv2Stack extends cdk.Stack {

  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    const rules: RuleProperty[] = []; // My own RuleProperty that mimicks the CDK generated resources
    rules.push(
      {
        Name: 'RuleOne,
        Action: {
          Block: {}
        },
        Priority: 1,
        Statement: {
          RateBasedStatement: {
            AggregateKeyType: 'IP',
            Limit: 2010
          }
        },
        VisibilityConfig: {
          MetricName: 'metricRuleOne'
          CloudWatchMetricsEnabled: true,
          SampledRequestsEnabled: true
        }
      }
    );
    const webAcl = new CfnWebACL(
        this.scope,
        'myWebAcl',
        {
          name: 'myWebAcl',
          description: `My Web ACL`,
          scope: 'REGIONAL',
          defaultAction: {
            allow: {}
          },
          visibilityConfig: {
            metricName: 'metricMyWebAcl',
            cloudWatchMetricsEnabled: true,
            sampledRequestsEnabled: true
          }
        }
    );
    webAcl.addPropertyOverride("Rules", rules); // Custom override to get correct CloudFormation template.
  }

}

/*
 * Below are the mimicked classes similar to what CDK generates apart from the PascalCase properties
 */

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-rule.html
 */
interface RuleProperty {
  Action?: RuleActionProperty
  Name: string,
  Priority: number,
  Statement: StatementOneProperty,
  VisibilityConfig: VisibilityConfigProperty
}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ruleaction.html
 */
interface RuleActionProperty {
  Allow?: AllowAction,
  Block?: BlockAction,
  Count?: CountAction
}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-overrideaction.html
 */
interface OverrideActionProperty {
  None?: NoneAction,
  Count?: CountAction
}

interface AllowAction {}
interface BlockAction {}
interface CountAction {}
interface NoneAction {}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-visibilityconfig.html
 */
interface VisibilityConfigProperty {
  MetricName: string,
  CloudWatchMetricsEnabled: boolean,
  SampledRequestsEnabled: boolean
}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-statementone.html
 * Expand this class as necessary, only used statements are added, but more are supported, see documentation.
 */
interface StatementOneProperty {
  RateBasedStatement?: RateBasedStatementProperty
}

/**
 * See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ratebasedstatementone.html#cfn-wafv2-webacl-ratebasedstatementone-scopedownstatement
 */
interface RateBasedStatementProperty {
  AggregateKeyType: string,
  Limit: number
}

Hope this helps in finding the error.

jamiepmullan commented 4 years ago

@dveijck Ah just changed it from CfnRuleGroup to CfnWebACL and it deployed ok!

Correction it works when actually trying a custom resource:


  public attachWaf(wafArn: string): void {
    new CfnResource(this, this.resourceName, {
      type: "AWS::WAFv2::WebACLAssociation",
      properties: {
        ResourceArn: `arn:aws:apigateway:${this.region}::/restapis/${Token.asString(this.api.restApiId)}/stages/prod`,
        WebACLArn: wafArn
      }
    });
  }
dveijck commented 4 years ago

@jamiepmullan it looks like it's really coming soon (in the other issue you've created: https://github.com/aws-cloudformation/aws-cloudformation-coverage-roadmap/issues/344#issuecomment-582848686).

But currently I'm using something like this:

private associateWebAcl(webACL: CfnWebACL) {
    const albArn = Fn.importValue(...); // In my case the ALB is in a different stack and its arn is exported.
    const albWebACLAssociation = new AwsCustomResource( // TODO: This should be replaced with a CDK resource as soon as it is supported by CDK or CloudFormation.
        this.scope,
        'alb-web-acl-association',
        {
          onCreate: {
            service: 'WAFV2',
            action: 'associateWebACL',
            parameters: {
              ResourceArn: albArn,
              WebACLArn: webACL.attrArn
            },
            physicalResourceId: 'something-you-want' // There is nothing to update on a WebACL association, so this should be fine.
          },
          onDelete: {
            service: 'WAFV2',
            action: 'disassociateWebACL',
            parameters: {
              ResourceArn: albArn,
            },
            physicalResourceId: 'something-you-want' // There is nothing to update on a WebACL association, so this should be fine.
          }
        }
    );
    // Additional permissions are required, see: https://docs.aws.amazon.com/waf/latest/developerguide/waf-api-permissions-ref.html
    const policyStatement = new PolicyStatement();
    policyStatement.effect = Effect.ALLOW;
    policyStatement.addActions('elasticloadbalancing:SetWebACL');
    policyStatement.addResources(albArn);
    publicAlbWebACLAssociation.grantPrincipal.addToPolicy(policyStatement);
}
jamiepmullan commented 4 years ago

@dveijck see above comment i edited 😄 cheers for your help!