Make WordPress Core

Opened 10 years ago

Last modified 11 months ago

#28625 assigned enhancement

Enhancement: Add constants to support SSL connections for mysqli

Reported by: hypertextranch's profile hypertextranch Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Database Keywords: has-patch needs-unit-tests
Focuses: privacy Cc:

Description

In order to support SSL'ed MySQL connections with the mysqli_* functions introduced in WordPress 3.9 / PHP 5.5 mysqli_ssl_set() must be called to set the path to the SSL certs and keys to be used by the MySQL client before mysqli_real_connect() is called.

We should add the following optional constants to allow for users to configure secure connections to the database.

  • MYSQL_SSL_KEY
  • MYSQL_SSL_CERT
  • MYSQL_SSL_CA
  • MYSQL_SSL_CA_PATH
  • MYSQL_SSL_CIPHER

In addition this should only be set if the feature flag MYSQLI_CLIENT_SSL is set for the existing MYSQL_CLIENT_FLAGS constant.

Attachments (5)

r28625.patch (1.0 KB) - added by hypertextranch 10 years ago.
Initial proposed patch
sslkey.patch (1.2 KB) - added by andrewbevitt 10 years ago.
Patch which passes options directly instead of checking files exist
sslkey-v2.patch (1.2 KB) - added by andrewbevitt 10 years ago.
Updated to document when SSL options are set.
28625.2.diff (1.5 KB) - added by pbiron 3 years ago.
update patch so it applies cleanly
db.php (5.5 KB) - added by szaqal21 3 years ago.
db.php

Download all attachments as: .zip

Change History (30)

@hypertextranch
10 years ago

Initial proposed patch

#1 @hypertextranch
10 years ago

  • Keywords has-patch added
  • Summary changed from Enhancement: Add constants for support SSL connections for mysqli to Enhancement: Add constants to support SSL connections for mysqli

#2 @andrewbevitt
10 years ago

I agree this should be included. But, the patch shouldn't silently drop the certificate file if the file does not exist. This would allow the connection to be opened without SSL; assuming the MySQL server allows that.

It would be better to pass the parameter as given, and allow the mysqli_real_connect call to fail. This seems to be the intended idea, as the PHP docs for mysqli_ssl_set indicate it always returns TRUE (http://www.php.net/manual/en/mysqli.ssl-set.php).

Last edited 10 years ago by andrewbevitt (previous) (diff)

@andrewbevitt
10 years ago

Patch which passes options directly instead of checking files exist

#3 @hypertextranch
10 years ago

Your proposed method works for me. But just for good measure I think we should add a comment in the if ( $call_set ) block, maybe something along the lines of...

// SSL added here! :-)

#4 @DrewAPicture
10 years ago

  • Keywords dev-feedback added

@andrewbevitt
10 years ago

Updated to document when SSL options are set.

#5 @SergeyBiryukov
10 years ago

  • Keywords 4.1-early added
  • Milestone changed from Awaiting Review to Future Release

#6 @hypertextranch
9 years ago

Seems like this did not get into 4.1; guess we should target 4.2 instead?

#7 @szaqal21
9 years ago

Keep in mind that solution based on constants will force SSL for every new connection so there could be problem when creating new wpdb for database which doesn't support SSL. I think SSL should be set per wpdb object.

#8 @nacin
8 years ago

@pento, any thoughts on this?


How do other software projects handle configuration for this? I presume they do not use constants, and thus can use an array (in the case of PHP projects).

Replying to szaqal21:

Keep in mind that solution based on constants will force SSL for every new connection so there could be problem when creating new wpdb for database which doesn't support SSL. I think SSL should be set per wpdb object.

I agree with the concern here.

#9 follow-up: @pento
8 years ago

  • Keywords needs-refresh needs-unit-tests added; dev-feedback 4.1-early removed
  • Milestone changed from Future Release to 4.5

Other projects generally use PDO, and do their DB config as arrays, instead of constants.

mysqli_ssl_set() was added in PHP 5.3 - we probably should add a function_exists() check, even though it'd really only be a problem if someone has defined WP_USE_EXT_MYSQL as false in PHP 5.2. (And maybe fail to connect in this case, so that we're not silently using a non-SSL connection.)

For the concern about new wpdb instances, perhaps we need to add an $options parameter to wpdb::__construct()? $options['ssl'] would be an array of the SSL options (if your global config doesn't do SSL, or you want different SSL options), or false to prevent an SSL connection. Undefined uses the global config.

For the actual patch itself, I'm not wild about adding a pile of constants, but that's kind of the best option that keeps with the WordPress-y way. I can't decide if the foreach / call_user_func_array combination is too clever, or just the right amount of clever. It'd need to be reworked to work with $options, anyway. Also, it needs unit tests.

With all that said, I find myself being in favour of adding this functionality to Core, instead of recommending a drop-in. It's useful, it increases security, and I don't think it's possible to render your site unrecoverably broken (worst case: remove the constants from wp-config.php) with it.

#10 in reply to: ↑ 9 @INNOVOT
8 years ago

Replying to pento:

With all that said, I find myself being in favour of adding this functionality to Core, instead of recommending a drop-in. It's useful, it increases security, and I don't think it's possible to render your site unrecoverably broken (worst case: remove the constants from wp-config.php) with it.

100% agree with the final sentence. In this day and age there is no excuse not to use secure communications. Hope this does make it to Core.

Last edited 8 years ago by INNOVOT (previous) (diff)

#11 @johnbillion
8 years ago

  • Owner set to pento
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#13 @jorbin
8 years ago

  • Milestone changed from 4.5 to Future Release

#14 @hypertextranch
8 years ago

I agree with most of what pento suggests as well but in the meantime I have published the plugin I've been using to set SSL flags for connections as a stop gap.

https://wordpress.org/plugins/secure-db-connection/

#15 @maheshbhavana
7 years ago

Why is this not yet part of latest wordpress release? I thought ssl is basic.

This ticket was mentioned in Slack in #forums by danhgilmore. View the logs.


7 years ago

#17 @pento
4 years ago

  • Owner pento deleted

#18 @pbiron
3 years ago

Just wondering what is necessary to jump-start the effort on this ticket moving again (hopefully, for 5.8)?

I have a new project where I require secure DB connections. I've tests @hypertextranch 's plugin and it seems that it will work for us. However, since it uses a custom db.php drop-in, would be really nice to have this in core as there are many other highly used plugins (e.g., W3TC and QM) that need/want their own drop-ins.

@pbiron
3 years ago

update patch so it applies cleanly

#19 @pbiron
3 years ago

  • Keywords needs-refresh removed

refreshed patch so that it applies cleanly.

#20 @pbiron
3 years ago

  • Focuses privacy added

#21 follow-up: @pbiron
3 years ago

Note, I discovered over the weekend that, in at least some cases, WP 5.6 is capable of opening a secure connection to MySQL without any mods (or need for something like the Secure DB Connection plugin).

Don't know whether that is a result of changes in core/MySQL/PHP since this ticket was opened (until a few days ago I didn't know that encrypted connections to MySQL where possible :-)

I've tested in the follow 2 environments, and all I had to do was add

define( 'MYSQL_CLIENT_FLAGS', MYSQLI_CLIENT_SSL )

to wp-config.php to get an encrypted connection.

Windows

WP 5.6
PHP 7.4.13
MySQL 5.7.10
Apache 2.4.37

CentOS 7

WP 5.6
PHP 7.4.14
MySQL 5.7.33
Apache 2.4.6 (altho I believe this includes many patches after that offial version num)

That doesn't mean that this ticket can be closed however. I guess there are setups that would require specifying other than default certs, which the patch allows to happen.

p.s. threw together a little plugin that displays whether the connection is secure:

  • At a Glance (and Network Right Now) dashboard widget (like Secure DB Connection does)
  • an admin bar node
  • a Site Health test and debug_info (the text of those need more work)

You can grab it from it from GitHub

#22 in reply to: ↑ 21 ; follow-up: @hypertextranch
3 years ago

Replying to pbiron:

Note, I discovered over the weekend that, in at least some cases, WP 5.6 is capable of opening a secure connection to MySQL without any mods (or need for something like the Secure DB Connection plugin).

Don't know whether that is a result of changes in core/MySQL/PHP since this ticket was opened (until a few days ago I didn't know that encrypted connections to MySQL where possible :-)

I've tested in the follow 2 environments, and all I had to do was add

define( 'MYSQL_CLIENT_FLAGS', MYSQLI_CLIENT_SSL )

to wp-config.php to get an encrypted connection.

I believe it depends on your server setup, if the system MySQL client was installed with trusted root certs and your database is using a key/cert that's signed by a root cert that you trust on the client then just adding the MYSQLI_CLIENT_SSL flag is enough. On modern systems with more root CAs preinstalled and use of cloud based database services that configure and install keys signed by common root CAs the need to explicitly set custom keys/certs/CAs becomes less needed.

This issue / patch was made for a time when things like https://letsencrypt.org didn't exist and spinning up a database didn't always come with a cert and might mean needing to generate random self-signed stuff.

#23 @szaqal21
3 years ago

SSL shouldn't be set by constants inside db_connect(), it will force SSL for every new wpdb() even for custom (external connections) to databases without SSL support. SSL should be set by passing some extra args to new wpdb() or some kind of filter before connection is made.

Attaching my db.php dropin.

@szaqal21
3 years ago

db.php

#24 in reply to: ↑ 22 @miahdsl
11 months ago

Replying to hypertextranch:

I believe it depends on your server setup, if the system MySQL client was installed with trusted root certs and your database is using a key/cert that's signed by a root cert that you trust on the client then just adding the MYSQLI_CLIENT_SSL flag is enough. On modern systems with more root CAs preinstalled and use of cloud based database services that configure and install keys signed by common root CAs the need to explicitly set custom keys/certs/CAs becomes less needed.

This issue / patch was made for a time when things like https://letsencrypt.org didn't exist and spinning up a database didn't always come with a cert and might mean needing to generate random self-signed stuff.

Actually this patch is more pertinent now than you might think.

Specifically, being able to specify a key/cert/ca at the client side allows the server to reject connections with untrusted certificates. Some of the cloud hosting environments require a client side key/cert to be used.

Would very much like to see this rolled into core.

#25 @miahdsl
11 months ago

Just as a heads up for anyone else using the secure db connection plugin, you need to tweak db.php to disable mysqli error reporting as wordpress handles this itself.

Without this, installation of a clean site will fail with a blank screen and errors in the logs.

Around line 39 of db.php, right after:

if ( $this->use_mysqli ) {

Add the line:

mysqli_report( MYSQLI_REPORT_OFF );

The resultant code will look like:

if ( $this->use_mysqli ) {

mysqli_report( MYSQLI_REPORT_OFF );
$this->dbh = mysqli_init();

Note: See TracTickets for help on using tickets.