-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
add permissions for systemctl services clash-dashboard change to updated one
Update README.md
# Conflicts: # rules/provider/classical_strategy.go
Update README.md
Update flake.nix hash
not yet tested
transport/shadowtls/shadowtls.go
Outdated
buf.Write(hashVal) | ||
buf.Write(b) | ||
_, err := s.Conn.Write(buf.Bytes()) | ||
return len(b), err |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I actually copied this from https://github.com/MetaCubeX/Clash.Meta/blob/3645fbf16161b12e2c2a762155be56130868e901/transport/simple-obfs/tls.go#L98-L113
I guess I'll fix them altogether.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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
buf.Write(b) | ||
remain := buf.Len() | ||
for remain > 0 { | ||
n, err := s.Conn.Write(buf.Bytes()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right.
There was a problem hiding this 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.
For Sure |
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. |
LGTM. |
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. |
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. |
Thanks for your great work, do you have any plan for ShadowTLS V3 ? |
@msshn Coming soon, just waiting. |
great thanks would you please add v3 too? |
implement shadow-tls as a shadowsocks plugin.
Introduction
With the implementation, a shadow-tls proxy could be defined as:
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:
Discussion
There were a few choices I made for my implementation:
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.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 adoptutls
for trojan.