condure icon indicating copy to clipboard operation
condure copied to clipboard

Incompatibility with Envoy Proxy when downstream client uses HTTP/2

Open 10p-freddo opened this issue 3 years ago • 3 comments

When the downstream client is connected over HTTP/2, Envoy sends upstream requests to Condure that are not fully compliant with RFC 6455. Specifically, Envoy sends Content-Length: 0 and does not send a Sec-WebSocket-Key, which causes Condure to (correctly) deny the request.

Clearly this is a problem with Envoy and not Condure, but I'm posting an issue here to document the incompatibility and provide a workaround for anyone who needs it. Here's a small patch that has solved the problem for me:

diff --git a/src/connection.rs b/src/connection.rs
index b98f513..c601537 100644
--- a/src/connection.rs
+++ b/src/connection.rs
@@ -2636,7 +2636,7 @@ where
         let req = handler.request(&mut scratch);
 
         let mut websocket = false;
-        let mut ws_key = None;
+        let mut ws_key: Option<&[u8]> = Some(&[]);
 
         for h in req.headers.iter() {
             if h.name.eq_ignore_ascii_case("Upgrade") {
@@ -2674,7 +2674,7 @@ where
         );
 
         let ws_accept: Option<ArrayString<[u8; WS_ACCEPT_MAX]>> = if websocket {
-            if req.method != "GET" || req.body_size != http1::BodySize::NoBody || ws_key.is_none() {
+            if req.method != "GET" {
                 return Err(ServerError::InvalidWebSocketRequest);
             }

As there's no plan to address this in Envoy, but it is popular and otherwise compatible with Condure, it would be nice to have a setting to enable this relaxed behavior in Condure. I don't think this would necessarily cause a security regression because Sec-WebSocket-Key is only used to prevent unintended requests, and that problem can be addressed by enabling CORS in Envoy.

Further background:

  • https://github.com/envoyproxy/envoy/issues/15464#issuecomment-799495181
  • https://github.com/envoyproxy/envoy/issues/8547

10p-freddo avatar Feb 08 '22 20:02 10p-freddo

For any brave souls who need to use the above patch, here's a Dockerfile that builds a custom Pushpin image from Condure source. On an arm64 system such as Apple M1, it will cross-compile Condure for amd64.

10p-freddo avatar Feb 08 '22 20:02 10p-freddo

Interesting, I didn't realize anyone was doing WebSockets over HTTP/2 yet. Will digest this. Thank you.

jkarneges avatar Feb 09 '22 19:02 jkarneges

BTW, the check has now been relaxed to allow Content-Length: 0, for compatibility with Java, which partly helps with this issue too. The Sec-WebSocket-Key header is still required though.

jkarneges avatar Apr 15 '22 22:04 jkarneges