feat: Allow assume role chaining with federated users (source identity and session tags) #5359

FlorianSW · 2023-10-09T07:59:48Z

Depending on the original credentials used to invoke the copilot cli command, the EnvManagerRole can only be assumed when it allows the original source identity and transitive tags to be passed to the session.

These permissions should not be of harm when the user's session does not have a source identity or transitive tags.

Fixes #5358

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

…session tags) Depending on the original credentials used to invoke the copilot cli command, the EnvManagerRole can only be assumed when it allows the original source identity and transitive tags to be passed to the session. These permissions should not be of harm when the user's session does not have a source identity or transitive tags.

iamhopaul123

The change looks good to me overall! Thank you for contributing! Have three general feedback. Can you

update the title/commit with https://conventionalcomments.org/ (this should be a feature)
test the code locally to make sure you would be able to assume env manager role with federated users
make this feature configurable and enabled by default. It would be nice if you can add a flag to env init for this

iamhopaul123 · 2023-10-11T21:52:27Z

internal/pkg/template/templates/environment/partials/environment-manager-role.yml

+ Action:
+ - sts:AssumeRole
+ - sts:SetSourceIdentity
+ - stsTagSession


Suggested change

- stsTagSession

- sts: TagSession

FlorianSW · 2023-10-23T10:39:11Z

Sorry for not coming back to you any sooner :( Thanks for your comments!

update the title/commit with https://conventionalcomments.org/ (this should be a feature)

Sure thing :)

test the code locally to make sure you would be able to assume env manager role with federated users

Will do that with the addition of 3.

make this feature configurable and enabled by default. It would be nice if you can add a flag to env init for this

Sounds like a good idea. I would probably go in the following way: Have an option in the environment configuration. When initing an environment, there is a flag to enable adding the necessary permissions. In that way, the default (e.g. for existing environments) can stay the same, hence not adding these additional permissions.
Would it be good to have a config option to add additional permissions to the trust policy, or having a switch which makes copilot add these additional permissions without an option for the user to adjust which permissions? I think the first one would be the most dynamic way.

github-actions · 2023-10-25T11:56:24Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	52212	51976	+0.45
macOS (arm)	53068	52812	+0.48
linux (amd)	45928	45716	+0.46
linux (arm)	45252	45060	+0.43
windows (amd)	43372	43168	+0.47

FlorianSW · 2023-11-01T15:17:21Z

@iamhopaul123 Sorry for all the noise here with the single commits 🙈
I think I got your input right and made some changes. I was a bit unsure about some details, though, and hope for your input:

Naming of the flag for copilot env init: The flag is currently named --federated-session, as I didn't find a short name which describes this better. However, tbh, I'm not very happy with it (as this is a specific type of federated sessions only).
The config in the env manifest allows a user to specify a list of permissions that should be added to the trust policy. I think this opens up the most flexibility that a user can customise which permissions are actually needed. Do you agree, or should it be more like a boolean flag?
the default value for the additional assume role permissions is false most of the times (when creating a new env for an existing app, or reading an existing env manifest without the config set, yet). In my opinion this should be the best approach right now: Existing setups will not change with this new version. New environments have the least privileges assigned (without the sts:SetSourceIdentity and sts:TagSession permission) by default. What do you mean? :)
When creating a new app (copilot init), the default for --federated-session is true (which would add the additional permissions when a new env is created as part of the init process, which is opt-in). I wasn't quite sure about that one, tbh. Alternatives might be: Adding a --federated-session flag to the init command as well, or using false as a default and let the user not create an environment during copilot init and instead use copilot env init afterwards (with the federated-session flag set)

Thanks for all your time reviewing this and investing the time to discuss this topic with me :)

codecov-commenter · 2023-11-01T15:17:49Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (9c9cf3b) 69.91% compared to head (bfec751) 69.92%.

Additional details and impacted files

@@            Coverage Diff            @@
##           mainline    #5359   +/-   ##
=========================================
  Coverage     69.91%   69.92%           
=========================================
  Files           299      299           
  Lines         45484    45508   +24     
  Branches        295      295           
=========================================
+ Hits          31799    31820   +21     
- Misses        12140    12143    +3     
  Partials       1545     1545

Files	Coverage Δ
internal/pkg/cli/deploy/env.go	`73.80% <100.00%> (+0.40%)`	⬆️
internal/pkg/cli/flag.go	`90.47% <ø> (ø)`
internal/pkg/deploy/cloudformation/stack/env.go	`79.15% <100.00%> (+0.10%)`	⬆️
internal/pkg/manifest/env.go	`77.28% <100.00%> (+0.51%)`	⬆️
internal/pkg/template/env.go	`52.63% <ø> (ø)`
internal/pkg/cli/init.go	`23.07% <0.00%> (-0.05%)`	⬇️
internal/pkg/config/env.go	`87.35% <0.00%> (ø)`
internal/pkg/cli/env_init.go	`65.49% <88.88%> (+0.15%)`	⬆️

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

iamhopaul123

Thank you so much for addressing my previous feedback. Left some suggestion regarding the scope of this feature but overall logic looks good to me!

iamhopaul123 · 2023-11-01T16:25:36Z

internal/pkg/cli/init.go

@@ -485,6 +485,7 @@ func (o *initOpts) deployEnv() error {
 // Set the application name from app init to the env init command, and check whether a flag has been passed for envName.
 initEnvCmd.appName = *o.appName
 initEnvCmd.name = o.initVars.envName
+ initEnvCmd.federatedSession = true


I think we should probably keep it consistent with the default value in env init which is false. I would assume relatively few users would need to set this and they could use env init instead of init.

iamhopaul123 · 2023-11-01T16:41:08Z

internal/pkg/template/templates/environment/manifest.yml

+# - sts:SetSourceIdentity
+# - sts:TagSession
+{{- else}}
+additionalAssumeRolePermissions:


This field name seems a little too specific. We usually have some level of abstraction for our manifest field because otherwise the experience will be almost the same as yaml override.

I would prefer not to add any additional field to the env manifest for now and only keep it as a flag at env init. Ideally we wouldn't need that flag either if we have a mechanism allowing users to customize their env bootstrap stack.

Ideally we wouldn't need that flag either if we have a mechanism allowing users to customize their env bootstrap stack.

With that in mind, I would agree that it would probably make a lot more sense to allow overrides for the bootstrap stack as well. Having a flag, which would change the stack only for one time wouldn't make much sense, as the changes would be overridden once the first env deployment is started.

Let me look into what would be needed to allow env overrides to kick in when bootstrapping an environment 👍

I think it would take much more efforts for allowing env bootstrap override, so feel free to start from a flag if you think it's too overwhelming!

Sorry for not coming back to you any sooner :(
I looked into the override stuff and from the concept point of view, I'm a bit stuck I think. YamlPatch would, iirc, error out, if there is an add/replace/remove statement for a yaml path, which does not exist. That means, that we couldn't easily re-use the existing overrides for environments, but would instead need to have an additional overrides location for the env bootstrap stack.
From my point of view this doesn't sound that nice at all, given that the overrides would only be used once during bootstrap.
This issue does not exist in CDK, at least the user could workaround that by checking if a resource actually exists before doing things in CDK code.

Based on that, I would think that having a command line option only is probably the best way to go forward. Would you agree with this? 🤔

Hello @FlorianSW. Sorry for getting back late. I was on vacation before.

I would think that having a command line option only is probably the best way to go forward. Would you agree with this? 🤔

I would agree if we are only adding an extra optional flag to the env init.

FlorianSW requested a review from a team as a code owner October 9, 2023 07:59

FlorianSW requested review from dannyrandall and removed request for a team October 9, 2023 07:59

amazon-ecs-cli-v2-pr-manager requested a review from efekarakus October 9, 2023 07:59

amazon-ecs-cli-v2-pr-manager added this to In review in Sprint 🏃‍♀️ Oct 9, 2023

iamhopaul123 reviewed Oct 11, 2023

View reviewed changes

FlorianSW added 2 commits October 24, 2023 13:06

Merge branch 'mainline' of github.com:aws/copilot-cli into issues/5358

6549671

Make additional assume role permissions in envmanager role configurable

3075aa7

FlorianSW changed the title ~~Allow assume role chaining with federated users (source identity and session tags)~~ feat: Allow assume role chaining with federated users (source identity and session tags) Oct 25, 2023

FlorianSW added 6 commits October 26, 2023 11:00

missing integration tests

01ec03b

fix codestyle :)

8a77a4c

adjust another integration test for new default env manifest yml

179975b

Merge branch 'mainline' of github.com:aws/copilot-cli into issues/5358

dd08f9d

fix typo

af271ee

add new permission from merging remote mainline

bfec751

iamhopaul123 reviewed Nov 1, 2023

View reviewed changes

Lou1415926 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Nov 9, 2023

iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Dec 13, 2023

iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 17, 2024

KollaAdithya added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 30, 2024

KollaAdithya added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 21, 2024

KollaAdithya removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 21, 2024

huanjani added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow assume role chaining with federated users (source identity and session tags) #5359

feat: Allow assume role chaining with federated users (source identity and session tags) #5359

FlorianSW commented Oct 9, 2023

iamhopaul123 left a comment

iamhopaul123 Oct 11, 2023

FlorianSW commented Oct 23, 2023

github-actions bot commented Oct 25, 2023 •

edited

FlorianSW commented Nov 1, 2023

codecov-commenter commented Nov 1, 2023

iamhopaul123 left a comment

iamhopaul123 Nov 1, 2023

iamhopaul123 Nov 1, 2023

FlorianSW Nov 6, 2023

iamhopaul123 Nov 6, 2023

FlorianSW Nov 20, 2023

iamhopaul123 Dec 1, 2023

feat: Allow assume role chaining with federated users (source identity and session tags) #5359

Are you sure you want to change the base?

feat: Allow assume role chaining with federated users (source identity and session tags) #5359

Conversation

FlorianSW commented Oct 9, 2023

iamhopaul123 left a comment

Choose a reason for hiding this comment

iamhopaul123 Oct 11, 2023

Choose a reason for hiding this comment

FlorianSW commented Oct 23, 2023

github-actions bot commented Oct 25, 2023 • edited

FlorianSW commented Nov 1, 2023

codecov-commenter commented Nov 1, 2023

Codecov Report

iamhopaul123 left a comment

Choose a reason for hiding this comment

iamhopaul123 Nov 1, 2023

Choose a reason for hiding this comment

iamhopaul123 Nov 1, 2023

Choose a reason for hiding this comment

FlorianSW Nov 6, 2023

Choose a reason for hiding this comment

iamhopaul123 Nov 6, 2023

Choose a reason for hiding this comment

FlorianSW Nov 20, 2023

Choose a reason for hiding this comment

iamhopaul123 Dec 1, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2023 •

edited