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

Implement UDP command. #12

Merged
merged 6 commits into from
Mar 8, 2022
Merged

Implement UDP command. #12

merged 6 commits into from
Mar 8, 2022

Conversation

yuguorui
Copy link
Contributor

Hi, I have been using fast-socks5 for quite a while, and it works great.

It's just a pity that UDP support is missing, I've done some work here, could you please write some comments?

Signed-off-by: yuguorui <yuguorui96@gmail.com>
Signed-off-by: yuguorui <yuguorui96@gmail.com>
Although most SOCKS5 clients (such as chrome, curl) don't care
about the address in the reply, setting the address will help
subsequent UDP ASSOCIATE implementations to include the correct
value in the corresponding reply.

Signed-off-by: yuguorui <yuguorui96@gmail.com>
Signed-off-by: yuguorui <yuguorui96@gmail.com>
Signed-off-by: yuguorui <yuguorui96@gmail.com>
Signed-off-by: yuguorui <yuguorui96@gmail.com>
@yuguorui
Copy link
Contributor Author

yuguorui commented Feb 10, 2022

BTW, you could run cargo test to see the basic unittests. 😊

@dizda
Copy link
Owner

dizda commented Feb 11, 2022

Hi @yuguorui,

Thank you for your PR.
Give me some time to review it, then I'll get back to you.

}
TargetAddr::Domain(ref domain, port) => {
debug!("TargetAddr::Domain");
if domain.len() > u8::max_value() as usize {
Copy link
Owner

Choose a reason for hiding this comment

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

u8::max_value() is deprecated, could you replace it with MAX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will fix that.

@dizda
Copy link
Owner

dizda commented Mar 8, 2022

It looks really good, thank you!
I don't have any real case scenario to test the UDP way though.

@dizda
Copy link
Owner

dizda commented Mar 8, 2022

Could you also update the README.md?

@dizda
Copy link
Owner

dizda commented Mar 8, 2022

Would it be possible to add a flag on the server::Config to toggle UDPAssociate support? (in order to disallow it)

@dizda dizda merged commit 0dc523b into dizda:master Mar 8, 2022
@dizda
Copy link
Owner

dizda commented Mar 8, 2022

I've merged that PR, you can submit new changed onto a new one 🙏

Thank you.

@yuguorui
Copy link
Contributor Author

Could you also update the README.md?

Of course, it's my pleasure.

@yuguorui
Copy link
Contributor Author

Would it be possible to add a flag on the server::Config to toggle UDPAssociate support? (in order to disallow it)

Yes, I also think it's a good idea to avoid code bloat, I'll do it asap.

@dizda
Copy link
Owner

dizda commented Mar 14, 2022

Thank you @yuguorui

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.

None yet

2 participants