Restore Forwarded header in reverse proxy Rewrite mode

jobs/gorouter/spec

@@ -386,11 +386,12 @@ properties:
     description: "If set gorouter reuses backend connection for requests expecting 100-Continue"
     default: false
   router.force_forwarded_proto_https:
-    description: "Enables setting X-Forwarded-Proto header if SSL termination happened upstream and incorrectly set the header value. When this property is set to true gorouter sets the header X-Forwarded-Proto to https. When this value set to false, gorouter set the header X-Forwarded-Proto to the protocol of the incoming request"
+    description: "Enables setting X-Forwarded-Proto header if SSL termination happened upstream and incorrectly set the header value. When this property is set to true gorouter sets the header X-Forwarded-Proto to https. When this value set to false, gorouter set the header X-Forwarded-Proto to the protocol of the incoming request. A client-supplied RFC 7239 Forwarded header is replaced with the client IP and scheme Gorouter observed, since backends may prefer its proto= parameter over X-Forwarded-Proto"
     default: false
   router.sanitize_forwarded_proto:
     description: |
       When true, Gorouter will strip the X-Forwarded-Proto header when present in client request and set it to the scheme of the request.
+      A client-supplied RFC 7239 Forwarded header is replaced with the client IP and scheme Gorouter observed, since backends may prefer its proto= parameter over X-Forwarded-Proto.
       When false, Gorouter will pass through the value of this header.
       When force_forwarded_proto_https: true, this property will be ignored.
       Otherwise,  we recommend setting the property to true if Gorouter is the first component to terminate TLS, and set to false when your load balancer is terminating TLS and setting the X-Forwarded-Proto header.

src/code.cloudfoundry.org/gorouter/handlers/x_forwarded_proto.go

@@ -1,7 +1,9 @@
 package handlers
 
 import (
+	"net"
 	"net/http"
+	"strings"
 )
 
 type XForwardedProto struct {
@@ -24,7 +26,48 @@ func (h *XForwardedProto) ServeHTTP(rw http.ResponseWriter, r *http.Request, nex
 			}
 			newReq.Header.Set("X-Forwarded-Proto", scheme)
 		}
+		// RFC 7239 carries the same claim in its proto parameter, and backends
+		// that read Forwarded tend to prefer it over X-Forwarded-Proto, so it
+		// needs sanitizing too. This middleware has no trusted source for the
+		// parameters the client sent, so replace the entire field with the
+		// values gorouter can establish for this hop. Test for presence, not
+		// content: an empty field value is still a field the client set.
+		if (h.ForceForwardedProtoHttps || h.SanitizeForwardedProto) && len(newReq.Header.Values("Forwarded")) > 0 {
+			newReq.Header.Set("Forwarded", forwardedElement(newReq))
+		}
 	}
 
 	next(rw, newReq)
 }
+
+// forwardedElement renders the RFC 7239 element gorouter can vouch for: the
+// immediate peer of the connection and the scheme sanitization settled on.
+// The peer is whoever connected to gorouter, which behind a load balancer is
+// the load balancer rather than the end client, so it is the hop gorouter can
+// establish rather than necessarily the original client. It is the same
+// immediate-peer address added to X-Forwarded-For. An app that gives Forwarded
+// precedence will not consult X-Forwarded-For, so leaving out for would
+// downgrade the address it sees to gorouter's own.
+//
+// host is left out on purpose. An app that finds no host parameter can fall
+// back to the Host header, which should be the value gorouter routed on.
+func forwardedElement(r *http.Request) string {
+	element := "proto=" + r.Header.Get("X-Forwarded-Proto")
+
+	host, _, err := net.SplitHostPort(r.RemoteAddr)
+	if err != nil {
+		return element
+	}
+	// Only an IP literal is a valid nodename. Anything else, a hostname or an
+	// address carrying an IPv6 zone, is dropped rather than rendered invalid.
+	if net.ParseIP(host) == nil {
+		return element
+	}
+	// Classify on the text, not on the parsed address: an IPv4-mapped address
+	// such as ::ffff:192.0.2.1 parses as IPv4 but is still written with colons,
+	// which a token cannot hold. Colons mean bracket and quote.
+	if strings.Contains(host, ":") {
+		return `for="[` + host + `]";` + element
+	}
+	return "for=" + host + ";" + element
+}

src/code.cloudfoundry.org/gorouter/handlers/x_forwarded_proto_test.go

@@ -58,6 +58,12 @@ var _ = Describe("X-Forwarded-Proto", func() {
 			req.Header.Set("X-Forwarded-Proto", "https")
 			Expect(processAndGetUpdatedHeader(handler)).To(Equal("https"))
 		})
+
+		It("does not sanitize Forwarded", func() {
+			req.Header.Set("Forwarded", "proto=https")
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Get("Forwarded")).To(Equal("proto=https"))
+		})
 	})
 
 	Context("when the ForceForwardedProtoHttps is true", func() {
@@ -80,6 +86,13 @@ var _ = Describe("X-Forwarded-Proto", func() {
 			Expect(processAndGetUpdatedHeader(handler)).To(Equal("https"))
 			Expect(nextCalled).To(BeTrue())
 		})
+
+		It("replaces a client-supplied Forwarded header", func() {
+			req.RemoteAddr = "10.0.0.1:4711"
+			req.Header.Set("Forwarded", "proto=http")
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Values("Forwarded")).To(Equal([]string{"for=10.0.0.1;proto=https"}))
+		})
 	})
 
 	Context("when the SanitizeForwardedProto is true", func() {
@@ -109,6 +122,49 @@ var _ = Describe("X-Forwarded-Proto", func() {
 			Expect(processAndGetUpdatedHeader(handler)).To(Equal("http"))
 			Expect(nextCalled).To(BeTrue())
 		})
+
+		It("replaces a Forwarded header claiming https on a cleartext connection", func() {
+			req.RemoteAddr = "10.0.0.1:4711"
+			req.Header.Set("Forwarded", "for=1.2.3.4;proto=https")
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Values("Forwarded")).To(Equal([]string{"for=10.0.0.1;proto=http"}))
+		})
+
+		It("replaces every value the client sent, not just the first", func() {
+			req.RemoteAddr = "10.0.0.1:4711"
+			req.Header.Add("Forwarded", "proto=https")
+			req.Header.Add("Forwarded", "proto=https")
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Values("Forwarded")).To(Equal([]string{"for=10.0.0.1;proto=http"}))
+		})
+
+		DescribeTable("renders the peer address as an RFC 7239 node",
+			func(remoteAddr, expected string) {
+				req.RemoteAddr = remoteAddr
+				req.Header.Set("Forwarded", "for=1.2.3.4")
+				processAndGetUpdatedHeader(handler)
+				Expect(req.Header.Get("Forwarded")).To(Equal(expected))
+			},
+			Entry("IPv4 is a token", "10.0.0.1:4711", "for=10.0.0.1;proto=http"),
+			Entry("IPv6 is bracketed and quoted", "[2001:db8::1]:4711", `for="[2001:db8::1]";proto=http`),
+			Entry("IPv4-mapped IPv6 is bracketed and quoted, since it is written with colons",
+				"[::ffff:192.0.2.1]:4711", `for="[::ffff:192.0.2.1]";proto=http`),
+			Entry("an IPv6 zone is not a valid nodename, so for is left out", "[fe80::1%eth0]:4711", "proto=http"),
+			Entry("a hostname is not a valid nodename, so for is left out", "lb.example.com:4711", "proto=http"),
+			Entry("an address that does not split leaves out for", "not-an-address", "proto=http"),
+		)
+
+		It("replaces a Forwarded header the client sent empty", func() {
+			req.RemoteAddr = "10.0.0.1:4711"
+			req.Header["Forwarded"] = []string{""}
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Values("Forwarded")).To(Equal([]string{"for=10.0.0.1;proto=http"}))
+		})
+
+		It("does not add a Forwarded header when the client did not send one", func() {
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header).NotTo(HaveKey("Forwarded"))
+		})
 	})
 
 	Context("when the client does not provide an X-Forwarded-Proto header with every property to false", func() {
@@ -136,5 +192,11 @@ var _ = Describe("X-Forwarded-Proto", func() {
 			Expect(processAndGetUpdatedHeader(handler)).To(Equal("http"))
 			Expect(nextCalled).To(BeTrue())
 		})
+
+		It("passes a client-supplied Forwarded header through", func() {
+			req.Header.Set("Forwarded", "for=1.2.3.4;proto=https")
+			processAndGetUpdatedHeader(handler)
+			Expect(req.Header.Get("Forwarded")).To(Equal("for=1.2.3.4;proto=https"))
+		})
 	})
 })

src/code.cloudfoundry.org/gorouter/proxy/proxy.go

@@ -142,11 +142,19 @@ func NewProxy(
 	rproxy := &httputil.ReverseProxy{
 		Rewrite: func(r *httputil.ProxyRequest) {
 			p.setupProxyRequest(r.Out)
-			// Rewrite mode strips X-Forwarded-* from r.Out before calling this
-			// function. Restore them to replicate the behavior Director had:
+			// Rewrite mode strips the Forwarded and X-Forwarded-* headers from
+			// r.Out before calling this function. Restore them to replicate the
+			// behavior Director had:
+			// - Forwarded: pass the client-supplied value(s) through verbatim.
+			//   Assigned directly rather than via Set so multiple field values
+			//   and their order survive, as RFC 7239 allows the list to span
+			//   several of them.
 			// - X-Forwarded-Proto: copy the value already set by the XForwardedProto middleware.
 			// - X-Forwarded-Host: preserve whatever the client/middleware set.
 			// - X-Forwarded-For: append the client IP from r.In.RemoteAddr.
+			if fwd := r.In.Header.Values("Forwarded"); len(fwd) > 0 {
+				r.Out.Header["Forwarded"] = append([]string(nil), fwd...)
+			}
 			if proto := r.In.Header.Get("X-Forwarded-Proto"); proto != "" {
 				r.Out.Header.Set("X-Forwarded-Proto", proto)
 			}

src/code.cloudfoundry.org/gorouter/proxy/proxy_test.go

@@ -498,6 +498,24 @@ var _ = Describe("Proxy", func() {
 			})
 		})
 
+		Describe("Forwarded", func() {
+			It("passes a client-supplied Forwarded header through verbatim", func() {
+				req.Header.Set("Forwarded", "for=1.2.3.4;proto=https;host=example.com")
+				Expect(getProxiedHeaders(req).Get("Forwarded")).To(Equal("for=1.2.3.4;proto=https;host=example.com"))
+			})
+
+			It("preserves multiple Forwarded header values", func() {
+				req.Header.Add("Forwarded", "for=1.2.3.4")
+				req.Header.Add("Forwarded", "for=5.6.7.8")
+				Expect(getProxiedHeaders(req).Values("Forwarded")).To(Equal([]string{"for=1.2.3.4", "for=5.6.7.8"}))
+			})
+
+			It("does not add a Forwarded header when the client does not send one", func() {
+				_, ok := getProxiedHeaders(req)["Forwarded"]
+				Expect(ok).To(BeFalse())
+			})
+		})
+
 		Describe("X-Forwarded-Host", func() {
 			Context("for expect-100-continue requests", func() {
 				It("preserves the X-Forwarded-Host header", func() {