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

Regression - S3 pre-signed GET "The request signature we calculated does not match" when using secure: false #988

Closed
julik opened this issue Nov 6, 2015 · 12 comments
Assignees

Comments

@julik
Copy link
Contributor

julik commented Nov 6, 2015

When upgrading the aws-sdk I stumbled upon a regression in Aws::S3::Object#presigned_url.

Using a bisect I narrowed it down to the version bump from 2.1.10 to 2.1.11.

We are composing presigned GET URLs with the following snippet

bucket.object(key).presigned_url(:get, :secure => false, :expires_in => 2 * 60 * 60).to_s

This works perfectly with 2.1.10, but with 2.1.11 it rejects the signature. This has to do with the secure flag. If I remove it, the tests pass and the URL is accessible.

Probably related to #883

@julik
Copy link
Contributor Author

julik commented Nov 6, 2015

Also probably related #965

@julik
Copy link
Contributor Author

julik commented Nov 6, 2015

Verified the branch from #979 and it exhibits the same failure

@awood45 awood45 self-assigned this Nov 9, 2015
@awood45
Copy link
Member

awood45 commented Nov 9, 2015

I'll take a look at this.

@awood45
Copy link
Member

awood45 commented Nov 9, 2015

It looks like we're not updating the port correctly when applying the HTTP scheme, which means the request is signed against port 443, but run against port 80, which will be a mismatched signature. Working on a patch.

@julik
Copy link
Contributor Author

julik commented Nov 9, 2015

❤️ thanks! I didn't dare to dive that deep into the signing stuff

@julik
Copy link
Contributor Author

julik commented Nov 9, 2015

We currently resorted to gsubbing the https to http and that works

@awood45
Copy link
Member

awood45 commented Nov 9, 2015

Could you give me an example of what you mean?

@julik
Copy link
Contributor Author

julik commented Nov 9, 2015

s3_object.presigned_url_for(:get, ...).to_s.gsub(/^https\:/, 'http:')

@awood45
Copy link
Member

awood45 commented Nov 9, 2015

This is curious, because on the tip of master the presigned URL generated is using an HTTP scheme, but appears to be using port 443 when signing. I'll take a closer look as I construct the patch.

@awood45
Copy link
Member

awood45 commented Nov 9, 2015

Looking a bit deeper, I'm suspecting this is an artifact of how the Ruby URI object is implemented. When we substitute the scheme value, it's still an "HTTPS" type:
#<URI::HTTPS:0x123123123123 URL:http://blahblahblah>

As such, when you pull the 'port' from this, it's still 443. Not sure if this changed over time since you seem to see this working in the past.

@julik
Copy link
Contributor Author

julik commented Nov 9, 2015

I know for sure that this regression was introduced in the transition from 2.1.10 to 2.1.11 (determined by bisection really). Maybe if you initialize the URI object with just the path and then set the scheme the results will be more predictable?

[1] pry(main)> require 'uri'
=> true
[2] pry(main)> u = URI('/some/path')
=> #<URI::Generic:0x007fb735893a98 URL:/some/path>
[3] pry(main)> u.scheme = 'https'
=> "https"
[4] pry(main)> u
=> #<URI::Generic:0x007fb735893a98 URL:https:/some/path>
[5] pry(main)> u.to_s
=> "https:/some/path"
[6] pry(main)> u.scheme = 'http'
=> "http"
[7] pry(main)> u.to_s
=> "http:/some/path"
[8] pry(main)> 

@trevorrowe
Copy link
Member

I took a look at this and came up with a fix. I'll be adding

require 'uri'

uri = URI.parse('https://foo.com')
puts "SCHEME: #{uri.scheme} PORT: #{uri.port}"
#=> SCHEME: https PORT: 443

# lets change the scheme
uri.scheme = 'http'

# this demonstrates the issue
puts "SCHEME: #{uri.scheme} PORT: #{uri.port}"
#=> SCHEME: http PORT: 443

# this unfortunate behavior is masking the issue, i would have
# expected to see 'http://foo.com:443'
puts uri.to_s
#=> http://foo.com

The bug sneaks by our unit tests beause we are returning the result of #to_s from the URI which is shown to be buggy above. The reason for this is the string formatting for the URI differs based on the URI class. There is both URI::HTTP and URI::HTTPS. Changing the scheme on a URI::HTTPS to http will cause it to omit the port number that is expected when the scheme and port don't match. This causes the bug is singing.

I've got an incoming fix for this shortly.

awood45 added a commit that referenced this issue Dec 16, 2015
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

No branches or pull requests

3 participants