575928 - "http-on-modify-request" doesn't work on "CONNECT" method

Status: NEW

(Core :: Networking: Proxy, defect, P3)

Description

[Hugues]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008110808 Ubuntu/7.10 (gutsy) Firefox/3.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008110808 Ubuntu/7.10 (gutsy) Firefox/3.0.3

In case pass through a proxy via https, a http "CONNECT" method is use (RFC 2817).The "http-on-modify-request" doesn't apply in this case. The fact is "nsHttpConnection.cpp" doesn't treat it.

Reproducible: Always

Steps to Reproduce:
1. Configure Firefox to Pass through a proxy.
2. Add your own HTTP headers to any request the application makes. ("http-on-modify-request" via add-ons).
3. Go to an https site ( CONNECT method).
Actual Results:  
Never add your own HTTP headers.

Expected Results:  
Add you own HTTP headers.

The fact is "nsHttpConnection.cpp" doesn't treat it.


Networks Filter addons (liveHTTPHeaders, httpfox ...) don't work in this case to.
Find this user note in liveHTTPHeaders :
"This is an awesome tool for looking at https headers. I work in Tech Support for a proxy product and we noticed that connect requests are not captured. It would certainly be helpful to have those requests/replies as well. (For those that do look at https traffic, the connect requests are in the clear and can be seen with packet analyzers. It is after the connect request/connection established that everything is encrypted and LiveHTTPHeaders is so helpful.)"

Comment 1

[Hugues]

Attachment: Work with this patch

Comment 2

[Hugues]

Attachment: This is for test in Add-ons

Comment 3

[Jason Duell]

Comment on attachment 455098 [details] [diff] [review]
Work with this patch

Hugues,

Why do you need the "nsShellHttpChannel" class?  It doesn't seem to do anything besides proxy all calls to the nsHttpChannel.  Can you not call pObsService->NotifyObservers with the original nsHttpChannel?

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to comment #3)
> Comment on attachment 455098 [details] [diff] [review]
> Work with this patch
> 
> Hugues,
> 
> Why do you need the "nsShellHttpChannel" class?  It doesn't seem to do anything
> besides proxy all calls to the nsHttpChannel.  Can you not call
> pObsService->NotifyObservers with the original nsHttpChannel?

Hugues can you anwser please ?

Comment 5

[Hugues]

Hi Jason,

sorry for the late answer.
I'm not expert in firefox C++ code, this patch have been developed with Mr Rouget.
It works fine since 3 years now but doesn't seem to work with Firefox 3.6.X anymore. 
I have post this patch to illustrate the functionality. This bug can be correct.
Sure this patch is not perfect and should be improve, you've certainely have better idea on how to use nsHttpChannel class.

Comment 6

[Bjarne (:bjarne)]

Comment on attachment 455098 [details] [diff] [review]
Work with this patch

Seems to me like this patch is simply triggering notifications of "http-on-modify-request" observers from the nsHttpConnection-object. The nsShellHttpChannel class is added to transfer http-headers between the connection-object and each observer.

I'm giving this an r- for two reasons: First, nsHttpHandler is available as nsHttpConnection::gObserverService and it is probably safer to use its OnModifyRequest() method here. Second (and most important) is that this fake channel does not represent the real channel. This reduces the value of the parameter and may actually confuse any observer actually using the channel for something else than setting headers (e.g. if the channel is examined for certain properties before setting headers). I believe it is possible to get to the real channel via trans::mChannel and a QI (keeping potential e10s-issues in mind).

Comment 7

[Etienne Goyer]

I was able to make the patch apply cleanly on 3.6.  Applying that patch to 3.6 is not the problem specifically (not anymore, at least!)

Better would be to do without the patch entirely.  To be able to do so, we would need to inject a custom header in a CONNECT connection that the proxy can read (ie, not in the SSL-encrypted connection to the server!).  Is there any API we can use to do that from an extension?

Comment 8

[Bjarne (:bjarne)]

Requesting blocking-status for 1.9.2 and/or 2.0 . If approved, I can help out with a patch as outlined in comment #6.

Comment 9

[Etienne Goyer]

Bjarne, thanks a whole lot for helping with that.  Just to be clear, I am not the author of the original patch, and I am not familiar with Mozilla code base.

That being said, how do you envision we should address the problem?  I have to admit the explanation you gave in comment 6 where a bit over my head.  Shall the connection between the client and the proxy be considered an actual nsHttpChannel, and should it notify observers of all the topic (http-on-modify-request, http-on-examine-response, etc)?

thanks again!

Comment 10

[Daniel Veditz [:dveditz]]

Doesn't meet branch blocking criteria. If you come up with a safe and tested (on trunk) patch with low risk commensurate with the gain we can consider approving it for a branch release.

Comment 11

[Johnny Stenback (:jst)]

Not blocking the release on this.

Comment 12

[Etienne Goyer]

Bjarne, is there still interest on your part to work on this bug?  I understand the patch needs work, and your suggestions have been noted, but I am not really sure how to get started with that.

Comment 13

[Bjarne (:bjarne)]

I can see the value of passing http-headers to the proxy itself (as opposed to to the target-server). However, I'm not sure that a "http-on-modify-request" notification is the correct mechanism. The proposed patch sends this notification from a layer below the http-channel, and I have a bad feeling about doing that. (I'm not aware of any other existing mechanisms to achieve this either, unfortunately.)

Jason - any input? Do we want to send notifications normally coming from nsHttpChannel from the socket-layer?

Comment 14

[Bjarne (:bjarne)]

A constructive approach could be to introduce a notification "http-on-proxy-connect" (or similar)  triggered by the connection-layer, which would be a clean enhancement (as opposed to a bug). This would eliminate potential for confusion in existing "http-on-modify-request"-observers. Not sure about policies for adding stuff like this though.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I don't think there's a policy.  If it would be useful and complements our other notifications we should add it.

Comment 16

[Bjarne (:bjarne)]

Etienne, Hugues: would the approach from comment #14 make sense to you? Moreover, what information would be necessary to pass and what would you expect to do with it?

Afaics, your current observer is passed a fake http channel which allows you to examine the uri, request method and headers, and to set headers (all info relevant to the proxy-request, not the actual request). Is this what you need?

Comment 17

[Etienne Goyer]

Bjarne, that's exactly it.  We're not dead-set on using the http-on-modify-request notification; a new http-on-proxy-connect notification would be just as good for our purpose.  In fact, it would actually work better for our use-case.

Basically, we need to examine the HTTP headers, and set some that are visible to the proxy.

Comment 18

[Bjarne (:bjarne)]

Ok - I've spent some time on this one (stumbling across an astonishing number of other issues with proxies, but that's another story) and believe I've found something which may work for you. I'll attach a WIP patch shortly, but the basic idea is to dispatch a "http-on-proxy-connect" notification which carries a pointer to info about the request to the proxy (I call it nsIHttpRequestInfo).

In the case of a plain http-request, nsIHttpRequestInfo represents the request to the server. In the case of a https-request, nsIHttpRequestInfo represents the headers sent to the proxy with the CONNECT-method (like in your original patch).

nsIHttpRequestInfo so far expose the following (extracted from IDL)

   readonly attribute ACString URI;
   ACString getHeader(in ACString aHeader);
   void setHeader(in ACString aHeader, in ACString aValue, in boolean aMerge);

Refer to nsIChannel and nsIHttpChannel for semantics.

Comment 19

[Bjarne (:bjarne)]

Attachment: WIP with test/demo

This is current thinking. Most changes comes from making |nsHttpChannel::mRequestHeader| a pointer.

The test/demo assumes a proxy running on localhost port 54321 - change the PAC script in the test if necessary. It should also be able to load from localhost using https, but I have not yet bothered to figure out how so I load from bugzilla instead.

Etienne, Hugues: Feedback on how this may work for you is appreciated.

Comment 20

[Bjarne (:bjarne)]

Attachment: WIP with test/demo (right file this time)

Wonder what happened to the previous upload...  :(

Comment 21

[Hugues]

Attachment: Patch & Compilation

Bjarne, thanks a lot for your work.

Don't know if i am right, but it seems to have problems with compilation.
Give you errors in attachment.

Comment 22

[Bjarne (:bjarne)]

Attachment: WIP with test/demo (compiling on recent trunk)

Weird - something must have happened on trunk in the meantime. Unbitrotted version...

Comment 23

[Hugues]

Patch works fine for me.Thanks again..
Hope this functionality will be included in firefox-4.0 ?

Comment 24

[Bjarne (:bjarne)]

Good to hear it works for you! Please get back here if there is anything that should be done different or better.

There are a few obstacles to pass it will be considered for 4.0, I'm afraid. :) I'm starting the process by requesting review and pushing it to tryserver.

Comment 25

[Honza Bambas (:mayhemer)]

I'd rather see:
- don't change mRequestHead to be allocated dynamically, leave it as is
- your new interface should only implement a wrapper that holds a ref to the request head and call on it, you work with it synchronously
- however, I don't much like call through a sync proxy ; that may lead to respin of the socket event queue and potential re-entrance of the same object or other quit unexpected issues... we are not doing this for safety reasons in the whole necko code

The last point means a more work to do.  I believe you have to asynchronously wait for the response of the callback... what means a potential delay for every request going through a proxy and more code complexity, that is undesirable.

Or, could we do the notification from nsHttpChannel, on the main thread, as soon as we know there will be a proxy request?  Ability of that notification to modify the request might be limited, but it might at least set header(s) to be appended to the proxy request, but probably not read existing headers in case of SSL proxy CONNECT request.

Hugues, would that be OK for you?

Comment 26

[Bjarne (:bjarne)]

> - don't change mRequestHead to be allocated dynamically, leave it as is

Any particular reason why?

> Or, could we do the notification from nsHttpChannel, on the main thread, as
> soon as we know there will be a proxy request?

That's a thought..  how would we know for the CONNECT-case?

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to comment #26)
> > - don't change mRequestHead to be allocated dynamically, leave it as is
> 
> Any particular reason why?

It raises a risk of null-dereferencing.  Also, as I suggested the wrapper class, it is actually unneeded.

> 
> > Or, could we do the notification from nsHttpChannel, on the main thread, as
> > soon as we know there will be a proxy request?
> 
> That's a thought..  how would we know for the CONNECT-case?

if (mConnectionInfo->UsingSSL() && mConnectionInfo->UsingHttpProxy()).  And if (mConnectionInfo->UsingHttpProxy()) for a general proxy usage?  Maybe this needs to verify, but I think it is what is sufficient to decide on the notification to be triggered.

My idea was to let the header changes be collected in the notification (kept by the wrapper) and then send them to the lower level (nsHttpTransaction -> nsHttpConnection) to let it be merged with headers of the CONNECT requests.  It would be better if nsHttpConnection grabs the added/modified headers when needs it, best via callbacks or other mechanism.

Comment 28

[Hugues]

Hi Bjarne,

I have test the patch in Firefox 7.0.1, unfortunaly it doen't run.
It was the case with firefox 4 but there wasn't a lot of change to do in patch to make it works.
It's different with Firefox 7, There is a lot of change in source code, particulary in nsHttpConnection.cpp. I didn't succeded to make it works.

It's a shame that your patch hasn't been included in mozilla-release. All changes in protocol source code since firefox 4 should be an occasion to integrate this functionality. Can you help me to upgrade the patch ??

regards

Comment 29

[Bjarne (:bjarne)]

-> default owner

Comment 30

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 31

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 32

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.