573043 - Enable Extended Protection (channel and service binding) for NTLM authentication

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[groblavicario]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; InfoPath.2; MS-RTC LM 8; .NET CLR 3.5.30729; .NET CLR 3.0.30729; .NET4.0C; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100617 Minefield/3.7a6pre

When Integrated Windows Authentication (IWA) takes place inside an outer secure channel (e.g. TLS), it is normally possible to perform an authentication relay attack to gain unauthorized access to resources under spoofed identities. This is due to the impossibility of binding together the inner channel session key with the outer channel session key that in most of the implementations of the protocols involved.
Window 7’s implementation of TLS and IWA protocols (such as NTLM) allow channel binding, and therefore provide the necessary means to avoid authentication forwarding attacks.
Firefox uses SSPI to perform NTLMv2 authentication when running on Windows, and Samba's ntlm_auth tool when running on Linux. I have updated Firefox to call SSPI with the new parameters needed for extended protection, and both Firefox and Samba to allow extending protection on Linux.
I didn't update NSS in order to do that, but I see bug 563276 is addressing channel binding on NSS. My solution gets the TLS/SSL info needed for channel binding without calling NSS, and works in Linux (when running my updated Samba version) and on updated Windows machines (Windows 7 and pathced previous versions).

Reproducible: Always

Steps to Reproduce:
1.Configure an IIS Windows Server to require Extended Protection (http://www.iis.net/ConfigReference/system.webServer/security/authentication/windowsAuthentication/extendedProtection)
2.Join the client machine to the same AD domain as the Server
3.Log in to the client machine using a valid AD account.
4.Set "property network.automatic-ntlm-auth.trusted-uris" to automatically try NTLM authentications against the server.
5.Try to access an application on the web server that requires extended protection.
Actual Results:  
The authentication will be rejected.

Expected Results:  
The user is authenticated and can access the application

I have already implemented and tested the code, and works in Linux and Windows. I have also documented the tests and the configuration.

On Linux it requires a Samba version updated for Extended Protection. I am in the process of submiting this patch to Samba.

Comment 1

[Jason Duell]

It would be great to have this stuff.  We look forward to seeing the patch.  Thanks for your work!

We'll want to make sure that we fail gracefully on Linux if the updated samba isn't present. 

Do we have a story for OSX?

Jim, do we have any test infrastructure for NTLM that we could use here?

Comment 2

[groblavicario]

Attachment: Diff file for the update

As you can see, the update modifies 6 source files and 3 "Makefile.in" files. I had to move 3 interface descriptors from one folder to another, so I could access them on the updated code. You will have to move these files manually in order for the update to compile (it is explained on the comments, and you can deduce it from the Makefile.in files).
Also, I put my initials (GRV) to signal where the changes have been made, and also explained some of the code. Some of this comments could be deleted before commiting the patch. Please feel free to modify or delete them, or to ask me to do that for you.

Comment 3

[groblavicario]

Attachment: Test document

This document shows the tests I have performed with the update, and its results.

Comment 4

[groblavicario]

Attachment: Configuration document

This document shows how to configure the machine to make the update work.
In Linux, when Samba incorporates the update, it wont be necessary to manually create the link to the tool ntlm_auth.

Comment 5

[groblavicario]

(In reply to comment #1)
> It would be great to have this stuff.  We look forward to seeing the patch. 
> Thanks for your work!
> We'll want to make sure that we fail gracefully on Linux if the updated samba
> isn't present. 
> Do we have a story for OSX?
> Jim, do we have any test infrastructure for NTLM that we could use here?

As you can see from the test document, when Linux doesnt have an updated version of Samba), Firefox falls back and performs authentication as before, so Extended Protection is not enabled, but it does not crash.

I havent done the update for OSX, but if you like the update for Windows and Linux and think that it would be nice to have it for OSX too, I can take a look and investigate if it possible, and if yes, work on it on my spare time.

Please feel free to ask any questions concerning the patch. Thank you!

Comment 6

[timeless]

* fwiw, we generally don't leave initials in code
* try not to add trailing whitespace
* please don't use <tab>s in mozilla files in general
* please include a <space> after <comma> in function calls and for constructs
* we use mSomeThing not someThing for member variables
in rare occasions we use them in Makefiles, but otherwise, they generally don't belong
* in class structures, we tend to use PRPackedBool instead of bool if there's a chance to get some compression if we expect the class to have lots of instances
* If you can replace fooString + fooLength with an nsString or nsCString, that makes things easier on the rest of us
(it also avoids manual memory management)
* strToHexStr - please switch to using ns*String's, use them as pass by reference

@@ -224,33 +231,38 @@
and for some reason you inserted a single space which broke our 4 space indentation for the file
* const char x[] = "...." is preferred over const char* x = "...."

* "executues", can you please use a spell checker? :)

* if Init()/Update()/Finish() can fail please rv check them
if they *can't* fail, using (void) is appreciated by me, since it helps me choose not to worry about you carelessly forgetting to rv check
* in header declarations, please have the function name or member name line up w/ its neighbors

* please use hg rename to record moves

Comment 7

[groblavicario]

Attachment: Diff file for the update

This is the new diff file, modified to follow timeless' recommendations.

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #1)
> Jim, do we have any test infrastructure for NTLM that we could use here?

Unfortunately not. I have a local server I use to do basic testing. QA really doesn't have anything setup beyond that. We've mostly relied on users to test bug fixes.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 457951 [details] [diff] [review]
Diff file for the update

>+    mCertLenght = 0;

typo - mCertLenght -> mCertLength?

>+    mIsFirst = true;

We have mozilla specific data types for this, please use PR_TRUE, and mIsFirst should be PRBool.

>-|
>+    |

Could we clean stuff like this up? Extra white space on lines is messy and just bloats the src.

>+// Several changes in this method, in order to compute a
>+// Channel Binding Token when possible.

Please delete this comment.

>+        mCertString = nsMemory::Alloc(inTokenLen);
>+        if (!mCertString)
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        memcpy(mCertString,inToken, inTokenLen);

>         if (mCtxt.dwLower || mCtxt.dwUpper) {
>             LOG(("Cannot restart authentication sequence!"));
>             return NS_ERROR_UNEXPECTED;
>         }
>+        ctxIn = nsnull;
>+        // This token needs to be erased before being passed 
>+        // to InitializeSecurityContextW().
>+        inToken = nsnull; 
>+    } else if (inToken && !mIsFirst) {


Looks like we could leak mCertString here, both in the error case and where we set inToken to nsnull.

Also, if we set inToken to nsnull here, how is |if (inToken && !mIsFirst)| true on the next call into GetNextToken?

(You also might want to add a corresponding free on mCertString in Reset, and set it to nsnull.)

>+            sspi_cbt = (char *) nsMemory::Alloc(ib[ibd.cBuffers].cbBuffer);

There's at least one failure case after this where we would leak.

>+const char*
>+strToHexStr(const char *str)

StrToHexStr

>+        delete(cbt_hex);

delete [] cbt_hex here?

>+    if (mIsFirst) {
>+    // First time if it comes with a token, the token represents
>+    // the server certificate.
>+        if (inToken) {

>+	        // Finally we execute ntlm_auth with CBT and SPN		
>+            rv = SpawnNTLMAuthHelper((const char*)hashString.get(),

>+            // Then we execute ntlm_auth with the SPN		
>+	        rv = SpawnNTLMAuthHelper(nsnull,(const char*)mServiceName.get());


formatting in this block is a little weird. Please make sure and match existing formatting of the src file.

>+// nsIX509Cert.idl and nsISSLStatus.idl moved from security/manager/ssl/public
>+// to /netwerk/base/public (Makefile.in changed)
>+// and nsISSLStatusProvider moved from security/manager/boot/public 
>+// to /netwerk/base/public (Makefile.in changed)

Some one from netwerk will have to sign off on this, :bz maybe?

>+            cert->GetRawDER(&length,&certArray);

|&length, &certArray|

+
+// This flag is required to enable the use of the internal NTLM
+// implementation when the use of the native implementation in Windows
+// or Linux is not possible and there is an outer channel.
+private:
+    bool mUseNative;

Lets move that comment block down to the variable and tab it over.

Comment 10

[groblavicario]

Attachment: Diff file for the update

Diff file after following Jim Mathies' indications

Comment 11

[groblavicario]

(In reply to comment #9)

Thank you for your review, I have corrected my patch and uploaded a new diff file.

> Comment on attachment 457951 [details] [diff] [review]
> Diff file for the update
> >+    mCertLenght = 0;
> typo - mCertLenght -> mCertLength?
That was a typo and has been corrected.

> >+    mIsFirst = true;
> We have mozilla specific data types for this, please use PR_TRUE, and mIsFirst
> should be PRBool.
That has been changed too.

> >-|
> >+    |
> Could we clean stuff like this up? Extra white space on lines is messy and just
Corrected.

> bloats the src.
> >+// Several changes in this method, in order to compute a
> >+// Channel Binding Token when possible.
> Please delete this comment.
Deleted.

> >+        mCertString = nsMemory::Alloc(inTokenLen);
> >+        if (!mCertString)
> >+            return NS_ERROR_OUT_OF_MEMORY;
> >+        memcpy(mCertString,inToken, inTokenLen);
> >         if (mCtxt.dwLower || mCtxt.dwUpper) {
> >             LOG(("Cannot restart authentication sequence!"));
> >             return NS_ERROR_UNEXPECTED;
> >         }
> >+        ctxIn = nsnull;
> >+        // This token needs to be erased before being passed 
> >+        // to InitializeSecurityContextW().
> >+        inToken = nsnull; 
> >+    } else if (inToken && !mIsFirst) {
> Looks like we could leak mCertString here, both in the error case and where we
> set inToken to nsnull.
I have double checked all possible error conditions and cleaned the memory after them.

> Also, if we set inToken to nsnull here, how is |if (inToken && !mIsFirst)| true
> on the next call into GetNextToken?
It is always true on the second call, since GetNextToken is called passing an NTLM token on the parameter "inToken". "inToken" is set to null here because it needs to be null before calling InitializeSecurityContextW() for the first time.

> (You also might want to add a corresponding free on mCertString in Reset, and
> set it to nsnull.)
That has been revised too to make sure that there are no memory leaks.

> >+            sspi_cbt = (char *) nsMemory::Alloc(ib[ibd.cBuffers].cbBuffer);
> There's at least one failure case after this where we would leak.
Also revised and updated.

> >+const char*
> >+strToHexStr(const char *str)
> StrToHexStr
Corrected to follow Mozilla's guidelines.

> >+        delete(cbt_hex);
> delete [] cbt_hex here?
I do not really know what is wrong with that. Could you please explain it?


> >+    if (mIsFirst) {
> >+    // First time if it comes with a token, the token represents
> >+    // the server certificate.
> >+        if (inToken) {
> >+	        // Finally we execute ntlm_auth with CBT and SPN		
> >+            rv = SpawnNTLMAuthHelper((const char*)hashString.get(),
> >+            // Then we execute ntlm_auth with the SPN		
> >+	        rv = SpawnNTLMAuthHelper(nsnull,(const char*)mServiceName.get());
> formatting in this block is a little weird. Please make sure and match existing
> formatting of the src file.
Corrected.

> >+// nsIX509Cert.idl and nsISSLStatus.idl moved from security/manager/ssl/public
> >+// to /netwerk/base/public (Makefile.in changed)
> >+// and nsISSLStatusProvider moved from security/manager/boot/public 
> >+// to /netwerk/base/public (Makefile.in changed)
> Some one from netwerk will have to sign off on this, :bz maybe?
> >+            cert->GetRawDER(&length,&certArray);
> |&length, &certArray|
> +
> +// This flag is required to enable the use of the internal NTLM
> +// implementation when the use of the native implementation in Windows
> +// or Linux is not possible and there is an outer channel.
> +private:
> +    bool mUseNative;
> Lets move that comment block down to the variable and tab it over.
Corrected.

Comment 12

[Jim Mathies [:jimm]]

>-    if (!ob.pvBuffer)
>+    if (!ob.pvBuffer){
>+	    if (sspi_cbt)
>+            nsMemory::Free(sspi_cbt);
>         return NS_ERROR_OUT_OF_MEMORY;

final minor nit - still off here, if you could update that for the final that would be great.

this needs reviews from some other folks. I can't sign off on the samba code or network changes. It also needs an sr.

Comment 13

[groblavicario]

(In reply to comment #12)
> >-    if (!ob.pvBuffer)
> >+    if (!ob.pvBuffer){
> >+	    if (sspi_cbt)
> >+            nsMemory::Free(sspi_cbt);
> >         return NS_ERROR_OUT_OF_MEMORY;
> final minor nit - still off here, if you could update that for the final that
> would be great.
> this needs reviews from some other folks. I can't sign off on the samba code or
> network changes. It also needs an sr.


Sorry for that, I will fix it. But I have a question that I should ask before doing so, to avoid changing the diff file too many times.

Should I put my name as a contributor in the files I have changed? Or would you do that once the update has been aproved? It be nice to see my name on the code of this fantastic Browser :-)

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> (In reply to comment #12)
> > >-    if (!ob.pvBuffer)
> > >+    if (!ob.pvBuffer){
> > >+	    if (sspi_cbt)
> > >+            nsMemory::Free(sspi_cbt);
> > >         return NS_ERROR_OUT_OF_MEMORY;
> > final minor nit - still off here, if you could update that for the final that
> > would be great.
> > this needs reviews from some other folks. I can't sign off on the samba code or
> > network changes. It also needs an sr.
> 
> 
> Sorry for that, I will fix it. But I have a question that I should ask before
> doing so, to avoid changing the diff file too many times.
> 
> Should I put my name as a contributor in the files I have changed? Or would you
> do that once the update has been aproved? It be nice to see my name on the code
> of this fantastic Browser :-)

Feel free to add that yourself in a future rev.

Comment 15

[groblavicario]

Attachment: Diff file for super review

This fixes the last tabulation problem (sorry the text editors that I am using do not really help with this) and adds my name to the contributors list of the files that I have changed.

Comment 16

[Wan-Teh Chang]

Comment on attachment 459522 [details] [diff] [review]
Diff file for super review

Guillermo: thanks for the patch.  Did you compile the code?
The patch uses "PR_BOOL", but the correct type name is "PRBool":
http://mxr.mozilla.org/mozilla-central/search?string=PR_BOOL

Comment 17

[groblavicario]

Attachment: Diff file for super review

There was indeed a typo with PRBool.

Of course I am compiling the code, the problem is that when I compile it on Linux, the files nsAuthSSPI.h and nsAuthSSPI.cpp are not compiled (they are only used on Windows), and I have my repository set up on Linux and made a typo on nsAuthSSPI.h (instead of replacing the file with the one from my Windows machine, I just changed the variable type and made a silly mistake).

Sorry for that. I have compiled it and tested again and works fine in both Windows and Linux.

Thank you for your help,

Guillermo.

Comment 18

[Wan-Teh Chang]

Attachment: Review comments on "Diff file for super review" (attachment 459676)

Guillermo: thanks a lot for the patch!

I did a moderately detailed review.  Since I have a lot of
comments, I decided to write my comments as an attachment.
Note especially the comments marked with "BUG" and "QUESTION".

biesi (or some other Mozilla developer): please comment on
the following issue:

The patch moved three nsI*.idl files from mozilla/security/manager
to mozilla/netwerk/base/public.  I don't know the structure
of the Mozilla source tree well enough to know if this is
fine, or if we can solve the problem in another way (such
as adding -I flags to get the headers you need).

Comment 19

[Wan-Teh Chang]

Comment on attachment 459676 [details] [diff] [review]
Diff file for super review

I checked this patch again, and confirmed the leak of
    SEC_CHANNEL_BINDINGS* pendpoint_binding = new SEC_CHANNEL_BINDINGS;
and the double free of mCertString.

To fix the double free, after every occurrence of
    nsMemory::Free(mCertString);
please add
    mCertString = nsnull;
    mCertLength = 0;

Comment 20

[Boris Zbarsky [:bzbarsky]]

Adding -I flags won't work, I'd think, because necko is built before caps and the like.  So the headers simply won't exist yet when it's built.  At least as I understand it.

It might make more sense to move nsIX509Cert into XPCOM, not necko.  Benjamin, thoughts?  The SSL stuff makes sense in Necko, I think.

Comment 21

[Benjamin Smedberg]

-I flags should work: all of our platform is built in one tier now.

But I'm happy to move nsIX509Cert into necko. I don't see any reason for it to be in xpcom/

Comment 22

[groblavicario]

Attachment: Diff file after Wan-Teh Chang's review

I have changed everything that Wan-Teh asked for in his review.

However, I couldn't use snprintf instead of sprintf, since the code wouldn't compile with that change.

About the questions included in the review:

- NTLM Reflection Attack: Since Firefox no longer performs a DNS resolution, I think it is not vulnerable to Reflection Attack. The SPN (Service Principle Name) that we are using for NTLM comes from the Server Certificate, and should not be manipulable by an attacker.

- SSPI comment on nsAuthSamba.cpp: I was trying to say that the server expects the SPN in an specific format. If the server is IIS (the only one updated for Extended Protection I believe), then I it will call the SSPI again and the SPN needs to be in that format. In any case, I have changed the comment to avoid confusion.

- Spawning less ntlm_auth processes: I do not think it is possible to do this without major changes in previous code (the authentication module spawns a process when it is created, and at that time we do not have the values needed for Extended Protection).

- Extended Protection definition: Fix to avoid authentication forwarding attacks by performing channel OR service bindings (when there is no outer channel).

- "Existance": I do not know how that ended up there, maybe it was wrong in the file I use as a starting point originally. I know how it is in English (and Spanish :-) )

- Should we retrieve the server certificate only if
the certificate has no errors?
Maybe we could, but I do not know how to perform that check.

Comment 23

[Wan-Teh Chang]

Comment on attachment 461319 [details] [diff] [review]
Diff file after Wan-Teh Chang's review

Guillermo: I've reviewed this new patch.  Thank you.  I'll write down
my detailed review comments later.

HELP NEEDED: I'd like a real Mozilla developer to check the following
issues of this patch.

1. Is it acceptable for nsAuthSambaNTLM::GetNextToken to shut down
and restart a ntlm_auth helper process every time it's called?  This
seems very expensive to me.

Guillermo, can the ntlm_auth program be long-lived and receive
new cbt and spn values over a pipe from Firefox?  It sucks that
a ntlm_auth process handles only a single request and exits.

2. Is it OK to move nsIX509Cert.idl to necko but leaves its
implementation nsNSSCertificate.{h.cpp} behind in security/manager?

This question also applies to the other two .idl files that this
patch moves from security/manager to necko.

3. Should new versions of the nsIX509Cert interface, nsIX509Cert2.idl
and nsIX509Cert3.idl, also be moved to necko?

4. Please review a block of heavy-duty XPCOM code in nsHttpNTLMAuth.cpp.
You can find it by searching for
    #if defined (XP_WIN) || defined (LINUX)
in this patch.

Comment 24

[Wan-Teh Chang]

Attachment: Review comments on "Diff file after Wan-Teh Chang's review" (attachment 461319)

jmathies: I've done two rounds of detailed review.  I
won't be able to review future versions of this patch.
After Guillermo addresses my review comments, the only
remaining issues would be those I listed in comment 23
and the issue below.

Guillermo: I wrote down my detailed review comments
in this attachment.  But I want to highlight a more
serious issue with your patch here.

There is no feature detection code to test if
the version of Windows supports channel binding.
What happens if we run the new code on Windows XP?
Does InitializeSecurityContext fail on XP when
there is an input sec buffer of type
SECBUFFER_CHANNEL_BINDINGS?

Similarly, the Linux code has no feature detection
code to check whether ntlm_auth supports the --cbt
and --spn command-line options.

Comment 25

[groblavicario]

(In reply to comment #24)
> Created attachment 462283 [details]
> Review comments on "Diff file after Wan-Teh Chang's review" (attachment 461319 [details] [diff] [review])
> jmathies: I've done two rounds of detailed review.  I
> won't be able to review future versions of this patch.
> After Guillermo addresses my review comments, the only
> remaining issues would be those I listed in comment 23
> and the issue below.
> Guillermo: I wrote down my detailed review comments
> in this attachment.  But I want to highlight a more
> serious issue with your patch here.
> There is no feature detection code to test if
> the version of Windows supports channel binding.
> What happens if we run the new code on Windows XP?
> Does InitializeSecurityContext fail on XP when
> there is an input sec buffer of type
> SECBUFFER_CHANNEL_BINDINGS?
> Similarly, the Linux code has no feature detection
> code to check whether ntlm_auth supports the --cbt
> and --spn command-line options.

Wan-Teh: I will address your latest review comments as soon as I can.

About the detection of unpatched Windows or Samba clients, I attached a test document that covers every scenario when I first created the bug. InitializeSecurityContext does not fail on XP, it just creates an NTLM package without the Extended Protection information. In Linux, if there is not an updated version of Samba running, the patch falls back and calls ntlm_auth without the new options.

Comment 26

[Wan-Teh Chang]

jmathies: you may consider checking in the Windows
part of the patch first, after Guillermo updates the
patch.

Guillermo: I didn't know you already considered the
feature detection issue.  Sorry about that.

I'm surprised that InitializeSecurityContext silently
ignores an input sec buffer of an unrecognized type.
Although that saves us the trouble of detecting
unpatched Windows, it could make it difficult to
discover a mistake in SSPI code.

On Linux, can you remember the fact that there is not
an updated version of Samba running, so that you don't
try CBT and then fall back every time
nsAuthSambaNTLM::GetNextToken is called (when
mIsFirst == PR_TRUE)?

Comment 27

[Jim Mathies [:jimm]]

(In reply to comment #26)
> jmathies: you may consider checking in the Windows
> part of the patch first, after Guillermo updates the
> patch.

I'd be happy to re-review/test/check-in whatever parts we are comfortable with. (I'll be on pto next week, so I'll check back in on the bug in 1.5 weeks.)

Comment 28

[Daniel Veditz [:dveditz]]

What's the status here? Sounds like we need an updated patch. Is [e-mail] still around or should we reassign this to jimm?

Comment 29

[Honza Bambas (:mayhemer)]

[e-mail]: if you don't mind, I'll take this bug.

-> me

Comment 30

[groblavicario]

Sorry everybody for my late answer. I have been very busy lately.

I am ok with Honza finishing this bug up, but I would appreciate keeping me as one of the authors.

Thank you and don´t hesitate on asking me any questions about the code.

Guillermo.

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to comment #30)
> but I would appreciate keeping me as one of the authors.
Sure.

Comment 32

[Honza Bambas (:mayhemer)]

Guillermo, are you willing to test future versions of the patch?  I think I might have an environment for the window platform, but I'm not sure about linux.

Comment 33

[Honza Bambas (:mayhemer)]

(In reply to comment #23)
> 2. Is it OK to move nsIX509Cert.idl to necko but leaves its
> implementation nsNSSCertificate.{h.cpp} behind in security/manager?
> 

We do this for lot of other cases like nsICryptoHash - idl in netwerk, implementation in security.  I'm ok with moving this interface...

> This question also applies to the other two .idl files that this
> patch moves from security/manager to necko.
> 

...and any other.

> 3. Should new versions of the nsIX509Cert interface, nsIX509Cert2.idl
> and nsIX509Cert3.idl, also be moved to necko?
> 

I'd leave it where it is, until someone needs them.

Comment 34

[Honza Bambas (:mayhemer)]

Guillermo, one question:

In [1] you are using httpChannel to get the security info form the channel.  However, I don't see this local var declared anywhere.  This is in all versions of the patch you provide.  Are the changes complete or is there some other patch we need to apply to make this patch compilable?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=461319&action=diff#a/netwerk/protocol/http/nsHttpNTLMAuth.cpp_sec5

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: v3

Looks like I don't have a testing environment for windows either.  IIS 5.1 I'm running apparently doesn't support EP for IWA.  I probably will have access to IIS 7.5 in a week or so, then I can test directly.

- patch addresses the Wan-Teh's review comments
- fixes the 'httpChannel' building problem
- quit untested


If anybody can run tests with this patch, I will be able to update ASAP after feedback.  Thanks.

Comment 36

[Honza Bambas (:mayhemer)]

I currently have IIS 7.5 with Extended Authentication:Required.  Hope I will be able to move forward with this.

Comment 37

[Jason Duell]

Honza: does your patch need review, or just testing?  Keeping as a 2.0 blocker.

Comment 38

[Honza Bambas (:mayhemer)]

There were no review and nor testing.  I don't have an environment.  

I want to check again for how to locally setup the environment.  I didn't get any answers from Guillermo.

If anyone has a testing environment, please test this patch.

Comment 39

[Honza Bambas (:mayhemer)]

With server 2003 and IIS 6 configured to require EA and Firefox m-c build running on windows (7) I can confirm we are:
- unable to authenticate without the patch
- able to authenticate with the patch

so, for me this seems to be tested on windows system.

Comment 40

[Honza Bambas (:mayhemer)]

So, I have spent some time on configuring linux to join my testing AD domain, so far unlucky.  I am sure I am able to test also on linux, but at this moment I'm not able to estimate how long it will take to have a working setup on linux.  I believe I can spend time on more important stuff.

I suggest:
- split the patch to winodws-only part that is now tested
- leave the linux part separated for future work ; I'll be continuing setting my linux machine up for ntlm authentication in free time

Opinions?

Comment 41

[Wan-Teh Chang]

Honza: I suggest checking in the Windows code first, if it's
easy to split the patch into two parts.

Comment 42

[Honza Bambas (:mayhemer)]

Attachment: v1, windows only part

- review comments should all be updated
- this patch only contains tested changes for window platform

Comment 43

[Honza Bambas (:mayhemer)]

Attachment: v1, linux additions only [moved to bug 630315]

This is here for reference.  So far, I don't have a valid testing environment to verify this patch at this time.  If the linux changes will be found necessary for Fx4 I can jump on it.

Comment 44

[Wan-Teh Chang]

Comment on attachment 502667 [details] [diff] [review]
v1, windows only part

Honza: I'm busy this week, and I've contributed enough time
to this bug before.  Can you ask someone else (such as jmathies
and jduell) to review this patch?  If you get no response in a
week, please ask me again and I'll review it.  Sorry.

Comment 45

[Honza Bambas (:mayhemer)]

Comment on attachment 502667 [details] [diff] [review]
v1, windows only part

Reassign review to Jim Mathies as wtc is busy.

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #45)
> Comment on attachment 502667 [details] [diff] [review]
> v1, windows only part
> 
> Reassign review to Jim Mathies as wtc is busy.

This is the original fully reviewed patch without changes correct? (Minus code not related to windows of course)

Comment 47

[Honza Bambas (:mayhemer)]

(In reply to comment #46)
> This is the original fully reviewed patch without changes correct? (Minus code
> not related to windows of course)

Yes, just changed a build condition in nsHttpNTLMAuth::GenerateCredentials.

Comment 48

[Jim Mathies [:jimm]]

Pushed this to try for a full set of builds and test runs.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4650e59ac608

Comment 49

[Jim Mathies [:jimm]]

Comment on attachment 502667 [details] [diff] [review]
v1, windows only part

This blew up on try server:

make[7]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/netwerk/base/public'
make[6]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/netwerk/base'
make[5]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/netwerk'
NEXT ERROR make[7]: *** No rule to make target `_xpidlgen/nsISSLStatus.h', needed by `export'.  Stop.
make[6]: *** [export] Error 2
make[5]: *** [export] Error 2
make[4]: *** [export_tier_platform] Error 2
make[4]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox'
make[3]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox'
make[2]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox'
make[1]: Leaving directory `/e/builds/moz2_slave/try-w32/build'
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [build] Error 2
make: *** [profiledbuild] Error 2
program finished with exit code 2

Comment 50

[Jim Mathies [:jimm]]

hmm, maybe I was missing some file moves? Did nsISSLStatus.idl get moved around here, I see make file changes but the files aren't in there.

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #50)
> hmm, maybe I was missing some file moves? Did nsISSLStatus.idl get moved around
> here, I see make file changes but the files aren't in there.

diff --git a/security/manager/ssl/public/nsISSLStatus.idl b/netwerk/base/public/nsISSLStatus.idl
rename from security/manager/ssl/public/nsISSLStatus.idl
rename to netwerk/base/public/nsISSLStatus.idl
diff --git a/security/manager/boot/public/nsISSLStatusProvider.idl b/netwerk/base/public/nsISSLStatusProvider.idl
rename from security/manager/boot/public/nsISSLStatusProvider.idl
rename to netwerk/base/public/nsISSLStatusProvider.idl
diff --git a/security/manager/ssl/public/nsIX509Cert.idl b/netwerk/base/public/nsIX509Cert.idl
rename from security/manager/ssl/public/nsIX509Cert.idl
rename to netwerk/base/public/nsIX509Cert.idl

Ok, looks like this didn't apply right locally. will sort this out and try another run.

Comment 52

[Jim Mathies [:jimm]]

Ok, pushed to try again:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-476e3ec27b26

Comment 53

[Jim Mathies [:jimm]]

Comment on attachment 502667 [details] [diff] [review]
v1, windows only part

Ok, that passed cleanly. Looks like we are clear to land. I don't see any issues with the patch, and it's been run through the review wringer multiple times.

Comment 54

[Johnny Stenback (:jst)]

Landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/52384369a8d4

Comment 55

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Backed out in bug 637361.

Comment 56

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.

Comment 57

[Mike Beltzner [:beltzner, not reading bugmail]]

(er updating flag to "-" as per previous comment!)

Comment 58

[Josh Aas]

Is there any reason we can't open this bug up?

Comment 59

[Andrew Bartlett]

I've just been supplied with the Samba ntlm_auth changes required for this, and hope to work on that 'soon'.  

I'm a little unclear on the status of the Linux/ntlm_auth side of things.  I presume you are waiting on the ntlm_auth changes?  If I get the changes into Samba (the patch is against an old version), is there a build I can try that has the patch applied, so I can confirm interoperability?

Thanks,

Comment 60

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Merged and fixed patch, I'll post an interdiff too.

The cause is described here and confirms the fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=637361#c30

When we were connecting an https site thought an ntlm proxy, the url was "https" (really not a good test!) but there were no security info on the channel since we didn't connect the target server yet.  The check for presence of security info was too hard.

I changed the patch to generate CBT only when there is a security info, which is fairly correct.

Brian, if you want, grab the review for your self as you can be more equipped to test (and Jim doesn't object or want to take a look him self, of course).

Comment 61

[Honza Bambas (:mayhemer)]

Attachment: interdiff v1 - v1.1

Comment 62

[Jim Mathies [:jimm]]

Comment on attachment 571490 [details] [diff] [review]
v1.1

basing this on the interdiff, which looks reasonable.

Comment 63

[Honza Bambas (:mayhemer)]

Nit for me: fix typos in the new comment before landing.

Comment 64

[Honza Bambas (:mayhemer)]

There is also no need to move the idl files.  Build system has apparently improved since that time.

Comment 65

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e01dbdb4c7e1

Comment 66

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e01dbdb4c7e1