feat: add origin-shield #207

syphernl · 2022-01-11T11:43:26Z

what

Add variables to enable the Origin Shield for the CloudFront distribution

why

Using Origin Shield can help reduce the load on your origin.

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudfront_distribution#origin_shield
Also implemented recently in other CF module: Adding origin shield variables terraform-aws-cloudfront-cdn#78

bridgecrew

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

bridgecrew · 2022-01-11T11:43:45Z

main.tf

@@ -390,6 +390,15 @@ resource "aws_cloudfront_distribution" "default" {
        origin_ssl_protocols   = var.origin_ssl_protocols
      }
    }
+


Verify CloudFront Distribution Viewer Certificate is using TLS v1.2
Resource: aws_cloudfront_distribution.default | ID: BC_AWS_NETWORKING_63
Error in referred variable: locals "minimum_protocol_version" > variable "minimum_protocol_version"

How to Fix

resource "aws_cloudfront_distribution" "pass" { ... viewer_certificate { cloudfront_default_certificate = false minimum_protocol_version = "TLSv1.2_2018" } }

Description
This policy identifies AWS CloudFront web distributions which are configured with TLS versions for HTTPS communication between viewers and CloudFront. As a best practice, use TLSv1.1_2016 or later as the minimum protocol version in your CloudFront distribution security policies

bridgecrew · 2022-01-11T11:43:45Z

main.tf

@@ -390,6 +390,15 @@ resource "aws_cloudfront_distribution" "default" {
        origin_ssl_protocols   = var.origin_ssl_protocols
      }
    }
+


Ensure CloudFront distribution has a strict security headers policy attached
Resource: aws_cloudfront_distribution.default | ID: BC_AWS_NETWORKING_65

…s-cloudfront-s3-cdn into feat/origin_shield

syphernl · 2022-01-11T13:21:01Z

✅ Verified on my own stack that it works.

The automated tests however will likely only work once #208 has been merged since the AWS Provider is now explicitly checking for a value to be set for ssl_support_method.

mergify · 2022-01-12T19:18:43Z

This pull request is now in conflict. Could you fix it @syphernl? 🙏

korenyoni · 2022-01-12T19:20:41Z

/rebuild-readme

bridgecrew

⚠️ Due to b5b9975 - Merge branch 'master' into feat/origin_shield - 2 errors were fixed.

Change details

Error ID	Change	Path	Resource
BC_AWS_NETWORKING_63	Fixed	/main.tf	aws_cloudfront_distribution.default
BC_AWS_NETWORKING_65	Fixed	/main.tf	aws_cloudfront_distribution.default

korenyoni · 2022-01-12T19:33:28Z

/test all

korenyoni

In general this LGTM, with the exception that I think that being able to set region = "auto" and the map that comes alongside it is a little too "smart" for Terraform. This is more something that should happen on the provider side. I'd like someone else to weigh in. @osterman ?

It'd also be nice to get @Nuru 's opinion on the addition of an origin_shield attribute to s3_origins and custom_origins, which is a breaking change.

variables.tf

korenyoni · 2022-01-12T21:13:59Z

main.tf

+  # Origin Shield mapping configuration
+  # See: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/origin-shield.html
+  origin_shield_region_mapping = {
+    # Regions where Origin Shield is available
+    us-east-2      = "us-east-2"      # US East (Ohio)
+    us-east-1      = "us-east-1"      # US East (N. Virginia)
+    us-west-2      = "us-west-2"      # US West (Oregon)
+    ap-south-1     = "ap-south-1"     # Asia Pacific (Mumbai)
+    ap-northeast-2 = "ap-northeast-2" # Asia Pacific (Seoul)
+    ap-southeast-1 = "ap-southeast-1" # Asia Pacific (Singapore)
+    ap-southeast-2 = "ap-southeast-2" # Asia Pacific (Sydney)
+    ap-northeast-1 = "ap-northeast-1" # Asia Pacific (Tokyo)
+    eu-central-1   = "eu-central-1"   # Europe (Frankfurt)
+    eu-west-1      = "eu-west-1"      # Europe (Ireland)
+    eu-west-2      = "eu-west-2"      # Europe (London)
+    sa-east-1      = "sa-east-1"      # South America (São Paulo)
+
+    # Regions where Origin Shield is NOT available (choose closest region)
+    us-west-1    = "us-west-2"      # US West (N. California)
+    af-south-1   = "eu-west-1"      # Africa (Cape Town)
+    ap-east-1    = "ap-southeast-1" # Asia Pacific (Hong Kong)
+    ca-central-1 = "us-east-1"      # Canada (Central)
+    eu-south-1   = "eu-central-1"   # Europe (Milan)
+    eu-west-3    = "eu-west-2"      # Europe (Paris)
+    eu-north-1   = "eu-west-2"      # Europe (Stockholm)
+    me-south-1   = "ap-south-1"     # Middle East (Bahrain)
+  }


See my other comment on this type of behaviour not having much precedent in our modules.

No, we do similar things here: https://github.com/cloudposse/terraform-aws-utils/blob/master/main.tf

ironically, I see we are using a local for it too, but I think a var would be more appropriate in this case.

@osterman right, but I didn't think https://github.com/cloudposse/terraform-aws-utils/blob/master/main.tf of a proper example since it's a utility module specifically meant to map region short-hands and not a regular module. But anyways, if it's acceptable precedent in your view, then that helps.

korenyoni · 2022-01-12T21:19:30Z

variables.tf

+    origin_shield = object({
+      enabled = bool
+      region  = string
+    })


It sucks that we have to make such a complex object even more complex. It's a breaking change, but there really isn't any other logical place for it IMO.

If we didn't have to opt in it'd be cool (https://www.terraform.io/language/functions/defaults)

But it is what it is.

I agree. Let's hope the defaults function will soon move out of the experimental phase.

@syphernl how much of a blocker is this for you? We're trying to keep breaking changes to a minimum, and yes, desperate for defaults function to graduate from experimental.

@osterman Our customers would like to have this implemented in their environment.
For the time being we could probably use our own fork/branch instead although from update-perspective that is not ideal.

korenyoni · 2022-01-12T21:19:49Z

variables.tf

+    origin_shield = object({
+      enabled = bool
+      region  = string
+    })


See comment on breaking change

korenyoni · 2022-01-12T21:32:14Z

So @syphernl looks good, not requesting any changes but want to get @osterman and @Nuru's opinions on what I've identified.

syphernl · 2022-01-13T07:22:09Z

@korenyoni I have changed the local into a var as @osterman suggested.

korenyoni · 2022-01-13T15:39:52Z

Thanks @syphernl —

@osterman please take another look. Deferring approval to you.

korenyoni · 2022-01-13T15:39:57Z

/test all

korenyoni · 2022-02-10T19:02:40Z

Hi @syphernl —

I've spoken to @Nuru who needs more time to be able to get a good look at this.

syphernl · 2022-03-14T14:54:47Z

@korenyoni @Nuru Do you have any updates regarding this? Our project development is nearing completion and we'd like to enable the Origin Shield for this soon.

korenyoni · 2022-03-14T15:09:26Z

@korenyoni @Nuru Do you have any updates regarding this? Our project development is nearing completion and we'd like to enable the Origin Shield for this soon.

At the moment it's in @Nuru 's hands.

From @Nuru :

I had to put this on hold because of the AWS 4.0 upgrade.

alexjurkiewicz · 2022-10-24T00:24:58Z

I implemented the non-breaking-change portion of this in #247

feat: add origin-shield

2e1a687

bridgecrew bot reviewed Jan 11, 2022

View reviewed changes

syphernl and others added 5 commits January 11, 2022 13:56

add region lookup logic

ffa8ab1

Auto Format

60a81da

remove temporary workaround for sni

3e5f911

Merge branch 'feat/origin_shield' of github.com:syphernl/terraform-aw…

d3a2be1

…s-cloudfront-s3-cdn into feat/origin_shield

Auto Format

1975393

syphernl marked this pull request as ready for review January 11, 2022 13:25

syphernl requested review from a team as code owners January 11, 2022 13:25

syphernl requested review from dotCipher and max-lobur and removed request for a team January 11, 2022 13:25

Merge branch 'master' into feat/origin_shield

b5b9975

bridgecrew bot reviewed Jan 12, 2022

View reviewed changes

Auto Format

093ba4e

korenyoni reviewed Jan 12, 2022

View reviewed changes

korenyoni requested review from osterman and Nuru January 12, 2022 21:22

korenyoni added the needs-discussion label Jan 12, 2022

syphernl and others added 2 commits January 13, 2022 08:17

change to variable

ed20673

Auto Format

de738a5

syphernl requested review from korenyoni and removed request for a team January 13, 2022 07:21

korenyoni added the no-release Do not create a new release (wait for additional code changes) label Jan 13, 2022

osterman mentioned this pull request Jan 14, 2022

03-first-aws-environment component/terraform/static-site fails to deploy cloudposse-archives/tutorials#10

Closed

korenyoni and others added 2 commits January 27, 2022 20:43

Merge branch 'master' into feat/origin_shield

68d04df

Merge branch 'master' into feat/origin_shield

04d380b

Nuru added do-not-merge and removed no-release Do not create a new release (wait for additional code changes) labels Feb 18, 2022

alexjurkiewicz mentioned this pull request Oct 24, 2022

Add support for origin shield #247

Merged

hans-d closed this Mar 2, 2024

syphernl deleted the feat/origin_shield branch March 2, 2024 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add origin-shield #207

feat: add origin-shield #207

syphernl commented Jan 11, 2022

bridgecrew bot left a comment

bridgecrew bot Jan 11, 2022 •

edited

Loading

bridgecrew bot Jan 11, 2022 •

edited

Loading

syphernl commented Jan 11, 2022

mergify bot commented Jan 12, 2022

korenyoni commented Jan 12, 2022

bridgecrew bot left a comment

korenyoni commented Jan 12, 2022

korenyoni left a comment •

edited

Loading

korenyoni Jan 12, 2022

osterman Jan 12, 2022

osterman Jan 12, 2022

korenyoni Jan 12, 2022

korenyoni Jan 12, 2022

syphernl Jan 13, 2022

osterman Jan 13, 2022

syphernl Jan 20, 2022

korenyoni Jan 12, 2022

korenyoni commented Jan 12, 2022

syphernl commented Jan 13, 2022

korenyoni commented Jan 13, 2022

korenyoni commented Jan 13, 2022

korenyoni commented Feb 10, 2022

syphernl commented Mar 14, 2022

korenyoni commented Mar 14, 2022

alexjurkiewicz commented Oct 24, 2022

feat: add origin-shield #207

feat: add origin-shield #207

Conversation

syphernl commented Jan 11, 2022

what

why

references

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

How to Fix

Description

bridgecrew bot Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

syphernl commented Jan 11, 2022

mergify bot commented Jan 12, 2022

korenyoni commented Jan 12, 2022

bridgecrew bot left a comment

Choose a reason for hiding this comment

korenyoni commented Jan 12, 2022

korenyoni left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korenyoni commented Jan 12, 2022

syphernl commented Jan 13, 2022

korenyoni commented Jan 13, 2022

korenyoni commented Jan 13, 2022

korenyoni commented Feb 10, 2022

syphernl commented Mar 14, 2022

korenyoni commented Mar 14, 2022

alexjurkiewicz commented Oct 24, 2022

bridgecrew bot Jan 11, 2022 •

edited

Loading

bridgecrew bot Jan 11, 2022 •

edited

Loading

korenyoni left a comment •

edited

Loading