add handshake fatal error handling

Errors that occur during the handshake stage should be fatal and should
cause the connection to be closed.

Currently, although unexpected errors such as ones created by
unexpected packets sent by the server, are being handled, they are not
considered fatal and do not result in closing the connection. On the other
hand, errors reported by client-side authentication plugins are being
ignored and are not reported to the application.

Since errors reported by server-side authentication plugins are
considered fatal and cause the connection to be closed, the fact that
the same does not happen for errors generated on the client-side leads
to some inconsistencies and forces custom authentication plugin
implementations to forcibly close the connection on their own.

This patch ensures that both this kind of errors are considered fatal and
cause the connection to be closed.

Author: ruiquelhas

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);
+});