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

SplitHTTP: New padding mechanism #3643

Merged
merged 3 commits into from
Aug 10, 2024
Merged

Conversation

mmmray
Copy link
Collaborator

@mmmray mmmray commented Aug 5, 2024

  1. "ooook" handling on client is removed entirely, since there is no released version of xray-server that actually writes variable-length "ok"

    This partially reverts commit 4b7947c (SplitHTTP server: add ok padding #3614) -- servers that are on that commit from main are incompatible with clients on this PR

  2. X-Padding header is added to every request and response (in fact, all of them have length characteristics if the transported protocol has length characteristics already. it can be reduced once VLESS adds its own padding, but I thought it's best to stay independent of VLESS and also handle other inner layers)

  3. The client reads two bytes, and if those are != "ok", forwards them to VLESS/Trojan. This should be acceptable because VLESS/Trojan headers are larger than two bytes.

I think this is the simplest way to deal with this. If headers are used for communication, then browser dialer has to be adjusted as well (may be done in later PR)

1. "ooook" handling on client is removed entirely, since there is no
   released version of xray-server that actually writes variable-length
   "ok"

   This partially reverts commit
   4b7947c
   (XTLS#3614)

2. X-Padding header is added to every request and response (in fact, all
   of them have length characteristics if the transported protocol has
   length characteristics already. it can be reduced once VLESS adds its
   own padding, but I thought it's best to stay independent of VLESS and
   also handle other inner layers)

3. The client reads two bytes, and if those are != "ok", forwards them
   to VLESS/Trojan.

I think this is the simplest way to deal with this. If headers are used
for communication, then browser dialer has to be adjusted as well (may
be done in later PR)
@ll11l1lIllIl1lll
Copy link
Contributor

ll11l1lIllIl1lll commented Aug 6, 2024

Whether adding padding to all traffic exceeds the responsibility of the transport layer is debatable, as that could arguably be handled by the protocol itself. Padding at the transport might only be necessary during the handshake phase.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 6, 2024

Here is how I'm thinking about it. Without any padding in the transport:

  1. GET request always has length characteristics
  2. GET response has length characteristics if the download is mostly idle (for example, small fixed-len amount of bytes every 40 seconds)
  3. POST request has length characteristics if the upload is mostly idle
  4. POST response always has length characteristics

If download and upload are very busy, there is no problem. I don't want to make assumptions there.

Anyway, I don't have a strong opinion on which layer should handle it. VLESS can do it but I can't imagine how it does that effectively without knowing the transport. Maybe it's also good to consider vmess/Trojan for now (vmess is still useful, it's encrypted). I also think the issue extends beyond the handshake for upload.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 6, 2024

Random thought: If your latency requirements are low, you can also perform a lot of timing-and-sizing-related traffic shaping in a layer wrapping the transport (like fragment) you can delay bytes and flush them in bursts, but not add padding -- it will not be as good as built into the transport, but maybe it is more generic

@ahmad0489
Copy link

I think it is better to add a new parameter called paddingtype with ok and header values.
This parameter should be set on the server side.
I feel ok padding is better to use with cdn.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 6, 2024

@ahmad0489 please take a look at the preceding discussion here: #3614 (comment) -- this decision to phase out ok was already made there as it causes various problems regarding compatibility between client and server, and because it does not solve the problem completely. This sentence:

I feel ok padding is better to use with cdn

needs much stronger justification. If you cannot provide this justification publicly please send me an email (see profile). We're not going to add multiple forms of padding without any. Everybody has opinions.

@ahmad0489
Copy link

@ahmad0489 please take a look at the preceding discussion here: #3614 (comment) -- this decision to phase out ok was already made there as it causes various problems regarding compatibility between client and server, and because it does not solve the problem completely. This sentence:

I feel ok padding is better to use with cdn

needs much stronger justification. If you cannot provide this justification publicly please send me an email (see profile). We're not going to add multiple forms of padding without any. Everybody has opinions.

Yes, you're right, I didn't know you were going to phase out ok.
Based on the tests I did, I feel that ok padding is a little better.
Maybe this small difference is due to my unstable internet, that's why I said "I feel".

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 6, 2024

Based on the tests I did, I feel that ok padding is a little better.

better than this PR or better than no padding at all? the latter I believe immediately, but this PR should be completely equivalent (for as long as you use TLS, but unless specifically targeted, probably without as well)

@ahmad0489
Copy link

better than this PR or better than no padding at all? the latter I believe immediately, but this PR should be completely equivalent (for as long as you use TLS, but unless specifically targeted, probably without as well)

Ok padding is better than this PR and better than no padding.
Of course, these tests I did aren't enough and more people should test, that's why I requested to add paddingtype to be tested in both modes.
But if you have to phase out "ok" due to incompatibility, it is better to phase out it.

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

我觉得个别 CDN 早晚会通过简单地禁止 X-Padding header 来禁止 SplitHTTP,比如国内某 CDN 禁止过转发 WebSocket

However, I had a specific, useful setup where absolutely no headers are forwarded (Set-Cookie neither).

你指的是对 CDN 进行特殊的设置使它不转发任何 header 吗?还是反代软件?总之我觉得应当提醒用户避免这样的设置而不是接受这种情况,所以客户端检测有没有 X-Padding 就行了,没有的话就还是原逻辑,甚至兼容 ok padding 那个 commit

此外想把 padding 改名为 xPaddingHeader

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

xPaddingHeader

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

xPaddingHeader

以及支持设置 -1 以不发送这个 header,方便调试 CDN 有没有禁止它

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 7, 2024

I think some CDNs will sooner or later ban SplitHTTP by simply banning the X-Padding header.

I was thinking it's possible but didn't know how likely it is so I tried to ignore it. I will re-design this PR. When using Set-Cookie/Cookie, I was not sure how browser dialer will interact. I should actually finish browser dialer in this PR as well so I can see.

Some ideas:

  • Randomized header name (both length and content is random)
  • Set-Cookie: PHPSESSID=<randomstring> (it's too strange...)

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

@mmmray 可以先用 X-Padding,等有人报告 CDN 禁止了这个 header 再改,此外这个 PR 内 Browser Dialer 也要支持 X-Padding

@PoneyClairDeLune
Copy link
Contributor

Encode via ETag perhaps?

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 8, 2024

@RPRX

你指的是对 CDN 进行特殊的设置使它不转发任何 header 吗?还是反代软件?总之我觉得应当提醒用户避免这样的设置而不是接受这种情况,所以客户端检测有没有 X-Padding 就行了,没有的话就还是原逻辑,甚至兼容 ok padding 那个 commit

Are you referring to making special settings on the CDN so that it doesn't forward any headers? Or is it proxy software? In any case, I think users should be advised to avoid such settings rather than accepting this situation, so the client should check for the presence of X-Padding. If it's not there, then it should revert to the original logic, and even be compatible with the 'ok padding' commit.

The "CDN" does not permit the forwarding of headers, there are no settings. If the protocol is switched based on the presence of the header, I think it will cause more incompatibility:

  1. server sends X-Padding and no ok (a future version of the server may remove ok entirely, not this PR because I attempt to stay compatible between new server and old client)
  2. CDN strips the header
  3. client sees that there is no header, assumes the server is old and will fallback to the old logic. it attempts to read ok and fails

Maybe I don't understand the issue. Anyway, I really don't think the current logic in this PR was difficult to build, personally I find it easier to reason about compatibility this way, although I agree it is not elegant to wrap the reader.

The support for ooook was only removed because the server side is not released yet. it can actually be restored if that is wished.

The setting has been renamed to xPaddingHeader. Like the previous responseOkPadding and various other randomized settings, it takes values -1, "10-100" and 100.

Actually, because the padding can be disabled with -1, I believe it is not a good idea to read the header from the client to determine if ok should be read. Disable padding and the client will assume the server is old...

@PoneyClairDeLune

Encode via ETag perhaps?

I thought about it a lot now, and I feel that if X-Padding is blocked, the header name should just become configurable, or the user can just disable the padding. Maybe some custom anti-domestic-CDN padding can be added with custom reverse proxies, I am not sure.

@ll11l1lIllIl1lll
Copy link
Contributor

ll11l1lIllIl1lll commented Aug 8, 2024

Encode via ETag perhaps?

The typical length of an ETag is 32 characters or less. A 1000 characters ETag is an infrequent occurrence and the consequence of blocking it would be insignificant. Nevertheless, it is not uncommon for cookie lengths of 1000 or even 2000 characters to be encountered (for example, on dash.cloudflare.com or youtube.com, based on personal usage). Alternatively, adding superfluous query string (or pathname) parameters may be considered. 414 Request-URI Too Large

In addition to the above, I support suggestion of using custom padding headers to address different scenarios.

edit: But it seems that Sec-WebSocket-Protocol is usually no longer than 500, so it's probably okay to put padding anywhere, although there may be some weird CDN providers.

@RPRX
Copy link
Member

RPRX commented Aug 9, 2024

Browser Dialer 支持?

@RPRX
Copy link
Member

RPRX commented Aug 9, 2024

Maybe I don't understand the issue.

我的意思是,CDN 应该会转发 header,除非进行了特殊设置,比如:

I had a specific, useful setup where absolutely no headers are forwarded

我的意思是应当避免这样的特殊设置,不过不用纠结这个了,我觉得 ok 的逻辑不与 X-Padding 挂钩也行

@PoneyClairDeLune
Copy link
Contributor

Nevertheless, it is not uncommon for cookie lengths of 1000 or even 2000 characters to be encountered (for example, on dash.cloudflare.com or youtube.com, based on personal usage)

I'm all for using cookies for extensible configurations.

Might have suggested Authorization if not for the TSID proposal I submitted. Kinda soft-bricked myself with the SplitHTTP header PR until that's either merged or rejected.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 9, 2024

Now that I looked at browser dialer, I'm questioning this entire PR as well. Any custom header in browser dialer will trigger a CORS preflight request, which is bad for performance and is its own characteristic. The only header that can be used for padding there is Content-Type, but I think that is not acceptable for padding due to SSE.

current idea:

  • keep the ok padding
  • server responds to POST with oook as well (the client will already ignore the body)
  • client sends request padding as querystring (it's the only thing that does not trigger CORS) -- padding unfortunately has some size limitations this way

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

@mmmray 我觉得客户端用 querystring ?X-Padding= 是可行的,服务端还用 X-Padding header,字段名改成 xPaddingBytes

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

对了,昨晚在群里看到了 Nginx 默认 1MB 导致测速上行没速度的问题,@Fangliding 进行更多测试?

@Fangliding
Copy link
Member

我自己几乎不用nginx的 只是怀疑 毕竟卡1MB鬼知道会不会有MiB和MB之类的小问题或者刚好因为什么原因多一点点导致抛出错误 我叫他改2MB也没回我

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 10, 2024

I think this can be merged, header padding on every response, querystring padding on every request. Tested browser dialer. If RPRX doesn't do it I will tomorrow, just want to give everybody time to comment.

@RPRX RPRX merged commit a3b306a into XTLS:main Aug 10, 2024
36 checks passed
@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

终于又合并了一个 PR,我看了下代码,如果未来把 ok 删掉应该不会导致客户端卡住,不过得花数个版本才能把 ok 彻底删掉

反正现在客户端一定兼容任何版本的服务端,或许服务端加个逻辑,querystring 为空才发 ok?v1.9 把两端冗余的逻辑都删掉

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 10, 2024

I propose:

  1. release 1.8.24
  2. wait until clients are updated to this new version
  3. remove ok entirely on both ends
  4. release 1.9.0

No matter what we do, we have to wait until most clients are updated and do not get stuck if the server does not respond with ok. So Step 2 is where most of the time is spent anyway. I think releasing a version with conditional server response will not accelerate this process, it only adds more steps. Maybe I am missing something.

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

I think releasing a version with conditional server response will not accelerate this process, it only adds more steps. Maybe I am missing something.

确实有 missing something,现在服务端的逻辑是一定发 ok,如果我们在同一个版本比如 v1.9 把两端关于 ok 的逻辑都删掉,会造成 v1.9 客户端接收到上个版本服务端一定会发的 ok,所以应当现在就给服务端加个 conditional ok 的逻辑,v1.9 才能都删掉

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.

6 participants