[Merged by Bors] - Handle HTTP upgrade request when steal + HTTP traffic filter are enabled. #973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…iginal destination. Get the response back and send it to remote? (doesnt work). At least it compiles now.
…irectional if its actually an upgrade.
…<TcpStream> (now only TcpStream in channel). Add result return for filter_task function.
…self.original_destination.
eyalb181
approved these changes
Jan 21, 2023
Co-authored-by: Eyal Bukchin <[email protected]>
|
Sup bors, could you merge this one? bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 22, 2023
…led. (#973) Issue: #928 Depends on [test-images #14](metalbear-co/test-images#14). Implements a pass-through mechanism to handle HTTP/1 upgrade requests from a client. ## Motivation Currently, if you run mirrord with steal traffic, and HTTP filter enabled, we end up not serving HTTP upgrade requests appropriately. Our implementation of [`Service`](https://docs.rs/hyper/1.0.0-rc.2/hyper/service/trait.Service.html) would either: - Forward the HTTP request to the local app (if a filter matched), send back a response and **not** upgrade the connection, or; - Pass the request to its original destination, get a response back from it, and, again, **not** upgrade the connection. This would prevent the user from handling websockets (our main motivator behind this feature), that start up as HTTP requests. ## Implementation We're manually handling connection upgrades, by first checking if an HTTP request contains the "Upgrade" header, and if that's the case, then we: - Current `unmacthed_request` flow: 1. Create a connection to the original destination (let's call this the `agent_original` connection); 2. Forward the HTTP request to it, awaiting its response; 3. Send the response back through our hyper connection (the `agent_client` connection we handle in hyper); - The new steps: 4. Send the `agent_original` connection to the `filter_task` (through the `upgrade_tx` channel); 5. Retrieve the `agent_client` connection from hyper, as a normal `TcpStream`; 6. Copy whatever bytes were leftover from these connections (bytes that were read, but not processed by hyper); 7. Use [`copy_bidirectional`](https://docs.rs/tokio/latest/tokio/io/fn.copy_bidirectional.html) to keep copying from one connection to the other. ### Tests The feature is being tested by `test_websocket_upgrade`, which is very similar to the `test_complete_passthrough` test. The main difference is that we use [`tokio-tugstenite`](https://docs.rs/tokio-tungstenite/latest/tokio_tungstenite/) to create a websocket connection (instead of a normal `TcpStream`). ## Alternatives ### Give a [`DuplexStream`](https://docs.rs/tokio/latest/tokio/io/struct.DuplexStream.html) to hyper We could feed hyper a `DuplexStream` rather than the actual `TcpStream` connection, and deal with the upgrade ourselves, by parsing the bytes into an `Header`, checking for the "Upgrade: " header, or even use the [`Response`] as a way to identify that this was an upgrade request. I've explored this approach, but this seemed a bit too much, especially when @t4lz asked me about "why not use the `unmatched_request`?" ### Use [`hyper::upgrade::on`] hyper has a proper handler for dealing with HTTP upgrade requests, where you give it a [`Request`](https://docs.rs/hyper/1.0.0-rc.2/hyper/struct.Request.html) that contains an [`OnUpgrade`](https://docs.rs/hyper/1.0.0-rc.2/hyper/upgrade/struct.OnUpgrade.html) extension, and it'll give you back its connection (`agent_client`) and the leftover bytes. I could not make this approach work, as we need to give the request to [`SendRequest`](https://docs.rs/hyper/1.0.0-rc.2/hyper/client/conn/http1/struct.SendRequest.html) by value, making it impossible to also feed it to the `upgrade::on` function. This was the case because we need to poll the connection (`agent_original`) in a separate task, thus requiring us to also move the request into it. The request cannot be copied/cloned, as it contains a `Receiver` channel for the `OnUpgrade` feature, and if you try to manually copy it, the upgrade is never triggered, because we're dealing with different channels. Co-authored-by: meowjesty <[email protected]>
|
Build failed: |
|
Hey bors, retry please. bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 23, 2023
…led. (#973) Issue: #928 Depends on [test-images #14](metalbear-co/test-images#14). Implements a pass-through mechanism to handle HTTP/1 upgrade requests from a client. ## Motivation Currently, if you run mirrord with steal traffic, and HTTP filter enabled, we end up not serving HTTP upgrade requests appropriately. Our implementation of [`Service`](https://docs.rs/hyper/1.0.0-rc.2/hyper/service/trait.Service.html) would either: - Forward the HTTP request to the local app (if a filter matched), send back a response and **not** upgrade the connection, or; - Pass the request to its original destination, get a response back from it, and, again, **not** upgrade the connection. This would prevent the user from handling websockets (our main motivator behind this feature), that start up as HTTP requests. ## Implementation We're manually handling connection upgrades, by first checking if an HTTP request contains the "Upgrade" header, and if that's the case, then we: - Current `unmacthed_request` flow: 1. Create a connection to the original destination (let's call this the `agent_original` connection); 2. Forward the HTTP request to it, awaiting its response; 3. Send the response back through our hyper connection (the `agent_client` connection we handle in hyper); - The new steps: 4. Send the `agent_original` connection to the `filter_task` (through the `upgrade_tx` channel); 5. Retrieve the `agent_client` connection from hyper, as a normal `TcpStream`; 6. Copy whatever bytes were leftover from these connections (bytes that were read, but not processed by hyper); 7. Use [`copy_bidirectional`](https://docs.rs/tokio/latest/tokio/io/fn.copy_bidirectional.html) to keep copying from one connection to the other. ### Tests The feature is being tested by `test_websocket_upgrade`, which is very similar to the `test_complete_passthrough` test. The main difference is that we use [`tokio-tugstenite`](https://docs.rs/tokio-tungstenite/latest/tokio_tungstenite/) to create a websocket connection (instead of a normal `TcpStream`). ## Alternatives ### Give a [`DuplexStream`](https://docs.rs/tokio/latest/tokio/io/struct.DuplexStream.html) to hyper We could feed hyper a `DuplexStream` rather than the actual `TcpStream` connection, and deal with the upgrade ourselves, by parsing the bytes into an `Header`, checking for the "Upgrade: " header, or even use the [`Response`] as a way to identify that this was an upgrade request. I've explored this approach, but this seemed a bit too much, especially when @t4lz asked me about "why not use the `unmatched_request`?" ### Use [`hyper::upgrade::on`] hyper has a proper handler for dealing with HTTP upgrade requests, where you give it a [`Request`](https://docs.rs/hyper/1.0.0-rc.2/hyper/struct.Request.html) that contains an [`OnUpgrade`](https://docs.rs/hyper/1.0.0-rc.2/hyper/upgrade/struct.OnUpgrade.html) extension, and it'll give you back its connection (`agent_client`) and the leftover bytes. I could not make this approach work, as we need to give the request to [`SendRequest`](https://docs.rs/hyper/1.0.0-rc.2/hyper/client/conn/http1/struct.SendRequest.html) by value, making it impossible to also feed it to the `upgrade::on` function. This was the case because we need to poll the connection (`agent_original`) in a separate task, thus requiring us to also move the request into it. The request cannot be copied/cloned, as it contains a `Receiver` channel for the `OnUpgrade` feature, and if you try to manually copy it, the upgrade is never triggered, because we're dealing with different channels. Co-authored-by: meowjesty <[email protected]>
|
Build failed: |
|
Another one. bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 23, 2023
…led. (#973) Issue: #928 Depends on [test-images #14](metalbear-co/test-images#14). Implements a pass-through mechanism to handle HTTP/1 upgrade requests from a client. ## Motivation Currently, if you run mirrord with steal traffic, and HTTP filter enabled, we end up not serving HTTP upgrade requests appropriately. Our implementation of [`Service`](https://docs.rs/hyper/1.0.0-rc.2/hyper/service/trait.Service.html) would either: - Forward the HTTP request to the local app (if a filter matched), send back a response and **not** upgrade the connection, or; - Pass the request to its original destination, get a response back from it, and, again, **not** upgrade the connection. This would prevent the user from handling websockets (our main motivator behind this feature), that start up as HTTP requests. ## Implementation We're manually handling connection upgrades, by first checking if an HTTP request contains the "Upgrade" header, and if that's the case, then we: - Current `unmacthed_request` flow: 1. Create a connection to the original destination (let's call this the `agent_original` connection); 2. Forward the HTTP request to it, awaiting its response; 3. Send the response back through our hyper connection (the `agent_client` connection we handle in hyper); - The new steps: 4. Send the `agent_original` connection to the `filter_task` (through the `upgrade_tx` channel); 5. Retrieve the `agent_client` connection from hyper, as a normal `TcpStream`; 6. Copy whatever bytes were leftover from these connections (bytes that were read, but not processed by hyper); 7. Use [`copy_bidirectional`](https://docs.rs/tokio/latest/tokio/io/fn.copy_bidirectional.html) to keep copying from one connection to the other. ### Tests The feature is being tested by `test_websocket_upgrade`, which is very similar to the `test_complete_passthrough` test. The main difference is that we use [`tokio-tugstenite`](https://docs.rs/tokio-tungstenite/latest/tokio_tungstenite/) to create a websocket connection (instead of a normal `TcpStream`). ## Alternatives ### Give a [`DuplexStream`](https://docs.rs/tokio/latest/tokio/io/struct.DuplexStream.html) to hyper We could feed hyper a `DuplexStream` rather than the actual `TcpStream` connection, and deal with the upgrade ourselves, by parsing the bytes into an `Header`, checking for the "Upgrade: " header, or even use the [`Response`] as a way to identify that this was an upgrade request. I've explored this approach, but this seemed a bit too much, especially when @t4lz asked me about "why not use the `unmatched_request`?" ### Use [`hyper::upgrade::on`] hyper has a proper handler for dealing with HTTP upgrade requests, where you give it a [`Request`](https://docs.rs/hyper/1.0.0-rc.2/hyper/struct.Request.html) that contains an [`OnUpgrade`](https://docs.rs/hyper/1.0.0-rc.2/hyper/upgrade/struct.OnUpgrade.html) extension, and it'll give you back its connection (`agent_client`) and the leftover bytes. I could not make this approach work, as we need to give the request to [`SendRequest`](https://docs.rs/hyper/1.0.0-rc.2/hyper/client/conn/http1/struct.SendRequest.html) by value, making it impossible to also feed it to the `upgrade::on` function. This was the case because we need to poll the connection (`agent_original`) in a separate task, thus requiring us to also move the request into it. The request cannot be copied/cloned, as it contains a `Receiver` channel for the `OnUpgrade` feature, and if you try to manually copy it, the upgrade is never triggered, because we're dealing with different channels. Co-authored-by: meowjesty <[email protected]>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #928
Depends on test-images #14.
Implements a pass-through mechanism to handle HTTP/1 upgrade requests from a client.
Motivation
Currently, if you run mirrord with steal traffic, and HTTP filter enabled, we end up not serving HTTP upgrade requests appropriately. Our implementation of
Servicewould either:This would prevent the user from handling websockets (our main motivator behind this feature), that start up as HTTP requests.
Implementation
We're manually handling connection upgrades, by first checking if an HTTP request contains the "Upgrade" header, and if that's the case, then we:
unmacthed_requestflow:agent_originalconnection);agent_clientconnection we handle in hyper);agent_originalconnection to thefilter_task(through theupgrade_txchannel);agent_clientconnection from hyper, as a normalTcpStream;copy_bidirectionalto keep copying from one connection to the other.Tests
The feature is being tested by
test_websocket_upgrade, which is very similar to thetest_complete_passthroughtest. The main difference is that we usetokio-tugsteniteto create a websocket connection (instead of a normalTcpStream).Alternatives
Give a
DuplexStreamto hyperWe could feed hyper a
DuplexStreamrather than the actualTcpStreamconnection, and deal with the upgrade ourselves, by parsing the bytes into anHeader, checking for the "Upgrade: " header, or even use the [Response] as a way to identify that this was an upgrade request.I've explored this approach, but this seemed a bit too much, especially when @t4lz asked me about "why not use the
unmatched_request?"Use [
hyper::upgrade::on]hyper has a proper handler for dealing with HTTP upgrade requests, where you give it a
Requestthat contains anOnUpgradeextension, and it'll give you back its connection (agent_client) and the leftover bytes.I could not make this approach work, as we need to give the request to
SendRequestby value, making it impossible to also feed it to theupgrade::onfunction. This was the case because we need to poll the connection (agent_original) in a separate task, thus requiring us to also move the request into it.The request cannot be copied/cloned, as it contains a
Receiverchannel for theOnUpgradefeature, and if you try to manually copy it, the upgrade is never triggered, because we're dealing with different channels.