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

net/http: go server behind nginx require read entire body before writing response #22209

Open
Tevic opened this Issue Oct 11, 2017 · 19 comments

Comments

Projects
None yet
@Tevic
Contributor

Tevic commented Oct 11, 2017

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

go version go1.9.1 windows/amd64

Does this issue reproduce with the latest release?

YES

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

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=E:\Documents\Develop\WorkSpace\Go
set GORACE=
set GOROOT=E:\Documents\Develop\RunTime\Go\x64
set GOTOOLDIR=E:\Documents\Develop\RunTime\Go\x64\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=D:\CommonSoft\MSYS2\tmp\go-build716063943=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

What did you do?

  1. Start Go Server
package main

import (
	"log"
	"net/http"
)

func main() {
	MockServer()
}

func MockServer() {
	http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
		w.WriteHeader(400)
		w.Write([]byte("HELLO"))
	})
	log.Fatal(http.ListenAndServe(":8080", nil))
}
  1. Start nginx
    nginx.conf
upstream nodes {
        server 127.0.0.1:8080 max_fails=0;
}
server {
        listen 80;
        server_name ~.*;

        access_log logs/access.log main;
        error_log logs/error.log;

        client_max_body_size 1024m;
        client_body_buffer_size 512K;
        proxy_next_upstream error timeout non_idempotent;
        client_body_temp_path   client_body_temp_path 3 2;
        location / {
                proxy_http_version 1.1;
                proxy_pass http://nodes;
                break;
        }
}
  1. Make A POST Request
package main

import (
	"bytes"
	"fmt"
	"log"
	"net/http"
	"net/http/httputil"
)

func main() {
	resp, err := http.Post("http://127.0.0.1:80", "application/octet-stream", bytes.NewReader(make([]byte, 1024*1024)))
	if err != nil {
		log.Fatal(err)
	}
	defer resp.Body.Close()
	respBuf, err := httputil.DumpResponse(resp, true)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(string(respBuf))
}

What did you expect to see?

Response:

HTTP/1.1 400 Bad Request
Content-Length: 5
Connection: keep-alive
Content-Type: text/plain; charset=utf-8
Date: Wed, 11 Oct 2017 04:16:07 GMT
Server: nginx/1.13.0

HELLO

What did you see instead?

Resp:

HTTP/1.1 502 Bad Gateway
Content-Length: 173
Connection: keep-alive
Content-Type: text/html
Date: Wed, 11 Oct 2017 04:02:37 GMT
Server: nginx/1.13.0

<html>
<head><title>502 Bad Gateway</title></head>
<body bgcolor="white">
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx/1.13.0</center>
</body>

Nginx Error:

2017/10/11 12:02:37 [error] 11956#2392: *49 WSASend() failed (10054: An existing connection was forcibly closed by the remote host) while sending request to upstream, client: 127.0.0.1, server: ~.*, request: "POST / HTTP/1.1", upstream: "http://127.0.0.1:8080/", host: "127.0.0.1:80"

If i send request without body or the server side read all the body, response will be as expected.
It seems golang should ensure that a FIN packet is sent before any RST packet.
https://trac.nginx.org/nginx/ticket/1037

@Tevic Tevic changed the title from net/http: go server behind nginx should read entire body before writing response to net/http: go server behind nginx require read entire body before writing response Oct 11, 2017

@nvartolomei

This comment has been minimized.

nvartolomei commented Oct 11, 2017

It seems golang should ensure that a FIN packet is sent before any RST packet.

Looks like server already tries to do this

go/src/net/http/server.go

Lines 1611 to 1623 in 6013052

// closeWrite flushes any outstanding data and sends a FIN packet (if
// client is connected via TCP), signalling that we're done. We then
// pause for a bit, hoping the client processes it before any
// subsequent RST.
//
// See https://golang.org/issue/3595
func (c *conn) closeWriteAndWait() {
c.finalFlush()
if tcp, ok := c.rwc.(closeWriter); ok {
tcp.CloseWrite()
}
time.Sleep(rstAvoidanceDelay)
}

@riiiiizzzzzohmmmmm

This comment has been minimized.

riiiiizzzzzohmmmmm commented Oct 19, 2017

I too am experiencing this issue running a Go server behind nginx. Any time my server receives a large enough POST payload (8K?) that fails Authorization via headers, I felt I should respond with http.StatusUnauthorized without reading the entire body.

I've read various relevant issue threads, including https://golang.org/issue/3595, but haven't found a resolution other than reading and discarding the request body, but that seems like an exploitable situation to me.

@huguesb

This comment has been minimized.

Contributor

huguesb commented Oct 19, 2017

Have you tried disabling proxy request buffering in your nginx config?
http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_request_buffering

@riiiiizzzzzohmmmmm

This comment has been minimized.

riiiiizzzzzohmmmmm commented Oct 23, 2017

@huguesb that is a really helpful tip. i'm going to give it a try and report back. thanks!

@Tevic

This comment has been minimized.

Contributor

Tevic commented Oct 24, 2017

I add the config below in nginx config and it works, i am confused why this issue related to tcp keepalive.

#add to server block
proxy_http_version 1.1;
proxy_set_header Connection "";
 
#add to upstream block
keepalive 120;
@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 3, 2017

If someone can distill this to a pure-Go test, we can take a closer look, but as of now there's no clear bug for us to fix. As @nvartolomei mentioned earlier, we already try to CloseWrite the connection to prevent sending an early RST. Perhaps we're not doing that in the right way, or not in all cases, but at the moment I can't tell if the problem is in Go or nginx.

@bradfitz bradfitz added this to the Go1.11 milestone Nov 7, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 7, 2017

I think this is due to this part of func (*chunkWriter) writeHeader:

        // Per RFC 2616, we should consume the request body before
        // replying, if the handler hasn't already done so. But we
        // don't want to do an unbounded amount of reading here for
        // DoS reasons, so we only try up to a threshold.
        // TODO(bradfitz): where does RFC 2616 say that? See Issue 15527
        // about HTTP/1.x Handlers concurrently reading and writing, like
        // HTTP/2 handlers can do. Maybe this code should be relaxed?

And it references #15527.

The threshold it uses is 256k:

// maxPostHandlerReadBytes is the max number of Request.Body bytes not
// consumed by a handler that the server will read from the client
// in order to keep a connection alive. If there are more bytes than
// this then the server to be paranoid instead sends a "Connection:
// close" response.
//
// This number is approximately what a typical machine's TCP buffer
// size is anyway.  (if we have the bytes on the machine, we might as
// well read them)
const maxPostHandlerReadBytes = 256 << 10

But it's not clear that changing this behavior would make nginx happy anyway.

I think we have enough information in this bug to repro (with nginx), though, and then make a decision whether we need to make changes.

I'll flag this as HelpWanted and NeedsInvestigation. Maybe somebody can play around.

@smasher164

This comment has been minimized.

Contributor

smasher164 commented Nov 20, 2017

Trying to reproduce this example in pure go led to the correct behavior.
client.go

package main

import (
	"bytes"
	"fmt"
	"log"
	"net/http"
	"net/http/httputil"
)

func main() {
	resp, err := http.Post("http://127.0.0.1:80", "application/octet-stream", bytes.NewReader(make([]byte, 1024*1024)))
	if err != nil {
		log.Fatal(err)
	}
	defer resp.Body.Close()
	respBuf, err := httputil.DumpResponse(resp, true)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(string(respBuf))
}

backend.go

package main

import (
	"log"
	"net/http"
)

func main() {
	http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
		w.WriteHeader(400)
		w.Write([]byte("HELLO"))
	})
	log.Fatal(http.ListenAndServe(":8080", nil))
}

proxy.go

package main

import (
	"log"
	"net/http"
	"net/http/httputil"
	"net/url"
)

func main() {
	u, err := url.Parse("http://127.0.0.1:8080")
	if err != nil {
		log.Fatalln(err)
	}
	proxy := httputil.NewSingleHostReverseProxy(u)
	http.Handle("/", proxy)
	log.Fatal(http.ListenAndServe(":80", nil))
}

Output

HTTP/1.1 400 Bad Request
Connection: close
Content-Length: 5
Content-Type: text/plain; charset=utf-8
Date: Mon, 20 Nov 2017 17:41:41 GMT

HELLO
@marcelmiguel

This comment has been minimized.

marcelmiguel commented Feb 19, 2018

I also have an error similar to this one.
There is a nginx in front of a golang API.
The first request works, but the second fails.
The solution of Tevic does not work for me.
Calling the API fails from Chrome, from postman works ok.

@herrberk

This comment has been minimized.

herrberk commented Mar 19, 2018

I am experiencing a similar issue as well. The following code reproduces the issue in pure-Go: @tombergan
https://play.golang.org/p/G8F0X4DQCDn

For HTTP 1.1 with Transfer-Encoding: chunked, with Content-Length: -1 the request body is closed if we attempt to write part of the response immediately after receiving the first chunk, which is a big problem if you are doing some kind of streaming operation.

@bradfitz When I look at the source code you mentioned above, a possible solution is to change the if condition to check for Content-Length > 0 instead of Content-Length != 0:

if w.req.ContentLength != 0 && !w.closeAfterReply {

This minor change fixes the issue for the code I posted, at least for my use case. Please take a look at it, since this is a very big issue for us affecting many users.

@barovic

This comment has been minimized.

barovic commented Mar 28, 2018

I am having the same annoying problem. The workaround @herrberk posted seems to fix the issue locally. @bradfitz, @tombergan is it possible to add this fix to the repo?

@dineshbhosale

This comment has been minimized.

dineshbhosale commented Apr 11, 2018

I had a similar issue while running go web server behind nginx proxy.
Nginx was returning error HTTP/1.1 502 Bad Gateway but net/http server without nginx was working fine.

Error was resolved after restarting nginx.

@herrberk

This comment has been minimized.

herrberk commented Apr 11, 2018

@dineshbhosale were you using Transfer-Encoding:chunked ? Asking because the example I gave above, fails to receive all the chunks, and I am not using any reverse proxy at all.

@dineshbhosale

This comment has been minimized.

dineshbhosale commented Apr 11, 2018

This is the response header for some static files that are being served by
http.ServeFile(w, r, file2serve)

Accept-Ranges: bytes
Connection: keep-alive
Content-Encoding: gzip
Content-Type: application/javascript
Date: Thu, 12 Apr 2018 03:29:39 GMT
Last-Modified: Wed, 11 Apr 2018 05:32:15 GMT
Server: nginx/1.12.2
Transfer-Encoding: chunked
Vary: Accept-Encoding

Request header

GET /abc/main.dart.bootstrap.js HTTP/1.1
Host: www.example.com
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36
Accept: */*
DNT: 1
Referer: https:/www.example.com/abc/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
Cookie: username=; sid=3; uid=1; ses=nnQvWdtPqmi32Q32Ne52

It works fine almost all the time but once it fails it is never able to recover and serve any request and I had to restart nginx to make it work again.

Also I noticed that http.ServeFile(w, r, file2serve) is adding Transfer-Encoding: chunked for files that are big and have Content-Length > 2000

@herrberk

This comment has been minimized.

herrberk commented Apr 11, 2018

@dineshbhosale You are sending a GET request, so in your case the request is completely read before the server responds to your request. What we are talking about here is when sending a POST request, if the client request body is large, and server starts responding before completing reading the entire request body, the connection closes and data loss occurs. This issue occurs regardless of the go server being behind a reverse proxy or not. Here is my example to reproduce the issue:
https://play.golang.org/p/G8F0X4DQCDn

@dineshbhosale

This comment has been minimized.

dineshbhosale commented Apr 12, 2018

Ok thanks for the info. I don't know why I get error 503 for https traffic , it happened again on my end. Sometimes works sometimes don't. Happens for get requests as well.

@marcelmiguel

This comment has been minimized.

marcelmiguel commented May 17, 2018

I previously posted I had a similar error.
But my error was due to a panic error, that caused NGINX to show the 502 error.
So right now i'm not affected by this error.

@hcconquer

This comment has been minimized.

hcconquer commented May 31, 2018

@huguesb

Have you tried disabling proxy request buffering in your nginx config?
http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_request_buffering

it seems not works, did you use tcpdump to verify ?

@wskinner

This comment has been minimized.

wskinner commented Jun 22, 2018

I'm having a similar problem. I have a go server behind an nginx reverse proxy via nginx-ingress in kubernetes. My POST request body is empty, and restarting nginx didn't solve it. If I query my server without going through nginx, it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment