-
Notifications
You must be signed in to change notification settings - Fork 34
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
feature request: define domain-based policies for web socket interception #341
Comments
I have made a first very dirty shortsighted demo PoC here: diff --git a/g3proxy/src/inspect/websocket/h1.rs b/g3proxy/src/inspect/websocket/h1.rs
index 37eb7f7f..8c40fc31 100644
--- a/g3proxy/src/inspect/websocket/h1.rs
+++ b/g3proxy/src/inspect/websocket/h1.rs
@@ -91,7 +91,19 @@ impl<SC: ServerConfig> H1WebsocketInterceptObject<SC> {
pub(crate) async fn intercept(mut self) -> ServerTaskResult<()> {
let r = match self.ctx.websocket_inspect_policy() {
- ProtocolInspectPolicy::Intercept => self.do_intercept().await,
+ ProtocolInspectPolicy::Intercept => {
+ // quick PoC test in case zh-jq wants to have it as a ws_interception config,
+ // in such case we could block the traffic here in a more worked out manner
+ if self.upstream.host_eq(
+ &UpstreamAddr::from_host_str_and_port("websocketstest.com", 443).unwrap(),
+ ) {
+ log::info!("h1: block WS based on upstream address: {}", self.upstream);
+ self.do_block().await
+ } else {
+ log::info!("h1: allow WS based on upstream address: {}", self.upstream);
+ self.do_intercept().await
+ }
+ }
#[cfg(feature = "quic")]
ProtocolInspectPolicy::Detour => self.do_detour().await,
ProtocolInspectPolicy::Bypass => self.do_bypass().await,
diff --git a/g3proxy/src/inspect/websocket/h2.rs b/g3proxy/src/inspect/websocket/h2.rs
index 7694653d..f3eb817a 100644
--- a/g3proxy/src/inspect/websocket/h2.rs
+++ b/g3proxy/src/inspect/websocket/h2.rs
@@ -75,7 +75,19 @@ impl<SC: ServerConfig> H2WebsocketInterceptObject<SC> {
ups_w: SendStream<Bytes>,
) {
let r = match self.ctx.websocket_inspect_policy() {
- ProtocolInspectPolicy::Intercept => self.do_intercept(clt_r, clt_w, ups_r, ups_w).await,
+ ProtocolInspectPolicy::Intercept => {
+ // quick PoC test in case zh-jq wants to have it as a ws_interception config,
+ // in such case we could block the traffic here in a more worked out manner
+ if self.upstream.host_eq(
+ &UpstreamAddr::from_host_str_and_port("websocketstest.com", 443).unwrap(),
+ ) {
+ log::info!("h2: block WS based on upstream address: {}", self.upstream);
+ self.do_block(clt_w, ups_w).await
+ } else {
+ log::info!("h2: allow WS based on upstream address: {}", self.upstream);
+ self.do_intercept(clt_r, clt_w, ups_r, ups_w).await
+ }
+ }
#[cfg(feature = "quic")]
ProtocolInspectPolicy::Detour => self.do_detour(clt_r, clt_w, ups_r, ups_w).await,
ProtocolInspectPolicy::Bypass => self.do_bypass(clt_r, clt_w, ups_r, ups_w).await, That is the locations I am aware of so far in case we want to go towards the approach for If you want to go the With that approach you should be able to go to https://websocketstest.com/ and see that it no longer works (the test) while other web sockets do still work. |
Yes, external contributions will always be welcomed. I have to cut the new v1.10 LTS branch this month, so all the new feaures will only be available in the next 1.11 development vesion.
That seems like a feature useful for all protocols. You can extend the
The
That's just a cheap way to do it but not the best. The config structure may be too large if there are many domain specific rules.
Thanks. That will work for me. |
Works for me.
Even better. Before I start with the real work of enabling that and hooking it up to the config and whatnot, #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub enum ProtocolInspectPolicy {
#[default]
Intercept(Option<g3_types::acl_set::AclDstHostRuleSet>),
#[cfg(feature = "quic")]
Detour,
Bypass,
Block,
} If not let me know how you see that policy struct. I can take it from there. |
It will be like |
You mean to replace the current struct ProtocolInspectPolicy {
exact: Option<InspectExactHostRule>,
child: Option<InspectChildDomainRule>,
regex: Option<InspectRegexSetRule>,
subnet: Option<InspectNetworkRule>,
missed_action: InspectAction,
}
struct InspectAction {
kind: InspectActionKind,
log: bool,
}
enum InspectActionKind {
Intercept,
#[cfg(feature = "quic")]
Detour,
Bypass,
Block,
}
struct InspectExactHostRule { ... }
struct InspectChildDomainRule { ... }
struct InspectRegexSetRule { ... }
struct InspectNetworkRule { ... } I don't mind doing that work. But seems like a lot of duplicated code from your Also where do you want this inspect (type) code to live? |
Yes. Maybe it could be
If we can't reuse the ACL data types, it can go to the same module as the current |
Replacing And thus that would mean to redefine pub struct AclDstHostRuleSet<Action> {
exact: Option<AclExactHostRule<Action>>,
child: Option<AclChildDomainRule<Action>>,
regex: Option<AclRegexSetRule<Action>>,
subnet: Option<AclNetworkRule<Action>>,
missed_action: Action,
} I am ok with any approach, so also this. But does seem like a pretty invasive change. |
Direction 1 the best, if possible. The breaking change in config maybe avoided by adding some workround code. |
Ok I’ll get to work, I know enough for now, thx! |
#344 is ready for review @zh-jq . It's my first real code contribution to your codebase, so I would not be surprised that you have a lot of requests and feedback. I tested it on a local g3-based proxy and it seems to work as expected for the web sockets. Where I block some, but intercept all others (as that is the default I used in my test). Given this PR is in line with the direction we agreed upon I hope we can find an alignment quickly. |
@zh-jq I can confirm that web the inspect policy now can contain ACL Rules, which is pretty neat. I consider that part finished until you deemed otherwise in a review whenever you have time for it. However... if we zoom in on web sockets, I do see an additional issue. By the time the web socket policy triggers, and thus closes the connection, it has already been upgraded and established. This can however give odd behavior in a proxy client as it would essentially look like an EOF after ws upgrade was established and active. As such I think that besides the feature I implemented in my PR, which I anyway can also use for other stuff given the policy is applicable to more then just web sockets, I also need to be able to specifically filter on http headers and block the traffic like that. Correct me if I'm wrong @zh-jq but I believe the proper solution for web sockets in specific would be to send a 400 BAD Request response when a web socket http request comes in. E.g. by filtering on I could however not found anything of this kind of support in g3, or am I wrong? Curious to hear your opinion on this matter. |
@GlenDC Yes you are right, websocket should be blocked gracefully. The check of inspect policy can be added when received H1 upgrade or H2 extended-connect-upgrade request. This just like the inspect policy check when parsing ALPN in TLS interception code. |
But I am correct in thinking this will need an additional PR / patch, right? This is not something you can do today? If so, can you guide me on how I would do that? |
Yes this should be considered as a BUG FIX.
I can write a PR tomorrow. If you want to fix it today, here are the code position that need to be updated: H2:
H1: g3/g3proxy/src/inspect/http/v1/upgrade/mod.rs Line 152 in 1f215cc
g3/g3proxy/src/inspect/http/v1/upgrade/mod.rs Line 177 in 1f215cc
|
I was indeed messing around with some nested called code originated from If you do not mind please do take over the bug fix for that. Please do link it here, so I can learn from it for future contributions, if you will and can. Thanks! |
OK. I will do it tomorrow. The method names may be confusing as I'm not good at English. Feel free to submit PR if you can make the code more readable. |
The method names are pretty Ok. But noted that once I get more comfortable with your code base that I can also submit auxiliary patches. Would do so with pleasure. Thanks for taking that bug fix over! Looking forward to learning from the solution. |
Here is the fix PR: #348 |
@zh-jq @zh-jq-b I merged it in my feature branch and also tested it in my demo. |
E.g. something like this, even though I Imagine there is a cleaner solution: diff --git a/g3proxy/src/inspect/http/v1/upgrade/mod.rs b/g3proxy/src/inspect/http/v1/upgrade/mod.rs
index 7def8c22..d5732d78 100644
--- a/g3proxy/src/inspect/http/v1/upgrade/mod.rs
+++ b/g3proxy/src/inspect/http/v1/upgrade/mod.rs
@@ -153,6 +153,15 @@ where
where
CW: AsyncWrite + Unpin,
{
+ if !self
+ .req
+ .hop_by_hop_headers
+ .get("upgrade")
+ .map(|h| h.to_str() == "websocket")
+ .unwrap_or_default()
+ {
+ return Ok(());
+ }
let policy_action = match self.req.host.as_ref() {
Some(upstream) => {
let (_, policy_action) = self.ctx.websocket_inspect_policy().check(upstream.host());
diff --git a/g3proxy/src/inspect/http/v2/connect/extended.rs b/g3proxy/src/inspect/http/v2/connect/extended.rs
index 7a6db8f4..e765a423 100644
--- a/g3proxy/src/inspect/http/v2/connect/extended.rs
+++ b/g3proxy/src/inspect/http/v2/connect/extended.rs
@@ -175,17 +175,25 @@ where
}
};
- let policy_action = match self.upstream.as_ref() {
- Some(upstream) => {
- let (_, policy_action) = self.ctx.websocket_inspect_policy().check(upstream.host());
- policy_action
+ if clt_req
+ .headers()
+ .get("upgrade")
+ .map(|h| h.to_str() == "websocket")
+ .unwrap_or_default()
+ {
+ let policy_action = match self.upstream.as_ref() {
+ Some(upstream) => {
+ let (_, policy_action) =
+ self.ctx.websocket_inspect_policy().check(upstream.host());
+ policy_action
+ }
+ None => self.ctx.websocket_inspect_policy().missing_action(),
+ };
+ if policy_action == ProtocolInspectAction::Block {
+ self.reply_forbidden(clt_send_rsp);
+ intercept_log!(self, "websocket blocked by inspection policy");
+ return;
}
- None => self.ctx.websocket_inspect_policy().missing_action(),
- };
- if policy_action == ProtocolInspectAction::Block {
- self.reply_forbidden(clt_send_rsp);
- intercept_log!(self, "websocket blocked by inspection policy");
- return;
}
let mut ws_notes = WebSocketNotes::new(clt_req.uri().clone()); |
Hmm I might be wrong actually. Might be that you already somehow do cleanly handle this, but didn't seem to find it anywhere... Now I am not sure anymore. |
Yes I forget to check for websocket in H1 code. The 'Upgrade' header in request may contain multiple values, a rewrite of that is needed there. |
No need to also update the |
That's already detected as a Websocket upgrade request, so we can just block there. |
A sane fix would probably be to change signtature
|
This is a PoC of that suggested approach for this bug fix: diff --git a/g3proxy/src/inspect/http/v1/upgrade/mod.rs b/g3proxy/src/inspect/http/v1/upgrade/mod.rs
index c2859e1b..abb6ba49 100644
--- a/g3proxy/src/inspect/http/v1/upgrade/mod.rs
+++ b/g3proxy/src/inspect/http/v1/upgrade/mod.rs
@@ -163,7 +163,7 @@ where
!matches!(p, HttpUpgradeToken::Websocket) || retain_websocket_upgrade
});
- if upgrade_token_count == 0 {
+ if upgrade_token_count == Some(0) {
let rsp = HttpProxyClientResponse::forbidden(self.req.version);
self.should_close = true;
if rsp.reply_err_to_request(clt_w).await.is_ok() {
diff --git a/lib/g3-http/src/server/transparent.rs b/lib/g3-http/src/server/transparent.rs
index 3c246e35..338348d4 100644
--- a/lib/g3-http/src/server/transparent.rs
+++ b/lib/g3-http/src/server/transparent.rs
@@ -299,11 +299,12 @@ impl HttpTransparentRequest {
Ok(())
}
- pub fn retain_upgrade<F>(&mut self, retain: F) -> usize
+ pub fn retain_upgrade<F>(&mut self, retain: F) -> Option<usize>
where
F: Fn(HttpUpgradeToken) -> bool,
{
let mut new_upgrade_headers = Vec::new();
+ let mut headers_found = false;
for header in self.hop_by_hop_headers.get_all(header::UPGRADE) {
let value = header.to_str();
for s in value.split(',') {
@@ -315,6 +316,7 @@ impl HttpTransparentRequest {
let Ok(protocol) = HttpUpgradeToken::from_str(s) else {
continue;
};
+ headers_found = true;
if retain(protocol) {
let mut new_value =
unsafe { HttpHeaderValue::from_string_unchecked(s.to_string()) };
@@ -331,7 +333,7 @@ impl HttpTransparentRequest {
for value in new_upgrade_headers {
self.hop_by_hop_headers.append(header::UPGRADE, value);
}
- retain_count
+ headers_found.then_some(retain_count)
}
fn insert_hop_by_hop_header(
@@ -653,7 +655,9 @@ mod tests {
let (mut request, _) = HttpTransparentRequest::parse(&mut buf_stream, 4096, false)
.await
.unwrap();
- let left_tokens = request.retain_upgrade(|p| matches!(p, HttpUpgradeToken::Http(_)));
+ let left_tokens = request
+ .retain_upgrade(|p| matches!(p, HttpUpgradeToken::Http(_)))
+ .unwrap();
assert_eq!(left_tokens, 1);
let token = request.hop_by_hop_headers.get(header::UPGRADE).unwrap();
assert_eq!(token.to_str(), "HTTP/2.0"); |
The code is run with
What's the upgrade token used? |
You can submit a PR and I will approve it. It is reasonable for |
Something is going wrong though, and it is returning a log such as
which indeed is seen on network interface as a http 500 status code. Even with the above patch applied I get it 100%. You can reproduce it yourself easily, just have a g3proxy running with this in your auditor config:
Then go to https://websocketstest.com/, you'll get a 500 status code even for the regular network traffic. |
…g happend as requested by zh-jq in bytedance#341
Not sure if triggers in |
Did a quick test... Seems to indeed block all traffic going through that code path as soon as you even just use |
I can open https://websocketstest.com/ with this result while testing with master branch:
|
Great that is what I was hoping. In that case there is something I did wrong in my PR branch. Still working on your existing suggestions, but something I will need to track down than in case you do not immediately see it yourself either. |
Hi, first of all I would like to ask if it is possible I contribute the code for this feature request myself, with guidance from you. There are other feature requests I'll have soon and contributions I would like to make. For now I know g3 more or less as a user and also having studies the code, but this will be my first code contributions to your code so guidance will be appreciated. If possible of course.
Currently there is already
websocket_inspect_policy
that you can define for an auditor. Which applies to all traffic. This allows you for example to intercept or block any ws(s) traffic. I would like to request however to make it also possible to do so for specific domains.E.g. I noticed there is no
ws_interception
config yet. So my first thought was that it might be an idea to add such a config and start using that so one can add an allow list. However for all other http traffic we currently make use already ofroute_upstream
to block traffic, rather than have it in thehttp interception
configs. So I imagine you might rather want to have supported in that fashion? I'll already play around with some ideas myself locally in a quick and dirty way, just to get it into my fingers.I would appreciate your feedback, guidance and thoughts on this matter. Once we are aligned and I have your seal of approval on the approach I can deliver you a first PR, if that's alright for you.
The text was updated successfully, but these errors were encountered: