fix: ensure env manager role has permission to upload

Makefile

@@ -223,6 +223,7 @@ gen-mocks: tools
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/cli/list/mocks/mock_list.go -source=./internal/pkg/cli/list/list.go
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/cli/deploy/mocks/mock_svc.go -source=./internal/pkg/cli/deploy/svc.go
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/cli/deploy/mocks/mock_env.go -source=./internal/pkg/cli/deploy/env.go
+	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/cli/deploy/patch/mocks/mock_env.go -source=./internal/pkg/cli/deploy/patch/env.go
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/initialize/mocks/mock_workload.go -source=./internal/pkg/initialize/workload.go
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/ecs/mocks/mock_ecs.go -source=./internal/pkg/ecs/ecs.go
 	${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/apprunner/mocks/mock_apprunner.go -source=./internal/pkg/apprunner/apprunner.go

internal/pkg/cli/deploy/env.go

@@ -14,13 +14,16 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/aws/partitions"
 	"github.com/aws/copilot-cli/internal/pkg/aws/s3"
 	"github.com/aws/copilot-cli/internal/pkg/aws/sessions"
+	"github.com/aws/copilot-cli/internal/pkg/cli/deploy/patch"
 	"github.com/aws/copilot-cli/internal/pkg/config"
 	"github.com/aws/copilot-cli/internal/pkg/deploy"
 	deploycfn "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation"
 	"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack"
 	"github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/term/log"
+	termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress"
 )
 
 type appResourcesGetter interface {
@@ -33,6 +36,10 @@ type environmentDeployer interface {
 	ForceUpdateOutputID(app, env string) (string, error)
 }
 
+type patcher interface {
+	EnsureManagerRoleIsAllowedToUpload(bucketName string) error
+}
+
 type prefixListGetter interface {
 	CloudFrontManagedPrefixListID() (string, error)
 }
@@ -48,6 +55,7 @@ type envDeployer struct {
 	// Dependencies to deploy an environment.
 	appCFN             appResourcesGetter
 	envDeployer        environmentDeployer
+	patcher            patcher
 	newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer
 
 	// Cached variables.
@@ -75,6 +83,7 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) {
 	if err != nil {
 		return nil, fmt.Errorf("get env session: %w", err)
 	}
+	cfnClient := deploycfn.New(envManagerSession, deploycfn.WithProgressTracker(os.Stderr))
 	return &envDeployer{
 		app: in.App,
 		env: in.Env,
@@ -84,7 +93,12 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) {
 		prefixListGetter: ec2.New(envRegionSession),
 
 		appCFN:      deploycfn.New(defaultSession, deploycfn.WithProgressTracker(os.Stderr)),
-		envDeployer: deploycfn.New(envManagerSession, deploycfn.WithProgressTracker(os.Stderr)),
+		envDeployer: cfnClient,
+		patcher: &patch.EnvironmentPatcher{
+			Prog:            termprogress.NewSpinner(log.DiagnosticWriter),
+			TemplatePatcher: cfnClient,
+			Env:             in.Env,
+		},
 		newStackSerializer: func(in *deploy.CreateEnvironmentInput, lastForceUpdateID string, oldParams []*awscfn.Parameter) stackSerializer {
 			return stack.NewEnvConfigFromExistingStack(in, lastForceUpdateID, oldParams)
 		},
@@ -97,6 +111,9 @@ func (d *envDeployer) UploadArtifacts() (map[string]string, error) {
 	if err != nil {
 		return nil, err
 	}
+	if err := d.patcher.EnsureManagerRoleIsAllowedToUpload(resources.S3Bucket); err != nil {
+		return nil, fmt.Errorf("ensure env manager role has permissions to upload: %w", err)
+	}
 	return d.uploadCustomResources(resources.S3Bucket)
 }
 

internal/pkg/cli/deploy/env_test.go

@@ -23,14 +23,14 @@ import (
 )
 
 type uploadArtifactsMock struct {
-	appCFN *mocks.MockappResourcesGetter
-	s3     *mocks.Mockuploader
+	appCFN  *mocks.MockappResourcesGetter
+	s3      *mocks.Mockuploader
+	patcher *mocks.Mockpatcher
 }
 
 func TestEnvDeployer_UploadArtifacts(t *testing.T) {
 	const (
-		mockManagerRoleARN = "mockManagerRoleARN"
-		mockEnvRegion      = "mockEnvRegion"
+		mockEnvRegion = "mockEnvRegion"
 	)
 	mockApp := &config.Application{}
 	testCases := map[string]struct {
@@ -50,11 +50,21 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) {
 			},
 			wantedError: fmt.Errorf("cannot find the S3 artifact bucket in region %s", mockEnvRegion),
 		},
+		"fail to patch the environment": {
+			setUpMocks: func(m *uploadArtifactsMock) {
+				m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{
+					S3Bucket: "mockS3Bucket",
+				}, nil)
+				m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(errors.New("some error"))
+			},
+			wantedError: errors.New("ensure env manager role has permissions to upload: some error"),
+		},
 		"fail to upload artifacts": {
 			setUpMocks: func(m *uploadArtifactsMock) {
 				m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{
 					S3Bucket: "mockS3Bucket",
 				}, nil)
+				m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(nil)
 				m.s3.EXPECT().Upload("mockS3Bucket", gomock.Any(), gomock.Any()).AnyTimes().Return("", fmt.Errorf("some error"))
 			},
 			wantedError: errors.New("upload custom resources to bucket mockS3Bucket"),
@@ -64,9 +74,9 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) {
 				m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{
 					S3Bucket: "mockS3Bucket",
 				}, nil)
+				m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(nil)
 				crs, err := customresource.Env(fakeTemplateFS())
 				require.NoError(t, err)
-
 				m.s3.EXPECT().Upload("mockS3Bucket", gomock.Any(), gomock.Any()).DoAndReturn(func(_, key string, _ io.Reader) (url string, err error) {
 					for _, cr := range crs {
 						if strings.Contains(key, strings.ToLower(cr.FunctionName())) {
@@ -92,19 +102,24 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) {
 			defer ctrl.Finish()
 
 			m := &uploadArtifactsMock{
-				appCFN: mocks.NewMockappResourcesGetter(ctrl),
-				s3:     mocks.NewMockuploader(ctrl),
+				appCFN:  mocks.NewMockappResourcesGetter(ctrl),
+				s3:      mocks.NewMockuploader(ctrl),
+				patcher: mocks.NewMockpatcher(ctrl),
 			}
 			tc.setUpMocks(m)
 
+			mockEnv := &config.Environment{
+				Name:           "mockEnv",
+				ManagerRoleARN: "mockManagerRoleARN",
+				Region:         mockEnvRegion,
+				App:            "mockApp",
+			}
 			d := envDeployer{
-				app: mockApp,
-				env: &config.Environment{
-					ManagerRoleARN: mockManagerRoleARN,
-					Region:         mockEnvRegion,
-				},
+				app:        mockApp,
+				env:        mockEnv,
 				appCFN:     m.appCFN,
 				s3:         m.s3,
+				patcher:    m.patcher,
 				templateFS: fakeTemplateFS(),
 			}
 

internal/pkg/cli/deploy/patch/env.go

@@ -0,0 +1,133 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package patch
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/aws/aws-sdk-go/aws/endpoints"
+	"github.com/aws/copilot-cli/internal/pkg/aws/cloudformation"
+	"github.com/aws/copilot-cli/internal/pkg/aws/s3"
+	"github.com/aws/copilot-cli/internal/pkg/config"
+	"github.com/aws/copilot-cli/internal/pkg/term/log"
+	"golang.org/x/mod/semver"
+	"gopkg.in/yaml.v3"
+)
+
+type environmentTemplateUpdateGetter interface {
+	EnvironmentTemplate(appName, envName string) (string, error)
+	UpdateEnvironmentTemplate(appName, envName, templateBody, cfnExecRoleARN string) error
+}
+
+type progress interface {
+	Start(label string)
+	Stop(label string)
+}
+
+// EnvironmentPatcher checks if the environment needs a patch and perform the patch when necessary.
+type EnvironmentPatcher struct {
+	Prog            progress
+	Env             *config.Environment
+	TemplatePatcher environmentTemplateUpdateGetter
+}
+
+// EnsureManagerRoleIsAllowedToUpload checks if the environment manager role has the necessary permissions to upload  
+// objects to bucket and patches the permissions if not.
+func (p *EnvironmentPatcher) EnsureManagerRoleIsAllowedToUpload(bucket string) error {
+	body, err := p.TemplatePatcher.EnvironmentTemplate(p.Env.App, p.Env.Name)
+	if err != nil {
+		return fmt.Errorf("get environment template for %q: %w", p.Env.Name, err)
+	}
+	ok, err := isManagerRoleAllowedToUpload(body)
+	if err != nil {
+		return err
+	}
+	if ok {
+		return nil
+	}
+	return p.grantManagerRolePermissionToUpload(p.Env.App, p.Env.Name, p.Env.ExecutionRoleARN, body, s3.FormatARN(endpoints.AwsPartitionID, bucket))
+}
+
+func (p *EnvironmentPatcher) grantManagerRolePermissionToUpload(app, env, execRole, body, bucketARN string) error {
+	// Detect which line number the EnvironmentManagerRole's PolicyDocument Statement is at.
+	// We will add additional permissions after that line.
+	type Template struct {
+		Resources struct {
+			ManagerRole struct {
+				Properties struct {
+					Policies []struct {
+						Document struct {
+							Statements yaml.Node `yaml:"Statement"`
+						} `yaml:"PolicyDocument"`
+					} `yaml:"Policies"`
+				} `yaml:"Properties"`
+			} `yaml:"EnvironmentManagerRole"`
+		} `yaml:"Resources"`
+	}
+
+	var tpl Template
+	if err := yaml.Unmarshal([]byte(body), &tpl); err != nil {
+		return fmt.Errorf("unmarshal environment template to find EnvironmentManagerRole policy statement: %v", err)
+	}
+	if len(tpl.Resources.ManagerRole.Properties.Policies) == 0 {
+		return errors.New("unable to find policies for the EnvironmentManagerRole")
+	}
+	// lines and columns are 1-indexed, so we have to subtract one from each.
+	statementLineIndex := tpl.Resources.ManagerRole.Properties.Policies[0].Document.Statements.Line - 1
+	numSpaces := tpl.Resources.ManagerRole.Properties.Policies[0].Document.Statements.Column - 1
+	pad := strings.Repeat(" ", numSpaces)
+
+	// Create the additional permissions needed with the appropriate indentation.
+	permissions := fmt.Sprintf(`- Sid: PatchPutObjectsToArtifactBucket
+  Effect: Allow
+  Action:
+    - s3:PutObject
+    - s3:PutObjectAcl
+  Resource:
+    - %s
+    - %s/*`, bucketARN, bucketARN)
+	permissions = pad + strings.Replace(permissions, "\n", "\n"+pad, -1)
+
+	// Add the new permissions to the body.
+	lines := strings.Split(body, "\n")
+	linesBefore := lines[:statementLineIndex]
+	linesAfter := lines[statementLineIndex:]
+	updatedLines := append(linesBefore, append(strings.Split(permissions, "\n"), linesAfter...)...)
+	updatedBody := strings.Join(updatedLines, "\n")
+
+	// Update the Environment template with the new content.
+	// CloudFormation is the only entity that's allowed to update the EnvManagerRole so we have to go through this route.
+	// See #3556.
+	var errEmptyChangeSet *cloudformation.ErrChangeSetEmpty
+	p.Prog.Start("Update the environment's manager role with permission to upload artifacts to S3")
+	err := p.TemplatePatcher.UpdateEnvironmentTemplate(app, env, updatedBody, execRole)
+	if err != nil && !errors.As(err, &errEmptyChangeSet) {
+		p.Prog.Stop(log.Serrorln("Unable to update the environment's manager role with upload artifacts permission"))
+		return fmt.Errorf("update environment template with PutObject permissions: %v", err)
+	}
+	p.Prog.Stop(log.Ssuccessln("Updated the environment's manager role with permissions to upload artifacts to S3"))
+	return nil
+}
+
+func isManagerRoleAllowedToUpload(body string) (bool, error) {
+	type Template struct {
+		Metadata struct {
+			Version string `yaml:"Version"`
+		} `yaml:"Metadata"`
+	}
+	var tpl Template
+	if err := yaml.Unmarshal([]byte(body), &tpl); err != nil {
+		return false, fmt.Errorf("unmarshal environment template to detect Metadata.Version: %v", err)
+	}
+	if !semver.IsValid(tpl.Metadata.Version) { // The template doesn't contain a version.
+		return false, nil
+	}
+	if semver.Compare(tpl.Metadata.Version, "v1.9.0") < 0 {
+		// The permissions to grant the EnvManagerRole to upload artifacts was granted with template v1.9.0.
+		return false, nil
+	}
+	return true, nil
+}