Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: ipsets #388

Closed
wants to merge 2 commits into from
Closed

Fix: ipsets #388

wants to merge 2 commits into from

Conversation

vboufleur
Copy link
Contributor

Two small fixes related to ipsets management. The one related to capacity management should be considered as a tmp workaround, as it requires a great change on the repo code, which is 1) deploy the ipsets on a different CloudFormation stack, 2) output the ipset ARN on the first stack, 3) replace the ipset name with the ARN on the FMS config, 4) then finally, deploy the FMS CloudFormation stack.

@@ -593,7 +593,16 @@ function calculateRatebasedStatementwithoutScopeDownStatement(customRule: FmsRul
* @returns the capacity of the IPSet statement
*/
function calculateIpsSetStatementCapacity(ipSetReferenceStatement: wafv2.CfnWebACL.IPSetReferenceStatementProperty) {
let ipSetRuleCapacity = 1;
// FIXME: ipset statements doesn't always has a capacity of one, it can vary, which was giving a error while deploying (sample of the error available below)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, according to the docs an IPSet reference statement is only 4 if you configure the statement to use forwarded IP addresses and specify a position of ANY, increase the WCU usage by 4.

https://docs.aws.amazon.com/waf/latest/developerguide/waf-rule-statement-type-ipset-match.html

Could you please give me an example rule which was not working, maybe we need to ask AWS to change their docs?

Copy link
Contributor Author

@vboufleur vboufleur May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @daknhh! It says in the docs which you've sent that the cost is 1 WCU for most, not for all cases.

image

I think there's some edge cases that the cost of a simple single IpsetReferenceStatement can be bigger then 1 WCU. In my FMS configs the error wasn't happening on all other configs, but on a single one which has a Scope of CLOUDFRONT.

I don't know if the scope and/or the Type of AWS::CloudFront::Distribution has anything to do with the bug, with the WCU cost being higher then "1", but I know that WAF started deploying on this config after I've set a fixed WCU cost of 4 for single IpSetStatements, hence this commit.

I've suggested a long term solution for this issue in the comments on the commit.

Copy link
Contributor

@daknhh daknhh Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be. I will open a support case on Monday to clarify with AWS support. I would like to avoid to just create a separate stack for IpSets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still waiting for Feedback from AWS Support. I am on it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :) Thanks

@daknhh
Copy link
Contributor

daknhh commented Jun 7, 2024

I wish to inform that, while calling IPset rule in WebACL, under “IP address to use as the originating address” we have two options.

  1. Source IP address
  2. IP address in Header (This has further three Subdivisions)
    a) First IP address
    b) Last IP address
    c) Any IP addresses

WCU 1 will be consumed for IPset rule along with the options Source IP or First IP address or Last IP address. When you choose Any IP address options it will be using WCU 4, so total you can see 5 WCU (1 for IPset Rule + 4 for Any IP address Option ), when using IPset in combination with Any IP address. <- this is what aws support reponded.

So technically:

/**
   * Implementation of the calculation of the capacity for IPSet statements according to https://docs.aws.amazon.com/waf/latest/developerguide/waf-rule-statement-type-ipset-match.html
   * @param ipSetReferenceStatement the IPSetReferenceStatement
   * @returns the capacity of the IPSet statement
   */
function calculateIpsSetStatementCapacity(ipSetReferenceStatement: wafv2.CfnWebACL.IPSetReferenceStatementProperty) {
  let ipSetRuleCapacity = 1;
  const ipSetForwardedIpConfig = ipSetReferenceStatement.ipSetForwardedIpConfig as wafv2.CfnWebACL.IPSetForwardedIPConfigurationProperty | undefined;
  if(ipSetForwardedIpConfig && ipSetForwardedIpConfig.position === "ANY") ipSetRuleCapacity += 4;
  return ipSetRuleCapacity;
}

This should work

@daknhh
Copy link
Contributor

daknhh commented Jun 17, 2024

Hi @vboufleur i was testing with the ipSets-global.ts with the calculation fix which was working fine.
Branch:
https://github.com/globaldatanet/aws-firewall-factory/tree/pullrequests/vboufleur/fix/ipsets

@vboufleur
Copy link
Contributor Author

Hi @daknhh, sorry for the delay in answering you, I got very busy with work lately.

As soon as I get some free time I'll retest setting the WCU to only "1" and I'll let you know on the results.

@daknhh daknhh closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants