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

feat: port forwarding support #60

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Conversation

cmars
Copy link
Contributor

@cmars cmars commented Jan 23, 2022

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

@cretz
Copy link
Owner

cretz commented Jan 24, 2022

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
Copy link
Contributor Author

cmars commented Jan 25, 2022

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
Copy link
Owner

cretz commented Jan 25, 2022

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 added a commit to cmars/onionpipe that referenced this pull request Jan 26, 2022
cmars added a commit to cmars/onionpipe that referenced this pull request Jan 26, 2022
@cmars
Copy link
Contributor Author

cmars commented Jan 27, 2022

@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/onionpipe#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?

tor/forward.go Outdated Show resolved Hide resolved
tor/forward.go Show resolved Hide resolved

// Forward as an onion service on test ports
conf := &tor.ForwardConf{
Version3: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that v2 services are past sunset and current releases of Tor will not allow these to be published. Integration tests which attempt to resolve or publish v2 hidden services are currently failing. I'll have a fix for these in a followup.

Copy link
Owner

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Thanks!

@cretz cretz merged commit 9267b2d into cretz:master Jan 28, 2022
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