1345910 - Allow restart of an auth sticky connection that has not yet started the conn-based auth process

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Steve92]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170302120751

Steps to reproduce:

Problem found after Firefox update from V51 to V52 under Windows 7.
SSO (Windows AD+Kerberos) no longer works. (FF 32 bits because Java may be required on this internal portal). :( :( :(

SSO can be tested only from a PC on LAN.




Actual results:

I get "Error 401" when I try to acces the logon page that uses Kerberos SSO with Windows session.
The POST giving this error is: https://.../portal/1601131546/BIPCoreWeb/VintelaServlet?vint_backURL=/InfoView/logon.faces&vint_cms=BO4-DEV:6400

"VintelaServlet" is involved.


NB: SSO is OK with Internet Explorer 11 when accessing this portal..

With FF 51 (and previous versions) the following parameters were OK to get SSO:


network.negotiate-auth.delegation-uris : https://site1.domain.fr,https://site2.domain.fr,https://site3.domain.fr
network.negotiate-auth.trusted-uris: https://site1.domain.fr,https://site2.domain.fr,https://site3.domain.fr




Expected results:

Automatic login (SSO) using Windows session credentials to access an internal portal (acces from LAN).

Comment 1

[Honza Bambas (:mayhemer)]

We've fixed bunch of stuff on 52, some of them might cause regressions, since we have no automated tests.  I'll try to find time and look at this issues.  Sorry for inconvenience.

Comment 2

[Steve92]

Hello,

Thanks to try to solve this bug.
I perfectly understand it's difficult for you to detect such a bug.
It twill also be difficult to reproduce it because it needs an environment using Kerberos and Windows AD and VintelaServlet.

How can I help you ?
Do you need some logs on client and server side (Tomcat7)?
Please tell me how to activate them.

Did dev. team change something in FF V52 linked to "network.negotiate-auth" parameters ?

Regards,

Steve

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Steve92 from comment #2)
> Hello,
> 
> Thanks to try to solve this bug.
> I perfectly understand it's difficult for you to detect such a bug.
> It twill also be difficult to reproduce it because it needs an environment
> using Kerberos and Windows AD and VintelaServlet.

First of all, I'll ask you test with Firefox Beta [1], there is one more fix related to NTLM that was not uplifted to 52 (current Release version).

[1] https://www.mozilla.org/en-US/firefox/channel/desktop/

> 
> How can I help you ?
> Do you need some logs on client and server side (Tomcat7)?
> Please tell me how to activate them.

If your case is still broken on Beta (53) I'll ask you for logs on the client side, please see [2], and change MOZ_LOG to sync,timestamp,negotiateauth:5,nsHttp:5

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging

> 
> Did dev. team change something in FF V52 linked to "network.negotiate-auth"
> parameters ?

I would have to find all the bugs that could be related to NTLM that has landed on 52.  We don't always mark auth/ntlm related bugs so it's not that simple to get a list with a simple query.


> 
> Regards,
> 
> Steve



Thanks!

Comment 4

[Steve92]

Hi!

I tested with FF Dev v53a2 and v54a2: I've got the same 401 error (32-bit and 64-bit).
I used portable versions, I hope it's not a problem for you.

Here is the log file got from FF Dev 54a2 (did not find how to attach a file) using "about:networking" logging function with your debug parameters:

http://www.cjoint.com/doc/17_03/GCom4B0pKdU_FFP-DEV54a2-log.txt-main.7764.7z

I hope it will help you to understand and solve the bug.

I can send you any further information you could need.

Thanks,

Regards,

Steve.

Comment 5

[Steve92]

Hi again,

Finally, I installed "FF 53.0.b1" (32-bit) on my base v52(not portable).

=> I've got the same error "401" with SSO (Kerberos+Windows AD).

Here is the log file:

http://www.cjoint.com/doc/17_03/GCooELQtJIU_FF-53.0b1-log.txt-main.7384.7z

Hope it can help.
Tell me if you need other info.

Thanks,

Steve.

Comment 6

[Steve92]

For comparison purpose, here is the log for a SSO session that is OK with FFP_51.0_1_32b (portable):

http://www.cjoint.com/doc/17_03/GCorz0MdmQU_FFP-51.0-1-32b-log.txt.7z

Thanks again to try to solve the problem,

Steve

Comment 7

[Steve92]

Hello,

Can I provide you more information to help you to understand this regression ?

Regards,

Steve.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Steve92 from comment #5)
> Hi again,
> 
> Finally, I installed "FF 53.0.b1" (32-bit) on my base v52(not portable).
> 
> => I've got the same error "401" with SSO (Kerberos+Windows AD).
> 
> Here is the log file:
> 
> http://www.cjoint.com/doc/17_03/GCooELQtJIU_FF-53.0b1-log.txt-main.7384.7z
> 
> Hope it can help.
> Tell me if you need other info.
> 
> Thanks,
> 
> Steve.

We receive the challenge, we build a response and try to send it over the same connection (it's sticky).  But the server closes it, so the second request ends up prematurely with net-reset error.  Before 52 we were restarting such failed transactions.  But since bug 486508 we prevent it, there is an explanatory comment why.

I'm not sure it's legal the server resets (closes) the connection after 401, but I somewhat think it is and we should be handling it.

Hence, I think the condition added at [1] should be made smarter.  We should either:

- drop the sticky flag from connections that haven't ever been fully authenticated yet
- or detect at [1] that the transaction can be restarted on a new connection (from the same reasons).  

The former seems to me simpler to implement.


A side note: The server sends two WWW-Authenticate: Negotiate response headers as part of the first 401 response.  That might indicate a misconfiguration of the server.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8803374&action=diff#a/netwerk/protocol/http/nsHttpTransaction.cpp_sec2

Comment 9

[Steve92]

Hi!

Thanks for these investigations ! :)

If parameters have to be tested on server side, I've a test server to do that (Tomcat 7).

I can also test FF alpha version that could solve this regression...

Regards,

Steve.

Comment 10

[Honza Bambas (:mayhemer)]

Nick, Dragana, just to make you aware of this bug, since it's related to bug 1346392.

A good summary is comment 8.  The patch for your bug could easily fix this one too.

Comment 11

[Steve92]

Hello,

Can I help you for an alpha version testing in my environment ?

FYI, I still have the regression with FF 53.0.

Thanks for your work.

Regards,

Steve.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1

To make this upliftable, I added a new flag that allows the first (NTLM message 1) be sent on a new restarted connection.  In a nut shell, it loosens the "don't restart sticky conns" requirement from bug 486508 for a new negotiation exchange.  

NTLM message 3 is still not allowed to restart tho.  

Originally I wanted to drop the sticky conn flag from caps, but it makes conn manager cry on some assertions when a connection is found on a transaction and there is no sticky-conn flag set.

Note: this makes Bug 1315332 a bit unnecessary now, but I'd rather leave it in, this is easy to break and hard to fix.

(No try, since no tests)

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Steve92 from comment #11)
> Hello,
> 
> Can I help you for an alpha version testing in my environment ?
> 
> FYI, I still have the regression with FF 53.0.
> 
> Thanks for your work.
> 
> Regards,
> 
> Steve.

So, here will soon (an hour or two) be test builds for all platforms including the patch above.  Please give it a try.  Note that this is based on Firefox 55 - the Nightly channel.  Thanks.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> So, here ...

Sorry, too late for me :))

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac0bc860e60840212092e5afce55dd0630dd20e

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8858060 [details] [diff] [review]
v1

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

thanks honza.

Comment 16

[Steve92]

Hi!

Very nice job ! :o)

I've tested your solution for this bug in my environment and all seems OK.


My environment:

Firefox Nightly build : firefox-55.0a1.en-US.win64.zip 
Server O/S: Microsoft Windows Server 2012 R2
Client O/S: Microsoft Windows 7
App. & web server: Apache Tomcat 7.0
Portal: SAP/BusinessObjects Business Intelligence Launchpad V4.1 SP7
Authentication: Windows AD
SSO solution : Kerberos (Vintella)

NB: I unzipped firefox-55.0a1.en-US.win64.zip and run firefox.exe, then tested the URL of the portal needing SSO.
I didn't have to set parameters for SSO in "about:config", the parameters of my Firefox base release are used:

network.negotiate-auth.delegation-uris : https://site1.domain.fr,https://site2.domain.fr,https://site3.domain.fr
network.negotiate-auth.trusted-uris: https://site1.domain.fr,https://site2.domain.fr,https://site3.domain.fr


Are there other tests I can do for you to validate this patch ?

Regards,

Steve,
happy to modestly contribute to Firefox exciting project :)

Comment 17

[Steve92]

Hi again,

Just to say I've also tested "firefox-55.0a1.en-US.win32.zip" (32-bit release) and it's OK too.

Thanks again for your work.

Regards,

Steve.

Comment 18

[Honza Bambas (:mayhemer)]

Thanks Steve for the testing!  I will land the patch soon.

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1 (merged)

Comment 20

[Honza Bambas (:mayhemer)]

Going to land this despite we may have regression from a different bug related to an NTLM touching bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1346392#c92)

No try, since no tests for this.  Only a 'it builds' in comment 14.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c182ffc4e76
Allow HTTP connection restart for the first connection-based authentication request in a round. r=mcmanus

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c182ffc4e76

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Looks we've already verified via Try builds that this fixes the issue. Did we want to consider this for uplift to Beta/ESR52 too?

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Looks we've already verified via Try builds that this fixes the issue. Did
> we want to consider this for uplift to Beta/ESR52 too?

Definitely.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8861992 [details] [diff] [review]
v1 (merged)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: critical functionality breakage, users can't log in (no workaround too) to certain NTLM authenticated servers ; this is an enterprise thing, so ESR users will be affected most
User impact if declined: as above
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): low, I tried to make this patch as upliftable as possible
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 486508 (52)
[User impact if declined]: as above
[Is this code covered by automated tests?]: not directly, but regressions in code affected would be caught I believe
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: would be nice, but the setup is hard to establish, so - no.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: the risk is low
[Why is the change risky/not risky?]: this is narrowed to only affect the NTLM/Negotiate auth and other code this could be affected with should be tested with some automated coverage and on Nightly 
[String changes made/needed]: none

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8861992 [details] [diff] [review]
v1 (merged)

Fix a regression related to NTLM/Negotiate auth and was verified. Beta54+. Should be in 54 beta 4.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb61031b0a99

Comment 28

[Andrei Vaida [:avaida]]

(In reply to Honza Bambas (:mayhemer) from comment #25)
> [Is this code covered by automated tests?]: not directly, but regressions in
> code affected would be caught I believe
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: would be nice, but
> the setup is hard to establish, so - no.

Setting qe-verify- based on Honza's assessment on manual testing needs and the fact that this fix has at least in some measure, automated coverage.

Comment 29

[Steve92]

Hello,

Just a few words to say I've tested FF 54 beta 4 (32-bit): Kerberos SSO works like a charm with "my" internal SAP portal. :o)

Thanks again !

Regards,

Steve.

Comment 30

[Honza Bambas (:mayhemer)]

Thanks for confirmation.  Changing the state to VERFIED based on comment 29.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8861992 [details] [diff] [review]
v1 (merged)

This is a severe regression and meets the ESR uplift criteria. ESR52.2+

Comment 32

[Ryan VanderMeulen [:RyanVM]]

This'll need a rebased patch for ESR52.

Comment 33

[Honza Bambas (:mayhemer)]

Attachment: v1 [for ESR52]

OK, this is the merge, but I'm no longer sure this should uplift...  there have been at least two other bugs landed that rode the train (and caused regressions too) and were touching this very code with changes related to the same functionality this patch is changing.

Hence, I'm not sure it's safe to take it to ESR w/o a deeper field testing.  Changing NTLM code is always very sensitive.

Lesson for me: don't change enterprise related features in ESR releases...

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #33)
> OK, this is the merge, but I'm no longer sure this should uplift...  there
> have been at least two other bugs landed that rode the train (and caused

(I had to merge because these two (or maybe even more) patches are missing on esr52 ; it was only mechanical, no dependencies, tho)

OTOH, the patch is written as upliftable and only somewhat reverts the effect of bug 486508... Hard to judge.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Ultimately, there's two things to consider IMO:
* Is this bug severe enough for the enterprise audience that it's worth fixing in the ESR if possible?
* Are we confident enough in the patch to uplift it right now?

Sounds like #1 is probably yes and #2 is no. Is that something where more bake time would help? One nice thing about ESRs is that we certainly have the ability to wait a cycle if needed.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> Ultimately, there's two things to consider IMO:
> * Is this bug severe enough for the enterprise audience that it's worth
> fixing in the ESR if possible?

MS based servers (IIS) don't suffer from this bug (exposed with bug 486508), so I'd say no.  This bug has been so far observed on Apache-Coyote server.  I have no data on how widely this is deployed in enterprise environments.

> * Are we confident enough in the patch to uplift it right now?

I personally am not.

> 
> Sounds like #1 is probably yes and #2 is no. Is that something where more
> bake time would help? One nice thing about ESRs is that we certainly have
> the ability to wait a cycle if needed.

There is no other release branch (channel) we could test having only bug 486508 and this bug in the code, AFAIK.  All branches have 2 or 3 or even 4 other patches in between.  So, I'm not sure how this could be tested (my ESR knowledge may be thin here)

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Is backing out bug 486508 from esr52 an option?

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #37)
> Is backing out bug 486508 from esr52 an option?

Probably yes.  But give me some time (a day) to check what all (if anything) has landed on esr52 after that bug.  I think we haven't uplifted anything, but I want to make sure.  Does it deserve a new bug?

Comment 39

[Ryan VanderMeulen [:RyanVM]]

If you decide that's the direction you want to go in, yes, let's do it in a new bug.

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> If you decide that's the direction you want to go in, yes, let's do it in a
> new bug.

Looks like one of the bugs that have landed in between has also landed on 52:
https://bugzilla.mozilla.org/show_bug.cgi?id=1360574#c43

Hence, I would be more confident to land this one as well now.  Will definitely need a merge once again.  I presume I need a patch for FIREFOX_ESR_52_1_X_RELBRANCH branch as well to apply cleanly?

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #40)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> > If you decide that's the direction you want to go in, yes, let's do it in a
> > new bug.
> 
> Looks like one of the bugs that have landed in between has also landed on 52:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1360574#c43

Ah!!!  It's a back out of bug 1346392.  Patch for this bug has originally been written when that bug still had been applied, hence, landing on esr52 now resembles m-c quite good.  There are also no other patches that would need to uplift in between.

I think it's OK to land "v1 [for ESR52]" now.  Thanks.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Thanks for digging into this, Honza!

https://hg.mozilla.org/releases/mozilla-esr52/rev/ec6b6720e54e181dbb58139ad581e4af8bf22058