Add proxy options to allow proxying tracker and peer connections

[yciabaud]

This PR is based on webtorrent/bittorrent-tracker#157 to proxy tracker connnections and adds proxying on TCP peers and webseeds.

WebRTC peers in webtorrent-hybrid is ok now electron-webrtc has been updated.

Options are structured like :

proxyOpts: {
    socksProxy: {
      proxy: {
        ipAddress: 'localhost',
        port: 1080,
        type: 5
      }
    },
    proxyTrackerConnections: true,
    proxyPeerConnections: false,
    httpAgent: {},
    httpsAgent: {}
}

Any thoughts?

Fixes #807

[yciabaud]

I believe it is ready now, some feedbacks?

[kewde]

@yciabaud

Thank you for your work on this.
Exactly what I was looking for :)

Kudos to you.

[romaincointepas]

@yciabaud Looks amazingly good! Any idea when this will be merged?

[yciabaud]

Well, it seems that @feross is not around these days, I am waiting for some feedbacks before merging webtorrent/bittorrent-tracker#157.

Then we will merge this... (hopefully)

[kewde]

Whilst this might not be on topic, proxy support is great for people who wish to run WebTorrent over I2P.

[feross]

@yciabaud I'm around, just really busy these days with other projects. I'll try to take a look soon.

[yciabaud]

No problem, this will wait.

[romaincointepas]

@yciabaud Testing this right now. Aren't we missing something like this to propagate the proxy options to the tracker client?

if (self.proxyOpts.proxyTrackerConnections && !self.tracker.proxyOpts) {
  self.tracker.proxyOpts = self.proxyOpts;
}

[yciabaud]

@romaincointepas The options should be passed to torrent-discovery moddule here: https://github.com/feross/webtorrent/blob/master/lib/torrent.js#L307

And from there to the tracker: https://github.com/feross/torrent-discovery/blob/master/index.js#L159

Do you have an issue with both this PR and the tracker one?

[romaincointepas]

@yciabaud Passing proxy settings via proxyOpts when creating a WebTorrent instance with proxyTrackerConnections enabled should make the tracker client automatically use those options without having to pass them again in tracker no?

At the moment, proxyOpts.proxyTrackerConnections is simply ignored.

[yciabaud]

@romaincointepas yes it should, are you testing with the bittorrent-tracker pr enabled?

[romaincointepas]

@yciabaud Yep, using https://github.com/yciabaud/webtorrent/tree/socks2 and https://github.com/yciabaud/bittorrent-tracker/tree/proxy.

[yciabaud]

Oh sorry @romaincointepas I did not get it at the first time. I am adding your snippet, thank you ;)

[romaincointepas]

self.tracker is already set to an empty object above (see https://github.com/yciabaud/webtorrent/blob/405a084da5406a59acf168460d038b449d26a5a3/index.js#L86), no need for this here?

[yciabaud]

self.tracker can still be false, but I should skip the forwarding if it is.

[romaincointepas]

Indeed, good catch!

[romaincointepas]

After some testing, there seems to be a problem with using a single instance of Socks.Agent for all http (or https) requests. When a request is done using the Socks.Agent instance, the instance updates some of its own properties (its proxy.target property containing the host and port), breaking all requests if multiple requests are done simultaneously with that same instance.

I made it to work by deep-copying the instance before each request, but there may be a better way (we need one instance of Socks.Agent per protocol/host/port combination, and we could save them for reuse and take advantage of the keepAlive depending on how frequent are the requests to each tracker).

I've also noticed that the Socks.Agent instance (httpAgent) can end up having proxy.command: 3 (which is for UDP only), so there may be something going on there as well, I'm investigating (UDP trackers do not seem to be working on my side in a real life scenario with with a mix of http, https and udp trackers).

Edit for UDP bug: I'm not a JS expert, but if https://github.com/JoshGlazebrook/socks/blob/master/lib/socks-agent.js#L7 references options instead of deep-copying whatever is passed when creating the instance, then that's probably what's messing up the proxy.command property (basically, httpAgent, httpsAgent and whatever is used for udp proxying all play with the same object that they modify every time it's used for a request).

[yciabaud]

You must be right, let's try to clone the options ourselves before passing it to socks.

[romaincointepas]

@yciabaud Got a working implementation: using the same instance of Sock.Agent for http and https proxying and Socks.Client modifying proxyOpts was indeed messing things up (not 100% sure about the Socks.Client one though, but it wasn't safe for sure).

If you are busy and if you are okay with it, I would like to take a shot at fixing this.

I also see two minor improvements that could be done, tell me what you think:

1. Remove the ability to pass http(s)Agents. I feel like HTTP proxying, if ever supported (it's rarely used for torrenting AFAIK), should be set the same way as SOCKS (by passing host, port and other options). For future reference, this looks good: https://github.com/TooTallNate/node-https-proxy-agent.
2. Not do anything that rely on UDP networking when using SOCKS 4/4a (only UDP trackers at the moment).

[yciabaud]

@romaincointepas good you made it work, sure you can push your work on my repo and I will merge it here.

Otherwise I can do it, tell me how you did it? Simply cloning the options?

For the improvements, I would rather keep the http agents to avoid having a new dependency in the project. I wanted to keep socks outside too but it was to complicated to handle.

I can do the improvement 2. by checking on socks version for UDP.

Tell me if you can manage to add your code to the PR.

[yciabaud]

OK @romaincointepas I made some changes in bittorrent-tracker:

• Check for socks version: webtorrent/bittorrent-tracker@55fa45a
• Clone options: webtorrent/bittorrent-tracker@c1e9640

Is it what you had in mind? I can do the same here too...

[romaincointepas]

@yciabaud Great!

Using https://www.npmjs.com/package/clone is cleaner as it offers deep-cloning of an object and will be needed for cloning the Socks.Agent "master" instances. It's a very light and commonly used dependencies, I think it's okay.

About the agents, I think they shouldn't be passed as options in proxyOpts (for both Webtorrent and the tracker client) but instead be generated as private properties when instantiating the Webtorrent and tracker client (as _http(s)Agent), see my reasoning in my previous comment about HTTP proxying. Then we can clone it when needed in http-trackers.js, webconn.js, etc.

Also, I didn't get your point about httpAgent and adding a dependency, I assume it's a misunderstanding?

About disabling some protocols/features, WebRTC should also be disabled if SOCKS has userid or authentication (forgot about that one 😓).

[yciabaud]

@romaincointepas are you saying you need to clone agents? Cloning options is not enough?

can you test the last commits I made?

[romaincointepas]

@yciabaud it does emit an error but doesn't stop WebRTC from working and ignoring the proxy settings also stops WebTorrent from working completely.

Uncaught Error: Uncaught, unspecified "error" event. (SOCKS Proxy must be version 5 with no authentication to work in electron-wrtc)

The question is: should using a SOCKS proxy with userid/auth or SOCKS 4 disable some capabilities (WebRTC only at the moment) or let those capabilities bypass the proxy? (IMO it should disable WebRTC, as a SOCKS proxy is usually used for privacy reasons).

[yciabaud]

@romaincointepas thank you very much for your help!

I believe too that we should just stop webrtc when the proxy cannot be used. Here is a commit in this way.

[romaincointepas]

• Replace https://github.com/yciabaud/webtorrent/blob/2a410fdd0f06bbe97269a192ceaaad2cc4214f44/index.js#L139-L140 with:

self.tracker.wrtc.electronDaemon.eval('require("electron").remote.getCurrentWebContents().session.setProxy(' +
    JSON.stringify(electronConfig) + ', function(){})', networkSettingsReady)

---

• websocket trackers are not actually using the SOCKS proxy in electron renderer process (https://github.com/feross/simple-websocket does not support setting an agent if WebSocket is available, it does however if using the ws module in node). Created an issue at feross/simple-websocket#12.

[yciabaud]

Ok i see why the second issue is occurring, my commit on simple-websocket is excluding electron... feross/simple-websocket@fcabf15

[yciabaud]

@romaincointepas can you test it with this revision? I used the electron-eval option to execute in the main process instead of pass it to the window and then use remote to come back in the main process.

I am looking forward to fix the second issue.

[romaincointepas]

@yciabaud Works!

[romaincointepas]

console.warn('You need to provide an electron-wrtc instance in opts.wrtc to use Socks proxy in electron -> WebRTC is disabled')

I don't think this warning should be thrown if self.tracker.wrtc === false.

[yciabaud]

You are right

[romaincointepas]

DHT doesn't seem to be currently proxied, is that correct?

(After taking a quick look, it seems that it should be done in https://github.com/mafintosh/k-rpc-socket/blob/master/index.js)

[yciabaud]

This is correct, I have not played around with dht... yet

[romaincointepas]

Can you also confirm that right now, when using a proxy, the client is not directly reachable by peers (meaning upload can only happens with peers that the client connects to)? (the listening port the client is identified by on the trackers, DHT and PEX can't be reached as it's the local listening port, whereas the IP is the proxy IP).

I assume most of the work involved would be with tcp-pool.js (by using Socks BIND instead of net.createServer(), announce that new proxy tcp port to trackers/DHT/PEX, and listen to new connections from that socket). Unfortunately SOCKS BIND seems to only be able to accept a single connection per newly opened port.

@yciabaud What's your take on this?

[yciabaud]

I agree with you @romaincointepas , Peer connexions and DHT/PEX should work with Socks BIND. I don't think we can open only one connection in this mode, do you have more information about this?

I wanted to merge this PR before going deep in inboud proxying but this is taking so loong!

@feross please can you have a look at this? I have worked hard to make it work, can you review the tracker part? webtorrent/bittorrent-tracker#157 I don't want to waste more time if you believe I am in the wrong way.

[JoshGlazebrook]

@yciabaud v2 of 'socks' is now available. Not sure if you're up for taking this on again, but usage of BIND/ASSOCIATE is now more sane than the prior version.

[yciabaud]

Thanks @JoshGlazebrook for this information, unfortunately I worked a lot on this one year ago and never got any feedback from the team so I left it.

I can still update my PRs if it gets some interest but I don't want to waste more time on this.

[romaincointepas]

@yciabaud I would definitely be interested in an updated PR (assuming it's not too much work on your side).

[kewde]

@yciabaud I think there are quite a few silent appreciators here..
👍

[Fenny]

@yciabaud Please don't give up! Trust me, this is the only thing webtorrent really needs at this point!

[brian123zx]

FWIW, I've been watching this pr for years now too 😄 Sad to see contribution die because @feross seems mia. Is there an active fork?