From 124047f1e6ad8e3492ee22ea90324ab253a9148a Mon Sep 17 00:00:00 2001 From: Stefan-Gabriel Muscalu Date: Fri, 22 Jul 2016 12:06:07 +0300 Subject: [PATCH 1/4] Fix: if there are duplicate hosts in the known_hosts file, we return on the first one and not call the callback on `loadFingerprint` more than once --- src/v1/internal/ch-node.js | 2 +- test/internal/tls.test.js | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/v1/internal/ch-node.js b/src/v1/internal/ch-node.js index 401147cfa..a8f9c4924 100644 --- a/src/v1/internal/ch-node.js +++ b/src/v1/internal/ch-node.js @@ -45,7 +45,7 @@ function loadFingerprint( serverId, knownHostsPath, cb ) { require('readline').createInterface({ input: fs.createReadStream(knownHostsPath) }).on('line', (line) => { - if( line.startsWith( serverId )) { + if( !found && line.startsWith( serverId )) { found = true; cb( line.split(" ")[1] ); } diff --git a/test/internal/tls.test.js b/test/internal/tls.test.js index ccd61bd9e..80999928e 100644 --- a/test/internal/tls.test.js +++ b/test/internal/tls.test.js @@ -76,6 +76,41 @@ describe('trust-on-first-use', function() { var driver; + it('should not throw an error if the host file contains two host duplicates', function(done) { + 'use strict'; + // Assuming we only run this test on NodeJS with TOFU support + if( !hasFeature("trust_on_first_use") ) { + done(); + return; + } + + // Given + var knownHostsPath = "build/known_hosts"; + if( fs.existsSync(knownHostsPath) ) { + fs.unlinkSync(knownHostsPath); + } + + fs.writeFileSync( + knownHostsPath, + 'localhost:7687 abcd\n' + + 'localhost:7687 abcd' + ); + + driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), { + encrypted: true, + trust: "TRUST_ON_FIRST_USE", + knownHosts: knownHostsPath + }); + + // When + driver.session().run("RETURN true AS a").then( function(data) { + // Then we get to here. + // And then the known_hosts file should have correct contents + expect( data.records[0].get('a') ).toBe( true ); + done(); + }); + }); + it('should accept previously un-seen hosts', function(done) { // Assuming we only run this test on NodeJS with TOFU support if( !hasFeature("trust_on_first_use") ) { From 6723da86e657174897867e4fa745bd28050d0dfb Mon Sep 17 00:00:00 2001 From: Stefan-Gabriel Muscalu Date: Fri, 22 Jul 2016 12:57:58 +0300 Subject: [PATCH 2/4] FIx: host fingerprint same tick duplication --- src/v1/internal/ch-node.js | 20 +++++++++++++++++++- test/internal/tls.test.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/v1/internal/ch-node.js b/src/v1/internal/ch-node.js index a8f9c4924..d2c75de68 100644 --- a/src/v1/internal/ch-node.js +++ b/src/v1/internal/ch-node.js @@ -56,12 +56,30 @@ function loadFingerprint( serverId, knownHostsPath, cb ) { }); } -function storeFingerprint(serverId, knownHostsPath, fingerprint) { +const _lockFingerprintFromAppending = {}; +function storeFingerprint( serverId, knownHostsPath, fingerprint ) { + // we check if the serverId has been appended + if(!!_lockFingerprintFromAppending[serverId]){ + // if it has, we ignore it + return; + } + + // we make the line as appended + // ( 1 is more efficient to store than true because true is an oddball ) + _lockFingerprintFromAppending[serverId] = 1; + + // we append to file fs.appendFile(knownHostsPath, serverId + " " + fingerprint + EOL, "utf8", (err) => { if (err) { console.log(err); } }); + + // since the error occurs in the span of one tick + // after one tick we clean up to not interfere with anything else + setImmediate(() => { + delete _lockFingerprintFromAppending[serverId]; + }); } const TrustStrategy = { diff --git a/test/internal/tls.test.js b/test/internal/tls.test.js index 80999928e..5112e2ad8 100644 --- a/test/internal/tls.test.js +++ b/test/internal/tls.test.js @@ -139,6 +139,36 @@ describe('trust-on-first-use', function() { }); }); + it('should not duplicate fingerprint entries', function(done) { + // Assuming we only run this test on NodeJS with TOFU support + if( !hasFeature("trust_on_first_use") ) { + done(); + return; + } + + // Given + var knownHostsPath = "build/known_hosts"; + if( fs.existsSync(knownHostsPath) ) { + fs.unlinkSync(knownHostsPath); + } + fs.writeFileSync(knownHostsPath, ''); + + driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), { + encrypted: true, + trust: "TRUST_ON_FIRST_USE", + knownHosts: knownHostsPath + }); + + // When + driver.session(); + driver.session(); + + setTimeout(function() { + expect( fs.readFileSync(knownHostsPath, 'utf8').split('\n').length ).toBe( 1 ); + done(); + }, 1000); + }); + it('should should give helpful error if database cert does not match stored certificate', function(done) { // Assuming we only run this test on NodeJS with TOFU support if( !hasFeature("trust_on_first_use") ) { From 2e93d80894d4983e53b06c7120bc78a13aecf614 Mon Sep 17 00:00:00 2001 From: Stefan-Gabriel Muscalu Date: Mon, 25 Jul 2016 17:31:35 +0300 Subject: [PATCH 3/4] Fix: tests were not properly implemented --- test/internal/tls.test.js | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/test/internal/tls.test.js b/test/internal/tls.test.js index 5112e2ad8..ffc5a9696 100644 --- a/test/internal/tls.test.js +++ b/test/internal/tls.test.js @@ -90,25 +90,28 @@ describe('trust-on-first-use', function() { fs.unlinkSync(knownHostsPath); } - fs.writeFileSync( - knownHostsPath, - 'localhost:7687 abcd\n' + - 'localhost:7687 abcd' - ); - driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), { encrypted: true, trust: "TRUST_ON_FIRST_USE", knownHosts: knownHostsPath }); + driver.session(); // write into the knownHost file + + // duplicate the same serverId twice + setTimeout(function() { + var text = fs.readFileSync(knownHostsPath, 'utf8'); + fs.writeFileSync(knownHostsPath, text + text); + }, 1000); + // When - driver.session().run("RETURN true AS a").then( function(data) { - // Then we get to here. - // And then the known_hosts file should have correct contents - expect( data.records[0].get('a') ).toBe( true ); - done(); - }); + setTimeout(function() { + driver.session().run("RETURN true AS a").then( function(data) { + // Then we get to here. + expect( data.records[0].get('a') ).toBe( true ); + done(); + }); + }, 2000); }); it('should accept previously un-seen hosts', function(done) { @@ -164,7 +167,7 @@ describe('trust-on-first-use', function() { driver.session(); setTimeout(function() { - expect( fs.readFileSync(knownHostsPath, 'utf8').split('\n').length ).toBe( 1 ); + expect( fs.readFileSync(knownHostsPath, 'utf8').trim().split('\n').length ).toBe( 1 ); done(); }, 1000); }); From 6e38ec12a51bffb57f943572a62753a39529bb90 Mon Sep 17 00:00:00 2001 From: Stefan-Gabriel Muscalu Date: Mon, 25 Jul 2016 18:00:31 +0300 Subject: [PATCH 4/4] Update: `should not duplicate fingerprint entries` test reworked to look for duplicate lines not just the number of them --- test/internal/tls.test.js | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/test/internal/tls.test.js b/test/internal/tls.test.js index ffc5a9696..727e5c75d 100644 --- a/test/internal/tls.test.js +++ b/test/internal/tls.test.js @@ -167,7 +167,30 @@ describe('trust-on-first-use', function() { driver.session(); setTimeout(function() { - expect( fs.readFileSync(knownHostsPath, 'utf8').trim().split('\n').length ).toBe( 1 ); + var lines = {}; + fs.readFileSync(knownHostsPath, 'utf8') + .split('\n') + .filter(function(line) { + return !! (line.trim()); + }) + .forEach(function(line) { + if (!lines[line]) { + lines[line] = 0; + } + lines[line]++; + }); + + var duplicatedLines = Object + .keys(lines) + .map(function(line) { + return lines[line]; + }) + .filter(function(count) { + return count > 1; + }) + .length; + + expect( duplicatedLines ).toBe( 0 ); done(); }, 1000); });