Merge pull request #980 from nghttp2/fix-forwarded-for-with-proxyprotocol

Fix bug that forwarded for is not affected by proxy protocol

integration-tests/nghttpx_http2_test.go

@@ -1113,14 +1113,45 @@ func TestH2H1Upgrade(t *testing.T) {
 	}
 }
 
+// TestH2H1ProxyProtocolV1ForwardedForObfuscated tests that Forwarded
+// header field includes obfuscated address even if PROXY protocol
+// version 1 containing TCP4 entry is accepted.
+func TestH2H1ProxyProtocolV1ForwardedForObfuscated(t *testing.T) {
+	pattern := fmt.Sprintf(`^for=_[^;]+$`)
+	validFwd := regexp.MustCompile(pattern)
+	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for", "--add-forwarded=for", "--forwarded-for=obfuscated"}, t, func(w http.ResponseWriter, r *http.Request) {
+		if got := r.Header.Get("Forwarded"); !validFwd.MatchString(got) {
+			t.Errorf("Forwarded: %v; want pattern %v", got, pattern)
+		}
+	})
+	defer st.Close()
+
+	st.conn.Write([]byte("PROXY TCP4 192.168.0.2 192.168.0.100 12345 8080\r\n"))
+
+	res, err := st.http2(requestParam{
+		name: "TestH2H1ProxyProtocolV1ForwardedForObfuscated",
+	})
+
+	if err != nil {
+		t.Fatalf("Error st.http2() = %v", err)
+	}
+
+	if got, want := res.status, 200; got != want {
+		t.Errorf("res.status: %v; want %v", got, want)
+	}
+}
+
 // TestH2H1ProxyProtocolV1TCP4 tests PROXY protocol version 1
 // containing TCP4 entry is accepted and X-Forwarded-For contains
 // advertised src address.
 func TestH2H1ProxyProtocolV1TCP4(t *testing.T) {
-	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, func(w http.ResponseWriter, r *http.Request) {
+	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for", "--add-forwarded=for", "--forwarded-for=ip"}, t, func(w http.ResponseWriter, r *http.Request) {
 		if got, want := r.Header.Get("X-Forwarded-For"), "192.168.0.2"; got != want {
 			t.Errorf("X-Forwarded-For: %v; want %v", got, want)
 		}
+		if got, want := r.Header.Get("Forwarded"), "for=192.168.0.2"; got != want {
+			t.Errorf("Forwarded: %v; want %v", got, want)
+		}
 	})
 	defer st.Close()
 
@@ -1143,10 +1174,13 @@ func TestH2H1ProxyProtocolV1TCP4(t *testing.T) {
 // containing TCP6 entry is accepted and X-Forwarded-For contains
 // advertised src address.
 func TestH2H1ProxyProtocolV1TCP6(t *testing.T) {
-	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, func(w http.ResponseWriter, r *http.Request) {
+	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for", "--add-forwarded=for", "--forwarded-for=ip"}, t, func(w http.ResponseWriter, r *http.Request) {
 		if got, want := r.Header.Get("X-Forwarded-For"), "2001:0db8:85a3:0000:0000:8a2e:0370:7334"; got != want {
 			t.Errorf("X-Forwarded-For: %v; want %v", got, want)
 		}
+		if got, want := r.Header.Get("Forwarded"), `for="[2001:0db8:85a3:0000:0000:8a2e:0370:7334]"`; got != want {
+			t.Errorf("Forwarded: %v; want %v", got, want)
+		}
 	})
 	defer st.Close()
 
@@ -1168,10 +1202,13 @@ func TestH2H1ProxyProtocolV1TCP6(t *testing.T) {
 // TestH2H1ProxyProtocolV1Unknown tests PROXY protocol version 1
 // containing UNKNOWN entry is accepted.
 func TestH2H1ProxyProtocolV1Unknown(t *testing.T) {
-	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, func(w http.ResponseWriter, r *http.Request) {
+	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for", "--add-forwarded=for", "--forwarded-for=ip"}, t, func(w http.ResponseWriter, r *http.Request) {
 		if got, notWant := r.Header.Get("X-Forwarded-For"), "192.168.0.2"; got == notWant {
 			t.Errorf("X-Forwarded-For: %v")
 		}
+		if got, notWant := r.Header.Get("Forwarded"), "for=192.168.0.2"; got == notWant {
+			t.Errorf("Forwarded: %v")
+		}
 	})
 	defer st.Close()
 

src/shrpx_client_handler.cc

@@ -445,25 +445,32 @@ ClientHandler::ClientHandler(Worker *worker, int fd, SSL *ssl,
       *p = '\0';
 
       forwarded_for_ = StringRef{buf.base, p};
-    } else if (family == AF_INET6) {
+    } else if (!faddr_->accept_proxy_protocol &&
-      // 2 for '[' and ']'
+               !config->conn.upstream.accept_proxy_protocol) {
-      auto len = 2 + ipaddr_.size();
+      init_forwarded_for(family, ipaddr_);
-      // 1 for terminating NUL.
-      auto buf = make_byte_ref(balloc_, len + 1);
-      auto p = buf.base;
-      *p++ = '[';
-      p = std::copy(std::begin(ipaddr_), std::end(ipaddr_), p);
-      *p++ = ']';
-      *p = '\0';
-
-      forwarded_for_ = StringRef{buf.base, p};
-    } else {
-      // family == AF_INET or family == AF_UNIX
-      forwarded_for_ = ipaddr_;
     }
   }
 }
 
+void ClientHandler::init_forwarded_for(int family, const StringRef &ipaddr) {
+  if (family == AF_INET6) {
+    // 2 for '[' and ']'
+    auto len = 2 + ipaddr.size();
+    // 1 for terminating NUL.
+    auto buf = make_byte_ref(balloc_, len + 1);
+    auto p = buf.base;
+    *p++ = '[';
+    p = std::copy(std::begin(ipaddr), std::end(ipaddr), p);
+    *p++ = ']';
+    *p = '\0';
+
+    forwarded_for_ = StringRef{buf.base, p};
+  } else {
+    // family == AF_INET or family == AF_UNIX
+    forwarded_for_ = ipaddr;
+  }
+}
+
 void ClientHandler::setup_upstream_io_callback() {
   if (conn_.tls.ssl) {
     conn_.prepare_server_handshake();
@@ -1459,6 +1466,14 @@ int ClientHandler::proxy_protocol_read() {
                      << " bytes read";
   }
 
+  auto config = get_config();
+  auto &fwdconf = config->http.forwarded;
+
+  if ((fwdconf.params & FORWARDED_FOR) &&
+      fwdconf.for_node_type == FORWARDED_NODE_IP) {
+    init_forwarded_for(family, ipaddr_);
+  }
+
   return on_proxy_protocol_finish();
 }
 

src/shrpx_client_handler.h

@@ -125,6 +125,9 @@ public:
 
   Worker *get_worker() const;
 
+  // Initializes forwarded_for_.
+  void init_forwarded_for(int family, const StringRef &ipaddr);
+
   using ReadBuf = DefaultMemchunkBuffer;
 
   ReadBuf *get_rb();