Accept credentials in proxyInfo object (non-SOCKS)

NEW
Unassigned

Status

()

Core
Networking
P1
normal
4 months ago
5 days ago

People

(Reporter: ericjung, Unassigned, NeedInfo)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-active][proxy])

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Created attachment 8862686 [details] [diff] [review]
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).
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!
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
(Reporter)

Comment 3

4 months ago
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.
(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.
(Reporter)

Comment 5

4 months ago
>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.
(Reporter)

Comment 6

4 months ago
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.
Flags: needinfo?(mcmanus)
Assignee: nobody → eric
Whiteboard: [necko-active]
Status: NEW → ASSIGNED
(Reporter)

Updated

3 months ago
Status: ASSIGNED → NEW
(Reporter)

Updated

3 months ago
Blocks: 1359543
Whiteboard: [necko-active] → [necko-active][proxy]
(Reporter)

Updated

a month ago
Blocks: 1381290
(Reporter)

Updated

a month ago
Blocks: 1283639
Priority: -- → P1

Updated

a month ago
Assignee: eric → nobody
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 7

27 days ago
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
Flags: needinfo?(mcmanus)
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.
Flags: needinfo?(mixedpuppy) → needinfo?(eric)
You need to log in before you can comment on or make changes to this bug.