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

supplied flag to turn off mysqli SSL verification if ssl_verify passed as false #4805

Merged
merged 2 commits into from Sep 9, 2016

Conversation

Projects
None yet
2 participants
@intekhabrizvi
Contributor

intekhabrizvi commented Sep 9, 2016

Updated codes to pass MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT flag

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Sep 9, 2016

Contributor

If this is a connect flag, do we still need the options() call?

And more importantly, while #4801 disabled SSL altogether (on PHP < 5.6.16), it did take into account what the manual says here:

MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT - Like MYSQLI_CLIENT_SSL, but disables validation of the provided SSL certificate. This is only for installations using MySQL Native Driver and MySQL 5.6 or later.

Isn't it therefore better not to set MYSQLI_CLIENT_SSL only if this flag here is set?

Contributor

narfbg commented Sep 9, 2016

If this is a connect flag, do we still need the options() call?

And more importantly, while #4801 disabled SSL altogether (on PHP < 5.6.16), it did take into account what the manual says here:

MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT - Like MYSQLI_CLIENT_SSL, but disables validation of the provided SSL certificate. This is only for installations using MySQL Native Driver and MySQL 5.6 or later.

Isn't it therefore better not to set MYSQLI_CLIENT_SSL only if this flag here is set?

@intekhabrizvi

This comment has been minimized.

Show comment
Hide comment
@intekhabrizvi

intekhabrizvi Sep 9, 2016

Contributor

I though the same, we are already using using MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT in options function but its not taking any effect hence i cross check PHP C source codes and found options function have no definition of MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT check line no 1733 of http://git.php.net/?p=php-src.git;a=blob;f=ext/mysqli/mysqli_api.c;hb=6baaa2bb1b0ec87d8c2d676da27d04370677a81c you can see definition of MYSQLI_OPT_SSL_VERIFY_SERVER_CERT but no definition of MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT hence no matter you write or not its not going to work.

Regards to MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT - Like MYSQLI_CLIENT_SSL due to poor explanation of flag behavior, at first i thought may be writing MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT only also do the job of MYSQLI_CLIENT_SSL as well but i was wrong, as per PHP C source codes test case file http://git.php.net/?p=php-src.git;a=blob;f=ext/mysqli/tests/bug51647.phpt;hb=6baaa2bb1b0ec87d8c2d676da27d04370677a81c#l44 on line no 44 you can see they are using both flag with pipe hence both flag have individual work and both are necessary.

Seems i need to apply for another patch for this.
Thanks

Contributor

intekhabrizvi commented Sep 9, 2016

I though the same, we are already using using MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT in options function but its not taking any effect hence i cross check PHP C source codes and found options function have no definition of MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT check line no 1733 of http://git.php.net/?p=php-src.git;a=blob;f=ext/mysqli/mysqli_api.c;hb=6baaa2bb1b0ec87d8c2d676da27d04370677a81c you can see definition of MYSQLI_OPT_SSL_VERIFY_SERVER_CERT but no definition of MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT hence no matter you write or not its not going to work.

Regards to MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT - Like MYSQLI_CLIENT_SSL due to poor explanation of flag behavior, at first i thought may be writing MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT only also do the job of MYSQLI_CLIENT_SSL as well but i was wrong, as per PHP C source codes test case file http://git.php.net/?p=php-src.git;a=blob;f=ext/mysqli/tests/bug51647.phpt;hb=6baaa2bb1b0ec87d8c2d676da27d04370677a81c#l44 on line no 44 you can see they are using both flag with pipe hence both flag have individual work and both are necessary.

Seems i need to apply for another patch for this.
Thanks

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Sep 9, 2016

Contributor

You don't need to open a new pull request, just push another commit (removing the options() call) to the same branch and this one will be automatically updated.

Contributor

narfbg commented Sep 9, 2016

You don't need to open a new pull request, just push another commit (removing the options() call) to the same branch and this one will be automatically updated.

@intekhabrizvi

This comment has been minimized.

Show comment
Hide comment
@intekhabrizvi

intekhabrizvi Sep 9, 2016

Contributor

Shall i comment that line or completely remove it, I am thinking may be in future they add those feature in PHP source codes so that we can start using options function again.

Contributor

intekhabrizvi commented Sep 9, 2016

Shall i comment that line or completely remove it, I am thinking may be in future they add those feature in PHP source codes so that we can start using options function again.

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Sep 9, 2016

Contributor

Remove it, we don't keep commented-out code.

Contributor

narfbg commented Sep 9, 2016

Remove it, we don't keep commented-out code.

@intekhabrizvi

This comment has been minimized.

Show comment
Hide comment
@intekhabrizvi

intekhabrizvi Sep 9, 2016

Contributor

Updated code pushed, pls check.

Contributor

intekhabrizvi commented Sep 9, 2016

Updated code pushed, pls check.

@narfbg narfbg added the Bug label Sep 9, 2016

@narfbg narfbg added this to the 3.1.1 milestone Sep 9, 2016

@narfbg narfbg merged commit 140c70e into bcit-ci:develop Sep 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

narfbg added a commit that referenced this pull request Sep 9, 2016

Merge pull request #4805 from intekhabrizvi/develop
Use MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT as a connection flag instead of option

narfbg added a commit that referenced this pull request Sep 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment