From ce53bd239e7a139db1409b00b409b04e295072d9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 6 Sep 2015 18:39:32 +0900 Subject: [PATCH 1/6] nghttpx: Implement PROXY protocol version 1 Use --accept-proxy-protocol to enable PROXY protocol handling --- gennghttpxfun.py | 1 + src/shrpx.cc | 5 + src/shrpx_client_handler.cc | 191 ++++++++++++++++++++++++++++++++++++ src/shrpx_client_handler.h | 4 + src/shrpx_config.cc | 10 ++ src/shrpx_config.h | 2 + src/util.cc | 6 +- src/util.h | 2 + 8 files changed, 220 insertions(+), 1 deletion(-) diff --git a/gennghttpxfun.py b/gennghttpxfun.py index 3114137d..eb95026d 100755 --- a/gennghttpxfun.py +++ b/gennghttpxfun.py @@ -100,6 +100,7 @@ OPTIONS = [ "tls-ticket-key-memcached-max-fail", "request-phase-file", "response-phase-file", + "accept-proxy-protocol", "conf", ] diff --git a/src/shrpx.cc b/src/shrpx.cc index 9aac0cde..fd918bd3 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -1915,6 +1915,7 @@ int main(int argc, char **argv) { 90}, {SHRPX_OPT_REQUEST_PHASE_FILE, required_argument, &flag, 91}, {SHRPX_OPT_RESPONSE_PHASE_FILE, required_argument, &flag, 92}, + {SHRPX_OPT_ACCEPT_PROXY_PROTOCOL, no_argument, &flag, 93}, {nullptr, 0, nullptr, 0}}; int option_index = 0; @@ -2316,6 +2317,10 @@ int main(int argc, char **argv) { // --response-phase-file cmdcfgs.emplace_back(SHRPX_OPT_RESPONSE_PHASE_FILE, optarg); break; + case 93: + // --accept-proxy-protocol + cmdcfgs.emplace_back(SHRPX_OPT_ACCEPT_PROXY_PROTOCOL, "yes"); + break; default: break; } diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 87c1659e..7d7f7a52 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -382,6 +382,16 @@ ClientHandler::ClientHandler(Worker *worker, int fd, SSL *ssl, conn_.rlimit.startw(); ev_timer_again(conn_.loop, &conn_.rt); + if (get_config()->accept_proxy_protocol) { + read_ = write_ = &ClientHandler::read_clear; + on_read_ = &ClientHandler::proxy_protocol_read; + on_write_ = &ClientHandler::upstream_noop; + } else { + setup_upstream_io_callback(); + } +} + +void ClientHandler::setup_upstream_io_callback() { if (conn_.tls.ssl) { conn_.prepare_server_handshake(); read_ = write_ = &ClientHandler::tls_handshake; @@ -829,4 +839,185 @@ ev_io *ClientHandler::get_wev() { return &conn_.wev; } Worker *ClientHandler::get_worker() const { return worker_; } +namespace { +ssize_t parse_proxy_line_port(const uint8_t *first, const uint8_t *last) { + auto p = first; + int32_t port = 0; + + for (; p != last && util::isDigit(*p); ++p) { + port *= 10; + port += *p - '0'; + + if (port > 65535) { + return -1; + } + } + + return p - first; +} +} // namespace + +int ClientHandler::proxy_protocol_read() { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol: Started"; + } + + auto first = rb_.pos; + + // NULL character really destroys getaddrinfo function. We won't + // expect it in PROXY protocol line, so find it here. + auto chrs = std::array{{'\r', '\0'}}; + auto end = std::find_first_of(rb_.pos, rb_.pos + rb_.rleft(), + std::begin(chrs), std::end(chrs)); + if (end + 2 > rb_.pos + rb_.rleft() || *end == '\0' || end[1] != '\n') { + return -1; + } + + constexpr const char HEADER[] = "PROXY "; + + if (rb_.rleft() < str_size(HEADER)) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: PROXY version 1 ID not found"; + } + return -1; + } + + if (!util::streq_l(HEADER, rb_.pos, str_size(HEADER))) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Bad PROXY protocol version 1 ID"; + } + return -1; + } + + rb_.drain(str_size(HEADER)); + + int family; + + if (rb_.pos[0] == 'T') { + if (rb_.rleft() < 5) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: INET protocol family not found"; + } + return -1; + } + + if (util::streq_l("TCP4 ", rb_.pos, 5)) { + family = AF_INET; + } else if (util::streq_l("TCP6 ", rb_.pos, 5)) { + family = AF_INET6; + } else { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family"; + } + return -1; + } + + rb_.drain(5); + } else { + if (rb_.rleft() < 7) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: INET protocol family not found"; + } + return -1; + } + if (!util::streq_l("UNKNOWN", rb_.pos, 7)) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family"; + } + return -1; + } + + return 0; + } + + // source address + auto token_end = std::find(rb_.pos, end, ' '); + if (token_end == end) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Source address not found"; + } + return -1; + } + + *token_end = '\0'; + if (!util::numeric_host(reinterpret_cast(rb_.pos), family)) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Invalid source address"; + } + return -1; + } + + auto src_addr = rb_.pos; + auto src_addrlen = token_end - rb_.pos; + + rb_.drain(token_end - rb_.pos + 1); + + // destination address + token_end = std::find(rb_.pos, end, ' '); + if (token_end == end) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Destination address not found"; + } + return -1; + } + + *token_end = '\0'; + if (!util::numeric_host(reinterpret_cast(rb_.pos), family)) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Invalid destination address"; + } + return -1; + } + + // Currently we don't use destination address + + rb_.drain(token_end - rb_.pos + 1); + + // source port + auto n = parse_proxy_line_port(rb_.pos, end); + if (n <= 0 || *(rb_.pos + n) != ' ') { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Invalid source port"; + } + return -1; + } + + rb_.pos[n] = '\0'; + auto src_port = rb_.pos; + auto src_portlen = n; + + rb_.drain(n + 1); + + // destination port + n = parse_proxy_line_port(rb_.pos, end); + if (n <= 0 || rb_.pos + n != end) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Invalid destination port"; + } + return -1; + } + + // Currently we don't use destination port + + rb_.drain(end + 2 - rb_.pos); + + ipaddr_.assign(src_addr, src_addr + src_addrlen); + port_.assign(src_port, src_port + src_portlen); + + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Finished, " << (rb_.pos - first) + << " bytes read"; + } + + setup_upstream_io_callback(); + + // Run on_read to process data left in buffer since they are not + // notified further + if (on_read() != 0) { + return -1; + } + + return 0; +} + } // namespace shrpx diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index 4d4ccd9c..45ef7a20 100644 --- a/src/shrpx_client_handler.h +++ b/src/shrpx_client_handler.h @@ -71,6 +71,8 @@ public: int upstream_http1_connhd_read(); int upstream_write(); + int proxy_protocol_read(); + // Performs I/O operation. Internally calls on_read()/on_write(). int do_read(); int do_write(); @@ -130,6 +132,8 @@ public: void signal_write(); ev_io *get_wev(); + void setup_upstream_io_callback(); + private: Connection conn_; ev_timer reneg_shutdown_timer_; diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index ce2927b7..fb2d34ef 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -625,6 +625,7 @@ void parse_mapping(const DownstreamAddr &addr, const char *src) { // generated by gennghttpxfun.py enum { + SHRPX_OPTID_ACCEPT_PROXY_PROTOCOL, SHRPX_OPTID_ACCESSLOG_FILE, SHRPX_OPTID_ACCESSLOG_FORMAT, SHRPX_OPTID_ACCESSLOG_SYSLOG, @@ -1099,6 +1100,11 @@ int option_lookup_token(const char *name, size_t namelen) { return SHRPX_OPTID_BACKEND_TLS_SNI_FIELD; } break; + case 'l': + if (util::strieq_l("accept-proxy-protoco", name, 20)) { + return SHRPX_OPTID_ACCEPT_PROXY_PROTOCOL; + } + break; case 'r': if (util::strieq_l("tls-ticket-key-ciphe", name, 20)) { return SHRPX_OPTID_TLS_TICKET_KEY_CIPHER; @@ -1968,6 +1974,10 @@ int parse_config(const char *opt, const char *optarg, LOG(WARN) << opt << ": ignored because mruby support is disabled at build time."; #endif // !HAVE_MRUBY + return 0; + case SHRPX_OPTID_ACCEPT_PROXY_PROTOCOL: + mod_config()->accept_proxy_protocol = util::strieq(optarg, "yes"); + return 0; case SHRPX_OPTID_CONF: LOG(WARN) << "conf: ignored"; diff --git a/src/shrpx_config.h b/src/shrpx_config.h index 2bbb9647..3da46dee 100644 --- a/src/shrpx_config.h +++ b/src/shrpx_config.h @@ -185,6 +185,7 @@ constexpr char SHRPX_OPT_TLS_TICKET_KEY_MEMCACHED_MAX_FAIL[] = "tls-ticket-key-memcached-max-fail"; constexpr char SHRPX_OPT_REQUEST_PHASE_FILE[] = "request-phase-file"; constexpr char SHRPX_OPT_RESPONSE_PHASE_FILE[] = "response-phase-file"; +constexpr char SHRPX_OPT_ACCEPT_PROXY_PROTOCOL[] = "accept-proxy-protocol"; union sockaddr_union { sockaddr_storage storage; @@ -409,6 +410,7 @@ struct Config { bool no_ocsp; // true if --tls-ticket-key-cipher is used bool tls_ticket_key_cipher_given; + bool accept_proxy_protocol; }; const Config *get_config(); diff --git a/src/util.cc b/src/util.cc index bfa292a8..9b709dbe 100644 --- a/src/util.cc +++ b/src/util.cc @@ -636,9 +636,13 @@ void write_uri_field(std::ostream &o, const char *uri, const http_parser_url &u, } bool numeric_host(const char *hostname) { + return numeric_host(hostname, AF_UNSPEC); +} + +bool numeric_host(const char *hostname, int family) { struct addrinfo *res; struct addrinfo hints {}; - hints.ai_family = AF_UNSPEC; + hints.ai_family = family; hints.ai_flags = AI_NUMERICHOST; if (getaddrinfo(hostname, nullptr, &hints, &res)) { return false; diff --git a/src/util.h b/src/util.h index 6463d19a..ab039235 100644 --- a/src/util.h +++ b/src/util.h @@ -512,6 +512,8 @@ void write_uri_field(std::ostream &o, const char *uri, const http_parser_url &u, bool numeric_host(const char *hostname); +bool numeric_host(const char *hostname, int family); + // Returns numeric address string of |addr|. If getnameinfo() is // failed, "unknown" is returned. std::string numeric_name(const struct sockaddr *sa, socklen_t salen); From d05b77b36ce01ed025c8cdf47e87936aaba31078 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 6 Sep 2015 21:44:45 +0900 Subject: [PATCH 2/6] nghttpx: More logging for PROXY protocol handling --- src/shrpx_client_handler.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 7d7f7a52..3bdb6123 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -870,6 +870,9 @@ int ClientHandler::proxy_protocol_read() { auto end = std::find_first_of(rb_.pos, rb_.pos + rb_.rleft(), std::begin(chrs), std::end(chrs)); if (end + 2 > rb_.pos + rb_.rleft() || *end == '\0' || end[1] != '\n') { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: No ending CR LF sequence found"; + } return -1; } From a1bb48770c939dcd21951545f674af098ae60c69 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 6 Sep 2015 23:11:07 +0900 Subject: [PATCH 3/6] nghttpx: Add tests for PROXY protocol handling --- integration-tests/nghttpx_http2_test.go | 321 ++++++++++++++++++++++++ src/shrpx_client_handler.cc | 32 ++- src/shrpx_client_handler.h | 1 + 3 files changed, 341 insertions(+), 13 deletions(-) diff --git a/integration-tests/nghttpx_http2_test.go b/integration-tests/nghttpx_http2_test.go index 41c527f7..fe1fd37a 100644 --- a/integration-tests/nghttpx_http2_test.go +++ b/integration-tests/nghttpx_http2_test.go @@ -787,6 +787,327 @@ func TestH2H1Upgrade(t *testing.T) { } } +// 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) { + 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) + } + }) + 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: "TestH2H1ProxyProtocolV1TCP4", + }) + + 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) + } +} + +// TestH2H1ProxyProtocolV1TCP6 tests PROXY protocol version 1 +// 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) { + 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) + } + }) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 2001:0db8:85a3:0000:0000:8a2e:0370:7334 ::1 12345 8080\r\n")) + + res, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1TCP6", + }) + + 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) + } +} + +// 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) { + if got, notWant := r.Header.Get("X-Forwarded-For"), "192.168.0.2"; got == notWant { + t.Errorf("X-Forwarded-For: %v") + } + }) + defer st.Close() + + st.conn.Write([]byte("PROXY UNKNOWN 192.168.0.2 192.168.0.100 12345 8080\r\n")) + + res, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1Unknown", + }) + + 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) + } +} + +// TestH2H1ProxyProtocolV1JustUnknown tests PROXY protocol version 1 +// containing only "PROXY UNKNOWN" is accepted. +func TestH2H1ProxyProtocolV1JustUnknown(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY UNKNOWN\r\n")) + + res, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1JustUnknown", + }) + + 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) + } +} + +// TestH2H1ProxyProtocolV1BadLineEnd tests that PROXY protocol version +// 1 line ending without \r\n should be rejected. +func TestH2H1ProxyProtocolV1BadLineEnd(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 12345 8080\r \n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1BadLineEnd", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1NoEnd tests that PROXY protocol version 1 +// line containing no \r\n should be rejected. +func TestH2H1ProxyProtocolV1NoEnd(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 12345 8080")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1NoEnd", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1EmbeddedNULL tests that PROXY protocol +// version 1 line containing NULL character should be rejected. +func TestH2H1ProxyProtocolV1EmbeddedNULL(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + b := []byte("PROXY TCP6 ::1*foo ::1 12345 8080\r\n") + b[14] = 0 + st.conn.Write(b) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1EmbeddedNULL", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1MissingSrcPort tests that PROXY protocol +// version 1 line without src port should be rejected. +func TestH2H1ProxyProtocolV1MissingSrcPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1MissingSrcPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1MissingDstPort tests that PROXY protocol +// version 1 line without dst port should be rejected. +func TestH2H1ProxyProtocolV1MissingDstPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 12345 \r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1MissingDstPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidSrcPort tests that PROXY protocol +// containing invalid src port should be rejected. +func TestH2H1ProxyProtocolV1InvalidSrcPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 123x 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidSrcPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidDstPort tests that PROXY protocol +// containing invalid dst port should be rejected. +func TestH2H1ProxyProtocolV1InvalidDstPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 123456 80x\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidDstPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1TooLargeSrcPort tests that PROXY protocol +// containing too large src port should be rejected. +func TestH2H1ProxyProtocolV1TooLargeSrcPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 65536 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1TooLargeSrcPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1TooLargeDstPort tests that PROXY protocol +// containing too large dst port should be rejected. +func TestH2H1ProxyProtocolV1TooLargeDstPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 12345 65536\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1TooLargeDstPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidSrcAddr tests that PROXY protocol +// containing invalid src addr should be rejected. +func TestH2H1ProxyProtocolV1InvalidSrcAddr(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 192.168.0.1 ::1 12345 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidSrcAddr", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidDstAddr tests that PROXY protocol +// containing invalid dst addr should be rejected. +func TestH2H1ProxyProtocolV1InvalidDstAddr(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 192.168.0.1 12345 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidDstAddr", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidProtoFamily tests that PROXY protocol +// containing invalid protocol family should be rejected. +func TestH2H1ProxyProtocolV1InvalidProtoFamily(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY UNIX ::1 ::1 12345 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidProtoFamily", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + +// TestH2H1ProxyProtocolV1InvalidID tests that PROXY protocol +// containing invalid PROXY protocol version 1 ID should be rejected. +func TestH2H1ProxyProtocolV1InvalidID(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PR0XY TCP6 ::1 ::1 12345 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1InvalidID", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + // TestH2H1GracefulShutdown tests graceful shutdown. func TestH2H1GracefulShutdown(t *testing.T) { st := newServerTester(nil, t, noopHandler) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 3bdb6123..e8a211d4 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -857,6 +857,18 @@ ssize_t parse_proxy_line_port(const uint8_t *first, const uint8_t *last) { } } // namespace +int ClientHandler::on_proxy_protocol_finish() { + setup_upstream_io_callback(); + + // Run on_read to process data left in buffer since they are not + // notified further + if (on_read() != 0) { + return -1; + } + + return 0; +} + int ClientHandler::proxy_protocol_read() { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol: Started"; @@ -878,7 +890,7 @@ int ClientHandler::proxy_protocol_read() { constexpr const char HEADER[] = "PROXY "; - if (rb_.rleft() < str_size(HEADER)) { + if (end - rb_.pos < str_size(HEADER)) { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol-v1: PROXY version 1 ID not found"; } @@ -897,7 +909,7 @@ int ClientHandler::proxy_protocol_read() { int family; if (rb_.pos[0] == 'T') { - if (rb_.rleft() < 5) { + if (end - rb_.pos < 5) { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol-v1: INET protocol family not found"; } @@ -917,7 +929,7 @@ int ClientHandler::proxy_protocol_read() { rb_.drain(5); } else { - if (rb_.rleft() < 7) { + if (end - rb_.pos < 7) { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol-v1: INET protocol family not found"; } @@ -930,7 +942,9 @@ int ClientHandler::proxy_protocol_read() { return -1; } - return 0; + rb_.drain(end + 2 - rb_.pos); + + return on_proxy_protocol_finish(); } // source address @@ -1012,15 +1026,7 @@ int ClientHandler::proxy_protocol_read() { << " bytes read"; } - setup_upstream_io_callback(); - - // Run on_read to process data left in buffer since they are not - // notified further - if (on_read() != 0) { - return -1; - } - - return 0; + return on_proxy_protocol_finish(); } } // namespace shrpx diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index 45ef7a20..55389146 100644 --- a/src/shrpx_client_handler.h +++ b/src/shrpx_client_handler.h @@ -72,6 +72,7 @@ public: int upstream_write(); int proxy_protocol_read(); + int on_proxy_protocol_finish(); // Performs I/O operation. Internally calls on_read()/on_write(). int do_read(); From f8c1da7f3c30d0bff3fe67f776fc92b36a1bbaec Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 6 Sep 2015 23:27:07 +0900 Subject: [PATCH 4/6] nghttpx: Add --accept-proxy-protocol usage to help message --- src/shrpx.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shrpx.cc b/src/shrpx.cc index fd918bd3..9354a14f 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -1268,6 +1268,8 @@ Connections: timeouts when connecting and making CONNECT request can be specified by --backend-read-timeout and --backend-write-timeout options. + --accept-proxy-protocol + Accept PROXY protocol version 1 on frontend connection. Performance: -n, --workers= From 79945c0c45cdf7230a29132bdabaf87de0594430 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 7 Sep 2015 22:35:02 +0900 Subject: [PATCH 5/6] nghttpx: Robust PROXY protocol implementation --- integration-tests/nghttpx_http2_test.go | 35 ++++++++++++++ src/shrpx_client_handler.cc | 64 +++++++++++++++++++++---- src/shrpx_client_handler.h | 1 + 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/integration-tests/nghttpx_http2_test.go b/integration-tests/nghttpx_http2_test.go index fe1fd37a..527cf4c8 100644 --- a/integration-tests/nghttpx_http2_test.go +++ b/integration-tests/nghttpx_http2_test.go @@ -885,6 +885,23 @@ func TestH2H1ProxyProtocolV1JustUnknown(t *testing.T) { } } +// TestH2H1ProxyProtocolV1TooLongLine tests PROXY protocol version 1 +// line longer than 107 bytes must be rejected +func TestH2H1ProxyProtocolV1TooLongLine(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY UNKNOWN ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff 65535 655350\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1TooLongLine", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + // TestH2H1ProxyProtocolV1BadLineEnd tests that PROXY protocol version // 1 line ending without \r\n should be rejected. func TestH2H1ProxyProtocolV1BadLineEnd(t *testing.T) { @@ -1006,6 +1023,24 @@ func TestH2H1ProxyProtocolV1InvalidDstPort(t *testing.T) { } } +// TestH2H1ProxyProtocolV1LeadingZeroPort tests that PROXY protocol +// version 1 line with non zero port with leading zero should be +// rejected. +func TestH2H1ProxyProtocolV1LeadingZeroPort(t *testing.T) { + st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler) + defer st.Close() + + st.conn.Write([]byte("PROXY TCP6 ::1 ::1 03000 8080\r\n")) + + _, err := st.http2(requestParam{ + name: "TestH2H1ProxyProtocolV1LeadingZeroPort", + }) + + if err == nil { + t.Fatalf("connection was not terminated") + } +} + // TestH2H1ProxyProtocolV1TooLargeSrcPort tests that PROXY protocol // containing too large src port should be rejected. func TestH2H1ProxyProtocolV1TooLargeSrcPort(t *testing.T) { diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index e8a211d4..f7602dac 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -104,6 +104,8 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) { } } // namespace +int ClientHandler::noop() { return 0; } + int ClientHandler::read_clear() { ev_timer_again(conn_.loop, &conn_.rt); @@ -383,7 +385,8 @@ ClientHandler::ClientHandler(Worker *worker, int fd, SSL *ssl, ev_timer_again(conn_.loop, &conn_.rt); if (get_config()->accept_proxy_protocol) { - read_ = write_ = &ClientHandler::read_clear; + read_ = &ClientHandler::read_clear; + write_ = &ClientHandler::noop; on_read_ = &ClientHandler::proxy_protocol_read; on_write_ = &ClientHandler::upstream_noop; } else { @@ -844,6 +847,17 @@ ssize_t parse_proxy_line_port(const uint8_t *first, const uint8_t *last) { auto p = first; int32_t port = 0; + if (p == last) { + return -1; + } + + if (*p == '0') { + if (p + 1 != last && util::isDigit(*(p + 1))) { + return -1; + } + return 1; + } + for (; p != last && util::isDigit(*p); ++p) { port *= 10; port += *p - '0'; @@ -869,6 +883,7 @@ int ClientHandler::on_proxy_protocol_finish() { return 0; } +// http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt int ClientHandler::proxy_protocol_read() { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol: Started"; @@ -876,18 +891,37 @@ int ClientHandler::proxy_protocol_read() { auto first = rb_.pos; - // NULL character really destroys getaddrinfo function. We won't - // expect it in PROXY protocol line, so find it here. - auto chrs = std::array{{'\r', '\0'}}; - auto end = std::find_first_of(rb_.pos, rb_.pos + rb_.rleft(), - std::begin(chrs), std::end(chrs)); - if (end + 2 > rb_.pos + rb_.rleft() || *end == '\0' || end[1] != '\n') { + // NULL character really destroys functions which expects NULL + // terminated string. We won't expect it in PROXY protocol line, so + // find it here. + auto chrs = std::array{{'\n', '\0'}}; + + constexpr size_t MAX_PROXY_LINELEN = 107; + + auto bufend = rb_.pos + std::min(MAX_PROXY_LINELEN, rb_.rleft()); + + auto end = + std::find_first_of(rb_.pos, bufend, std::begin(chrs), std::end(chrs)); + + if (end == bufend) { + if (rb_.rleft() >= MAX_PROXY_LINELEN) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: No ending CR LF sequence found"; + } + return -1; + } + return 0; + } + + if (*end == '\0' || end == rb_.pos || *(end - 1) != '\r') { if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol-v1: No ending CR LF sequence found"; } return -1; } + --end; + constexpr const char HEADER[] = "PROXY "; if (end - rb_.pos < str_size(HEADER)) { @@ -916,11 +950,21 @@ int ClientHandler::proxy_protocol_read() { return -1; } - if (util::streq_l("TCP4 ", rb_.pos, 5)) { + if (rb_.pos[1] != 'C' || rb_.pos[2] != 'P') { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family"; + } + return -1; + } + + switch (rb_.pos[3]) { + case '4': family = AF_INET; - } else if (util::streq_l("TCP6 ", rb_.pos, 5)) { + break; + case '6': family = AF_INET6; - } else { + break; + default: if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family"; } diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index 55389146..b9931301 100644 --- a/src/shrpx_client_handler.h +++ b/src/shrpx_client_handler.h @@ -56,6 +56,7 @@ public: const char *port); ~ClientHandler(); + int noop(); // Performs clear text I/O int read_clear(); int write_clear(); From 026ab797eb49c4e148a266f4d88f5a7ddab14027 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 7 Sep 2015 22:40:37 +0900 Subject: [PATCH 6/6] src: util::numeric_host: Use inet_pton instead of getaddrinfo --- src/util.cc | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/util.cc b/src/util.cc index 9b709dbe..58d6272e 100644 --- a/src/util.cc +++ b/src/util.cc @@ -636,19 +636,16 @@ void write_uri_field(std::ostream &o, const char *uri, const http_parser_url &u, } bool numeric_host(const char *hostname) { - return numeric_host(hostname, AF_UNSPEC); + return numeric_host(hostname, AF_INET) || numeric_host(hostname, AF_INET6); } bool numeric_host(const char *hostname, int family) { - struct addrinfo *res; - struct addrinfo hints {}; - hints.ai_family = family; - hints.ai_flags = AI_NUMERICHOST; - if (getaddrinfo(hostname, nullptr, &hints, &res)) { - return false; - } - freeaddrinfo(res); - return true; + int rv; + std::array dst; + + rv = inet_pton(family, hostname, dst.data()); + + return rv == 1; } std::string numeric_name(const struct sockaddr *sa, socklen_t salen) {