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: make Transport check whether a GET Request.Body has any bytes before sending stream? #18891

Open
kalbasit opened this issue Feb 2, 2017 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kalbasit
Copy link

kalbasit commented Feb 2, 2017

What version of Go are you using (go version)?

go version go1.7.5 darwin/amd64 (also tested on linux)

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/kalbasit/code"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.5/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.5/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ty/mr64q0m92n98g9lkdrthg2qc0000gn/T/go-build293044907=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

https://play.golang.org/p/uwnSRGt27v

package main

import (
	"bytes"
	"crypto/tls"
	"io"
	"io/ioutil"
	"log"
	"net/http"
)

func main() {
	c := http.Client{}

	// According to the CloudFront documentation for a request behavior, if the
	// request is GET and includes a body, it returns a 403 Forbidden. See the
	// documentation here:
	// https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/RequestAndResponseBehaviorCustomOrigin.html#RequestCustom-get-body
	var body bytes.Buffer
	r, err := http.NewRequest("GET", "https://d1ytf8d9p4nfnz.cloudfront.net/optivpaid/optivpaid.js?cb=847742584", ioutil.NopCloser(&body))
	if err != nil {
		log.Fatalf("error creating the request: %s", err)
	}

	res, err := c.Do(r)
	if err != nil {
		log.Fatalf("error doing the request: %s", err)
	}
	io.Copy(ioutil.Discard, res.Body)
	res.Body.Close()

	log.Printf("response status for an HTTP/2 request: %s", res.Status)

	// doing the same request without HTTP/2 does work
	c.Transport = &http.Transport{
		TLSNextProto: map[string]func(string, *tls.Conn) http.RoundTripper{},
	}
	r, err = http.NewRequest("GET", "https://d1ytf8d9p4nfnz.cloudfront.net/optivpaid/optivpaid.js?cb=847742584", ioutil.NopCloser(&body))
	if err != nil {
		log.Fatalf("error creating the request: %s", err)
	}

	res, err = c.Do(r)
	if err != nil {
		log.Fatalf("error doing the request: %s", err)
	}
	io.Copy(ioutil.Discard, res.Body)
	res.Body.Close()

	log.Printf("response status for an HTTP/1 request: %s", res.Status)
}

What did you expect to see?

% go run main.go
2017/02/01 17:27:31 response status for an HTTP/2 request: 200 OK
2017/02/01 17:27:32 response status for an HTTP/1 request: 200 OK

What did you see instead?

% go run main.go
2017/02/01 17:27:31 response status for an HTTP/2 request: 403 Forbidden
2017/02/01 17:27:32 response status for an HTTP/1 request: 200 OK
@odeke-em
Copy link
Member

odeke-em commented Feb 2, 2017

Same result if you use http.NoBody as well.

Using http.NoBody

--- s1	2017-02-01 18:35:44.000000000 -0700
+++ s2	2017-02-01 18:36:52.000000000 -0700
@@ -17,7 +17,7 @@
 	// documentation here:
 	// https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/RequestAndResponseBehaviorCustomOrigin.html#RequestCustom-get-body
 	var body bytes.Buffer
-	r, err := http.NewRequest("GET", "https://d1ytf8d9p4nfnz.cloudfront.net/optivpaid/optivpaid.js?cb=847742584", ioutil.NopCloser(&body))
+	r, err := http.NewRequest("GET", "https://d1ytf8d9p4nfnz.cloudfront.net/optivpaid/optivpaid.js?cb=847742584", http.NoBody)
 	if err != nil {
 		log.Fatalf("error creating the request: %s", err)
 	}
2017/02/01 18:35:02 response status for an HTTP/2 request: 403 Forbidden
2017/02/01 18:35:02 response status for an HTTP/1 request: 200 OK

Using nil as the body

Setting the body explicitly to nil works though.

2017/02/01 18:35:13 response status for an HTTP/2 request: 200 OK
2017/02/01 18:35:13 response status for an HTTP/1 request: 200 OK

/cc @bradfitz

@bradfitz
Copy link
Contributor

bradfitz commented Feb 2, 2017

@odeke-em, thanks.

The HTTP/2 trace is:

$ GODEBUG=http2debug=2 go run front.go
2017/02/02 01:39:23 http2: Transport failed to get client conn for d1ytf8d9p4nfnz.cloudfront.net:443: http2: no cached connection was available
2017/02/02 01:39:23 http2: Transport creating client conn 0xc420001a00 to 52.84.64.92:443
2017/02/02 01:39:23 http2: Framer 0xc420078750: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2017/02/02 01:39:23 http2: Framer 0xc420078750: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2017/02/02 01:39:23 http2: Transport encoding header ":authority" = "d1ytf8d9p4nfnz.cloudfront.net"
2017/02/02 01:39:23 http2: Transport encoding header ":method" = "GET"
2017/02/02 01:39:23 http2: Transport encoding header ":path" = "/optivpaid/optivpaid.js?cb=847742584"
2017/02/02 01:39:23 http2: Transport encoding header ":scheme" = "https"
2017/02/02 01:39:23 http2: Transport encoding header "accept-encoding" = "gzip"
2017/02/02 01:39:23 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2017/02/02 01:39:23 http2: Framer 0xc420078750: wrote HEADERS flags=END_HEADERS stream=1 len=74
2017/02/02 01:39:23 http2: Framer 0xc420078750: read SETTINGS len=18, settings: MAX_CONCURRENT_STREAMS=128, INITIAL_WINDOW_SIZE=65536, MAX_FRAME_SIZE=16777215
2017/02/02 01:39:23 http2: Transport received SETTINGS len=18, settings: MAX_CONCURRENT_STREAMS=128, INITIAL_WINDOW_SIZE=65536, MAX_FRAME_SIZE=16777215
2017/02/02 01:39:23 http2: Framer 0xc420078750: wrote DATA flags=END_STREAM stream=1 len=0 data=""
2017/02/02 01:39:23 http2: Framer 0xc420078750: wrote SETTINGS flags=ACK len=0
2017/02/02 01:39:23 http2: Framer 0xc420078750: read WINDOW_UPDATE len=4 (conn) incr=2147418112
2017/02/02 01:39:23 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=2147418112
2017/02/02 01:39:24 http2: Framer 0xc420078750: read SETTINGS flags=ACK len=0
2017/02/02 01:39:24 http2: Transport received SETTINGS flags=ACK len=0
2017/02/02 01:39:24 http2: Framer 0xc420078750: read HEADERS flags=END_HEADERS stream=1 len=187
2017/02/02 01:39:24 http2: decoded hpack field header field ":status" = "403"
2017/02/02 01:39:24 http2: decoded hpack field header field "server" = "CloudFront"
2017/02/02 01:39:24 http2: decoded hpack field header field "date" = "Thu, 02 Feb 2017 01:39:23 GMT"
2017/02/02 01:39:24 http2: decoded hpack field header field "content-type" = "text/html"
2017/02/02 01:39:24 http2: decoded hpack field header field "content-length" = "551"
2017/02/02 01:39:24 http2: decoded hpack field header field "x-cache" = "Error from cloudfront"
2017/02/02 01:39:24 http2: decoded hpack field header field "via" = "1.1 7496200b9bee5aedf66ca80c92d30654.cloudfront.net (CloudFront)"
2017/02/02 01:39:24 http2: decoded hpack field header field "x-amz-cf-id" = "-IioZgk6uvdQb7Fmw596kmwW4oPC_F2qcGzPj2t5bj_fb-Skq6xS6A=="
2017/02/02 01:39:24 http2: Transport received HEADERS flags=END_HEADERS stream=1 len=187
2017/02/02 01:39:24 http2: Framer 0xc420078750: read DATA flags=END_STREAM stream=1 len=551 data="<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\" \"http://www.w3.org/TR/html4/loose.dtd\">\n<HTML><HEAD><META HTTP-EQUIV=\"Content-Type\" CONTENT=\"text/html; charset=iso-8859-1\">\n<TITLE>ERROR: The request could not be satisfied</TITLE>\n</HEAD><BOD" (295 bytes omitted)
2017/02/02 01:39:24 http2: Transport received DATA flags=END_STREAM stream=1 len=551 data="<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\" \"http://www.w3.org/TR/html4/loose.dtd\">\n<HTML><HEAD><META HTTP-EQUIV=\"Content-Type\" CONTENT=\"text/html; charset=iso-8859-1\">\n<TITLE>ERROR: The request could not be satisfied</TITLE>\n</HEAD><BOD" (295 bytes omitted)
2017/02/02 01:39:24 http2: Framer 0xc420078750: read WINDOW_UPDATE stream=1 len=4 incr=2147418111
2017/02/02 01:39:24 http2: Transport received WINDOW_UPDATE stream=1 len=4 incr=2147418111
2017/02/02 01:39:24 response status for an HTTP/2 request: 403 Forbidden

I guess we could make the http2 package recognize http.NoBody.

We can do that for Go 1.9. (Too late for Go 1.8, especially as it's not a regression.)

@kalbasit, there's a simpler fix: use nil for your body.

I'm kinda surprised CloudFront distinguishes where the END_STREAM is. But fair enough.

We could also investigate doing the 1-byte Body.Read check in http2 (which was removed and restored during Go 1.8 in 6e36811). But it was removed because it kept causing problems and is hard to get right.

@kalbasit, how did this happen for you? I assume this is a reduction of a bigger program. What is the concrete type of the body in your actual case? Is it all composed of standard library Readers/Closers, or is it some custom type?

@bradfitz bradfitz added this to the Go1.9 milestone Feb 2, 2017
@bradfitz bradfitz changed the title net/http: client returns 403 from CloudFront if GET and body present (even empty) x/net/http2: make Transport recognize NoBody, and maybe read a byte again on GETs Feb 2, 2017
@kalbasit
Copy link
Author

kalbasit commented Feb 2, 2017

@bradfitz We have a proxy server that does a clone of the incoming request to the outgoing request, very similar to what

outreq := req.WithContext(ctx) // includes shallow copies of maps, but okay
if req.ContentLength == 0 {
outreq.Body = nil // Issue 16036: nil Body for http.Transport retries
}
p.Director(outreq)
outreq.Close = false
// We are modifying the same underlying map from req (shallow
// copied above) so we only copy it if necessary.
copiedHeaders := false
// Remove hop-by-hop headers listed in the "Connection" header.
// See RFC 2616, section 14.10.
if c := outreq.Header.Get("Connection"); c != "" {
for _, f := range strings.Split(c, ",") {
if f = strings.TrimSpace(f); f != "" {
if !copiedHeaders {
outreq.Header = make(http.Header)
copyHeader(outreq.Header, req.Header)
copiedHeaders = true
}
outreq.Header.Del(f)
}
}
}
// Remove hop-by-hop headers to the backend. Especially
// important is "Connection" because we want a persistent
// connection, regardless of what the client sent to us.
for _, h := range hopHeaders {
if outreq.Header.Get(h) != "" {
if !copiedHeaders {
outreq.Header = make(http.Header)
copyHeader(outreq.Header, req.Header)
copiedHeaders = true
}
outreq.Header.Del(h)
}
}
if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
// If we aren't the first proxy retain prior
// X-Forwarded-For information as a comma+space
// separated list and fold multiple headers into one.
if prior, ok := outreq.Header["X-Forwarded-For"]; ok {
clientIP = strings.Join(prior, ", ") + ", " + clientIP
}
outreq.Header.Set("X-Forwarded-For", clientIP)
}
does. To fix it, I am setting the body explicitly to nil if the request method is not POST/PUT/PATCH.

Thanks for looking into it @bradfitz @odeke-em

@kalbasit
Copy link
Author

kalbasit commented Feb 2, 2017

here's the entire (real function) that prepares an outgoing request from the incoming request with the workaround in place.

var (
        // Hop-by-hop headers. These are removed when sent to the backend.
        // http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html
        hopHeaders = []string{
                "Connection",
                "Keep-Alive",
                "Proxy-Authenticate",
                "Proxy-Authorization",
                "Te", // canonicalized version of "TE"
                "Trailers",
                "Transfer-Encoding",
                "Upgrade",
        }

        blackListedHeaders = append(hopHeaders, "X-Forwarded-Proto", "X-Amz-Cf-Id", "X-Forwarded-Port")
)

// prepareOutgoingRequest clones the request and prepare it for proxying
func prepareOutgoingRequest(req *http.Request) *http.Request {
        var preq http.Request
        preq = *req // includes shallow copies of maps, but okay

        // remove things that must be not be part of the client
        preq.Proto = "HTTP/1.1" // not used by the client
        preq.ProtoMajor = 1     // not used by the client
        preq.ProtoMinor = 1     // not used by the client
        preq.Close = false
        preq.Host = ""
        preq.RequestURI = ""
        preq.URL = nil
        preq.ContentLength = 0
        preq.TransferEncoding = []string{}

        // if the method is not a POST, PUT or PATCH, remove the body
        if preq.Method != "POST" && preq.Method != "PUT" && preq.Method != "POST" {
                preq.Body = nil
        }

        // Copy headers
        preq.Header = cloneHeader(req.Header)

        // Remove hop-by-hop headers to the backend. Especially
        // important is "Connection" because we want a persistent
        // connection, regardless of what the client sent to us.
        for _, name := range blackListedHeaders {
                if preq.Header.Get(name) != "" {
                        preq.Header.Del(name)
                }
        }

        // Remove the Cookie header, this header is handled by a cookie jar
        preq.Header.Del("Cookie")

        if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
                // If we aren't the first proxy retain prior
                // X-Forwarded-For information as a comma+space
                // separated list and fold multiple headers into one.
                if prior, ok := preq.Header["X-Forwarded-For"]; ok {
                        clientIP = strings.Join(prior, ", ") + ", " + clientIP
                }
                preq.Header.Set("X-Forwarded-For", clientIP)
        }

        return &preq
}

@bradfitz
Copy link
Contributor

bradfitz commented Feb 2, 2017

@kalbasit, yeah, that sounds fine. The other thing you can do is read a byte in your proxy to see if there's actually a byte there. If not (if you get 0, EOF right away), then set the body to nil. If you do get a byte, stitch back your read byte with the remainder using io.MultiReader and set that as your output Request.Body.

@kalbasit
Copy link
Author

kalbasit commented Feb 2, 2017

@bradfitz thanks for the tip, I'll give that a try.

@gopherbot
Copy link
Contributor

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

@bradfitz bradfitz self-assigned this Jun 16, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 16, 2017
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/net that referenced this issue Jun 16, 2017
Updates golang/go#18891

Change-Id: I866862d805dc1757b27817ddb30cf22dc48ac3ff
Reviewed-on: https://go-review.googlesource.com/45993
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bradfitz bradfitz removed their assignment Jun 16, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 16, 2017
@bradfitz
Copy link
Contributor

Okay, for Go 1.9, http2 will recognize http.NoBody as if it were nil.

For Go 1.10 we can consider whether there are any cases warranting us to reconsider the decision to not do the 1-byte read test to see if a non-nil ReadCloser is actually empty.

@bradfitz bradfitz added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jun 16, 2017
gopherbot pushed a commit that referenced this issue Jun 16, 2017
Updates http2 to x/net/http2 git rev 973f3f3 for:

   http2: make Transport treat http.NoBody like it were nil
   https://golang.org/cl/45993

Updates #18891

Change-Id: I846ccf286992ed2c6249014e51fdeb40b35e50ed
Reviewed-on: https://go-review.googlesource.com/46000
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@odeke-em
Copy link
Member

odeke-em commented Jan 9, 2018

Happy New Year @bradfitz! How are we doing here? At least x/net/http2 now recognizes http.NoBody so perhaps we can adjust the title accordingly

Perhaps we should punt this for Go1.11 since this issue hasn't gotten much attention in the cycle. In regards to "not doing the 1-byte read test to see if a non-nil ReadCloser is actually empy"
for example we can check if:

  • if body implements interface { Len() int }
  • if implements io.Seek, seek by 1 and on no error check if has content
// for bytes.Buffer and strings.Reader
if lener, ok := body.(interface{
   Len() int
}); ok {
   return lener.Len() > 0
}
// Any seekable readers e.g os.File
if seeker, ok := body.(io.Seeker); ok {
   n, err := seeker.Seek(1, io.SeekStart)
   if err == nil && n > 0 {
      defer seeker.Seek(-n, io.SeekCurrent)
   }
   return err == nil && n > 0
}
// For the rest no option but to try to read the single byte

what do you think?

@bradfitz bradfitz changed the title x/net/http2: make Transport recognize NoBody, and maybe read a byte again on GETs x/net/http2: make Transport check whether a GET Request.Body has any bytes before sending stream? Jan 11, 2018
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Jan 11, 2018
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Updates golang/go#18891

Change-Id: I866862d805dc1757b27817ddb30cf22dc48ac3ff
Reviewed-on: https://go-review.googlesource.com/45993
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants