-
-
Notifications
You must be signed in to change notification settings - Fork 26
Allow Lambda in us-east-1 required for cloudfront lambda on edge #65
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
base: main
Are you sure you want to change the base?
Conversation
/terratest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 quotas
why
Migration v0 to v1
If
RestrictToSpecifiedRegions
andDenyRegions
service control policies are in usecloudfront_lambda_edge_enabled
parameter set tofalse
to preserve back-compatible behaviorservicequotas_enabled
parameter set tofalse
to preserve back-compatible behaviorRestrictLambdaToSpecifiedRegions
orDenyLambdaInRegions
policies to preserve back-compatible behaviorcloudfront_lambda_edge_enabled
totrue
references