fix: KMS access denied errors when downloading the static env file

[bencehornak]

Bug description

Since Copilot 1.32.0 cross account deployments are broken for all services (except static website) due to lacking permissions on the task execution role to perform the kms:Decrypt operation on the required key.

Background

Since #5329 the artifacts are encrypted with a Kms Key, which belongs to the App stack.

Due to the wrong configuration of the ExecutionRole resource if one deploys the workload to a different account than where the app stack is located, the download of the env file fails due to the lacking kms:Decrypt permission on the corresponding key.

https://github.com/bencehornak/copilot-cli/blob/67db7a2d4f94c69c836586f714341e587b1b8220/internal/pkg/template/templates/workloads/partials/cf/executionrole.yml#L39-L43

Note that this policy grants access to all keys in the workload's account (which is not a great practice either, see #4628), which is wrong, considering that the key is located in the app's account.

Fix

The policy was changed to the following:

- Effect: 'Allow'
  Action:
    - 'kms:Decrypt'
  Resource:
    - !Ref ArtifactKeyARN

where ArtifactKeyARN is the ARN of the Kms Key.

TODO

• There are many failing test cases, because the generated YAML changes. Is there a way to efficiently fix all of these test cases?
• Would it make sense to add a test case, which prevents this bug in the feature? If yes, how? -> changed some test cases to test if the KMS key ARN gets passed to the stacks

Conclusion

Fixes partially: #4628.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

[bencehornak-gls]

This workaround worked for fixing deployments for a Backend Service with Copilot 1.32.0:

- op: replace
  path: /Resources/ExecutionRole/Properties/Policies/0/PolicyDocument/Statement/2/Resource/0
  value: arn:aws:kms:....:key/... # the KMS key's ARN, which you see exported on the app's CloudFormation stack

[github-actions]

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	56796	55192	🥺 +2.91
macOS (arm)	57800	56144	🥺 +2.95
linux (amd)	49804	48440	🥺 +2.82
linux (arm)	49088	47744	🥺 +2.82
windows (amd)	46928	45672	🥺 +2.75

[bencehornak]

This line might also be buggy

copilot-cli/internal/pkg/template/templates/workloads/partials/cf/instancerole.yml

Line 60 in cce61ba

- !Sub 'arn:${AWS::Partition}:kms:${AWS::Region}:${AWS::AccountId}:key/*'

UPDATE: yes, it was, fixed it in a later commit.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.05%. Comparing base (d42cf47) to head (3a0330c).
⚠️ Report is 160 commits behind head on mainline.

Files with missing lines	Patch %	Lines
internal/pkg/cli/deploy/backend.go	0.00%	1 Missing ⚠️
internal/pkg/cli/deploy/job.go	0.00%	1 Missing ⚠️
internal/pkg/cli/deploy/lbws.go	0.00%	1 Missing ⚠️
internal/pkg/cli/deploy/rdws.go	0.00%	1 Missing ⚠️
internal/pkg/cli/deploy/worker.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           mainline    #5514   +/-   ##
=========================================
  Coverage     70.04%   70.05%           
=========================================
  Files           302      302           
  Lines         46331    46351   +20     
  Branches        309      309           
=========================================
+ Hits          32453    32469   +16     
- Misses        12300    12304    +4     
  Partials       1578     1578           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[iamhopaul123]

Awesome! Thank you for fixing this bug!

[iamhopaul123]

Can we in at least one integ test verifying ArtifactKeyARN can be rendered correctly with the app stackset resource info?

[iamhopaul123]

It would be great if you can fix another wrong reference for the KMS key here for the static site. We can have the same ArtifactKeyARN param for consistency and address #5472 🙏

[bencehornak]

Sure, will address your points tomorrow, @iamhopaul123

[bencehornak]

@iamhopaul123 I believe I have resolved both of your remarks, see my last 2 commits

[iamhopaul123]

@iamhopaul123 I believe I have resolved both of your remarks, see my last 2 commits

Looks good! Thank you!

[CaptainCarpensir]

Changes look good to me! Thanks so much for contributing ❤️