-
Notifications
You must be signed in to change notification settings - Fork 64
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 shadow-tls(v3 only) #346
Conversation
here is the shadow-tls's server startup script(via docker-compose), you may test or use it by running this script and copy&paste the output proxy config of this outbound. https://gist.github.com/VendettaReborn/d8f8b546241e259f1cb18ce7b36b4106 |
|
||
let r = fake_request(tls).await; | ||
|
||
return Err(io::Error::new(io::ErrorKind::Other, format!("V3 strict enabled: traffic hijacked or TLS1.3 is not supported, fake request, res:{:?}", r))); |
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 not need to check the result before turning Error?
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.
actually, even if the fake_request succeeds, this function shall also return error, so i just combined them together.
the original client code can be found in shadow-tls native client
but it's true that by returning the error with '?', we can make the code more readable.
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.
i see. or just put a comment, the proto it self is a bit confusing here
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.
yep, i've fixed the logic and added some comments here
@@ -46,6 +46,15 @@ impl TryFrom<&OutboundShadowsocks> for AnyOutboundHandler { | |||
.try_into() | |||
.map(OBFSOption::V2Ray) | |||
.ok(), | |||
"shadow-tls" => s |
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.
mind adding a docker test for this?
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.
Alright, but need some modification of docker test. Since the shadow-tls is a plugin of shadowsocks, we need to extend the docker test's ability by enabling it to start multiple images 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.
or we shall create a new docker images, including the binary of shadowsocks&shadow-tls. what do you think
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.
right. another option might be extending our tests to support building a local image and we make a Dockerfile for ss and the plugin.
this can be done in a separate PR later I guess.
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.
i've extended the test utility and run multiple images at the same time as the preparation for the docker test. you can have a quick look, shall i merge it in this PR or in a separate one?
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.
yeah i think it's better to separate for easier review
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.
alright
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. plz resolve the conflicts
mention roadmap #190 |
implement shadow-tls protocol's client(v3)
🤔 This is a ...
🔗 Related issue link
#321
💡 Background and solution
see the background mentioned in the issue.
solution:
Tests has been done in my ubuntu 22.04.
📝 Changelog
Support shadow-tls(v3 only)
☑️ Self-Check before Merge