Remove Certifi and use default Ruby SSL config

[zanker]

By default, Raven isn't actually using the SSL certificates it gets from Certifi. Because of a bug in Faraday, the default Net::HTTP adapter it uses, sets both cert_store + ca_file/ca_path on the same instance. The cert_store it defaults to is the Ruby default with system SSL paths and that takes precedence over ca_file/ca_path.

My suggestion is to remove Certifi and just use the Ruby defaults for SSL.

[tarcieri]

There are a few reasons I'd suggest avoiding Certifi:

1. Some users actually do maintain and/or customize their local truststores with certificates/CAs specific to their internal infrastructure. Trying to ship a separate cert bundle can potentially break these configurations. It seems you avoided this by luck here thanks to a bug in Faraday
2. Unless maintained, shipping a certificate bundle is dangerous. CAs are routinely compromised and need to be removed, and the cert bundles are also being updated all the time with new certificates. Certifi has not been updated for over a year (May 18, 2014) and is probably in a much worse state than your typical OS-level truststore

[dcramer]

@tarcieri im like 95% confident that SSL was broken for any number of people before we added Certifi/our own certs

[tarcieri]

@dcramer people can likewise "fix" problems with SSL by configuring OpenSSL::SSL::VERIFY_NONE, but it doesn't make it a good idea. While it will get things "working", it's defeating the point of attempting to make a secure connection.

If people actually find Certifi solves their problems, they should be using it as end users. Forcing it onto everyone who uses the raven-ruby gem stands the chance of actively downgrading the security of people who do have properly-configured local truststores.

[dcramer]

@tarcieri I'd agree that ideally it'd use system certificates, and then our backups, but you underestimate how many people use something like Sentry that rarely understand how SSL works.

[tarcieri]

Let me say this again: you are forcing an insecure CA bundle on everyone who uses your gem. This is bad.

[taoeffect]

Sorry for jumping in, but I am confused.. Tony said:

There are a few reasons I'd suggest avoiding Certifi:

But the OP doesn't seem to be suggesting that either?

My suggestion is to remove Certifi and just use the Ruby defaults for SSL.

[taoeffect]

Wait, nm, I think Tony is saying 👍 to this PR. Sorry for the noise.

[dcramer]

@tarcieri all software can be considered insecure, its why we have releases of it. If the certificates became invalidated, we could push a new release.

I'm not willing to accept raven-ruby not working for the large audience to satisfy anything, which is why we started shipping certs in the first place.

If it's true Certifi isnt working at all (in any condition) then we should obviously remove it, but I dont know if that's actually the case or not. I'll leave it to others to determine that. Outside that, we will always ship getsentry.com (at minimum) certificates with raven-ruby, and those will always be the default configuration.

[tarcieri]

If the certificates became invalidated, we could push a new release.

You're not listening to what I'm saying. They're already well out-of-date. Certifi at the very least NEEDS regular releases.

@dcramer I would suggest removing Certifi by default and providing instructions to use it for people who are having problems, rather than forcing it onto everyone by default

[dcramer]

@tarcieri as I mentioned, for the reasons stated, we will always ship with our certificates. Whether that is via Certifi or simply just shipping the getsentry.com root. Certifi (at least from a high level) is a better solution than just shipping ours, but if it's not maintained well then we should certainly revisit relying on it.

[dcramer]

Also feel free to open an issue discussing SSL, as we're getting pretty far off topic from what this PR is actually about

[tarcieri]

we're getting pretty far off topic from what this PR is actually about

I'm confused. The PR is about removing Certifi, and that's exactly what I'm discussing?

[dcramer]

@tarcieri this PR is explicitly about removing it because it wasn't actually being used. If that's true, it makes sense. If that's not true (i.e. in some situations it was), then I'd be -1 on this PR.

[tarcieri]

Yes, due to a Faraday bug this does not have the behavior you expect.

[dcramer]

Also of note here, is it not at all possible to correct specify the ca file with Faraday? This effectively removes functionality (beyond just removing Certifi). Do you know which version(s) of faraday were affected by this? I'm fairly certain at some point this was working.

[zanker]

@dcramer I tested on 0.9.1 which was released back in January. The relevant code is: https://github.com/lostisland/faraday/blob/master/lib/faraday/adapter/net_http.rb#L98. Faraday sets cert_store to a default OpenSSL one. Net:HTTP is the default adapter in Faraday, https://github.com/lostisland/faraday/blob/master/lib/faraday.rb#L105

Net::HTTP ignores ca_file/ca_path if cert_store is set. I can't point you to the relevant code in Net::HTTP, since it's a monster. I confirmed it just by testing removing cert_store and our cert verification fails. Which is what I expect since the Certifi certs don't include our custom CA (while the system ones do).

[zanker]

@dcramer for your concern on breaking things:

I can't give you a 100% guarantee it won't break things. It's a standard thing to do in Ruby though, some references: Bugsnag, Excon, Net::HTTP.

If their SSL certs are busted, Net::HTTP at the very least will break, along with most other gems doing networking regardless of whether Raven bundles it since they rely on Net::HTTP.

Alternatively, if you really do not want to risk breaking things, how about checking if the default SSL cert exists? That may still break things, but it means we use the system CAs when available and only use Certifi as a fallback. I can make the change, but it would effectively be:

unless File.exist?(OpenSSL::X509::DEFAULT_CERT_FILE)
  self.ssl_ca_file = Certifi.where
end

[dcramer]

Quick QA on this confirms the change doesn't break major compatibility. It's possible there are versions out there that that's not true, but I'm having a hard time tracing any of this code.

[dcramer]

@zanker Thanks!

[tarcieri]

Thank you!

[dcramer]

One quick follow-up here that we should take here: if Certifi wasn't working, it'd suggest ssl_ca_file isn't even functional. Questioning whether we should remove that or implement the full range of options.

[kuzmik]

👍

[zanker]

You could change the adapter in Faraday to one that doesn't have the bug, or set cert_store instead of ca_file.

My recommend would be to drop Faraday, given your HTTP code is pretty straightforward. I'm happy to contribute a PR to change it out to Net::HTTP, so the gem could be dropped if you're interested @dcramer.

I submitted a PR lostisland/faraday#504 on Faraday to see if we can get it fixed.

[dcramer]

I'm pretty much a passenger seat maintainer, but I'm never opposed to removing complex dependencies in favor of more direct simple solutions. I'll defer to @nateberkopec but I assume no one would be opposed.

[sluukkonen]

As a user, I'd like Raven to keep using Faraday. I'm using Raven with Manticore as my Faraday adapter, as it properly supports keepalive connections and has robust connection pooling. Without it, the overhead from continuous TCP/TLS initialization can be fairly large.

[nateberkopec]

@zanker I'm skeptical about the benefits (if we already have the infrastructure in place to support Faraday, why remove it?) and Net::HTTP is quite janky as a library. That said, I'm still interested in a PR.