phpList / phplist3

Fully functional Open Source email marketing manager for creating, sending, integrating, and analysing email campaigns and newsletters.
https://www.phplist.org
GNU Affero General Public License v3.0
736 stars 268 forks source link

SSL/TLS MySQL Configuration #833

Open jjthiessen opened 2 years ago

jjthiessen commented 2 years ago

I believe that https://github.com/phpList/phplist3/commit/a3bc7189b8b3d048af3a5c685bcc53358af42046 introduced a regression for MySQL configurations where SSL/TLS is enforced. It is also possible that this behaviour has changed between PHP versions, or is/was different between the use of libmysql and mysqlnd. The PHP Manual seems to suggest that the change was valid and should work; however, this does not seem to be the case in my tests.

$ php -v
PHP 7.4.24 (cli) (built: Sep 21 2021 11:23:23) ( ZTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.24, Copyright (c), by Zend Technologies

$ php -m | grep -i mysql
mysqli
mysqlnd
pdo_mysql

$ cat test.php 
<?php

$db = mysqli_init();

foreach ([
    'MYSQLI_CLIENT_SSL'                                             => MYSQLI_CLIENT_SSL,
    'MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT'                     => MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT,
    'MYSQLI_CLIENT_SSL | MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT' => MYSQLI_CLIENT_SSL | MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT,
] as $option => $flags) {
    printf('%s: ', $option);

    if (!mysqli_real_connect(
        $db,
        getenv('DB_SERVER'),
        getenv('DB_USER'),
        getenv('DB_PASS'),
        'mysql',
        3306,
        null,
        $flags
    )) {
        var_dump(mysqli_connect_errno());
    } else {
        echo "Success!\n";
    }
}

$ php test.php 
MYSQLI_CLIENT_SSL: Success!
MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT: 
Warning: mysqli_real_connect(): (HY000/3159): Connections using insecure transport are prohibited while --require_secure_transport=ON. in /REDACTED/test.php on line 20
int(3159)
MYSQLI_CLIENT_SSL | MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT: Success!
jjthiessen commented 2 years ago

That is, I believe that MYSQLI_CLIENT_SSL is required for the client to advertise SSL capabilities (independently of whether certificates should or shouldn't be verified).

matejzero commented 2 months ago

We just migrated to SSL enabled database and were hit with this issue.

PR https://github.com/phpList/phplist3/pull/834 was never merged. Would the 2nd option from @jjthiessen be acceptable as a start fix for the issue.

I did some more tests. If I have a MySQL server with mandatory SSL connection, I can only connect to the DB if flag MYSQLI_CLIENT_SSL is set. It doesn't work with MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT.

Then I did a test with a MySQL server that supports SSL, but is not forcing it.

I used @jjthiessen script with added sleep and then ran query to check connection status:

SELECT sbt.variable_value AS tls_version,  t2.variable_value AS cipher, 
         processlist_user AS user, processlist_host AS host 
FROM performance_schema.status_by_thread  AS sbt 
   JOIN performance_schema.threads AS t ON t.thread_id = sbt.thread_id 
   JOIN performance_schema.status_by_thread AS t2 ON t2.thread_id = t.thread_id 
WHERE sbt.variable_name = 'Ssl_version' AND t2.variable_name = 'Ssl_cipher' 
ORDER BY tls_version

Given that, I think changing default flag to MYSQLI_CLIENT_SSL won't make anything worse than they already are as right now, switching database_connection_ssl to true doesn't make SSL connection anyway. And since MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT is used for client cert auth and we are not using ssl_set() to set client certs, it would make sense to revert back to MYSQLI_CLIENT_SSL, parameter database_connection_ssl will actually force phplist to use SSL and we'll have a working SSL connection again.

michield commented 2 months ago

834 was automatically closed when I renamed "master" to "main" as is now common in projects. I guess it's still a valid PR. If someone can send a PR that can be verified as working, possibly similar to #834 I'll be happy to merge.

matejzero commented 2 months ago

We are using the MR change in production for the last 10 days without issues.