feature request: define domain-based policies for web socket interception

[GlenDC]

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 of route_upstream to block traffic, rather than have it in the http 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.

[GlenDC]

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 ws_interception Config (newly introduced). But as I am new to this code base I might be forgetting some locations. Do let me know. I am open for anything.

If you want to go the route_upstream approach (dunno if applicable here) or something similar, I will need even more guidance / hints.

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.

[zh-jq]

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.

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.

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.

That seems like a feature useful for all protocols.

You can extend the inspect policy enum to be an ACL like struct data type and add domain based rules.

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.

The *_interception config options are used to control protocol interception params insted of policy. We will add ws_interception config once we want to inspect into websocket sub protocols directly inside g3roxy.

However for all other http traffic we currently make use already of route_upstream to block traffic, rather than have it in the http interception configs. So I imagine you might rather want to have supported in that fashion?

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.

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.

Thanks. That will work for me.

[GlenDC]

so all the new feaures will only be available in the next 1.11 development vesion

Works for me.

You can extend the inspect policy enum to be an ACL like struct data type and add domain based rules.

Even better. Before I start with the real work of enabling that and hooking it up to the config and whatnot, do you mean to have the ProtocolInspectPolicy be this:

#[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.

[zh-jq]

Before I start with the real work of enabling that and hooking it up to the config and whatnot, do you mean to have the ProtocolInspectPolicy be this:

#[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 AclDstHostRuleSet but with the enum AclAction value replaced as enum InspectAction, which should be the original enum ProtocolInspectPolicy.

[GlenDC]

You mean to replace the current enum ProtocolInspectPolicy with:

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 acl module.

Also where do you want this inspect (type) code to live?

[zh-jq]

You mean to replace the current enum ProtocolInspectPolicy with:

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 acl module.

Yes. Maybe it could be AclDstHostRuleSet<InspectionAction>? I'm not sure whether it's possible/suitable.

Also where do you want this inspect (type) code to live?

If we can't reuse the ACL data types, it can go to the same module as the current ProtocolInspectPolicy.

[GlenDC]

Replacing ProtocolInspectPolicy with AclDstHostRuleSet<InspectionAction> you mean? Would break existing configs though, unless you also would want me to still support a direct InspectAction string as well when parsing the config.

And thus that would mean to redefine AclDstHostRuleSet as

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. Do tell me what you see best for me as an approach here. The details we can refine in the PR, but would be good to at least already decide if this is the direction you want (direction 1), or if you rather prefer to for now keep the entire inspect policy and its rules in to live independent from Acl in g3_dpi::config (direction 2).

[zh-jq]

Direction 1 the best, if possible. The breaking change in config maybe avoided by adding some workround code.

[GlenDC]

Ok I’ll get to work, I know enough for now, thx!

[GlenDC]

https://github.com/bytedance/g3/pull/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.

[GlenDC]

@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 Upgrade: websocket. Which should be basically as easy as saying "if domain = X and contains(Upgrade: websocket)" than send this response. Can be a pretty generic feature.

I could however not found anything of this kind of support in g3, or am I wrong? Whatever the case may be, while the current block policy does block any actual web socket traffic from passing on, it does so however at the cost of confusing potential web socket clients that might take it as a temporary connection error and just retry web sockets over and over again.

Curious to hear your opinion on this matter.

[zh-jq]

@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.

[GlenDC]

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?

[zh-jq]

But I am correct in thinking this will need an additional PR / patch, right?

Yes this should be considered as a BUG FIX.

This is not something you can do today? If so, can you guide me on how I would do that?

I can write a PR tomorrow.

If you want to fix it today, here are the code position that need to be updated:

H2:

https://github.com/bytedance/g3/blob/1f215ccfe3ddeba19c1ce62470f65f7e45e2aae5/g3proxy/src/inspect/http/v2/connect/extended.rs#L142

H1:

https://github.com/bytedance/g3/blob/1f215ccfe3ddeba19c1ce62470f65f7e45e2aae5/g3proxy/src/inspect/http/v1/upgrade/mod.rs#L152 https://github.com/bytedance/g3/blob/1f215ccfe3ddeba19c1ce62470f65f7e45e2aae5/g3proxy/src/inspect/http/v1/upgrade/mod.rs#L177

[GlenDC]

I was indeed messing around with some nested called code originated from pub(super) async fn forward_original<CW, UR, UW>(. A quick hack I did seemed to work. However I don't think I have a good enough understanding yet to do a clean bug fix on this one.

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!

[zh-jq]

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.

[GlenDC]

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.

[zh-jq-b]

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!

Here is the fix PR: https://github.com/bytedance/g3/pull/348

[GlenDC]

@zh-jq @zh-jq-b I merged it in my feature branch and also tested it in my demo. Maybe I'm wrong, but it seems the web socket policy is now always enforced for matching ACL rules, even if the incoming request in question is nothing to do with web sockets. Seems you do not check yet if it is an upgrade, and even more if the upgrade is related to web sockets. Unless I am wrong, but doesn't seem like it?

[GlenDC]

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());

[GlenDC]

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.

[zh-jq-b]

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.

[zh-jq-b]

@GlenDC you can try this new commit https://github.com/bytedance/g3/commit/b32bba65cc4cb3ae83bb47fc8e7fe64d956f01f3

[GlenDC]

No need to also update the run_extended_websocket function in g3proxy/src/inspect/http/v2/connect/extended.rs? Or is that already a code path that won't be reached in case of a block?

[zh-jq-b]

No need to also update the run_extended_websocket function in g3proxy/src/inspect/http/v2/connect/extended.rs? Or is that already a code path that won't be reached in case of a block?

That's already detected as a Websocket upgrade request, so we can just block there.

[GlenDC]

async fn check_blocked<CW>(&mut self, clt_w: &mut CW) -> ServerTaskResult<()> in code such as g3proxy/src/inspect/http/v1/upgrade/mod.rs is still buggy.

if upgrade_token_count == 0 { <= this can be 0 because there was never a token to begin with.

A sane fix would probably be to change signtature

pub fn retain_upgrade<F>(&mut self, retain: F) -> usize to pub fn retain_upgrade<F>(&mut self, retain: F) -> Option<usize>, in line with some std functions in rust. To indicate no upgrade headers were there in the first place? There are probably ten different approaches as well, but whichever choice you make, currently that code path is still blocking legit non-websocket traffic.

[GlenDC]

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");

[zh-jq-b]

async fn check_blocked<CW>(&mut self, clt_w: &mut CW) -> ServerTaskResult<()> in code such as g3proxy/src/inspect/http/v1/upgrade/mod.rs is still buggy.

if upgrade_token_count == 0 { <= this can be 0 because there was never a token to begin with.

The code is run with H1UpgradeTask which will only be created if upgrade header exists in the request.

There are probably ten different approaches as well, but whichever choice you make, currently that code path is still blocking legit non-websocket traffic.

What's the upgrade token used?

[zh-jq-b]

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");

You can submit a PR and I will approve it. It is reasonable for HttpTransparentRequest::retain_upgrade().

[GlenDC]

Something is going wrong though, and it is returning a log such as

... INFO auditor_name=default, daemon_name=, depth=3, dur_req_pipeline=68.876µs, intercept_type=HttpUpgrade, log_type=Intercept, origin_status=0, pid=1777316, received_at=...., request_id=1, rsp_status=500, task_id=..., uri=/, internal adapter error: upgrade protocol blocked by inspection policy

which indeed is seen on network interface as a http 500 status code. Even with the above patch applied I get it 100%. That error is only returned in 1 code loc AFAIK (g3proxy/src/inspect/http/v1/upgrade/mod.rs).

You can reproduce it yourself easily, just have a g3proxy running with this in your auditor config:

websocket_inspect_policy:
      child:
        default: intercept
        block:
          - "websocketstest.com"

Then go to https://websocketstest.com/, you'll get a 500 status code even for the regular network traffic.

[GlenDC]

Not sure if triggers in master branch with websocket_inspect_policy: block, but I imagine it should result in similar behaviour, But then again I would think not as otherwise I would hope some red flags in your tests should go off as that would block any traffic once websocket inspect policy is active.

[GlenDC]

Did a quick test... Seems to indeed block all traffic going through that code path as soon as you even just use websocket_inspect_policy: block. That's still on my own PR branch derivative though. Might be that I screwed something up and that your master branch does not get that.

[zh-jq-b]

Did a quick test... Seems to indeed block all traffic going through that code path as soon as you even just use websocket_inspect_policy: block. That's still on my own PR branch derivative though. Might be that I screwed something up and that your master branch does not get that.

I can open https://websocketstest.com/ with this result while testing with master branch:

Connected   No✘ Disconnected at 20:20:45

[GlenDC]

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.