overextended / oxmysql

MySQL resource for FXServer.
https://overextended.dev/oxmysql
GNU Lesser General Public License v3.0
309 stars 192 forks source link

SSL support in connection string Azure MySQL/MariaDB #202

Closed lalashkin closed 10 months ago

lalashkin commented 11 months ago

Not sure how fair it is to ask for such fix in repo that is problematic piece as a dependency, but assuming that FiveM users do not have direct control over node code (or want to use it as-is) - I think that's a fair place to raise this issue and share my findings.

Is your feature request related to a problem? Please describe. I started playing around with oxmysql, and since I had some free credits laying around on Azure decision was made to put them to good use. However, I met with unexpected lack of support for SSL - default behavior, without extra flags, produced following result:

[      script:oxmysql] Error 9002: SSL connection is required. Please specify SSL options and retry.<\0>

When using ssl=true flags in Connection String or equivalent - it changed to this:

cfx> (node:20092) UnhandledPromiseRejectionWarning: TypeError: Unknown SSL profile 'true'
    at Function.getSSLProfile (@oxmysql/dist/build.js:15847:17)
    at new ConnectionConfig (@oxmysql/dist/build.js:15753:71)
    at new PoolConfig (@oxmysql/dist/build.js:16874:33)
    at Object.exports.createPool (@oxmysql/dist/build.js:17189:34)
    at createPool2 (@oxmysql/dist/build.js:17607:29)
    at setConnectionPool (@oxmysql/dist/build.js:24787:40)
    at @oxmysql/dist/build.js:24797:3
    at Object.callback (citizen:/scripting/v8/timer.js:145:21)
    at onTick (citizen:/scripting/v8/timer.js:171:27)
    at citizen:/scripting/v8/timer.js:270:13

After diffing around in build.js, which I assume is just bundled node-mysql2 library, it seems that it uses some sort of "SSLProfiles" which is populated through require_ssl_profiles() function. There are already predefined SSL CAs for Amazon RDS services, however - no other services are defined. https://github.com/sidorares/node-mysql2/blob/master/lib/constants/ssl_profiles.js

Describe the solution you'd like Solution was quite simple - we need to define one of the "SSLProfiles" manually in build.js, for it to be picked up from Connection String defined in FXServer configuration:

    exports["Azure"] = {
    };

For why it requires empty object - I'm not sure, I assume this is the equivalent of --ssl flag on MySQL command line for node-mysql2 library. As for why there's no CA contents - see Additional Context.

Now Connection String can accept ssl=Azure; flag or equivalent.

Additional context I was mislead heavily by this support article on Microsoft Docs: https://learn.microsoft.com/en-us/azure/mariadb/concepts-ssl-connection-security#default-settings

Using it as a content for ca: <cert> confuses mysql2 library, how exactly - can't tell, but it seems to be something with certificate chain:

cfx> [      script:oxmysql] Unable to establish a connection to the database (HANDSHAKE_SSL_ERROR)!
[      script:oxmysql] Error undefined: unable to get local issuer certificate

Both oxmysql and CLI returned similar results:

ERROR 2026 (HY000): TLS/SSL error: unable to get local issuer certificate

Describe alternatives you've considered I've cross-checked node-mysql2 repo, and seems contents of the "SSLProfiles" file are similar there - issue can be raised there, and profile can be introduced directly in dependency.

Or everyone who will stumble across this issue can just manually edit build.js as mentioned above - and it's gonna work for him.

Sorry if this is irrelevant for this specific project - I just wanted to share my findings and maybe expedite some change, if possible :)

thelindat commented 11 months ago

When using ssl=true flags in Connection String or equivalent

https://github.com/sidorares/node-mysql2/blob/master/typings/mysql/lib/Connection.d.ts#L252 https://github.com/sidorares/node-mysql2/blob/master/typings/mysql/lib/Connection.d.ts#L23

lalashkin commented 11 months ago

When using ssl=true flags in Connection String or equivalent

https://github.com/sidorares/node-mysql2/blob/master/typings/mysql/lib/Connection.d.ts#L252 https://github.com/sidorares/node-mysql2/blob/master/typings/mysql/lib/Connection.d.ts#L23

Thanks for the prompt reply, this might indeed resolve my question

However, I'm not quite sure on how to pass object through connection string:

set mysql_connection_string "user=fxserverdb;password=;host=example.azure.com;port=3306;database=fxserver;ssl=?"

Passing curly braces attempting to setup object just results in the following behaviour:

cfx> (node:31452) UnhandledPromiseRejectionWarning: TypeError: Unknown SSL profile '{}'

Singular quotes are not supported in *.cfg, and I'm a bit confused - maybe I'm completely off the mark here?

thelindat commented 11 months ago

Connection options for SSL and some other settings use JSON.

lalashkin commented 11 months ago

I was still having issues with string passed to MySQL2 as-is from the connection string, rather than been parsed as JSON, as you mentioned.

I've dug through the source code of src/config.ts and found line responsible for the conversion: https://github.com/overextended/oxmysql/blob/0398abc501453098823244a1bfe93bb27091b538/src/config.ts#L83-L91

On doing some log outputs, it seems that key value in loop equals 0, 1, 2 instead of "dateStrings", "flags", "ssl" - that makes value undefined. Reading through some google results, it seems like for-in for Arrays is used for enumeration, rather then regular array traverse. for-of on the other hand is used for iteration, just as intended, I assume. And thus - failing compare to string type and exiting without conversion.

I've replaced for-in with for-of - and conversion worked as expected. Attaching pull request, if my solution makes sense and I didn't overlook something https://github.com/overextended/oxmysql/pull/203

looterz commented 10 months ago

Thanks for this fix, I have been experiencing the same issue when using PlanetScale and trying to enable SSL in the connection string, have been manually enabling SSL by default as a quick workaround for awhile but it seems like this will fix the issue.