Closed Bug 1315332 Opened 3 years ago Closed 3 years ago

Getting "The connection was reset" when I don't make it to enter correct NTLM credentials on time

Categories

(Core :: Networking: HTTP, defect, major)

52 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

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

Attachments

(1 file, 1 obsolete file)

I am afraid that the patch for bug 486508 is too aggressive.  When the server closes the connection simply before I fill in the credentials, only result (even the creds are actually correct) is to get an error page with "The connection was reset" message.

We used to restart the transaction on a new connection, but that was faulty in many ways.

To avoid this stupid error page (interesting that IIS server doesn't keep the connection longer anyway...) we need to reinstate restart, but in a smarter way.  Probably by a redirect to the same URL.  Anyway, not that easy to fix.
Good news is that just pressing the big "Try Again" button fixes the problem - creds are cached.  But we should do this "try" internally for the user.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [ntlm][necko-next] → [ntlm][necko-active]
Attached patch wip1 (obsolete) — Splinter Review
This patch is incomplete, since before the seconds try we don't reach GenCredsAndSetEntry that would make the second try use cached credentials w/o an auth dialog.  Hence, the auth dialog pops up twice anyway and if users retype their username/password (believing the creds were passed wrong) they just may get into the same loop again.

I'm not sure what is the right fix here and maybe this is just a WONTFIX.  It's easy to press [F5] and [enter] to re-send the user name and password stored in the password manager and see that the server is the one that reset the connection.

Putting the patch here just for reference.  I don't want to make the code around NTLM be more overcomplicated.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Whiteboard: [ntlm][necko-active] → [ntlm][necko-would-take]
I found a simple solution!
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [ntlm][necko-would-take] → [ntlm][necko-active]
- this is using api 1309438 in bug 1309438
- it seems to be OK (at least with IIS) to send the first round response (message type 1) on a new connection ; hence when we ask user for creds and the schema is conn-based, we simply always open a new connection to send the first round message

This is mostly what we were doing before bug 486508, but this time controlled and correct.

Gary, is this OK to do with squid and any other NTLM server you can thing of?
Attachment #8809837 - Attachment is obsolete: true
Attachment #8810959 - Flags: review?(mcmanus)
Attachment #8810959 - Flags: feedback?(gary)
Depends on: 1309438
I think this in the correct fix.  I would just change the comment to say we always want a new connection, to be sure we haven't sent NTLM packets on it already. 

Andrew Bartlett
Attachment #8810959 - Flags: review?(mcmanus) → review+
Comment on attachment 8810959 [details] [diff] [review]
v1 (when auth dialog is open, don't reuse the sticky conn)

Taking comment 5 as f+ for this patch.

Thanks Patrick for the review.
Attachment #8810959 - Flags: feedback?(gary) → feedback+
Can't land, deps on 1309438 that needs r.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/132bb5e7c5b8
Don't reuse the connection for conn-based auth schemes when asking user for credentials, r=mcmanus
Keywords: checkin-needed
Yes, of course.  The dependency bug is also marked checkin-needed.  I was hoping you notice the dep and land that one first (used to work in the past).
Flags: needinfo?(honzab.moz)
Blocks: 1320999
Dep landed, please land now this one as well.  Thanks.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1219cdda7496
Don't reuse the connection for conn-based auth schemes when asking user for credentials. r=mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1219cdda7496
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1346392
If this affected to ESR52, It seems to be worth to up lift this to ESR52.
Comment on attachment 8810959 [details] [diff] [review]
v1 (when auth dialog is open, don't reuse the sticky conn)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: major functionality breakage
User impact if declined: inability to log into an NTLM/Negotiate protected site
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): low, this has been riding trains a long time
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8810959 - Flags: approval-mozilla-esr52?
This depends on bug 1309438 which is not in 52.
Flags: needinfo?(honzab.moz)
(In reply to Julien Cristau [:jcristau] from comment #18)
> This depends on bug 1309438 which is not in 52.

Aggrrr... yes, that too should have been uplifted.  Never mind now.
Flags: needinfo?(honzab.moz)
Comment on attachment 8810959 [details] [diff] [review]
v1 (when auth dialog is open, don't reuse the sticky conn)

Clearing esr52 approval flag so this gets off my radar, feel free to set it again if bug 1309438 is upliftable.
Attachment #8810959 - Flags: approval-mozilla-esr52?
when bug 1309438 gets to esr52, ask for a?esr52
Flags: needinfo?(honzab.moz)
Comment on attachment 8810959 [details] [diff] [review]
v1 (when auth dialog is open, don't reuse the sticky conn)

DEPENDS ON BUG 1309438 !

applies cleanly on esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: major issues with NTLM authentication
User impact if declined: can't authenticate to an NTLM site
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): low if not none
String or UUID changes made by this patch: none



See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(honzab.moz)
Attachment #8810959 - Flags: approval-mozilla-esr52?
Comment on attachment 8810959 [details] [diff] [review]
v1 (when auth dialog is open, don't reuse the sticky conn)

ntlm auth fix for esr52
Attachment #8810959 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.