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.

Sign up for GitHub

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

Jump to bottom

FUM-3033-waf-module-refactor #4

Merged
merged 16 commits into from
Feb 22, 2024
Merged

FUM-3033-waf-module-refactor #4

merged 16 commits into from
Feb 22, 2024

Conversation

DCamma
Copy link
Contributor

@DCamma DCamma commented Feb 20, 2024 •
edited
Loading

This will be a major release, a lot of breaking changes.

The goal of the refactor is to

  • reduce the number of inputs required
  • stronger defaults based on our usage of the module
  • re-order priorities
  • cleanup conditional code
  • more specific variables (less list(object(any)))

Added functionalities:

  • AWS managed rule groups
  • Rules based on the aws managed rule labels can be used in block, captcha or challenge mode
  • possibility to have different rules per groups of labels

⚠️ Breaking changes:

  • removed support for allowed_partners (whitelisting of hostnames). This is dangerous because the hostname header can be modified
  • logs_bucket_name becomes logs_bucket_name_override
  • var.self_ips, var.allowed_ips and var.whitelisted_ip_ranges have been removed and consolidated into var.whitelisted_ips_v4
  • allowed_ips_v6 becomes whitelisted_ips_v6
  • all objects that contained the field country_code now required the filed country_codes instead
  • aws_managed_rules_labels list becomes aws_managed_rule_groups object
  • aws_managed_rules list becomes aws_managed_rule_lables object
  • aws_managed_rules_limit has been removed and is now part of each object in aws_managed_rule_lables
  • enable_count_ch_requests becomes count_requests_from_ch
  • count_ch_priority removed, enable_count_ch_requests rule now have a fixed priority
  • count_ch_limit removed, this rule is anyway a failsafe that would be changed in the aws console during attacks, therefore the limit can also be changed there
  • search_limitation becomes limit_search_requests_by_countries, a limit of 0 downst deactivate the rule, but and empty list of countries does
  • Priorities are now enforced by varaibles validations

@DCamma DCamma force-pushed the FUM-3033-waf-module-refactor branch 3 times, most recently from 8755b7b to 8845f86 Compare February 21, 2024 15:39
@DCamma DCamma force-pushed the FUM-3033-waf-module-refactor branch from f7145e0 to 93d003b Compare February 22, 2024 10:38
@DCamma DCamma force-pushed the FUM-3033-waf-module-refactor branch from e5cba1c to f324794 Compare February 22, 2024 11:34
DCamma
DCamma commented Feb 22, 2024
View reviewed changes
variables.tf
Comment on lines -13 to -29
variable "self_ips" {
default = []
description = "The IP from own AWS account (NAT gateways)"
type = set(string)
}

variable "allowed_ips" {
default = []
description = "The IPv4 to allow"
type = set(string)
}

variable "whitelisted_ip_ranges" {
default = []
description = "List of enterprise IP ranges to be whitelisted. Set to empty list to disable the whitelisting"
type = list(string)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consolidated into whitelisted_ips_v4

DCamma
DCamma commented Feb 22, 2024
View reviewed changes
DCamma
DCamma commented Feb 22, 2024
View reviewed changes
DCamma
DCamma commented Feb 22, 2024
View reviewed changes
waf.tf Outdated
Comment on lines 352 to 362
# dynamic "challenge_config" {
# # avaliable in the console but seems to be not supported on tf (?)
# # even if is mentioned in the docs https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/wafv2_web_acl#challenge_config-block
# # and is in an open issue https://github.com/hashicorp/terraform-provider-aws/issues/29071
# for_each = rule.value.action == "challenge" ? [1] : []
# content {
# immunity_time_property {
# immunity_time = rule.value.immunity_seconds
# }
# }
# }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not clear why this is, but there is no way to set a custom immunity time for challenges in terraform

DCamma
DCamma commented Feb 22, 2024
View reviewed changes
waf.tf
# Change "count" to "block" if you are under attack and want to
# rate limit to a low number of requests every country apart from Switzerland
rule {
name = "Rate_limit_everything_apart_from_CH"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just moved, not completely removed

DCamma
DCamma commented Feb 22, 2024
View reviewed changes
waf.tf
}

dynamic "rule" {
for_each = var.enable_count_ch_requests ? [1] : []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

DCamma
DCamma commented Feb 22, 2024
View reviewed changes
waf.tf
}

dynamic "rule" {
for_each = var.aws_managed_rules
Copy link
Contributor Author

Choose a reason for hiding this comment

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

completely reworked

DCamma
DCamma commented Feb 22, 2024
View reviewed changes
waf.tf
# rule for var.aws_managed_rules this rule would have no labels to check and therefore should not be created
for_each = length(var.aws_managed_rules_labels) > 0 && length(var.aws_managed_rules) > 0 ? [1] : []
content {
name = "Rate_limit_based_on_AWS_labels"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

completely reworked

@DCamma DCamma marked this pull request as ready for review February 22, 2024 12:39
sgametrio
sgametrio reviewed Feb 22, 2024
View reviewed changes
@sgametrio
Copy link
Contributor

Good stuff, added some comments

DCamma and others added 6 commits February 22, 2024 14:47
@DCamma @sgametrio
Update README.md
c777d7c 
Co-authored-by: Demetrio Carrara <[email protected]>
@DCamma @sgametrio
Update variables.tf
7719cef 
Co-authored-by: Demetrio Carrara <[email protected]>
@DCamma @sgametrio
Update variables.tf
92fca83 
Co-authored-by: Demetrio Carrara <[email protected]>
@DCamma @sgametrio
Update waf.tf
426189f 
Co-authored-by: Demetrio Carrara <[email protected]>
@DCamma @sgametrio
Update waf.tf
f790ece 
Co-authored-by: Demetrio Carrara <[email protected]>
@DCamma
FUM-3033 move example in separated folder
1a4eb6d 
Further examples that can be created: regional waf
@DCamma DCamma force-pushed the FUM-3033-waf-module-refactor branch from d843775 to 1a4eb6d Compare February 22, 2024 14:18
sgametrio
sgametrio approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@sgametrio sgametrio left a comment

Choose a reason for hiding this comment

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

LGTM

variables.tf Outdated
}

variable "aws_managed_rules_labels" {
variable "aws_managed_rule_lables" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
variable "aws_managed_rule_lables" {
variable "aws_managed_rule_labels" {

DCamma and others added 2 commits February 22, 2024 16:30
@DCamma DCamma merged commit 532e578 into main Feb 22, 2024
4 checks passed
@DCamma DCamma deleted the FUM-3033-waf-module-refactor branch February 22, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgametrio sgametrio sgametrio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DCamma @sgametrio