Grant GitHub Actions AWS IAM Permissions to Provision Marketing Site Stacks#65584
Conversation
cat5inthecradle
left a comment
There was a problem hiding this comment.
LGTM with some naming nitpicks.
If we get the ARN's wrong in this policy, this template will still deploy correctly, as they aren't validated. When we go to deploy the marketing stack, we will get an error if we're missing permisions, and we'll have to come back here to fix it.
There aren't really any good solutions that don't get us into weird chicken/egg bootstrapping. Just acknowledging that we may have to come back and tweak this policy when we go to use these permissions for the first time.
unlox775-code-dot-org
left a comment
There was a problem hiding this comment.
I have serious concerns about storing access tokens for any user with this much access (full CloudFormation blank check) in AWS Secrets manager. All it takes is for any new policy to mention secrets with an insecure pattern and they have a very large access profile by stealing these creds.
This PR creates a user with tightly scoped Cloudformation permissions. For example, And to clarify - these credentials would not be stored in AWS Secrets Manager, they would be stored in GitHub for use by the continuous deployment Github Actions workflows. That doesn't eliminate risk, but I want to make sure we're all on the same page here as we evaluate risk. |
The original proposal was to generate an API key, store it and its secret in an AWS Secret, and then an engineer would manually copy those values over to a GitHub Environment secret ( I agree that there’s a risk with creating the API Key and placing it in an AWS Secret, so I’m removing that from this template. An Infrastructure engineer would need to manually create the API Key and copy it to the GitHub Environment secret. All other Engineers do not have permission to create the API key. |
Doesn't that just constrain the name of the stack? The DevPermissions is a little more restrictive. What would prevent me from adding to the cloudformation template things like:
I think if we made a much more restrictive Permission Bounday (I am working on it as part of INF-1618), then we could find a way that granting CloudFormation access isn't extremely high risk. But permission boundaries that encompass such a wide range of activities that our stack uses, is by nature VERY complex, and complexity is the enemy of security. Which is why I lean in general against CF access |
These are the only Actions (and Resources) that the GitHub Deployer User will be able to execute: code-dot-org/aws/cloudformation/iam.yml.erb Lines 871 to 919 in a45253c Changes to this IAM Stack can only be deployed by an Infrastructure Engineer. |
| Action: | ||
| - iam:GetRole | ||
| - iam:PassRole | ||
| Resource: marketing-sites-<%=environment_type%>-*" |
There was a problem hiding this comment.
Where is this role defined: marketing-sites-<%=environment_type%>-*"? I don't see it in The CloudFormation template here. Create Stack operation (which uses iam:PassRole) executes as this role, and not the permissions of the current user. I.e. a user bob-tiny with no access but CloudFormation CreateStack (with resource of * role or just Administrator), would allow them to run the stack create with Adminstrator (full access), regardless of bob-tiny's access restrictions.
There was a problem hiding this comment.
I'm guessing this is for passing the ECS Task Execution role (docs). In which case, we should still make this much more specific with conditions.
Looks like neither the Task Execution Role nor the Task Role are named, so these magic strings may not yet be correct.
- Effect: Allow
Action: iam:PassRole
Resource:
- arn:aws:iam::123456789012:role/ecsTaskExecutionRole
- arn:aws:iam::123456789012:role/yourTaskRole
Condition:
StringEqualsIfExists:
iam:PassedToService: ecs-tasks.amazonaws.com| # Deployer should have permission to all Buckets with the naming pattern `org.dev-code*static-assets` | ||
| - "<%=configuration[:base_domain_name].split('.').tap(&:reverse!).join('.')%>*static-assets" | ||
| - "<%=configuration[:base_domain_name].split('.').tap(&:reverse!).join('.')%>*static-assets/*" | ||
| <% end %> |
There was a problem hiding this comment.
To note some more permissions to add for Cloudfront invalidation and autoscaling features:
- Effect: Allow
Action:
- 'iam:GetRole'
Resource: !GetAtt ECSAutoScalingRole.Arn
- Effect: Allow
Action:
- "cloudfront:CreateInvalidation"
Resource: !Sub 'arn:aws:cloudfront::${AWS::AccountId}:distribution/${CloudFrontDistribution}'
| # permissions needed to create/update/delete a marketing sites CloudFormation Stack. | ||
| # ------------------------------------------------------------ | ||
| <% | ||
| marketing_sites_environment_type_configuration = { |
There was a problem hiding this comment.
Further down the PR, I see you differentiate sites/clusters by indicating that the environment_type is the suffix of the name which flattens the structure
One suggestion is to go with a multi-dimensional configuration with "site" (or "cluster") being one of the dimensions. Although I think your flattened config might make more sense to start with and is less restrictive.
marketing_sites_environment_type_configuration = {
"hoc": { // This one would not be here for this PR, but eventually added
dev: {}, test: {}, prod: {}
},
"cdo": {
dev: {}, test: {}, prod: {}
}
}
There was a problem hiding this comment.
Yup. This initial approach proposes one Role per environment type (development, test, production), and there's a risk that a a GitHub Action that builds the test corporate site would maliciously or accidentally modify/delete/create resources for the test Hour of Code site. For now, I think the simplicity of provisioning 3 Roles outweighs having 9 or more Roles for the matrix of environment type and the type of marketing website.
| # The Stack Name must match this naming prefix convention. Examples: | ||
| # marketing-sites-production-code-org | ||
| # marketing-sites-production-hourofcode-com | ||
| # marketing-sites-test-teachai-org |
There was a problem hiding this comment.
This is not related to your PR, but I think a "low traffic" multi-tenant stack may more sense than having one for each site from a cost perspective. Especially since HA enables handling peak Code.org levels of traffic by default.
There was a problem hiding this comment.
With our current 1 vCPU per Availability Zone X 3 Zones per Stack, I think our costs will be relatively low and we won't need to modify the design to support a single Stack that operates multiple websites. Definitely open to considering that as we monitor costs of the first website.
…nd grant permissions to provision development, test, and production marketing sites Stacks.
…s Stacks can provision.
| - Sid: AllowCloudfrontInvalidation | ||
| Effect: Allow | ||
| Action: | ||
| - cloudfront:CreateInvalidation | ||
| Resource: '*' | ||
| Condition: | ||
| StringEquals: | ||
| aws:ResourceTag/environment-type: <%=environment_type%> |
There was a problem hiding this comment.
cloudfront:CreateInvalidation can be scoped down to specific distributions via Resource. Looks like you're using tagging instead? I'm curious why.
There was a problem hiding this comment.
The Distribution ARN doesn't have any human readable identifiers. Just that randomly generated Distribution ID.
There was a problem hiding this comment.
ah, makes sense. I wonder if we should go even farther with standardized tagging?
… to specific domain names.
|
deployed latest commit: |
|
Deployed latest commit: |
| # Add more statements here as we need the Roles provisioned within a marketing sites Stack to have more permissions. | ||
|
|
There was a problem hiding this comment.
Are there still any remaining statements to be added here? If we have a working stack, does that mean we have added all we need to? If not, could this comment be removed?
| # Add more statements here as we need the Roles provisioned within a marketing sites Stack to have more permissions. |
Co-authored-by: Dave2 Buchanan <146779710+unlox775-code-dot-org@users.noreply.github.com>
|
Deployed most recent commit: |
…ible." This reverts commit 7af1ce5.
|
reverted and deployed the revert of the tag-based restrictions as they don't work for Actions that Create Resources because when using Stack tag propagation, the tag doesn't exist yet on the Resource when CloudFormation tries to create it. |
|
Deployed latest commit: |
|
Deployed latest commit: |
|
Deployed last 2 commits: |
|
Deployed latest commit: |
|
Deployed latest 2 commits: |
|
Deployed latest commit: |
https://codedotorg.atlassian.net/browse/CMS-572
https://codedotorg.atlassian.net/browse/INF-1652
Testing story
Deployment strategy
Follow-up work
Updating the Stack that provisions marketing sites to namespace or tag all Resources with
marketing-sitesto identify the type of application (Stack) and the environment type (development,test,production) so that GitHub Actions has permissions (depending on the GitHub Environment) to provision specific Resources.Privacy
Security
Caching
PR Checklist: