Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#22865 closed defect (fixed)

Explicitly set Content-Length to zero when there is no data to send

Reported by: twim Owned by: dcf
Priority: Medium Milestone:
Component: Obfuscation/meek Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Since Go 1.8 http.Transport does not set Content-Length header if body contains no bytes.
https://golang.org/doc/go1.8#net_http
https://go-review.googlesource.com/c/31445/

However some of domain fronts do inspect Content-Length header and return 411 error when it is not set. This breaks connection entierly since these packets do not travel beyond a frontend to a meek server.

Child Tickets

Attachments (4)

0001-Explicitly-set-Content-Length-to-zero-when-there-is-.patch (1.3 KB) - added by twim 5 months ago.
client.go (666 bytes) - added by twim 4 months ago.
server.go (444 bytes) - added by twim 4 months ago.
demo.go (2.0 KB) - added by dcf 4 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 months ago by twim

Status: newneeds_review

comment:2 Changed 5 months ago by dcf

Thanks for the clear explanation. I plan to merge this patch.

Can you give an example of a service that this was causing problems with? If it was Amazon or Azure, then we need to change something about our test procedure because we didn't detect it.

comment:3 Changed 5 months ago by twim

Unfortunately I can't give you an example right now because it looks like something that should not happen. Maybe it's even a bug.

comment:4 in reply to:  3 Changed 5 months ago by dcf

Replying to twim:

Unfortunately I can't give you an example right now because it looks like something that should not happen. Maybe it's even a bug.

Do you mean a bug in the fronting service, or a bug somewhere else in meek-client?

I would guess, that with a POST, the http client would omit Content-Length, and send Transfer-Encoding: chunked, then send a zero-length chunk if there were nothing to send. But I haven't checked.

I'm still planning to merge your patch, because it looks like the right thing to do.

comment:5 Changed 5 months ago by twim

Yes, I mean there is something wrong at the fronting service about it.

comment:6 Changed 5 months ago by twim

Seems to be not a bug. This is now persistent behavior at Google AppEngine Flexible Environment. Most likely it was being upgraded at that moment. Standard Environment works as before (without inspection middleware).

I'm curious how it works. This middleware returns an error only if domain is being fronted. If one goes with a correct appspot.com SNI it takes all requests directly to the backend. Probably there is routing in place that routes requests by SNI to some non-AppEngine frontend which performs these pre-checks. Though this doesn't explain why there are no such checks on Standard Environment.

comment:7 Changed 5 months ago by dcf

Thanks for the information. I'll try to reproduce with the flexible environment.

Does attachment:0001-Explicitly-set-Content-Length-to-zero-when-there-is-.patch fix the error in the flexible environment? Or does it still return status 411 in some cases.

comment:8 Changed 5 months ago by twim

Yes, this patch fixes 411 in GAE flex which always returns 411.

comment:9 Changed 4 months ago by dcf

I've just done a test here locally, and meek-client compiled with go1.8.3 linux/amd64 sends Content-Length: 0 even without the patch from this ticket. I inspected the traffic by running a socat shim on port 4000:

socat -v -v TCP-LISTEN:4000,fork,reuseaddr OPENSSL:xxx.appspot.com:443

Then I configured tor to connect through 127.0.0.1:4000

UseBridges 1
ClientTransportPlugin meek exec ./meek-client --log meek-client.log
Bridge meek 0.0.2.0:3 url=http://xxx.appspot.com/ front=127.0.0.1:4000

The socat output contains examples like this:

POST / HTTP/1.1\r
Host: meek-reflect-test.appspot.com\r
User-Agent: Go-http-client/1.1\r
Content-Length: 0\r
X-Session-Id: Mv2/KXfE3mE\r
Accept-Encoding: gzip\r
\r

Are you able to reproduce this? I don't see how the patch would cause it to behave any differently. And the documentation for http.NewRequest says that a *bytes.Reader has special handling and sets the body to the magic value NoBody when the length of the Reader is 0:

If body is of type *bytes.Buffer, *bytes.Reader, or *strings.Reader, the returned request's ContentLength is set to its exact value (instead of -1), GetBody is populated (so 307 and 308 redirects can replay the body), and Body is set to NoBody if the ContentLength is 0.

So I'm wondering if this patch is really needed? If so, can you give me complete reproduction instructions so that I can see the bug for myself?


Replying to twim:

Seems to be not a bug. This is now persistent behavior at Google AppEngine Flexible Environment. Most likely it was being upgraded at that moment. Standard Environment works as before (without inspection middleware).

I'm confused now by your references to the flexible environment. I tried deploying reflect.go to the flexible environment by adding env: flex to app.yaml:

env: flex
runtime: go
api_version: go1
automatic_scaling:
  max_idle_instances: 2
  min_pending_latency: 1000ms

handlers:
- url: /.*
  script: _go_app
  secure: always

However that caused gcloud app deploy to display an error message:

2017/07/15 13:04:04 failed analyzing meek/appengine: the root of your app needs to be package "main" (currently "reflect")

If I change the package name from "reflect" to "main", I get another error:

2017/07/15 13:05:45 failed analyzing meek/appengine: cannot find package "appengine" in any of:
        ...

So it seems that reflect.go needs more extensive changes than just attachment:0001-Explicitly-set-Content-Length-to-zero-when-there-is-.patch​ in order to run in the flexible environment. Are you running something other than reflect.go on App Engine?

comment:10 in reply to:  9 Changed 4 months ago by twim

Replying to dcf:

I've just done a test here locally, and meek-client compiled with go1.8.3 linux/amd64 sends Content-Length: 0 even without the patch from this ticket. I inspected the traffic by running a socat shim on port 4000:
...
Are you able to reproduce this? I don't see how the patch would cause it to behave any differently.

Yes I am. It turns out that Content-Length is being set to 0 when HTTP/1.1 is used, and omitted in case of HTTP/2.

And the documentation for http.NewRequest says that a *bytes.Reader has special handling and sets the body to the magic value NoBody when the length of the Reader is 0:

No, it says that body is set to NoBody if request.ContentLength == 0.

So I'm wondering if this patch is really needed? If so, can you give me complete reproduction instructions so that I can see the bug for myself?

Yes, see https://github.com/golang/go/issues/20257 for details. And this is a blocker on GAE Flex (maybe others).

I wrote a PoC for this (see attachments). With HTTP/2 it makes a request like this:
POST / HTTP/2.0\r\nHost: meek.appspot.com\r\nAccept-Encoding: gzip\r\nUser-Agent: Go-http-client/2.0\r\n\r\n
So this gets proxied via HTTP/1.1 to the application. If there is a middleware in between it returns 411 Length Required.

Changed 4 months ago by twim

Attachment: client.go added

comment:11 in reply to:  9 Changed 4 months ago by twim

I'm confused now by your references to the flexible environment. I tried deploying reflect.go to the flexible environment by adding env: flex to app.yaml:
So it seems that reflect.go needs more extensive changes than just attachment:0001-Explicitly-set-Content-Length-to-zero-when-there-is-.patch​ in order to run in the flexible environment. Are you running something other than reflect.go on App Engine?

Sorry for this confusion. GAE flex is absolutely different environment with no 'lightweight restricted Go 1.6 runtime'. So one can run anything on there (in fact this is just Docker containers) thus enabling use of any reverse proxy. I run a reflector based on net/http/httputil.ReverseProxy. I've just bundled it and you can find it at https://github.com/nogoegst/reflector.

Changed 4 months ago by twim

Attachment: server.go added

Changed 4 months ago by dcf

Attachment: demo.go added

comment:12 Changed 4 months ago by dcf

Status: needs_reviewmerge_ready

Thanks to your example code, I was finally able to reproduce the 411 "Length Required" error, and I think I have traced the source of the problem. It comes down to http2 not treating body==http.NoBody the same as body==nil in Go 1.8. Even though http.NewRequest correctly special-cases *bytes.Reader, sets req.ContentLength = 0, and sets req.Body = http.NoBody (as noted in comment:9), the http2 code doesn't recognize http.NoBody as being special, and resets req.ContentLength = -1, which causes the Content-Length header to be omitted when it should be included. A change to make body==http.NoBody and body==nil have the same meaning in the http2 code is not due until Go 1.9.

The fix is attachment:0001-Explicitly-set-Content-Length-to-zero-when-there-is-.patch, which sets body = nil. Setting body = http.NoBody, which would seem to even more clearly signal an empty body, does not work, for the reasons explained above.

The problem occurs only when:

  • Using HTTP/2
  • Sending a 0-length body

In the case of App Engine, you additionally have to be:

  • Using the App Engine flexible environment (not the standard environment)
  • Domain fronting (as mentioned in comment:6)

Here is the go commit that added http.NoBody (it's the resolution of https://go-review.googlesource.com/c/31445/ from the ticket description). Notice how they expand r.Body == nil into r.Body == nil || r.Body == NoBody—but only for http code, not http2.

net/http: add NoBody, don't return nil from NewRequest on zero bodies

Here is a bug report about http2 not honoring http.NoBody, and the ensuing code change (that is not in Go 1.8):

https://github.com/golang/go/issues/18891
http2: make Transport treat http.NoBody like it were nil

The important diff is here, which causes actualContentLength to return 0 instead of -1 for http.NoBody. The value of 0 in turn causes shouldSendReqContentLength to return true instead of false.

I made a demo program that demonstrates all the possibilities. It allows you to represent an empty body as a 0-length *strings.Reader (the default), a nil pointer (-nil option), or http.NoBody (-nobody option). It additionally allows you to set a front domain with the -front option.

  • example.com (only nil or non-empty body works)
    $ ./demo https://example.com/ ""
    HTTP/2.0 411 Length Required
    $ ./demo -nil https://example.com/ ""
    HTTP/2.0 200 OK
    $ ./demo -nobody https://example.com/ ""
    HTTP/2.0 411 Length Required
    $ ./demo https://example.com/ "content"
    HTTP/2.0 200 OK
    
  • App Engine flexible environment without front domain (everything works)
    $ ./demo https://xxx.appspot.com/ ""
    HTTP/2.0 200 OK
    $ ./demo -nil https://xxx.appspot.com/ ""
    HTTP/2.0 200 OK
    $ ./demo -nobody https://xxx.appspot.com/ ""
    HTTP/2.0 200 OK
    $ ./demo https://xxx.appspot.com/ "content"
    HTTP/2.0 200 OK
    
  • App Engine flexible environment with front domain (only nil or non-empty body works)
    $ ./demo -front www.google.com https://xxx.appspot.com/ ""
    HTTP/2.0 411 Length Required
    $ ./demo -front www.google.com -nil https://xxx.appspot.com/ ""
    HTTP/2.0 200 OK
    $ ./demo -front www.google.com -nobody https://xxx.appspot.com/ ""
    HTTP/2.0 411 Length Required
    $ ./demo -front www.google.com https://xxx.appspot.com/ "content"
    HTTP/2.0 200 OK
    

You can get a lot of HTTP/2 debugging info by setting GODEBUG=http2debug=1 in the environment. Here we can see that the header "content-length" = "0" is not encoded when using a 0-length Reader, but it when using nil:

$ GODEBUG=http2debug=1 ./demo https://example.com/ ""
2017/07/26 11:13:53 http2: Transport failed to get client conn for example.com:443: http2: no cached connection was available
2017/07/26 11:13:54 http2: Transport creating client conn 0xc4200016c0 to 93.184.216.34:443
2017/07/26 11:13:54 http2: Transport encoding header ":authority" = "example.com"
2017/07/26 11:13:54 http2: Transport encoding header ":method" = "POST"
2017/07/26 11:13:54 http2: Transport encoding header ":path" = "/"
2017/07/26 11:13:54 http2: Transport encoding header ":scheme" = "https"
2017/07/26 11:13:54 http2: Transport received SETTINGS len=30, settings: HEADER_TABLE_SIZE=4096, MAX_CONCURRENT_STREAMS=100, INITIAL_WINDOW_SIZE=1048576, MAX_FRAME_SIZE=16384, MAX_HEADER_LIST_SIZE=16384
2017/07/26 11:13:54 http2: Transport encoding header "accept-encoding" = "gzip"
2017/07/26 11:13:54 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2017/07/26 11:13:54 Unhandled Setting: [HEADER_TABLE_SIZE = 4096]
2017/07/26 11:13:54 Unhandled Setting: [MAX_HEADER_LIST_SIZE = 16384]
2017/07/26 11:13:54 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=983041
2017/07/26 11:13:54 http2: Transport received SETTINGS flags=ACK len=0
2017/07/26 11:13:54 http2: Transport received HEADERS flags=END_HEADERS stream=1 len=19
HTTP/2.0 411 Length Required
$ GODEBUG=http2debug=1 ./demo -nil https://example.com/ ""
2017/07/26 11:14:02 http2: Transport failed to get client conn for example.com:443: http2: no cached connection was available
2017/07/26 11:14:03 http2: Transport creating client conn 0xc4200016c0 to 93.184.216.34:443
2017/07/26 11:14:03 http2: Transport encoding header ":authority" = "example.com"
2017/07/26 11:14:03 http2: Transport encoding header ":method" = "POST"
2017/07/26 11:14:03 http2: Transport encoding header ":path" = "/"
2017/07/26 11:14:03 http2: Transport encoding header ":scheme" = "https"
2017/07/26 11:14:03 http2: Transport encoding header "content-length" = "0"
2017/07/26 11:14:03 http2: Transport encoding header "accept-encoding" = "gzip"
2017/07/26 11:14:03 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2017/07/26 11:14:03 http2: Transport received SETTINGS len=30, settings: HEADER_TABLE_SIZE=4096, MAX_CONCURRENT_STREAMS=100, INITIAL_WINDOW_SIZE=1048576, MAX_FRAME_SIZE=16384, MAX_HEADER_LIST_SIZE=16384
2017/07/26 11:14:03 Unhandled Setting: [HEADER_TABLE_SIZE = 4096]
2017/07/26 11:14:03 Unhandled Setting: [MAX_HEADER_LIST_SIZE = 16384]
2017/07/26 11:14:03 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=983041
2017/07/26 11:14:03 http2: Transport received SETTINGS flags=ACK len=0
2017/07/26 11:14:03 http2: Transport received HEADERS flags=END_HEADERS stream=1 len=157
HTTP/2.0 200 OK

comment:13 Changed 4 months ago by dcf

Resolution: fixed
Status: merge_readyclosed

I merged your patch and made a new tag 0.28.

Thanks for your help in understanding this tricky issue.

comment:14 Changed 4 months ago by twim

This is just awesome, thanks for your extensive digging into net/http internals!
It turns out to be much more complicated - hiding complexity isn't easy.

Note: See TracTickets for help on using tickets.