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

x/net/http2: requests with absolute URIs in URL.Opaque produce incorrect :path header #16847

Closed
saljam opened this issue Aug 23, 2016 · 14 comments

Comments

Projects
None yet
6 participants
@saljam
Copy link
Contributor

commented Aug 23, 2016

go version go1.7 darwin/amd64

If one sets URL.Opaque in a request to an absolute URI or a scheme-relative one (e.g. "//example.com/foo"), that request would work over HTTP/1.1, but fail over HTTP/2 on some servers (e.g. NGINX) with a PROTOCOL_ERROR.

func main() {
    req, _ := http.NewRequest("GET", "https://nginx.0f.io/hello", nil)
    req.URL.Opaque = "//nginx.0f.io/hello"
    resp, err := http.DefaultClient.Do(req)
    if err != nil {
        log.Fatal(err)
    }
    log.Println(resp.Proto)
}
$ GODEBUG=http2client=0 go run play.go 
2016/08/23 11:53:48 HTTP/1.1
$ GODEBUG=http2client=1 go run play.go 
2016/08/23 11:53:50 Get https://nginx.0f.io/: stream error: stream ID 1; PROTOCOL_ERROR
exit status 1

That's because x/net/http2 uses URL.Opaque (if present) for the :path header. And while HTTP/1.1 allows a Request-URI to be an absolute URI, HTTP/2 expects the :path header to be just the path part. URL.Opaque now has two slightly different meanings in the context of http.

I'm happy to write a CL to for http2 to ignore URL.Opaque for :path, or maybe parse it and use its path part. That would make existing packages that use Opaque (e.g. googleapi) not break on the upgrade to http2.

But perhaps just documenting this Opaque caveat is sufficient.

http2debug=1 output:

$ GODEBUG=http2debug=1 go run play.go 
2016/08/23 11:54:41 http2: Transport failed to get client conn for nginx.0f.io:443: http2: no cached connection was available
2016/08/23 11:54:41 http2: Transport creating client conn 0xc420136000 to [2400:cb00:2048:1::681c:50d]:443
2016/08/23 11:54:41 http2: Transport encoding header ":authority" = "nginx.0f.io"
2016/08/23 11:54:41 http2: Transport encoding header ":method" = "GET"
2016/08/23 11:54:41 http2: Transport encoding header ":path" = "https://nginx.0f.io/"
2016/08/23 11:54:41 http2: Transport encoding header ":scheme" = "https"
2016/08/23 11:54:41 http2: Transport encoding header "accept-encoding" = "gzip"
2016/08/23 11:54:41 http2: Transport received SETTINGS len=18, settings: MAX_CONCURRENT_STREAMS=128, INITIAL_WINDOW_SIZE=65536, MAX_FRAME_SIZE=16777215
2016/08/23 11:54:41 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2016/08/23 11:54:41 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=2147418112
2016/08/23 11:54:41 http2: Transport received SETTINGS flags=ACK len=0
2016/08/23 11:54:41 http2: Transport received RST_STREAM stream=1 len=4 ErrCode=PROTOCOL_ERROR
2016/08/23 11:54:41 RoundTrip failure: stream error: stream ID 1; PROTOCOL_ERROR
2016/08/23 11:54:41 Get https://nginx.0f.io/: stream error: stream ID 1; PROTOCOL_ERROR
exit status 1
@buro9

This comment has been minimized.

Copy link

commented Aug 23, 2016

Worth noting that the Google-API generated clients can not be currently generated in Go1.7 due to this bug.

The generated code will always call SetOpaque (via Expand or directly) https://github.com/google/google-api-go-client/blob/master/google-api-go-generator/gen.go#L1909-L1918 and this results in the "stream ID 1; PROTOCOL_ERROR" at runtime.

However this is not necessarily a google-api generator bug, as it affects all use of Opaque with http/2 when an absolute URI is used.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

/cc @bradfitz

@quentinmit quentinmit added this to the Go1.7.1 milestone Aug 23, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

This seems like it's potentially a regression in 1.7. @bradfitz to weigh in on whether this belongs in a 1.7.1

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Go 1.6 has the same behavior.

@saljam, @buro9, are either of you implying this is a regression from Go 1.6?

@buro9, I'm not sure why you're talking about problems "generating" the code ("can not be currently generated in Go1.7 due to this bug"). This is purely a runtime issue.

Why didn't we hear about it during Go 1.6 if it affects google-api-go-client?

/cc @broady @okdave @mcgreevy

@saljam

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

@bradfitz this has been the behaviour in http2 for a while, and go1.6.2 exhibits this as well. I wouldn't call it a regression.

So far I've only managed to reproduce it with nginx as the server. A Go http2 server is more lenient in how it parses :path and the request wouldn't fail. Maybe this is why it slipped past.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Well, we're definitely violating the protocol:

The :path pseudo-header field includes the path and query parts of the target URI (the path-absolute production and optionally a '?' character followed by the query production (see Sections 3.3 and 3.4 of [RFC3986]). A request in asterisk form includes the value '*' for the :path pseudo-header field.

Where RFC 3986 says:

      path          = path-abempty    ; begins with "/" or is empty
                    / path-absolute   ; begins with "/" but not "//"
@broady

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Definitely interesting that this hasn't come up with google-api-go-client. I don't know how that happened. Maybe the Google HTTP servers don't have a problem with this.

I can't recommend this for 1.7.1, but don't feel strongly about it.

If I'm reading correctly, the workaround is to not use Opaque. Instead, use a combination of Host/RawPath/RawQuery.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

When I use google-api-go-client's SetOpaque function,

func SetOpaque(u *url.URL) {
        u.Opaque = "//" + u.Host + u.Path
}

on,

        req, _ := http.NewRequest("GET", "https://www.google.com/humans.txt", nil)
        SetOpaque(req.URL)

What happens is that http2 package uses req.URL.RequestURI() to generate the :path and that generates the string "https://www.google.com/humans.txt".

Google GFE surprisingly accepts that:

2016/08/23 18:12:12 http2: Transport encoding header ":authority" = "www.google.com"
2016/08/23 18:12:12 http2: Transport encoding header ":method" = "GET"
2016/08/23 18:12:12 http2: Transport encoding header ":path" = "https://www.google.com/humans.txt"
2016/08/23 18:12:12 http2: Transport encoding header ":scheme" = "https"

... no clue why. Maybe for compatibility for broken people like us. I'll ask.

So @buro9, what issue are you talking about? google-api-go-client seems to work. Or are you using it against non-Google hosts for things like go-endpoints?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

(Note to self: I filed internal bug http://b/31037249 to ask why the Google GFE accepts this.)

@bradfitz bradfitz modified the milestones: Unreleased, Go1.7.1 Aug 23, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

I'm included to do nothing here. Or at least wait until Go 1.8 for any fix.

There's no good place to document this, so I could just fix it in the common case: if we would send a :path starting with https://HOST/, replace it with /. But not if the HOST != Request.URI.Host, to be conservative probably. In that case I'd just return an error early rather than violate the protocol.

@broady opened googleapis/google-api-go-client#161 to update google-api-go-client using the Go 1.5 API added to replace Opaque.

@buro9

This comment has been minimized.

Copy link

commented Aug 23, 2016

So @buro9, what issue are you talking about? google-api-go-client seems to work. Or are you using it against non-Google hosts for things like go-endpoints?

Yes :)

The generator that is part of the google API go client project is used by other companies to generate their own Go clients.

At CloudFlare we use it to generate a Go client for a 3rd party system that is documented by JSON Schema, and so we are then using Go http/2 to call a Nginx http/2 server that fronts a Glassfish of the 3rd party API.

This specific project has been stuck on Go1.6.1 for a while not knowing where the root cause of the issue was. Salman and I finally looked into it and got to the bottom of it... Opaque handling in http/2.

@bradfitz bradfitz added NeedsFix and removed WaitingForInfo labels Aug 23, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Okay, good to know what's happening. You two can follow along at googleapis/google-api-go-client#161 for a short-term fix for go-endpoints.

I'll do a low-priority fix for Go 1.8.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 23, 2016

CL https://golang.org/cl/27632 mentions this issue.

gopherbot pushed a commit to googleapis/google-api-go-client that referenced this issue Aug 30, 2016

googleapi: use RawPath, not Opaque, for template expansion
Historically the google-api-go-client has had trouble sending certain
characters in paths without the Go standard library (net/url and
net/http packages) double escaping or escaping in an unexpected manner
(for example, space encoding in issue #64).

As a workaround, we started escaping the URL manually and using
url.Opaque (for example, 02cfcab and 5c258d4). This mostly works but has
problems with HTTP/2 (golang/go#16847). In Go 1.5, the URL struct
introduced the RawPath field which was more suitable for this task: if
RawPath is provided and is a valid escaping of Path, then the url
package will use it as the value of EscapedPath (and EscapedPath is then
subsequently used when constructing HTTP requests etc.).

This commit changes uritemplates.Expand to return both the unescaped and
escaped forms of the the template expansion. This allows us to fill in
both url.Path and url.RawPath in a way that satisfies the criteria for
url.EscapedPath to function correctly.

Issue #161.

Change-Id: I51e54e18f198b6465a6d032b1072282bf3d2f9ce
Reviewed-on: https://code-review.googlesource.com/7110
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Sep 13, 2016

CL https://golang.org/cl/29110 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 13, 2016

net/http: update bundled x/net/http2
Updates x/net/http2 (and x/net/lex/httplex) to git rev 749a502 for:

   http2: don't sniff first Request.Body byte in Transport until we have a conn
   https://golang.org/cl/29074
   Fixes #17071

   http2: add Transport support for unicode domain names
   https://golang.org/cl/29071
   Updates #13835

   http2: don't send bogus :path pseudo headers if Request.URL.Opaque is set
   https://golang.org/cl/27632
     +
   http2: fix bug where '*' as a valid :path value in Transport
   https://golang.org/cl/29070
   Updates #16847

   http2: fix all vet warnings
   https://golang.org/cl/28344
   Updates #16228
   Updates #11041

Also uses the new -underscore flag to x/tools/cmd/bundle from
https://golang.org/cl/29086

Change-Id: Ica0f6bf6e33266237e37527a166a783d78c059c4
Reviewed-on: https://go-review.googlesource.com/29110
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>

warpfork added a commit to polydawn/redesigned-potato that referenced this issue Dec 8, 2016

In which one tries to kernel headers and glibc. Not quite.
Hit golang/go#16847 while cloning the kernel sources.  whee.

Signed-off-by: Eric Myhre <hash@exultant.us>

jasdel added a commit to aws/aws-sdk-go that referenced this issue Dec 13, 2016

private/protocol/rest: Use RawPath instead of Opaque (#993)
It was discovered the bug golang/go#16847 prevents usage of AWS services using HTTP 2, such as AWS X-Ray with the AWS SDK for Go because Go 1.6.2 - 1.7.4 does not correctly build HTTP2 request when the URL uses Opaque to set the escaped version of the URL path. This has been fixed int he Go 1.8 branch, but this change will ensure users using the current version of Go will be able to connect to AWS services using HTTP2.

With this change the SDK will no longer with with the unsupported Go version 1.4.

@golang golang locked and limited conversation to collaborators Sep 13, 2017

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: don't send bogus :path pseudo headers if Request.URL.Opaque is…
… set

Fixes golang/go#16847

Change-Id: I1e6ae1e0746051fd4cf7afc9beae7853293d5b68
Reviewed-on: https://go-review.googlesource.com/27632
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.