From 4d76606fa269a1af5ab5eff3e9c50e7652cb134f Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 9 Aug 2017 22:44:14 +0900 Subject: [PATCH] Fix bug that forwarded for is not affected by proxy protocol --- integration-tests/nghttpx_http2_test.go | 43 +++++++++++++++++++++-- src/shrpx_client_handler.cc | 45 ++++++++++++++++--------- src/shrpx_client_handler.h | 3 ++ 3 files changed, 73 insertions(+), 18 deletions(-) diff --git a/integration-tests/nghttpx_http2_test.go b/integration-tests/nghttpx_http2_test.go index 4d4cb71c..9ec1c264 100644 --- a/integration-tests/nghttpx_http2_test.go +++ b/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() diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 14900616..bf81f715 100644 --- a/src/shrpx_client_handler.cc +++ b/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) { - // 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_; + } else if (!faddr_->accept_proxy_protocol && + !config->conn.upstream.accept_proxy_protocol) { + init_forwarded_for(family, 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(); } diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index bfe22024..37451d0c 100644 --- a/src/shrpx_client_handler.h +++ b/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();