Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support authentication switch request and basic auth plugin. #1776

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Readme.md
Expand Up @@ -1324,6 +1324,7 @@ The following flags are sent by default on a new connection:
- `RESERVED` - Old flag for the 4.1 protocol.
- `SECURE_CONNECTION` - Support native 4.1 authentication.
- `TRANSACTIONS` - Asks for the transaction status flags.
- `PLUGIN_AUTH` - Support basic auth plugin protocol, including plugin `mysql_native_password` and `mysql_old_password`.

In addition, the following flag will be sent if the option `multipleStatements`
is set to `true`:
Expand All @@ -1339,7 +1340,6 @@ available to specify.
- COMPRESS
- INTERACTIVE
- NO_SCHEMA
- PLUGIN_AUTH
- REMEMBER_OPTIONS
- SSL
- SSL_VERIFY_SERVER_CERT
Expand Down
2 changes: 1 addition & 1 deletion lib/ConnectionConfig.js
Expand Up @@ -106,7 +106,7 @@ ConnectionConfig.getDefaultFlags = function getDefaultFlags(options) {
'+LONG_PASSWORD', // Use the improved version of Old Password Authentication
'+MULTI_RESULTS', // Can handle multiple resultsets for COM_QUERY
'+ODBC', // Special handling of ODBC behaviour
'-PLUGIN_AUTH', // Does *NOT* support auth plugins
'+PLUGIN_AUTH', // Support basic auth plugins, including mysql_native_method and mysql_old_password
'+PROTOCOL_41', // Uses the 4.1 protocol
'+PS_MULTI_RESULTS', // Can handle multiple resultsets for COM_STMT_EXECUTE
'+RESERVED', // Unused
Expand Down
2 changes: 2 additions & 0 deletions lib/Pool.js
Expand Up @@ -112,7 +112,9 @@ Pool.prototype.acquireConnection = function acquireConnection(connection, cb) {

if (changeUser) {
// restore user back to pool configuration
var previousClientPluginAuth = connection.config.clientPluginAuth;
connection.config = this.config.newConnectionConfig();
connection.config.clientPluginAuth = previousClientPluginAuth;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to rework how this is tracked so it stops changing the hidden class of the config object.

connection.changeUser({timeout: this.config.acquireTimeout}, onOperationComplete);
} else {
// ping connection
Expand Down
16 changes: 16 additions & 0 deletions lib/protocol/Auth.js
Expand Up @@ -150,3 +150,19 @@ Auth.int32Read = function(buffer, offset){
+ (buffer[offset + 2] << 8)
+ (buffer[offset + 3]);
};

Auth.tokenByPlugin = function(pluginName, pluginData, password) {
switch (pluginName) {
case 'mysql_native_password':
// mysql_native_password only need first 20 bytes scramble.
return this.token(password, pluginData.slice(0, 20));
case 'mysql_old_password':
// mysql_old_password only need first 8 bytes scramble.
return this.scramble323(pluginData.slice(0, 8), password);
default:
var err = new Error('The auth plugin: ' + pluginName + ' is not supported by this client driver.');
err.code = 'UNSUPPORTED_AUTH_PLUGIN';
err.fatal = true;
throw err;
}
};
5 changes: 5 additions & 0 deletions lib/protocol/Parser.js
Expand Up @@ -309,6 +309,11 @@ Parser.prototype._nullByteOffset = function() {
return offset;
};

Parser.prototype.parsePacketTerminatedBuffer = function() {
var length = this._packetEnd - this._offset;
return this.parseBuffer(length);
};

Parser.prototype.parsePacketTerminatedString = function() {
var length = this._packetEnd - this._offset;
return this.parseString(length);
Expand Down
13 changes: 13 additions & 0 deletions lib/protocol/packets/AuthSwitchPacket.js
@@ -0,0 +1,13 @@
module.exports = AuthSwitchPacket;
function AuthSwitchPacket(options) {
options = options || {};
this.scrambleBuff = options.scrambleBuff;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is greenfield, I'm just changing these props to have the same name as in the protocol docs for sanity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

}

AuthSwitchPacket.prototype.parse = function(parser) {
this.scrambleBuff = parser.parsePacketTerminatedBuffer();
};

AuthSwitchPacket.prototype.write = function(writer) {
writer.writeBuffer(this.scrambleBuff);
};
24 changes: 16 additions & 8 deletions lib/protocol/packets/ClientAuthenticationPacket.js
Expand Up @@ -4,14 +4,16 @@ module.exports = ClientAuthenticationPacket;
function ClientAuthenticationPacket(options) {
options = options || {};

this.clientFlags = options.clientFlags;
this.maxPacketSize = options.maxPacketSize;
this.charsetNumber = options.charsetNumber;
this.filler = undefined;
this.user = options.user;
this.scrambleBuff = options.scrambleBuff;
this.database = options.database;
this.protocol41 = options.protocol41;
this.clientFlags = options.clientFlags;
this.maxPacketSize = options.maxPacketSize;
this.charsetNumber = options.charsetNumber;
this.filler = undefined;
this.user = options.user;
this.scrambleBuff = options.scrambleBuff;
this.database = options.database;
this.protocol41 = options.protocol41;
this.clientPluginAuth = options.clientPluginAuth;
this.authPluginName = options.authPluginName;
}

ClientAuthenticationPacket.prototype.parse = function(parser) {
Expand All @@ -23,6 +25,9 @@ ClientAuthenticationPacket.prototype.parse = function(parser) {
this.user = parser.parseNullTerminatedString();
this.scrambleBuff = parser.parseLengthCodedBuffer();
this.database = parser.parseNullTerminatedString();
if (this.clientPluginAuth) {
this.authPluginName = parser.parseNullTerminatedString();
}
} else {
this.clientFlags = parser.parseUnsignedNumber(2);
this.maxPacketSize = parser.parseUnsignedNumber(3);
Expand All @@ -41,6 +46,9 @@ ClientAuthenticationPacket.prototype.write = function(writer) {
writer.writeNullTerminatedString(this.user);
writer.writeLengthCodedBuffer(this.scrambleBuff);
writer.writeNullTerminatedString(this.database);
if (this.clientPluginAuth) {
writer.writeNullTerminatedString(this.authPluginName);
}
} else {
writer.writeUnsignedNumber(2, this.clientFlags);
writer.writeUnsignedNumber(3, this.maxPacketSize);
Expand Down
22 changes: 16 additions & 6 deletions lib/protocol/packets/ComChangeUserPacket.js
Expand Up @@ -2,19 +2,26 @@ module.exports = ComChangeUserPacket;
function ComChangeUserPacket(options) {
options = options || {};

this.command = 0x11;
this.user = options.user;
this.scrambleBuff = options.scrambleBuff;
this.database = options.database;
this.charsetNumber = options.charsetNumber;
this.command = 0x11;
this.user = options.user;
this.scrambleBuff = options.scrambleBuff;
this.database = options.database;
this.charsetNumber = options.charsetNumber;
this.clientPluginAuth = options.clientPluginAuth;
this.authPluginName = options.authPluginName;
}

ComChangeUserPacket.prototype.parse = function(parser) {
this.command = parser.parseUnsignedNumber(1);
this.user = parser.parseNullTerminatedString();
this.scrambleBuff = parser.parseLengthCodedBuffer();
this.database = parser.parseNullTerminatedString();
this.charsetNumber = parser.parseUnsignedNumber(1);
if (!parser.reachedPacketEnd()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this key off the fact that plugin auth is used, not if packet end was reached? For example, this will not work with CLIENT_CONNECT_ATTRS because if client auth is off but CLIENT_CONNECT_ATTRS is on, then the packet end is indeed not reached, but the rest of the packet is not related to client plugin auth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let me change the parse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this check here? Shouldn't the charsetNumber just be outside the if like it used to be, or is there a reason behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dougwilson , this is for the document COM_CHANGE_USER packet, it include the charset in the if more data condition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry :) I was thinking the handshake. Nevermind on this, then :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm writing a test to exercise this case, there the charsetNumber may not get populated.

this.charsetNumber = parser.parseUnsignedNumber(2);
if (this.clientPluginAuth === true) {
this.authPluginName = parser.parseNullTerminatedString();
}
}
};

ComChangeUserPacket.prototype.write = function(writer) {
Expand All @@ -23,4 +30,7 @@ ComChangeUserPacket.prototype.write = function(writer) {
writer.writeLengthCodedBuffer(this.scrambleBuff);
writer.writeNullTerminatedString(this.database);
writer.writeUnsignedNumber(2, this.charsetNumber);
if (this.clientPluginAuth) {
writer.writeNullTerminatedString(this.authPluginName);
}
};
19 changes: 17 additions & 2 deletions lib/protocol/packets/HandshakeInitializationPacket.js
Expand Up @@ -20,11 +20,17 @@ function HandshakeInitializationPacket(options) {
this.filler3 = options.filler3;
this.pluginData = options.pluginData;
this.protocol41 = options.protocol41;
this.clientPluginAuth = options.clientPluginAuth;
this.authPluginName = options.authPluginName;

if (this.protocol41) {
// force set the bit in serverCapabilities1
this.serverCapabilities1 |= Client.CLIENT_PROTOCOL_41;
}

if (this.clientPluginAuth) {
this.serverCapabilities2 |= Client.CLIENT_PLUGIN_AUTH >> 16;
}
}

HandshakeInitializationPacket.prototype.parse = function(parser) {
Expand All @@ -38,16 +44,22 @@ HandshakeInitializationPacket.prototype.parse = function(parser) {
this.serverStatus = parser.parseUnsignedNumber(2);

this.protocol41 = (this.serverCapabilities1 & (1 << 9)) > 0;
this.clientPluginAuth = false;

if (this.protocol41) {
this.serverCapabilities2 = parser.parseUnsignedNumber(2);
this.clientPluginAuth = (this.serverCapabilities2 & (1 << 3)) > 0;
this.scrambleLength = parser.parseUnsignedNumber(1);
this.filler2 = parser.parseFiller(10);
// scrambleBuff2 should be 0x00 terminated, but sphinx does not do this
// so we assume scrambleBuff2 to be 12 byte and treat the next byte as a
// filler byte.
this.scrambleBuff2 = parser.parseBuffer(12);
scrambleBuff2Length = Math.max(12, this.scrambleLength - 9);
this.scrambleBuff2 = parser.parseBuffer(scrambleBuff2Length);
this.filler3 = parser.parseFiller(1);
if (this.clientPluginAuth) {
this.authPluginName = parser.parseNullTerminatedString();
}
} else {
this.filler2 = parser.parseFiller(13);
}
Expand Down Expand Up @@ -80,8 +92,11 @@ HandshakeInitializationPacket.prototype.write = function(writer) {
writer.writeUnsignedNumber(2, this.serverCapabilities2);
writer.writeUnsignedNumber(1, this.scrambleLength);
writer.writeFiller(10);
writer.writeNullTerminatedBuffer(this.scrambleBuff2);
if (this.clientPluginAuth) {
writer.writeNullTerminatedString(this.authPluginName);
}
}
writer.writeNullTerminatedBuffer(this.scrambleBuff2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a miss in the first, and since it's fixed in here, I'm trying to write a test case for this bug.


if (this.pluginData !== undefined) {
writer.writeNullTerminatedString(this.pluginData);
Expand Down
19 changes: 19 additions & 0 deletions lib/protocol/packets/UseAuthSwitchPacket.js
@@ -0,0 +1,19 @@
module.exports = UseAuthSwitchPacket;
function UseAuthSwitchPacket(options) {
options = options || {};
this.firstByte = options.firstByte || 0xfe;
this.authPluginName = options.authPluginName || '';
this.authPluginData = options.authPluginData || '';
}

UseAuthSwitchPacket.prototype.parse = function(parser) {
this.firstByte = parser.parseUnsignedNumber(1);
this.authPluginName = parser.parseNullTerminatedString();
this.authPluginData = parser.parsePacketTerminatedBuffer();
};

UseAuthSwitchPacket.prototype.write = function(writer) {
writer.writeUnsignedNumber(1, this.firstByte);
writer.writeNullTerminatedString(this.authPluginName);
writer.writeBuffer(this.authPluginData);
};
2 changes: 2 additions & 0 deletions lib/protocol/packets/index.js
@@ -1,3 +1,4 @@
exports.AuthSwitchPacket = require('./AuthSwitchPacket');
exports.ClientAuthenticationPacket = require('./ClientAuthenticationPacket');
exports.ComChangeUserPacket = require('./ComChangeUserPacket');
exports.ComPingPacket = require('./ComPingPacket');
Expand All @@ -17,4 +18,5 @@ exports.ResultSetHeaderPacket = require('./ResultSetHeaderPacket');
exports.RowDataPacket = require('./RowDataPacket');
exports.SSLRequestPacket = require('./SSLRequestPacket');
exports.StatisticsPacket = require('./StatisticsPacket');
exports.UseAuthSwitchPacket = require('./UseAuthSwitchPacket');
exports.UseOldPasswordPacket = require('./UseOldPasswordPacket');
44 changes: 35 additions & 9 deletions lib/protocol/sequences/ChangeUser.js
Expand Up @@ -8,22 +8,25 @@ Util.inherits(ChangeUser, Sequence);
function ChangeUser(options, callback) {
Sequence.call(this, options, callback);

this._user = options.user;
this._password = options.password;
this._database = options.database;
this._charsetNumber = options.charsetNumber;
this._currentConfig = options.currentConfig;
this._user = options.user;
this._password = options.password;
this._database = options.database;
this._charsetNumber = options.charsetNumber;
this._currentConfig = options.currentConfig;
this._authPluginName = options.authPluginName;
}

ChangeUser.prototype.start = function(handshakeInitializationPacket) {
var scrambleBuff = handshakeInitializationPacket.scrambleBuff();
scrambleBuff = Auth.token(this._password, scrambleBuff);

var packet = new Packets.ComChangeUserPacket({
user : this._user,
scrambleBuff : scrambleBuff,
database : this._database,
charsetNumber : this._charsetNumber
user : this._user,
scrambleBuff : scrambleBuff,
database : this._database,
charsetNumber : this._charsetNumber,
clientPluginAuth : this._currentConfig.clientPluginAuth,
authPluginName : this._authPluginName || this._currentConfig.authPluginName
});

this._currentConfig.user = this._user;
Expand All @@ -34,8 +37,31 @@ ChangeUser.prototype.start = function(handshakeInitializationPacket) {
this.emit('packet', packet);
};

ChangeUser.prototype.determinePacket = function(firstByte) {
if (firstByte === 0xff) {
return Packets.ErrorPacket;
}

if (firstByte === 0xfe) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fixing up this edge case where the server can actually respond here with the old password packet, of 0xfe doesn't always mean auth switch as implemented here, so just gotta add the same logic from the handshake.

return Packets.UseAuthSwitchPacket;
}

return Packets.OkPacket;
};

ChangeUser.prototype['ErrorPacket'] = function(packet) {
var err = this._packetToError(packet);
err.fatal = true;
this.end(err);
};

ChangeUser.prototype['UseAuthSwitchPacket'] = function(packet) {
try {
var scrambleBuff = Auth.tokenByPlugin(packet.authPluginName, packet.authPluginData, this._password);
this.emit('packet', new Packets.AuthSwitchPacket({
scrambleBuff: scrambleBuff
}));
} catch (err) {
this.end(err);
}
};