150604 - digest authentication problem: Mozilla ignores the stale flag from WWW-Authenticate Response Header.

Status: RESOLVED FIXED

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

Description

[Anatoliy Shibkov]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
BuildID:    2002053012

Mozilla ignores the stale flag from WWW-Authenticate Response Header.

Stale is a flag, indicating that the previous request from the client was
rejected because the nonce value was stale. 
If stale is TRUE (case-insensitive), 
the client may wish to simply retry the request
with a new encrypted response, without reprompting the user for 
a new username and password.
(RFC2617)
But Mozilla reprompts the user in this case as well as 
when stale is FALSE or absent.

Reproducible: Always
Steps to Reproduce:
1. Set the digest authentication scheme on server
   (the implementation must response on incorrect nonce and set stale
    flag in WWW-Authenticate in this case). I use proxy server Oops
    with my modules to test.
2. After changing nonce-value on server side server responses on request based 
on stale nonce with 401 error (WWW-Authenticate with stale=TRUE)

Actual Results:  Client should simply retry the request
with a new encrypted response, without reprompting the user for a
new username and password.

Expected Results:  Mozilla reprompts user for new credentials.

Comment 1

[An-Cheng Huang]

Attachment: patch for the trunk (2002-07-04 06:00)

This patch adds support for the "stale" flag.  An additional method
is added to the nsIHttpAuthenticator interface so that nsHttpChannel can
query the authenticator whether the username/password is still valid
(it's still valid if "stale" is true).

The patch is tested with apache using the AuthDigestNonceLifetime directive.
(The digest-auth test site "http://digest-test.agranat.com/" is down again,
so I couldn't test the patch with it.)

Comment 2

[Ben Bucksch (:BenB)]

Thanks for the patch.
Please poke appropriate people <http://www.mozilla.org/hacking/>.

Comment 3

[An-Cheng Huang]

Attachment: patch for the trunk (0724)

The same patch for yesterday's trunk...

The test site is still down, so the patch is only tested
with apache.

Comment 4

[Darin Fisher]

patch makes sense, but the name IsUserPassValid doesn't make a whole lot of
sense.  there is no username or password passed to the function, and the
authenticator has no knowledge of the username or password.  still, i understand
what the function is meant to do.  perhaps a name like AreCredentialsStale()
might be better, but even that doesn't really suggest that the credentials must
be regenerated as is the case when digest challenge says stale is TRUE.

i think this might be a good name for the function:

  boolean DoesChallengeRequireUserPass(in string challenge);

then you would probably want to modify nsHttpChannel.cpp like this:

@@ -1706,6 +1706,12 @@
     //
     nsHttpAuthEntry *entry = nsnull;
     authCache->GetAuthEntryForDomain(host, port, realm.get(), &entry);
+
+    PRBool requiresUserPass = PR_FALSE;
+    rv = auth->DoesChallengeRequireUserPass(challenge.get(),&requiresUserPass);
+    if (NS_FAILED(rv)) return rv;
+
+    if (requiresUserPass) {
     if (entry) {
         if (user->Equals(entry->User()) && pass->Equals(entry->Pass())) {
             LOG(("clearing bad credentials from the auth cache\n"));
@@ -1737,6 +1742,7 @@
                                getter_Copies(*user),
                                getter_Copies(*pass));
         if (NS_FAILED(rv)) return rv;
+        }
     }
 
     // ask the auth cache for a container for any meta data it might want to

Comment 5

[An-Cheng Huang]

Attachment: modified patch according to darin's comments

I'm not sure about the indentation in nsHttpChannel.cpp though...
(that's why in the earlier patch I put the condition in the two if's)

Comment 6

[Darin Fisher]

Comment on attachment 92913 [details] [diff] [review]
modified patch according to darin's comments

>+    if (requireUserPass) {
>     if (entry) {

>+    }
>     }

sorry, i should have submitted a complete patch.  but, can you
please fix the indentation here.  and let's test it out to make
sure that it doesn't cause any problems.  surely Basic auth will
be unaffected, but we need to verify that Digest auth with a bad
password still results in a new prompt, etc.

BTW: this new API should also be useful for the kerberos auth 
module which doesn't need to prompt the user for a password.

Comment 7

[An-Cheng Huang]

Attachment: patch with indentation fix

This fixes the indentation (darin: I thought you were trying to avoid having
all those lines show up in diffs...)

Comment 8

[An-Cheng Huang]

By the way, I've tested the patch with apache and haven't seen any problems so
far.  I.e. prompts username/password at first, re-prompts if wrong
username/password, no re-prompts after timeout (stale) if username/password
correct, etc...

Comment 9

[Darin Fisher]

Comment on attachment 93000 [details] [diff] [review]
patch with indentation fix

sr=darin

thx!!

Comment 10

[Darin Fisher]

will try to get this into 1.1

Comment 11

[Darin Fisher]

actually, i think this should wait for 1.2alpha.  i don't want to risk any auth
regressions for 1.1beta.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 93000 [details] [diff] [review]
patch with indentation fix

r=dougt

Comment 13

[Darin Fisher]

waiting for the trunk to open for 1.2alpha

Comment 14

[Darin Fisher]

fixed-on-trunk