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

Aws:STS:Presigner sometimes sets X-Amz-Expires < 900 #2887

Open
rittneje opened this issue Jul 25, 2023 · 18 comments
Open

Aws:STS:Presigner sometimes sets X-Amz-Expires < 900 #2887

rittneje opened this issue Jul 25, 2023 · 18 comments
Assignees
Labels
feature-request A feature should be added or improved. p3 This is a minor priority issue

Comments

@rittneje
Copy link

Describe the bug

After upgrading from 3.173.0 to 3.178.0 of aws-sdk-core, get_caller_identity_presigned_url sometimes sets X-Amz-Expires to a value < 900.
A value other than 900 does not work for anything other than S3.

We use these presigned URLs for authn, and the server strictly enforces the value of this parameter for safety.

I believe it is due to this "feature" from aws-sdk-s3 v1.127.0:

Feature - Select minimum expiration time for presigned urls between the expiration time option and the credential expiration time.

I don't see anything of relevance in the changelog for aws-sdk-core.

Expected Behavior

see above

Current Behavior

see above

Reproduction Steps

Generate a presigned url when role creds will expire in < 15 minutes

Possible Solution

No response

Additional Information/Context

No response

Gem name ('aws-sdk', 'aws-sdk-resources' or service gems like 'aws-sdk-s3') and its version

aws-sdk-core 3.178.0

Environment details (Version of Ruby, OS environment)

n/a

@rittneje rittneje added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 25, 2023
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Jul 25, 2023
@yenfryherrerafeliz yenfryherrerafeliz added investigating Issue is being investigated and removed needs-triage This issue or PR still needs to be triaged. labels Jul 25, 2023
@mullermp
Copy link
Contributor

Thanks for opening up an issue. The change was in aws-sigv4:

1.6.0 (2023-06-28)
------------------

* Feature - Select the minimum expiration time for presigned urls between the expiration time option and the credential expiration time.

Selecting the minimum credentials time should have been safe.

We use these presigned URLs for authn, and the server strictly enforces the value of this parameter for safety.

Can you be specific about what server is enforcing this? Do you have a reproduction case?

@yenfryherrerafeliz yenfryherrerafeliz added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating Issue is being investigated labels Jul 25, 2023
@rittneje
Copy link
Author

It's a server that works very much like Kubernetes does, where we leverage the presigned URL to prove the client's identity.

@mullermp
Copy link
Contributor

Who owns the software where that validation occurs? I tested that a GetCallerIdentity presigned url will work for an expiration < 900, so any other validation that is occurring is happening elsewhere and not on any AWS service. You can verify the client's identity but perhaps not specifically look at an expiration of 900 seconds, that seems very fragile.

@mullermp mullermp added third-party This issue is related to third-party libraries or applications. wontfix We have determined that we will not resolve the issue. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. bug This issue is a bug. labels Jul 25, 2023
@mullermp
Copy link
Contributor

The feature of selecting minimum expiration time is very much intended. Otherwise, there are cases where a pre-signed url with expiration of 900 seconds may not work if the credentials expire any time sooner.

@rittneje
Copy link
Author

rittneje commented Jul 25, 2023

@mullermp No other AWS SDK works that way. And for anything other than S3, an expiration time other than 900 doesn't actually work in general, in that AWS ignores it and uses 900 anyway. Attempting to use that field to convey when the underlying role creds expire is kind of a hack that is not supported by SigV4 spec.

@mullermp
Copy link
Contributor

Internally the SDK teams talked about using the minimum of credential expiration and expiration time config - we need to do this for an upcoming feature. Do you have any supporting documentation for AWS ignoring the value and using 900 anyway? As I said in the previous message - expiration in the URL was not always a guarantee if the credentials were expiring before 900 seconds (or whatever was configured for the URL)

@rittneje
Copy link
Author

rittneje commented Jul 25, 2023

@mullermp It is mentioned in the Go SDK, and confirmed by our own experimentation. https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/request#Request.Presign

The expire parameter is only used for presigned Amazon S3 API requests. All other AWS services will use a fixed expiration time of 15 minutes.

As has always been the case for presigning, if the role creds expire first, then the presigned URL is no longer valid. However, if, say, your role creds are valid for one hour, and you presign STS GetCallerIdentity with X-Amz-Expires=600, it is still is valid for 15 minutes. Hence, attempting to use X-Amz-Expires to hold the role cred expiration is kind of a hack, and in general no one can rely on a value other than 900 to mean anything.

@mullermp
Copy link
Contributor

I think STS is an exceptional case where the credentials, even when expired, will still work when calling GetCallerIdentity. Other presigned URLs like for RDS and Polly will not work this way.

@mullermp
Copy link
Contributor

I'll ask around internally.

@mullermp mullermp added investigating Issue is being investigated and removed third-party This issue is related to third-party libraries or applications. wontfix We have determined that we will not resolve the issue. labels Jul 25, 2023
@rittneje
Copy link
Author

@mullermp The other thing I want to point out is I don't think this new behavior of "clamping" the presigned expiration is the right solution, even for S3. Let's say my role creds expire in 10 minutes and I try to generate a presigned S3 url for 30 minutes. As per this change, you will instead generate a presigned url with X-Amz-Expires set to 10 minutes. But that doesn't really help as the presigned URL was only valid for 10 minutes anyway. I think the better thing to do in that situation is to just refresh the credentials first, so the resulting presigned url is valid for the intended duration.

@mullermp
Copy link
Contributor

When vending presigned urls to another party (a very common case of presigned urls), the recipient doesn't know anything about what credentials were used and when they may expire. Previously the vended url would indicate 30 minutes expiration in your example and the third party would be surprised when it doesn't work. Both the vendor and the recipient will know exactly when the URL will no longer be valid.

@rittneje
Copy link
Author

@mullermp Only if they know the presigned url came from this SDK, using role credentials whose expiration is known to you. And as I mentioned, I believe the desired behavior would be to refresh the credentials to honor the requested expiration, not to clamp it.

@mullermp
Copy link
Contributor

Refreshing does not guarantee that credentials will be valid for longer than 900 seconds, the previous default. Some credential types only last 5 minutes.

@rittneje
Copy link
Author

Sure, but that's less common, and would consistently not work as expected. This change still does not address the original problem, which is that I want a presigned URL valid for a certain duration, but sometimes the SDK fails to do so. Hence the SDK should first check if credential expiration < presigned expiration. If so, force a refresh. Only after that would clamping be reasonable.

@mullermp
Copy link
Contributor

mullermp commented Jul 28, 2023

Some clarification I got from STS is that, GetCallerIdentity will always succeed if the access key id is still known (hasn't been fully rotated) - this means that the expiration parameter is not considered.

I talked with some other engineers on other SDKs. This is another potential approach we can take -
introduce a presigner option that has three modes:

  • ignore_expiration (default) - This is the previous behavior (I would have to partially revert the change) where the URL always had the expiration that was configured by the presigner, ignoring credential expiration.

  • refresh_if_will_expire - This would refresh the credentials before generating a presigned url. It would potentially have the same issue where credentials may expire before the URL indicates that it may expire.

  • require_full_signing_duration - This would refresh the credentials before generating a presigned url AND compare expiration to the configured amount. If the credentials expire before the configured amount, raise an error.

@mullermp
Copy link
Contributor

mullermp commented Aug 7, 2023

@rittneje Did you have any preference on the above?

@rittneje
Copy link
Author

rittneje commented Aug 7, 2023

require_full_signing_duration might be a little dicey if you are off by a few seconds. For example, if my role assumption is valid for one hour, and I try to generated a presigned URL valid for one hour. Maybe it should add some buffer to the role expiration before taking the minimum. Also raising an error sounds like a harsh penalty. Perhaps the SDK should just log a warning under refresh_if_will_expire instead?

@mullermp mullermp added feature-request A feature should be added or improved. and removed investigating Issue is being investigated labels May 22, 2024
@bhavya2109sharma bhavya2109sharma added the p3 This is a minor priority issue label Jul 31, 2024
@RanVaknin
Copy link

Related to this issue we got on the Go SDK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

5 participants