Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibility with Envoy Proxy when downstream client uses HTTP/2 #7

Open
10p-freddo opened this issue Feb 8, 2022 · 3 comments
Open

Comments

@10p-freddo
Copy link

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:

@10p-freddo
Copy link
Author

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.

@jkarneges
Copy link
Member

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

@jkarneges
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants