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

Custom Block Response #56

Merged
merged 16 commits into from
May 17, 2022
Merged

Custom Block Response #56

merged 16 commits into from
May 17, 2022

Conversation

rsmets
Copy link
Contributor

@rsmets rsmets commented May 14, 2022

Description

I added the capability to define custom responses for block actions. More info on custom responses can be found here and here.

Note that this does not apply to managed rule group statements per AWS design.

@Ohid25 Ohid25 added the enhancement New feature or request label May 16, 2022
Copy link
Contributor

@Ohid25 Ohid25 left a comment

Choose a reason for hiding this comment

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

Appreciate the contribution!

I have a request if you can make the changes to what you have done so far.

Also, if you can please run the make commands to validate, add to changelog and docs, i'd appreciate that too!

main.tf Outdated
content {}
content {
custom_response {
response_code = var.custom_response_code
Copy link
Contributor

@Ohid25 Ohid25 May 16, 2022

Choose a reason for hiding this comment

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

Suggested change
response_code = var.custom_response_code
response_code = lookup(rule.custom_response, 403)

Instead of using a variable, a lookup function should be used since these arguments are filled in via the rules variable. This would eliminate the need to create a standalone variable for this.

Also, from the docs you sent I can see that there are two other arguments that can be used here based on terraform docs:

    "Block": {
     "CustomResponse": {
      "CustomResponseBodyKey": "CustomResponseBodyKey1",
      "ResponseCode": 404,
      "ResponseHeaders": [
       {
        "Name": "BlockActionHeader1Name",
        "Value": "BlockActionHeader1Value"
       }
      ]
     }

Can you please add these too?

@rsmets rsmets changed the title Block status code Custom Block Response May 16, 2022
@rsmets
Copy link
Contributor Author

rsmets commented May 16, 2022

Okay, added the ability to define the custom body and headers as well. Certainly more robust now. 👍🏼

However, I am unable to run the make commands. I also do not see any documentation on them?

Edit: ah I see... tucked under the Install dependencies section of the README.

@rsmets
Copy link
Contributor Author

rsmets commented May 16, 2022

Make commands ran ✅

variables.tf Outdated
Comment on lines 90 to 100
variable "enable_custom_response" {
type = bool
description = "Set to `true` to define custom responses (status code, body, and/or headers) for non managed rule group statements block actions."
default = false
}

variable "custom_response_body" {
type = map(string)
description = "A custom response body to be referenced on a per rule basis."
default = {}
}
Copy link
Contributor

@naseemkullah naseemkullah May 16, 2022

Choose a reason for hiding this comment

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

Just saw this PR! Looking to solve the same problem, WDYT about an array of custom response bodies so we are not limited to just one? Please see https://github.com/umotif-public/terraform-aws-waf-webaclv2/pull/57/files

Copy link
Contributor Author

@rsmets rsmets May 16, 2022

Choose a reason for hiding this comment

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

@naseemkullah have you tested that? I was not able to get it to work with an array input for custom_response_body. I like your use of object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated my PR with the inclusion of your purposed custom_response_bodies. 👍🏼 Good call.

…tions. Dropped the enabled variable as not needed.
Copy link
Contributor

@Ohid25 Ohid25 left a comment

Choose a reason for hiding this comment

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

Really appreciate the examples!

I just have a few comments to make sure the examples will work.

Comment on lines 26 to 34
output "custom_ip_set_arn" {
description = "The ARN of the Custom IP Set"
value = aws_wafv2_ip_set.custom_ip_set.arn
}

output "block_ip_set_arn" {
description = "The ARN of the Block IP Set"
value = aws_wafv2_ip_set.block_ip_set.arn
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these resources that you're referencing here in the outputs?

This example with these outputs.

Similar comment to the other outputs in the other examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Good catch... got overzealous with my copy pasta.

}
]

rules = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch here as well... nuked the visibility_config from the copy of my personal block because had local variables I didn't want to bother with here. But just added the block ✔️ .

Comment on lines 38 to 50
custom_response = true
custom_response_code = 412
custom_response_key = "custom_response_body_1"
custom_response_headers = [
{
name = "X-Custom-Header"
value = "You are not authorized to access this resource."
},
{
name = "X-Custom-Header-2"
value = "You are still not authorized to access this resource."
}
]
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
custom_response = true
custom_response_code = 412
custom_response_key = "custom_response_body_1"
custom_response_headers = [
{
name = "X-Custom-Header"
value = "You are not authorized to access this resource."
},
{
name = "X-Custom-Header-2"
value = "You are still not authorized to access this resource."
}
]
custom_response = {
enabled = true
code = 412
body_key = "custom_response_body_1"
headers = [
{
name = "X-Custom-Header"
value = "You are not authorized to access this resource."
},
{
name = "X-Custom-Header-2"
value = "You are still not authorized to access this resource."
}
]
}

nit: nested instead of flat

}
}
]
}
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
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Was working on this very change. :)

@rsmets
Copy link
Contributor Author

rsmets commented May 17, 2022

Thanks for all the feedback @Ohid25 and @naseemkullah. I think this is ready to go.

Took me a bit to grasp the super slick syntax for dynamic rules (i.e. from visibility_config) , which I used for the custom_response rule block:

  dynamic "custom_response" {
    for_each = length(lookup(rule.value, "custom_response", [])) == 0 ? [] : [lookup(rule.value, "custom_response", {})]

Very clever!


custom_response = {
response_code = 412
response_key = "default_1",
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
response_key = "default_1",
custom_response_body_key = "default_1",

Just a better exposure of the underlying api imho (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-customresponse.html)

main.tf Outdated
dynamic "custom_response" {
for_each = length(lookup(rule.value, "custom_response", [])) == 0 ? [] : [lookup(rule.value, "custom_response", {})]
content {
custom_response_body_key = lookup(custom_response.value, "response_key", null)
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
custom_response_body_key = lookup(custom_response.value, "response_key", null)
custom_response_body_key = lookup(custom_response.value, "custom_response_body_key", null)

Just better exposure of underlying api

@naseemkullah
Copy link
Contributor

lgtm, as mentioned optional nit: would just change response_key for custom_response_body_key to expose the underlying api as is

Copy link
Contributor

@Ohid25 Ohid25 left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@Ohid25 Ohid25 merged commit 1d3e1c2 into umotif-public:main May 17, 2022
@rsmets rsmets deleted the block_status_code branch May 17, 2022 19:48
@rsmets rsmets restored the block_status_code branch May 17, 2022 19:48
@Ohid25
Copy link
Contributor

Ohid25 commented May 18, 2022

Thanks for the contribution! 🎉 .

This is now released as a part of 3.8.0

@rsmets rsmets deleted the block_status_code branch May 19, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants