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

net/http/httputil: NewSingleHostReverseProxy overwrites Director header transformations when hop headers occur #19706

Closed
CSEMike opened this issue Mar 25, 2017 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@CSEMike
Copy link

CSEMike commented Mar 25, 2017

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

go version go1.8.typealias linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/usr/local/google/home/piatek/gopath"
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build329872370=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Modified http headers in an httputil.ReverseProxy Director.

What did you expect to see?

The modification applied to the outgoing request.

What did you see instead?

The modifications are reverted when copying hop headers.

As the docs describe, the Director field of an httputil.ReverseProxy is for modifying the request to be sent by the proxy. Headers are a natural thing to change in a Director. This works fine, unless there are hop by hop headers, in which case the ReverseProxy will undo the edits before sending the request.

The problems are here and here:
https://golang.org/src/net/http/httputil/reverseproxy.go#L173
https://golang.org/src/net/http/httputil/reverseproxy.go#L188

The ReverseProxy makes a shallow copy of the request to be sent to the origin. Then, the Director is called, possibly modifying headers. Then, if the ReverseProxy itself decides to modify headers, it copies them from the original request, req.Header, rather than the request passed to the Director, outreq.

My view is that a fix will require an unconditional deep copy of outreq. The existence of the Director API in ReverseProxy seems in conflict with the http.Handler API and a shallow copy. The http.Handler docs prohibit modification of the *http.Request passed to ServeHTTP. Yet, the Director API offers for modification a shallow copy of the *http.Request given to ReverseProxy via ServeHTTP.

The program below reproduces the failure precisely.

package proxy_test

import (
  "net/http"
  "net/http/httptest"
  "net/http/httputil"
  "net/url"
  "testing"
)

func copyHeader(dst, src http.Header) {
  for k, vv := range src {
    for _, v := range vv {
      dst.Add(k, v)
    }
  }
}

func TestHeaders(t *testing.T) {
  // An origin, which should receive a request modified by our reverse proxy director.
  srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    if got, want := r.Header.Get("X-Want-Dropped"), ""; got != want {
      t.Errorf("%s: origin request header: X-Want-Dropped: got: %q want: %q", r.URL.Path, got, want)
    }
  }))
  defer srv.Close()
  parsed, err := url.Parse(srv.URL)
  if err != nil {
    t.Fatalf("url.Parse(%q): %v", srv.URL, err)
  }
  // The reverse proxy.
  rp := httputil.NewSingleHostReverseProxy(parsed)
  origDirector := rp.Director
  rp.Director = func(r *http.Request) {
    origDirector(r)
    // Copy header, since r.Header is a shallow copy of the original request
    // headers, and  calls rp.Director for an http.Handler, and http.Handlers
    // aren't permitted to modify the original request.
    h := make(http.Header)
    copyHeader(h, r.Header)
    r.Header = h
    r.Header.Del("X-Want-Dropped")
  }
  rpSrv := httptest.NewServer(rp)
  defer rpSrv.Close()

  tests := []struct {
    label, path      string
    connectionHeader string
  }{
    {"header is dropped with empty connection header", "/without_connection_header", ""},
    {"header is dropped with Connection: close", "/with_connection_header", "close"}, // bug repro.
  }
  for _, test := range tests {
    func() {
      req, err := http.NewRequest(http.MethodGet, rpSrv.URL+test.path, nil)
      if err != nil {
        t.Errorf("%s: http.NewRequest: %v", test.label, err)
        return
      }
      req.Header.Set("X-Want-Dropped", "1")
      if test.connectionHeader != "" {
        req.Header.Set("Connection", test.connectionHeader)
      }
      resp, err := http.DefaultTransport.RoundTrip(req)
      if err != nil {
        t.Errorf("%s: RoundTrip: %v", test.label, err)
        return
      }
      resp.Body.Close()
    }()
  }
}
@CSEMike CSEMike changed the title httputil.NewSingleHostReverseProxy overwrites Director header transformations when hop headers occur net: httputil.NewSingleHostReverseProxy overwrites Director header transformations when hop headers occur Mar 25, 2017
@odeke-em odeke-em changed the title net: httputil.NewSingleHostReverseProxy overwrites Director header transformations when hop headers occur net/http/httputil: NewSingleHostReverseProxy overwrites Director header transformations when hop headers occur Mar 25, 2017
@odeke-em
Copy link
Member

/cc @bradfitz

@bradfitz bradfitz added this to the Go1.9Maybe milestone May 19, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 19, 2017
@bradfitz
Copy link
Contributor

My view is that a fix will require an unconditional deep copy of outreq.

That happened in 3c0f69a

And confirmed that your test passes now:

bradfitz@gdev:~/src/issue_19706$ go test -v
=== RUN   TestHeaders
--- PASS: TestHeaders (0.00s)
PASS
ok      issue_19706     0.005s

@golang golang locked and limited conversation to collaborators Jun 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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