NEW Added MySQL SSL PDO Support (fixes #7077) #7233

johndalangin · 2017-08-02T13:03:14Z

Modified ConfigureFromEnv.php to parse SS_DATABASE_SSL variables (also added a bit of documentation)
Modified PDOConnector.php to implement variables set in ConfigureFromEnv if exists
Modified install files MySQLDatabaseConfigurationHelper and install.php5 to accept and implement SS_DATABASE_SSL variables set in _ss_environment.php

TODO: Add documentation

Modified ConfigureFromEnv.php to parse SS_DATABASE_SSL variables (also added a bit of documentation) Modified PDOConnector.php to implement variables set in ConfigureFromEnv if exists Modified install files MySQLDatabaseConfigurationHelper and install.php5 to accept and implement SS_DATABASE_SSL variables set in _ss_environment.php TODO: Add documentation

johndalangin · 2017-08-02T13:10:31Z

@dhensby fixes #7077

Thank you.

dhensby

Looks good - only thing I'm concerned about is tight coupling to MySQL

dhensby · 2017-08-02T14:28:03Z

conf/ConfigureFromEnv.php

@@ -116,6 +124,25 @@
 	// For SQlite3 memory databases (mainly for testing purposes)
 	if(defined('SS_DATABASE_MEMORY'))
 		$databaseConfig["memory"] = SS_DATABASE_MEMORY;
+
+	// PDO MySQL SSL parameters
+	if(defined('SS_DATABASE_CLASS') && SS_DATABASE_CLASS === 'MySQLPDODatabase') {


Is this only for MySQL using PDO or do all PDO drivers support this?

For now it's only for MySQL using PDO.

dhensby · 2017-08-02T14:29:43Z

I'd like @tractorcow's thoughts, again

johndalangin · 2017-08-02T16:23:03Z

@tractorcow @dhensby Thank you for your prompt response. I checked the contents of /model/connect and it seems that the only 2 DB connectors included by default are MySQLi and MySQLPDO.

I've taken a look at the MySQLi documentation and it seems that there might be a way to implement SSL via the mysqli::ssl_set directive. (http://php.net/manual/en/mysqli.ssl-set.php). I can create another issue and work on this as well.

I don't have any experience working with PostgreSQL or MSSQL with Silverstripe so it might take me a while to get working on an implementation. Maybe we can indicate in the documentation that SSL is only supported on MySQL via MySQLi and PDOMySQL in the mean time so we can push this forward and worry about tight coupling later? I can work on the documentation as well.

Let me know your thoughts.

Thank you.

tractorcow · 2017-08-02T21:37:47Z

conf/ConfigureFromEnv.php

+			defined('SS_DATABASE_SSL_CA')
+			) {
+
+			$databaseConfig['ssl_key'] = SS_DATABASE_SSL_KEY;


My feeling is that perhaps we should set these for all connectors, and we allow classes that support these options to configure them internally. Otherwise, $databaseConfig will keep these values but will be ignored by unsupported backends.

As it stands I wouldn't be able to support a custom connector with SSL.

Understood. The reason why I encapsulated setting the SSL variables to PDO is because I feared I would break something with the other databases. If the setting the variables will be innocuous for other database, I can remove the condition.

tractorcow · 2017-08-02T21:39:35Z

dev/install/install.php5

+			$databaseConfig['ssl_key'] = SS_DATABASE_SSL_KEY;
+			$databaseConfig['ssl_cert'] = SS_DATABASE_SSL_CERT;
+			$databaseConfig['ssl_ca'] = SS_DATABASE_SSL_CA;
+			$databaseConfig['ssl_cipher'] = defined('SS_DATABASE_SSL_CIPHER') ? SS_DATABASE_SSL_CIPHER : 'DHE-RSA-AES256-SHA';


Instead of setting the default cipher here, let's leave it up to the DB connection to choose it's own default.

tractorcow · 2017-08-02T21:40:04Z

conf/ConfigureFromEnv.php

+			$databaseConfig['ssl_key'] = SS_DATABASE_SSL_KEY;
+			$databaseConfig['ssl_cert'] = SS_DATABASE_SSL_CERT;
+			$databaseConfig['ssl_ca'] = SS_DATABASE_SSL_CA;
+			$databaseConfig['ssl_cipher'] = defined('SS_DATABASE_SSL_CIPHER') ? SS_DATABASE_SSL_CIPHER : 'DHE-RSA-AES256-SHA';


Also here, let's leave it blank if not defined, and let the connector choose it's own default.

tractorcow · 2017-08-02T21:41:21Z

model/connect/PDOConnector.php

+					$options[PDO::MYSQL_ATTR_SSL_KEY] = $parameters['ssl_key'];
+					$options[PDO::MYSQL_ATTR_SSL_CERT] =$parameters['ssl_cert'];
+					$options[PDO::MYSQL_ATTR_SSL_CA] = $parameters['ssl_ca'];
+					$options[PDO::MYSQL_ATTR_SSL_CIPHER] = $parameters['ssl_cipher'];


Lets make this a config rather than hard coding it.

private static $ssl_cipher_default = 'DHE-RSA-AES256-SHA'; // ... $options[PDO::MYSQL_ATTR_SSL_CIPHER] = $parameters['ssl_cipher'] ?: $this->config()->ssl_cipher_default;

Understood.

johndalangin · 2017-08-03T00:58:54Z

@tractorcow I'll have the changes ready within the day. Thank you for your feedback.

tractorcow · 2017-08-03T03:20:36Z

That's fine @johndalangin, thanks for the great feature!

johndalangin · 2017-08-03T07:01:36Z

@tractorcow No worries! It's my pleasure to contribute. We've been working with Silverstripe for a couple of years now and we'd love to contribute every change we'd get.

The requested revisions have been pushed. Feel free to take a look and let me know if you have any more comments.

tractorcow · 2017-08-03T09:28:38Z

I'm happy. @dhensby you have any thoughts or shall we just merge it?

johndalangin · 2017-08-03T10:13:44Z

@tractorcow @dhensby Thanks for your feedback! I'll work on the MySQLi connector integration in a separate issue. Do you have a release schedule for the next minor version? I can work on the SSL MySQLi integration by next week but if you need it faster, I can adjust my schedule so we can release both simultaneously.

I'll also do the documentation in a separate issue which will most likely affect Security, Models and Databases, and Configuration.

Let me know if you have concerns.

dhensby · 2017-08-03T10:20:43Z

I'm happy

johndalangin mentioned this pull request Aug 2, 2017

SSL capability for PDO MySQL driver #7077

Closed

dhensby reviewed Aug 2, 2017

View reviewed changes

dhensby requested a review from tractorcow August 2, 2017 14:29

tractorcow suggested changes Aug 2, 2017

View reviewed changes

Refactor code to decouple SSL feature from MySQL PDO

a3678c7

tractorcow approved these changes Aug 3, 2017

View reviewed changes

dhensby merged commit 723ae37 into silverstripe:3 Aug 3, 2017

johndalangin mentioned this pull request Aug 3, 2017

Add SSL support for MySQLi connector #7242

Closed

johndalangin deleted the 7077-add-mysql-pdo-ssl branch August 3, 2017 10:41

johndalangin mentioned this pull request Aug 7, 2017

Add documentation for database SSL feature #7253

Closed

lerni mentioned this pull request Mar 20, 2019

MySQL SSL Support - gone? #8871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Added MySQL SSL PDO Support (fixes #7077) #7233

NEW Added MySQL SSL PDO Support (fixes #7077) #7233

johndalangin commented Aug 2, 2017

johndalangin commented Aug 2, 2017

dhensby left a comment

dhensby Aug 2, 2017

johndalangin Aug 2, 2017

dhensby commented Aug 2, 2017

johndalangin commented Aug 2, 2017

tractorcow Aug 2, 2017

johndalangin Aug 3, 2017

tractorcow Aug 2, 2017

johndalangin Aug 3, 2017

tractorcow Aug 2, 2017

johndalangin Aug 3, 2017

tractorcow Aug 2, 2017

johndalangin Aug 3, 2017

johndalangin commented Aug 3, 2017

tractorcow commented Aug 3, 2017

johndalangin commented Aug 3, 2017

tractorcow commented Aug 3, 2017

johndalangin commented Aug 3, 2017 •

edited

Loading

dhensby commented Aug 3, 2017

NEW Added MySQL SSL PDO Support (fixes #7077) #7233

NEW Added MySQL SSL PDO Support (fixes #7077) #7233

Conversation

johndalangin commented Aug 2, 2017

johndalangin commented Aug 2, 2017

dhensby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Aug 2, 2017

johndalangin commented Aug 2, 2017

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

johndalangin commented Aug 3, 2017

tractorcow commented Aug 3, 2017

johndalangin commented Aug 3, 2017

tractorcow commented Aug 3, 2017

johndalangin commented Aug 3, 2017 • edited Loading

dhensby commented Aug 3, 2017

johndalangin commented Aug 3, 2017 •

edited

Loading