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

feat: Support ShadowTLS v2 as Shadowsocks plugin #330

Merged
merged 26 commits into from
Jan 10, 2023
Merged

feat: Support ShadowTLS v2 as Shadowsocks plugin #330

merged 26 commits into from
Jan 10, 2023

Conversation

3andero
Copy link

@3andero 3andero commented Jan 9, 2023

implement shadow-tls as a shadowsocks plugin.

Introduction

With the implementation, a shadow-tls proxy could be defined as:

- name: stls
  type: ss
  server: [REDACTED]
  port: 443
  cipher: chacha20-ietf-poly1305
  password: [SHADOWSOCKS_PASSWORD]
  plugin: shadow-tls
  plugin-opts:
    host: "cloud.tencent.com"
    password: [SHADOW_TLS_PASSWORD]

where server is the address of your shadowsocks server, host is the domain name that you want to disguise as, [SHADOWSOCKS_PASSWORD] is the password of the ss-server and [SHADOW_TLS_PASSWORD] is the password of shadow-tls itself.

You can refer to shadow-tls for server side setups. The idea is straightforward though.
To make the above config work, here's what you need to do on your server:

  1. start the shadow-tls server (e.g., on 443):
    ./shadow-tls-x86_64-unknown-linux-musl server --server 127.0.0.1:24000 --tls cloud.tencent.com:443 --password [SHADOW_TLS_PASSWORD] --listen 0.0.0.0:443
    
  2. start the shadowsocks server (e.g., on 24000)
    ss-server -s 127.0.0.1 -p 24000 -m chacha20-ietf-poly1305 -k [SHADOWSOCKS_PASSWORD]
    
  3. done

Discussion

There were a few choices I made for my implementation:

  1. the plugin-opts:host doesn't seem to matter at all. You can provide an incorrect one but it still works. Technically I could have removed this entry but since shadow-tls requires this in its client config, I decided to keep it for consistency.
  2. The shadow-tls client needs to do a tls handshake with the server. For this part I just reused the StreamConn from trojan (see https://github.com/MetaCubeX/Clash.Meta/blob/7e05d5c34975e0335b578b2e30ee7f735344583d/transport/shadowtls/shadowtls.go#L111-L115). I believe this will make it easier to modify tls fingerprint for shadow-tls once we adopt utls for trojan.
  3. I fixed a bug in trojan (it didn't do handshake properly because of a typo).

transport/trojan/trojan.go Outdated Show resolved Hide resolved
buf.Write(hashVal)
buf.Write(b)
_, err := s.Conn.Write(buf.Bytes())
return len(b), err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May this return a wrong number when some errors occured and no bytes are written?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

@3andero 3andero Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, there's still a bug. I'll fix it right away

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that Conn.Write doesn't write all the buffer? I wrote based on the assumption that it's possible but that might look too defensive & verbose anyway.

Copy link
Author

@3andero 3andero Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs said a writer must return an error if n < len(buf). I'll clean those retries up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

transport/shadowtls/shadowtls.go Outdated Show resolved Hide resolved
@H1JK H1JK linked an issue Jan 10, 2023 that may be closed by this pull request
@wwqgtxx wwqgtxx changed the base branch from Meta to Alpha January 10, 2023 05:06
1. constant naming
2. import format
3. remove ephemeral trojan instance and add it as a shadowsocks struct field
4. incorrect numOfBytesWritten in ShadowTls::write and obfs::tls::write
5. incomplete write in ShadowTls::write and obfs::tls::write
@3andero 3andero requested a review from H1JK January 10, 2023 05:53
buf.Write(b)
remain := buf.Len()
for remain > 0 {
n, err := s.Conn.Write(buf.Bytes())
Copy link
Collaborator

@H1JK H1JK Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean n, err := s.Conn.Write(buf.Bytes()[buf.Len()-remain:])?
btw, I prefer this:

// start from L123
buf.Write(b)
n, err := s.Conn.Write(buf.Bytes())
if frontHeadroom := tlsHeaderLen + len(hashVal); n > frontHeadroom {
	return n - frontHeadroom, err
}
return 0, err

Copy link
Collaborator

@H1JK H1JK Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean n, err := s.Conn.Write(buf.Bytes()[buf.Len()-remain:])?

im outdated sry

Copy link
Author

@3andero 3andero Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out. I've made some change since this version.
The docs said a writer must return an error if n < len(buf).
I kinda suspect if it's worth it to return the exact number of bytes written since we'll return an error anyways.
Another reason I would prefer just return 0 is that once we have a partially written packet, the connection is poisoned.
For example, suppose the packet is 100 bytes long and we only write 50 bytes. The next call to write could try to write the remaining 50 bytes, but write will treat it as next packet (and add header (0x17...) to it). The server would take this as the remaining 50 bytes (it doesn't know that actually comes with a header & it expects 100 bytes) then the connection has doomed to be closed.
So in this case simply returning a 0, err would probably enough.

Copy link
Author

@3andero 3andero Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im outdated sry

LOL no worries at all!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

Copy link
Collaborator

@H1JK H1JK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @cubemaze how do you think?

This branch is now a little mess so please use squash when merge.

Reminder: This PR implements ShadowTLS v2 only.

@3andero
Copy link
Author

3andero commented Jan 10, 2023

This branch is now a little mess so please use squash when merge.

For Sure

@H1JK H1JK changed the title Impl shadow tls feat: Support ShadowTLS v2 as Shadowsocks plugin Jan 10, 2023
@3andero
Copy link
Author

3andero commented Jan 10, 2023

It seems that something's wrong with the CI. Is there anything I could do? I'll wait and merge once I get approved from @cubemaze and the CI fixed

@H1JK
Copy link
Collaborator

H1JK commented Jan 10, 2023

It seems that something's wrong with the CI. Is there anything I could do? I'll wait and merge once I get approved from @cubemaze and the CI fixed

You can ignore that error since PR workflow has no permission to operate delete and upload to releases. The build was succeed, that's enough.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Jan 10, 2023

LGTM.

@3andero
Copy link
Author

3andero commented Jan 10, 2023

It seems that something's wrong with the CI. Is there anything I could do? I'll wait and merge once I get approved from @cubemaze and the CI fixed

You can ignore that error since PR workflow has no permission to operate delete and upload to releases. The build was succeed, that's enough.

Thanks for letting me know! I'll be AFK for a couple of hours. Feel free to leave me messages and I'll respond later. It's a pleasure to work with you guys. I'm happy to update my implementation once ShadowTLS updates correspondingly.

@stitchrs stitchrs merged this pull request into MetaCubeX:Alpha Jan 10, 2023
@H1JK
Copy link
Collaborator

H1JK commented Jan 11, 2023

Is it possible for us to invent a share link format for shadow-tls plugin (following SIP002)?

Then it would be much easier for clients to import shadowsocks proxies with shadow-tls, and also good for our converter.

@msshn
Copy link

msshn commented Feb 20, 2023

Thanks for your great work, do you have any plan for ShadowTLS V3 ?

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Feb 20, 2023

@msshn Coming soon, just waiting.

@hiddify-com
Copy link

great thanks would you please add v3 too?

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Mar 5, 2023

@hiddify1 @msshn baaf509

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.

Proposal: Adding Shadow-TLS support
9 participants