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

Generates invalid URIs #344

Closed
SergeyTsalkov opened this issue Aug 10, 2015 · 4 comments
Closed

Generates invalid URIs #344

SergeyTsalkov opened this issue Aug 10, 2015 · 4 comments

Comments

@SergeyTsalkov
Copy link

An HTTP PUT request should be of the form

PUT path/to/file.jpg
Host: bucket.s3.amazonaws.com

However, this library generates PUT requests of the form

PUT http://bucket.s3.amazonaws.com/path/to/file.jpg
Host: bucket.s3.amazonaws.com

Although S3 handles this correctly, it violates both HTTP/1.1 and the spec here: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html

This also breaks when using DreamObjects, an S3-compatible object store.

@jasdel
Copy link
Contributor

jasdel commented Aug 10, 2015

Hi @SergeyTsalkov Thanks for reporting this issue. This is occurring due to the SDK usage of net/url's URL.Opaque field. This field is needed by the SDK to prevent the Go net/url library from mutating the URI by escaping and unescaping. URL.String() defines how Opaque is used when constructing a URL.

From what I found absolute URI for all methods except CONNECT is valid. According to the HTTP1.1 spec

To allow for transition to absoluteURIs in all requests in future versions of HTTP, all HTTP/1.1 servers MUST accept the absoluteURI form in requests, even though HTTP/1.1 clients will only generate them in requests to proxies.

The Go net/url seems to be taking this as being allowed to use absoluteURI when URL.Opaque is set. The SDK could be updated to use the Request.Host and only set the URL's path in URL.Opaque since URL.Host would be ignored when making requests. But this breaks the ability to call URL.String() to receive a useful URL string. Since URL.Host is ignored.

@lsegal
Copy link
Contributor

lsegal commented Aug 12, 2015

@SergeyTsalkov is the report specifically regarding support for DreamObjects? I would imagine that if DreamObjects is "S3-compatible" as you mention, then it should support whatever is supported by S3, including absoluteURI syntax (otherwise it may not be entirely S3-compatible). Specifically, the SDK is not doing anything that another tool, or even a very well intentioned proxy, might be doing, and according to the HTTP/1.1 spec, web servers (including a server like DreamObjects) "MUST" support the absoluteURI form that the SDK is providing in order to be compliant-- even though you could argue that we "SHOULD" not be crafting the request in this way.

The problem is that, unfortunately, there is no other way to control encoding logic for URIs other than using the Opaque field, and since the URL value is part of our public API (req.HTTPRequest.URL), we want to ensure that it is a self-contained API; in other words, calling req.HTTPRequest.URL.String() returns the fully qualified endpoint without having to manually build it yourself by concatenating values from the URL and the HTTPRequest.Host, which can be confusing and complicated to do. Request pre-signing specifically needs this functionality, for example.

If the issue is really specifically regarding SDK compatibility with DreamObjects (since the SDK itself works fine with S3), I would recommend opening an issue with DreamObjects and requesting that they investigate support for the absoluteURI syntax in their web server logic-- according to the HTTP/1.1 spec, this is something they should ("MUST", technically) be supporting, and something they should be supporting "in all requests in future versions of HTTP", so it is explicitly not a legacy thing.

@lsegal lsegal added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. third-party This issue is related to third-party libraries or applications. labels Aug 12, 2015
@SergeyTsalkov
Copy link
Author

Guys, thanks for the quick and thoughtful responses! Yes, my complaint is entirely about DreamObjects compatibility. Clearly I was wrong about HTTP/1.1, so thanks for that lesson!

I'd hoped that fixing the problem on this end would be faster and easier than getting DreamObjects changed, but it sounds like this would be impractical.

@jasdel
Copy link
Contributor

jasdel commented Jan 12, 2016

@SergeyTsalkov I'm going to close this issue since using the absolute URL within the HTTP request is apart of the HTTP spec, and valid. The Go net/http.URL auto (un)escapes values in the URL which are incompatible with the AWS services. The SDK needs to use the Opaque field of the URL to ensure the auto unescaping logic is not used, and the request are made how services need them.

@jasdel jasdel closed this as completed Jan 12, 2016
@diehlaws diehlaws added 3rd-party and removed third-party This issue is related to third-party libraries or applications. labels Jan 4, 2019
skotambkar pushed a commit to skotambkar/aws-sdk-go that referenced this issue May 20, 2021
Services
===
* Synced the V2 SDK with latest AWS service API definitions.
* Fixes [aws#341](aws/aws-sdk-go-v2#341)
* Fixes [aws#342](aws/aws-sdk-go-v2#342)

SDK Breaking Changes
===
* `aws`: Add default HTTP client instead of http.DefaultClient/Transport ([aws#315](aws/aws-sdk-go-v2#315))
  * Adds a new BuildableHTTPClient type to the SDK's aws package. The type uses the builder pattern with immutable changes. Modifications to the buildable client create copies of the client.  Adds a HTTPClient interface to the aws package that the SDK will use as an abstraction over the specific HTTP client implementation. The SDK will default to the BuildableHTTPClient, but a *http.Client can be also provided for custom configuration.  When the SDK's aws.Config.HTTPClient value is a BuildableHTTPClient the SDK will be able to use API client specific request timeout options.
  * Fixes [aws#279](aws/aws-sdk-go-v2#279)
  * Fixes [aws#269](aws/aws-sdk-go-v2#269)

SDK Enhancements
===
* `service/s3/s3manager`: Update S3 Upload Multipart location ([aws#324](aws/aws-sdk-go-v2#324))
  * Updates the Location returned value of S3 Upload's Multipart UploadOutput type to be consistent with single part upload URL. This update also brings the multipart upload Location inline with the S3 object URLs created by the SDK.
  * Fixes [aws#323](aws/aws-sdk-go-v2#323)
  * V2 Port [aws#2453](aws#2453)

SDK Bugs
===
* `private/model`: Handles empty map vs unset map behavior in send request ([aws#337](aws/aws-sdk-go-v2#337))
  * Updated shape marshal model to handle the empty map vs nil map behavior. Adding a test case to assert behavior when a user sends an empty map vs nil map.
  * Fix [aws#332](aws/aws-sdk-go-v2#332)
* `service/rds`: Fix presign URL for same region ([aws#331](aws/aws-sdk-go-v2#331))
  * Fixes RDS no-autopresign URL for same region issue for aws-sdk-go-v2. Solves the issue by making sure that the presigned URLs are not created, when the source and destination regions are the same. Added and updated the tests accordingly.
  * Fix [aws#271](aws/aws-sdk-go-v2#271)
* `private/protocola/json/jsonutil`: Fix Unmarshal map[string]bool ([aws#320](aws/aws-sdk-go-v2#320))
  * Fixes the JSON unmarshaling of maps of bools. The unmarshal case was missing the condition for bool value, in addition the bool pointer.
  * Fix [aws#319](aws/aws-sdk-go-v2#319)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants