Allow secure connections using SSL on mysqli #2570

pgrau · 2016-12-02T09:06:20Z

The current driver has no option to allow the usage of SSL connections on mysql. I've used the same name params as mysql client.
An example of mysql client usage would be:

mysql --ssl-ca='path_to_ca.pem' --ssl-cert='path_to_client_cert.pem' --ssl-key='path_to_client_key.pem'

deeky666 · 2017-01-17T00:46:46Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

@@ -63,6 +63,19 @@ public function __construct(array $params, $username, $password, array $driverOp

        $this->_conn = mysqli_init();

+        if (isset($driverOptions['ssl_key']) &&
+            isset($driverOptions['ssl_cert']) &&
+            isset($driverOptions['ssl_ca'])


Can you tell which of those 5 options are required at minimum to use this feature? I would like to have those evaluated and a MysqliException being thrown if one of the required options is missing, so that misconfiguration is exposed to the user. Unfortunately the PHP docs only state Any unused SSL parameters may be given as NULL. It doesn't give a word about which are definitely required.

Also we need test cases for all of the possibilities.

Also please make them connection parameters instead of driver options. We have SSL related connection parameters in other drivers too. Those are more appropriate for connection parameters.

Then finally please document them in our docs (also which are required).

As you say unfortunately PHP and MySQL docs are not clear respect about what minimum of parameters are mandatory about secure connections.

I think the common use is with key, cert and ca as mandatory parameter, but I've talk with various colleagues and we think is possible stablish a secure connection with only key and cert.

In conclusion mandatory fields must be key and cert.
@deeky666 Are you agree?

@pgrau agreed. MySQL documentation also states that the ca is only mandatory if also used on the server side so we have to make it optional. Making ssl_key and ssl_cert required is a good choice then.

@deeky666 I think connection ssl parameters must be driver options as PDO library. Can you put an example of driver with ssl parameters instead driver options?

@pgrau simply put them into $params argument instead of $driverOptions. $driverOptions is exclusively used for \MYSQLI_* constants so the SSL related parameters do not fit in there.

Example

$connection = \Doctrine\DBAL\DriverManager::getConnection( array( 'dbname' => 'mydb', 'user' => 'user', 'password' => 'secret', 'host' => 'localhost', 'driver' => 'mysqli', 'ssl_key' => '/path/to/ssl_key', 'ssl_cert' => '/path/to/ssl_key', 'ssl_ca' => '/path/to/ssl_key', // ... ) );

If we put ssl parameters as connection parameters against driver options will not run ok automatically in some php frameworks as Symfony.

Example:
[Symfony\Component\Config\Definition\Exception\InvalidConfigurationException]
Unrecognized options "ssl_key, ssl_cert, ssl_ca" under "doctrine.dbal.connections.bd1"

Are you sure do you prefer ssl parameters as connection parameters against driver options?

The reason it does not work with Symfony is because the available connection parameters are defined in the DoctrineBundle. Changes would have to be done in the bundle, too then to allow using those parameters. So yes new connection parameters are still preferred over driver options.

Ok! Now, it's time to develop it : )

pgrau · 2017-01-30T22:29:23Z

@deeky666 I've done some changes. Can you review, please?

deeky666

This also requires documentation of the newly supported connection parameters for mysqli. Please see here

deeky666 · 2017-01-31T08:40:39Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

+    {
+        $matches = preg_grep('/^(ssl_)+(key|cert|ca|capath|cipher)$/', array_keys($params));
+        if ($matches && (false === array_search('ssl_key', $matches) || false === array_search('ssl_cert', $matches))) {
+            throw new MysqliException('ssl_key and ssl_cert are mandatory for secure connections');


It is weird to throw an exception in a boolean method, also this method is not really required. I think you do not need to make it more complicated than it is:

private function setSecureConnection(array $params) { if (! isset($params['ssl_key']) && ! isset($params['ssl_cert']) && ! isset($params['ssl_ca']) && ! isset($params['ssl_capath']) && ! isset($params['ssl_cipher']) ) { return; } if (! isset($params['ssl_key']) || ! isset($params['ssl_cert']) ) { // exception } // $this->_conn->ssl_set() }

deeky666 · 2017-01-31T08:46:20Z

tests/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php

+    /**
+     * @dataProvider secureMissingParamsProvider
+     */
+    public function testItShouldReturnAnExceptionWhenMissingMandatorySecureParams(array $secureParams)


Can you please reword to testThrowsExceptionWhenMissingMandatorySecureParams as we do not really test that an exception is "returned". Also "it should" convention is more of a BDD / PHPSpec one. We do not use it in PHPUnit ;)

deeky666 · 2017-01-31T08:52:45Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

+    {
+        $matches = preg_grep('/^(ssl_)+(key|cert|ca|capath|cipher)$/', array_keys($params));
+        if ($matches && (false === array_search('ssl_key', $matches) || false === array_search('ssl_cert', $matches))) {
+            throw new MysqliException('ssl_key and ssl_cert are mandatory for secure connections');


Also can we reword the exception message to something like "ssl_key" and "ssl_cert" parameters are mandatory when using secure connection parameters.? I usually am not so picky about this but it can be confusing in certain circumstances, when it is not clear to the user that he used (or misused) one of the SSL connection parameters. This way it gets more obvious that SSL parameters are not mandatory for a mysqli connection in general, only if used explicitly.

pgrau · 2017-01-31T10:35:31Z

@deeky666 You can see the doc in 8788fd4

deeky666

Nearly done. Just some last nitpicks.

deeky666 · 2017-01-31T12:55:00Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

+
+        if (! isset($params['ssl_key']) || ! isset($params['ssl_cert'])) {
+            $msg = "\"ssl_key\" and \"ssl_cert\" parameters are mandatory when using secure connection parameters.";
+            throw new MysqliException($msg);


Please don't do unnecessary string escaping. You can easily avoid that by using single quotes:

throw new MysqliException( '"ssl_key" and "ssl_cert" parameters are mandatory when using secure connection parameters.' );

deeky666 · 2017-01-31T12:57:36Z

tests/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php

+    public function testThrowsExceptionWhenMissingMandatorySecureParams(array $secureParams)
+    {
+        $this->expectException(MysqliException::class);
+        $msg = "\"ssl_key\" and \"ssl_cert\" parameters are mandatory when using secure connection parameters.";


deeky666 · 2017-01-31T12:58:16Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

@@ -263,4 +264,35 @@ public function ping()
    {
        return $this->_conn->ping();
    }
+
+    /**
+     * Stablish a secure connection


Typo: Establish

pgrau

@deeky666 It's all ok?

Do you want I move php5 code to php7
Example:

php5:
       $port = isset($params['port']) ? $params['port'] : ini_get('mysqli.default_port');

        // Fallback to default MySQL port if not given.
        if ( ! $port) {
            $port = 3306;
        }

        $socket = isset($params['unix_socket']) ? $params['unix_socket'] : ini_get('mysqli.default_socket');
        $dbname = isset($params['dbname']) ? $params['dbname'] : null;

        $flags = isset($driverOptions[static::OPTION_FLAGS]) ? $driverOptions[static::OPTION_FLAGS] : null;

php7:
     
        $port = $params['port'] ?? ini_get('mysqli.default_port') ?? 3306;
        $socket = $params['unix_socket'] ?? ini_get('mysqli.default_socket');
        $dbname = $params['dbname'] ?? null;
        $flags = $driverOptions[static::OPTION_FLAGS] ?? null;

pgrau · 2017-01-31T14:37:30Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

@@ -263,4 +264,35 @@ public function ping()
    {
        return $this->_conn->ping();
    }
+
+    /**
+     * Stablish a secure connection


pgrau · 2017-01-31T14:37:37Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php

+
+        if (! isset($params['ssl_key']) || ! isset($params['ssl_cert'])) {
+            $msg = "\"ssl_key\" and \"ssl_cert\" parameters are mandatory when using secure connection parameters.";
+            throw new MysqliException($msg);


deeky666 · 2017-02-01T12:04:05Z

@pgrau no please don't convert any code to PHP7 that could potentially also be backported to DBAL 2.5 (we are still supporting PHP 5.3 there). Doing so might cause a real hassle when backporting code.

deeky666 · 2017-02-01T12:07:11Z

@pgrau the patch looks good to me now. Can you please squash your commits? I'll merge afterwards. 👍

pgrau · 2017-02-01T13:37:38Z

@deeky666 I squash all commits.

The current driver has no option to allow the usage of SSL connections on mysql. I've used the same name params as mysql client. An example of mysql client usage would be: mysql --ssl-ca='path_to_ca.pem' --ssl-cert='path_to_client_cert.pem' --ssl-key='path_to_client_key.pem' refactor secure connection as params remove space add space refactor secure connection rename method rename method docs: add ssl params to use in mysqli driver rename test method refactor method setSecureConnection fix bug add comment to method remove string escaping

deeky666 · 2017-02-01T14:02:02Z

@pgrau seems you did some kind of merge instead of a squash. There are still 15 commits. You need to:

git rebase -i origin/master
git push -f origin patch-1

While rebasing you can select the commits to squash.

pgrau · 2017-02-01T14:02:40Z

@deeky666 please, see now.

deeky666 · 2017-02-01T14:08:45Z

@pgrau thanks! :)

deeky666 suggested changes Jan 17, 2017

View reviewed changes

deeky666 self-assigned this Jan 17, 2017

deeky666 added Configuration Connecting Drivers Improvement MySQL mysqli labels Jan 17, 2017

deeky666 suggested changes Jan 31, 2017

View reviewed changes

pgrau commented Feb 1, 2017

View reviewed changes

deeky666 added this to the 2.6 milestone Feb 1, 2017

pgrau force-pushed the patch-1 branch from 5a1d104 to 78e5837 Compare February 1, 2017 13:52

deeky666 approved these changes Feb 1, 2017

View reviewed changes

deeky666 merged commit f347a83 into doctrine:master Feb 1, 2017

antoinedeschenes mentioned this pull request Jun 5, 2017

Mysqli is missing some driver connection options #2735

Closed

Ocramius changed the title ~~Mysqli: Allow secure connections using SSL~~ Allow secure connections using SSL on mysqli Jul 22, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow secure connections using SSL on mysqli #2570

Allow secure connections using SSL on mysqli #2570

pgrau commented Dec 2, 2016

deeky666 Jan 17, 2017

deeky666 Jan 17, 2017

pgrau Jan 25, 2017

deeky666 Jan 25, 2017

pgrau Jan 25, 2017

deeky666 Jan 27, 2017

pgrau Jan 30, 2017

deeky666 Jan 30, 2017

pgrau Jan 30, 2017

pgrau commented Jan 30, 2017

deeky666 left a comment

deeky666 Jan 31, 2017

pgrau Jan 31, 2017

deeky666 Jan 31, 2017

pgrau Jan 31, 2017

deeky666 Jan 31, 2017

pgrau Jan 31, 2017

pgrau commented Jan 31, 2017

deeky666 left a comment

deeky666 Jan 31, 2017

pgrau Jan 31, 2017

deeky666 Jan 31, 2017

deeky666 Jan 31, 2017

pgrau Jan 31, 2017

pgrau left a comment •

edited

Loading

pgrau Jan 31, 2017

pgrau Jan 31, 2017

deeky666 commented Feb 1, 2017

deeky666 commented Feb 1, 2017

pgrau commented Feb 1, 2017 •

edited

Loading

deeky666 commented Feb 1, 2017

pgrau commented Feb 1, 2017

deeky666 commented Feb 1, 2017

Allow secure connections using SSL on mysqli #2570

Allow secure connections using SSL on mysqli #2570

Conversation

pgrau commented Dec 2, 2016

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

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

Choose a reason for hiding this comment

pgrau commented Jan 30, 2017

deeky666 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrau commented Jan 31, 2017

deeky666 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeky666 commented Feb 1, 2017

deeky666 commented Feb 1, 2017

pgrau commented Feb 1, 2017 • edited Loading

deeky666 commented Feb 1, 2017

pgrau commented Feb 1, 2017

deeky666 commented Feb 1, 2017

pgrau left a comment •

edited

Loading

pgrau commented Feb 1, 2017 •

edited

Loading