fix(task): grant task-run execution role access to secrets based on tag #3256

paragbhingre · 2022-02-08T19:45:02Z

This PR fixes the issue #2641

Fixed the issue by adding policies to the execution role when --secrets are provided.

Tests -
Following manual tests have been taken into consideration

When we send both --app and --env flags in copilot task run command then secrets are accessible only when copilot-application and copilot-environment tags are present with the correct values on the secret
When we send both --app and --env flags in copilot task run command then secrets are inaccessible only when copilot-application and copilot-environment tags are NOT present on the secret or they have incorrect values

internal/pkg/template/templates/task/cf.yml

efekarakus · 2022-02-09T18:41:42Z

internal/pkg/template/templates/task/cf.yml

+                    "ssm:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
+                    "ssm:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]
+                    "ssm:ResourceTag/copilot-task": !If [ HasAppAndEnv, !Ref "AWS::NoValue", !Ref TaskName ]


I really like the changes with allowing access to the copilot-application and copilot-environment tags -- makes total sense to me. I am not sure though how obvious it's going to be when not using a copilot managed cluster, requiring the copilot-task tag for a one-time job seems a bit cumbersome.

What if we had an experience like this?

$ copilot task run --default --secrets hello=/hello,world=arn:aws:secretsmanager:us-west-2:111122223333:secret:aes128-1a2b3c Looks like you're requesting ssm:GetParameters to the following SSM parameters: * /hello and secretsmanager:GetSecretValue to the following Secrets Manager secrets: * arn:aws:secretsmanager:us-west-2:111122223333:secret:aes128-1a2b3c Do you grant permission to the ECS/Fargate agent for these secrets? (y/N)

Or if they'd like to skip the prompt they can pass a flag similar to what the CDK does with --require-approval never

$ copilot task run --default --secrets hello=/hello --require-approval never

Then we will augment the Task Execution role such that only the input resources are granted access.

Let me know what you think!

Do you grant permission to the ECS/Fargate agent for these secrets? (y/N)

If they choose "N", do we just error out saying they can't use these secrets without granting access? And what are available options for --require-approval?

If they choose "N", do we just error out saying they can't use these secrets without granting access?

I assume we'll abort then, but we can look to what the CDK does as well.

And what are available options for --require-approval?

hmm good point for us none, maybe we can flip the flag --acknowledge-iam

efekarakus

Looks good to me! Just to validate we tested it with both secrets (ssm and secretsmanager) and it works right? 😂

Similarly, can we test as well with a secret that's not tagged to make sure it still cannot access it

paragbhingre · 2022-02-09T23:45:42Z

Looks good to me! Just to validate we tested it with both secrets (ssm and secretsmanager) and it works right? 😂

Similarly, can we test as well with a secret that's not tagged to make sure it still cannot access it

Both the secrets (ssm and secretsmanager) and scenarios you mentioned are tested ✅

iamhopaul123 · 2022-02-09T23:49:32Z

internal/pkg/template/templates/task/cf.yml

+  HasApp:
+    !Not [!Equals [!Ref App, ""]]
+  HasEnv:
+    !Not [!Equals [!Ref Env, ""]]


Do we really need hasapp and has env?

Actually we can remove this and put these conditions directly into HasAppAndEnv condition.

iamhopaul123 · 2022-02-09T23:51:18Z

internal/pkg/template/templates/task/cf.yml

+                  Condition:
+                    StringEquals:
+                      "ssm:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
+                      "ssm:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]
+                  Resource:
+                    - !Sub arn:${AWS::Partition}:ssm:${AWS::Region}:${AWS::AccountId}:parameter/*
+                - Effect: 'Allow'
+                  Action:
+                    - secretsmanager:GetSecretValue
+                  Condition:
+                    StringEquals:
+                      "secretsmanager:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
+                      "secretsmanager:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]
+                  Resource:
+                    - !Sub arn:${AWS::Partition}:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:*
+          - !Ref AWS::NoValue
+      {{- end}}


This is awesome!

Lou1415926 · 2022-02-10T18:09:13Z

internal/pkg/template/templates/task/cf.yml

+                      "ssm:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
+                      "ssm:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]


Can we remove these !If conditionals now that we have line 101-102?

Lou1415926 · 2022-02-10T18:09:31Z

internal/pkg/template/templates/task/cf.yml

+                      "secretsmanager:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
+                      "secretsmanager:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]


paragbhingre added 2 commits February 8, 2022 10:50

fix(task): copilot task run with --secrets

4de175d

fix unit test for TestTaskStackConfig_Parameters

9838d8b

paragbhingre requested a review from a team as a code owner February 8, 2022 19:45

paragbhingre requested review from efekarakus and removed request for a team February 8, 2022 19:45

amazon-ecs-cli-v2-pr-manager requested review from huanjani and removed request for a team February 8, 2022 19:45

paragbhingre changed the title ~~Task run secrets~~ fix(task): copilot task run with --secrets Feb 8, 2022

Lou1415926 changed the title ~~fix(task): copilot task run with --secrets~~ fix(task): grant task-run execution role access to secrets based on tag Feb 8, 2022

Lou1415926 reviewed Feb 8, 2022

View reviewed changes

internal/pkg/template/templates/task/cf.yml Outdated Show resolved Hide resolved

efekarakus reviewed Feb 9, 2022

View reviewed changes

Address feedback to allow access to secretsmanager secrets

692b9a5

efekarakus approved these changes Feb 9, 2022

View reviewed changes

iamhopaul123 reviewed Feb 9, 2022

View reviewed changes

remove extra conditions

11132ee

Lou1415926 reviewed Feb 10, 2022

View reviewed changes

Remove extra conditions

94ee963

Lou1415926 approved these changes Feb 14, 2022

View reviewed changes

Merge branch 'mainline' into taskRunSecrets

eb032de

mergify bot merged commit dc74b80 into aws:mainline Feb 14, 2022

hirokihokari mentioned this pull request Apr 12, 2022

--generate-cmd secretsmanager YAML not well-formed #3458

Closed

paragbhingre deleted the taskRunSecrets branch January 26, 2023 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(task): grant task-run execution role access to secrets based on tag #3256

fix(task): grant task-run execution role access to secrets based on tag #3256

paragbhingre commented Feb 8, 2022 •

edited

Loading

efekarakus Feb 9, 2022 •

edited

Loading

iamhopaul123 Feb 9, 2022

efekarakus Feb 9, 2022

efekarakus left a comment

paragbhingre commented Feb 9, 2022

iamhopaul123 Feb 9, 2022

paragbhingre Feb 10, 2022

iamhopaul123 Feb 9, 2022

Lou1415926 Feb 10, 2022 •

edited

Loading

Lou1415926 Feb 10, 2022

		"ssm:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
		"ssm:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]

		"secretsmanager:ResourceTag/copilot-application": !If [ HasAppAndEnv, !Ref App, !Ref "AWS::NoValue" ]
		"secretsmanager:ResourceTag/copilot-environment": !If [ HasAppAndEnv, !Ref Env, !Ref "AWS::NoValue" ]

fix(task): grant task-run execution role access to secrets based on tag #3256

fix(task): grant task-run execution role access to secrets based on tag #3256

Conversation

paragbhingre commented Feb 8, 2022 • edited Loading

efekarakus Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

iamhopaul123 Feb 9, 2022

Choose a reason for hiding this comment

efekarakus Feb 9, 2022

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

paragbhingre commented Feb 9, 2022

iamhopaul123 Feb 9, 2022

Choose a reason for hiding this comment

paragbhingre Feb 10, 2022

Choose a reason for hiding this comment

iamhopaul123 Feb 9, 2022

Choose a reason for hiding this comment

Lou1415926 Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Lou1415926 Feb 10, 2022

Choose a reason for hiding this comment

paragbhingre commented Feb 8, 2022 •

edited

Loading

efekarakus Feb 9, 2022 •

edited

Loading

Lou1415926 Feb 10, 2022 •

edited

Loading