1360404 - Accept credentials in proxyInfo object (non-SOCKS)

Status: RESOLVED WONTFIX

(Core :: Networking, enhancement, P3)

Description

[Eric Jung [:ericjung]]

Attachment: diff.patch

I am not sure why bug 1200802 only permitted username/password credentials in newProxyInfoWithAuth() for SOCKS proxy servers, but we need it for HTTP and HTTPS proxy servers for bug 1359543.

This should be a trivial fix. Patch attached but untested. I'm first looking for the green light that a change like this would even be considered.

It is needed in bug 1359543 because WebExtensions wants to permit addons to specify proxy username/password in the same function in which they specify proxy server host and port (unlike legacy addons where the username/password must be specified in the addon's own nsIAuthPromptProvider implementation).

Comment 1

[u408661]

Seems to me like this is something we'd take (I haven't looked at the code yet at all). Let's ask the module owner what he thinks!

Comment 2

[Patrick McManus [:mcmanus]]

the reason is buried in https://bugzilla.mozilla.org/show_bug.cgi?id=1200802#c5

the idl is missing the authorization type (digest/basic/ntlm..) and the tor folks doing that patch weren't interested.. maybe it all works in http if you wait for the 401 and then replay request, though a 401 shouldn't be absolutely required.

before just taking the patch you would need to show what happens there and it would be better if you could supply the type to avoid the 401.

Comment 3

[Eric Jung [:ericjung]]

I can tell you from experience that the 407 is avoided if the HTTP authorization headers are sent with the initial request and contain valid credentials. But you are right; we won't know which auth type to specify since this is not in the idl.

>before just taking the patch you would need to show what happens there
Do you need unit tests for this or can I show it with live examples (requests/responses to HTTP and HTTPS proxy servers), or something else?

>and it would be better if you could supply the type to avoid the 401.
Agreed. Otherwise, I imagine WebExtensions will need to install its own nsIAuthPromptProvider implementation (to hide this from WebExtension addon developers), or we scrap bug 1359543. So if this means an idl change, what is the preferred approach? An optional argument somewhere in the newProxyInfoWithAuth() signature? Create a yet another function e.g. newHttpProxyInfoWithAuth()? Something else?

Changing the code to accept the argument is something I can handle, but changing the code to actually make use of the userame/password is out of my league and something I'll need help with. I went through nsProtocolProxyService.cpp (I've looked at this file many times over the years), but it's not obvious to me if such changes need to be done there and/or elsewhere.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Eric H. Jung from comment #3)
> I can tell you from experience that the 407 is avoided if the HTTP
> authorization headers are sent with the initial request and contain valid
> credentials. But you are right; we won't know which auth type to specify
> since this is not in the idl.
> 
> >before just taking the patch you would need to show what happens there
> Do you need unit tests for this or can I show it with live examples
> (requests/responses to HTTP and HTTPS proxy servers), or something else?
> 

I think I was reacting to the "untested" part of your original comment - it sounded like you write the patch and never tried to observe what it does :).. does this make authentication happen after a 401?


> >and it would be better if you could supply the type to avoid the 401.
> Agreed. Otherwise, I imagine WebExtensions will need to install its own
> nsIAuthPromptProvider implementation (to hide this from WebExtension addon
> developers), or we scrap bug 1359543. So if this means an idl change, what
> is the preferred approach? An optional argument somewhere in the
> newProxyInfoWithAuth() signature? Create a yet another function e.g.
> newHttpProxyInfoWithAuth()? Something else?
> 

what if we just make the type argument to the existing function be http:basic, http:digest, http (the latter of which would require a 401..)

would that work?



> Changing the code to accept the argument is something I can handle, but
> changing the code to actually make use of the userame/password is out of my
> league and something I'll need help with. I went through
> nsProtocolProxyService.cpp (I've looked at this file many times over the
> years), but it's not obvious to me if such changes need to be done there
> and/or elsewhere.

Comment 5

[Eric Jung [:ericjung]]

>it sounded like you write the patch and never tried to observe what it does :).. does this make authentication happen after a 401?
It had better. I'll test it and report back.

>what if we just make the type argument to the existing function be http:basic, http:digest, http (the latter of which would require a 401..)

>would that work?

Yes, I think so.

Comment 6

[Eric Jung [:ericjung]]

I'm kind of at a loss as to where to parse out the auth type (i think we need NTLM, too, by the way) and what to do with it. I can do it in nsProtocolProxyService::NewProxyInfoWithAuth() but that just calls nsProtocolProxyService::NewProxyInfo_Internal(). So I can do it there, but then what? Put it into the nsProxyInfo() object in that method? (which would require changing that data structure). I can't actually find where this info (as well as username/password) is used when talking to the proxy servers. Please help.

Comment 7

[Eric Jung [:ericjung]]

This bug can be greatly simplified. If we wait for the 407 from the server, then we'll know the proxy auth type required since  it's included in the Proxy-Authenticate header. Therefore, the type argument doesn't need to be overloaded with an authorization type (and clients generally don't know and don't care what auth type a server requires). Notice, for example, the Firefox GUI does not allow users to specify proxy auth type.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authenticate

Comment 8

[Shane Caraveo (:mixedpuppy)]

Eric,
Other than optimizing and avoiding the 407 in the first place, is there a real need to address this bug?  Given auth works via webrequest, and that is now further fixed in bug 1388289, it seems like we're good to go for auth and we can optimize later.

Comment 9

[Firefox Bug Husbandry Bot]

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

Comment 10

[Eric Jung [:ericjung]]

> Other than optimizing and avoiding the 407 in the first place, is there a real need to address this bug?
> Given auth works via webrequest, and that is now further fixed in bug 1388289, it seems like we're good to go for auth
> and we can optimize later.

As I've written in other bugs and in many other places, forcing proxy addons to do auth outside of the sandboxed FindProxyForURL() script is immensely inconvenient: it means we have to keep auth info (potentially for many different proxy servers) in two different contexts, and synchronize the two when the user changes them. The sandboxing and synchronous nature of FindProxyForURL() is the real issue, of course. And we've discussed that, too, ad nauseum I think.

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to Eric Jung [:ericjung] from comment #10)
> > Other than optimizing and avoiding the 407 in the first place, is there a real need to address this bug?
> > Given auth works via webrequest, and that is now further fixed in bug 1388289, it seems like we're good to go for auth
> > and we can optimize later.
> 
> As I've written in other bugs and in many other places, forcing proxy addons
> to do auth outside of the sandboxed FindProxyForURL() script is immensely
> inconvenient: it means we have to keep auth info (potentially for many
> different proxy servers) in two different contexts, and synchronize the two
> when the user changes them. The sandboxing and synchronous nature of
> FindProxyForURL() is the real issue, of course. And we've discussed that,
> too, ad nauseum I think.

That is an issue better fixed by addressing Bug 1152332.  We could then have a proxy.onResolve function that can be used in the background script.

Regarding this bug and use via webextension PAC scripts:

The nsIProxyInfo data we return from the PAC script can now contain the username/password (though it's only used for socks). We should wait for the 407 so we can do auth correctly based on what the server asks for (digest/etc.).  The extra round trip is not that big of a deal as it should only happen when auth is needed.  Then wherever we get cached credentials for setting the proxy auth header, we should also check for username/password on the proxyinfo instance that was used for the proxy connection.

Doing this may be easier than addressing bug 1152332, however that must be done regardless to get rid of synchronous code paths into extensions (the ultimate reason for the problems that Eric is describing above).

Comment 12

[Shane Caraveo (:mixedpuppy)]

Once bug 1409878 is done, I'm not seeing a need for this, closing.