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

Shadowsocks should send data along with header #232

Closed
Mygod opened this issue May 1, 2020 · 20 comments
Closed

Shadowsocks should send data along with header #232

Mygod opened this issue May 1, 2020 · 20 comments
Assignees

Comments

@Mygod
Copy link
Contributor

Mygod commented May 1, 2020

Currently, this implementation does handshake first, which is problematic for obfuscation, e.g.

let svr_s = match ProxyStream::connect(server.clone_context(), svr_cfg, addr).await {

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 2, 2020

Hmm, IV is already sent with the first packet:

  1. Client -> Server: target's Address
  2. Server -> Client: the first response packet.

@Mygod
Copy link
Contributor Author

Mygod commented May 2, 2020

Two connections should use independent IVs.

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 2, 2020

Yes, they are.

@Mygod
Copy link
Contributor Author

Mygod commented May 2, 2020

The first packet should contain the header/handshake as well as actual payload.

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 2, 2020

Hmm, then the ProxyStream should store the Address and send it with the first write call.

@Mygod
Copy link
Contributor Author

Mygod commented May 2, 2020

CC @madeye to double check on this.

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 2, 2020

Made a simple but a little bit ugly solution.

@Mygod
Copy link
Contributor Author

Mygod commented May 3, 2020

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 3, 2020

Server side doesn't have a handshake packet.
The first data response is sent with IV.

@Mygod
Copy link
Contributor Author

Mygod commented May 3, 2020

Cool just double checked via tcpdump and can confirm that both sides are doing it correctly with 45a3469.

@Mygod Mygod closed this as completed May 3, 2020
@Mygod
Copy link
Contributor Author

Mygod commented May 3, 2020

@madeye Maybe we should include this in official doc as well?

@madeye
Copy link
Contributor

madeye commented May 3, 2020

Yeah, we should do that.

@Mygod
Copy link
Contributor Author

Mygod commented May 15, 2020

@madeye
Copy link
Contributor

madeye commented Dec 11, 2020

Hmm, this fix caused another problem: shadowsocks/shadowsocks-android#2621.

We'd better have a delay control to send out the header, even if there's no data. Some underlying protocol requires this behavior like FTP.

@zonyitoo zonyitoo reopened this Dec 11, 2020
@zonyitoo
Copy link
Collaborator

Well, it would be quite tricky to do that in a Future based server... But it should be able to do.

@zonyitoo zonyitoo self-assigned this Dec 11, 2020
@madeye
Copy link
Contributor

madeye commented Dec 11, 2020

Yes, I tried to implement this myself at first, but found it's not easy.

@zonyitoo
Copy link
Collaborator

I am currently working on #326 . This issue will probably have to be fixed after #326 being done.

@Mygod
Copy link
Contributor Author

Mygod commented Dec 11, 2020

Considering this only covers a few use cases like FTP, maybe it is better to have a timeout of maybe 500ms, and an option to turn off this behavior (default on).

EDIT: Or a configurable timeout defaulting to 500ms, 0 indicating not waiting...

@madeye
Copy link
Contributor

madeye commented Dec 11, 2020

A fixed 500ms timeout looks good enough.

Given most of protocols today let client send the request first, I don't think we would see header only handshakes often.

@zonyitoo
Copy link
Collaborator

This 500ms is only applied on the client side: client -> remote. The other side of the tunnel on servers can still wait for the first response from the target remote servers.

zonyitoo added a commit that referenced this issue Dec 20, 2020
* Refactored and separate library into crates

- shadowsocks: the core feature of shadowsocks
- shadowsocks-service: library for building shadowsocks services
    - dns, http, redir, socks, tunnel
    - load balancer
- shadowsocks-rust: release binaries

fix #347

* unified DnsResolver implementation

* unified local service common parameters into ServiceContext

- ServiceContext is common parameters shared between all local
implementations
- Completely removed https local support

* add #292 reply attack protection

* migrated redir local server

* support customizing outbound socket bind address

* manager outbound socket should accepts connect_opts

* republic local implementations

* socks5 udp server should always listen to client address

* socks4 controlled by local-socks4 feature

* socks4 also obey mode configuration

* socks server tcp cannot be disable. add support of udp-bind-addr parameter

* add udp-bind-addr for customizing udp-relay bind-addr

* local-dns infra, support customizing resolver

* fully implements DNS relay server

* support binding to specific interface on Linux-like platform

* tcp cannot be disabled in socks

* enable local-flow-stat

* fixed windows build

* fixed android specific warnings and compile errors

* allow udp_only mode in socks5

* dns relay listens to both TCP and UDP, mode controls outbound upstreams

* dns relay retries twice if request failed

* doc

* fix DnsClient typo

* fix stream EncryptWriter bug

* allow disable logging output

updated dependencies

* add readme

* refine doc

* remove depending on trust-dns-client

* socks4/4a client

* allow socks5 udp_only mode, fixes compile warning

* create standalone socks5 UDP relay server

- socks5 UDP association full cone (NAT2)

* server udp relay supports full cone (NAT2)

* acl moved to crate root

* redir udp relay support full cone (NAT2)

* standard socks5 udp test must use tcp_and_udp mode

* set server context fields with pub APIs

* udp_max_associations and udp_timeout default value set in Config

* local dns resolver retry with fixed attempts

* max_udp_association keeps unlimited by default

* fixed logging binary name

* pops first exited future result for local and server

* update reverse target index cache

* fix ProxyClientStreamWriteHalf that allows sending empty buffers

ref #232

* remove unused import when socks4 is disabled

* make balancer become a globally shared object

* print plugin exit status

* control local, server, manager services in features
atkdef pushed a commit to atkdef/shadowsocks-rust that referenced this issue Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants