2.3 HttpSocket enhancements #947

Merged
merged 7 commits into from Nov 13, 2012

2 participants

@markstory
CakePHP member

Implement changes for two tickets:

I've implemented SSL peer verification using the changes in #334 as a base. SSL context options can be specified using the ssl_ prefix instead of having to deal with nested context options. Additionally I've included the mozilla root ca bundle for ssl certificate verification. You can use all of the standard ssl options when configuring HttpSocket now.

I'll update the documentation once this is merged in.

markstory added some commits Nov 10, 2012
@markstory markstory Rename HttpResponse to HttpSocketResponse.
This fixes long standing issues with pecl/http

Fixes #2765
3e3af1f
@markstory markstory Add context() to CakeSocket.
This will help enable peer verification for HttpSocket later on.
9e72564
@markstory markstory Add mozilla root CA bundle.
This will enable SSL peer verification in HttpSocket.
7a5dfdf
@markstory markstory Implement SSL peer verification in HttpSocket
Use the stream API and enable SSL certificate validation. This
increases the security features of HttpSocket, and provides easy access
to the stream context options.  SSL related configuration should be
prefixed with 'ssl_'.

Refs #2270
240c871
@lorenzo lorenzo commented on the diff Nov 11, 2012
lib/Cake/Network/Http/HttpResponse.php
- *
- * @param string $offset
- * @return void
- */
- public function offsetUnset($offset) {
- }
-
-/**
- * Instance as string
- *
- * @return string
- */
- public function __toString() {
- return $this->body();
- }
+class HttpResponse extends HttpSocketResponse {
@lorenzo
CakePHP member
lorenzo added a note Nov 11, 2012

This is a very clever solution to that problem :)

@bar
bar added a note Nov 15, 2012

Nice to see this one here, no more http_build_str() conflicts :)

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

I'm going to fix the build failures, as I clearly didn't run AllTests 😢

@markstory markstory merged commit bfbd055 into 2.3 Nov 13, 2012

1 check passed

Details default The Travis build passed
@ryantology

It looks like this change is throwing new warnings. I am still trying to track down a proper abstracted test case but it looks like the change is not compatible with ElasticSource https://github.com/dkullmann/CakePHP-Elastic-Search-DataSource The new 'context' parameter is throwing a notice. I am going to look into how the Elastic Datasource is generating URLs and see if this can be updated.

This is the debug output that I have so far.


Notice (8): Array to string conversion [CORE/Cake/Network/Http/HttpSocket.php, line 718]
Code Context
$uri = array(
    'host' => '*****',
    'port' => '*****',
    'scheme' => 'http',
    'user' => null,
    'pass' => null,
    'path' => 'content/item/_search',
    'query' => '',
    'fragment' => null,
    'persistent' => false,
    'protocol' => (int) 6,
    'timeout' => (int) 30,
    'context' => array(
        'ssl' => array(
            'verify_peer' => true,
            'verify_depth' => (int) 5,
            'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
            'CN_match' => '127.0.0.1'
        )
    )
)
$uriTemplate = 'http://127.0.0.1:9200/content/item/_search'
$stripIfEmpty = array(
    'host' => '*****',
    'query' => '?%query',
    'fragment' => '#%fragment',
    'user' => '%user:%pass@'
)
$key = 'host'
$strip = '%host:%port/'
$defaultPorts = array(
    'http' => (int) 80,
    'https' => (int) 443
)
$property = 'context'
$value = array(
    'ssl' => array(
        'verify_peer' => true,
        'verify_depth' => (int) 5,
        'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
        'CN_match' => '127.0.0.1'
    )
)
str_replace - [internal], line ??
HttpSocket::_buildUri() - CORE/Cake/Network/Http/HttpSocket.php, line 718
HttpSocket::url() - CORE/Cake/Network/Http/HttpSocket.php, line 567
ElasticSource::logQuery() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1607
ElasticSource::execute() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1417
ElasticSource::__call() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1378
ElasticSource::get() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
ElasticSource::read() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
Model::find() - CORE/Cake/Model/Model.php, line 2674
ContentitemElastic::getRecent() - APP/Model/ContentitemElastic.php, line 54
ReflectionMethod::invokeArgs() - [internal], line ??
Controller::invokeAction() - CORE/Cake/Controller/Controller.php, line 485
Dispatcher::_invoke() - CORE/Cake/Routing/Dispatcher.php, line 186
Dispatcher::dispatch() - CORE/Cake/Routing/Dispatcher.php, line 161
[main] - APP/webroot/index.php, line 92

The issue has to do with the persistent HttpSocket instance. It looks like the SSL peer verification does not clean up the 'context' value between requests. Here is an example.

    public function example() {
        App::uses('HttpSocket', 'Network/Http');
        $config = array(
            'datasource' => 'Example',
            'host' => '127.0.0.1',
            'port' => 80,
            'format' => 'application/json',
            'encoding' => 'utf-8',
        );
        extract($config);
        $scheme = 'http';
        $request = array('uri' => compact('host', 'port', 'scheme'), 'header' => array('Accept' => $config['format'], 'Content-Type' => $config['format']));
        $httpConfig = compact('host', 'port', 'request');
        $this->Http = new HttpSocket($httpConfig);

        // string query     
        $path = '/content/';
        $query  = null;
        $uri = $this->_uri(compact('path', 'query'));
        $body = null;
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        $uri = $this->_uri(compact('path', 'query'));
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        debug($response);

        $this->autoRender = false;
    }

    protected function _uri($config) {
        $config = Set::merge($this->Http->config, $config);
        unset($config['request']);
        return $config;
    }

CakePHP member

I'm a bit confused as I don't use this specific datasource. Is it just merging all the config options into the request array? I don't see where in HttpSocket the context is merged into the request data. Also your example doesn't use persistent connections, is it still related to that?

Sorry, the point about persistence wasn't very clear. What I meant was that the instance of Httpsocket is re-used rather than re-instantiated between requests.

As you pointed out the issue has to do with the reuse of $this->Http->config as the merged array of the uri value for Httpsocket::get The change in SSL peer verification commit pollutes the Httpsocket::config array and does not clean up the context value between requests.

It could be argued that ElasticSource is not implementing Httpsocket properly by merging the config parameters. As a patch to Elastic source I will remove the context parameter from the uri generator but it is possible that other people are implementing this the same way.

CakePHP member

I'll see if I can reproduce outside of the ElasticSearch source. Merging the entire config array into the url is dangerous and shouldn't be done.

CakePHP member

Running the following from a shell method doesn't trigger any errors or warnings:

$http = new HttpSocket();
$res = $http->get('https://github.com/markstory');
debug($res->isOk());

$res2 = $http->get('https://github.com/lorenzo');
debug($res2->isOk());

Additionally $http->request does not contain the context options. I'm inclined to believe that the ElasticSearch datasource is doing something silly here.

Sorry, it has only to do with the http->settings being re-used for the computation of the URI. It's not really a bug in Httpsocket as much as an unexpected change. This was the fix that I pushed to ElasticSource IdeaCouture/CakePHP-Elastic-Search-DataSource@4c7e669 As you can see there is already a edge case for the 'request' parameter being an array. Sorry for the witch hunt.

I do think it is a bit strange to change the settings array after the construct is called but it's not 'wrong'. It just makes it strange that the settings for one call are different if the same call is made twice.

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