FIX Add back missing SSL support for database connections #10784

GuySartorelli · 2023-05-18T02:27:59Z

Reinstates the SSL support for databases which was unintentionally partially removed at some stage.

Note for testing

You'll need to set up SSL for mysql, obviously. The official mysql docker image does already have SSL enabled, and has keys in the /var/lib/mysql/ directory.
You can copy the relevant files out of the container to your host with the following commands:

docker cp CONTAINER_NAME:/var/lib/mysql/ca.pem ./ca.pem
docker cp CONTAINER_NAME:/var/lib/mysql/client-key.pem ./client-key.pem
docker cp CONTAINER_NAME:/var/lib/mysql/client-cert.pem ./client-cert.pem

Note that if you go that route, you will most likely run into this error:

ERROR [Warning]: mysqli::real_connect(): Peer certificate CN='MySQL_Server_8.0.30_Auto_Generated_Server_Certificate' did not match expected CN='database-container'

This is because PHP is trying to validate that the server's SSL cert is correct, and in checking the common name it finds that the common name which was set at the time the SSL cert was created doesn't match the host name your webserver docker container is using to access the database docker container.

You can bypass that validation for the purposes of testing this PR by making the following change in vendor/silverstripe/framework/src/ORM/Connect/MySQLiConnector.php:

  $this->dbConn->real_connect(
      $parameters['server'],
      $parameters['username'],
      $parameters['password'],
      $selectedDB,
-     !empty($parameters['port']) ? $parameters['port'] : ini_get("mysqli.default_port")
+     !empty($parameters['port']) ? $parameters['port'] : ini_get("mysqli.default_port"),
+     null,
+     MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT
  );

It says "don't verify server cert" but it actually only disables validating the common name.

You can, alternatively, either make it so that the host name your webserver is using to access the db container matches the common name of the existing cert, or go down the rabbit hole of creating new certs for the mysql instance to use.

Checking if you have a working SSL connection

Put this somewhere, e.g. in a page controller or in _config.php:

use SilverStripe\ORM\Connect\MySQLQuery;
use SilverStripe\ORM\DB;

/** @var MySQLQuery $result */
$result = DB::get_conn()->getConnector()->query("SELECT * FROM performance_schema.session_status WHERE VARIABLE_NAME IN ('Ssl_version','Ssl_cipher')");

echo '<pre>';
var_Dump($result->map());
echo '</pre>';

The output when not using SSL:

array(2) {
["Ssl_cipher"]=>
string(0) ""
["Ssl_version"]=>
string(0) ""
}

The output when using SSL:

array(2) {
["Ssl_cipher"]=>
string(22) "TLS_AES_256_GCM_SHA384"
["Ssl_version"]=>
string(7) "TLSv1.3"
}

Issue

MySQL SSL Support - gone? #8871

Follow on issues created

Add environment variables for mysqli connection params #10787

GuySartorelli · 2023-05-18T02:29:14Z

src/Core/CoreKernel.php

+        // Having only the key or cert without the other is bad configuration.
+        if (isset($databaseConfig['ssl_key']) xor isset($databaseConfig['ssl_cert'])) {
+            user_error('Database SSL cert and key must both be defined to use SSL in the database.', E_USER_WARNING);
+            unset($databaseConfig['ssl_key']);
+            unset($databaseConfig['ssl_cert']);
+        }


A warning instead of an exception because if a live environment is poorly configured this shouldn't necessarily break the entire site - but it should provide a warning in the logs.

Note that the previous implementation didn't even give a warning, so this is an improvement over what would have been here if SSL support had never been erroneously removed in the first place.

GuySartorelli · 2023-05-18T02:30:00Z

src/Core/CoreKernel.php

+        }
+
+        // Having only the key or cert without the other is bad configuration.
+        if (isset($databaseConfig['ssl_key']) xor isset($databaseConfig['ssl_cert'])) {


Must have either the CA, or both the key and cert, or all three.
This is mirrored in the original implementation: https://github.com/silverstripe/silverstripe-framework/pull/7233/files#diff-6b84b243cc8c7653ef8cadbb9fa5620f991f7454609aaea3f6527e45c3a5820eR128-R142

Can you rewrite without xor, it's very uncommon so many people would need to lookup what it means in order to read this code

I think xor as a concept isn't by any means unusual - but I'll rewrite this as I don't feel any particular need to keep it. It'll just make the condition a little more convoluted but not to the extent that it would be worth me pushing back on.

GuySartorelli · 2023-05-18T02:31:06Z

src/Dev/Install/MySQLDatabaseConfigurationHelper.php

-                    // Set SSL parameters if they exist. All parameters are required.
-                    if (array_key_exists('ssl_key', $databaseConfig) &&
-                        array_key_exists('ssl_cert', $databaseConfig) &&
-                        array_key_exists('ssl_ca', $databaseConfig)
+                    // Set SSL parameters if they exist.
+                    // Must have both the SSL cert and key, or the common authority, or preferably all three.
+                    if ((array_key_exists('ssl_key', $databaseConfig) && array_key_exists('ssl_cert', $databaseConfig))
+                        || array_key_exists('ssl_ca', $databaseConfig)


This works fine with just the CA, and with just the key and cert.

GuySartorelli · 2023-05-18T02:42:14Z

I'll create a separate card for testing this - I don't think there's any way for us to test it with a normal unit test. Instead, I think we'll have to update our CI to use an SSL connection to the db in at least one of the runs.

Edit: silverstripe/gha-ci#68

emteknetnz · 2023-05-21T21:32:40Z

src/Core/CoreKernel.php

+        }
+
+        // Having only the key or cert without the other is bad configuration.
+        if (isset($databaseConfig['ssl_key']) xor isset($databaseConfig['ssl_cert'])) {


Can you rewrite without xor, it's very uncommon so many people would need to lookup what it means in order to read this code

emteknetnz · 2023-05-21T21:37:34Z

src/Dev/Install/MySQLDatabaseConfigurationHelper.php

-                            $databaseConfig['ssl_key'],
-                            $databaseConfig['ssl_cert'],
+                            $databaseConfig['ssl_key'] ?? null,
+                            $databaseConfig['ssl_cert'] ?? null,
                            $databaseConfig['ssl_ca'],


Should ssl_ca also have ?? null? - we have that below in MySQLiConnector

Oops, yes it should.

emteknetnz

Tested locally, confirmed it works with various combinations of key+cert, ca only, and key+cert+ca

GuySartorelli commented May 18, 2023

View reviewed changes

This was referenced May 18, 2023

MySQL SSL Support - gone? #8871

Closed

Can't currently test SSL connection works for databases silverstripe/gha-ci#68

Open

emteknetnz requested changes May 21, 2023

View reviewed changes

FIX Add back missing SSL support for database connections

b73c301

GuySartorelli force-pushed the pulls/4.13/db-ssl branch from 34f1d6f to b73c301 Compare May 21, 2023 23:09

emteknetnz approved these changes May 22, 2023

View reviewed changes

emteknetnz mentioned this pull request May 22, 2023

Add environment variables for mysqli connection params #10787

Closed

emteknetnz merged commit c4b8d9a into silverstripe:4.13 May 22, 2023

emteknetnz deleted the pulls/4.13/db-ssl branch May 22, 2023 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Add back missing SSL support for database connections #10784

FIX Add back missing SSL support for database connections #10784

GuySartorelli commented May 18, 2023 •

edited by emteknetnz

Loading

GuySartorelli May 18, 2023

GuySartorelli May 18, 2023

emteknetnz May 21, 2023

GuySartorelli May 21, 2023 •

edited

Loading

GuySartorelli May 21, 2023

GuySartorelli May 18, 2023

GuySartorelli commented May 18, 2023 •

edited

Loading

emteknetnz May 21, 2023

emteknetnz May 21, 2023

GuySartorelli May 21, 2023

GuySartorelli May 21, 2023

emteknetnz left a comment

FIX Add back missing SSL support for database connections #10784

FIX Add back missing SSL support for database connections #10784

Conversation

GuySartorelli commented May 18, 2023 • edited by emteknetnz Loading

Note for testing

Checking if you have a working SSL connection

Issue

Follow on issues created

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli May 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented May 18, 2023 •

edited by emteknetnz

Loading

GuySartorelli May 21, 2023 •

edited

Loading

GuySartorelli commented May 18, 2023 •

edited

Loading