Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MySQL SSL Support - gone? #8871

Closed
extravio opened this issue Mar 20, 2019 · 17 comments
Closed

MySQL SSL Support - gone? #8871

extravio opened this issue Mar 20, 2019 · 17 comments

Comments

@extravio
Copy link

extravio commented Mar 20, 2019

According to the docs
https://docs.silverstripe.org/en/4/getting_started/installation/how_to/mysql_ssl_support/
https://docs.silverstripe.org/en/4/getting_started/environment_management/
the path of SSL certs, keys, ca can be defined in the environment variables SS_DATABASE_SSL_KEY/CERT/CA

These variables have disappeared from the framework code.
Can someone explain how to configure SSL and update the docs?

see issue #7077
Edit: on the latest version at the time 4.3.3

Update:
We should either reinstate this functionality (preferred) or remove the dead code (see #10729) - but the current state is not correct.

PRs

@lerni
Copy link
Contributor

lerni commented Mar 20, 2019

I've done it a while ago with 3.x, there you need 3.7 - it's not in 3.6. Probable it's similar with 4.x - are you on a recent version?
Edit: I've checked with 4.3.3 and it looks as you've said there isn't anything but there is in 3.7 :(

@extravio
Copy link
Author

@lerni I forgot to mention that I'm using the latest 4.3.3 version

@lerni
Copy link
Contributor

lerni commented Mar 20, 2019

I've needed to fork 3.6 to make this work and it took me a while... I remember MySQLi gave better errors than PDO (this was the ugly part). Even tough once the key-setup was right, it also worked with PDO. I do not see trough if and how upmerging should have happened - at least there is some documentation, to know about it ;) I wonder how straight this now could go up into 4.x ?
#7233
#7243
#7254
#7259

@maxime-rainville
Copy link
Contributor

Maybe removing SSL support was a concious decision and we just forgot to update the doc ... or maybe it got removed by accident. There's still reference to SSL in PDOConnector and MySQLiConnector.

We'll have to investigate.

If you want to bring it back now, you might be able to define your own connector and use the injector to override it. I haven't tried this, so I don't know if it will work. But if it does, you should be able to use your SSL DB connection without forking framework.

Step one - define you own Connector

<?php
namespace YourProject;

use SilverStripe\ORM\Connect\MySQLiConnector;
use SilverStripe\Core\Environment;

class CustomConnector extends MySQLiConnector # OR PDOConnector if using PDO
{

    public function connect($parameters, $selectDB = false)
    {
        $envMap = [
            'SS_DATABASE_SSL_KEY' => 'ssl_key',
            'SS_DATABASE_SSL_CERT' => 'ssl_cert',
            'SS_DATABASE_SSL_CA' => 'ssl_ca',
        ];
        
        // Loop through the list of possible SSL DB environment variables
        foreach ($envMap as $envKey => $paramKey) {
            if ($val = Environment::getEnv($envKey)) {
                $parameters[$paramKey] = $val;
            }
        }
        
        return parent::connect($parameters, $selectDB);
    }

}

Step two - Inject CustomConnector in your YML config

Add something like this in your YML config

SilverStripe\Core\Injector\Injector:
  MySQLiConnector: # Or PDOConnector
    class: 'YourProject\CustomConnector'
    type: prototype

Step three - Define SSL environment variable

Add your SSL settings in your .env file.

SS_DATABASE_SSL_KEY='/home/newdrafts/mysqlssltest/client-key.pem'
SS_DATABASE_SSL_CERT='/home/newdrafts/mysqlssltest/client-cert.pem'
SS_DATABASE_SSL_CA='/home/newdrafts/mysqlssltest/ca-cert.pem'

@maxime-rainville
Copy link
Contributor

@chillu I'm setting this has an [impact/high] because it's precluding a valid use case and it doesn't look like it was remove on purpose.

@obj63mc
Copy link
Contributor

obj63mc commented Mar 22, 2019

I started following this issue as I had tried to get an SSL connection in v4.x working in the past but couldn't. @maxime-rainville - I tried the injector method but it never seemed to use my custom class.

Example from testing install of 4.3.3-

	---
	Name: app
	---
	SilverStripe\Core\Manifest\ModuleManifest:
	  project: app
	SilverStripe\Control\Director:
	  alternate_base_url: '`SS_BASE_URL`'
	SilverStripe\Forms\HTMLEditor\TinyMCEConfig:
	  editor_css:
	    - 'themes/mytheme/css/editor.css'
	SilverStripe\ErrorPage\ErrorPage:
	      enable_static_file: false
	SilverStripe\Core\Injector\Injector:
	  SilverStripe\ORM\Connect\MySQLiConnector:
	    class: 'MySite\Connectors\MySQLiSSLConnector'
	    type: prototype

The other thing I wanted to mention though is right now the current MySQLiConnector class only sets ssl if you have the cert, key and CA.

    // Set SSL parameters if they exist. All parameters are required.
    if (array_key_exists('ssl_key', $parameters) &&
        array_key_exists('ssl_cert', $parameters) &&
        array_key_exists('ssl_ca', $parameters)) {

If you are using a DB like Amazon RDS though or from a provider like https://www.compose.com/, you will only get the CA pem file. So you would connect with the following as an example -

    $this->dbConn->ssl_set(NULL, NULL, '/rds-combined-ca-bundle.pem', NULL, NULL);

I have tested this locally by editing the MySQLiConnector class directly since I couldn't get the injector works and was able to verify the connection was using the SSL fine. If/when this is brought back to 4.x+ if we can connect with just the ca bundle and not needing all 3 would be great. Or if there is a way to get the Injector to work overwriting the MySQLiConnector class would work as well too.

@obj63mc
Copy link
Contributor

obj63mc commented Mar 22, 2019

I was able to find a way to use a new custom connector class - For your example above (CustomConnector Class) - you can set the following in your yml config -

---
Name: app
After:
  - '#databaseconnectors'
---
...
SilverStripe\Core\Injector\Injector:
  MySQLDatabase:
    class: 'SilverStripe\ORM\Connect\MySQLDatabase'
    properties:
      connector: '%$CustomConnector'
      schemaManager: '%$MySQLSchemaManager'
      queryBuilder: '%$MySQLQueryBuilder'
  CustomConnector:
    class: 'YourProject\CustomConnector'
    type: prototype
  MySQLSchemaManager:
    class: 'SilverStripe\ORM\Connect\MySQLSchemaManager'
  MySQLQueryBuilder:
    class: 'SilverStripe\ORM\Connect\MySQLQueryBuilder'

@chillu
Copy link
Member

chillu commented Mar 26, 2019

SSL support has been introduced through ConfigureFromEnv.php and in the installer.

From what I can tell, it was broken in the installer through 806ffb9#diff-91804554bb2fa0c2383ed0534250cdfcL143.

And then broken by removing ConfigureFromEnv.php without an appropriate replacement in 4.0.0-beta1.

You could add it back in CoreKernel->getDatabaseConfig(), and then ensure that the drivers pick up on this config?

@unclecheese unclecheese mentioned this issue Jun 4, 2019
17 tasks
@elliot-sawyer
Copy link
Contributor

I've turned the suggested fix into a module if anyone wants to try it out: https://github.com/elliot-sawyer/silverstripe-mysql-ssl. I've tested it out on a few different sites using Ubuntu and OSX. There's an optional environmentcheck included if you want to verify the SSL connection is working.

Run composer require elliotsawyer/silverstripe-mysql-ssl and see the README for configuration details.

Cheers to @maxime-rainville and @obj63mc for the initial sample code

@chillu
Copy link
Member

chillu commented Jan 29, 2020

Thanks @elliot-sawyer! Is there a reason this exists as a module rather than a pull request to framework? :D

@elliot-sawyer
Copy link
Contributor

@chillu, I built it as a module rather than a pull request so people can opt-in to the feature if they want to use it. It specifically uses MySQLi instead of PDO and I don't know enough about either to unilaterally make that decision for every Silverstripe developer. I think it's best left as an "optional" module like siteconfig and versioned.

@chillu
Copy link
Member

chillu commented Jan 31, 2020

I thought the opt-in was implied by setting those environment variables? I don't see the value in a separate adapter for this configuration variation, as opposed to logic branches in the same adapter.

Good point about MySQLi vs. PDO, although I'd be happy with a PR that just fixed this for MySQLi, and clearly declared that limitation in the docs.

@elliot-sawyer
Copy link
Contributor

If you don't see the value in a separate adapter to add a secured database connection, that's fine, nobody is forcing you to use it. But I think the community would appreciate having a configurable option that works right now, rather than waiting for some future version of SilverStripe that might or might not include it.

Pull requests on framework don't get merged promptly. In my experience they sit for months or years at a time, requiring me to rebase and maintain unsupported forks for nobody's benefit. Building a module first avoids all that overhead.

@elliot-sawyer
Copy link
Contributor

I also want to point out a PR here would be unsuccessful because nobody has described what the expected fix is - it still isn't clear from the comments here whether or not this removal was intentional.

  • If I make a PR to add it, will you ask me to remove it?
  • If I make a PR to remove it, will you ask me to add it?

I can make a good guess that it wasn't intentional, but some more clarity from the maintainers would have been great. What if I'm incorrect, and the maintainers actually want to finish the removal? I just don't know, and that's why this module exists.

That said, there are some other PR's I'd like to raise on MySQLiConnector that have arisen as a result of this work. I'll create new issues (and PRs!) for those

@chillu
Copy link
Member

chillu commented Feb 2, 2020

Based on my research, I don't see how SSL support would've been explicitly removed, it seems like an accident. This removal would be considered an API change, and in my view anyone with commit/merge access here has enough sense of responsibility to bring that up as an explicit decision rather than hide it away :) Regardless, I think we should make secure defaults easy in our framework. So from my view, if you send a PR to add SSL support into MySQLi that's the right next step.

@HARVS1789UK
Copy link
Contributor

@elliot-sawyer Thank you for putting together the module, I will attempt using that as an interim solution.

I have come across this issue whilst setting up a SilverStripe site in Azure using an 'Azure AppService' and 'Azure Database for MySQL servers' the latter of which enforces SSL connections by default and strongly advises against disabling this feature. I set the relevant environment variables in .env as per the documentation, only to be prompted with "SSL connection is required. Please specify SSL options and retry." which lead me here.

As @chillu has already suggested, I would also be massively in favour of a PR to get this reintroduced/fixed in core, worries me quite a bit that this documented functionality managed to get removed by accident! I'd be very grateful if you have anytime available to implement some of your modules work into a PR.

@emteknetnz
Copy link
Member

Linked PR has been merged and tagged as 4.13.4 and 5.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants