278885 - cache access denied from (ntlm) proxy on websites that require basic authentication

Status: RESOLVED FIXED

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

Description

[Julian Reich]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20050118 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20050118 Firefox/1.0

In most cases I got directly a 301 after requesting a page secured by basic
authentication. I have been relocated to something like
www.example-domain.com/secure_area/ (from: www.example-domain.com/secure_area -
the slash was missing). Firefox sent a new request which was followed by a 407
from my proxy server. My proxy server offered me ntlm authentication and basic
authentication. Firefox chose ntlm and authenticated transparently. After that I
got a 401 - the webserver only offered basic authentication. Therefore Firefox
chose basic authentication. I entered the correct username and password and a
popup occured telling me to authenticate against my proxy server.

The first problem was that Firefox chose to use basic authentication to
authenticate against the proxy server because it had used it before to
authenticate against the webserver. I've made a change in
nsHttpChannel::GetCredentials that avoids that this selection is performed for
proxy authentication.
I think this change does not break anything because we can only use one proxy
(that requires authentication) anyways and it should not hurt anyone when we
choose the first authentication scheme that we support to authenticate against a
proxy server - just as in all the other requests.
After I had changed this behaviour it worked when requesting
www.example-domain.com/secure_area/ directly (not when requesting
www.example-domain.com/secure_area).

The second problem was that Firefox automatically sent the ntlm type 3 message
when requesting the secured page with basic credentials. It seems as it simply
takes it from the last request. Therefore I added a little check to
nsHttpChannel::ProccessAuthentication to ensure that this message would not be
sent before it has been requested. The ntlm authentication starts again and
everything works fine now.

I'm not very familiar with the mozilla code but I hope that this patch
nevertheless fits into the design of the mozilla code.

I know about https://bugzilla.mozilla.org/show_bug.cgi?id=211843 but because of
the date of the last message and because I'm posting a potential patch I thought
it would be no mistake to create a new bug report. In this bug report a similar
- but not identical - situation is described.

Reproducible: Sometimes

Steps to Reproduce:
1. Setup proxy server (I use squid) offering ntlm and basic authentication.
2. Browse website that requires basic authentication.

Actual Results:  
A popup will occur telling you to authenticate against your proxy server
although you have authenticated already using the ntlm authentication method.

Expected Results:  
After entering correct username and password for the basic authentication on the
requested website the secured content should appear.

Comment 1

[Julian Reich]

Attachment: proposed patch (described in bug report)

Comment 2

[Julian Reich]

Attachment: This one looks much nicer than the one posted last time.

Comment 3

[Darin Fisher]

See also bug 211843.  Is this a duplicate report?

Comment 4

[Darin Fisher]

Comment on attachment 172830 [details] [diff] [review]
This one looks much nicer than the one posted last time.

>+                if (!(precedingAuthFlags & nsIHttpAuthenticator::REUSABLE_CREDENTIALS)) {
>+                    mRequestHead.ClearHeader(nsHttp::Proxy_Authorization);
>+                }

I agree that we ultimately need to remove the Proxy_Authorization header in
some cases, but all that should really require is testing some flag that is
set back when we starting doing proxy auth with an authenticator that doesn't
support reusable creds.


>+            // Do not perform this selection when authenticating against a proxy.
>+            // - jreich
>             //
>-            if (!mAuthType.IsEmpty() && authType != mAuthType)
>+            if (!mAuthType.IsEmpty() && authType != mAuthType && !proxyAuth)
>                 continue;

this is definitely wrong.  we want to do this for both proxies and non-proxies.
what i think we want to do is keep separate state for each instead of lumping
them together like this.  or, we should clear mAuthType once we finish
authenticating with the proxy.

Comment 5

[Darin Fisher]

I actually consider this a pretty major flaw.  Thanks for the initial patch.  If
you don't have time to revise it as I suggested, please let me know, and I will
take it from here.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

+        rv = CallGetService(contractid.get(), (nsIHttpAuthenticator **)
getter_AddRefs(precedingAuth));

this should just be:
  precedingAuth = do_GetService(contractid.get(), &rv);

Comment 7

[Julian Reich]

Attachment: This one fixes more problems and is not as hakish as the version posted 3 months ago.

This version fixes problems that even occur without authenticating against a
webserver. In some special cases the proxy asks us to close the connection. In
this cases we failed to authenticate against the proxy.
I'll make some tests with ntlm authentication on webservers in the next few
days.

Comment 8

[Darin Fisher]

Comment on attachment 182288 [details] [diff] [review]
This one fixes more problems and is not as hakish as the version posted 3 months ago.

Adding to my review queue.  Many thanks for working on this patch!

Comment 9

[Julian Reich]

Attachment: Fixes problems with NTLM on webservers.

Comment 10

[Darin Fisher]

This patch looks pretty good at first glance.  I still need to spend some more
time reviewing it.  More in a bit...

Comment 11

[Darin Fisher]

The logic in this patch looks solid to me.  Well done!

I have some suggestions for cleaning up PrepareForAuthentication.  I am going to
attach a slightly revised version of this patch since it's just easier to do
that then explain the changes I would like to see made :)

Comment 12

[Darin Fisher]

Actually, one thing I noticed:

+    if (NS_SUCCEEDED(rv) && responseStatus != 401 && responseStatus != 407) {
+        // reset the current continuation state because our last
+        // authentication attempt has been completed successfully
+        NS_IF_RELEASE(mAuthContinuationState);
+
+        LOG(("  continuation state has been reset"));
+    }

This block, which is at the bottom of PrepareForAuthentication, will never be
entered.  PrepareForAuthentication is only called from ProcessAuthentication,
which is only called if the response status is 401 or 407.  Instead, I think
that this code should move into ProcessResponse and apply to all responses other
than 401 and 407.  We already have a block of code in ProcessResponse for that
case, and so I will add this NS_IF_RELEASE call into that block.

Comment 13

[Darin Fisher]

I also think that the check in PrepareForAuthentication that looks at
REUSABLE_CREDENTIALS should look at REQUEST_BASED instead.  Because, we should
always send the Proxy-Authorization header along with every request if the
authentication scheme is request based (i.e., for authentication schemes that
conform to RFC 2617).

Comment 14

[Darin Fisher]

Attachment: Revised patch

Comment 15

[Julian Reich]

Darins' version makes me happy ;)

Comment 16

[Darin Fisher]

Comment on attachment 183421 [details] [diff] [review]
Revised patch

marking r=

I'd really like to see this included in firefox 1.1a to maximize user testing.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 183421 [details] [diff] [review]
Revised patch

a=asa

Comment 18

[Darin Fisher]

fixed-on-trunk