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

Jump to bottom

net/http: Expect 100 continue header shouldn't be deleted unnecessarily in server.go #13893

Closed
harshavardhana opened this issue Jan 9, 2016 · 9 comments
Closed

net/http: Expect 100 continue header shouldn't be deleted unnecessarily in server.go #13893

harshavardhana opened this issue Jan 9, 2016 · 9 comments
Labels
FrozenDueToAge

Comments

@harshavardhana
Copy link
Contributor

Issue in net/http/server.go for Expect "100-continue" support.

               // Expect 100 Continue support
                req := w.req
                if req.expectsContinue() {
                        if req.ProtoAtLeast(1, 1) && req.ContentLength != 0 {
                                // Wrap the Body reader with one that replies on the connection
                                req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
                        }
                        req.Header.Del("Expect")
                } else if req.Header.get("Expect") != "" {
                        w.sendExpectationFailed()
                        return
                }

Incoming client headers.

2016-01-09 16:14:17,988 - Thread-6 - botocore.auth - DEBUG - CanonicalRequest:
PUT
/mybucket/filter-coffee.jpg

content-md5:7FgxPuugREsJH7jrMSpq8A==
content-type:image/jpeg
expect:100-continue
host:localhost:9000
user-agent:aws-cli/1.9.17 Python/2.7.6 Linux/3.13.0-38-generic botocore/1.3.17
x-amz-content-sha256:85d43d9100694f7a81ae286710321225b718776a902f51a538e716b51e2a3bf1
x-amz-date:20160109T104417Z

content-md5;content-type;expect;host;user-agent;x-amz-content-sha256;x-amz-date
85d43d9100694f7a81ae286710321225b718776a902f51a538e716b51e2a3bf1

While calculating signature of incoming headers from the client, there was as signature mismatch on the server side because server cannot generate the same canonical request which was sent over the wire.

The reason is because while all the headers are present Expect header is removed pro-actively https://github.com/golang/go/blob/master/src/net/http/server.go#L1455.

I don't see any reason to delete this header entry (I may be wrong please correct me here), since it is a value which might be useful for the top level caller to inspect and verify.

Following line fixes this problem.

$ git diff
diff --git a/src/net/http/server.go b/src/net/http/server.go
index ac7086c..19324d0 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1452,7 +1452,6 @@ func (c *conn) serve() {
                                // Wrap the Body reader with one that replies on the connection
                                req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
                        }
-                       req.Header.Del("Expect")
                } else if req.Header.get("Expect") != "" {
                        w.sendExpectationFailed()
                        return
@gopherbot
Copy link

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

@bradfitz
Copy link
Contributor

bradfitz commented Jan 9, 2016

I don't remember why that's there, and I can't think of a good reason either.

Can you do some archaeology and summarized the history for me? Go 1? Go 1.1, 1.2, etc?

@harshavardhana
Copy link
Contributor Author

Can you do some archaeology and summarized the history for me? Go 1? Go 1.1, 1.2, etc?

Sure let me do that.

@harshavardhana
Copy link
Contributor Author

The code base been there un-touched since 2011.

commit c7d16cc4118bf0db3e4268a7ab657577911999f4
Author: Brad Fitzpatrick <bradfitz@golang.org>
Date:   Wed Apr 13 14:09:04 2011 -0700

    http: flesh out server Expect handling + tests

    This mostly adds Expect 100-continue tests (from
    the perspective of server correctness) that were
    missing before.

    It also fixes a few missing cases that will
    probably never come up in practice, but it's nice
    to have handled correctly.

    Proper 100-continue client support remains a TODO.

    R=rsc, bradfitzwork
    CC=golang-dev
    https://golang.org/cl/4399044

+               // Expect 100 Continue support
+               req := w.req
+               if req.expectsContinue() {
+                       if req.ProtoAtLeast(1, 1) {
+                               // Wrap the Body reader with one that replies on the connection
+                               req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
+                       }
+                       if req.ContentLength == 0 {
+                               w.Header().Set("Connection", "close")
+                               w.WriteHeader(StatusBadRequest)
+                               break
+                       }
+                       req.Header.Del("Expect")
+               } else if req.Header.Get("Expect") != "" {
+                       // TODO(bradfitz): let ServeHTTP handlers handle
+                       // requests with non-standard expectation[s]? Seems
+                       // theoretical at best, and doesn't fit into the
+                       // current ServeHTTP model anyway.  We'd need to
+                       // make the ResponseWriter an optional
+                       // "ExpectReplier" interface or something.
+                       //
+                       // For now we'll just obey RFC 2616 14.20 which says
+                       // "If a server receives a request containing an
+                       // Expect field that includes an expectation-
+                       // extension that it does not support, it MUST
+                       // respond with a 417 (Expectation Failed) status."
+                       w.Header().Set("Connection", "close")
+                       w.WriteHeader(StatusExpectationFailed)
+                       break
+               }

@harshavardhana
Copy link
Contributor Author

  • go1
c7d16cc4 src/pkg/http/server.go     (Brad Fitzpatrick           2011-04-13 14:09:04 -0700  626)                         req.Header.Del("Expect")
  • go11
c7d16cc4 src/pkg/http/server.go     (Brad Fitzpatrick           2011-04-13 14:09:04 -0700 1085)                         req.Header.Del("Expect")
  • go12
c7d16cc4 src/pkg/http/server.go     (Brad Fitzpatrick           2011-04-13 14:09:04 -0700 1156)                         req.Header.Del("Expect")

.. Rest is history... :-)

@bradfitz
Copy link
Contributor

bradfitz commented Jan 9, 2016

Thanks. Indeed, there doesn't seem like any particular reason it was deleted. I think there was just some general design confusion back then about which fields we kept in Header and which we promoted to be more first-class. But we shouldn't hide this information.

@harshavardhana
Copy link
Contributor Author

Is this possible to get this fix in go1.5.3? or go1.6 ? - should i be sending a fix to those branches as well?

@bradfitz
Copy link
Contributor

It will be in 1.6 but not 1.5.x.

@harshavardhana
Copy link
Contributor Author

It will be in 1.6 but not 1.5.x.

Okay cool

@harshavardhana harshavardhana mentioned this issue Jan 10, 2016
Closed
@golang golang locked and limited conversation to collaborators Jan 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bradfitz @harshavardhana @gopherbot