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

Feat(Terraform): front door #222

Merged
merged 13 commits into from
Dec 13, 2022
Merged

Feat(Terraform): front door #222

merged 13 commits into from
Dec 13, 2022

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Nov 30, 2022

Closes #208

This PR sets up a Front Door with a WAF policy to allow only a certain list of IP addresses.

Note there is some overlap with #213 since the whitelist is different per-environment.

Before merging:

  • Clean up manually created Front Door resources that were used to get a working configuration
  • Set TF_VAR_IP_ADDRESS_WHITELIST_DEV variable in Azure pipeline

After merging:

  • Update Benefits dev data migration to use front-door hostname in EligibilityVerifier API URL

Some reference material

@angela-tran angela-tran self-assigned this Nov 30, 2022
remove ip restriction blocks from app service because for app services
that integrate with a virtual network using service endpoints (such as
the CDT-hosted Benefits client), requests will be routed through Azure's
optimized backbone and will not use the app's list of outbound IP
addresses.
only allow our Front Door and availability tests.
@angela-tran angela-tran added this to the Courtesy Cards milestone Nov 30, 2022
@angela-tran angela-tran marked this pull request as ready for review November 30, 2022 23:06
@angela-tran angela-tran requested a review from a team as a code owner November 30, 2022 23:06
Copy link
Contributor

@afeld afeld left a comment

Choose a reason for hiding this comment

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

Nice work! Just one blocker; the rest are optional. Questions:

  • Thoughts about adding a bit of documentation around why we're using Front Door? Or sufficient to have this comment?
  • We are allowing the availability tests through, which is good… What do we think about having a smoke test to ensure other requests are not allowed? Could do this through a GitHub Action or Azure Pipeline task.

terraform/app_service.tf Show resolved Hide resolved
resource "azurerm_cdn_frontdoor_origin_group" "main" {
name = local.front_door_name
cdn_frontdoor_profile_id = azurerm_cdn_frontdoor_profile.main.id
session_affinity_enabled = true
Copy link
Contributor

@afeld afeld Dec 2, 2022

Choose a reason for hiding this comment

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

Minor: Since (I don't think) we're using cookies, not sure this is necessary. Given there's only one origin, not sure it would make a difference anyway.

Suggested change
session_affinity_enabled = true

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in f1bd673

https_port = 443
origin_host_header = azurerm_linux_web_app.main.default_hostname
certificate_name_check_enabled = true
priority = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: The numbers above are all same as the defaults, so could be removed for brevity. Fine either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 0c160c7

terraform/front_door.tf Show resolved Hide resolved
@angela-tran
Copy link
Member Author

Thoughts about adding a bit of documentation around why we're using Front Door? Or sufficient to have #208 (comment)?

I added a section to the README in 5e6f74e

@angela-tran
Copy link
Member Author

We are allowing the availability tests through, which is good… What do we think about having a smoke test to ensure other requests are not allowed? Could do this through a GitHub Action or Azure Pipeline task.

Any objection to creating a new issue for this?

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Left some questions - not necessarily required changes, but hoping for some additional clarity ahead of Approval.

terraform/front_door.tf Show resolved Hide resolved
enabled = true
mode = "Prevention"
custom_block_response_status_code = 403
custom_block_response_body = base64encode("Blocked")
Copy link
Member

Choose a reason for hiding this comment

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

The term most often associated with 403 is Forbidden: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 14a548b


match_condition {
match_variable = "RequestUri"
operator = "EndsWith"
Copy link
Member

Choose a reason for hiding this comment

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

This mean any request that ends with /healthcheck will go through, right?

Should we do an exact match here to shore up this one exception and make sure it is the only one? I think Equals is the operator for that: https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/cdn_frontdoor_firewall_policy#operator

Copy link
Member Author

Choose a reason for hiding this comment

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

It took me a bit to figure out what the Equals operator is matching against, but I got it to work. The match value needs to include the URI scheme, host, and port in addition to the path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in f9b039d

terraform/front_door.tf Show resolved Hide resolved
Copy link
Contributor

@afeld afeld left a comment

Choose a reason for hiding this comment

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

We currently have the uptime check accessing the App Service directly, not through Front Door. Thinking we should change it to the latter to make it end-to-end. This would remove the need for allowing those requests to the App Service, though not a big deal to leave them.

@thekaveman
Copy link
Member

We currently have the uptime check accessing the App Service directly, not through Front Door. Thinking we should change it to the latter to make it end-to-end. This would remove the need for allowing those requests to the App Service, though not a big deal to leave them.

Seems like a good follow-up! #223

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Looks like a great start, thank you @angela-tran! We can worry about deployment when you're back 😄

@angela-tran
Copy link
Member Author

@afeld let me know if there's anything else to change here. This needs your approval before I can merge. Thanks!

@angela-tran angela-tran merged commit caa686c into dev Dec 13, 2022
@angela-tran angela-tran deleted the feat/front-door branch December 13, 2022 23:28
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.

Restrict server to respond only to a list of IP addresses
3 participants