@adambaratz @madorin is this reasonable ?

@ghfjdksl this doesn't appear to have satisfactory tests included ?

No, I'm not familiar with php's test system, but I think I can take some time to copy and modify an existing one.
Is there any other regulation I should follow?

@ghfjdksl we have an article on the QA website for how test cases work: http://qa.php.net/write-test.php

But to answer your question, yes you can easily copy/paste and replace the values within a file and make it work. We have another small note about how to run the tests here: http://qa.php.net/running-tests.php

Happy hacking!

Hmm, after some digging, things turned out to be not so simple.

1. To correctly test the feature of this PR, SSL/TLS connection to a mysql server is required.
2. I can't find a test regarding the original SSL connection feature which I planned to copy, I mean those testing MYSQL_ATTR_SSL_CA and such.
3. If I understand correctly, other tests connects to a local mysql server on the travis server. If I'm to do the same, the mysql server needs to be properly configured to accept SSL connection.
4. If the mysql server is properly configured, I need at least the CA file on the travis server to do a SSL connection using PDO.
5. And I also need to know the certificate CN to test the feature. It can be either hard-coded, which will break every time the certificate changed; or reads the value on the fly, which will need some extra dependancy to parse the certificate.
6. The test will not run on users' machine, and skipped most of the time.

tl;dr

I'd like to know the ssl setting of the mysql server for test on the travis machine.

Judging by this issue MySQL on Travis currently doesn't have an SSL setup. Looks like testing this change would be more effort than its worth, so it's fine to not have one in this case.

ACK, no test required then ...

@ghfjdksl still I'd like to hear a response/see a solution to the mysql_ssl_set problem.

I think SSL connection parameters should be at PDO base class level, not just especially tuned/named for MySQL as soon as other DBMSes supports SSL connections (Oracle, PostgreSQL) or others support it by plugins (Firebird).
Common SSL settings for all DBMSes will avoid code fragmentation, the main goal of PDO itself.

@madorin these are good points ... @ghfjdksl thoughts on those points ?

Hmm, not very sure I get the point. The ideal is to have a common interface, and I should export the variable up to that point? Or the common interface will be less likely to have such a specific setting, so it should not be included?

@ghfjdksl, at least to have a set of common PDO constants/flags like PDO::SSL_* (PDO::SSL_MODE, PDO::SSL_CERTIFICATE, your PDO::SSL_CNAME) or a set of common agreed/documented parameters specified in DSN. This is a subject of an RFC.

I agree with that, but that's another topic. There is no such common constants yet (or I missed something there) and that's not my fault. And as a outsider I don't know about the schedule.
Just like the only way for PDO to SSL connect to mysql is to set MYSQL_ATTR_SSL_CA, which ideally should be something like PDO::SSL_CA. I totally agree, but I'm not doing that.

@ghfjdksl it's difficult to merge this, and then merge the common constants that you seem to agree are better ...

@madorin any interest in drafting/implementing that RFC ?

Given that all the other SSL options are MySQL specific (MYSQL_ATTR_SSL_*), I don't think it's reasonable to require a generalized implementation as part of this change. Providing generic options seems like an orthogonal future improvement to me.

@nikic okay, so (to be clear) you'd be in favour of just merging this and then attempting a generalized interface for PDO SSL later on ?

Reasonable I guess, but there only seems to be a few ATTR_SSL options (used here), and they seem to be generally applicable, mode, cn, cert, am I missing something ?

@madorin no problem, just thought I'd ask ... I'll leave you alone to do fb stuff :)

@nikic okay, so (to be clear) you'd be in favour of just merging this and then attempting a generalized interface for PDO SSL later on ?

Yeah.

Reasonable I guess, but there only seems to be a few ATTR_SSL options (used here), and they seem to be generally applicable, mode, cn, cert, am I missing something ?

They may be generally applicable, but we'd also have to implement support in the respective drivers for a generic option to make sense. That would require a concerted effort by a number of people.

I'd also prefer leaving the PDO options as driver-specific, for basically the reasons stated by @nikic.

I'm totally on board with that, if it's the best thing to do 👍

It is a subject for a driver implementation to add SSL support or not, anyway it is optional. Even adding them in a common space will motivate maintainers to add SSL support for their DBMS.
At least in PostgreSQL and Oracle, according to their documentation, client can bring a certificate on connection establishment. From this POV I see some benefits, although I'm not an expert in security. Just my two cents :)

Strange, I have fixed the problem of libmysql by using compile time flag. But some irrelevant test failed in travis(ext/gd/tests/imagebmp_basic.phpt, I don't even have that file). I have tried to revert to the commit back in May, and it still failed. Also appveyor seems to refuse to recognize my #ifdef. Does anyone have an idea about this? Some breaking change about the test in the past few months?

@ghfjdksl Don't worry about the test failure on travis ... there are some intermittent failures. The failure on appveyor however looks genuine, but looking at the code I don't understand what's wrong :/

Two general notes: I don't think we should modify the signature of set_ssl, even if mysqlnd is used. The signatures of the emulated libmysql functions should be preserved. Instead a separate function can be added to the mysqlnd API to set the ssl CN.

However... while thinking about this I noticed another issue: Even if we don't modify the set_ssl signature, this change requires at least a change to the st_mysqlnd_vio_options struct. This struct is part of an exported header and as such subject to ABI stability guarantees. This means that we cannot apply this change to PHP 7.0 and PHP 7.1, only to master.

This is fine per se, but we probably still want to have some solution to the problem in 7.0. Probably we should land something similar to the patch proposed on https://bugs.php.net/bug.php?id=71003 to allow disabling certificate validation. It's not the ideal solution for cases where we just have a CN mismatch, but better than nothing.

I think I can make the CN attribute transfer in a independent way. But judging from the later part of comment, it looks like you want a DONT_VERIFY_CERT more. Then I think I should halt the work for now as there will be less demand for this...

Could someone explain the status of this PR? Even if it was more easily tested (and the tests passed) would it solve 71845 or is just a workaround? Meaning, if this was tested and merged, would MySQL SSL be fixed for PDO?

This PR can solve 71845, but according to nikic's comment, it cannot be merged into 7.0 and 7.1 since it will change a exported struct. So a CLIENT_SSL_DONT_VERIFY_SERVER_CERT solution is favored. And since such a solution will also solve this problem, I'm halting the work now unless new demand on this subject appeared.

At this point is there any way of disabling certificate validation on php7? For example using other libraries other than mysqli/PDO? How can someone using PHP 7 and mysql connect with a certificate that has a CN that can not be validated? I understand that CLIENT_SSL_DONT_VERIFY_SERVER_CERT is preferred but is there current work being done to port/implement this on php7? If not, why not merge the current code to satisfy the need and then add CLIENT_SSL_DONT_VERIFY_SERVER_CERT.

mysqli already have MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT flag.
http://php.net/manual/en/mysqli.real-connect.php
You can skip ssl verification.

I've just merged the patch from bug #71003 into PHP 7.0+ (247ce05), which adds a PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT option. I'd appreciate if someone with the required setup can confirm that this solves the problem.

Hi @nikic ,
I checked out your fix in a debian jessie docker container and it seems to work.

The code I used to test connects to a google cloud sql database:

<?php

$pdo = new PDO("mysql:host=***.***.***.***;dbname=test;charset=utf8;", "test", "**********", [
  PDO::MYSQL_ATTR_SSL_KEY => '/var/phptest/mysql-client-key.pem',
  PDO::MYSQL_ATTR_SSL_CERT => '/var/phptest/mysql-client-cert.pem',
  PDO::MYSQL_ATTR_SSL_CA => '/var/phptest/mysql-server-ca.pem',
]);
$statement = $pdo->prepare('SELECT * FROM test');
$statement->execute();
$result = $statement->fetchAll();
var_dump($result);

The initial error I got from 7.1.2 stable build was:

PHP Fatal error:  Uncaught PDOException: PDO::__construct(): Peer certificate CN=`esoteric-stream-****:************' did not match expected CN=`***.***.***.***' in /var/phptest/test.php:4

I then checked out the 7.1.4 build from github and tried it without the new constant:

PHP Fatal error:  Uncaught PDOException: PDO::__construct(): Peer certificate CN=`esoteric-stream-****:************' did not match expected CN=`***.***.***.***' in /var/phptest/test.php:4

Still the same error, I then added the new constant like so, and the connection got through without any problems.

$pdo = new PDO("mysql:host=***.***.***.***;dbname=test;charset=utf8;", "test", "************", [
  PDO::MYSQL_ATTR_SSL_KEY => '/var/phptest/mysql-client-key.pem',
  PDO::MYSQL_ATTR_SSL_CERT => '/var/phptest/mysql-client-cert.pem',
  PDO::MYSQL_ATTR_SSL_CA => '/var/phptest/mysql-server-ca.pem',
  PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT => false
]);

If you need any additional information about the setup please let me know, I really would appreciate it if this fix goes into 7.1.4 :)

@nikic Just checked on 7.0.18 and is working well.

Thank you!

Does this completely turn off verification of the host cert, or does it just disable checking the CN?

I think its not good implements, this patches just check CN.

Hi, as there is a PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT now and there's less demand for a check-specific-CN feature, I decided to close this PR. Thank you.