From 99b0c2e7a7b299e127234334e5bd23cf600902d9 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Sat, 17 Dec 2016 07:05:45 -0800 Subject: [PATCH 1/6] test: move common tls connect setup into fixtures TLS connection setup boilerplate is common to many TLS tests, factor it into a test fixture so tests are clearer to read and faster to write. PR-URL: https://github.com/nodejs/node/pull/10389 Reviewed-By: James M Snell Reviewed-By: Gibson Fahnestock Reviewed-By: Michael Dawson --- test/fixtures/tls-connect.js | 101 ++++++++++++++++++ test/parallel/test-tls-addca.js | 78 ++++++-------- .../test-tls-connect-secure-context.js | 48 ++++----- test/parallel/test-tls-peer-certificate.js | 72 +++++-------- 4 files changed, 181 insertions(+), 118 deletions(-) create mode 100644 test/fixtures/tls-connect.js diff --git a/test/fixtures/tls-connect.js b/test/fixtures/tls-connect.js new file mode 100644 index 00000000000000..72f83736bf370e --- /dev/null +++ b/test/fixtures/tls-connect.js @@ -0,0 +1,101 @@ +// One shot call to connect a TLS client and server based on options to +// tls.createServer() and tls.connect(), so assertions can be made on both ends +// of the connection. +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const join = require('path').join; +const tls = require('tls'); +const util = require('util'); + +module.exports = exports = checkCrypto; + +function checkCrypto() { + if (!common.hasCrypto) { + common.skip('missing crypto'); + process.exit(0); + } + return exports; +} + +exports.assert = require('assert'); +exports.debug = util.debuglog('test'); +exports.tls = tls; + +// Pre-load keys from common fixtures for ease of use by tests. +const keys = exports.keys = { + agent1: load('agent1', 'ca1'), + agent2: load('agent2', 'agent2'), + agent3: load('agent3', 'ca2'), + agent4: load('agent4', 'ca2'), + agent5: load('agent5', 'ca2'), + agent6: load('agent6', 'ca1'), + agent7: load('agent7', 'fake-cnnic-root'), + ec: load('ec', 'ec'), +}; + +function load(cert, issuer) { + issuer = issuer || cert; // Assume self-signed if no issuer + const id = { + key: read(cert + '-key.pem'), + cert: read(cert + '-cert.pem'), + ca: read(issuer + '-cert.pem'), + }; + return id; +} + +function read(file) { + return fs.readFileSync(join(common.fixturesDir, 'keys', file), 'binary'); +} + +exports.connect = function connect(options, callback) { + callback = common.mustCall(callback); + + const server = {}; + const client = {}; + const pair = { + server: server, + client: client, + }; + + tls.createServer(options.server, function(conn) { + server.conn = conn; + conn.pipe(conn); + maybeCallback() + }).listen(0, function() { + server.server = this; + + const optClient = util._extend({ + port: this.address().port, + }, options.client); + + tls.connect(optClient) + .on('secureConnect', function() { + client.conn = this; + maybeCallback(); + }) + .on('error', function(err) { + client.err = err; + client.conn = this; + maybeCallback(); + }); + }); + + function maybeCallback() { + if (!callback) + return; + if (server.conn && (client.conn || client.err)) { + const err = pair.client.err || pair.server.err; + callback(err, pair, cleanup); + callback = null; + + function cleanup() { + if (server.server) + server.server.close(); + if (client.conn) + client.conn.end(); + } + } + } +} diff --git a/test/parallel/test-tls-addca.js b/test/parallel/test-tls-addca.js index 0e9571efdf0abf..7a6f9a77516e22 100644 --- a/test/parallel/test-tls-addca.js +++ b/test/parallel/test-tls-addca.js @@ -1,62 +1,50 @@ 'use strict'; const common = require('../common'); -const fs = require('fs'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} -const tls = require('tls'); - -function filenamePEM(n) { - return require('path').join(common.fixturesDir, 'keys', n + '.pem'); -} +// Adding a CA certificate to contextWithCert should not also add it to +// contextWithoutCert. This is tested by trying to connect to a server that +// depends on that CA using contextWithoutCert. -function loadPEM(n) { - return fs.readFileSync(filenamePEM(n)); -} +const join = require('path').join; +const { + assert, connect, keys, tls +} = require(join(common.fixturesDir, 'tls-connect'))(); -const caCert = loadPEM('ca1-cert'); const contextWithoutCert = tls.createSecureContext({}); const contextWithCert = tls.createSecureContext({}); -// Adding a CA certificate to contextWithCert should not also add it to -// contextWithoutCert. This is tested by trying to connect to a server that -// depends on that CA using contextWithoutCert. -contextWithCert.context.addCACert(caCert); +contextWithCert.context.addCACert(keys.agent1.ca); const serverOptions = { - key: loadPEM('agent1-key'), - cert: loadPEM('agent1-cert'), + key: keys.agent1.key, + cert: keys.agent1.cert, }; -const server = tls.createServer(serverOptions, function() {}); const clientOptions = { - port: undefined, - ca: [caCert], + ca: [keys.agent1.ca], servername: 'agent1', rejectUnauthorized: true, }; -function startTest() { - // This client should fail to connect because it doesn't trust the CA +// This client should fail to connect because it doesn't trust the CA +// certificate. +clientOptions.secureContext = contextWithoutCert; + +connect({ + client: clientOptions, + server: serverOptions, +}, function(err, pair, cleanup) { + assert(err); + assert.strictEqual(err.message, 'unable to verify the first certificate'); + cleanup(); + + // This time it should connect because contextWithCert includes the needed CA // certificate. - clientOptions.secureContext = contextWithoutCert; - clientOptions.port = server.address().port; - const client = tls.connect(clientOptions, common.fail); - client.on('error', common.mustCall(() => { - client.destroy(); - - // This time it should connect because contextWithCert includes the needed - // CA certificate. - clientOptions.secureContext = contextWithCert; - const client2 = tls.connect(clientOptions, common.mustCall(() => { - client2.destroy(); - server.close(); - })); - client2.on('error', (e) => { - console.log(e); - }); - })); -} - -server.listen(0, startTest); + clientOptions.secureContext = contextWithCert; + connect({ + client: clientOptions, + server: serverOptions, + }, function(err, pair, cleanup) { + assert.ifError(err); + cleanup(); + }); +}); diff --git a/test/parallel/test-tls-connect-secure-context.js b/test/parallel/test-tls-connect-secure-context.js index 9e1059f169cf62..d1552a62169207 100644 --- a/test/parallel/test-tls-connect-secure-context.js +++ b/test/parallel/test-tls-connect-secure-context.js @@ -1,37 +1,25 @@ 'use strict'; const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} -const tls = require('tls'); +// Verify connection with explicitly created client SecureContext. -const fs = require('fs'); -const path = require('path'); +const join = require('path').join; +const { + assert, connect, keys, tls +} = require(join(common.fixturesDir, 'tls-connect'))(); -const keysDir = path.join(common.fixturesDir, 'keys'); - -const ca = fs.readFileSync(path.join(keysDir, 'ca1-cert.pem')); -const cert = fs.readFileSync(path.join(keysDir, 'agent1-cert.pem')); -const key = fs.readFileSync(path.join(keysDir, 'agent1-key.pem')); - -const server = tls.createServer({ - cert: cert, - key: key -}, function(c) { - c.end(); -}).listen(0, function() { - const secureContext = tls.createSecureContext({ - ca: ca - }); - - const socket = tls.connect({ - secureContext: secureContext, +connect({ + client: { servername: 'agent1', - port: this.address().port - }, common.mustCall(function() { - server.close(); - socket.end(); - })); + secureContext: tls.createSecureContext({ + ca: keys.agent1.ca, + }), + }, + server: { + cert: keys.agent1.cert, + key: keys.agent1.key, + }, +}, function(err, pair, cleanup) { + assert.ifError(err); + return cleanup(); }); diff --git a/test/parallel/test-tls-peer-certificate.js b/test/parallel/test-tls-peer-certificate.js index ddbbf72a6309e7..eb5be6dcb2241b 100644 --- a/test/parallel/test-tls-peer-certificate.js +++ b/test/parallel/test-tls-peer-certificate.js @@ -1,53 +1,39 @@ 'use strict'; const common = require('../common'); -const assert = require('assert'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} -const tls = require('tls'); +// Verify that detailed getPeerCertificate() return value has all certs. -const fs = require('fs'); -const util = require('util'); const join = require('path').join; +const { + assert, connect, debug, keys +} = require(join(common.fixturesDir, 'tls-connect'))(); -const options = { - key: fs.readFileSync(join(common.fixturesDir, 'keys', 'agent1-key.pem')), - cert: fs.readFileSync(join(common.fixturesDir, 'keys', 'agent1-cert.pem')), - ca: [ fs.readFileSync(join(common.fixturesDir, 'keys', 'ca1-cert.pem')) ] -}; +connect({ + client: {rejectUnauthorized: false}, + server: keys.agent1, +}, function(err, pair, cleanup) { + assert.ifError(err); + const socket = pair.client.conn; + let peerCert = socket.getPeerCertificate(); + assert.ok(!peerCert.issuerCertificate); -const server = tls.createServer(options, function(cleartext) { - cleartext.end('World'); -}); -server.listen(0, common.mustCall(function() { - const socket = tls.connect({ - port: this.address().port, - rejectUnauthorized: false - }, common.mustCall(function() { - let peerCert = socket.getPeerCertificate(); - assert.ok(!peerCert.issuerCertificate); + peerCert = socket.getPeerCertificate(true); + debug('peerCert:\n', peerCert); - // Verify that detailed return value has all certs - peerCert = socket.getPeerCertificate(true); - assert.ok(peerCert.issuerCertificate); + assert.ok(peerCert.issuerCertificate); + assert.strictEqual(peerCert.subject.emailAddress, 'ry@tinyclouds.org'); + assert.strictEqual(peerCert.serialNumber, '9A84ABCFB8A72AC0'); + assert.strictEqual(peerCert.exponent, '0x10001'); + assert.strictEqual( + peerCert.fingerprint, + '8D:06:3A:B3:E5:8B:85:29:72:4F:7D:1B:54:CD:95:19:3C:EF:6F:AA' + ); + assert.deepStrictEqual(peerCert.infoAccess['OCSP - URI'], + [ 'http://ocsp.nodejs.org/' ]); - console.error(util.inspect(peerCert)); - assert.strictEqual(peerCert.subject.emailAddress, 'ry@tinyclouds.org'); - assert.strictEqual(peerCert.serialNumber, '9A84ABCFB8A72AC0'); - assert.strictEqual(peerCert.exponent, '0x10001'); - assert.strictEqual( - peerCert.fingerprint, - '8D:06:3A:B3:E5:8B:85:29:72:4F:7D:1B:54:CD:95:19:3C:EF:6F:AA' - ); - assert.deepStrictEqual(peerCert.infoAccess['OCSP - URI'], - [ 'http://ocsp.nodejs.org/' ]); + const issuer = peerCert.issuerCertificate; + assert.strictEqual(issuer.issuerCertificate, issuer); + assert.strictEqual(issuer.serialNumber, '8DF21C01468AF393'); - const issuer = peerCert.issuerCertificate; - assert.strictEqual(issuer.issuerCertificate, issuer); - assert.strictEqual(issuer.serialNumber, '8DF21C01468AF393'); - server.close(); - })); - socket.end('Hello'); -})); + return cleanup(); +}); From ea72331afc66150a9f5578e5b50f9a6d9024bc22 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Tue, 20 Dec 2016 10:52:32 -0800 Subject: [PATCH 2/6] test: check tls server verification with addCACert SecureContext.addCACert() adds to the existing root store, preserving root cert entries. option.ca is applied without calling SecureContext.addRootCerts() so should add to the default, empty, root store. This test confirms that the built-in root CAs are not included when options.ca is used. Based on: https://github.com/shigeki/node/commit/acd5837fd737351dd953b0387695705067cd69cb PR-URL: https://github.com/nodejs/node/pull/10389 Reviewed-By: James M Snell Reviewed-By: Gibson Fahnestock Reviewed-By: Michael Dawson --- test/internet/test-tls-add-ca-cert.js | 55 +++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 test/internet/test-tls-add-ca-cert.js diff --git a/test/internet/test-tls-add-ca-cert.js b/test/internet/test-tls-add-ca-cert.js new file mode 100644 index 00000000000000..d4e85302856969 --- /dev/null +++ b/test/internet/test-tls-add-ca-cert.js @@ -0,0 +1,55 @@ +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} + +// Test interaction of compiled-in CAs with user-provided CAs. + +const assert = require('assert'); +const fs = require('fs'); +const tls = require('tls'); + +function filenamePEM(n) { + return require('path').join(common.fixturesDir, 'keys', n + '.pem'); +} + +function loadPEM(n) { + return fs.readFileSync(filenamePEM(n)); +} + +const caCert = loadPEM('ca1-cert'); + +var opts = { + host: 'www.nodejs.org', + port: 443, + rejectUnauthorized: true +}; + +// Success relies on the compiled in well-known root CAs +tls.connect(opts, common.mustCall(end)); + +// The .ca option replaces the well-known roots, so connection fails. +opts.ca = caCert; +tls.connect(opts, fail).on('error', common.mustCall((err) => { + assert.strictEqual(err.message, 'unable to get local issuer certificate'); +})); + +function fail() { + assert(false, 'should fail to connect'); +} + +// New secure contexts have the well-known root CAs. +opts.secureContext = tls.createSecureContext(); +tls.connect(opts, common.mustCall(end)); + +// Explicit calls to addCACert() add to the default well-known roots, instead +// of replacing, so connection still succeeds. +opts.secureContext.context.addCACert(caCert); +tls.connect(opts, common.mustCall(end)); + +function end() { + this.end(); +} From f9665280a495d80b7ace32d59ab29b1983f96731 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Tue, 20 Dec 2016 14:16:18 -0800 Subject: [PATCH 3/6] doc: use correct tls certificate property name Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. PR-URL: https://github.com/nodejs/node/pull/10389 Reviewed-By: James M Snell Reviewed-By: Gibson Fahnestock Reviewed-By: Michael Dawson --- doc/api/tls.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/doc/api/tls.md b/doc/api/tls.md index 377bfd3dd7bb0b..64fd343c0449d5 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -583,13 +583,16 @@ For Example: `{ type: 'ECDH', name: 'prime256v1', size: 256 }` added: v0.11.4 --> -* `detailed` {boolean} Specify `true` to request that the full certificate - chain with the `issuer` property be returned; `false` to return only the - top certificate without the `issuer` property. +* `detailed` {boolean} Include the full certificate chain if `true`, otherwise + include just the peer's certificate. Returns an object representing the peer's certificate. The returned object has some properties corresponding to the fields of the certificate. +If the full certificate chain was requested, each certificate will include a +`issuerCertificate` property containing an object representing its issuer's +certificate. + For example: ```text @@ -600,15 +603,15 @@ For example: O: 'node.js', OU: 'Test TLS Certificate', CN: 'localhost' }, - issuerInfo: + issuer: { C: 'UK', ST: 'Acknack Ltd', L: 'Rhys Jones', O: 'node.js', OU: 'Test TLS Certificate', CN: 'localhost' }, - issuer: - { ... another certificate ... }, + issuerCertificate: + { ... another certificate, possibly with a .issuerCertificate ... }, raw: < RAW DER buffer >, valid_from: 'Nov 11 09:52:22 2009 GMT', valid_to: 'Nov 6 09:52:22 2029 GMT', @@ -616,8 +619,7 @@ For example: serialNumber: 'B9B0D332A1AA5635' } ``` -If the peer does not provide a certificate, `null` or an empty object will be -returned. +If the peer does not provide a certificate, an empty object will be returned. ### tlsSocket.getProtocol()