HTTP API Updates

[sivel]

Changes:

• Allow cURL/pecl_http/Streams to GET/POST/HEAD to site protected by invalid SSL cert, fopen works with invalid SSL certs without configuration. Fsockopen will not work with invalid SSL certs, and does not contain configurations to allow it to do so. Fsockopen is the last transport listed so hopefully an https request using an invalid SSL cert will be handled before fsockopen, otherwise is will just return an error.
• Use timeout argument in cURL for the connect timeout also
• Use true/false in place of numeric boolean values in curl_setopt
• Add support to POST via cURL
• Update default user-agent. See ​http://trac.wordpress.org/changeset/10238 for format.
• Enabling non-blocking request caused curl to make a head request due to curlopt_nobody being set to true.

[jacobsantos]

The commit for r10238 is only for a few use cases, it should not be the default.

[jacobsantos]

Why am I CCed on this?

[sivel]

I've been using this on my site with success since I added the patch. Removing the needs-testing keyword as I think we are good to go.

If needed I can update the patch since I know changes have been made to the http api since I submitted this.

[westi]

I don't think we should default to allowing invalid SSL certs for the transports that support validity checking.

In fact I think we should probably blacklist the ones that accept invalid SSL certs whatever for SSL request.

I am quite happen for it to be filterable but the default should be secure.

This also needs to sit in trunk first.

[DD32]

Do self-signed certs fall into the "invalid cert" bin?

If anything, There should be an option to ignore the certificates authority (That'd need to be set at request time) so that those of us who dont fork out for a cert signed by some company can still rely upon private-secure connections

[sivel]

Self-signed certs would fall into the category of "invalid certs" seeing as though self-signed certs do not come from a trusted CA.

I would rather just ignore the SSL cert validity, but if overall people think we shouldn't then a filter could be added to override this.

[DD32]

I would rather just ignore the SSL cert validity, but if overall people think we shouldn't then a filter could be added to override this.

Not a filter, Simply because that'd end up with people setting it to ignore it for every domain.. Setting a extra arg. during request time would allow it to be set on a per-request basis if the author specifically knows that it will be hitting an invalid cert. Just because a cert is "invalid" it doesnt mean the content behind it isnt trusted, It just depends on what the host is to the owner.

[sivel]

The one problem I see with making it an arg and not a filter is that if your whole WP site is protected by an untrusted SSL cert the calls already built in to core are going to fail. Perhaps an arg and a filter?

[sivel]

Straight update of the code for the current trunk revision (r10374)

[sivel]

New diff for trunk revision. For the time being I have left it as is (referring to SSL). Once more has been discussed something can possibly be done for this.

[sivel]

Okay...here is the new version of this patch.

Changes:

• Added blacklisting of transports if url is https
• Added filter https_local_ssl_verify (used in spawn_cron) so that users can override ssl verification for their site if using an untrusted ssl cert. This insures that cron can still work on an untrusted ssl cert without removing ssl verification for external urls.
• Added filter https_ssl_verify so that users can override ssl verification for all https calls
• Added new default argument 'sslverify' which defaults to true

I have not done much testing on this so any help testing would be appreciated.

This patch fits into the current trunk revision 10446.

[westi]

Replying to sivel:

Okay...here is the new version of this patch.

Changes:

• Added blacklisting of transports if url is https
• Added filter https_local_ssl_verify (used in spawn_cron) so that users can override ssl verification for their site if using an untrusted ssl cert. This insures that cron can still work on an untrusted ssl cert without removing ssl verification for external urls.
• Added filter https_ssl_verify so that users can override ssl verification for all https calls
• Added new default argument 'sslverify' which defaults to true

I have not done much testing on this so any help testing would be appreciated.

This patch fits into the current trunk revision 10446.

Thank you for the updated patch.

As I have said before I much prefer to be secure out of the box but give people the flexibility to go unsecure if they want.

It is good to see an easy option to enable cron to work with a unsigned cert to allow people to self-cert to get https while still having a safe route to getting SSl working.

It would be good to see some testing on this and get it into trunk.

[sivel]

I spent some time doing ssl testing this evening with self signed certs and all appears to be working well by use of the filters.

Note that I updated the 8702.3.diff patch as it had 2 small typos.

I'd appreciate some testing from someone else to make sure I haven't overlooked anything in my testing.

[sivel]

Streams does not allow for ssl validation if not using stream_socket_client to make the connection as far as I have been able to determine. Currently Streams is using fopen which, even when passed verify_peer and verify_host options via stream_context_create, ignores ssl validity.

This patch blacklists streams from making ssl connections.

At this point, if we do not rewrite streams to use stream_socket_client, the only transports capable of handling https are curl and the http extension.

[jacobsantos]

Lets do it. I was not aware stream_socket_client was so awesome and comparable to fsockopen.

[sivel]

Let's do what? Rewrite Streams to use stream_socket_client?

[jacobsantos]

Replying to sivel:

Let's do what? Rewrite Streams to use stream_socket_client?

Yes.

[sivel]

Alright...I have removed the ssl blacklisting for the streams transport and added back in the stream_context_create options for ssl verification.

Jacob Santos will be taking on rewriting the streams transport to use stream_socket_client so that ssl verification will work. His patch will be entered on a separate ticket.

Since no one else has reported on their testing of this patch and I have had time to do more extensive I think this is ready to be committed.

[sivel]

r10474 removes the wp_remote_post to fire off the cron and replaces it with an AJAX call. Removing the https_local_ssl_verify filter since that was the only place it was in use at.

8702.6.diff is the new patch with this removed.

[sivel]

Any chance on moving forward with the commit for this ticket?

[ryan]

(In [10507]) HTTP API updates and fixes. Props sivel. see #8702

[ryan]

(In [10508]) Revert 10507. Had extra bits in. see #8702

[ryan]

(In [10509]) HTTP API updates and fixes. Props sivel. see #8702

[ryan]

Ignore 10507 and 10508, I screwed up and committed extra stuff along with this patch.

[azaozz]

(In [10526]) Add filter for local connection ssl verify to cron spawning, props sivel, see #8702

[sivel]

this has been committed. closing.