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

Allow configuration of credential refresh behavior #2520

Conversation

lelandmiller
Copy link

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Hey all! We came across this when using presigned URLs in S3. We often need our presigned URLs a little bit longer than 5 minutes. This change would allow us to ensure we refreshed our credentials within a larger buffer before expiration thus giving us the window we need to use the presigned URLs before they are revoked for an expired token. We were going to work around this in our project, but figured we might as well try to add the option. Maybe if it is useful for us it will be for others as well.

I added tests only where the 5 minute behavior was already tested but added docs everywhere the option is available. I am happy to add tests, or limit which classes can configure the behavior (I personally only care about Aws::InstanceProfileCredentials right now), but wanted to see what you all thought about this change before going any further.

Please let me know if this makes sense and if there is anything you would like me to clean up. Thank you!

@mullermp
Copy link
Contributor

Thanks for opening a pull request. I'm a little confused on the motivation. S3 Presigned Urls by default expire in 15 minutes and they can be set to expire at a maximum of 1 week. My understanding is that credentials refreshing doesn't change an already signed request because it was valid at that time.

You can use this option (in the docs):

:expires_in (Integer) — default: 900 — The number of seconds before the presigned URL expires. Defaults to 15 minutes. As signature version 4 has a maximum expiry time of one week for presigned URLs, attempts to set this value to greater than one week (604800) will raise an exception.

@mqchau
Copy link

mqchau commented May 12, 2021

The effective period of S3 Presigned Urls is the earlier time of either temporary credentials expiration or the expiration of the presigned URL like you pointed out. We ran into the problem that the temporary credentials from InstanceProfileCredentials expires before the expiration of presigned URL.

@lelandmiller
Copy link
Author

Just for reference, this is the issue we are trying to avoid:

If you created a presigned URL using a temporary token, then the URL expires when the token expires, even if the URL was created with a later expiration time.

From https://docs.aws.amazon.com/AmazonS3/latest/userguide/ShareObjectPreSignedURL.html

I initially didn't expect this to be the case, it kind of surprised me 😄

@mullermp
Copy link
Contributor

Ah! Interesting.. I didn't expect that either.

@lelandmiller lelandmiller force-pushed the add-credential-expiration-buffer-option branch from 2cffacd to 7e6ca09 Compare May 12, 2021 18:18
@mullermp
Copy link
Contributor

I need to discuss this with other SDK teams. I'm not sure of the proposed approach because IMDS docs say We make new credentials available at least five minutes before the expiration of the old credentials. but there's no guarantee that new credentials will exist if the proposed buffer value is large enough.

@mullermp
Copy link
Contributor

Related Java SDK feature request: aws/aws-sdk-java-v2#2379

@mqchau
Copy link

mqchau commented May 12, 2021

I tried to have a test cron job to refresh credentials and see how often is too often for the refresh. I find that I can get new credentials as frequent as every hour.

@alextwoods
Copy link
Contributor

alextwoods commented May 13, 2021

Just a thought on this - rather than trying to fix this through adding additional configuration, could you instead fix this by creating a new instance of InstanceProfileCredentials or explicitly calling refresh on your credentials before calling presign?

Thinking a little further - this is a tricky edge case issue that can bite SDK users. Rather than asking our users to add configuration or change how they are using credentials, could we detect this when building the presigned url by looking for credential expiration time before the presigned url experiation and then calling refresh at that point?

@lelandmiller
Copy link
Author

I agree with that second point for sure. It would be much better if SDK users never had to think about this 😄

We have already worked around this by doing refreshing on our side, we just wanted to see if it was generally useful for the SDK so other people would have an easy way to do something similar in a more built-in way.

Either way though, there is still the issue of only having a 5 minute guarantee for new credentials right? We haven't had an issue refreshing credentials early, but if the guarantee is only 5 minutes then it makes me a little nervous.

The way presigned link expiration is working now does seem kind of unintuitive, or at least a little bit tricky to think about. Since we have a 5 minute guaranteed refresh window, the only real way to guarantee a token lasts the max 6 hour link expiration in this flow is to create a new instance of InstanceProfileCredentials and S3 client whenever we need the link. It then requires a decent amount of knowledge of how this all works to get the behavior that many users might expect. I guess it would also be possible to provide a way that the S3 client wrap this logic up somehow.

If it was safe to refresh at any point, that would be awesome. Then we could just add the logic @alextwoods mentioned to refresh in a library when we generate the URL and nobody would have to think about it again. 😃

@mqchau
Copy link

mqchau commented May 13, 2021

the only real way to guarantee a token lasts the max 6 hour link expiration in this flow is to create a new instance of InstanceProfileCredentials and S3 client whenever we need the link

I tried this and it doesn't work. EC2 Instance Metadata Service will return same access key ID if you make many requests close to each other.

@mullermp
Copy link
Contributor

I think this may help, but I've pushed a change that takes the minimum time between expires_in option and the credentials expiration (if there is one), and uses that value for the pre-signed URL, so there's no surprises on when the link expires. #2872

I'm going to close this since we decided not to take a configuration option for this, and there is a work around of refreshing credentials prior to creating a pre-signed url.

@mullermp mullermp closed this Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants