This repository was archived by the owner on Jun 22, 2026. It is now read-only.

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

Open

FlorianSW wants to merge 9 commits into

aws:mainline from

FlorianSW:issues/5358

Open

feat: Allow assume role chaining with federated users (source identity and session tags) #5359
FlorianSW wants to merge 9 commits into
aws:mainline from
FlorianSW:issues/5358

Conversation

@FlorianSW

@FlorianSW FlorianSW commented Oct 9, 2023

Copy link

Copy Markdown

Contributor

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.

@FlorianSW


 Allow assume role chaining with federated users (source identity and ...

...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.

@FlorianSW

FlorianSW requested a review from a team as a code owner

October 9, 2023 07:59

@FlorianSW

FlorianSW requested review from dannyrandall and removed request for a team

October 9, 2023 07:59

@amazon-ecs-cli-v2-pr-manager

amazon-ecs-cli-v2-pr-manager requested a review from efekarakus

October 9, 2023 07:59

iamhopaul123

iamhopaul123 reviewed

Oct 11, 2023

View reviewed changes

@iamhopaul123 iamhopaul123 left a comment

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Action:

- sts:AssumeRole

- sts:SetSourceIdentity

- stsTagSession

@iamhopaul123 iamhopaul123 Oct 11, 2023

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

- stsTagSession

- sts: TagSession

@FlorianSW

FlorianSW commented Oct 23, 2023

Copy link

Copy Markdown

Contributor Author

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.

FlorianSW added 2 commits

October 24, 2023 13:06

@FlorianSW


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

@FlorianSW


 Make additional assume role permissions in envmanager role configurable

3075aa7

@github-actions

github-actions Bot commented Oct 25, 2023 •

edited

Loading

Copy link

Copy Markdown

🍕 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 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

@FlorianSW


 missing integration tests

01ec03b

@FlorianSW


 fix codestyle :)

8a77a4c

@FlorianSW


 adjust another integration test for new default env manifest yml

179975b

@FlorianSW


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

dd08f9d

@FlorianSW


 fix typo

af271ee

@FlorianSW


 add new permission from merging remote mainline

bfec751

@FlorianSW

FlorianSW commented Nov 1, 2023

Copy link

Copy Markdown

Contributor Author

@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

codecov-commenter commented Nov 1, 2023 •

edited

Loading

Copy link

Copy Markdown

Codecov Report

❌ Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.92%. Comparing base (9c9cf3b) to head (bfec751).
⚠️ Report is 210 commits behind head on mainline.

Files with missing lines	Patch %	Lines
internal/pkg/cli/env_init.go	88.88%	2 Missing ⚠️
internal/pkg/cli/init.go	0.00%	1 Missing ⚠️
internal/pkg/config/env.go	0.00%	1 Missing ⚠️

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

☔ 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

iamhopaul123 reviewed

Nov 1, 2023

View reviewed changes

@iamhopaul123 iamhopaul123 left a comment

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

internal/pkg/cli/init.go

// 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

@iamhopaul123 iamhopaul123 Nov 1, 2023

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

# - sts:SetSourceIdentity

# - sts:TagSession

additionalAssumeRolePermissions:

@iamhopaul123 iamhopaul123 Nov 1, 2023

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@FlorianSW FlorianSW Nov 6, 2023

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@iamhopaul123 iamhopaul123 Nov 6, 2023

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@FlorianSW FlorianSW Nov 20, 2023

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔