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
Conversation
Hi @fitzoh |
I think that should do it |
And just out of curiosity, is there a reason for having different connections for core and tracker? |
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. |
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. Also, I added another option for self-signed certs, |
Sure @diosmosis , might be able to do that tonight, probably some time this weekend (No idea what I'm doing in PHP land). |
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 |
9df91bf
to
32828c5
Compare
@diosmosis does that look about right? |
@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! |
core/Tracker/Db/Mysqli.php
Outdated
$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']; |
There was a problem hiding this comment.
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'];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
core/Tracker/Db/Mysqli.php
Outdated
@@ -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); |
There was a problem hiding this comment.
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. ...);
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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.
core/Tracker/Db/Pdo/Mysql.php
Outdated
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')) { |
There was a problem hiding this comment.
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
.
@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). |
core/Tracker/Db/Mysqli.php
Outdated
// 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){ |
There was a problem hiding this comment.
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
).
updated @diosmosis |
Tests are failing since it's missing the commits from #10866, otherwise the PR looks good and works for me. |
* Update Tracker DB config to support SSL * Add SSL option to tracker Mysqli config * pr feedback
This might just handle the PDO adapter, does
matomo/core/Tracker/Db/Mysqli.php
also need to be modified?refs #10866