Merge pull request #1352 from ruiquelhas/master

Add proper handshake fatal error handling

Author: sidorares

lib/commands/client_handshake.js

@@ -1,3 +1,8 @@
+// This file was modified by Oracle on June 17, 2021.
+// Handshake errors are now maked as fatal and the corresponding events are
+// emitted in the command instance itself.
+// Modifications copyright (c) 2021, Oracle and/or its affiliates.
+
 'use strict';
 
 const Command = require('./command.js');
@@ -151,20 +156,28 @@ class ClientHandshake extends Command {
         }
         return ClientHandshake.prototype.handshakeResult;
       } catch (err) {
+        // Authentication errors are fatal
+        err.code = 'AUTH_SWITCH_PLUGIN_ERROR';
+        err.fatal = true;
+
         if (this.onResult) {
           this.onResult(err);
         } else {
-          connection.emit('error', err);
+          this.emit('error', err);
         }
         return null;
       }
     }
     if (marker !== 0) {
       const err = new Error('Unexpected packet during handshake phase');
+      // Unknown handshake errors are fatal
+      err.code = 'HANDSHAKE_UNKNOWN_ERROR';
+      err.fatal = true;
+
       if (this.onResult) {
         this.onResult(err);
       } else {
-        connection.emit('error', err);
+        this.emit('error', err);
       }
       return null;
     }

lib/connection.js

@@ -3,6 +3,11 @@
 // the MySQL server when the connection is closed unexpectedly.
 // Modifications copyright (c) 2021, Oracle and/or its affiliates.
 
+// This file was modified by Oracle on June 17, 2021.
+// The changes involve logic to ensure the socket connection is closed when
+// there is a fatal error.
+// Modifications copyright (c) 2021, Oracle and/or its affiliates.
+
 'use strict';
 
 const Net = require('net');
@@ -105,9 +110,10 @@ class Connection extends EventEmitter {
     if (!this.config.isServer) {
       handshakeCommand = new Commands.ClientHandshake(this.config.clientFlags);
       handshakeCommand.on('end', () => {
-        // this happens when handshake finishes early and first packet is error
-        // and not server hello ( for example, 'Too many connactions' error)
-        if (!handshakeCommand.handshake) {
+        // this happens when handshake finishes early either because there was
+        // some fatal error or the server sent an error packet instead of
+        // an hello packet (for example, 'Too many connactions' error)
+        if (!handshakeCommand.handshake || this._fatalError || this._protocolError) {
           return;
         }
         this._handshakePacket = handshakeCommand.handshake;
@@ -229,6 +235,10 @@ class Connection extends EventEmitter {
     if (bubbleErrorToConnection || this._pool) {
       this.emit('error', err);
     }
+    // close connection after emitting the event in case of a fatal error
+    if (err.fatal) {
+      this.close();
+    }
   }
 
   write(buffer) {

test/integration/test-auth-switch-plugin-error.js

@@ -0,0 +1,87 @@
+// Copyright (c) 2021, Oracle and/or its affiliates.
+
+'use strict';
+
+const mysql = require('../../index.js');
+const Command = require('../../lib/commands/command.js');
+const Packets = require('../../lib/packets/index.js');
+
+const assert = require('assert');
+
+class TestAuthSwitchPluginError extends Command {
+  constructor(args) {
+    super();
+    this.args = args;
+  }
+
+  start(_, connection) {
+    const serverHelloPacket = new Packets.Handshake({
+      // "required" properties
+      protocolVersion: 10,
+      serverVersion: 'node.js rocks'
+    });
+    this.serverHello = serverHelloPacket;
+    serverHelloPacket.setScrambleData(() => {
+      connection.writePacket(serverHelloPacket.toPacket(0));
+    });
+    return TestAuthSwitchPluginError.prototype.sendAuthSwitchRequest;
+  }
+
+  sendAuthSwitchRequest(_, connection) {
+    const asr = new Packets.AuthSwitchRequest(this.args);
+    connection.writePacket(asr.toPacket());
+    return TestAuthSwitchPluginError.prototype.finish;
+  }
+
+  finish(_, connection) {
+    connection.end();
+    return TestAuthSwitchPluginError.prototype.finish;
+  }
+}
+
+const server = mysql.createServer(conn => {
+  conn.addCommand(
+    new TestAuthSwitchPluginError({
+      pluginName: 'auth_test_plugin',
+      pluginData: Buffer.allocUnsafe(0)
+    })
+  );
+});
+
+let error;
+let uncaughtExceptions = 0;
+
+const portfinder = require('portfinder');
+portfinder.getPort((_, port) => {
+  server.listen(port);
+  const conn = mysql.createConnection({
+    port: port,
+    authPlugins: {
+      auth_test_plugin: () => {
+        throw new Error('boom');
+      }
+    }
+  });
+
+  conn.on('error', err => {
+    error = err;
+
+    conn.end();
+    server.close();
+  });
+});
+
+process.on('uncaughtException', err => {
+  // The plugin reports a fatal error
+  assert.equal(error.code, 'AUTH_SWITCH_PLUGIN_ERROR');
+  assert.equal(error.message, 'boom');
+  assert.equal(error.fatal, true);
+  // The server must close the connection
+  assert.equal(err.code, 'PROTOCOL_CONNECTION_LOST');
+
+  uncaughtExceptions += 1;
+});
+
+process.on('exit', () => {
+  assert.equal(uncaughtExceptions, 1);
+});

test/integration/test-handshake-unknown-packet-error.js

@@ -0,0 +1,83 @@
+// Copyright (c) 2021, Oracle and/or its affiliates.
+
+'use strict';
+
+const mysql = require('../../index.js');
+const Command = require('../../lib/commands/command.js');
+const Packet = require('../../lib/packets/packet.js');
+const Packets = require('../../lib/packets/index.js');
+
+const assert = require('assert');
+
+class TestUnknownHandshakePacket extends Command {
+  constructor(args) {
+    super();
+    this.args = args;
+  }
+
+  start(_, connection) {
+    const serverHelloPacket = new Packets.Handshake({
+      // "required" properties
+      protocolVersion: 10,
+      serverVersion: 'node.js rocks'
+    });
+    this.serverHello = serverHelloPacket;
+    serverHelloPacket.setScrambleData(() => {
+      connection.writePacket(serverHelloPacket.toPacket(0));
+    });
+    return TestUnknownHandshakePacket.prototype.writeUnexpectedPacket;
+  }
+
+  writeUnexpectedPacket(_, connection) {
+    const length = 6 + this.args.length;
+    const buffer = Buffer.allocUnsafe(length);
+    const up = new Packet(0, buffer, 0, length);
+    up.offset = 4;
+    up.writeInt8(0xfd);
+    up.writeBuffer(this.args);
+    connection.writePacket(up);
+    return TestUnknownHandshakePacket.prototype.finish;
+  }
+
+  finish(_, connection) {
+    connection.end();
+    return TestUnknownHandshakePacket.prototype.finish;
+  }
+}
+
+const server = mysql.createServer(conn => {
+  conn.addCommand(new TestUnknownHandshakePacket(Buffer.alloc(0)));
+});
+
+let error;
+let uncaughtExceptions = 0;
+
+const portfinder = require('portfinder');
+portfinder.getPort((_, port) => {
+  server.listen(port);
+  const conn = mysql.createConnection({
+    port: port
+  });
+
+  conn.on('error', err => {
+    error = err;
+
+    conn.end();
+    server.close();
+  });
+});
+
+process.on('uncaughtException', err => {
+  // The plugin reports a fatal error
+  assert.equal(error.code, 'HANDSHAKE_UNKNOWN_ERROR');
+  assert.equal(error.message, 'Unexpected packet during handshake phase');
+  assert.equal(error.fatal, true);
+  // The server must close the connection
+  assert.equal(err.code, 'PROTOCOL_CONNECTION_LOST');
+
+  uncaughtExceptions += 1;
+});
+
+process.on('exit', () => {
+  assert.equal(uncaughtExceptions, 1);
+});