CurlClient fail to exec HTTPS with empty HTTPOptions

[eclipxe13]

Hello, thanks for your great work!

Was trying to use CurlClient without any (default) HTTPOptions but it fail with a curl error:

chillerlan\HTTP\Psr18\RequestException : error setting certificate verify locations:
  CAfile: 
  CApath: /etc/ssl/certs

wich is weird because making a plain curl connection didn't fail:

    public function testCurlIsWorking()
    {
        $curl = curl_init();
        curl_setopt($curl, \CURLOPT_URL, 'https://example.com/');
        curl_setopt($curl, \CURLOPT_SSL_VERIFYHOST, 2);
        curl_setopt($curl, \CURLOPT_SSL_VERIFYPEER, true);

        \curl_exec($curl);

        $errno = \curl_errno($curl);
        if ($errno !== \CURLE_OK) {
            $error = \curl_error($curl);
            $this->fail($error);
        }
        $this->assertTrue(true, 'test complete');
    }

So I take a look into your package and I see that in CurlHandle::init() you are setting $this->options->curl_options[\CURLOPT_CAINFO] with a NULL value.

If you modify the test and add this line will fail in the same way.

curl_setopt($curl, \CURLOPT_CAINFO, null);

I think that the problem is in CurlHandle::initCurlOptions() that returns an array with CURLOPT_CAINFO => $this->options->ca_info even when $this->options->ca_info === NULL. I tested including CURLOPT_CAINFO only when $this->options->ca_info is not falsy and it work just fine.

By the way, the same would happens with CURLOPT_CAPATH if you provide a way to work with.

Looks like CURLOPT_CAINFO or CURLOPT_CAPATH cannot be removed using null or empty strings, and it will fail. Also, if both are set then both must succeed.

Could you consider to fix this? Do you want me to make a PR? I'm not familiar with your test suite. The only this I was thinking is to simply remove 'ca_info' => __DIR__.'/../cacert.pem' from the initiation parameters on CurlClientTest.php.

[eclipxe13]

I just did #3 that also covers StreamClient with the same issue.

[codemasher]

Hey, thank you for the heads-up! I think #3 is a bit too invasive. The concept is to decouple the "settings" logic as far as possible from the application logic. So the whole process of determining the CA info went into HTTPOptions over here (see also #1):

php-httpinterface/src/HTTPOptionsTrait.php

Lines 83 to 189 in a780569

	protected function setCA():void{
	// disable verification if wanted so
	if($this->ssl_verifypeer !== true || (isset($this->curl_options[\CURLOPT_SSL_VERIFYPEER]) && !$this->curl_options[\CURLOPT_SSL_VERIFYPEER])){
	unset($this->curl_options[\CURLOPT_CAINFO], $this->curl_options[\CURLOPT_CAPATH]);
	$this->curl_options[\CURLOPT_SSL_VERIFYHOST] = 0;
	$this->curl_options[\CURLOPT_SSL_VERIFYPEER] = false;
	return;
	}
	$this->curl_options[\CURLOPT_SSL_VERIFYHOST] = 2;
	$this->curl_options[\CURLOPT_SSL_VERIFYPEER] = true;
	// a path/dir/link to a CA bundle is given, let's check that
	if(\is_string($this->ca_info)){
	// if you - for whatever obscure reason - need to check Windows .lnk links,
	// see http://php.net/manual/en/function.is-link.php#91249
	switch(true){
	case \is_dir($this->ca_info):
	case \is_link($this->ca_info) && \is_dir(\readlink($this->ca_info)): // @codeCoverageIgnore
	$this->curl_options[\CURLOPT_CAPATH] = $this->ca_info;
	unset($this->curl_options[\CURLOPT_CAINFO]);
	return;
	case \is_file($this->ca_info):
	case \is_link($this->ca_info) && \is_file(\readlink($this->ca_info)): // @codeCoverageIgnore
	$this->curl_options[\CURLOPT_CAINFO] = $this->ca_info;
	unset($this->curl_options[\CURLOPT_CAPATH]);
	return;
	}
	throw new ClientException('invalid path to SSL CA bundle (HTTPOptions::$ca_info): '.$this->ca_info);
	}
	// we somehow landed here, so let's check if there's a CA bundle given via the cURL options
	$ca = $this->curl_options[\CURLOPT_CAPATH] ?? $this->curl_options[\CURLOPT_CAINFO] ?? false;
	if($ca){
	// just check if the file/path exists
	switch(true){
	case \is_dir($ca):
	case \is_link($ca) && \is_dir(\readlink($ca)): // @codeCoverageIgnore
	unset($this->curl_options[\CURLOPT_CAINFO]);
	return;
	case \is_file($ca):
	case \is_link($ca) && \is_file(\readlink($ca)): // @codeCoverageIgnore
	return;
	}
	throw new ClientException('invalid path to SSL CA bundle (CURLOPT_CAPATH/CURLOPT_CAINFO): '.$ca);
	}
	// check php.ini options - PHP should find the file by itself
	if(\file_exists(\ini_get('curl.cainfo'))){
	return; // @codeCoverageIgnore
	}
	// this is getting weird. as a last resort, we're going to check some default paths for a CA bundle file
	$cafiles = [
	// check other php.ini settings
	\ini_get('openssl.cafile'),
	// Red Hat, CentOS, Fedora (provided by the ca-certificates package)
	'/etc/pki/tls/certs/ca-bundle.crt',
	// Ubuntu, Debian (provided by the ca-certificates package)
	'/etc/ssl/certs/ca-certificates.crt',
	// FreeBSD (provided by the ca_root_nss package)
	'/usr/local/share/certs/ca-root-nss.crt',
	// SLES 12 (provided by the ca-certificates package)
	'/var/lib/ca-certificates/ca-bundle.pem',
	// OS X provided by homebrew (using the default path)
	'/usr/local/etc/openssl/cert.pem',
	// Google app engine
	'/etc/ca-certificates.crt',
	// Windows?
	// http://php.net/manual/en/function.curl-setopt.php#110457
	'C:\\Windows\\system32\\curl-ca-bundle.crt',
	'C:\\Windows\\curl-ca-bundle.crt',
	'C:\\Windows\\system32\\cacert.pem',
	'C:\\Windows\\cacert.pem',
	// working path
	__DIR__.'/cacert.pem',
	];
	foreach($cafiles as $file){
	if(\is_file($file) || (\is_link($file) && \is_file(\readlink($file)))){
	$this->curl_options[\CURLOPT_CAINFO] = $file;
	return;
	}
	}
	// @codeCoverageIgnoreStart
	$msg = 'No system CA bundle could be found in any of the the common system locations. '
	.'In order to verify peer certificates, you will need to supply the path on disk to a certificate bundle via '
	.'HTTPOptions::$ca_info or HTTPOptions::$curl_options. If you do not need a specific certificate bundle, '
	.'then you can download a CA bundle over here: https://curl.haxx.se/docs/caextract.html. '
	.'Once you have a CA bundle available on disk, you can set the "curl.cainfo" php.ini setting to point '
	.'to the path of the file, allowing you to omit the $ca_info or $curl_options setting. '
	.'See http://curl.haxx.se/docs/sslcerts.html for more information.';
	throw new ClientException($msg);
	// @codeCoverageIgnoreEnd
	}

The cURL options are applied here before firing the request:

php-httpinterface/src/Psr18/CurlHandle.php

Lines 249 to 250 in a780569

	// overwrite the default values with $curl_options
	\curl_setopt_array($this->curl, $options + $this->options->curl_options);

So if there's a problem finding the correct CA on your system, it might be that i'm probably missing a default path or something. The tests currently don't include much testing for default paths. So if you're looking to fix that, the best would be in HTTPOptionsTrait in first place. Also, i should point out, that the StreamClient is in "experimental" status at best.

[eclipxe13]

As you mention, on php-httpinterface/src/Psr18/CurlHandle.php:250 is where a null value for CURLOPT_CAINFO is set.

My first thought was to check if CURLOPT_CAINFO exists (not with isset, but using array_key_exists), and if it is falsy then remove it from the options to apply.

Then I think that would be better to check the place where this is set and found that this null value comes from CurlHandle::initCurlOptions not include CURLOPT_CAINFO.

Anyhow, please, play around with the test I post in the issue.
It is not correct to set CURLOPT_CAINFO or CURLOPT_CAPATH to null, false or "" empty strings.

[codemasher]

Ok, i investigated a bit further and found an error on my end over here:

php-httpinterface/src/Psr18/CurlHandle.php

Lines 249 to 250 in a780569

	// overwrite the default values with $curl_options
	\curl_setopt_array($this->curl, $options + $this->options->curl_options);

The PHP documentation says:

The + operator returns the right-hand array appended to the left-hand array; for keys that exist in both arrays, the elements from the left-hand array will be used, and the matching elements from the right-hand array will be ignored.

So it would be simply:

curl_setopt_array($this->curl, $this->options->curl_options + $options);

To circumvent any future bugs, preserve the numerical array keys (array_merge() doesn't) and dump the + operator i just changed it to a quick loop.

php-httpinterface/src/Psr18/CurlHandle.php

Lines 249 to 252 in 5afc327

	// overwrite the default values with $curl_options
	foreach($this->options->curl_options as $k => $v){
	$options[$k] = $v;
	}

As for the fix in StreamClient, i've added this line to make sure a CA is set in the options:

php-httpinterface/src/HTTPOptionsTrait.php

Line 174 in c28ada3

$this->ca_info = $file;

Anything else should throw an error now. Please let me know if this helps.

[eclipxe13]

Thank you.