From b17524443be88b1dbd28440dbab7283b312d5940 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Klinda?= Date: Sun, 21 Mar 2021 02:52:04 +0100 Subject: [PATCH] fix mTLS connection support, add tls tests (#147) --- CHANGELOG.md | 7 +- lib/winston-syslog.js | 30 +++--- package-lock.json | 27 ++++-- package.json | 3 +- test/tls-test.js | 213 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 256 insertions(+), 24 deletions(-) create mode 100644 test/tls-test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a26d0e1..344d451 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,14 @@ # CHANGELOG +## Latest + +- #[147], (@zenonhun) fix TLS connection support + ## v2.4.0 / 2020-01-01 - (@DABH) Node v12 support, fix node-unix-dgram issues, update dependencies - #[115], (@pepakriz) TLS connection support -- #[123], (@JeffTomlinson, @elliotttf) Handle oversize messages sent over UDP transports +- #[123], (@JeffTomlinson, @elliotttf) Handle oversize messages sent over UDP transports - #[116], (@pepakriz) Make socket options configurable - #[122], (@cjbarth) Correct improper opening and closing of sockets @@ -20,4 +24,3 @@ - #[108], (@vrza) Make winston 3 a peer dependency - #[102], (@stieg) Require winston >= 3 and add corresopnding note in readme - #[105], (@mohd-akram) Update dependencies for latest Node compatibility - diff --git a/lib/winston-syslog.js b/lib/winston-syslog.js index 5b1e9f0..6e4df64 100644 --- a/lib/winston-syslog.js +++ b/lib/winston-syslog.js @@ -279,7 +279,6 @@ class Syslog extends Transport { this.socket.close(); } } - this.emit('closed', this.socket); } else { attempt++; @@ -339,14 +338,6 @@ class Syslog extends Transport { return this.connectDgram(callback); } - this.socket = /^tls[4|6]?$/.test(this.protocol) - ? new secNet.TLSSocket() - : new net.Socket(); - this.socket.setKeepAlive(true); - this.socket.setNoDelay(); - - this.setupEvents(); - const connectConfig = Object.assign({}, this.protocolOptions, { host: this.host, port: this.port @@ -356,7 +347,13 @@ class Syslog extends Transport { connectConfig.family = this.protocolFamily; } - this.socket.connect(connectConfig); + this.socket = /^tls[4|6]?$/.test(this.protocol) + ? secNet.connect(connectConfig) + : net.connect(connectConfig); + this.socket.setKeepAlive(true); + this.socket.setNoDelay(); + + this.setupEvents(); // // Indicate to the callee that the socket is not ready. This @@ -396,7 +393,8 @@ class Syslog extends Transport { this.retries = 0; this.connected = true; }) - .on('error', function () { + .on('error', (error) => { + this.emit('error', error); // // TODO: Pass this error back up // @@ -409,10 +407,12 @@ class Syslog extends Transport { const interval = Math.pow(2, this.retries); this.connected = false; - setTimeout(() => { - this.retries++; - this.socket.connect(this.port, this.host); - }, interval * 1000); + if (!this.socket.destroyed) { + setTimeout(() => { + this.retries++; + this.socket.connect(this.port, this.host); + }, interval * 1000); + } }) .on('timeout', () => { if (this.socket.destroy) { diff --git a/package-lock.json b/package-lock.json index 50b8a72..44ca356 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1137,9 +1137,9 @@ "dev": true }, "nan": { - "version": "2.14.0", - "resolved": "https://registry.npmjs.org/nan/-/nan-2.14.0.tgz", - "integrity": "sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg==", + "version": "2.14.2", + "resolved": "https://registry.npmjs.org/nan/-/nan-2.14.2.tgz", + "integrity": "sha512-M2ufzIiINKCuDfBSAUr1vWQ+vuVcA9kqx8JJUsbQi6yf1uGRyb7HfpdfUr5qLXf3B/t8dPvcjhKMmlfnP47EzQ==", "optional": true }, "natural-compare": { @@ -1162,6 +1162,12 @@ "path-to-regexp": "^1.7.0" } }, + "node-forge": { + "version": "0.10.0", + "resolved": "https://registry.npmjs.org/node-forge/-/node-forge-0.10.0.tgz", + "integrity": "sha512-PPmu8eEeG9saEUvI97fm4OYxXVB6bFvyNTyiUOBichBpFG8A1Ljw3bY62+5oOjDEMHRnd0Y7HQ+x7uzxOzC6JA==", + "dev": true + }, "normalize-package-data": { "version": "2.5.0", "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.5.0.tgz", @@ -1521,6 +1527,15 @@ "integrity": "sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg==", "dev": true }, + "selfsigned": { + "version": "1.10.8", + "resolved": "https://registry.npmjs.org/selfsigned/-/selfsigned-1.10.8.tgz", + "integrity": "sha512-2P4PtieJeEwVgTU9QEcwIRDQ/mXJLX8/+I3ur+Pg16nS8oNbrGxEso9NyYWy8NAmXiNl4dlAp5MwoNeCWzON4w==", + "dev": true, + "requires": { + "node-forge": "^0.10.0" + } + }, "semver": { "version": "5.7.1", "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz", @@ -1785,9 +1800,9 @@ "dev": true }, "unix-dgram": { - "version": "2.0.3", - "resolved": "https://registry.npmjs.org/unix-dgram/-/unix-dgram-2.0.3.tgz", - "integrity": "sha512-Bay5CkSLcdypcBCsxvHEvaG3mftzT5FlUnRToPWEAVxwYI8NI/8zSJ/Gknlp86MPhV6hBA8I8TBsETj2tssoHQ==", + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/unix-dgram/-/unix-dgram-2.0.4.tgz", + "integrity": "sha512-7tpK6x7ls7J7pDrrAU63h93R0dVhRbPwiRRCawR10cl+2e1VOvF3bHlVJc6WI1dl/8qk5He673QU+Ogv7bPNaw==", "optional": true, "requires": { "bindings": "^1.3.0", diff --git a/package.json b/package.json index 122347c..1614fc3 100644 --- a/package.json +++ b/package.json @@ -32,13 +32,14 @@ "glossy": "^0.1.7" }, "optionalDependencies": { - "unix-dgram": "2.0.3" + "unix-dgram": "^2.0.4" }, "peerDependencies": { "winston": "^3.0.0" }, "devDependencies": { "eslint-config-populist": "^4.2.0", + "selfsigned": "^1.10.8", "sinon": "^8.0.2", "vows": "^0.8.3", "winston": "^3.0.0" diff --git a/test/tls-test.js b/test/tls-test.js new file mode 100644 index 0000000..f8c109d --- /dev/null +++ b/test/tls-test.js @@ -0,0 +1,213 @@ +const vows = require('vows'); +const assert = require('assert'); +const selfsigned = require('selfsigned'); +const tls = require('tls'); +const winston = require('winston'); +require('../lib/winston-syslog').Syslog; + +// ----- Constants +const HOST = 'localhost'; +const PORT = 10514; +const PROMISE_TIMEOUT = 1000; + +// ----- Helpers +function wrapToPromise(target, event) { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject('Timeout for event promise'); + }, PROMISE_TIMEOUT); + target.on(event, (...args) => { + clearTimeout(timeout); + resolve(...args); + }); + }); +} + +function nodeMajorVersion() { + return Number.parseInt(process.version.match(/^v(\d+\.\d+)/)[1], 10); +} + +// ----- Certificate handling +function generateCertificate() { + // Generate server and client certificates + const attributes = [{ name: 'commonName', value: 'localhost' }]; + const x509 = selfsigned.generate(attributes, { + days: 1, + clientCertificate: true, + extensions: [ + { + name: 'keyUsage', + keyCertSign: true, + digitalSignature: true, + nonRepudiation: true, + keyEncipherment: true, + dataEncipherment: true + }, + { + name: 'subjectAltName', + altNames: [ + { + type: 2, // DNS + value: 'localhost' + } + ] + } + ] + }); + return x509; +} + +// ----- TLS configs +const x509 = generateCertificate(); +const validServerTLS = { + key: x509.private, + cert: x509.cert, + ca: [x509.cert], + rejectUnauthorized: true, + requestCert: true +}; + +const validClientTLS = { + key: x509.clientprivate, + cert: x509.clientcert, + ca: [x509.cert], + rejectUnauthorized: true +}; + +const untrustedClientTLS = { + ...validClientTLS, + ca: null +}; + +const missingClientTLS = { + ca: null, + key: null, + cert: null, + rejectUnauthorized: false +}; + +// ----- TLS Server +const serverEvents = { + data: 'data', + tlsClientError: 'tlsClientError', + error: 'error', + listening: 'listening', + secureConnection: 'secureConnection' +}; + +async function startServer({ host = HOST, port = PORT, tlsOpts } = {}) { + const server = tls.createServer({ ...tlsOpts }); + let clients = []; + server.on('secureConnection', (client) => { + clients.push(client); + client.on('close', () => { + clients = clients.filter((c) => c !== client); + }); + client.on('data', (data) => { + server.emit(serverEvents.data, data.toString()); + }); + }); + server.forceClose = () => { + clients.forEach((client) => client.destroy()); + server.close(); + }; + server.listen({ host, port }); + await wrapToPromise(server, serverEvents.listening); + return server; +} + +// ----- Init Logger +function initLogger({ host = HOST, port = PORT, tlsOpts } = {}) { + const syslogOptions = { + host, + port, + protocol: 'tls4', + protocolOptions: { ...tlsOpts } + }; + const logger = winston.createLogger({ + transports: [new winston.transports.Syslog(syslogOptions)] + }); + return logger; +} + +// ----- Test Cases +const TEST_MESSAGE = 'Test Message'; +const SYSLOG_FORMAT = `"message":"${TEST_MESSAGE}"`; + +let serverInstance; + +vows + .describe('tls-connect') + .addBatch({ + 'Trying to connect to a TLS server with mutual TLS': { + 'topic': function () { + startServer({ tlsOpts: validServerTLS }).then((server) => { + serverInstance = server; + const promise = wrapToPromise(server, serverEvents.data); + initLogger({ tlsOpts: validClientTLS }).info(TEST_MESSAGE); + promise.then(msg => this.callback(null, msg)); + }); + }, + 'TLS server should receive log message': function (msg) { + assert.include(msg, SYSLOG_FORMAT); + }, + 'teardown': function () { + serverInstance.forceClose(); + } + } + }) + .addBatch({ + 'Trying to connect to a TLS server with untrusted certificate': { + 'topic': function () { + startServer({ tlsOpts: validServerTLS }).then((server) => { + serverInstance = server; + const logger = initLogger({ tlsOpts: untrustedClientTLS }); + logger.on('error', (loggerError) => { + this.callback(null, loggerError); + }); + logger.info(TEST_MESSAGE); + }); + }, + 'Client should refuse connection': function (e, loggerError) { + assert.strictEqual(loggerError.code, 'DEPTH_ZERO_SELF_SIGNED_CERT'); + assert.include(loggerError.message, 'self signed certificate'); + }, + 'teardown': function () { + serverInstance.forceClose(); + } + } + }) + .addBatch({ + 'Trying to connect to a TLS server without client certificate': { + 'topic': function () { + startServer({ tlsOpts: validServerTLS }).then((server) => { + serverInstance = server; + const promise = wrapToPromise(server, serverEvents.tlsClientError); + const logger = initLogger({ tlsOpts: missingClientTLS }); + logger.on('error', (loggerError) => { + promise + .then((serverError) => this.callback(null, loggerError, serverError)) + .catch((error) => this.callback(error, loggerError, null)); + }); + logger.info(TEST_MESSAGE); + }); + }, + 'Server should refuse connection': function (e, loggerError, serverError) { + // Client and Server error type changes between Node versions + if (nodeMajorVersion() >= 12) { + assert.strictEqual(loggerError.code, 'ERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED'); + assert.include(loggerError.message, 'alert number 116'); + assert.strictEqual(serverError.code, 'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE'); + assert.include(serverError.message, 'peer did not return a certificate'); + } else { + assert.strictEqual(loggerError.code, 'EPROTO'); + assert.include(loggerError.message, 'alert number 40'); + assert.include(serverError.message, 'peer did not return a certificate'); + } + }, + 'teardown': function () { + serverInstance.forceClose(); + } + } + }) + .export(module);