feat: 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.
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.
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 initfor 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.
🍕 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 |
@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 duringcopilot initand instead usecopilot env initafterwards (with the federated-session flag set)
Thanks for all your time reviewing this and investing the time to discuss this topic with me :)
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%) |
:arrow_up: |
| internal/pkg/cli/flag.go | 90.47% <ø> (ø) |
|
| internal/pkg/deploy/cloudformation/stack/env.go | 79.15% <100.00%> (+0.10%) |
:arrow_up: |
| internal/pkg/manifest/env.go | 77.28% <100.00%> (+0.51%) |
:arrow_up: |
| internal/pkg/template/env.go | 52.63% <ø> (ø) |
|
| internal/pkg/cli/init.go | 23.07% <0.00%> (-0.05%) |
:arrow_down: |
| internal/pkg/config/env.go | 87.35% <0.00%> (ø) |
|
| internal/pkg/cli/env_init.go | 65.49% <88.88%> (+0.15%) |
:arrow_up: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.