Skip to content

[#10] Add CURLOPT_SSLVERSION #11

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[#10] Add CURLOPT_SSLVERSION #11

wants to merge 1 commit into from

Conversation

visitek
Copy link

@visitek visitek commented May 12, 2017

Fix for SSLVERSION. Curl has changed defaults in PHP7

@tjconcept
Copy link
Member

What is the default for this value?

I am hesitant about hardcoding a version as you would then have to update this when the world moves on. Usually this is negotiated between the client and the server to use the highest common version.

What is the motivation for this change? To force some older platforms to use the minimum required version?

Could it be achieved in a more "at least this"-fashion, e.g. without "limiting" to version 1.2?

@visitek
Copy link
Author

visitek commented May 12, 2017

There is importance for Paylike API.

CURL_SSLVERSION_DEFAULT
The default action. This will attempt to figure out the remote SSL protocol version.

Paylike API revokes this option

according to: https://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html

SSLv2 is disabled by default since 7.18.1. Other SSL versions availability may vary depending on which backend libcurl has been built to use.
SSLv3 is disabled by default since 7.39.0.

@tjconcept
Copy link
Member

tjconcept commented May 12, 2017 •
edited
Loading

Paylike API revokes this option

What do you mean by this?

The Paylike API (the http service itself) will not allow insecure connections (and none of the old SSLv* versions), so unless you have a connection issue, you should probably just have your platform negotiate the best version.

From the documentation you linked, it seems that curl_easy_setopt(curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1.1 | CURL_SSLVERSION_MAX_DEFAULT); would be an option to prevent downgrade attacks, but I would have to research a bit more to be confident in adding something like that.

@visitek
Copy link
Author

visitek commented May 14, 2017

https://github.com/stripe/stripe-php
it is known issue

SSL / TLS compatibility issues

Stripe's API now requires that all connections use TLS 1.2. Some systems (most notably some older CentOS and RHEL versions) are capable of using TLS 1.2 but will use TLS 1.0 or 1.1 by default. In this case, you'd get an invalid_request_error with the following error message: "Stripe no longer supports API requests made with TLS 1.0. Please initiate HTTPS connections with TLS 1.2 or later. You can learn more about this at https://stripe.com/blog/upgrading-tls.".

The recommended course of action is to upgrade your cURL and OpenSSL packages so that TLS 1.2 is used by default, but if that is not possible, you might be able to solve the issue by setting the CURLOPT_SSLVERSION option to either CURL_SSLVERSION_TLSv1 or CURL_SSLVERSION_TLSv1_2:

$curl = new \Stripe\HttpClient\CurlClient(array(CURLOPT_SSLVERSION => CURL_SSLVERSION_TLSv1));
\Stripe\ApiRequestor::setHttpClient($curl);

@ionutcalara
Copy link
Contributor

I think adapting this to allow user defined parameters for curl should be fine, for testing purposes you may not always be able to test with a valid certificate. If the Paylike api does not allow insecure connections for live credentials, then this is not an issue of security. I think the default setting allows for better support in this case, but adding the option of changing the curl arguments may be useful.

@tjconcept
Copy link
Member

tjconcept commented May 14, 2017 •
edited
Loading

Okay, so, the Stripe explanation provides the context (and merits) for setting such an option.

Some systems (most notably some older CentOS and RHEL versions) are capable of using TLS 1.2 but will use TLS 1.0 or 1.1 by default

The reason for setting such option is to force some older systems that do not advertise a newer protocol, although they have some capability of it, probably because it has been considered "experimental" or at least not "ready for stable production".

It is definitely not something we should add to our code as the "right" solution is almost always to upgrade your system. Stripe API author seems to agree:

The recommended course of action is to upgrade your cURL and OpenSSL packages

I do not want our code to consider non-stable, non-production, experimental or beta software.

I would only consider such change if this was affecting a significant number of users for which the act of upgrading was out of their control and the "experimental" TLS 1.2 support was considered safe and secure by trusted sources.

If someone wants to do this anyway they must be very aware (of the security implications of running outdated software, if that's the case), and thus skilled in software, so forking the code and adding the setting should be a no-brainer.

Thanks @visitek for bringing this up. I learnt something along the way. I hope you understand (and might agree) why this PR is not getting merged.
Hopefully if someone hits this issue, they will see this discussion and upgrade their system.

@tjconcept tjconcept closed this May 14, 2017
@tjconcept
Copy link
Member

@ionutcalara, if you had something more generic in mind, or regarding the CURLOPT_SSL_VERIFYPEER option, and there's a use case for it, it can be considered in a new PR.

@tjconcept tjconcept mentioned this pull request May 14, 2017
Closed
@visitek
Copy link
Author

visitek commented May 14, 2017 via email

@tjconcept
Copy link
Member

tjconcept commented May 14, 2017 •
edited
Loading

Interesting. So you're saying it might actually be a problem to a significant number of users that cannot upgrade? This would also mean that the Stripe statement is flawed.

I've just tested the following:

$ docker run -i -t centos:7
$ yum install -y curl php
$ curl https://api.paylike.io/ping
$ php test.php

Where test.php is a script using this library to do the same. Both work just fine.

Can you provide me with a Dockerfile or similar instructions on how to reproduce this? I'm trying to figure out who and how many are affected by this.

@visitek
Copy link
Author

visitek commented May 14, 2017 via email

@tjconcept
Copy link
Member

Could very well be. There might be a use case for the certificate verification option if you are for instance mocking the Paylike service when unit testing.

I'm still interested in discovering why you are seeing this issue on a recent REHL installation. Could you dump the output of uname -a, curl --version and php --version? Or debug why you are seeing when I'm not in my docker test?

@visitek
Copy link
Author

visitek commented May 14, 2017 via email

@tjconcept
Copy link
Member

Okay, so the kernel is older than mine (I'm on 4.9.27) and the PHP version is much newer. Can you guide me to how you upgraded PHP?

@visitek
Copy link
Author

visitek commented May 14, 2017

simply by
yum install php71w php71w-devel

@tjconcept
Copy link
Member

tjconcept commented May 14, 2017 •
edited
Loading

Ah, this is not a package in the official repositories, it's from Webtatic, right?

I added their repos and installed php71w. The script still runs without problem.

We seem to now have identical PHP versions:

PHP 7.1.4 (cli) (built: Apr 15 2017 08:07:03) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2017 Zend Technologies

I can't really get further from here unless you can provide me some clue. Could you try debugging your setup? E.g. checking the cURL version used from PHP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@visitek @tjconcept @ionutcalara