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.

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

Manually override Ruby's SSL defaults when making Stripe requests #107

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@ebroder
Copy link
Contributor

ebroder commented Jan 25, 2014

The Ruby maintainers are in the middle of a bit of a kerfluffle about the SSL defaults (which are...fine as long as you're running a recent Linux release or Mavericks and...aren't being man-in-the-middle'd)

As long as Ruby upstream is failing to take action, we're really in a position where we should be protecting our users ourselves.

This overrides Ruby's default parameters for new SSL_CTX objects for the duration of a Stripe API request. Specifically, it constrains the cipher suite list and disables SSLv2, on the off chance it's available.

Thanks to @jmhodges for making us aware of these specific issues, and more generally for his noble crusade for better SSL configurations.

@ab Could you take a look at this? I'm pretty unhappy about having to do this, but I'm not sure I see a better option.

Manually override Ruby's SSL defaults when making Stripe requests
The Ruby maintainers are in the middle of a bit of a kerfluffle about
the SSL defaults (which are...fine as long as you're running a recent
Linux release or Mavericks and...aren't being man-in-the-middle'd)

As long as Ruby upstream is failing to take action, we're really in a
position where we should be protecting our users ourselves.

This overrides Ruby's default parameters for new SSL_CTX objects for
the duration of a Stripe API request. Specifically, it constrains the
cipher suite list and disables SSLv2, on the off chance it's
available.

Thanks to @jmhodges for making us aware of these specific issues, and
more generally for his noble crusade for better SSL configurations.
@ebroder

This comment has been minimized.

Copy link
Contributor

ebroder commented Jan 25, 2014

Ah right - I used this to confirm that it was working: [1]

Unfortunately, it still negotiates TLSv1.1 on my laptop because of [2], which was discovered in [3]. Ruby only added support for ssl_version=:TLSv1_2 in 2.0.0; for versions before that, there doesn't seem to be any way to trigger a TLSv1.2 negotiation on modern Debian/Ubuntu builds of OpenSSL.

I'm still on the fence about whether we should override the SSL protocol version in the subset of cases where we can. Everything else we're doing should be sufficient.

[1] https://gist.github.com/ebroder/7ed9572c91ba670a8354
[2] https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1256576/comments/4
[3] https://twitter.com/ebroder/status/427086361980461056

@andrewpthorp

This comment has been minimized.

Copy link
Contributor

andrewpthorp commented Jan 25, 2014

Added a comment to your gist, what I get with TLSv1.2.

@ebroder

This comment has been minimized.

Copy link
Contributor

ebroder commented Jan 26, 2014

Ok. From looking at the patch [1], there is no way to encourage Debian/Ubuntu OpenSSL to negotiate to TLSv1.2 - setting all of SSL_OP_NO_{SSLv2,SSLv3,TLSv1,TLSv1_1} causes it to conclude that there's no version of SSL/TLS it can negotiate to and therefore it closes the connection 😭

Our only option is definitely to force the version to :TLSv1_2, but I don't want to do that because it won't allow us to negotiate to future TLS versions. One option is to compare OpenSSL::SSL::SSLContext::METHODS to its current value to ensure that a new version hasn't been introduced. I don't love that, but I think it will work. Thoughts?

[1] http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/saucy/openssl/saucy/view/head:/debian/patches/tls12_workarounds.patch

@amfeng

This comment has been minimized.

Copy link
Contributor

amfeng commented Mar 5, 2014

Ping! Is this still relevant?

@ab

This comment has been minimized.

Copy link
Contributor

ab commented Mar 7, 2014

Yeah it unfortunately is. I'm also thinking about whether to put some or all of this logic into rest-client itself. I think @ebroder's plan to examine OpenSSL::SSL::SSLContext::METHODS is the best we can do given the Debian/Ubuntu nonsense.

@ab

This comment has been minimized.

Copy link
Contributor

ab commented Mar 13, 2014

Also, is this thread safe?

@ebroder

This comment has been minimized.

Copy link
Contributor

ebroder commented Mar 13, 2014

Uggggggh why do you ask questions when you know the answer will make you unhappy

@ebroder

This comment has been minimized.

Copy link
Contributor

ebroder commented Mar 13, 2014

(i.e. not even a little bit)

@amfeng

This comment has been minimized.

Copy link
Contributor

amfeng commented Jul 8, 2014

ping! (:

@ebroder

This comment has been minimized.

Copy link
Contributor

ebroder commented Jul 8, 2014

Sigh. I really wanted to do something here, but I think it might be impossible without way more monkey-patching than I'm comfortable with. rest-client is slowly growing enough flags that we can replicate a lot of this functionality, so I think we should just give up and encourage people to upgrade when that's an option.

@ebroder ebroder closed this Jul 8, 2014

@ab

This comment has been minimized.

Copy link
Contributor

ab commented Jul 8, 2014

Agreed on all points, though it's not super pleasing.

@matthewarkin

This comment has been minimized.

Copy link

matthewarkin commented Nov 21, 2014

Given the great amount of grief POODLE has caused at least in #Stripe's irc room, I think this should be revisited.

@peterkeen

This comment has been minimized.

Copy link

peterkeen commented Nov 21, 2014

I agree with @matthewarkin. This should be addressed in a way that helps people now, even if they have to upgrade again later at some point.

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