Do not use :insecure option by default in Patron

[stayhero]

The session.insecure option is set to true by default. This disables
the ssl verification of the host and, according to the Patron
documentation, one should "think twice before using this option".

See also
http://www.rubydoc.info/gems/patron/0.8.0/Patron%2FSession%3Ainsecure

[stayhero]

I came across this default setting when I tried to use Patron with a proxy (and fighting with SSL cert verification issues). As far as I'm aware, I guess the insecure option shouldn't be enabled by default.

Unfortunately it was added in 2012 already:
db73953

Therefore I'm not sure if reverting this commit would break some production apps. But from a security standpoint, it should be disabled, I guess. 🙈

[stayhero]

😢 The CI tests are failing while my local tests went through. It seems the CI server has some CA certs missing for the ssl certificates issued for example.com. Still, I think just disabling SSL verification by default is a rather bad idea.

I'm not very familiar with Travis. Probably that could be solved by adding something like that to travis.yml (see travis-ci/travis-ci#6142)?

addons:
  apt:
    packages:
      - ca-certificates

[iMacTia]

Hi @stayhero and thanks for the Issue report.
I can see the problem here and I suspect that was due to tests failing on Travis.
I agree that's not a good reason to decrease the security level, so what we can do is to call the #insecure method only when running tests.
You can easily do that using the config_block, (see https://github.com/lostisland/faraday#patron).

The only other thing that scares me is the backward-compatibility, but we'll figure that out later on. Let's get tests green again first!

[stayhero]

@iMacTia It seems the test were failing because the ca_file option was not supported by the adapter. Because the integration tests itself supply a valid certificate for the localhost server. The tests also disable the ssl verification, but this was not supported by the Patron adapter as well.

Now the tests should work and the insecure option is supported properly. Regarding the backwards-compatibility: I think most users will appreciate the security improvement, otherwise using the Faraday option ssl: { verify: false } should work for Patron now.

[iMacTia]

I see, the adapter was not supporting the ssl: { verify: false } option.
I would definitely consider this a bug then. Sorry for the backward-compatibility but I agree this is something serious that needs to be addressed.

However, does that mean that ca_cert has to be provided unless you set the ssl: { verify: false } option? From my understanding, ca_cert is not always required, e.g. if the server you're reaching has a valid certificate installed you don't need any local certificate.
I think that session.ca_cert should be set:

• If and only if the ssl[:ca_cert] is passed
• Regardless of the ssl[:verify] option

Is my understanding correct?

[stayhero]

I had the same thoughts but Patron raises an error if the cacert is set and the verification is deactivated (i. e. insecure is set to true). Hence we are not allowed to set a cacert and disable ssl verification.

[stayhero]

To make it clear: you don't need to set cacert. I just optimized the additional if away because it should not matter if you set session.cacert = nil or set it not at all. Surely you could modify the line to something like

session.cacert = ssl[:ca_cert] unless ssl[:ca_cert].nil?

[iMacTia]

I get your point now.
In that case then happy to merge and release with next version 👍
Thanks for the contribution