Closed
Bug 1410257
Opened 7 years ago
Closed 7 years ago
Handle the case when the server closes a connection between 401/7 and our authentication request for any schema (and not only connection-based)
Categories
(Core :: Networking: HTTP, defect, P2)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: mixedpuppy, Assigned: xeonchen)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-triaged] )
Attachments
(4 files)
Any pre-loaded secure page that gets reloaded after changing the proxy returned by a PAC script will fail to reload.
STR from reporter with added notes from me (preceded with dash):
1) Open an about:debugging tab, and a tab to a website (I used https://news.ycombinator.com/).
2) In about:debugging, load the test extension.
3) Go to the other tab and reload the page. You should notice the "Secure Connection Failed" screen. If you press reload again, the page loads fine.
- if I reload an http tab first, this will not happen, the https page will reload just fine.
- intermittently, the page just appears to be taking a long time to load, you never get the error page.
- using the awesome bar (focus then hit enter) the page loads fine after a proxy switch
- if I switch proxy enough times (5) that the PAC script cycles back to the first proxy again, it no longer fails, so it seems to be only when introducing a new/unused proxy and reloading the page that had been loaded via a proxy.
4) In the extension popup, click Switch Proxy. Reload the news.ycombinator page again, and notice the "Secure Connection Failed" screen again. Another reload, and the error screen disappears.
5) Continue repeating step 4 until you cycle through all the proxies in proxy-script.js (there are 4). Once you cycle through all of them once, you are able to switch proxies and reload without the error screen appearing.
The PAC script looks like this:
const proxyStrings = [ "HTTPS site1:8080; DIRECT",
"HTTPS site2:8080; DIRECT",
"HTTPS site3:8080; DIRECT",
"HTTPS site4:8080; DIRECT" ];
var currentProxy = 0;
browser.runtime.onMessage.addListener((msg, sender) => {
if (msg.command === "changeProxy") {
currentProxy = (currentProxy + 1) % proxyStrings.length;
}
});
function FindProxyForURL(url, host) {
return proxyStrings[currentProxy];
}
Reporter | ||
Comment 1•7 years ago
|
||
Since the extension the report sent has open proxy servers in it, I'm not attaching it. I've asked if they mind posting it here, otherwise I'll forward it to anyone internal that wants to see it.
Reporter | ||
Updated•7 years ago
|
Blocks: webextensions-proxy-api
Comment 2•7 years ago
|
||
@xeonchen can you take a look at this issue first?
Flags: needinfo?(xeonchen)
Priority: -- → P2
Whiteboard: [necko-triaged]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → xeonchen
Flags: needinfo?(xeonchen)
Reporter | ||
Comment 3•7 years ago
|
||
Here's the test addon, it uses test credentials for the proxy that will be deleted once it is unnecessary.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
This is a HTTPS proxy requires authentication, and it did some authentication steps.
But somehow it failed to use the connection.
Honza may have some idea?
Flags: needinfo?(honzab.moz)
Comment 6•7 years ago
|
||
This doesn't seem like a duplicate of bug 1409858.
For the attempt to auth to the proxy (an answer to 407) I can see in the log that the socket (to proxy ip-165-227-29-95.lazerpenguin.com:8080) is gracefully closed after we send out the second transaction (an answer to the 407):
2017-10-21 01:59:59.206 │ Socket Thread │ nsSocketInputStream::Read [this=0x118deaa78 count=32768]
2017-10-21 01:59:59.206 │ Socket Thread │ calling PR_Read [count=32768]
2017-10-21 01:59:59.206 │ Socket Thread │ PR_Read returned [n=0]
2017-10-21 01:59:59.206 │ Socket Thread │ nsHttpConnection::OnSocketReadable 0x118de4800 trans->ws rv=0 n=0 socketin=80470002
2017-10-21 01:59:59.206 │ Socket Thread │ nsHttpConnection::CloseTransaction[this=0x118de4800 trans=0x118dea400 reason=80470002]
The transaction doesn't take it as a restart but as an h.9 response:
2017-10-21 01:59:59.206 │ Socket Thread │ nsHttpTransaction::Close [this=0x118dea400 reason=0]
2017-10-21 01:59:59.206 │ Socket Thread │ nsHttpConnectionMgr::RemoveActiveTransaction t=0x118dea400 tabid=200000001(1) thr=0
2017-10-21 01:59:59.206 │ Socket Thread │ nsHttpTransaction::Close 0x118dea400 0 Byte 0.9 Response
This closes the trans with NET_RESET according [1]
But it's not restarted in Close() because of NS_HTTP_STICKY_CONNECTION trans cap. I believe the connection could be even in case of an http basic/digest auth restartable after the first 401/7.
Hence, the fix here could be to remove 'mConnectionBased &&' at [2]. I *think* I was playing safe when the condition was built as 'mConnectionBased && !authAtProgress' which only allowed restart of a connection that was asked to auth with NTLM/Nego/Kerberos auth schemes. But the same can IMO apply to any auth scheme.
This opens the question if using the NS_HTTP_STICKY_CONNECTION flag for auth restart transaction makes sense. There is no comment that would explain it. The code is there for years and I don't have time to dig in CVS to find the bug it has been added there and try to find out why (maybe just some crappy server or proxy in those early days?)
[1] https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/netwerk/protocol/http/nsHttpTransaction.cpp#1143
[2] https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#838
Flags: needinfo?(honzab.moz)
Assignee | ||
Updated•7 years ago
|
Assignee: xeonchen → honzab.moz
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #6)
> Hence, the fix here could be to remove 'mConnectionBased &&' at [2]. I
> *think* I was playing safe when the condition was built as 'mConnectionBased
> && !authAtProgress' which only allowed restart of a connection that was
> asked to auth with NTLM/Nego/Kerberos auth schemes. But the same can IMO
> apply to any auth scheme.
Yes, this fixes the issue. I guess you might want to take this :)
Comment 9•7 years ago
|
||
(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #8)
> Yes, this fixes the issue. I guess you might want to take this :)
I can also just review it :)
Assignee: honzab.moz → xeonchen
Reporter | ||
Comment 10•7 years ago
|
||
It would be great to see this land for 58. I came up with a very hacky workaround for the proxy extension that ran into this, but it is *very* hacky.
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Comment 11•7 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> It would be great to see this land for 58. I came up with a very hacky
> workaround for the proxy extension that ran into this, but it is *very*
> hacky.
This could also be fixed on the proxy side - just support and do connection keep-alive.
Comment 12•7 years ago
|
||
And I think the problem will be present even when there is no extension, when the proxy is set manually.
Summary: dynamically changing proxy filter results in "secure connection failed" → Handle the case when the server closes a connection between 401/7 and our authentication request for any schema (and not only connection-based)
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8922217 [details]
Bug 1410257 - make non-connection-based connections restartable;
https://reviewboard.mozilla.org/r/193236/#review198538
Thanks Gary! Please push to try before landing. Note that this has to ride the trains. I'm a bit worried about this change...
Attachment #8922217 -
Flags: review?(honzab.moz) → review+
Comment 15•7 years ago
|
||
Pushed by gachen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f34c0436828
make non-connection-based connections restartable; r=mayhemer
Comment 16•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•