search

cretz / bine

Go library for accessing and embedding Tor clients and servers
MIT License
761 stars 71 forks source link

feat: port forwarding support #60

Closed cmars closed 2 years ago

cmars commented 2 years ago

Could do some refactoring of Listen/Forward. If this is generally a good idea happy to tidy things up.

cretz commented 2 years ago

Thanks! I don't work on this library too much anymore tbh. But I think the best way to do this is to stop making ListenConf.LocalListener and ListenConf.RemotePorts required, and add a ForwardRemotePorts map[int][]string that forwards port keys to local addrs.

cmars commented 2 years ago

Let me know if I'm mistaken, but it looks like zero values for ListenConf.LocalPort and ListenConf.LocalListener are valid config, resulting in a listener that gets created on an unused port. How would I differentiate between that and "no listener, just forwarding"? I'm not quite sure what you have in mind for this.

As an alternative... in this PR there are a lot of duplicate fields between the "Forward" and "Listen"-type structs (I did a quick and dirty hack here to get cmars/oniongrok working). What if we refactor this into common "HiddenService"-config related structs, and embed in both types (Listen- and Forward-related) of structs? And then refactor out the common bits among Tor.Forward / Tor.Listen into private methods? That way Listen can be all about setting up a hidden net.Listener, Forward can be just about local port forwards, and they can reuse all the common parts (setting up a hidden service in Tor).

I think either way, we're likely looking at a breaking API change.

cretz commented 2 years ago

Let me know if I'm mistaken, but it looks like zero values for ListenConf.LocalPort and ListenConf.LocalListener are valid config, resulting in a listener that gets created on an unused port. How would I differentiate between that and "no listener, just forwarding"? I'm not quite sure what you have in mind for this.

@cmars - Hrmm, that seems true. I had a whole thing typed out about how we could reuse Listen by maybe adding DisableLocalListener bool to ListenConf, but it is clear my original intention of Listen was to return something that implements net.Listener which makes no sense for forwarding.

As an alternative... in this PR there are a lot of duplicate fields between the "Forward" and "Listen"-type structs (I did a quick and dirty hack here to get cmars/oniongrok working). What if we refactor this into common "HiddenService"-config related structs, and embed in both types (Listen- and Forward-related) of structs? And then refactor out the common bits among Tor.Forward / Tor.Listen into private methods? That way Listen can be all about setting up a hidden net.Listener, Forward can be just about local port forwards, and they can reuse all the common parts (setting up a hidden service in Tor).

After some thought, I agree with the idea but not exactly the impl. I don't think we want embeds (just copy the common fields, embeds is a backwards incompat change), but reusing any code internally is very reasonable if at all possible.

So, really, revisiting this PR, I think it's mostly good :-) Thanks for going in circles here with me, heh. I do think PortMap should change to a map[int][]string and remove LocalAddr that way people can map to other local addrs, unix paths, etc. Basically anything HiddenServicePort supports which will also make the code simple.

You may also want an integration test or two to confirm it works.

cmars commented 2 years ago

@cretz Updated the port mapping -- I found that it needs to be a map[string][]int as we're mapping from a single local socket (which can be TCP or UNIX, so it must be a string) to one or more onion TCP ports, which are always port numbers.

I've tested this in cmars/oniongrok#1 and it seems to work. What do you think?

I started going further down the path of refactoring out common parts in Listen and Forward. I think it's doable, but I'm getting quite a bit worried about landing such a drastic change without unit test coverage. Something to revisit in a followup?