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

DialerProxy: Fix SplitHTTP H3 dialerProxy #3570

Merged
merged 8 commits into from
Aug 11, 2024
Merged

Conversation

mmmray
Copy link
Collaborator

@mmmray mmmray commented Jul 21, 2024

Summary for users: If you have Fragment turned on in v2rayNG and other clients, splithttp+h3 configs may not connect. With this PR, this bug is fixed.


I implemented the suggestions in #3560, but I ran into some difficulties:

@dyhkwong
Copy link
Contributor

dyhkwong commented Jul 22, 2024

net.Conn returned by internet.DialSystem can be anything that wraps as a net.Conn. If someone writes a custom UseAlternativeSystemDialer, it may not be a *cnc.connection. Should wrap the net.Conn returned by internet.DialSystem instead.

conn, err := internet.DialSystem(ctx, dest, streamSettings.SocketSettings)
if err != nil {
	return nil, err
}
var packetConn net.PacketConn
switch c := conn.(type) {
......
default:
	packetConn = &connWrapper{Conn: conn}
}
return quic.DialEarly(ctx, packetConn, conn.RemoteAddr(), tlsCfg, cfg)
type connWrapper struct {
	net.Conn
}

func (c *connWrapper) ReadFrom(p []byte) (n int, addr net.Addr, err error) {
	n, err = c.Read(p)
	return n, c.RemoteAddr(), err
}

func (c *connWrapper) WriteTo(p []byte, _ net.Addr) (n int, err error) {
	return c.Write(p)
}

func (c *connWrapper) LocalAddr() net.Addr {
	// return some random and unique things
}

For the warning in https://github.com/quic-go/quic-go/blob/c40d4ccb7fe00974095f06a7eb5ac00dd62b7f5b/sys_conn_buffers_write.go#L19, just ignore it (or implement a dummy SetWriteBuffer in the wrapper?)

@mmmray
Copy link
Collaborator Author

mmmray commented Jul 22, 2024

Thanks, this does seem cleaner. Howvever, when using the config files from #3560, I still get the same error as before even for a single connection:

[Info] [663055977] transport/internet: redirecting request udp:127.0.0.1:6001 to direct
[Debug] [663055977] transport/internet: dialing to udp:127.0.0.1:6001
[Info] [663055977] proxy/freedom: connection opened to udp:127.0.0.1:6001, local endpoint [::]:45120, remote endpoint 127.0.0.1:6001
[Info] [663055977] transport/internet/splithttp: failed to send upload > Post "https://127.0.0.1:6001/a12581e7-ef02-45ff-80e1-6b7257eeef38/0": http3: parsing frame failed: INTERNAL_ERROR (local): EOF
[Info] [663055977] transport/internet/splithttp: failed to send download http request > Get "https://127.0.0.1:6001/a12581e7-ef02-45ff-80e1-6b7257eeef38": http3: parsing frame failed: INTERNAL_ERROR (local): EOF

removing dialerProxy fixes it

@RPRX
Copy link
Member

RPRX commented Jul 24, 2024

SplitHTTP H3 不支持 dialerProxy 只是我不小心发现的,实际上似乎没有这样的使用场景,有时间可以先实现 #3560 (comment)

@dyhkwong
Copy link
Contributor

For dialerProxy itself:
Need ConnectionOutputMulti for TCP and ConnectionOutputMultiUDP for UDP. Maybe it never works for UDP before?

cnc.ConnectionOutputMulti(dr),

Wrong context used? Maybe using the ctx from getHTTPClient is correct.
conn, err := internet.DialSystem(ctx, dest, streamSettings.SocketSettings)

For a connection issue: I tried with minimal config
client.json
server.json
but still can't make splithttp3 work for trojan and vless (but vmess is ok). Connections always hang and reports context cancelled after a few seconds. Dialer reuse is problematic and reverting in 22535d8 make it work.

@alireza-2030
Copy link

It is true that the dialer doesn't work with splithttp h3, but there is another way to send splithttp h3 outbound to another outbound.
Here, I have considered freedom (direct) as the second outbound for simplicity, but I also tested it with vless and vmess and it was connected.

{
  "dns": {
    "hosts": {
      "domain:googleapis.cn": "googleapis.com"
    },
    "servers": [
      "8.8.8.8"
    ]
  },
  "inbounds": [
    {
      "listen": "127.0.0.1",
      "port": 10808,
      "protocol": "socks",
      "settings": {
        "auth": "noauth",
        "udp": true,
        "userLevel": 8
      },
      "sniffing": {
        "destOverride": [
          "quic",
          "http",
          "tls"
        ],
        "enabled": true
      },
      "tag": "socks"
    },
    {
      "listen": "127.0.0.1",
      "port": 10809,
      "protocol": "http",
      "settings": {
        "userLevel": 8
      },
      "tag": "http"
    },
{
      "listen": "127.0.0.1",
      "port": 23451,
      "protocol": "dokodemo-door",
      "settings": {
        "address": "ip or domain", // config domain or ip
            "port": 443,  // config port
         "network": "tcp,udp",  // udp is required
         "timeout": 0,
        "userLevel": 8,
        "followRedirect": false
      },
      "tag": "proxytodirect"
    }
  ],
  "log": {
    "loglevel": "none"
  },
  "outbounds": [
  {
      "mux": {
        "concurrency": 8,
        "enabled": false,
        "xudpConcurrency": 8,
        "xudpProxyUDP443": "allow"
      },
      "protocol": "vless",
      "settings": {
        "vnext": [
          {
            "address": "127.0.0.1",
            "port": 23451,
            "users": [
              {
                "encryption": "none",
                "flow": "",
                "id": "your-id", // config id
                "level": 8,
                "security": "auto"
              }
            ]
          }
        ]
      },
      "streamSettings": {
        "network": "splithttp",
        "security": "tls",
        "splithttpSettings": {
          "host": "your.host",  // config host
          "path": "/yourpath"  // config path
        },
        "tlsSettings": {
          "allowInsecure": false,
          "alpn": [
            "h3" // quic
          ],
          "fingerprint": "chrome",
          "serverName": "your.sni" // config sni
        }
      },
      "tag": "proxy"
    },
    {
      "protocol": "freedom",
      "settings": {},
      "tag": "direct"
    },
    {
      "protocol": "blackhole",
      "settings": {
        "response": {
          "type": "http"
        }
      },
      "tag": "block"
    }
  ],
  "policy": {
    "levels": {
      "8": {
        "connIdle": 300,
        "downlinkOnly": 1,
        "handshake": 4,
        "uplinkOnly": 1
      }
    },
    "system": {
      "statsOutboundUplink": true,
      "statsOutboundDownlink": true
    }
  },
  "routing": {
    "domainStrategy": "AsIs",
    "rules": [
   // rule to send dokodemo-door to direct.
{
        "inboundTag": [
          "proxytodirect"
        ],
        "outboundTag": "direct",
        "type": "field"
      },
    // It is important to set DNS and send it to the right outbound (i.e. the second outbound) if you put the domain in the address section of dokodemo-door.
      {
        "outboundTag": "direct",
        "port": "53",
        "type": "field"
      }
    ]
  },
  "stats": {}
}

A few important points:

1- If the second outbound has Mux, it must be xudpProxyUDP443 allow or skip.

2- Pay attention to dns, I explained it in the form of a comment in the code.

I used this method before to send warp (wireguard) to vless, and now I tested it with splithttp h3 and it connected without any problem.

@dyhkwong
Copy link
Contributor

dyhkwong commented Jul 29, 2024

It is true that the dialer doesn't work with splithttp h3, but there is another way to send splithttp h3 outbound to another outbound.

The discussion is about how to fix it rather than using another way to workaround it. The dialer does work, and dialerProxy will work after applying the changes I mentioned. It is dialer reuse(?) that causes a very basic configuration (without dialerProxy) having connection issues.

@alireza-2030
Copy link

It is true that the dialer doesn't work with splithttp h3, but there is another way to send splithttp h3 outbound to another outbound.

The discussion is about how to fix it rather than using another way to workaround it. The dialer does work, and dialerProxy will work after applying the changes I mentioned. It is dialer reuse(?) that causes a very basic configuration (without dialerProxy) having connection issues.

Pay attention to the first paragraph of my comment. I didn't say don't improve dialerproxy!
I temporarily taught a method for those who need it.
As I said: I used this method before to send warp (wireguard) to vless, and now I tested it with splithttp h3 and it connected without any problem.

Pay attention to what is written on the xray github page.
https://github.com/XTLS/Xray-core/releases/tag/v1.8.23
下个版本主要会引入 multiplex control,以及支持 H3 dialerProxy。目前我们计划等到 uQuic v0.1.0 时用其替换 quic-go。

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 9, 2024

I had to delete another comment of mine. Thanks to @dyhkwong's comments I got it working. But I have a different conclusion:

  • DialSystem is called with some context. This context is used to control cancellation of dialling itself, and in fact http3.Transport will cancel this context after dialling has returned a connection.
  • dialerProxy however uses this context to control the lifetime of the connection, because it spawns a goroutine go h.Dispatch(...). So when dial returns, and the dial ctx is cancelled, the entire connection breaks.
  • Many transports do not call DialSystem with a short-lived context, so it doesn't matter.

To reproduce this issue with dialerproxy, do this:

  1. call DialSystem with some fresh context, and wait until it returns
  2. cancel the context
  3. now, depending on whether a dialerProxy was used, the connection works or does not work

this explains why some workarounds worked:

  • Using the outer context of getHTTPClient: in some situations, this context lives longer and so the connection did not get terminated. but doing this workaround breaks connection reuse, so now connection reuse has to be disabled

Anyway, this PR now fixes dialerProxy without affecting QUIC connection reuse. I'm not 100% sure if any other uses of dialerProxy are affected though, and maybe it leaks connections in some other scenario now due to context.WithoutCancel...

I found that changing to ConnectionOutputMultiUDP has no effect for this issue, but v2fly does it too, and it seems to improve the situation in #2850, so I added it here.

@dyhkwong
Copy link
Contributor

dyhkwong commented Aug 10, 2024

GRPC and HTTP use ctx like this, is that your case?

gctx = c.ContextWithID(gctx, c.IDFromContext(ctx))
gctx = session.ContextWithOutbounds(gctx, session.OutboundsFromContext(ctx))
gctx = session.ContextWithTimeoutOnly(gctx, true)

hctx = c.ContextWithID(hctx, c.IDFromContext(ctx))
hctx = session.ContextWithOutbounds(hctx, session.OutboundsFromContext(ctx))
hctx = session.ContextWithTimeoutOnly(hctx, true)

And can you reproduce the connectivity issue? It even happens without dialerProxy.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 10, 2024

@dyhkwong it's pretty interesting. I tried this code snippet now, it seems that it does not fix http3 client reuse (but patching in WithoutCancel like I did in recent commits works fine).

I was not able to reproduce the general connectivity issue you described earlier. I am inclined to believe it only happens on some systems because there's also tests for this 😅

@mmmray mmmray changed the title [WIP] Fix SplitHTTP H3 dialerProxy Fix SplitHTTP H3 dialerProxy Aug 11, 2024
@mmmray mmmray marked this pull request as ready for review August 11, 2024 13:13
@yuhan6665 yuhan6665 changed the title Fix SplitHTTP H3 dialerProxy DialerProxy: Fix SplitHTTP H3 dialerProxy Aug 11, 2024
@yuhan6665 yuhan6665 merged commit 498d8eb into XTLS:main Aug 11, 2024
36 checks passed
@yuhan6665
Copy link
Member

Looks good to me, thanks all!
@dyhkwong consider raise a separate ticket for your issue to get to the bottom of it

leninalive pushed a commit to amnezia-vpn/amnezia-xray-core that referenced this pull request Oct 29, 2024
* wip

* wip

* formatting

* cnc connection no longer needs to be a Packetconn

* dialerProxy: do not cancel connection when Dial context is cancelled
@ghost ghost mentioned this pull request Nov 29, 2024
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

Successfully merging this pull request may close these issues.

5 participants