Add configuration to disable peer verification #62
Conversation
|
👍 but you need to add the feature on all adapters. Additionally, I would add two tests, one with a valid ssl certificate and one with an invalid ssl certificate. |
|
Ok, but I don't know when I will get to to that, but I will! On another note - I noticed a dependency `"phpunit/phpunit-mock-objects": "dev-matcher-verify as 2.3.x-dev" in the composer.json, but I couldn't find this branch/tag anywhere - what's about that? |
|
Thsi is related to sebastianbergmann/phpunit-mock-objects#191 I use my fork since this bug is not fixed. |
|
Ah, thanks for the info! |
|
I just updated most of the adapters, but I am currently hanging at React - could you have a look at it? From what I see, it seems not possible to pass context options when creating a socket (see reactphp-legacy/socket-client#4), and you have had a similar request as well, as I just saw (reactphp-legacy/socket-client#17 (comment)). React will have a problem with insecure Certificates starting with PHP 5.6, where the verification has been set to |
| 'allow_self_signed' => !$this->getConfiguration()->getSSLVerifyPeer(), | ||
| ); | ||
|
|
||
| $context = stream_context_create(array(), $contextOptions); |
There was a problem hiding this comment.
This is what would be needed to be set for React as well.
There was a problem hiding this comment.
If react is not able to do it, please open an issue if there is none and document it as not usable with this adapter.
There was a problem hiding this comment.
The issue is already there: reactphp-legacy/socket-client#4.
|
Can you add a test with an ssl verification? Thanks |
|
How do you envision such a test with ssl verification - in which TestCase would you see it and what should it include? I must admit, I wasn't able to setup a fully working test environment yet and was performing my local test suite exclusively with Except the mentioned test, everything should be in and as you wished, though. Perhaps you would be able to add those tests more quickly than I would be? |
|
I know the test suite is not so easy to setup but at the same time, what it does is not easy as well :) Anyway, I understand your issues and will play with your code. Unfortunatelly, I should not be able to do it before the end of the week. If I forget, don't hesitate to ping me :) |
|
Btw, thanks for your work! 👍 ❤️ |
|
Thank you! The HTTP adapter is a wonderful library, I am happy to be able to participate and appreciate you letting me! |
|
Is this issue PR the one that is standing between the release of 0.6? If yes, I would try to work out how to to an SSL test this evening. |
|
No, the 0.6 bloker is #61. |
|
I will try to add some tests on this one |
|
I have added two tests called Unfortunatelly, it seems there is only the curl adapter which works :s Would you like I submit them as PR on your fork? I need first to set up it on travis... Here the output of my tests: There were 4 errors:
1) Ivory\Tests\HttpAdapter\BuzzCurlHttpAdapterTest::testSendWithSelfSignedSslCertificate
Ivory\HttpAdapter\HttpAdapterException: An error occurred when fetching the URL "https://127.0.0.1:11000/server.php" with the adapter "buzz" ("SSL: unable to obtain common name from peer certificate").
/home/gelo/Workspace/Ivory/http-adapter/src/HttpAdapterException.php:101
/home/gelo/Workspace/Ivory/http-adapter/src/BuzzHttpAdapter.php:89
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:292
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:74
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapterTemplate.php:28
/home/gelo/Workspace/Ivory/http-adapter/tests/AbstractHttpAdapterTest.php:305
2) Ivory\Tests\HttpAdapter\CakeHttpAdapterTest::testSendWithSelfSignedSslCertificate
Ivory\HttpAdapter\HttpAdapterException: An error occurred when fetching the URL "https://127.0.0.1:11000/server.php" with the adapter "cake" ("stream_socket_client(): Unable to locate peer certificate CN
stream_socket_client(): Failed to enable crypto
stream_socket_client(): unable to connect to ssl://127.0.0.1:11000 (Unknown error)").
/home/gelo/Workspace/Ivory/http-adapter/src/HttpAdapterException.php:101
/home/gelo/Workspace/Ivory/http-adapter/src/CakeHttpAdapter.php:69
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:292
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:74
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapterTemplate.php:28
/home/gelo/Workspace/Ivory/http-adapter/tests/AbstractHttpAdapterTest.php:305
3) Ivory\Tests\HttpAdapter\Zend1CurlHttpAdapterTest::testSendWithSelfSignedSslCertificate
Ivory\HttpAdapter\HttpAdapterException: An error occurred when fetching the URL "https://127.0.0.1:11000/server.php" with the adapter "zend1" ("Error in cURL request: SSL certificate problem: self signed certificate").
/home/gelo/Workspace/Ivory/http-adapter/src/HttpAdapterException.php:101
/home/gelo/Workspace/Ivory/http-adapter/src/Zend1HttpAdapter.php:71
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:292
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:74
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapterTemplate.php:28
/home/gelo/Workspace/Ivory/http-adapter/tests/AbstractHttpAdapterTest.php:305
4) Ivory\Tests\HttpAdapter\Zend2CurlHttpAdapterTest::testSendWithSelfSignedSslCertificate
Ivory\HttpAdapter\HttpAdapterException: An error occurred when fetching the URL "https://127.0.0.1:11000/server.php" with the adapter "zend2" ("Error in cURL request: SSL certificate problem: self signed certificate").
/home/gelo/Workspace/Ivory/http-adapter/src/HttpAdapterException.php:101
/home/gelo/Workspace/Ivory/http-adapter/src/Zend2HttpAdapter.php:71
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:292
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapter.php:74
/home/gelo/Workspace/Ivory/http-adapter/src/AbstractHttpAdapterTemplate.php:28
/home/gelo/Workspace/Ivory/http-adapter/tests/AbstractHttpAdapterTest.php:305
--
There were 9 failures:
1) Ivory\Tests\HttpAdapter\FileGetContentsHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
2) Ivory\Tests\HttpAdapter\FopenHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
3) Ivory\Tests\HttpAdapter\GuzzleHttpStreamHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
4) Ivory\Tests\HttpAdapter\ReactHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
5) Ivory\Tests\HttpAdapter\SocketHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
6) Ivory\Tests\HttpAdapter\Zend1ProxyHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
7) Ivory\Tests\HttpAdapter\Zend1SocketHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
8) Ivory\Tests\HttpAdapter\Zend2ProxyHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
9) Ivory\Tests\HttpAdapter\Zend2SocketHttpAdapterTest::testSendWithInvalidSslCertificate
Failed asserting that exception of type "\Ivory\HttpAdapter\HttpAdapterException" is thrown.
FAILURES!
Tests: 2581, Assertions: 40737, Failures: 9, Errors: 4, Skipped: 6. |
|
Of course, please feel free to add them to my fork - starting there, I can learn from what you did and perhaps continue the research on the topic. |
|
I think you also can push them to "my" branch directly if you add my fork as a remote, or just send me the code as a Gist and I will work it in. |
|
Then, can you give me push permission to your fork? |
|
Oops, done! |
|
Thanks! I'm still not able to make it working on travis. It seems I need to generate a new signed certificate according to the hostname used... I can't do it right now but I will be on week-end tomorrow afternoom so, I will work on it tomorrow night and I will release the 0.6 on the same time. Then, I will be able to resync the lib with the latest PSR-7 version and fix other issues related to the event dispatcher :) Btw, have you been able to make the tests running on your laptop? |
|
How do you know I'm using a Laptop? ARE YOU STALKING ME! I haven't had the opportunity to even have a look at it yet, but I will try this evening. What would you think about having a dedicated Webserver somewhere to run the tests against? We could hide it by using dotenv with an encrypted .env file - or find a service like http://httpstat.us ... or create one :). |
|
If you were okay with using httpstat.us as a test server, this would greatly simplify the travis workflow, and we could use it to test:
at least for simple tests... what do you think? |
|
Have you pushed the new tests already? I haven't found them yet on one of my branches... |
|
I will push them later on the night. I still need to make them running on travis... I'm not familiar with the service you pointed out. My only concern is performance. I don't want tests to run during more than 10 minutes, it will become insane. Currently, there is lot of requests which are send (sometime more than 60 in parallel...), so if the service does not decrease too much performance and if it simplifies tests, I'm 👍 |
|
@jeromegamez I just added tests which are not fully complete (some travis steps are missing). |
|
@jeromegamez I just rebase this PR and rework a little bit the patch in order to make it working on travis + make tests passing for some adapters. The remaining failling ones will need more investigation in order to be solved... The broken one are:
For other adapters, tests are green! |
|
Phew, this is tough... Guzzle 3.x is deprecated anyways - I know, this would be quite a "workaround", but do you really want to continue to support it when the library itself is two versions behind and not supported anymore? As for the other two... unfortunately, I still am not able to setup the same development environment as you have to perform the tests with the test server that is also used on travis. Do you perhaps have a vagrant Dev-VM that we don't know about yet, that you could share? :) |
|
For the record, I'm on this one again. I must admit it is a pain due to the buggy adapters which require a lot of debug... |
|
The remaining ones which need to be fixed are Zend 1 & 2. |
|
How about not doing anything on clients that don't support it and adding a E_NOTICE or something similar to them? For the unit tests, they could be marked as skipped so that they don't get forgotten and can be re-tested at later times when new versions of the clients have been released? |
|
Yeah, for the adapter which does not support a feature, I explicitely mark the related tests as skipped. I like the idea of E_NOTICE but I wouldn't be able to do it for all unsupported feature... If you have an idea, I would like to see it :) So, for now, I prefer document it to let end user be aware of this missing feature. |
|
Here, I am! Everything is fine except for React 🤘 |
| @@ -48,6 +48,18 @@ public function testSendWithTimeoutExceeded($timeout) | |||
| $this->markTestSkipped(); | |||
| } | |||
|
|
|||
| public function testSendWithSelfSignedSslCertificate() | |||
There was a problem hiding this comment.
Note to myself, this test shouldn't be skipped but for a reason I don't understand for now, React fails for PHP >= 5.6 only.
There was a problem hiding this comment.
Probably due to missconfiguration of php.
|
The build is probably broken due to the usage of my fork of guzzle/RingPHP which is not compatible with the lowest deps... I will need to refine the guzzlehttp deps... |
|
I think this should be reimplemented after the migration. @jeromegamez do you mind that? |
|
Personally, I'm fine with that! I will keep the code anyway. And I have still an issue to open on react as I have different behaviors on different PHP versions. |
|
I don't mind at all - ping me when the migration is finished and I will get to it 👍 |
|
This is a known limitation of the library and will not be fixed as this repository is deprecated in favor of https://github.com/php-http The good fix is to upgrade following http://docs.httplug.io/en/latest/httplug/migrating/ |
Sometimes, one might want (or have) to connect to an SSL host with a self-signed certificate.
This PR adds the possibility to disable the peer certificate verification (the default remains
true). The naming of the variable and methods follow the naming in the curl_setopt() specification (CURLOPT_SSL_VERIFYPEER).Cheers
Jérôme