PR comments; fix bug with large requests being allowed

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Author: Kevin Dorosh

api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto

@@ -45,9 +45,10 @@ message ProxyProtocol {
   //
   // .. attention::
   //
+  //   This breaks conformance with the specification.
   //   Only enable if ALL traffic to the listener comes from a trusted source.
   //   For more information on the security implications of this feature, see
-  //   https://www.haproxy.org/download/1.9/doc/proxy-protocol.txt
+  //   https://www.haproxy.org/download/2.1/doc/proxy-protocol.txt
   //
   bool allow_requests_without_proxy_protocol = 2;
 }

source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc

@@ -93,7 +93,7 @@ void Filter::onRead() {
   if (read_state == ReadOrParseState::Error) {
     config_->stats_.downstream_cx_proxy_proto_error_.inc();
     cb_->continueFilterChain(false);
-  } else if (read_state == ReadOrParseState::SkipFilterError) {
+  } else if (read_state == ReadOrParseState::SkipFilter) {
     resetAndContinue(cb_->socket().ioHandle());
   }
 }
@@ -320,7 +320,7 @@ ReadOrParseState Filter::parseExtensions(Network::IoHandle& io_handle, uint8_t*
     const auto recv_result = io_handle.recv(buf, to_read, 0);
     if (!recv_result.ok()) {
       if (recv_result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again) {
-        return ReadOrParseState::TryAgainLaterError;
+        return ReadOrParseState::TryAgainLater;
       }
       ENVOY_LOG(debug, "failed to read proxy protocol (no bytes avail)");
       return ReadOrParseState::Error;
@@ -435,7 +435,7 @@ ReadOrParseState Filter::readProxyHeader(Network::IoHandle& io_handle) {
 
     if (!result.ok()) {
       if (result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again) {
-        return ReadOrParseState::TryAgainLaterError;
+        return ReadOrParseState::TryAgainLater;
       }
       ENVOY_LOG(debug, "failed to read proxy protocol (no bytes read)");
       return ReadOrParseState::Error;
@@ -445,16 +445,15 @@ ReadOrParseState Filter::readProxyHeader(Network::IoHandle& io_handle) {
     if (nread < 1) {
       ENVOY_LOG(debug, "failed to read proxy protocol (no bytes read)");
       return ReadOrParseState::Error;
-    } else if (nread < PROXY_PROTO_V2_HEADER_LEN &&
-               config_.get()->allowRequestsWithoutProxyProtocol()) {
+    } else if (config_.get()->allowRequestsWithoutProxyProtocol()) {
       if (memcmp(buf_, PROXY_PROTO_V1_SIGNATURE,
                  std::min<size_t>(buf_off_ + nread, PROXY_PROTO_V1_SIGNATURE_LEN)) &&
           memcmp(buf_, PROXY_PROTO_V2_SIGNATURE,
                  std::min<size_t>(buf_off_ + nread, PROXY_PROTO_V2_SIGNATURE_LEN))) {
         // the bytes we have seen so far do not match v1 or v2 proxy protocol, so we can safely
         // short-circuit
         ENVOY_LOG(debug, "request does not use v1 or v2 proxy protocol, forwarding as is");
-        return ReadOrParseState::SkipFilterError;
+        return ReadOrParseState::SkipFilter;
       }
     }
 

source/extensions/filters/listener/proxy_protocol/proxy_protocol.h

@@ -75,7 +75,7 @@ using ConfigSharedPtr = std::shared_ptr<Config>;
 
 enum ProxyProtocolVersion { Unknown = -1, InProgress = -2, V1 = 1, V2 = 2 };
 
-enum class ReadOrParseState { Done, TryAgainLaterError, Error, SkipFilterError };
+enum class ReadOrParseState { Done, TryAgainLater, Error, SkipFilter };
 
 /**
  * Implementation the PROXY Protocol listener filter

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc

@@ -222,15 +222,30 @@ TEST_P(ProxyProtocolTest, V1Basic) {
   disconnect();
 }
 
-TEST_P(ProxyProtocolTest, AllowNoProxyProtocol) {
-  // allows request through even though it doesn't use proxy protocol
+TEST_P(ProxyProtocolTest, AllowTinyNoProxyProtocol) {
+  // allows a small request (less bytes than v1/v2 header) through even though it doesn't use proxy
+  // protocol
   envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
   proto_config.set_allow_requests_without_proxy_protocol(true);
   connect(true, &proto_config);
 
-  write("more data");
+  write("data");
 
-  expectData("more data");
+  expectData("data");
+
+  disconnect();
+}
+
+TEST_P(ProxyProtocolTest, AllowLargeNoProxyProtocol) {
+  // allows a large request (more bytes than v1/v2 header) through even though it doesn't use proxy
+  // protocol
+  envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
+  proto_config.set_allow_requests_without_proxy_protocol(true);
+  connect(true, &proto_config);
+
+  write("more data more data more data");
+
+  expectData("more data more data more data");
 
   disconnect();
 }