Validate source of joiner, entire cluster used to crash when sent an …

…invalid host-port as the source field
esatterwhite · Dec 3, 2015 · 70a3b78 · 70a3b78
1 parent ba1ce0f
commit 70a3b78
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 5 deletions.
diff --git a/client.js b/client.js
@@ -20,6 +20,7 @@
 'use strict';
 
 var safeParse = require('./lib/util.js').safeParse;
+var validateHostPort = require('./lib/util.js').validateHostPort;
 var TChannel = require('tchannel');
 var TypedError = require('error/typed');
 
@@ -30,6 +31,12 @@ var ChannelDestroyedError = TypedError({
     channelType: null
 });
 
+var InvalidHostPortError = TypedError({
+    type: 'ringpop.client.invalid-hostport',
+    message: 'Request made with invalid host port combination ({hostPort})',
+    hostPort: null
+});
+
 function RingpopClient(subChannel) {
     this.subChannel = subChannel;
     this.isChannelOwner = false;
@@ -121,7 +128,14 @@ RingpopClient.prototype._request = function _request(opts, endpoint, head, body,
         return;
     }
 
-    this.subChannel.waitForIdentified({
+    if (!opts || !validateHostPort(opts.host)) {
+        callback(InvalidHostPortError({
+            hostPort: String(opts && opts.host)
+        }));
+        return;
+    }
+
+    self.subChannel.waitForIdentified({
         host: opts.host
     }, function onIdentified(err) {
         if (err) {

diff --git a/lib/util.js b/lib/util.js
@@ -153,6 +153,20 @@ function removeUnderscorePrefix(newStr, oldStr) {
     return (newStr.indexOf('_') === 0 && oldStr.indexOf('_') !== 0) ? newStr.substring(1) : newStr;
 }
 
+function validateHostPort(hostPort) {
+    var isString = typeof hostPort === 'string';
+    var parts = hostPort && hostPort.split(':');
+    var isColonSeparated = parts && parts.length === 2;
+    var isPort = parts && parts[1] &&
+        !isNaN(parseInt(parts[1], 10));
+
+    if (!isString || !isColonSeparated || !isPort) {
+        return false;
+    }
+
+    return true;
+}
+
 
 module.exports = {
     captureHost: captureHost,
@@ -164,5 +178,6 @@ module.exports = {
     safeParse: safeParse,
     toLispCase: replaceKeys.bind(null, camelCase, replaceWithLispCase),
     toSnakeCase: replaceKeys.bind(null, camelCase, replaceWithSnakeCase),
-    toCamelCase: replaceKeys.bind(null, lispOrSnakeCase, replaceWithCamelCase)
+    toCamelCase: replaceKeys.bind(null, lispOrSnakeCase, replaceWithCamelCase),
+    validateHostPort: validateHostPort
 };
diff --git a/server/protocol/join.js b/server/protocol/join.js
@@ -21,6 +21,7 @@
 
 var safeParse = require('../../lib/util').safeParse;
 var TypedError = require('error/typed');
+var validateHostPort = require('../../lib/util').validateHostPort;
 
 var BlacklistedError = TypedError({
     type: 'ringpop.invalid-join.blacklist',
@@ -43,13 +44,19 @@ var InvalidJoinAppError = TypedError({
     actual: null
 });
 
-var InvalidJoinSourceError = TypedError({
-    type: 'ringpop.invalid-join.source',
+var SelfJoinError = TypedError({
+    type: 'ringpop.self-join',
     message:  'A node tried joining a cluster by attempting to join itself.' +
         ' The joiner ({actual}) must join someone else.',
     actual: null
 });
 
+var InvalidJoinSourceError = TypedError({
+    type: 'ringpop.invalid-join.source',
+    message:  'A node tried joining a cluster with an invalid host-port ({actual})',
+    actual: null
+});
+
 function validateDenyingJoins(ringpop, callback) {
     if (ringpop.isDenyingJoins) {
         callback(DenyJoinError());
@@ -61,6 +68,13 @@ function validateDenyingJoins(ringpop, callback) {
 
 function validateJoinerAddress(ringpop, joiner, callback) {
     if (joiner === ringpop.whoami()) {
+        callback(SelfJoinError({
+            actual: joiner
+        }));
+        return false;
+    }
+
+    if (!validateHostPort(joiner)) {
         callback(InvalidJoinSourceError({
             actual: joiner
         }));

diff --git a/test/unit/index-test.js b/test/unit/index-test.js
@@ -24,6 +24,7 @@ var mock = require('../mock');
 var Ringpop = require('../../index.js');
 var test = require('tape');
 var testRingpop = require('../lib/test-ringpop.js');
+var allocRingpop = require('../lib/alloc-ringpop.js');
 
 var createAdminJoinHandler = AdminMember.memberJoin.handler;
 var createAdminLeaveHandler = AdminMember.memberLeave.handler;
@@ -200,7 +201,7 @@ test('protocol join disallows joining itself', function t(assert) {
         incarnationNumber: 1
     }), null, function(err) {
         assert.ok(err, 'an error occurred');
-        assert.equals(err.type, 'ringpop.invalid-join.source', 'a node cannot join itself');
+        assert.equals(err.type, 'ringpop.self-join', 'a node cannot join itself');
         ringpop.destroy();
         assert.end();
     });
@@ -475,3 +476,25 @@ test('first time member, not alive', function t(assert) {
     ringpop.destroy();
     assert.end();
 });
+
+var badHostPorts = ['I.AM.BAD.IP.123', null];
+badHostPorts.forEach(function each(hostPort) {
+    test('don\'t call tchannel with invalid host port pair', function t(assert) {
+        assert.plan(1);
+
+        var ringpop = allocRingpop();
+
+        ringpop.client.protocolPing({
+                host: hostPort
+            }, 
+            {},
+            function onPing(err, res) {
+                console.log(err);
+                assert.equals(err.type, 'ringpop.client.invalid-hostport',
+                    'get an ringpop.client.invalid-hostport error');
+                ringpop.destroy();
+                assert.end();
+            }
+        );
+    });
+});
diff --git a/test/unit/server/protocol/join_test.js b/test/unit/server/protocol/join_test.js
@@ -42,3 +42,46 @@ test('join fails with blacklist error', function t(assert) {
         ringpop.destroy();
     });
 });
+
+var badHostPorts = ['127.0.0.1.3000', '127.0.0.1:0:3000', '127.0.0.1.3000', '12.34.56.78:abc'];
+
+badHostPorts.forEach(function each(hostPort) {
+    test('join fails with bad hostPort as source', function t(assert) {
+        var ringpop = new Ringpop({
+            app: 'ringpop',
+            hostPort: '127.0.0.1:3000'
+        });
+        var handleProtocolJoin = createProtocolJoinHandler(ringpop);
+        handleProtocolJoin(null, JSON.stringify({
+            app: 'ringpop',
+            source: hostPort,
+            incarnationNumber: 1
+        }), null, function onHandled(err) {
+            assert.ok(err, 'an error occurred');
+            assert.equals(err.type, 'ringpop.invalid-join.source');
+            assert.end();
+            ringpop.destroy();
+        });
+    });
+});
+
+var goodHostPorts = ['127.0.0.1:3001', '12.34.56.78:1234'];
+
+goodHostPorts.forEach(function each(hostPort) {
+    test('join succeeds with correctly formatted hostPort as source', function t(assert) {
+        var ringpop = new Ringpop({
+            app: 'ringpop',
+            hostPort: '127.0.0.1:3000'
+        });
+        var handleProtocolJoin = createProtocolJoinHandler(ringpop);
+        handleProtocolJoin(null, JSON.stringify({
+            app: 'ringpop',
+            source: hostPort,
+            incarnationNumber: 1
+        }), null, function onHandled(err) {
+            assert.ifError(err, 'an error occurred');
+            assert.end();
+            ringpop.destroy();
+        });
+    });
+});