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

Fix MySQLi SSL flags handling to explicitly set MYSQLI_CLIENT_SSL if … #169

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

EreMaijala
Copy link
Contributor

…use_ssl is set.

Signed-off-by: Ere Maijala ere.maijala@helsinki.fi

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes

Description

This fixes an issue where setting use_ssl to true alone doesn't actually make the connection secure. It is only made secure if any of the client_key, client_cert, ca_cert, ca_path or cipher is also set to a non-empty value.

To reproduce the problem prior to this fix do something like this:

use Laminas\Db\Adapter\Adapter;
$adapter = new Adapter(
    [
        'driver' => 'mysqli',
        'hostname' => 'localhost',
        'username' => 'user',
        'password' => 'password',
        'database' => 'test',
        'use_ssl' => true
    ]
);
$statement = $adapter->createStatement("SHOW SESSION STATUS LIKE 'Ssl_cipher'");
foreach ($statement->execute() as $res) {
    print_r($res);
}

Expected: a non-empty value for Ssl_cipher
Actual: Ssl_cipher is empty
Tested with: PHP 7.2.20

The patch moves mysqli class creation in the Connection class to a helper method to make it easier to test the class and adds a couple of tests that verify the connection parameters using mock objects. The failed connection test is moved last since it sets connect_error property in mysqli, and I couldn't find a simple way to mock or override it later on (trying to mock the __get method causes a warning with PHPUnit).

@kris-sum
Copy link

This will close #53

@weierophinney weierophinney added the Bug Something isn't working label Feb 19, 2021
@weierophinney weierophinney added this to the 2.11.4 milestone Feb 19, 2021
@weierophinney weierophinney changed the base branch from master to 2.11.x February 20, 2021 18:45
…use_ssl is set.

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants