Better TLS (hopefully) #299

olivierbellone · 2016-09-21T17:18:58Z

r? @brandur
cc @stripe/api-libraries @remistr

This PR aims to solve the various TLS issues that some of our users have encountered, usually caused by weird interactions between software versions (most notably curl and its underlying SSL library, typically but not always OpenSSL) and the way we configure TLS in the library.

To summarize:

the library does not set CURLOPT_SSLVERSION at all anymore. From the PHP docs: "Your best bet is to not set this and let it use the default. "
users who need to set this option (on some older RedHat / CentOS versions, the server is capable of using TLS 1.2 but will use TLS 1.0 unless CURLOPT_SSLVERSION is explicitly set to either CURL_SSLVERSION_TLSv1 or CURL_SSLVERSION_TLSv1_2) can now do so by passing the option themselves (see README)
curl constants definitions have been moved out of the request() method so that we don't check if the constants are already defined for every request
the X-Stripe-Client-User-Agent header now includes curl version and the SSL library name and version

This update will unfortunately break some users' integrations (for those cases where the library happens to work now, but will require manually setting CURLOPT_SSLVERSION after the update), and so should be released with a major version bump.

brandur

This looks great to me; thanks again @olivierbellone! If it's peaceful with Field Engineering, I'll cut a major release with this later today like we talked about.

brandur · 2016-09-21T17:30:21Z

lib/HttpClient/CurlClient.php

+if (!defined('CURL_SSLVERSION_TLSv1_2')) {
+    define('CURL_SSLVERSION_TLSv1_2', 6);
+}
+// @codingStandardsIgnoreEnd


In retrospect, this definitely fits best up in the header of this file. Nice!

remi-stripe · 2016-09-21T19:10:02Z

README.md

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


Minor comment but I'd recommend that you add the exact error message people might get on this page. This is helpful for someone looking for the exact error either on the page or via Google.

Great point. Done!

remi-stripe · 2016-09-21T19:11:04Z

lib/HttpClient/CurlClient.php

+
+// @codingStandardsIgnoreStart
+// PSR2 requires all constants be upper case. Sadly, the CURL_SSLVERSION
+// constants to not abide by those rules.


minor nit "constants to not" => "constants do not"

remi-stripe · 2016-09-21T19:12:56Z

lib/ApiRequestor.php


        $langVersion = phpversion();
        $uname = php_uname();
+        $curlVersion = curl_version();


have we confirmed that calling this has a minimal impact? We've discussed this before in private but can't remember seeing clear results

I ran some basic benchmarking, and it takes approximately 1.3 seconds to run 1 million calls to curl_version on my laptop. Given that ~all of our users will never send more than 100 req/s, I think we're fine.

remi-stripe · 2016-09-21T19:13:58Z

Added a few minor comments here to address first but otherwise I think we're ready to ship. Since this is a major change, it might be worth delaying it a bit more and trying to think of anything we've wanted to change or fix for some time but couldn't because it would require a major version release

pderksen · 2016-09-21T19:55:49Z

Not sure if this helps or is too late...

Though they don't use this Stripe PHP library, the WooCommerce Stripe payment gateway added this:
woocommerce/woocommerce-gateway-stripe@877dcab

remi-stripe · 2016-09-21T20:01:42Z

@pderksen this is the original code we had in our library too as far as I can tell (they even mention olivier's name directly in the comments).

Unfortunately, we've ended up in situations where some users have PHP built with one specific version of curl and then uses a completely different one in production. When that happens, forcing the constant sometimes doesn't work at all producing errors such as the one documented in #289.

This PR is a different attempt to avoid this issue. We now rely on curl doing the right thing by default and then provide an option for users to explicitly force a different constant otherwise if they need to.

brandur · 2016-09-21T20:57:54Z

Since this is a major change, it might be worth delaying it a bit more and trying to think of anything we've wanted to change or fix for some time but couldn't because it would require a major version release

@remistr Given that major version releases tend to be slightly intimidating as is, what do you think about trying to keep this one fairly minimal? I'm a little worried that if there are too many other changes they might present a bit of a barrier to upgrade.

remi-stripe · 2016-09-21T21:42:04Z

@brandur well that seems the opposite of what a major release should be no? Otherwise we'll keep not changing things we want to fix because it's too minor for a major release and at the same time have to release major releases every time something less minor changes.

I can't remember a specific thing we wanted to fix but couldn't recently. I'm just trying to advocate for taking this a bit slower than usual and trying to make sure we're not missing a quick win we could get and avoid a 5.X in a few weeks.

olivierbellone · 2016-09-22T09:01:42Z

Hey @pderksen, thanks. As @remistr pointed out, this is how we previously set the option in our own library, and I'm the one who suggested that they do the same here. I've left another comment to let them know about our latest findings and this new update.

brandur · 2016-09-22T17:21:59Z

I can't remember a specific thing we wanted to fix but couldn't recently. I'm just trying to advocate for taking this a bit slower than usual and trying to make sure we're not missing a quick win we could get and avoid a 5.X in a few weeks.

Well I should point out that we're not burning through major releases at an incredible rate (the last was almost a year and a half ago), so it doesn't seem like we'd be starting a 5.x line anytime soon. But cool, it would be nice to get some of the people on #289 fixed up, but aside from that I guess we're not in a particular rush. Let me know when you think it would be wise to move forward on this one.

remi-stripe · 2016-09-23T01:06:43Z

@brandur Then I think we're okay merging and releasing this!

olivierbellone · 2016-09-23T17:16:54Z

I added a paragraph at the end of the README for plugin devs, to let them know about setAppInfo (this should have been in #294) and that they should plan for a configuration option to set CURLOPT_SSLVERSION if they want to ensure that their plugin can be used on all systems.

Feel free to tweak the wording or just rewrite the whole thing!

brandur

+1 to trying out the new README section. I left one minor comment on a typo above, but this looks good to me. Lets get it shipped out today.

brandur · 2016-09-26T17:22:43Z

README.md


 Alternately, a callable can be passed to the CurlClient constructor that returns the above array based on request inputs. See `testDefaultOptions()` in `tests/CurlClientTest.php` for an example of this behavior. Note that the callable is called at the beginning of every API request, before the request is sent.

+### SSL / TLS compatibiliy issues


Ugh, pretty minor, but I just noticed this typo on "compatibility". Mind fixing that?

olivierbellone · 2016-09-28T16:23:20Z

@brandur Sorry for the delay, GitHub failed to notify me for some reason. Typo is fixed, let's 🚢 this!

brandur · 2016-09-28T18:13:40Z

Released as 4.0.0. Fingers crossed :)

stripe-ci assigned brandur Sep 21, 2016

brandur approved these changes Sep 21, 2016

View reviewed changes

remi-stripe reviewed Sep 21, 2016

View reviewed changes

brandur mentioned this pull request Sep 21, 2016

TLS1.2 support for 2.x and 1.x versions #288

Closed

brandur mentioned this pull request Sep 21, 2016

Setting CURLOPT_SSLVERSION explicitly causes it to fail (curl error 35). #289

Closed

olivierbellone force-pushed the ob-fix-289 branch from b139ae3 to ae61759 Compare September 22, 2016 08:48

olivierbellone mentioned this pull request Sep 22, 2016

Ensure we're enforcing TLS 1.2 woocommerce/woocommerce-gateway-stripe#45

Closed

olivierbellone force-pushed the ob-fix-289 branch from ae61759 to 2c2d822 Compare September 23, 2016 17:10

brandur suggested changes Sep 26, 2016

View reviewed changes

olivierbellone force-pushed the ob-fix-289 branch from 2c2d822 to cf8bc52 Compare September 28, 2016 16:21

Better TLS (hopefully)

458adc0

olivierbellone force-pushed the ob-fix-289 branch from cf8bc52 to 458adc0 Compare September 28, 2016 16:21

brandur merged commit 329af1d into master Sep 28, 2016

brandur deleted the ob-fix-289 branch September 28, 2016 17:43

olivierbellone mentioned this pull request Oct 3, 2016

TLS version #301

Closed

ob-stripe mentioned this pull request Nov 21, 2016

Force cURL to use TLS 1.2 when TLS 1.2 is supported but not used by default #310

Closed

jasonvarga mentioned this pull request Mar 6, 2018

Stripe no longer supports API requests made with TLS 1.0. Please initiate HTTPS connections with TLS 1.2 or later laravel/cashier-stripe#494

Closed


		Alternately, a callable can be passed to the CurlClient constructor that returns the above array based on request inputs. See `testDefaultOptions()` in `tests/CurlClientTest.php` for an example of this behavior. Note that the callable is called at the beginning of every API request, before the request is sent.

		### SSL / TLS compatibiliy issues

Better TLS (hopefully) #299

Better TLS (hopefully) #299

Uh oh!

Conversation

olivierbellone commented Sep 21, 2016

Uh oh!

brandur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remi-stripe commented Sep 21, 2016

Uh oh!

pderksen commented Sep 21, 2016

Uh oh!

remi-stripe commented Sep 21, 2016

Uh oh!

brandur commented Sep 21, 2016

Uh oh!

remi-stripe commented Sep 21, 2016

Uh oh!

olivierbellone commented Sep 22, 2016

Uh oh!

brandur commented Sep 22, 2016

Uh oh!

remi-stripe commented Sep 23, 2016

Uh oh!

olivierbellone commented Sep 23, 2016

Uh oh!

brandur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olivierbellone commented Sep 28, 2016

Uh oh!

brandur commented Sep 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants