WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#8702 closed enhancement (fixed)

HTTP API Updates

Reported by: sivel Owned by: westi
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7
Component: HTTP API Keywords: has-patch tested commit
Focuses: Cc:

Description

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.

Attachments (6)

8702.diff (5.0 KB) - added by sivel 12 years ago.
8702.2.diff (5.1 KB) - added by sivel 12 years ago.
Straight update of the code for the current trunk revision (r10374)
8702.3.diff (6.5 KB) - added by sivel 12 years ago.
8702.4.diff (6.7 KB) - added by sivel 12 years ago.
8702.5.diff (6.7 KB) - added by sivel 12 years ago.
8702.6.diff (6.3 KB) - added by sivel 12 years ago.

Download all attachments as: .zip

Change History (33)

@sivel
12 years ago

#1 @jacobsantos
12 years ago

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

#2 @jacobsantos
12 years ago

  • Component changed from General to HTTP
  • Owner anonymous deleted

Why am I CCed on this?

#3 @jacobsantos
12 years ago

  • Cc wordpress@… removed

#4 @sivel
12 years ago

  • Keywords needs-testing removed

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.

#5 @jacobsantos
12 years ago

  • Keywords tested commit added

#6 @westi
12 years ago

  • Milestone changed from 2.7.1 to 2.8
  • Owner set to westi
  • Status changed from new to assigned

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.

#7 @DD32
12 years ago

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

#8 @sivel
12 years ago

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.

#9 @DD32
12 years ago

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.

#10 @sivel
12 years ago

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
12 years ago

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

#11 @sivel
12 years ago

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.

#12 follow-up: @sivel
12 years ago

  • Keywords needs-testing added; tested commit removed

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.

#13 in reply to: ↑ 12 @westi
12 years ago

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
12 years ago

#14 @sivel
12 years ago

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
12 years ago

#15 @sivel
12 years ago

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.

#16 @jacobsantos
12 years ago

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

#17 follow-up: @sivel
12 years ago

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

#18 in reply to: ↑ 17 @jacobsantos
12 years ago

Replying to sivel:

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

Yes.

@sivel
12 years ago

#19 @sivel
12 years ago

  • Keywords tested commit added; needs-testing removed

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.

#20 @sivel
12 years ago

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
12 years ago

#21 @sivel
12 years ago

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

#22 @ryan
12 years ago

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

#23 @ryan
12 years ago

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

#24 @ryan
12 years ago

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

#25 @ryan
12 years ago

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

#26 @azaozz
12 years ago

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

#27 @sivel
12 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

this has been committed. closing.

Note: See TracTickets for help on using tickets.