Closed Bug 1311720 Opened 3 years ago Closed 3 years ago

Connections no longer sticky for NTLM negotiation

Categories

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

51 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

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

Attachments

(1 file, 2 obsolete files)

I found out that we do not stick connections to channels during NTLM authentication loops.  No idea when this has regressed.
This is intermittent, what happens:

- we have some existing, already authenticated connections to the origin
- we have space for one or more more new connections to the origin
- a resource, that will later be found behind NTLM authentication, is requested at the origin
- a transaction for it is created
- it happens to be dispatched on a new connection
- gets 401 + WWW-Authenticate: NTLM (the first challenge)
- the connection is put back to the poll as idle and available (the server doesn't see it as authenticated), because we set the sticky flag on it later (on the main thread, in nshttpchannel::onauthretry called from onstoprequest)
- we generate type 1 message
- create a new transaction
- and put it to the pending list, now with the conn_sticky flag set
- an already authenticated connection is chosen to handle the transaction <= this is the problem!

When we send type 1 message on an already authenticated connection, server may respond with 401 with either a type 2 message (or something I don't follow the binary form of) or the initial challenge.  In this case let's consider type 2.  

We proceed with the authentication (ntlm type 3 message is generated) and use the same connection for a new transaction.  The server resets the connection (or maybe already did right after 401 but we figured out no sooner than before we sent out a request), this behavior is a bit strange.  The transaction is then restarted.  

And put again to the pool.  It later gets a different already authenticated connection and tries to send the type 3 message.  In response it gets 401 + WWW-Authenticate: NTLM.  (Connection is later reset again, same way as in the previous case.)  

This all makes the channel believe the cached credentials are wrong and prompts again to the users.  It leads to bug 486508.

This bug is about the early sticky problem, but the connection reset (and sending in-progress ntlm messages on new connections) is mostly a general problem, that we may consider fixing separately.

There is no clear solution in my head right now for this.  We need to set the sticky flag on the connection on the socket thread right at the moment we find the WWW-Authenticate: NTLM (or any sticky scheme) header in the response, before we would put the connection back to the pool as idle.
Whiteboard: [necko-active]
the patch simply adds a check made early in nsHttpTransaction::ParseHead for auth schemes that have the connection-based flag set.  if found, sets itself as sticky.

questions:
- should there be a cache or are we ok with do_createInstance?
- should we do the same also for proxy auth?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=782b32b7351b478609f10cdf7deda0bbc1d57cef
Attachment #8803101 - Flags: review?(mcmanus)
Status: NEW → ASSIGNED
Gary, could you please check this patch (and only this patch) if it fixes bug 486508?  At least as a confirmation.  I will take a look at your patches there tomorrow.

Thanks!
Flags: needinfo?(gary)
Comment on attachment 8803101 [details] [diff] [review]
v1 (set transaction's sticky flag more early on NTLM/Negotiate in response)

Review of attachment 8803101 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the research and great explanation. this has been broken for years as I understand the description. just one thing to look at for a review

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1556,5 @@
>          if (NS_FAILED(rv))
>              return rv;
>  
> +        if (mHaveAllHeaders) {
> +            CheckForStickyAuthScheme();

I think you should call this out of HandleContentStart().. wdyt?
Attachment #8803101 - Flags: review?(mcmanus)
Patch does not fix bug 486508, still see the same behaviour.
Flags: needinfo?(gary)
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Created attachment 8803101 [details] [diff] [review]

> questions:
> - should there be a cache or are we ok with do_createInstance?

probly not

> - should we do the same also for proxy auth?
> 
probly yes :)
- doing the check also for proxy auth

But I left the call to Check..() where it was.  If I move it to nsHttpTransaction::ProcessData() { if (mHaveAllHeaders) { .. } } I may not always have the response head.

Patrick, please suggest what you believe is more proper place or way.
Attachment #8803101 - Attachment is obsolete: true
Attachment #8803407 - Flags: feedback?(mcmanus)
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Created attachment 8803407 [details] [diff] [review]
> v2 (set transaction's sticky flag more early on NTLM/Negotiate in response)
> 
> - doing the check also for proxy auth
> 
> But I left the call to Check..() where it was.  If I move it to
> nsHttpTransaction::ProcessData() { if (mHaveAllHeaders) { .. } } I may not
> always have the response head.
> 
> Patrick, please suggest what you believe is more proper place or way.

what about HandleContentStart() ? If there is going to be a response header - that will always have it.
Yep, HandleContentStart it is.  Thanks.

(try down)
Attachment #8803407 - Attachment is obsolete: true
Attachment #8803407 - Flags: feedback?(mcmanus)
Attachment #8803427 - Flags: review?(mcmanus)
Attachment #8803427 - Flags: review?(mcmanus) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/596e11697034
Stick HTTP connection to its channel when NTLM/Negotiate auth challenge is received more early, r=mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/596e11697034
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Duplicate of this bug: 902452
Duplicate of this bug: 1157206
Duplicate of this bug: 702220
Whiteboard: [necko-active] → [necko-active][ntlm]
You need to log in before you can comment on or make changes to this bug.