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

Update Tracker DB config to support SSL #12631

Merged
merged 4 commits into from Apr 2, 2018
Merged

Conversation

fitzoh
Copy link
Contributor

@fitzoh fitzoh commented Mar 21, 2018

This might just handle the PDO adapter, does matomo/core/Tracker/Db/Mysqli.php also need to be modified?

refs #10866

@mattab
Copy link
Member

mattab commented Mar 21, 2018

Hi @fitzoh
Thanks for the PR! Yes, Mysqli also needs to work with SSL as it makes it easier to document the feature when we have support across the board 👍

@mattab mattab added this to the 3.4.0 milestone Mar 21, 2018
@mattab mattab added the Needs Review PRs that need a code review label Mar 21, 2018
@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 21, 2018

I think that should do it

@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 21, 2018

And just out of curiosity, is there a reason for having different connections for core and tracker?
Feels like there's a lot of copy paste between the two

@mattab
Copy link
Member

mattab commented Mar 21, 2018

Initially it was for performance reasons, but this is not a valid reason anymore, so now it is actually technical debt and ultimately we would like to refactor both to use the same code.

@diosmosis
Copy link
Member

hi @fitzoh can you recreate this PR in a separate branch instead of using the 3.x-dev branch? I can't pull these changes down locally to test.

Also, I haven't tested (since I can't pull down the code), but I don't believe your changes to the MySQLi adapter will work. $mysqlOptions doesn't exist in the MySQLi , the array isn't used & PDO options don't have meaning to MySQLi. Can you fix this?

Also, I added another option for self-signed certs, ssl_no_verify, in f2d5f49. Would you be able to replicate those changes here?

@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 22, 2018

Sure @diosmosis , might be able to do that tonight, probably some time this weekend (No idea what I'm doing in PHP land).

@diosmosis
Copy link
Member

diosmosis commented Mar 22, 2018

No worries, happy to answer any questions (even over the weekend), the MySQLi changes might be a pain, took me a while to figure out some things while fixing up the other PR.

You'll want to replicate what's happening here: https://github.com/matomo-org/matomo/pull/10866/files
in the libs/Zend/Db/Adapter/Mysqli.php file based on the config options.

@mattab mattab modified the milestones: 3.4.0, 3.4.1 Mar 22, 2018
@fitzoh fitzoh force-pushed the 3.x-dev branch 4 times, most recently from 9df91bf to 32828c5 Compare March 24, 2018 02:02
@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 24, 2018

@diosmosis does that look about right?
Also, what exactly did you have in mind for branches?
Did you just want me to change the branch the PR is targeting?

@diosmosis
Copy link
Member

@fitzoh that looks pretty good, I’ll test it and let you know tomorrow.

Don’t worry about the branch, I should be able to pull down the changes without that, enjoy your weekend!

$this->ssl_ca = $dbInfo['ssl_ca'];
$this->ssl_ca_path = $dbInfo['ssl_ca_path'];
$this->ssl_cipher = $dbInfo['ssl_cipher'];
$this->ssl_no_verify = $dbInfo['ssl_no_verify'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$dbInfos may not have all of these entries since some are optional, so we'll have to surround each line in a !empty() if like:

if (!empty($dbInfo['enable_ssl'])) {
    $this->enable_ssl = $dbInfo['enable_ssl'];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -75,11 +94,22 @@ public function connect()

$this->connection = mysqli_init();


if($this->enable_ssl){
$this->connection.ssl_init($this->ssl_key, $this->ssl_cert, $this->ssl_ca, $this->ssl_ca_path, $this->ssl_cipher);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssl_init isn't a method on connection, instead you have to call mysqli_ssl_set() like so:

mysqli_ssl_set($this->connection, $this->ssl_key, ... etc. ...);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, definitely should have been set, not init...
Would connection.ssl_set work?
Or is the procedural way preferred?
http://php.net/manual/en/mysqli.ssl-set.php

Object oriented style

bool mysqli::ssl_set ( string $key , string $cert , string $ca , string $capath , string $cipher )

Procedural style

bool mysqli_ssl_set ( mysqli $link , string $key , string $cert , string $ca , string $capath , string $cipher )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in PHP, the . operator is actually the string concatenation operator. For member access, you'd have to use ->.

Anyway, just tried it w/ ->ssl_set(, and it works, so 👍 if you want to use that.

if (!empty($dbInfo['ssl_cipher'])) {
$this->mysqlOptions[PDO::MYSQL_ATTR_SSL_CIPHER] = $dbInfo['ssl_cipher'];
}
if (!empty($config['ssl_no_verify']) && defined('PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$config is the wrong variable, should be $dbInfo.

@diosmosis
Copy link
Member

diosmosis commented Mar 24, 2018

@fitzoh 👍 for the changes, had to make a couple extra changes to get this to work locally, if you can apply them, then it should be good to go. Feel free to let me know if you're unable to make them (do to lack of time or something).

// Make sure MySQL returns all matched rows on update queries including
// rows that actually didn't have to be updated because the values didn't
// change. This matches common behaviour among other database systems.
// See #6296 why this is important in tracker
$flags = MYSQLI_CLIENT_FOUND_ROWS;
if ($this ->enable_ssl){
Copy link
Member

@diosmosis diosmosis Mar 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor issue, but noticed a typo here (w/ the space after $this).

@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 24, 2018

updated @diosmosis

@diosmosis
Copy link
Member

Tests are failing since it's missing the commits from #10866, otherwise the PR looks good and works for me.

@diosmosis diosmosis added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 25, 2018
@diosmosis diosmosis merged commit 5732af8 into matomo-org:3.x-dev Apr 2, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Update Tracker DB config to support SSL

* Add SSL option to tracker Mysqli config

* pr feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants