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

use LocalAddr for UDP associate bind address #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ge9
Copy link

@ge9 ge9 commented May 25, 2024

In UDP associate, the IP address used for connection from the client should be notified to client (at least, https://github.com/3proxy/3proxy is implemented in this way).
This also fixes IPv6 address ([::]) replied for IPv4 UDP associate request (in bytes, [5, 3, 0, 1, 0, 0, 0, 0, 0, 0]). Sorry this is wrong.

@ge9
Copy link
Author

ge9 commented May 25, 2024

Oh sorry I found there is another pull request of much the same purpose (#63) but I think my implementation is more appropriate because clients on other hosts require external IP address to use UDP associate bind address.

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 32.39437% with 48 lines in your changes missing coverage. Please review.

Project coverage is 62.55%. Comparing base (ebe069a) to head (8cfd2f7).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
handle.go 37.09% 33 Missing and 6 partials ⚠️
option.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   66.66%   62.55%   -4.11%     
==========================================
  Files          14       14              
  Lines         726      836     +110     
==========================================
+ Hits          484      523      +39     
- Misses        184      253      +69     
- Partials       58       60       +2     
Flag Coverage Δ
unittests 62.55% <32.39%> (-4.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ge9
Copy link
Author

ge9 commented Dec 2, 2024

@thinkgos Hi, can you merge this pull request? I still think this is better than #63 .

@ge9
Copy link
Author

ge9 commented Dec 2, 2024

(Sorry I mistakenly generated merge commit so I discarded that and did rebase instead. The code is unchanged.)

@thinkgos
Copy link
Member

thinkgos commented Dec 2, 2024

@ge9 sorry, I am busy recently! I have a idea, custom bind address that user can define, but the package give a default implement. so the fregie #63 implemt can easy define itself.

@ge9
Copy link
Author

ge9 commented Dec 2, 2024

But then it will be difficult to make go-socks5 listen to the wildcard address (0.0.0.0:XXXXX)?

@thinkgos
Copy link
Member

thinkgos commented Dec 3, 2024

But then it will be difficult to make go-socks5 listen to the wildcard address (0.0.0.0:XXXXX)?

listenUDP need *net.UDPAddr, we can define a custom function like ResolveUdpAddr , define some parameter, return the *net.UDPAddr and error, this is same as this PR .

here:

  ResolveUdpAddr(bindIp net.IP, request *Request) (*net.UDPAddr, error)

@ge9 impl:

  ResolveUdpAddr(bindIp net.IP, request *Request) (*net.UDPAddr, error) {
     return  &net.UDPAddr{IP: request.LocalAddr.(*net.TCPAddr).IP, Port: 0}, nil
 }

@fregie impl:

  ResolveUdpAddr(bindIp net.IP, request *Request) (*net.UDPAddr, error) {
    if bindIp == nil {
      return nil, errors.New("not support")
   }
     return net.ResolveUDPAddr("udp", bindIp.String()+":0")
  }

any other impl:

  ResolveUdpAddr(bindIp net.IP, request *Request) (*net.UDPAddr, error) {
     panic("implment me")
  }

maybe we can direct get *net.UDPConn?

   ListenUdp(bindIp net.IP, request *Request) (*net.UDPConn, error) 

@ge9
Copy link
Author

ge9 commented Dec 3, 2024

Ah, I see, that's great! Thank you for your idea.

@thinkgos
Copy link
Member

thinkgos commented Dec 3, 2024

@ge9 ResolveUdpAddr(not get real address) may be not a well name. when you implement it, think about it . thank you!

@thinkgos
Copy link
Member

@ge9 the code change so much, not resolve #63 #65.

@ge9
Copy link
Author

ge9 commented Dec 17, 2024

Oh, sorry, I've forgot to create a branch...

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.

2 participants