Aws-sdk-ruby: S3 presigned urls using response_expires convert values to ISO8601 instead of http-date

[jrochkind]

Confirm by changing [ ] to [x] below to ensure that it's a bug:

• [x] I've gone though Developer Guide for v3 and API reference
• [x] I've checked AWS Forums and StackOverflow for answers
• [x] I've searched for previous similar issues and didn't find any solution

Describe the bug

For S3 presigned URL generation, supplying the response_expires param will result in a presigned URL asking for a response-expires value in iso8601 format, which results in a subsequent S3 response with an HTTP Expires header with a value in ISO8601 format. Eg, 2015-10-21T07:28:00Z.

This is not a legal value for the Expires header, which should be in http-date format instead. Eg Wed, 21 Oct 2015 07:28:00 GMT

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Expires

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

aws-sdk-s3 1.81.1

Version of Ruby, OS environment

ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-darwin19]

To Reproduce (observed behavior)

bucket.object(object_key).presigned_url(:get, response_expires: "Wed, 21 Oct 2015 07:28:00 GMT")

This results in a presigned URL which has in it the query param response-expires=2015-10-21T07%3A28%3A00Z

When this URL is requested from s3, the response has a header Expires: 2015-10-21T07:28:00Z

Expected behavior

The response-expires query in the generated URL, which is used to create the signature as well, should be in http-date format, not ISO8601 format. The query param for above example should look instead like: response-expires=Wed%2C%2021%20Oct%202015%2007%3A28%3A00%20GMT

In order to result in a response from S3 with a standards-compliant Expires header that will be interpreted by user-agents as intended, Expires: Wed, 21 Oct 2015 07:28:00 GMT

[mullermp]

I suspect that this is a service modeling issue. Here we are checking the timestampFormat of any shapes and there is none declared, so it defaults to ISO format. The ResponseExpires shape on the model should have a timestampFormat of rfc822. I need to check with other SDK teams on this.

[mullermp]

Through some testing, I'm able to send a http-date formatted time object as a query string to S3 using GetObject and it comes back as a valid http-date formatted Expires header. You can try this for yourself by commenting out the default code (in my previous reply) in the query string builder and use httpdate instead. You could even monkey patch the SDK for now if you need the behavior. Apparently there is no SDK trait upstream to support converting to rfc822 (http-date), so even if S3 updates their model, it would not work. I need to make changes to SDK tooling to allow for this trait to exist, then peddle the change to S3 and have them take it.

[jrochkind]

Huh, weird, thanks for the investigation. So are the SDKs in other languages also having this issue, if the problem is to upstream SDK modelling?

Has somehow nobody until now ever noticed that there's no way to use the response-expires S3 presigned URL query param feature, and get a good Expires header, from any of the SDKs? I guess it's not a popular thing to want to do! Odd and surprising!

[mullermp]

Other SDKs may or may not be impacted. Likely not. In the Ruby SDK we have a param converter that will convert to ISO format before it's sent to S3 (particularly look at the String example). We do this because it's supposed to be the default. But then we look at the model to see if it should be a different format (code you saw in first response), and if so, apply that format instead - but there isn't a specific format even though there should be. This should be in the model but isn't. Other SDKs may not have the same conversion going on. ISO8601 is supposed to be default for rest-xml and for query params.

[mullermp]

I've submitted a code review for the packages upstream, that's the easy part. Hard part is getting S3 to take my change to their model.

[mullermp]

I haven't forgotten about this. I am preparing a release with S3. I also tracked down the release where this breakage occurred: https://github.com/aws/aws-sdk-ruby/commit/03f80307dff047621ea95108775d3abc8b70efb7#diff-885d88e0d8505b74befae6d25478446244da798f4ab6bff3e2886957bba983b7

I am probably going to get some flak for this change, because it's going to change behavior for other SDKs too, but it is the correct fix.

[jrochkind]

Thanks!

In this case for Expires header specifically, I believe the changed behavior just changes from something browsers would not do the right thing with (just ignore the header) to something where browsers will do the right thing. So I don't think there is any reason for anyone to prefer the former behavior, even on backwards compat grounds, in any SDK -- the "backwards compatible" behavior was just not doing anything anyone would want.

Unless some browsers would do something with the standardnon-compliant date format in Expires... but I don't think any do. And then if it also effects things other than S3 HTTP response Expires headers, who knows!

[mullermp]

@jrochkind That's true (I will forward that feedback), however, this is not limited to just browsers - any code that relies on parsing (like in applications) may be broken if the expected format is different - it depends on the language and the library used, so it's hard to say definitively. S3 still wants to proceed with caution and I'm doing the diligence testing other SDKs.

[mullermp]

Also (unrelated) - I saw your faster_s3_url implementation was featured on ruby weekly! That's really cool. I just wanted to mention that the runtime plugin design of V3 definitely causes most of the slow down. It's something that we want to eliminate in the next major version when that happens. If you didn't want to maintain the signing code, you can use the presigned_url method in aws-sigv4 directly.

[jrochkind]

I didn't even know faster_s3_url was on ruby weekly, thanks!

Yeah, I initially wanted to maybe try to PR here to speed up S3 URL generation (it was causing a bottleneck in my real app, is what led down that whole path). But looking into it, I saw how baked into the architecture it was -- there were no good ways to improve performance without significant refactoring and/or un-DRY'ing of your code. I could tell you more about where the biggest bottlenecks were -- but you probably already know. (That weird thing where it creates a mock 'request' in order to... get a signature or something?)

My implementation (which is really just a ruby port of AWS's own "how to make a signature" example for python, with some details for S3 figured out) was actually non-trivially faster than even the presigned_url method directly though -- actually, cause ruby stdlib URI.parse is really quite slow, and a lot of the AWS code (including the method we're talking about, I think) ends up calling it multiple times on the same input. But even once is too much if you don't need it -- and for S3, we really don't. At least for the non-legacy-edge-cases of S3, I realize supporting legacy edge cases with DRY code is another thing that leads to AWS code's slowness.

So, yeah, that's why I gave up on PR'ing perf improvements, alas.

[mullermp]

Ah, interesting about URI.parse! Yes, a lot of the code is to support legacy and what not - for example our SDK also supports Ruby 1.9.. yuck. If I could wave a magic wand I'd fix and drop many things. I'll keep that in mind for a new implementation. I'm open to reviewing any improvements that are sane to do :)

[mullermp]

FYI - I've gotten this approved by senior leaders (because it will be a behavior change for all SDKs) and this is planned for around 12/21 after re:Invent.

[mullermp]

This went out today - https://github.com/aws/aws-sdk-ruby/blob/2bd4e16d097f8ade6129b2ed514a8f0c56febf70/apis/s3/2006-03-01/api-2.json#L6737-L6740

Gem will be published in a few hours (I verified the change locally, but will verify the published gem too)

[mullermp]

Closing

[github-actions[bot]]

⚠️COMMENT VISIBILITY WARNING⚠️

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.