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

add a attribute to specify CN in PDO(version 7) #1913

Closed
wants to merge 10 commits into from

Conversation

@ghfjdksl
Copy link

commented May 19, 2016

Basically the same thing as #1910 but based on master.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@adambaratz @madorin is this reasonable ?

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

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 9, 2017

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?

@@ -2254,7 +2254,7 @@ PHP_FUNCTION(mysqli_ssl_set)
}
}

mysql_ssl_set(mysql->mysql, ssl_parm[0], ssl_parm[1], ssl_parm[2], ssl_parm[3], ssl_parm[4]);
mysql_ssl_set(mysql->mysql, ssl_parm[0], ssl_parm[1], ssl_parm[2], ssl_parm[3], ssl_parm[4], NULL);

This comment has been minimized.

Copy link
@nikic

nikic Jan 9, 2017

Member

I think this will break the build against libmysql. mysql_ssl_set is a mysql API emulated by mysqlnd, so I don't think it's possible to simply change the signature.

This comment has been minimized.

Copy link
@ghfjdksl

ghfjdksl Jan 9, 2017

Author

Sorry for the late response. I'm thinking about the possibility to use some #ifdef some-compile-time-flag #endif to wrap it. I'm not sure about if there's also a scenario pdo_mysql used with libmysql, or any other similar case. Also I'm not sure about the coding style. Will look into it further tomorrow.

This comment has been minimized.

Copy link
@nikic

nikic Jan 9, 2017

Member

The appropriate flag should be #ifdef PDO_USE_MYSQLND (For the PDO parts that is). It's unfortunate that this would be mysqlnd-only functionality, but I guess there's no way around that.

@KalleZ

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@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!

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 9, 2017

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.

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

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.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

ACK, no test required then ...

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

@madorin

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

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.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

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

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 9, 2017

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?

@madorin

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

@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.

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 9, 2017

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.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@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 ?

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

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.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

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

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@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.

@adambaratz

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

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

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

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

@madorin

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

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 :)

Yun-An Chang

@ghfjdksl ghfjdksl force-pushed the ghfjdksl:add_pdo_server_cn_master branch from 7da025a to 1eb1e4c Jan 10, 2017

Yun-An Chang

@ghfjdksl ghfjdksl force-pushed the ghfjdksl:add_pdo_server_cn_master branch 3 times, most recently from 43414fc to a82df46 Jan 10, 2017

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 10, 2017

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?

@nikic nikic self-assigned this Jan 10, 2017

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

@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 :/

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

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.

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

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...

@phpguru

This comment has been minimized.

Copy link

commented Feb 3, 2017

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?

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Feb 4, 2017

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.

@jmarrero

This comment has been minimized.

Copy link

commented Feb 21, 2017

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.

@ossar

This comment has been minimized.

Copy link

commented Feb 24, 2017

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.

@nikic

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

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.

@gries

This comment has been minimized.

Copy link

commented Mar 17, 2017

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 :)

@mendicm

This comment has been minimized.

Copy link

commented May 15, 2017

@nikic Just checked on 7.0.18 and is working well.

Thank you!

@mpdude

This comment has been minimized.

Copy link

commented Mar 14, 2018

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

@motecshine

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

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

@ghfjdksl

This comment has been minimized.

Copy link
Author

commented Apr 3, 2018

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.

@ghfjdksl ghfjdksl closed this Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.