Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Reduce role duration default from 6 hours to 1 hour #103

Closed
amancevice opened this issue Jul 31, 2020 · 7 comments
Closed

Proposal: Reduce role duration default from 6 hours to 1 hour #103

amancevice opened this issue Jul 31, 2020 · 7 comments
Labels
bug Something isn't working effort/small This issue will take less than a day of effort to fix next-major-version This issue will be fixed/implemented in the next major version p2

Comments

@amancevice
Copy link
Contributor

The action default for role-duration-seconds is 6 hours but the CLI default is 1 hour. I think these defaults should be consistent.

amancevice pushed a commit to amancevice/configure-aws-credentials that referenced this issue Jul 31, 2020
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
amancevice pushed a commit to amancevice/configure-aws-credentials that referenced this issue Jul 31, 2020
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
@piradeepk piradeepk self-assigned this Aug 3, 2020
@piradeepk
Copy link
Contributor

piradeepk commented Aug 4, 2020

@amancevice thanks for reporting this issue. This has been raised in the past, but we had decided at the time to maintain the 6 hour default as this could potentially be a breaking change (if users depended on that behaviour).

That being said, if this is a use case that more users would like to see changed (either by +1 or thumbs up-ing this issue), we're more than willing to update the default. Or if anyone is dependent on the current behaviour, we'd like to know that as well!

We'd love to hear more feedback from the community! Does changing the default impact you negatively in any way?

@amancevice
Copy link
Contributor Author

Thanks, @pkandasamy91 — the reason I discovered this is that I used terraform to create my role and the default max duration for the terraform resource is 1h so I either have to update the max duration for the role, which feels like a moderate security risk to me, or override the default in EVERY workflow configuration, which is slightly inconvenient.

@piradeepk piradeepk changed the title Assume role duration default should match CLI Proposal: Reduce role duration default from 6 hours to 1 hour Aug 4, 2020
@piradeepk
Copy link
Contributor

I can see how this can make things difficult if users are required to override the configured value, and the benefit of changing the default, but seeing as how this is a heavily used action, we're cautious in making widespread changes to existing behaviour.

We'll definitely have a better idea of which direction to take once we get more community feedback!

@spyoungtech
Copy link

Another case where this may be useful is when using an assumed role to assume another role. When role chaining (assuming roles with temporary credentials), you can only request a maximum duration of 1 hour.

@piradeepk @allisaurus I would propose a change with a smaller impact: whenever aws-session-token is provided (meaning temporary credentials/role-chaining is being used), use 1 hour as the default.

This won't be a breaking change because you can never use a session token and request a role for more than 1 hour.

amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Jun 22, 2022
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Jun 22, 2022
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Jun 22, 2022
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
@peterwoodworth peterwoodworth added needs-triage This issue still needs to be triaged bug Something isn't working p2 next-major-version This issue will be fixed/implemented in the next major version effort/small This issue will take less than a day of effort to fix and removed pending community feedback needs-triage This issue still needs to be triaged labels Sep 30, 2022
@peterwoodworth
Copy link
Contributor

I would propose a change with a smaller impact: whenever aws-session-token is provided (meaning temporary credentials/role-chaining is being used), use 1 hour as the default

This is a great idea, I'd be okay with this. Regardless of if this this gets submitted and implemented by anyone in the community, this should change in our next major release to be the hard default.

amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Oct 5, 2022
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Oct 5, 2022
use 1h instead of 6h to match AWS CLI default (as well as default `max_session_duration` for terraform `aws_iam_role` resource)
amancevice added a commit to amancevice/configure-aws-credentials that referenced this issue Oct 5, 2022
use 1h instead of 6h when session token is provided
otherwise use GitHub Actions max duration (6h)
@amancevice
Copy link
Contributor Author

Thanks, @peterwoodworth — I tooled up some changes that I think match your specs: #513

@github-actions
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working effort/small This issue will take less than a day of effort to fix next-major-version This issue will be fixed/implemented in the next major version p2
Projects
None yet
Development

No branches or pull requests

5 participants