@ghost opened this Pull Request on June 6th 2015

Hello,
My first ever open source pull request I am beginner but I use Piwik in work and want to make it even better, so please be nice but say if I did it wrong and how I should fix it.

I added the support of ssl for mysqli Adapter. From my point the needed options are ssl-ca and ssl-capath but this can vary on different platforms.

It would be nice to note that mysqli will silently fall back to non ssl connection if server don't support ssl.

Pls. let me know if this will be good, if yes i can think about adding options for sql settings to installer.

MYSQLI_CLIENT_SSL in mysqli_real_connect is completely optional but would be nice to have it in case there will be some changes in mysqli.

fixes #7039

@mattab commented on June 7th 2015 Owner

Hi @ktroska and thanks for this nice Pull request!

Feedback:

  • can you add comments in the global.ini.php file to document "example values" for each of the ssl* settings? It's nice to show user what values are expected for each setting.
  • it would be nice to add SSL support in PDO Mysql as well as Mysqli (as we officially support both Mysqli and PDO)
  • maybe we can add a unit or integration test for this? not sure if it's easily do-able.
    • With such a test the advantage is that we would detect whenever the SSL support stops working in the future. Also if we have such a test, it will automatically run for both Mysqli and PDO as our tests in CI server run in both configurations.
  • to let the community know about this feature and how to use it, we will need to write a little FAQ "How do I configure Piwik to connect to Mysql database over secure SSL connection?"
@ghost commented on June 8th 2015

Hi @mattab Thx for feedback.

can you add comments in the global.ini.php

Something like this:

; Turn on or off SSL connection possible values for use_ssl: true or false
use_ssl = false
; Direct path to CA cert file, CA bundle supported (required for ssl connection)
ssl_ca =
; Direct path to client cert file (optional)
ssl_cert =
; Direct path to client key file (optional)
ssl_key =
; Direct path to CA cert files directory (optional)
ssl_ca_path =
; List of one or more ciphers for SSL encryption, in OpenSSL format (optional)
ssl_cipher =

I think something like that? Notice I think of adding underscores for all options for better visibility in both options and code.
Note: Requires PHP 5.3.7 http://php.net/manual/en/ref.pdo-mysql.php#pdo-mysql.constants

support in PDO Mysql

code is done and tested I will merge it when we decide about global.ini.php content and underscores

unit or integration test

really first time I see this I will take a look and let you know if I can manage this, but since I started I want to make it to the end :)

FAQ

I am thinking of integrating this into installer like SSL on and upload a ssl cert, the problem is there is no (i can think about) safe way to upload and store client key file if it is required, Or just FAQ info like this:

To Enable secure connection with MySQL over ssl connection please add to file config/config.ini.php under [database] section:

NOTE: Please not that if server is not supporting SSL, this function will **silently** fails to plain text connection. With this in mind this option can save you from sniffing packets, but **can't** be used as MITM protection (link to wikipedia? http://en.wikipedia.org/wiki/Man-in-the-middle_attack ).

Also we can do a test to check ssl support in connection like:
SHOW STATUS LIKE 'Ssl_cipher';
!empty( Value )
Its still not protecting from mitm so I think its no use.

@mattab commented on June 9th 2015 Owner

Another feedback: we should report to the user whether the secure connection works or not:

  • when SSL connection to DB works, in the system check (screenshot) we can display "Secure connection to database" with a green tick.
  • when a user has setup the SSL config settings, but somehow the SSL connection does not work (ie. one of the settings is wrong or the server SSL config is wrong), then ideally we want to detect and let user know. In the system check we would write "Warning: Secure connection could not be established!" next to "Secure connection to database".
  • if a user has not specified any the ssl_* INI settings, then user does not care about secure connection and the system check should not display the row "Secure connection to database".
@mattab commented on June 9th 2015 Owner

I think something like that?

it is easier to read :+1:

code is done and tested I will merge it when we decide about global.ini.php content and underscores

sounds good

really first time I see this I will take a look and let you know if I can manage this, but since I started I want to make it to the end :)

Great. Automated testing is a powerful amazing concept for software development :-) Some pointers

I am thinking of integrating this into installer like SSL on and upload a ssl cert

thanks for suggestion but it's not needed. FAQ will be good enough to document this feature.
We could also add a link to the FAQ from: http://piwik.org/docs/how-to-secure-piwik/

Also we can do a test to check ssl support in connection like:
SHOW STATUS LIKE 'Ssl_cipher';
!empty( Value )
Its still not protecting from mitm so I think its no use.

It's still useful if Piwik lets user know whether the connection is done over SSL. See my previous comment about System check.

Finally:

  • in your PR you modify the libs/ Zend_Db files. We don't usually modify core lib files. I'm not sure how we should handle this situation. Maybe latest version of Zend_Db has support for SSL directly? Or do we maybe really need to modify the Zend_Db code itself?
@ghost commented on June 9th 2015

Finally:

Well since I did mysqli first i did not notice that Pdo can be easily done in piwik/core/Db/Adapter/Pdo/Mysql.php. So no Zend modification for this.

If it comes to Mysqli.... we need to call mysqli_ssl_set() between mysqli_init() and @mysqli_real_connect()
And in zend there is only mysqli_options() which won't help us:
https://github.com/piwik/piwik/blob/master/libs/Zend/Db/Adapter/Mysqli.php#L300
same upstream:
https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Mysqli/Connection.php#L115
Maybe there is something I missed.
If there is no way I will think of changing the code to be compatible with Zend style and more object oriented and push it upstream to Zend meanwhile we will have it working in Piwik.

system check

I started working on it based on LoadDataInfileCheck.php after work and some sleep I think I will have it ready.

Test

I got general idea now to coding.

I can't thank you enough for sticking with me. :+1: But Thank you.

@ghost commented on June 10th 2015

I am now working on the automated testing, I will try to make it tomorrow (European time) but i am not sure if I will make it.
I think that all other things are done and ready.
Maybe I missed something, but I have tested it in my home lab and it looks ok. please confirm.

@mattab commented on June 11th 2015 Owner

@ktroska it looks good. I left more comments. once you manage to add the automated test, it should be ready (or almost)!

@ghost commented on June 12th 2015

I have done test but i am not sure if it is ok, it was sure fun to learn something new and usefull.

Test:
Main problem is that we need to 'use PDO' which fails if it not exists and this way the whole test will fail badly, but if PDO is used all other things are tested ( like if used constants exists and if they values fits the ones used in code).
Also mysql test if there is a way to turn on ssl in mysql, since if the value would be NO (other than yes and disabled will be unexpected and test will fail because its not expected also in code) then you need to recompile mysql, test will fail but piwik will work just there will be no(?) way to connect to mysql through SSL.

@mattab commented on June 16th 2015 Owner

@ktroska as you can see in the Travis CI output for the job, the tests are failing: https://travis-ci.org/piwik/piwik/jobs/66525061

Here is how the test should work:

  • the goal is to test that when the ssl_* config are set, then the Mysql connection over SSL works
  • to test for this you need, in your test, to set those config flags before connecting to the database with Db::createDatabaseObject for example:
        $dbConfig = Config::getInstance()->database;
        $dbConfig['ssl_xxxx'] = xyz;
        Db::createDatabaseObject($dbConfig);
  • after connection is successful, you can test that the mysql uses SSL, ideally by re-using the logic in DbOverSSLCheck diagnostic and asserting that SSL connection should be used.

Result:

  • if connection to DB fails, test will fail
  • if connection to DB is not done over SSL, test will fail
  • if anything else errors, test will fail
  • if DB connection works and SSL is used, then test will pass

As mentioned our CI runs on both PDO and Mysqli so this test(s) will automatically run on both.

@ghost commented on June 17th 2015

I have learned one thing from earlier testes, that constants in PDO have different values depending on build we have. So I changed the code to only use PDO constants or set values (those 1010-1014) only when using mysqli in case we don't have PDO support.

Those values work on my system and they can fail on any others so we can make them very high or change them to something different.

Since we are using SSL based constants from PDO this will require PHP 5.3.7 ( http://php.net/manual/en/ref.pdo-mysql.php#pdo-mysql.constants ) this can't(?) be worked around when using PDO driver.

I wanted to do mysqli call easier and PDO compatible that's why I am using PDO constants values for mysqli, they can be changed to any other but it needs to be done in 3 places if you think its a good way changing it to some specially set values not PDO compatible than I can change it quickly.

  • core/Db/Adapter/Mysqli.php 1x
  • libs/Zend/Db/Adapter/Mysqli.php 2x
@ghost commented on June 18th 2015

After some thinking:

  • I will change the mysqli settings to not depend on PDO constants - use [driver_option][ssl_key] etc.
  • change the enable_ssl to 1 in config to test the ssl calls in travis.
  • remove require PHP version from tests.
    • split into one for mysqli, and other for mysql (add test for defined const - not for values since i know they are different and i dont need them)
@mattab commented on September 11th 2015 Owner

Hi @ktroska

We are still interested to have Mysql and SSL support in Piwik and this PR is very promising. What work is left to do in your opinion? if you need some help feel free to ask. Looking forward to having this one merged sometime :-)

@mattab commented on February 1st 2016 Owner

@ktroska it would be possible to merge this Pull request in Piwik 3.0.0 - let us know if you are still interested :+1:

@mattab commented on September 23rd 2016 Owner

Closing the pull request as the work has stalled - it would be awesome if someone would finish this nice work!

@RafalLukawiecki commented on November 9th 2017

Apologies for asking a question on a closed PR, but could @mattab (or someone else) clarify if Piwik 3.2.0 supports secure connections to its MySQL database? If so, what are the SSL (or otherwise) configuration parameters, please?

@sgiehl commented on November 9th 2017 Member
@mattab commented on November 9th 2017 Owner

@RafalLukawiecki what we need is support from a developer in the community to finish this PR. we can re-open the PR if anyone will continue work on it :+1:

@PiwikForumBot commented on November 9th 2017

This pull request has been mentioned on Piwik forums. There might be relevant details there:

https://forum.piwik.org/t/force-mysql-connection-to-use-ssl/9447/3

This Pull Request was closed on September 23rd 2016
Powered by GitHub Issue Mirror