fix: invalid request when assume_role_ttl >1h used from upstream source_profile #215
Conversation
## Background When role chaining, AWS sets a [hard upper limit of 1h on the chained assume role TTL][1]. However, the upstream non-chained role can still have a TTL of up to 12h. ## Context Prior to this PR, if the chained role did not have a value set for `assume_role_ttl`, aws-okta would recurse up the chain looking for one. If one was set upstream with a value >1h, aws-okta would make a bad request resulting in: ``` ValidationError: The requested DurationSeconds exceeds the 1 hour session limit for roles assumed by role chaining. status code: 400, request id: 2e2a5e15-da98-4bd0-8457-6e95565b7fe2 ``` ## Change Adds the `recursive` boolean to `GetValue` that toggles whether it will descend into `source_profile` when the given key is not found. I've updated `updateDurationFromConfigProfile` to only look for `assume_role_ttl` in the selected profile. I set all other instances of `GetValue` to `true` to preserve the old behavior. However, it's worth noting that [aws-cli][2] [does not appear to recurse in this way][3]. Feedback welcome, of course. [1]: https://github.com/awsdocs/iam-user-guide/blob/8d78057/doc_source/id_roles_terms-and-concepts.md [2]: https://github.com/aws/aws-cli [3]: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-role.html
|
@andy-v-h I think you accidentally stamped the upstream project, but thank you for the review all the same 😄 |
lib/config.go
Outdated
| @@ -66,12 +66,16 @@ func sourceProfile(p string, from Profiles) string { | |||
| return p | |||
| } | |||
|
|
|||
| func (p Profiles) GetValue(profile string, config_key string) (string, string, error) { | |||
| func (p Profiles) GetValue(profile string, config_key string, recursive bool) (string, string, error) { | |||
There was a problem hiding this comment.
I think I'd prefer a new GetValueRecursive function
https://github.com/segmentio/aws-okta/wiki/Style-Guide#api-compatibility
There was a problem hiding this comment.
I started to make this change, but ended up creating a GetDirectValue function instead. Here's why:
I was calling the existing (before this PR) GetValue behavior recursive. Upon revisiting this, it occurred to me that it was a bit misleading because it doesn't descend past the immediate source_profile.
To address the underlying issue, I needed a function that only looked at settings set directly on the given profile, which is what GetDirectValue does. My most recent commit restores the original GetValue behavior, but I did make it use GetDirectValue under-the-hood to DRY things up a little. Happy to restore the old implementation if you'd prefer less indirection over DRY here.
|
@brandt We also have this issue and would like to see you PR get in. Will you be making the refactor changes requested? |
This allows us to introduce the change without breaking API compatibility. Upon revisiting this issue, it occurred to me that calling the default `GetValue` behavior "recursive" could be a bit misleading. It only descends to its immediate `source_profile`. It doesn't then descend into that profile's `source_profile` as word "recursive" would imply.
|
@sdann I just pushed a new commit that I think addresses the core ask. I'll try to keep an eye on this over the next few days in case other changes are needed. |
There was a problem hiding this comment.
Thanks @brandt. I think these changes match the coding style guidelines. I've tested your patch stack on my setup and it works as expected. assume_role_ttl must now explicitly be set per role, otherwise the default is used. This is the behavior I expected originally.
Background
When role chaining, AWS sets a hard upper limit of 1h on the chained assume role TTL. However, the upstream non-chained role can still have a TTL of up to 12h.
Context
Prior to this PR, if the chained role did not have a value set for
assume_role_ttl, aws-okta would look for it in the source profile. If the source profile set it to a value >1h, aws-okta would make a bad request resulting in:Change
Adds a
GetDirectValuefunction to only search for the setting in the given profile (i.e. doesn't descend intosource_profileor the okta profile). This is matches the behavior of aws-cli which does not appear to perform a recursive search for missing values.I've updated
updateDurationFromConfigProfileto use this so that it only looks forassume_role_ttlin the top-level profile.