Opened 3 years ago

Closed 3 years ago

#10984 closed defect (implemented)

PHP relay for meek

Reported by: arlolra Owned by: dcf
Priority: Medium Milestone:
Component: Obfuscation/meek Version:
Severity: Keywords: meek
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A first pass at the php middle relay is at,
https://github.com/arlolra/meek/tree/php

It borrows heavily from GoAgent.

Deployed to,
https://meek-reflect.herokuapp.com/

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by dcf

  • Component changed from Pluggable transport to meek

comment:2 Changed 3 years ago by dcf

  • Status changed from new to needs_revision

This PHP reflector is undeniably useful. I just used it to prototype a reflector on Azure Websites for #13189.

I'm still doubtful about the quality of code derived from GoAgent and I wish we didn't have to include the huge license. I think we can simplify it quite a bit with a new implementation?

We don't actually have to reflect Content-Type. I think we can replace all the GetHeaders code with just:

$headerArray = array();
if (array_key_exists("HTTP_X_SESSION_ID", $_SERVER)) {
        $headerArray[] = "X-Session-Id: " . $_SERVER["HTTP_X_SESSION_ID"];
}
curl_setopt($ch, CURLOPT_HTTPHEADER, $headerArray);

Likewise, I think you can replace all the method processing code with just:

curl_setopt($ch, CURLOPT_CUSTOMREQUEST, $_SERVER['REQUEST_METHOD']);
curl_setopt($ch, CURLOPT_POSTFIELDS, file_get_contents("php://input"));

I found I had to change $HTTP_RAW_POST_DATA to file_get_contents("php://input") since b1f6a7ec which removed Content-Type from the header.

I was going to suggest using the HTTPS bridge by default, but I tried it and Azure must not have a trusted CA list installed. It prints:

502 Urlfetch Error
PHP Urlfetch Error: curl(60)
SSL certificate problem: unable to get local issuer certificate

It turns out that CURLOPT_SSL_VERIFYHOST is not a boolean, but an integer. 0 means something unsafe, 1 means something unsafe, and 2 is what you want. Unfortunately true is converted to 1. It's used as an example of a bad API in this paper. It looks like PHP has our back though; I saw this message in the log:

PHP Notice:  curl_setopt_array(): CURLOPT_SSL_VERIFYHOST no longer accepts the value 1, value 2 will be used instead in index.php on line 111

Do you think you can get the file down to 50 lines? I think it should be possible and then I'll like it better.

Last edited 3 years ago by dcf (previous) (diff)

comment:3 Changed 3 years ago by arlolra

comment:4 Changed 3 years ago by arlolra

Sorry, wrong commit. See the branch https://github.com/arlolra/meek/tree/php

comment:5 Changed 3 years ago by dcf

  • Status changed from needs_revision to accepted

Great work! It works for me. Please commit it.

It seems to work without this line:

header("Content-Type:text/plain; charset=utf-8");

Let's remove that line, even though without it PHP seems to be sniffing text/html. We'd be better off whitelisting response headers with CURLOPT_HEADERFUNCTION like before. Otherwise we're claiming "UTF-8" even for our POST responses, which doesn't seem to hurt anything but is weird.

Add a little note about the php directory to the main README file.

comment:6 follow-up: Changed 3 years ago by arlolra

We'd be better off whitelisting response headers with CURLOPT_HEADERFUNCTION like before.

Can I put that back?

comment:7 in reply to: ↑ 6 Changed 3 years ago by dcf

Replying to arlolra:

We'd be better off whitelisting response headers with CURLOPT_HEADERFUNCTION like before.

Can I put that back?

Yeah, sure.

comment:8 Changed 3 years ago by arlolra

  • Resolution set to implemented
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.