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

StsAssumeRoleSessionCredentialsProvider is not using the session_duration attribute & is calling assume role with each request. #1179

Open
ksamykandil opened this issue Oct 25, 2018 · 6 comments
Labels
Milestone

Comments

@ksamykandil
Copy link

@ksamykandil ksamykandil commented Oct 25, 2018

There is a bug in the StsAssumeRoleSessionCredentialsProvider that a new request to assume role is done with each request of AWS resource & the session_duration parameter is not used for caching.

This cause a huge load on performance & throttling the Assume role API if you have a high load.

This value is valid for one hour, so it should be used till it expires as it blocks the cross account usage of Rusoto.

Example: Kinesis stream get records API is calling Assume Role with each request which shouldn't be the case.

@ksamykandil

This comment has been minimized.

Copy link
Author

@ksamykandil ksamykandil commented Oct 25, 2018

  1. rusoto_credential::AutoRefreshingProvider is the solution to wrap the StsAssumeRoleSessionCredentialsProvider.

  2. StsAssumeRoleSessionCredentialsProvider should have AutoRefreshingProvider implementation so it will be there by default.

  3. The documentation part of the example using the StsAssumeRoleSessionCredentialsProvider should be updated to avoid such issues.

@matthewkmayer

This comment has been minimized.

Copy link
Member

@matthewkmayer matthewkmayer commented Oct 28, 2018

Thanks for the bug report and the writeup on how you found it. I wish it was discovered in a less expensive way, though. 😭

At the very least we should get a PR for point three: update the documentation to explain current behavior. I really want to keep others from getting a surprise bill like that!

On a more general note, our STS testing is lackluster and should be improved.

@ksamykandil

This comment has been minimized.

Copy link
Author

@ksamykandil ksamykandil commented Oct 28, 2018

@matthewkmayer I'm working on the fix, will be ready in a week hopefully, will update the documentation as well & make a pull request.

@ksamykandil

This comment has been minimized.

Copy link
Author

@ksamykandil ksamykandil commented Oct 29, 2018

@matthewkmayer I made a pull request for the documentation update.
#1181

Thanks!

@matthewkmayer matthewkmayer added this to the 0.37.0 milestone Dec 1, 2018
@copumpkin

This comment has been minimized.

Copy link
Contributor

@copumpkin copumpkin commented Feb 18, 2019

Is it likely that the STS provider will eventually include the auto refreshing behavior itself? It seems pretty counterintuitive to have to wrap it before getting sensible behavior, but I could see some argument for the current behavior.

@matthewkmayer

This comment has been minimized.

Copy link
Member

@matthewkmayer matthewkmayer commented Feb 18, 2019

Yes, the STS provider should get auto refreshing and caching behavior. Nobody's gotten around to doing that yet. 😄

I think it could be done by using the re-exported rusoto_credential crate from rusoto_core and making some custom code for STS. Making the workaround part of the STS crate so it can be found and used. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.