Allow Lambda in us-east-1 required for cloudfront lambda on edge#65
Allow Lambda in us-east-1 required for cloudfront lambda on edge#65
Conversation
|
/terratest |
There was a problem hiding this comment.
I think this is not the best approach.
The issues
To begin with, someone automatically updating to the latest version of this SCP would find that all of a sudden, Lambdas are allowed in all regions. That is not good: security weakened by an automatic upgrade.
Second, just because you need Lambdas in us-east-1 for CloudFront doesn't mean everyone wants to allow Lambdas in us-east-1. It needs to be a configurable option.
Third, whatever changes you make to RestrictToSpecifiedRegions you need to make similar changes to DenyRegions.
My recommendation
I think, instead, you should add a separate template option: cloudfront_enabled. When true:
acm:*kms:*lambda:*waf:*
are allowed in us-east-1 along with the other allowed_regions. This needs to default to false so as not to surprise anyone with relaxed restrictions. This also needs to be implemented as a separate condition in the existing SCP, not as a separate SCP.
For bonus points, omit the second condition (the exception for CloudFront) when cloudfront_enabled is false or when allowed_regions includes us-east-1.
|
/terratest |
|
/terratest |
|
/terratest |
|
/terratest |
…service-control-policies into lambda-on-edge * 'lambda-on-edge' of github.com:cloudposse/terraform-aws-service-control-policies: Update RestrictToSpecifiedRegions.yaml Update RestrictToSpecifiedRegions.yaml
|
/terratest |
|
/terratest |
1 similar comment
|
/terratest |
|
/terratest |
Nuru
left a comment
There was a problem hiding this comment.
We only need to make changes where the old rule would prevent deployment to us-east-1. I suggested specific changes for Cloudfront, but you should make the same changes for servicequotas.
| - sid: "DenyLambdaInRegions" | ||
| effect: "Deny" | ||
| actions: | ||
| - "lambda:*" | ||
| condition: | ||
| - test: "StringEqualsIgnoreCase" | ||
| variable: "aws:RequestedRegion" | ||
| # List of denied regions | ||
| values: | ||
| %{ for r in split(",", denied_regions) } | ||
| %{ if cloudfront_lambda_edge_enabled != "true" || trimspace(r) != "us-east-1" } | ||
| - ${trimspace(r)} | ||
| %{ endif } | ||
| %{ endfor } | ||
| resources: | ||
| - "*" |
There was a problem hiding this comment.
This entire statement is not needed unless cloudfront_lambda_edge_enabled == true and us-east-1 is one of several denied regions, and will cause drift and be useless otherwise.
| - sid: "DenyLambdaInRegions" | |
| effect: "Deny" | |
| actions: | |
| - "lambda:*" | |
| condition: | |
| - test: "StringEqualsIgnoreCase" | |
| variable: "aws:RequestedRegion" | |
| # List of denied regions | |
| values: | |
| %{ for r in split(",", denied_regions) } | |
| %{ if cloudfront_lambda_edge_enabled != "true" || trimspace(r) != "us-east-1" } | |
| - ${trimspace(r)} | |
| %{ endif } | |
| %{ endfor } | |
| resources: | |
| - "*" | |
| %{ if cloudfront_lambda_edge_enabled && strcontains(denied_regions, "us-east-1") && trimspace(denied_regions) != "us-east-1" } | |
| - sid: "DenyLambdaInRegions" | |
| effect: "Deny" | |
| actions: | |
| - "lambda:*" | |
| condition: | |
| - test: "StringEqualsIgnoreCase" | |
| variable: "aws:RequestedRegion" | |
| # List of denied regions | |
| values: | |
| %{~ for r in split(",", denied_regions) ~} | |
| %{~ if trimspace(r) != "us-east-1" ~} | |
| - ${trimspace(r)} | |
| %{~ endif ~} | |
| %{~ endfor ~} | |
| resources: | |
| - "*" | |
| %{ endif } |
| %{ if cloudfront_lambda_edge_enabled == "true" } | ||
| - "lambda:*" | ||
| %{ endif } |
There was a problem hiding this comment.
This is good: if cloudfront_lambda_edge_enabled != true (the default, right?), then the rule is not changed, and there will be no Terraform drift.
However, let's clean up the whitespace and remove the redundant condition
| %{ if cloudfront_lambda_edge_enabled == "true" } | |
| - "lambda:*" | |
| %{ endif } | |
| %{~ if cloudfront_lambda_edge_enabled && strcontains(denied_regions, "us-east-1") ~} | |
| - "lambda:*" | |
| %{~ endif ~} |
| %{ if servicequotas_enabled == "true" } | ||
| - "servicequotas:*" | ||
| %{ endif } |
There was a problem hiding this comment.
You need to handle servicequotas_enabled just like cloudfront_lambda_edge_enabled
| %{ if servicequotas_enabled == "true" } | ||
| - "servicequotas:*" | ||
| %{ endif } |
There was a problem hiding this comment.
Again, needs to be handled like cloudfront_lambda_edge_enabled
| - sid: "RestrictLambdaToSpecifiedRegions" | ||
| effect: "Deny" | ||
| actions: | ||
| - "lambda:*" | ||
| condition: | ||
| - test: "StringNotEqualsIgnoreCase" | ||
| variable: "aws:RequestedRegion" | ||
| # List of allowed regions | ||
| values: | ||
| # Us east-1 required for cloudfront lambda on edge | ||
| %{ if cloudfront_lambda_edge_enabled == "true" && !contains(split(",", allowed_regions), "us-east-1") } | ||
| - us-east-1 | ||
| %{ endif } | ||
| %{ for r in split(",", allowed_regions) } | ||
| - ${trimspace(r)} | ||
| %{ endfor } | ||
| resources: | ||
| - "*" |
There was a problem hiding this comment.
Same idea as with DenyLambdaInRegions
| - sid: "RestrictLambdaToSpecifiedRegions" | |
| effect: "Deny" | |
| actions: | |
| - "lambda:*" | |
| condition: | |
| - test: "StringNotEqualsIgnoreCase" | |
| variable: "aws:RequestedRegion" | |
| # List of allowed regions | |
| values: | |
| # Us east-1 required for cloudfront lambda on edge | |
| %{ if cloudfront_lambda_edge_enabled == "true" && !contains(split(",", allowed_regions), "us-east-1") } | |
| - us-east-1 | |
| %{ endif } | |
| %{ for r in split(",", allowed_regions) } | |
| - ${trimspace(r)} | |
| %{ endfor } | |
| resources: | |
| - "*" | |
| %{ if cloudfront_lambda_edge_enabled && !strcontains(allowed_regions, "us-east-1") } | |
| - sid: "RestrictLambdaToSpecifiedRegions" | |
| effect: "Deny" | |
| actions: | |
| - "lambda:*" | |
| condition: | |
| - test: "StringNotEqualsIgnoreCase" | |
| variable: "aws:RequestedRegion" | |
| # List of allowed regions | |
| values: | |
| # Us east-1 required for cloudfront lambda on edge | |
| - us-east-1 | |
| %{~ for r in split(",", allowed_regions) ~} | |
| - ${trimspace(r)} | |
| %{~ endfor ~} | |
| resources: | |
| - "*" | |
| %{ endif } |
| %{ if cloudfront_lambda_edge_enabled == "true" } | ||
| - "lambda:*" | ||
| %{ endif } |
There was a problem hiding this comment.
Same idea as with DenyLambdaInRegions
| %{ if cloudfront_lambda_edge_enabled == "true" } | |
| - "lambda:*" | |
| %{ endif } | |
| %{~ if cloudfront_lambda_edge_enabled && !strcontains(allowed_regions, "us-east-1") ~} | |
| - "lambda:*" | |
| %{~ endif ~} |
what
service quotaswhy
Migration v0 to v1
If
RestrictToSpecifiedRegionsandDenyRegionsservice control policies are in usecloudfront_lambda_edge_enabledparameter set tofalseto preserve back-compatible behaviorservicequotas_enabledparameter set tofalseto preserve back-compatible behaviorRestrictLambdaToSpecifiedRegionsorDenyLambdaInRegionspolicies to preserve back-compatible behaviorcloudfront_lambda_edge_enabledtotruereferences