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

Automagic socket sharing for net/tls connections via proxy server #702

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
d0d2e19
casing fixup
natevw Dec 17, 2014
a959ccf
rough w.i.p. streamplex/proxy client integration into net module
natevw Dec 17, 2014
461fce7
finish hooking ProxiedSocket up into net-tmp
natevw Dec 18, 2014
a84d76d
(broken at least due to streamplex require) start towards class clust…
natevw Dec 23, 2014
df6b08d
clean up tls suite output a bit
natevw Dec 23, 2014
1fd0f23
factor Socket subclasses in a different direction: must be handled in…
natevw Dec 23, 2014
88549e2
implement dynamic subclass conversion for net.Socket
natevw Dec 26, 2014
ea020f1
bit closer to being able to asyncronously switch Socket subclass
natevw Dec 26, 2014
3517f12
further progress and experiment with returning ProxiedSocket instance…
natevw Dec 26, 2014
5eaabb1
continued tweaking
natevw Dec 27, 2014
37b4ef6
properly forward needed events to pass test suite (most of the time…)
natevw Dec 27, 2014
68d177b
more event forwarding
natevw Dec 30, 2014
cb8ebfc
avoid test failure due to multi-chunk larger response
natevw Dec 31, 2014
c79791e
t.ok(true… -> t.pass(…
natevw Dec 31, 2014
73c4203
avoid delay of socket end event due to keep-alive
natevw Dec 31, 2014
df06435
use new tunnel-level auth, gets rid of now-extraneous tokenServer and…
natevw Dec 31, 2014
81e2541
buffer abstract socket methods and call them after subclass setup
natevw Dec 31, 2014
1de84f0
extra whitespace
natevw Dec 31, 2014
45ad7d5
fussing with require statements, start getting tls working
natevw Dec 31, 2014
e1de8fa
more dependency fussing
natevw Dec 31, 2014
23c6177
close tunnel when not in use (tls test now terminates)
natevw Dec 31, 2014
196ec35
use browserify to build streamplex into colony
natevw Jan 9, 2015
0f6fc6c
fix broken net error test (by upgrading to fixed streamplex) and clea…
natevw Jan 9, 2015
127c9db
fix up issue with tls/net circular import causing HTTPS agent to crea…
natevw Jan 10, 2015
d3466c8
finish getting HTTPS test to pass
natevw Jan 10, 2015
e0c804b
destroy tunnel when proxySocket has an error
natevw Jan 10, 2015
dd56bcf
pull out proxy server host/port config
natevw Jan 20, 2015
1698c33
get user-configuration stuff mostly into place
natevw Jan 22, 2015
769ff68
better defaults for proxy stuff (except not gonna try guess PROXY_CER…
natevw Jan 22, 2015
c3dc3d5
put as-deployed cert in place
natevw Jan 23, 2015
f5b4340
add proxy:false option to sockets, especially for the exciting use ca…
natevw Jan 26, 2015
7cec5f7
get rid of old non-TLS proxy test code, easy enough to tweak locally …
natevw Jan 26, 2015
f97fd03
fix bug introduced by rebase against TLS changes
natevw Jan 27, 2015
bcb8a19
add PROXY_TRUSTED opt-in, must be set non-zero before secure sockets …
natevw Feb 2, 2015
7c3421c
add PROXY_IDLE (and _PROXY_DEBUG) settings
natevw Feb 6, 2015
2ecc1fa
improve logging for local/proxied result
natevw Feb 6, 2015
24ec2b8
give proxySocket error to callback during initial tunnel setup (as al…
natevw Feb 6, 2015
aa1fe42
log auth response when debugging
natevw Feb 6, 2015
2fdfd78
default PROXY_IDLE to a minute and a half (seemed like a the next rou…
natevw Feb 13, 2015
fdc6ae3
add comment re. hardcoded cert
natevw Feb 20, 2015
f01660a
ProxiedSocket should re-emit close event too
natevw Feb 20, 2015
b615030
remove default PROXY_CERT (which is already expired) and make that se…
natevw Feb 24, 2015
2c8f8f4
add wrappers around main net/http suites to re-test under proxy
natevw Feb 25, 2015
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"miniz_path": "../deps/miniz",
"miniz_inc_path": "../deps/miniz-inc",
"node_libs_path": "../deps/node-libs",
"npm_bin_path": "../node_modules/.bin",
"approxidate_path": "../deps/approxidate",
"tools_path": "../tools",
'enable_ssl%': 0,
Expand Down
15 changes: 15 additions & 0 deletions config/libcolony.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,19 @@
'<(SHARED_INTERMEDIATE_DIR)/<(_target_name).c'
],
'actions': [
{
'action_name': 'bundle_streamplex',
'variables': {
'main_file': '<!(node -p "require.resolve(\'streamplex\')")'
},
'inputs': [
'<!@(<(npm_bin_path)/browserify --list --bare <(main_file))',
],
'outputs': [
'<(SHARED_INTERMEDIATE_DIR)/_streamplex.js',
],
'action': [ '<(npm_bin_path)/browserify', '--bare', '--standalone', 'streamplex', '<(main_file)', '-o', '<@(_outputs)'],
},
{
'action_name': '<(_target_name)_compile',
'inputs': [
Expand All @@ -148,6 +161,8 @@
'<(runtime_path)/colony/modules/http.js',
'<(runtime_path)/colony/modules/https.js',
'<(runtime_path)/colony/modules/net.js',
'<(runtime_path)/colony/modules/_net_proxied.js',
'<(SHARED_INTERMEDIATE_DIR)/_streamplex.js',
'<(runtime_path)/colony/modules/os.js',
'<(node_libs_path)/path.js',
'<(node_libs_path)/punycode.js',
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
"bindings": "~1.2.0",
"colony-compiler": "~0.6.23",
"mkdirp": "~0.3.5",
"semver": "^4.1.0"
"semver": "^4.1.0",
"streamplex": "^0.14.0"
},
"devDependencies": {
"nodejs-websocket": "^1.0.0",
"browserify": "^8.1.0",
"faye-websocket": "~0.7.2",
"nodejs-websocket": "^1.0.0",
"tap": "git+https://github.com/tcr/node-tap.git#4f96b1",
"tape": "~2.3.2",
"tinytap": "^0.2.0"
Expand Down
184 changes: 184 additions & 0 deletions src/colony/modules/_net_proxied.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
var util = require('util'),
events = require('events'),
net = require('net'),
tls = require('tls'),
streamplex = require('_streamplex');

// NOTE: this list may not be exhaustive, see also https://tools.ietf.org/html/rfc5735#section-4
var _PROXY_LOCAL = "10.0.0.0/8 172.16.0.0/12 192.168.0.0/16 169.254.0.0/16 127.0.0.0/8 localhost";

var _PROXY_DBG = ('_PROXY_DBG' in process.env) || false,
PROXY_HOST = process.env.PROXY_HOST || "proxy.tessel.io",
PROXY_PORT = +process.env.PROXY_PORT || 443,
PROXY_TRUSTED = +process.env.PROXY_TRUSTED || 0,
PROXY_TOKEN = process.env.PROXY_TOKEN || process.env.TM_API_KEY,
PROXY_LOCAL = process.env.PROXY_LOCAL || _PROXY_LOCAL,
Copy link
Member

Choose a reason for hiding this comment

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

Want to move splitting logic to here?

PROXY_LOCAL = (process.env.PROXY_LOCAL || _PROXY_LOCAL).split(' ')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not particularly, the env variable as defaulted here gets processed all in once place later I don't see a compelling reason to split the split part of that logic away?

PROXY_IDLE = +process.env.PROXY_IDLE || 90e3,
PROXY_CERT = process.env.PROXY_CERT || null;

/**
* Tunnel helpers
*/

function createTunnel(cb) {
if (_PROXY_DBG) console.log("TUNNEL -> START", new Date());
tls.connect({host:PROXY_HOST, port:PROXY_PORT, proxy:false, ca:(PROXY_CERT && [PROXY_CERT])}, function () {
var proxySocket = this,
tunnel = streamplex(streamplex.B_SIDE);
tunnel.pipe(proxySocket).pipe(tunnel);
proxySocket.on('error', shutdownTunnel);
proxySocket.on('close', shutdownTunnel);
proxySocket.on('error', cb);

var idleTimeout;
tunnel.on('inactive', function () {
if (_PROXY_DBG) console.log("TUNNEL -> inactive", new Date());
idleTimeout = setTimeout(shutdownTunnel, PROXY_IDLE);
Copy link
Member

Choose a reason for hiding this comment

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

Should idleTimeout be cleared if it is already set? (Failsafe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The timeout is cleared by the 'active' event which would have to happen before another 'inactive' one.

});
tunnel.on('active', function () {
if (_PROXY_DBG) console.log("TUNNEL -> active", new Date());
clearTimeout(idleTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

To be failsafey, maybe this?

if (idleTimeout != null) clearTimeout(idleTimeout);
idleTimeout = null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if clearTimeout follows spec, it should be just fine to call it with null (or any other invalid!) values.

});

tunnel.sendMessage({token:PROXY_TOKEN});
tunnel.once('message', function (d) {
if (_PROXY_DBG) console.log("TUNNEL: auth response?", d);
proxySocket.removeListener('error', cb);
if (!d.authed) cb(new Error("Authorization failed."));
else cb(null, tunnel);
});
function shutdownTunnel(e) {
if (_PROXY_DBG) console.log("TUNNEL -> STOP", new Date());
tunnel.destroy(e);
if (this !== proxySocket) proxySocket.end();
proxySocket.removeListener('close', shutdownTunnel);
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be removed as the 'error' listener, and wherever else it's added?

Copy link
Member

Choose a reason for hiding this comment

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

I guess including clearTimeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the 'close' listener is specifically to prevent shutdownTunnel from getting called twice during an 'error'->'close' sequence; GC should take care of the rest.

}
}).on('error', cb);
}

var tunnelKeeper = new events.EventEmitter();
Copy link
Member

Choose a reason for hiding this comment

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

I am the Tunnel Master I am the Gate Keeper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit ecdoaf6bf5a2b1


tunnelKeeper.getTunnel = function (cb) { // CAUTION: syncronous callback!
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have to be synchronous? (Rather than, process.nextTick)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is internal code, I didn't see a reason to artificially delay protoForConnection and Socket.prototype.connect (the only callers of this helper) when a tunnel is already active. Reasonable enough?

if (this._tunnel) return cb(null, this._tunnel);

var self = this;
if (!this._pending) createTunnel(function (e, tunnel) {
delete self._pending;
Copy link
Member

Choose a reason for hiding this comment

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

self._pending = false would be better by Colony semantics

if (e) return self.emit('tunnel', e);

self._tunnel = tunnel;
tunnel.on('close', function () {
self._tunnel = null;
});
var streamProto = Object.create(ProxiedSocket.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, why object.create instead of new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

streamProto becomes the prototype for an already-existing socket object (see @tcr's "I hate you" comment above ;-) and this bypasses calling the actual constructors again, only preparing the [think of it like a] "mix-in" we now need.

If it weren't for needing a link between socket instances and their associated tunnel (next two lines) the logic that uses this would just ProxiedSocket.prototype directly instead.

streamProto._tunnel = tunnel;
tunnel._streamProto = streamProto;
self.emit('tunnel', null, tunnel);
Copy link
Member

Choose a reason for hiding this comment

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

I'd call you out on doing null, tunnel rather than tunnel and later this.once('tunnel', cb.bind(null, null)) and this.once('error', cb) but I'm assuming this is basically an "internal" event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an internal event, whose parameters are always (err, tunnel) as you see also above in line 83. It's only used within the confines of this function as a means to avoid manually collecting an array of pending callbacks.

});
this._pending = true;
this.once('tunnel', cb);
};

var local_matchers = PROXY_LOCAL.split(' ').map(function (str) {
Copy link
Member

Choose a reason for hiding this comment

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

See PROXY_LOCAL splitting comment earlier

Copy link
Member

Choose a reason for hiding this comment

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

Incidentally, can we get a test case in for the local_matchers generator? Since it's a lot of bit fiddling, it'd be good to ensure it never regresses subtley

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah…agree this is a pretty reasonable thing to have test coverage for. I suppose I'll need to refactor this a bit and expose via an [underscored] property on the [underscored] module for it.

var parts = str.split('/');
if (parts.length > 1) {
// IPv4 + mask
var bits = +parts[1],
mask = 0xFFFFFFFF << (32-bits) >>> 0,
base = net._ipStrToInt(parts[0]) & mask; // NOTE: left signed to match test below
return function (addr, host) {
return ((addr & mask) === base);
};
} else if (str[0] === '.') {
// base including subdomains
str = str.slice(1);
return function (addr, host) {
var idx = host.lastIndexOf(str);
return (~idx && idx + str.length === host.length);
};
} else return function (addr, host) {
// exact domain/address
return (host === str);
}
});

function protoForConnection(host, port, opts, cb) { // CAUTION: syncronous callback!
Copy link
Member

Choose a reason for hiding this comment

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

Same: can just be made to always be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be if the optimization above is removed.

var addr = (net.isIPv4(host)) ? net._ipStrToInt(host) : null,
force_local = !PROXY_TOKEN || (opts._secure && !PROXY_TRUSTED) || (opts.proxy === false),
local = force_local || local_matchers.some(function (matcher) { return matcher(addr, host); });
if (_PROXY_DBG) {
if (force_local) console.log(
"Forced to use local socket to \"%s\". [token: %s, secure/trusted: %s/%s, opts override: %s]",
host, Boolean(PROXY_TOKEN), Boolean(opts._secure), Boolean(PROXY_TRUSTED), (opts.proxy === false)
);
else console.log("Proxied socket to \"%s\"? %s", host, !local);
}
if (local) cb(null, net._CC3KSocket.prototype);
else tunnelKeeper.getTunnel(function (e, tunnel) {
if (e) return cb(e);
cb(null, tunnel._streamProto);
});
}

/**
* ProxiedSocket
*/

function ProxiedSocket(opts) {
if (!(this instanceof ProxiedSocket)) return new ProxiedSocket(opts);
net.Socket.call(this, opts);
this._tunnel = this._opts.tunnel;
this._setup(this._opts);
}
util.inherits(ProxiedSocket, net.Socket);

ProxiedSocket.prototype._setup = function () {
var type = (this._secure) ? 'tls' : 'net';
this._transport = this._tunnel.createStream(type);

var self = this;
// TODO: it'd be great if we is-a substream instead of has-a…
this._transport.on('data', function (d) {
var more = self.push(d);
if (!more) self._transport.pause();
});
this._transport.on('end', function () {
self.push(null);
Copy link
Member

Choose a reason for hiding this comment

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

Is self._transport need any cleanup past 'end'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure this is what you're asking, but ProxiedSocket may never emit a 'close' event…investigating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0674dbe

});

function reEmit(evt) {
self._transport.on(evt, function test() {
var args = Array.prototype.concat.apply([evt], arguments);
self.emit.apply(self, args);
});
}
['connect', 'secureConnect', 'error', 'timeout', 'close'].forEach(reEmit);
};

ProxiedSocket.prototype._read = function () {
this._transport.resume();
};
ProxiedSocket.prototype._write = function (buf, enc, cb) {
this._transport.write(buf, enc, cb);
};

ProxiedSocket.prototype._connect = function (port, host) {
this.remotePort = port;
this.remoteAddress = host;
this._transport.remoteEmit('_pls_connect', port, host);
Copy link
Member

Choose a reason for hiding this comment

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

OP PLS

};

ProxiedSocket.prototype.setTimeout = function (msecs, cb) {
this._transport.remoteEmit('_pls_timeout', msecs);
if (cb) {
if (msecs) this.once('timeout', cb);
else this.removeListener('timeout', cb);
}
};

ProxiedSocket.prototype.destroy = function () {
this._transport.destroy();
this.end();
};

exports._protoForConnection = protoForConnection;
Loading