search

iamstuartwilson / strava

PHP Class for the Strava API (v3)
MIT License
120 stars 26 forks source link

Enable CURLOPT_SSL_VERIFYPEER option #18

Open mjaschen opened 8 years ago

mjaschen commented 8 years ago

CURLOPT_SSL_VERIFYPEER shouldn't be set to false as this possibly weakens security.

Niellles commented 6 years ago

Any reason for this not to be implemented yet?

mjaschen commented 6 years ago

It's not backward compatible because it possibly breaks existing installations. Therefore it's a feature for the next major release (2.0).

Niellles commented 6 years ago

I don't see how this will break existing installations, except for those that don't have a correct/complete/up-to-date certificate bundle. That's not so much breaking the installation of this repo as it is a poorly configured php/curl installation. The effect will be the same though and this is probably a good reason to wait for a major release.

Or am I missing something here?

mjaschen commented 6 years ago

You're right, but we cannot assume that the CA bundle is up to date everywhere.

Enforcing certificate validation without shipping a current CA bundle will possibly break installations.

Niellles commented 6 years ago

Fair enough.

How about a temporary fix, that won't break anything in existing installations, that will attempt to verify the certificate and will do the request without verification if it fails. Also it can be easily removed in a future release without breaking backwards compatibility.

For testing purposes I've edited my php.ini to the following -- and it indeed throws an Exception now. Since I am running this on a Windows machine it's probably the worst case scenario, although I am not that familiar with certificates and CURL:

[openssl]
; The location of a Certificate Authority (CA) file on the local filesystem
; to use when verifying the identity of SSL/TLS peers. Most users should
; not specify a value for this directive as PHP will attempt to use the
; OS-managed cert stores in its absence. If specified, this value may still
; be overridden on a per-stream basis via the "cafile" SSL stream context
; option.
;openssl.cafile=
; If openssl.cafile is not specified or if the CA file is not found, the
; directory pointed to by openssl.capath is searched for a suitable
; certificate. This value must be a correctly hashed certificate directory.
; Most users should not specify a value for this directive as PHP will
; attempt to use the OS-managed cert stores in its absence. If specified,
; this value may still be overridden on a per-stream basis via the "capath"
; SSL stream context option.
;openssl.capath=

Suggested fixes:

  1. verifypeer as argument for method.
  2. verifypeer as class property (preferred fix.)

CURL doesn't throw an Exception upon error so I compared the error message as a string, not great practice, but should work if this is indeed the error (and the only error) that will be caused by a missing CA bundle. Added commit to tackle this.