fix: custom resource upload should be namespaced by env by iamhopaul123 · Pull Request #3998 · aws/copilot-cli

iamhopaul123 · 2022-09-13T23:36:08Z

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

codecov-commenter · 2022-09-13T23:49:17Z

Codecov Report

Merging #3998 (a0a7e3b) into mainline (4b45d66) will increase coverage by 0.01%.
The diff coverage is 54.54%.

@@             Coverage Diff              @@
##           mainline    #3998      +/-   ##
============================================
+ Coverage     68.64%   68.65%   +0.01%     
============================================
  Files           248      248              
  Lines         35084    35067      -17     
  Branches        264      264              
============================================
- Hits          24083    24076       -7     
+ Misses         9807     9801       -6     
+ Partials       1194     1190       -4

Impacted Files	Coverage Δ
internal/pkg/aws/s3/s3.go	`93.20% <ø> (+6.64%)`	⬆️
internal/pkg/cli/deploy/env.go	`71.15% <0.00%> (ø)`
internal/pkg/cli/deploy/svc.go	`47.32% <ø> (+0.14%)`	⬆️
...pkg/deploy/upload/customresource/customresource.go	`83.51% <66.66%> (ø)`
...ternal/pkg/deploy/cloudformation/cloudformation.go	`72.47% <0.00%> (-0.55%)`	⬇️
internal/pkg/term/selector/selector.go	`78.25% <0.00%> (-0.06%)`	⬇️
internal/pkg/config/app.go	`92.39% <0.00%> (ø)`
internal/pkg/template/env.go	`45.90% <0.00%> (ø)`
internal/pkg/deploy/cloudformation/app.go	`74.49% <0.00%> (ø)`
internal/pkg/cli/env_init.go	`64.89% <0.00%> (+0.05%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

efekarakus · 2022-09-14T16:19:57Z

-// ZipAndUpload zips all files and uploads the zipped file to an S3 bucket under the specified key.
-// Per s3's recommendation https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html:
-// The bucket owner, in addition to the object owner, is granted full control.
-func (s *S3) ZipAndUpload(bucket, key string, files ...NamedBinary) (string, error) {


yay! thank you for deleting dead code

efekarakus · 2022-09-14T16:34:53Z


 		templateFS:       template.New(),
-		s3:               s3.New(envRegionSession),
+		s3:               s3.New(envManagerSession),


I actually don't get why this issue is happening if the object is uploaded with bucket ownership:

copilot-cli/internal/pkg/aws/s3/s3.go

Line 217 in 4b45d66

ACL: aws.String(s3.ObjectCannedACLBucketOwnerFullControl),

and the bucket allows all actions from the env accounts:

copilot-cli/internal/pkg/template/templates/app/cf.yml

Lines 74 to 82 in 4b45d66

- s3:*

Effect: Allow

Resource:

- !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}

- !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}/*

Principal:

AWS:

- !Sub arn:${AWS::Partition}:iam::${AWS::AccountId}:root{{range $accounts}}

- !Sub arn:${AWS::Partition}:iam::{{.}}:root{{end}}

Then why is there an access denied error? What am I missing? I'd have guessed that as long as the objects are uploaded with BucketOwnership then it wouldn't matter if they're overriden.

thanks for fixing the session here!

efekarakus · 2022-09-14T21:36:39Z

OK I finally get what is going on

The buckets are created by default with BucketOwnership set to ObjectWriter.

Everything gets fixed if we change the BucketOwnership to BucketOwnerEnforced or BucketOwnerPreferred in the app template (we don't have to touch any of our s3.go code).
In these scenarios, the access control to the data becomes based on the BucketPolicy instead of these confusing ACLs:

copilot-cli/internal/pkg/template/templates/app/cf.yml

Lines 71 to 82 in 35772b4

    
           Statement: 
        
             - 
        
               Action: 
        
                 - s3:* 
        
               Effect: Allow 
        
               Resource: 
        
                 - !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket} 
        
                 - !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}/* 
        
               Principal: 
        
                 AWS: 
        
                   - !Sub arn:${AWS::Partition}:iam::${AWS::AccountId}:root{{range $accounts}} 
        
                   - !Sub arn:${AWS::Partition}:iam::{{.}}:root{{end}}

My recommendation is instead of namespacing the custom resources, we should change the control ownership to be BucketOwnerEnforced and force the users that need the fix to run copilot app upgrade.

efekarakus

Should we update the TemplateVersion: 'v1.1.0' field in the app template and deploy.LatestAppTemplateVersion to v1.2.0 ?

efekarakus · 2022-09-14T22:41:52Z

 		f, err := w.Create(file.name)
 		if err != nil {
-			return fmt.Errorf("create zip file %q for custom resource %q: %v", file.name, cr.FunctionName(), err)
+			return fmt.Errorf("create zip file %q for custom resource %q: %v", file.name, cr.name, err)


nit: can we use .Name() instead to rely on the public interface of the type rather than the internal data structure

Should we update the TemplateVersion: 'v1.1.0' field in the app template and deploy.LatestAppTemplateVersion to v1.2.0 ?

No we don't because it is not released yet.

Lou1415926 · 2022-09-14T22:58:05Z

 	}
 }

-// ZipAndUpload zips all files and uploads the zipped file to an S3 bucket under the specified key.


Lou1415926 · 2022-09-14T22:58:45Z


 		templateFS:       template.New(),
-		s3:               s3.New(envRegionSession),
+		s3:               s3.New(envManagerSession),


thanks for fixing the session here!

Lou1415926 · 2022-09-14T22:59:27Z

 	if err != nil {
 		return nil, fmt.Errorf("create default: %w", err)
 	}
-	if err != nil {


iamhopaul123 added 2 commits September 13, 2022 16:24

fix: custom resource upload should be namespaced by env

d1b1316

Remove unused func

699e8a0

iamhopaul123 requested a review from a team as a code owner September 13, 2022 23:36

iamhopaul123 requested review from dannyrandall and removed request for a team September 13, 2022 23:36

amazon-ecs-cli-v2-pr-manager requested a review from efekarakus September 13, 2022 23:36

iamhopaul123 requested a review from Lou1415926 September 13, 2022 23:36

iamhopaul123 mentioned this pull request Sep 13, 2022

Pipeline failed after upgrade to 1.21 with "Your access has been denied by S3" error #3950

Closed

Fix static check

71f9d51

efekarakus reviewed Sep 14, 2022

View reviewed changes

Change object owner setting instead

f394d84

efekarakus reviewed Sep 14, 2022

View reviewed changes

Use Name()

a0a7e3b

iamhopaul123 force-pushed the acl-fix branch from b6e30df to a0a7e3b Compare September 14, 2022 23:03

Lou1415926 approved these changes Sep 14, 2022

View reviewed changes

efekarakus approved these changes Sep 14, 2022

View reviewed changes

Merge branch 'mainline' into acl-fix

2c34505

mergify Bot merged commit 40f2514 into aws:mainline Sep 14, 2022

dannyrandall mentioned this pull request Sep 27, 2022

Access Denied Deploying Job #4040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: custom resource upload should be namespaced by env#3998

fix: custom resource upload should be namespaced by env#3998
mergify[bot] merged 6 commits intoaws:mainlinefrom
iamhopaul123:acl-fix

iamhopaul123 commented Sep 13, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 13, 2022 •

edited

Loading

Uh oh!

efekarakus Sep 14, 2022

Uh oh!

efekarakus Sep 14, 2022

Uh oh!

Lou1415926 Sep 14, 2022

Uh oh!

efekarakus commented Sep 14, 2022 •

edited

Loading

Uh oh!

efekarakus left a comment

Uh oh!

efekarakus Sep 14, 2022

Uh oh!

iamhopaul123 Sep 14, 2022

Uh oh!

Lou1415926 Sep 14, 2022

Uh oh!

Lou1415926 Sep 14, 2022

Uh oh!

Lou1415926 Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	- s3:*
	Effect: Allow
	Resource:
	- !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}
	- !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}/*
	Principal:
	AWS:
	- !Sub arn:${AWS::Partition}:iam::${AWS::AccountId}:root{{range $accounts}}
	- !Sub arn:${AWS::Partition}:iam::{{.}}:root{{end}}

Conversation

iamhopaul123 commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iamhopaul123 commented Sep 13, 2022 •

edited

Loading

codecov-commenter commented Sep 13, 2022 •

edited

Loading

efekarakus commented Sep 14, 2022 •

edited

Loading