copilot-cli icon indicating copy to clipboard operation
copilot-cli copied to clipboard

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

Open FlorianSW opened this issue 2 years ago • 4 comments

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 avatar Oct 09 '23 07:10 FlorianSW

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

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

Sure thing :)

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

  1. 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 avatar Oct 23 '23 10:10 FlorianSW

🍕 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

github-actions[bot] avatar Oct 25 '23 11:10 github-actions[bot]

@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 :)

FlorianSW avatar Nov 01 '23 15:11 FlorianSW

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.

codecov-commenter avatar Nov 01 '23 15:11 codecov-commenter