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

SSL Question #481

Closed
crh3675 opened this issue May 10, 2013 · 47 comments
Closed

SSL Question #481

crh3675 opened this issue May 10, 2013 · 47 comments
Milestone

Comments

@crh3675
Copy link

crh3675 commented May 10, 2013

I am trying to integrate this library on an AWS EC2 instance but I need to have the connection secured using SSL to RDS.

A typical command line solution for that is:

mysql -u [username] -h [hostname] dbnamer -p --ssl --ssl-ca=[path_to_cert]

I know it is not fully supported but would you be able to identify in your code base where I could make some tweaks for passing the "ssl" flag along with the "ssl-ca" path?

Or, will this driver pickup the native settings from /etc/my.cnf?

Edit:

I have been digging through the MySQL source code and found this

#if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
#define SET_SSL_OPTION(opt_var,arg) \
    if (mysql->options.opt_var) \
      my_free(mysql->options.opt_var); \
    mysql->options.opt_var= arg ? my_strdup(arg, MYF(MY_WME)) : NULL; \
    if (mysql->options.opt_var) \
      mysql->options.use_ssl= 1
#define EXTENSION_SET_SSL_STRING(OPTS, X, STR) \
    EXTENSION_SET_STRING(OPTS, X, STR); \
    if ((OPTS)->extension->X) \
      (OPTS)->use_ssl= 1

Where those options get set but I don't know how that would get applied to connections in a Node Architecture

Thanks

@felixge
Copy link
Collaborator

felixge commented May 10, 2013

This driver has it's own implementation of the mysql protocol, it does not bind to any C library.

This means that there is no "switch" to enable SSL. You would have to implement SSL support by using node's tls module. It's probably not too hard, but hasn't been done yet.

@crh3675
Copy link
Author

crh3675 commented May 10, 2013

I have been perusing the code and came up with a "stubbed" solution in lib/Connection.js

Connection.prototype.connect = function(cb) {
  if (!this._connectCalled) {
    this._connectCalled = true;

    var self = this;

    if(Object.getOwnPropertyNames(self.config.ssl).length > 0)
    {
      self._socket = TLS.connect(self.config.port, self.config.host, self.config.ssl, function(){

        console.log(self._socket.authorizationError);

        self._socket.on('secureConnect', self._handleProtocolConnect.bind(self));

        if(self.authorized) continueConnect.call(self);
      });
    }
    else
    {
      self._socket = (self.config.socketPath)
        ? Net.createConnection(self.config.socketPath)
        : Net.createConnection(self.config.port, self.config.host);

      self._socket.on('connect', self._handleProtocolConnect.bind(self));
    }

    var continueConnect = function(){

      // Node v0.10+ Switch socket into "old mode" (Streams2)
      self._socket.on("data",function() {});

      self._socket.pipe(self._protocol);
      self._protocol.pipe(self._socket);

      self._socket.on('error', self._handleNetworkError.bind(self));
      self._protocol.on('handshake', self._handleProtocolHandshake.bind(self));
      self._protocol.on('unhandledError', self._handleProtocolError.bind(self));
      self._protocol.on('drain', self._handleProtocolDrain.bind(self));
      self._protocol.on('end', self._handleProtocolEnd.bind(self));

    }
  }

  self._protocol.handshake(cb);
};

Note that this does not work but I wanted to make sure I was looking the correct location for such functionality to be added

@felixge
Copy link
Collaborator

felixge commented May 10, 2013

@crh3675 yeah, I think you're looking at the right part of the code. You probably also want to check out: http://dev.mysql.com/doc/internals/en/ssl.html

@crh3675
Copy link
Author

crh3675 commented May 10, 2013

Cool, next I have looked at /lib/protocol/Protocol and updated this:

Protocol.prototype._parsePacket = function() {

  var sequence = this._queue[0];
  var Packet   = this._determinePacket(sequence, this._config.ssl);

// ...

And added to protocol/sequences/handshake:

Handshake.prototype['SSLHandshakeInitializationPacket'] = function(packet) {
  this._handshakeInitializationPacket = packet;

  this._config.clientFlags.CLIENT_SSL;

  this.emit('packet', new Packets.ClientAuthenticationPacket({
    clientFlags   : this._config.clientFlags,
    maxPacketSize : this._config.maxPacketSize,
    charsetNumber : this._config.charsetNumber
  }));
};

As well as updated:

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

  if (!this._handshakeInitializationPacket) {
    return ssl ? Packets.SSLHandshakeInitializationPacket : Packets.HandshakeInitializationPacket;
  }

  if (firstByte === 0xfe) {
    return Packets.UseOldPasswordPacket;
  }
};

I get this error:

Error: 140735082482880:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:../deps/openssl/openssl/ssl/s23_clnt.c:683:

But think I am on the right track using a config like this:

var mysql      = require('./node-mysql');
var connection = mysql.createConnection({
  host     : '127.0.0.1',
  user     : 'root',
  password : '',
  database : 'test',
  ssl      : { key: null, cert: null, ca : [ fs.readFileSync('server-cert-bogus.pem') ] }
});

@crh3675
Copy link
Author

crh3675 commented May 10, 2013

After looking more into the code I noticed that I was implementing the
SSL in a rather unconventional way than you had built the library.

Please review the (emailed) diff file as I created the functionality as an
appropriate sequence. Haven't end-to-end tested yet but is starting to
look like the right direction.

Thanks
Craig

@crh3675
Copy link
Author

crh3675 commented May 11, 2013

Ok, forked the repo to post my updates, seemed a lot easier:

https://github.com/crh3675/node-mysql-ssl-dev

Still testing and in development

@felixge
Copy link
Collaborator

felixge commented May 13, 2013

@crh3675 awesome, keep us posted!

@Jonahss
Copy link

Jonahss commented May 14, 2013

@crh3675 keep it up, looking forward to this feature.

@crh3675
Copy link
Author

crh3675 commented May 14, 2013

Having a blocker with OpenSSL right now, it doesn't seem to be playing nice with MySQL. If anyone wants to jump on board, feel free. Here is the issue:

https://forums.aws.amazon.com/thread.jspa?messageID=450289

I have also sent a request to the OpenSSL team to help resolve.

@reedog117
Copy link

I'm definitely interested in this too. Let me know if you need additional help with testing. SSL support would be a lifesaver for a few different projects I'm working on.

@crh3675
Copy link
Author

crh3675 commented May 29, 2013

I have an update but it is rather sad at this time. In order for SSL to work within NodeJs when communicating with MySQL, NodeJS must be manually compiled with NPN_ENABLED against a current version of OpenSSL.

Without NPN_ENABLED, you cannot override the default protocol that the TLS module (OpenSSL) uses to communicate with MySQL and you will always get this error:

Error: 140378910898176:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:s23_clnt.c:766:

You will get that error (as MySQL expects TLS/1) unless your version of NodeJS recognizes the option:

NPNProtocols: [ 'tls/1' ]

(doc: http://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback)

If anyone wants to manually compile NodeJS and checkout the code I have added on my fork https://github.com/crh3675/node-mysql-ssl-dev, please have at it. I didn't realize how exhaustive this would actually be :-)

@Jonahss
Copy link

Jonahss commented May 29, 2013

Wow, good detective work there.
Appreciate the time you put into it.

@felixge
Copy link
Collaborator

felixge commented May 31, 2013

@crh3675 wow. That sucks. Do you know if there is any reason why node may not enable this by default? If not, I think it's worth raising a bug with node.

@crh3675
Copy link
Author

crh3675 commented May 31, 2013

It isn't Node that has the issue, it is the version of OpenSSL that Node
is complied with. I typically use a package manager (yum) to install
server components. The pre-compiled versions are tied to older versions
of OpenSSL.

I presume if someone manually compiled Node with the most recent
OpenSSL, it would work.

On 5/31/13 2:58 PM, Felix Geisendörfer wrote:

@crh3675 https://github.com/crh3675 wow. That sucks. Do you know if
there is any reason why node may not enable this by default? If not, I
think it's worth raising a bug with node.


Reply to this email directly or view it on GitHub
#481 (comment).

@danielsantiago
Copy link
Contributor

Thanks for your time, this is a must!

@felixge
Copy link
Collaborator

felixge commented Jun 7, 2013

@crh3675 well, I guess we'll just have to document this for people who need SSL. Or do you see another option?

@crh3675
Copy link
Author

crh3675 commented Jun 10, 2013

I probably should spend some time and compile a custom version of NodeJS against a valid version of OpenSSL that allows NPN support. Then, I will be in a better position to say that it at least "works".

@KeithBronstrup
Copy link

Any update? It would be great to have SSL support in this library!

@danielsantiago
Copy link
Contributor

https://github.com/sidorares/node-mysql2 has succesfully added SSL support. @crh3675 maybe in this library you can find something to add SSL support to node-mysql.

@crh3675
Copy link
Author

crh3675 commented Jan 2, 2014

Any updates with porting SSL from node-mysql2 over to this project? @sidorares

@danielsantiago
Copy link
Contributor

I see that @sidorares is contributing more to node-mysql. SSL is the only reason I'm using node-mysql2, but node-mysql is more contributions.

@crh3675
Copy link
Author

crh3675 commented Jan 2, 2014

Yeah, but most other libraries I am using, like SailsJS is built on node-mysql 2.0.0-alpha8 and I really don't want to have to replace core functionality with a different library just for SSL. It would be great if the concept could be merged in.

@KeithBronstrup
Copy link

I would like this, as well. I'll look into it this weekend and report back. If it looks like something I'll be able to do in my spare time I'll fork it and make it happen; hopefully @felixge will accept the pull request if I do it.

@danielsantiago
Copy link
Contributor

If @sidorares don't have time to work on this, maybe he could document how to implement this.

@crh3675
Copy link
Author

crh3675 commented Jan 2, 2014

I tried previously with node-mysql but am not as wise as @felixge and @sidorares so my attempt was left incomplete

@danielsantiago
Copy link
Contributor

@crh3675 have you try to look how it's implemented in https://github.com/sidorares/node-mysql2 ?

@crh3675
Copy link
Author

crh3675 commented Jan 2, 2014

lib/commands/client_handshake.js looks to be where it gets invoked and the base object resides here lib/packets/ssl_request.js.

It looks straight forward but I don't want to "bastardize" another developer's code to gerry-rig in a fix

@sidorares
Copy link
Member

It's actually quite simple. I might try to find time to add ssl support (at risk of losing mysql2 users :)) ) but no promise - very busy time for me.

You just need to create secure socket pair and pipe to it after initial "request ssl packet". This is how you create secure pair: https://gist.github.com/TooTallNate/848444 SSL Request is similatr to auth packet https://github.com/sidorares/node-mysql2/blob/master/lib/packets/ssl_request.js , but shorter and and client flag ClientConstants.SSL set to 1. After it is sent you immediately switch to secure connection

@KeMBro2012 - if you can do it I'll be happy to review. It should be a little bit simpler to stack protocols in node-mysql, I'm trying to optimise too much and don't use streams properly and connect everything manually (compression can be done as easy as another Transform stream and one pipe)

@crh3675 - there is one more reason to use mysql2 - it's still considerably faster according to my benchmarks, from 20% with 1row results up to x2 with large result sets

@crh3675
Copy link
Author

crh3675 commented Jan 2, 2014

@sidorares I will swap out the library on sails-mysql and see if I run into any issues running node-mysql2

@felixge
Copy link
Collaborator

felixge commented Jan 3, 2014

@KeMBro2012 I'm also happy to review. This feature is pretty uncontroversial, so if you get a working patch, it's very likely going to be merged.

@KeithBronstrup
Copy link

@felixge and @sidorares, thanks for pledging the time to review my work if I do this. I don't have a ton of free time to throw at this, but I certainly would like to add SSL support to a project of mine using node-mysql and really don't want to switch libraries.

I wouldn't advise relying on this being done in the next few weeks unless @sidorares gets to it in that time, but if I get to it first, I'll be more than happy to see it put to good use.

@crh3675
Copy link
Author

crh3675 commented Jan 3, 2014

Thanks guys, the SSL is important to me for HIPAA related data transfer and storage so adding it in would be huge for me

@sidorares sidorares mentioned this issue Feb 5, 2014
@sidorares
Copy link
Member

@KeMBro2012 @crh3675 - could you try master?

@dresende
Copy link
Collaborator

Open a specific issue if a problem arises with SSL.

@crh3675
Copy link
Author

crh3675 commented Feb 20, 2014

I'll try as well!

@crh3675
Copy link
Author

crh3675 commented Feb 20, 2014

I chose to integrate with SailsJS which is using sails-mysql which looks to be at version ~2.0.1. I'll see if any issues arise with it.

@sidorares
Copy link
Member

@crh3675 - v2.1.0 is the first version with ssl - afaik ~2.0.1 matches only 2.0.x versions

@ANDRIUS-D-ILGUNAS
Copy link

I see that ssl is available in mysql v2.1.1, though still trying to figure out the syntax. Any tips? Other than referencing the Crypto docs?

But my main question is, now that mysql is fully(?) capable of using ssl, how are the goals of mysql2 different? Maybe this ought to be another thread . . .

@dougwilson
Copy link
Member

You can check out the ssl option at the bottom of https://github.com/felixge/node-mysql#connection-options for information. MySQL SSL is not the same as like HTTPS, so you need to do more configuring to use SSL. Typically you just need to provide the ca according to your MySQL setup.

@ANDRIUS-D-ILGUNAS
Copy link

Right, that was the document I was looking at to try to understand what the syntax is to configure it to use SSL. And I've just re-read the doc on the crypto page. Apparently I missed that it's supposed to be a dictionary. Thanks for having me re-read it!

I'm still hoping for elucidation on the deltas between mysql and mysql2

@dougwilson
Copy link
Member

I'll look into adding more details on the Readme. Please ask about mysql2 on their project page :)

@dougwilson dougwilson reopened this Apr 24, 2014
@dougwilson dougwilson added this to the 2.2 milestone Apr 24, 2014
@ANDRIUS-D-ILGUNAS
Copy link

Will do. Thank you for your help :)

@sidorares
Copy link
Member

@t3knos @dougwilson ssl parameter passed as is to crypto.createCredentials() unless it's a string, in that case it's name of pre-defined profile (again, profile is same hash as createCredentials expects). Currently we ship only one profile - 'Amazon RDS'. Yeah, probably need better documentation in readme

@sidorares
Copy link
Member

oops, I basically wrote exactly same as in readme. IMO should be enough @t3knos - can you indicate what was misleading to you in docs?

@dougwilson
Copy link
Member

I don't think anything was misleading just that @t3knos would have liked to see an example (like ssl: {ca: ...}) to better put the pieces together.

@sidorares
Copy link
Member

I think we should indicate difference with node-mariasql ssl options - keys are the same, but values are actual keys/certs (as string or Buffer) and not paths to them.

@ANDRIUS-D-ILGUNAS
Copy link

@sidorares , dougwilson is exactly correct in that nothing was misleading. I'm just new to node, and I was trying to understand the syntax, looking for an example.

Thanks a brazillion for everyone's hard work! Maybe I can contribute back one of these days,...soon.

dveeden pushed a commit to dveeden/mysql that referenced this issue Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

10 participants