VMess protocol support #632
Conversation
garmr-ulfr
left a comment
garmr-ulfr
left a comment
There was a problem hiding this comment.
LG! Just a few comments, and VMessAddr should be added here.
For the code in outbound.go and server.go that was copied over, is every function needed? Some aren't referenced anywhere else. Would it still work if we removed them?
|
@garmr-ulfr thanks for the review. all points addressed |
|
complements getlantern/flashlight#1453 |
|
@garmr-ulfr @hwh33 It's been tested with the linked Flashlight PR |
There was a problem hiding this comment.
I haven't got through it all in detail yet, there's a lot going on here.
Side note: why did the author copy and include stuff from the go stdlib?? Some, like common.format is basically a stripped down fmt, common/binary.go even still has the Go Author copyright header. And they call panic quite a bit, be it directly or through helper functions. So there's a plethora of places a panic can happen, and with nothing to let consumers know so caller can handler it. It's mental...
protoListeners.go
Outdated
| p.wrapMultiplexing(p.listenShadowsocks), | ||
| }, | ||
| {"water", p.WaterAddr, p.listenWATER}, | ||
| {"vmess", p.VMessAddr, p.listenVMess(p.listenTCP)}, |
There was a problem hiding this comment.
Do we need to enable multiplexing for vmess?
There was a problem hiding this comment.
honestly - I don't know. any pointers?
There was a problem hiding this comment.
I'm not really sure either. I faintly remember @hwh33 saying that we multiplex everything, but I don't know. He'd be the one to answer this.
There was a problem hiding this comment.
Yeah we multiplex everything at this point. I would just bake that in here and on the flashlight side.
There was a problem hiding this comment.
This is the only remaining item needed on this PR @reflog
| service *vmess.Service[int] | ||
| } | ||
|
|
||
| func NewVMessListener(baseListener net.Listener, uuids []string) (net.Listener, error) { |
There was a problem hiding this comment.
Can you add documentation? Specifically, what are the UUIDs? How are we supposed to know what to configure here?
There was a problem hiding this comment.
Looking at the test, it appears that each client would have to present its own UUID which needs to be pre-configured on the proxy. Is that correct? How do you plan to achieve that?
There was a problem hiding this comment.
@hwh33 each proxy is configured with X UUIDs.
A random UUID from those X is returned when client requests proxy config.
I'll add a comment
There was a problem hiding this comment.
What happens when two clients are assigned the same UUID for a proxy? Or do we just assign so many that the odds of that are negligible?
There was a problem hiding this comment.
@hwh33 no, it's no 1-to-1, think of it more of a 'password' and less 'user-id'.
you can have just one, but I think adding some entropy is always nicer
There was a problem hiding this comment.
There's still no documentation @reflog
There was a problem hiding this comment.
@hwh33 doh. slipped my mind. will add in a followup PR
| conn, err := l.Listener.Accept() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| h := &handler{} | ||
| err = l.service.NewConnection(context.Background(), conn, metadata.Socksaddr{}, nil, h) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return h.conn, nil |
There was a problem hiding this comment.
This is nice and simple compared to what we expected!
|
@hwh33 done |
This PR adds VMess protocol support based on https://github.com/SagerNet/sing-vmess package.