Closed
Bug 15860
Opened 25 years ago
Closed 23 years ago
[RFE] Digest authentication?
Categories
(Core :: Networking: HTTP, enhancement, P3)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla0.9.7
People
(Reporter: anders, Assigned: darin.moz)
References
Details
(Keywords: relnote, testcase)
Attachments
(3 files, 16 obsolete files)
2.62 KB,
text/html
|
Details | |
65.98 KB,
patch
|
darin.moz
:
review+
rpotts
:
superreview+
|
Details | Diff | Splinter Review |
550 bytes,
patch
|
Details | Diff | Splinter Review |
Mozilla should support the "digest" authentication scheme, in addition to
"basic" authentication.
Basic authentications has a LOT of shortcomings, among them that it sends
cleartext passwords. It also has no way of forcing a browser to "forget" a
password without restarting it.
Digest authentication solves most of these problems and has been available in
the Apache web server for years...
Digest authentication is specified in RFC 2617.
Comment 1•25 years ago
|
||
2 months and no comment yet from the developers? As Mozilla nears feature lock
for beta, I am worried that once again Mozilla/Netscape is ignoring yet another
security enhancment for God know what reason.
Reporter | ||
Comment 2•25 years ago
|
||
At least it got assigned to Milestone 16, whatever that is...
Bulk move of all Necko (to be deleted component) bugs to new Networking
component.
Comment 4•25 years ago
|
||
You might want to use the new MD5 implementation from Alladin instead of the
reference implementation. The C code is a lot more modern and the license is
much more liberal. Here is the Freshmeat entry:
http://www.freshmeat.net/appindex/1999/05/04/925811006.html
Comment 5•25 years ago
|
||
And since it has already been determined that export regs do not apply to
message signing (authentication) algorithms, there is no problem inplementing
this in Mozilla.
Comment 6•25 years ago
|
||
As if it weren't bad enough, SeaMonkey currently seems to use Basic
authentication even when server sends a Digest challenge only. So it is as
broken as Netscape 4.7... Oh well, M$ IE 5 doesn't behave correctly either,but
at least it is possible to use the digest auth there. Face it, guys: Sending
passwords out in the clear is as bad a security hole as they can be.
Comment 7•25 years ago
|
||
What kind of crap is this that my votes are removed from this bug? I guess
Mozilla couldn't handle that people wanted certain bugs fixed and voted more
than one vote to them.
Comment 8•25 years ago
|
||
Jerry, please take it to an appropriate forum, not here.
Comment 9•25 years ago
|
||
Moving to M17 which is now considered part of beta2.
Target Milestone: M16 → M17
Comment 10•25 years ago
|
||
Putting on [nsbeta2-] radar. Not critical to beta2. Unless someone can put to a
top site and a reprodicible testcase.
Whiteboard: 1w → [nsbeta2-]1w
Comment 11•25 years ago
|
||
The days of cleartext passwords are numbered, basic auth just doesn't cut it.
The plumbing is there for m5d digest authentication, why not just use it.
Comment 12•25 years ago
|
||
Comment 13•25 years ago
|
||
The patch above supports RFC 2069 and much of RFC 2617's additions.
Really, only two things are missing: support for "auth-int" quality of
protection extension, and client-side nonce counting. Additionally, the
client nonce generation could be better. These are all optional extensions
onto the digest auth mechanism, though, and are very unlikely to be widely
used. Basically, this provides digest auth for the vast majority of current
server implementations.
Comment 14•25 years ago
|
||
The digest patch doesn't include nsNetwerkMD5.[cpp|h], therefore mozilla
doesn't compile after applying the patch.
Comment 15•25 years ago
|
||
Comment 16•25 years ago
|
||
I accidentally posted the wrong file originally (just the patch).
The new attachment is a tar.gz with the new files and the patch.
Comment 17•25 years ago
|
||
shaver: can you review this? I'd like an extra pair of eyes to go over this.
thx.
Target Milestone: M17 → M18
Comment 18•25 years ago
|
||
The patch currently posted is getting a bit old, so I'll post a new patch to
resolve any merge problems with the current CVS code. Also, the code changes in
nsHTTPChannel::Authenticate() will change slightly if my patch for bug 44041 is
approved, which is, arguably, more important than this one (since this is an
enhancement and 44041 is an actual bug).
Comment 19•24 years ago
|
||
Comment 20•24 years ago
|
||
The tests available through http://digest-test.agranat.com/ test both basic and
digest authentication, both for single realms and multiple realms. M18 fails 3
out of 4 tests, since it doesn't have digest authentication support yet nor does
it support multiple realms, a feature that would probably be part of the
Password Manager.
The site was apparently set up by a coauthor of RFC 2617.
Comment 21•24 years ago
|
||
M$ IE 5.5 can pass all the tests available through http://digest-
test.agranat.com/, but M18 can't.
Comment 23•24 years ago
|
||
We've had a patch here since June, could we please get it reviewed, super-
reviewed, and examined by the module owner? It would be a shame to have Justin's
work go unloved...
Justin: Thanks for the patch!
Keywords: correctness,
testcase
Comment 24•24 years ago
|
||
*** Bug 58418 has been marked as a duplicate of this bug. ***
Comment 25•24 years ago
|
||
Just for the record: Apache supports disgest, squid (a popular http cache) will
support digest in squid 2.5 (the code is available via CVS). In squid the cache
supports the client side nonce, and process's auth-info headers correctly. It
does this in acceleration and proxy modes. There is a good chance that it will
support message integrity as well by the time 2.5 is released.
I expect that the exposure and demand for rfc 2617 compliance will increase
with both these popular products supporting it. I tested mozilla 0.7 (sorry not
the snapshot - no time) with squid sending _only_ a digest authentication
challenge and it failed.
Comment 27•24 years ago
|
||
Can we get reviews, please, so this can get into 0.9.2?
Component: Networking → Networking: HTTP
Assignee | ||
Comment 28•24 years ago
|
||
the code will have to be brought up to date since much of the http core arch
has changed. who owns this patch?
Comment 29•24 years ago
|
||
Comment 30•24 years ago
|
||
The above patch (for 0.9.2 release) provides the HTTP Digest Authentication
(partially). It is based on Justin's patch from June 2000.
The following are my testing results.
What works:
Digest authentication with Apache 1.3.20/mod_auth_digest, which uses
"MD5" and "auth" only (no MD5-sess and auth-int)
What doesn't work:
1. The test site in the previous attachment, which uses MD5-sess.
I guess the problem is with MD5-sess, but since Apache doesn't
support MD5-sess yet, I can't really test it.
2. Justin's patch doesn't support auth-int.
3. I did not test Proxy-Authentication/-Authorization at all, so it might
not work.
Here are some implementation issues:
1. The NS_HTTPDIGESTAUTH_CID defined in build/nsNetCID.h is copied from
the NS_DIGESTAUTH_CID from Justin's patch, though I have no idea how the
CID is generated.
2. Digest authentication uses a new authorization request header for every
request, since A2 includes the request-uri. The original
nsHttpChannel::AddAuthorizationHeaders() uses the credential in the
authCache. This works for Basic authentication, but not Digest.
It should be possible to check whether it is Digest, and if it is,
generate a new credential in AddAuthorizationHeaders().
However, since I wanted to reuse existing code as much as possible,
I changed AddAuthorizationHeaders() so that it doesn't add Authorization
if the credential is for Digest. This will result in a 401 response,
which will be handled by ProcessAuthorization().
3. GetCredentials() needs to get the username/password from the authCache
so that we don't prompt the user for every request, but we can't get the
username/password from the cached credential. I modified the nsHttpAuthCache
to store the A1 part for Digest credentials. A new function
generateCredentialsFromA1() is added to nsIHttpAuthenticator.idl to handle
this case.
If you look at my patch, you'll probably notice that I'm not an experienced
C++ programmer and I've never worked on Mozilla until a few days ago when
I tried to set up Apache with digest authentication.
So I'm sure much of this can be written in a much better way.
However, since this "bug" has been around for a long time and Justin's
patch has been there for more than a year, I just hope this can at least
be a starting point of a good implementation.
Comment 31•24 years ago
|
||
I forgot to mention that the attachment is tar.gz, and the tests were done on
a Linux box (RH7.1).
Comment 32•24 years ago
|
||
current patch is tar.gz in case anyone is wondering.
This is the second (third?) implementation of MD5 i've seen in a week.
First my traditional licensing gripe: If you aren't a netscape employee you
have no business using NPL, please use MPL instead, the change is very simple,
please see http://www.mozilla.org/MPL/boilerplate-1.1/
as an additional comment, please use the Contributors: section unless you have
some specific need (the only two instances I know of are IBM and another thing
which I hope to squish), an entry in contributors can have a parenthetical
explaining what someone added.
Personally I'm opposed to Contributors because I'd prefer to rely on
CVSBlame/CVSLog + Bugzilla, but if you need to clutter the code w/ the
reference the contributors section is appropriate.
As to MD5 itself... I think NSS has an implementation (lord/wtc?) and think you
should probably use it. Otherwise you run the risk of mucking w/ our security
encapsulation which allows people to build mozilla in places that forbid
encryption (i'm not sure MD5 checksums count, if they don't then please cite
something -- i'm tired).
mozilla code generally groups all includes together.
please split all if (...) return ...; statements across two lines to aid
debugging. (you do it right in nearly all instances...)
don't use tabs. [follow modelines if the exist]
is there a reason to use this logic:
+ if (!entry) {
+ if (creds) {
+ entry = new nsEntry(path, realm, creds, a1);
+ if (!entry)
+ return NS_ERROR_OUT_OF_MEMORY;
+ mList.AppendElement(entry);
+ }
+ // else, nothing to do
+ }
+ else if (!creds) {
+ mList.RemoveElementAt(i);
+ delete entry;
w/o reading the code carefully,
if (creds) {
if (entry) {
...
} else {
...
}
} else {
...
}
seems like a better arrangement
!PL_strncasecmp(creds.get(), "digest ", 7);
is there a reason not to use scc's string compare functions?
+ nsCString A1;
+ A1.AssignWithConversion(user.get());
+ A1 += ":";
+ A1 += realm.get();
+ A1 += ":";
+ A1.AppendWithConversion(pass.get());
I think you should probably rewrite this (scc, advice?)
+ // now need to store A1, too
+ //return authCache->SetCredentials(host, port, path, realm.get(),
creds.get());
+ return authCache->SetCredentials(host, port, path, realm.get(),
+ creds.get(), A1.get());
changing api signatures is always a concern, but better sooner than post 1.0 i
suppose.
Foo::Bar::Baz is taboo, we don't use namespaces.
http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace
your endian handling worries me, comments from nspr people (wtc)?
statics usually make me worry about threadsafeness
authString += "\", algorithm=";
if (algorithm & ALGO_MD5_SESS)
authString += "MD5-sess";
else
authString += "MD5";
why not
authString += "\", algorithm=MD5";
if (algorithm & ALGO_MD5_SESS)
authString += "-sess";
the pattern a+="cstring";a+=nsCString(something);... bothers me, i'm sure scc
will have advice. I think
a+=NS_LITERAL_CSTRING("cstring")+something+NS_LITERAL_CSTRING("anotherCstring")
+somethingElse/*+...*/; is probably better.
Instead of long // blocks of code, please use #if 0 or #ifdef
FIXME_HTTP_DIGESTAUTH so that lines which aren't changed when it's fixed don't
mess up cvs blame.
for long comment blocks /* */ would be nice...
nsHttpDigestAuth::ExpandToHex looks like a function which should already exist
in our string routines...
Thanks for working on this.
cid's can be generated by connecting to irc.mozilla.org and /msg mozbot uuid.
Whiteboard: [nsbeta2-]1w
Comment 33•24 years ago
|
||
Comment 34•24 years ago
|
||
The above patch (tar.gz file) includes fixes for a few problems mentioned
in the previous comment:
1. tabs are eliminated.
2. long // blocks of code from Justin's patch are changed to "#if 0"
3. long // coment blocks from Justin's patch are changed to /* */
Sorry about the licensing issue. I created nsHttpDigestAuth.h|cpp by copying
nsHttpBasicAuth.h|cpp, and that's why the NPL is there. I didn't even notice
that it is NPL until you mentioned it. However, since much of the code came
from Justin's patch (nsDigestAuth.h|cpp), which also used the NPL, I don't
think I have the right to change it (?).
As for the MD5 implementation, the nsNetwerkMD5.h|cpp in my patch were copied
from Justin's patch without any changes (I guess this is why you mentioned
the use of the "Contributors", since these two files don't use it).
I didn't look at how it works or its legal implications. If there is an
"official" implementation somewhere in the source tree, I can certainly use
it. But my goal was to make Justin's patch work under the new netwerk code,
and I wanted to re-use as much existing code as possible and make as few
modifications as possible. Many of the things you mentioned are a result of
this, so I probably shouldn't change them. I list these things below:
1. scattered includes in nsNetModule.cpp: I was just following how
nsHttpBasicAuth was included there
2. "if (...) return...": I don't usually use this. The ones you see are
a result of copy-and-paste. I fixed those in nsHttpDigestAuth.cpp, but
there are a lot of them in the original nsHttpChannel.cpp, and I don't think
I should change those.
3. the logic you quoted from nsHttpAuthCache.cpp: it's an exact copy of
the original function, so I don't know why it's written that way.
4. the PL_strncasecmp: I copied it from Justin's code. I don't know
what's the "right" way to do it. (I don't even know who scc is...)
5. the nsCString concat thing: I know it's really ugly. I copied it
from somewhere (I forgot), and I don't know what's the "right" way to
do it. Please advise.
6. "Foo::Bar::Baz" in nsHttpAuthCache.cpp: I didn't change the definition.
I only added more functions to those classes.
In addition to these issues, my greater concern is that my approach to
extend the AuthCache to support digest authentication was simply a hack.
I'm sure there is a better way to extend the AuthCache interface.
The same problem applies to the nsIHttpAuthenticator interface.
(For example, the nsHttpChannel probably shouldn't have to look at the
challenge/credential and take different actions according to the
authentication type.)
Comment 35•24 years ago
|
||
Tried the latest patch on the trunk on linux 2.4/SuSE. Had to tweak it a bit to
get it compiling (add a mising method to nsHttpDigestAuth.h).
On the testsite http://digest-test.agranat.com/ the digest tests still fail. The
code seems to not call the prompt box for user/password.
Comment 36•24 years ago
|
||
Sorry I forgot to mention that the new patch is still for 0.9.2 release.
In my own testing, it works with Apache/mod_auth_digest, but the
digest test on "http://digest-test.agranat.com" failed. However,
it did prompt me for username/password, and an authorization header was
generated. According to the test trace, the server says "authorization
forgery", which probably means the calculation of the "response" field
is wrong (since the other fields are correct). As I mentioned previously,
my guess is that there is a bug in the MD5-sess part of the patch, which
I haven't looked into. (Apache/mod_auth_digest only supports MD5 currently,
and I actually fixed a few bugs in th MD5 part by debugging from both sides.)
So the first problem is that something was changed after 0.9.2 so the patch
doesn't prompt the user any more. The second problem is the MD5-sess.
Comment 37•24 years ago
|
||
My code can be changed to MPL.
I copied the MD5 code from the mailnews module long ago. I could not figure
out whether MD5 was provided in NSS, nor how to use NSS, so I just followed
mailnews' lead. The are other authentication methods which need various
encryption and hashing algorithms (SHA, DES, etc). So knowing how to access
those implementations in NSS would be good to know? Any places in particular to
look for documentation/examples?
Comment 38•24 years ago
|
||
+relnote - we shoudl release note that this didn't make it into current versions
yet.
Keywords: relnote
Comment 39•24 years ago
|
||
Comment 40•24 years ago
|
||
After staring at the RFC for a while, I found that H() denotes the hex string
of the MD5 hash, not the binary data. This caused the problem in the MD5-sess
part of the patch, which uses H(). The above patch fixes this problem, and my
testing shows that it passes the first two digest tests on the test site
"http://digest-test.agranat.com". (The third test uses the authentication-info
header...)
Comment 41•24 years ago
|
||
I applied the new patch on the trunk and this time I applied the *whole* patch.
Now I'm getting the user/password box, but I'm not getting past that, it returns
every time unless I cancel. I will check if patch has done the right thing ...
Comment 42•24 years ago
|
||
I hate patch ... okay the code also works on the trunk as it does on the 0.9.2
branch described by pach.
Comment 43•24 years ago
|
||
timeless@mac.com wrote:
> As to MD5 itself... I think NSS has an implementation
> (lord/wtc?) and think you should probably use it.
You should not call the MD5 code in NSS directly.
You should call PSM's MD5 interface, which is declared
in mozilla/caps/idl/nsISignatureVerifier.idl.
(http://lxr.mozilla.org/seamonkey/source/caps/idl/nsISignatureVerifier.idl#54)
> your endian handling worries me, comments from nspr
> people (wtc)?
I am sorry that I can't review the patch so I have no
comments to add regarding the endian handling.
Comment 44•24 years ago
|
||
i don't see how we could use that interface, mstoltz?
Comment 45•24 years ago
|
||
Is this how to use the interface?
nsresult rv;
nsCOMPtr<nsISignatureVerifier> verifier =
do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);
It seems to work (limited testing with the Hash* functions), but
is this the "right" way to do it?
Regarding the endian handling: the only piece of code that involves
endian handling is the MD5 implementation (as far as I can see).
So if the official implementation is used, there shouldn't be any
problems.
Comment 46•24 years ago
|
||
RELNOTE for NS 6.1:
Digest authentication is not supported in this release.
Comment 47•23 years ago
|
||
Comment 48•23 years ago
|
||
The above new patch (tar.gz) is for the trunk (2001-08-21). According
to my own testing, it passed all four tests on the test site
"digest-test.agranat.com" (though I'm not sure why it actually passed
the fourth one). Also:
1. It now uses the "official" MD5 implementation (using the method I
described in my previous comment).
2. The authentication scheme is now transparent to the nsHttpChannel
(i.e. it doesn't have to look at "basic"/"digest" any more).
3. There is no longer an extra round-trip overhead for subsequent
requests.
4. The license of the two new files is changed to MPL. (However, I
don't know how to fill in the "fields", so I left that paragraph
untouched.)
Some old problems probably still apply, though.
(E.g. string handling.)
Comment 49•23 years ago
|
||
OK, what's it going to take to get this in? Can someone summarize in small
words what the status is on this patch? Is it "done" and ready for review?
Comment 50•23 years ago
|
||
Status of the patch:
On my machine, it passes all tests on the digest auth test site, and
it also works with apache/mod_auth_digest. That's pretty much all
I can test by myself. I don't know if anyone else has had time to
look at it.
Comment 51•23 years ago
|
||
I did some tests on an older version of the patch on linux and it passed all
tests on the given testside it was supposed to pass.
Comment 52•23 years ago
|
||
Have you tested digest proxy-authentication?
And also tested that when presented with multiple challenges - say basic, digest
in that order, that digest is used?
Comment 53•23 years ago
|
||
No, I haven't tested anything other than what I mentioned above.
For proxy-auth: it would be great if someone who has a proxy can test this.
For multi-challenge: it would be great if someone who has a server that does
multi-challenge can test this. (However, this is handled by the original
code, i.e. not part of the patch.)
Comment 54•23 years ago
|
||
> For proxy-auth: it would be great if someone who has a proxy can test this.
Squid version 2.5 has primitive digest support. (It doesn't support trailers,
and thus cannot do message body authentication. It also doesn't support two way
authentication - no time to do that yet). I don't have a build environment for
mozilla, and squids build environment is _much_ smaller than mozilla's, so I'll
pass the task to someone else. Unless someone wants to provide me with a mozilla
binary with this patch in it.
> For multi-challenge: it would be great if someone who has a server that does
> multi-challenge can test this. (However, this is handled by the original
> code, i.e. not part of the patch.)
Oh, ok. Well I think apache does multi challenge, just pick and choose the
order. If it doesn't then squid definately doesn, just choose the order the
auth_param lines occur in.
Comment 55•23 years ago
|
||
The issue of presenting multiple authentication methods to the client, and
choosing an appropriate one is covered by bug 44041. It should work with proxy
authentication, too, but I never tested that case. I don't know if anyone else
has either.
Assignee | ||
Comment 56•23 years ago
|
||
the changes to the existing code seem large.
an-cheng: can you summarize the changes made to the existing code and explain
why they are necessary? ie. what was not provided by the nsIHttpAuthenticator
interface as well as the code in nsHttpAuthCache and nsHttpChannel?
Comment 57•23 years ago
|
||
The main thing is that digest auth needs to generate a new credential for
every request (with a different URI), since the request URI is used in
the calculation. Therefore, storing the credential in the AuthCache is
not enough; username and password (or the hash of them) also need to be
cached. As a result, for each subsequent request, basic auth will simply
use the cached credential, while digest auth uses the cached user & pass
to generate a new credential.
To make this transparent to the nsHttpChannel, a function
NewCredentialRequired is added to the nsIHttpAuthenticator interface so
that HttpChannel can query the interface to see whether to use the cached
credential or to generate a new credential, instead of looking at the
challenge string to determine the auth scheme. This is the largest
change in nsHttpChannel.
nsHttpAuthCache is changed to also store the username, password, and
challenge. The majority of the changes (in terms of line count) in this
patch is here. The reason for the high line count is that I simply
overloaded all relevant functions, so there are essentially two copies of
each function. In fact, if the old AuthCache interface is not used
anywhere else, it can be removed.
Assignee | ||
Comment 58•23 years ago
|
||
nsHttpChannel is the only consumer of nsHttpAuthCache, so please cleanup the
unused functions (BTW: you can use lxr.mozilla.org to verify dependencies).
also, i would prefer NewCredentialsRequired (instead of NewCredentialRequired),
since this would be more consistent with the existing "grammar."
Comment 59•23 years ago
|
||
Comment 60•23 years ago
|
||
The above patch cleans up nsHttpAuthCache (now it has the new interface).
Also, the function name is changed to NewCredentialsRequired.
Comment 61•23 years ago
|
||
*** Bug 98497 has been marked as a duplicate of this bug. ***
Comment 62•23 years ago
|
||
Comment 63•23 years ago
|
||
the above patch contains some minor changes to make it work with the current
trunk.
by the way, it seems that the test site "digest-test.agranat.com" is no longer
accessible, so I could only test the patch with apache/mod_auth_digest
(1.3.20).
Updated•23 years ago
|
Keywords: mozilla1.0
This patch has been languishing for too long. It should be checked in or
rejected. Whose door needs to be banged on?
Comment 65•23 years ago
|
||
Darin, you're "it" -- feel free to reassign to a delegate who will not let this
patch rot. If it should wait till after 1.0, make that case. Given how long it
has waited, and the standards compliance angle, if we can take the patch without
spending lots of followup or support effort, I'm in favor of it.
Thanks,
/be
Assignee: neeti → darin
Comment 66•23 years ago
|
||
This is an internet standard, and I'd be interested in hearing reason why it
should be deferred out of 1.0.
My case for inclusion 1.0 or earlier is made here:
http://www.packetgram.com/pktg/docs/edit/july.html
Assignee | ||
Comment 67•23 years ago
|
||
Assignee | ||
Comment 68•23 years ago
|
||
ok, i've skimmed the patch. it looks good for the most part... there is some
cleanup that needs to be done. i'll have some review comments once i get a
chance to do a more thorough reading of the patch.
Assignee | ||
Updated•23 years ago
|
Summary: Digest authentication? → [RFE] Digest authentication?
Assignee | ||
Comment 70•23 years ago
|
||
this latest patch cleans up a lot of the auth cache stuff. it doesn't touch
nsHttpDigestAuth.{h,cpp} other than to replace a few prints with LOG
statements.
i still need to run this patch through some tests.
Attachment #10103 -
Attachment is obsolete: true
Attachment #10580 -
Attachment is obsolete: true
Attachment #43081 -
Attachment is obsolete: true
Attachment #43173 -
Attachment is obsolete: true
Attachment #43329 -
Attachment is obsolete: true
Attachment #46731 -
Attachment is obsolete: true
Attachment #48257 -
Attachment is obsolete: true
Attachment #53418 -
Attachment is obsolete: true
Attachment #55673 -
Attachment is obsolete: true
Assignee | ||
Comment 71•23 years ago
|
||
Attachment #57695 -
Attachment is obsolete: true
Comment 73•23 years ago
|
||
+ if (!node) {
+ // only create a new node if we have a real entry
+ if (!creds && !user && !pass && !challenge)
return NS_OK;
Do basic auth requests have a challenge?
+ // this is random, and should be changed
+ nsCAutoString cnonce("0123456789abcdef");
OK, should it be changed?
Comment 74•23 years ago
|
||
Ideally, "cnonce" should be a random string. The current approach works
(i.e. it does not violate the RFC), but by having the same cnonce for
all requests it defeats the purpose of using the cnonce.
In addition, "nonce_count" is currently always "00000001". This will fail
if the server checks this field. The reason that it works now is that,
in apache 1.3.20, mod_auth_digest only checks nonce_count if:
(AuthDigestNcCheck is On (default Off)) &&
(the SHMEM_MM feature is On (default Off)). To implement this correctly,
nonce_count needs to be a per-nonce counter (i.e. incremented for each
request that uses a particular nonce).
Assignee | ||
Comment 75•23 years ago
|
||
a basic auth challenge looks like: basic realm="foo"
in other words, the challenge the value of the WWW/Proxy-Authenticate header.
Assignee | ||
Comment 76•23 years ago
|
||
An-Cheng: do you think its important that we get the cnonce and nonce_count
right before landing this patch?
Comment 77•23 years ago
|
||
I don't really know the criteria for landing a patch...
In terms of "working code", the patch will work for most people that use
apache. In fact, the AuthDigestNcCheck directive is marked as "not
implemented yet" in apache documentations, so it's not supposed to be
turned On. I haven't had a chance to test the patch with squid, IIS,
or any other servers, so I don't know what their defaults are.
In terms of correctness, the cnonce is not "wrong" (since it is supposed
to be anything chosen by the client). The nonce_count probably violates
the RFC, but since its purpose is "to allow the server to detect request
replays", this may not be important if the servers don't check nonce_count.
Additionally, there are a few unimplemented features (auth-int QoP is the
most obvious one, but mod_auth_digest hasn't implemented it, either), so
this is going to be a partial implementation of the RFC anyway...
Comment 78•23 years ago
|
||
With a fixed nonce-count, Mozilla will fail to interoperate with servers which
correctly implement the protocol specification. Thus we should not land the
patch until nonce-count is correctly implemented.
Assignee | ||
Comment 79•23 years ago
|
||
yeah, i agree with that. we should get nonce_count correct before landing this
patch.
An-Cheng: are you going to implement nonce_count, or should i take a stab at it?
Assignee | ||
Comment 80•23 years ago
|
||
perhaps one way to support nonce-count would be to have an extra nsISupports
opaque parameter stored in the auth cache with each entry. nsIHttpAuthenticator::
GenerateCredentials could take a corresponding inout nsISupports parameter.
digest auth could use a nsISupportsPRUint32 for this and simply increment the
stored value (the nonce-count) each time GenerateCredentials is called. sound good?
Comment 81•23 years ago
|
||
Darin: that's also my first thought. At the moment I am not able to
implement it, so it would be great if you can do it. Also, the
"XXX bug"'s in nsHttpDigestAuth.cpp can be removed now (I put them there
to remind myself where I modified the original code). Thanks.
Comment 82•23 years ago
|
||
By the way, the cnonce should be much easier to implement, probably even
2 or 3 lines of code, e.g. get a random number (there must be an RNG
somewhere in the source tree...), convert it to a hex string, and use it
as the cnonce. I think this is sufficient (but I am not a security
expert...).
Assignee | ||
Comment 83•23 years ago
|
||
An-Cheng: thanks for the info... i'll work on a patch.
Assignee | ||
Comment 84•23 years ago
|
||
1- storing an extra nsISupports parameter in the auth cache. digest auth uses
this to store the nonce-count. it's incremented each time GenerateCredentials
is called.
2- cnonce is calculated using the rand() system call... is this ok/sufficient?
from what i could tell, rfc 2617 isn't very specific about how to calculate a
cnonce.
Attachment #57739 -
Attachment is obsolete: true
Assignee | ||
Comment 85•23 years ago
|
||
Assignee | ||
Comment 87•23 years ago
|
||
ok, the latest patch is ready to be reviewed.
Assignee | ||
Comment 88•23 years ago
|
||
Comment on attachment 57976 [details] [diff] [review]
v1.4 removed old XXX bug? comments
trigger happy or something :/
Attachment #57976 -
Attachment is obsolete: true
Comment 89•23 years ago
|
||
+ nsXPIDLString &user,
+ nsXPIDLString &pass)
is it a good idea to be passing XPIDLStrings as function parameters?
Comment 90•23 years ago
|
||
blizzard's right -- nsXPIDLString and nsXPIDLCString should be used only as
types of local variables, for automatic management of PRUnichar** and char** out
param result storage when calling XPCOM methods.
/be
Comment 91•23 years ago
|
||
I tested the v1.4 patch with squid-head-200111140000, and it didn't work.
After looking into it, I found two places in squid's implementation that
differ from my intepretation of RFC 2617:
1. squid assumes the "qop-value" in Authorization Request Header is quoted,
but that seems to be wrong (see sections 3.2.2 and 3.5).
2. squid assumes the "algorithm" in Authorization Request Header is
present, but if we assume the definition of this field is the same as
that in section 3.2.1, it is optional and defaults to "MD5" (see also
section 3.5). (Again, squid assumes the algorithm value is quoted,
which seems to be wrong.)
After slightly modifying the squid implementation
(authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
successfully authenticates with squid. (Note that I also tried IE 5.5,
and it works fine without the modifications...)
However, there is another problem:
after the authentication, mozilla segfaults! The reason that I think the
authentication is successful is that I can see the TCP_MISS/200 following
the TCP_DENIED/407 in squid's access.log, and I also see the actual 200
response in tcpdump. I tried a few different sites, and the results are
the same (segfault). Of course, mozilla works fine if no authentication
is required...
Comment 92•23 years ago
|
||
> 1. squid assumes the "qop-value" in Authorization Request Header is quoted,
> but that seems to be wrong (see sections 3.2.2 and 3.5).
3.5 shows qop-value as being quoted and not quoted. Hmm that is unclear isn't
it. However section 3.2.1 shows quoted values for this, and 3.2.2.1 explicitly
unquotes the value before use. Perhaps we should seek clarification from the
WWW?
> 2. squid assumes the "algorithm" in Authorization Request Header is
> present, but if we assume the definition of this field is the same as
> that in section 3.2.1, it is optional and defaults to "MD5" (see also
> section 3.5). (Again, squid assumes the algorithm value is quoted,
> which seems to be wrong.)
As for this being present, thats a bug, and I will rectify in squid pre 2.5
being release. However, for quotation the same applies as for the qop
quotation.
> After slightly modifying the squid implementation
> (authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
> successfully authenticates with squid. (Note that I also tried IE 5.5,
> and it works fine without the modifications...)
Because, in this case, IE is closer to the standard.
I can't comment on the mozilla segfault though.
Comment 93•23 years ago
|
||
> > 1. squid assumes the "qop-value" in Authorization Request Header is quoted,
> > but that seems to be wrong (see sections 3.2.2 and 3.5).
>
> 3.5 shows qop-value as being quoted and not quoted. Hmm that is unclear isn't
> it. However section 3.2.1 shows quoted values for this, and 3.2.2.1
> explicitly
> unquotes the value before use. Perhaps we should seek clarification from the
> WWW?
Actually, I think the definitions are quite clear...
From 3.2.1: (Authenticate Response Header)
qop-options = "qop" "=" <"> 1#qop-value <">
qop-value = "auth" | "auth-int" | token
From 3.2.2: (Authorization Request Header)
message-qop = "qop" "=" qop-value
So from these definitions, the qop-options in Authenticate should be quoted
(note the explicit <"> that represents an actual quote), since there may
be more than one qop-value. On the other hand, the message-qop in
Authorization should not be quoted, since qop-value is unquoted.
Also, from the description of qop in section 3.2.2 (page 11,
second paragraph, 4th line): "Note that this is a single token, not a
quoted list of alternatives as in WWW-Authenticate."
This is also confirmed by 3.5 (examples), where the qop in Authenticate
is a quoted list, and the qop in Authorization is an unquoted token.
> > 2. squid assumes the "algorithm" in Authorization Request Header is
> > present, but if we assume the definition of this field is the same as
> > that in section 3.2.1, it is optional and defaults to "MD5" (see also
> > section 3.5). (Again, squid assumes the algorithm value is quoted,
> > which seems to be wrong.)
>
> As for this being present, thats a bug, and I will rectify in squid pre 2.5
> being release. However, for quotation the same applies as for the qop
> quotation.
Again, from 3.2.1: (Authenticate Response Header)
algorithm = "algorithm" "=" ( "MD5" | "MD5-sess" |
token )
Since there are no explicit quotes (i.e. <">), the value should be an
unquoted token. (If in doubt, see the ABNF definition in RFC2616 section 2.)
> > After slightly modifying the squid implementation
> > (authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
> > successfully authenticates with squid. (Note that I also tried IE 5.5,
> > and it works fine without the modifications...)
>
> Because, in this case, IE is closer to the standard.
Which standard?
Assignee | ||
Comment 94•23 years ago
|
||
blizzard, brendan: yeah, there's no reason for me to be passing around
nsXPIDLString's... will make the change.
Comment 95•23 years ago
|
||
> Actually, I think the definitions are quite clear...
...
>Also, from the description of qop in section 3.2.2 (page 11,
>second paragraph, 4th line): "Note that this is a single token, not a
>quoted list of alternatives as in WWW-Authenticate."
Granted. Thanks for the pointers. (It's been some time since I reviewed this in
any detail.)
...
>> Because, in this case, IE is closer to the standard.
>Which standard?
RFC 2617. Can I get your patch for squid if you are willing to contribute it,
as it will save some time.
Comment 96•23 years ago
|
||
rbtcollins: I don't really have a patch, as I only hacked it for my
purpose (testing this mozilla patch). What I did was: use the 4 chars
after "qop=" for qop-value, and default to "MD5" if algorithm is NULL.
For a real patch for squid, you probably need more than this.
Also, could you let us know when you make the changes in squid?
(So that it will be easier to test this patch and figure out what's
causing the segfault.)
Comment 97•23 years ago
|
||
Comment on attachment 57977 [details] [diff] [review]
v1.4 removed old "XXX bug?" comments
Not sure I understand why we need allocateExtra on the http authenticator
interface.
Is nsCString to expensive to use for all of your string allocation (mPath,
mRealm, ect.)
Fix this:
+#include "caps/nsISignatureVerifier.h"
There is alot of parsing in this code. We should really run Purify over this
and bang on it.
In the GenerateCredentials function, there are alot of TODO statements. Should
these be addressed before you land this?
Comment 98•23 years ago
|
||
This patch fixes the "caps/nsISignatureVerifier.h" issue.
The TODO's are for the not-yet-implemented "auth-int" QoP option,
which provides message integrity in addition to authentication.
As far as I know, apache and squid haven't implemented it, but
I don't know about other browsers/servers...
Comment 99•23 years ago
|
||
FYI, for interop with IIS, you should accept *all* Digest challenge parameters
quoted and unquoted: IIS quotes some which shouldn't be quoted (or possibly the
other way round, can't remember). (that is sensible "be lenient in what you
accept behaviour anyway)
Comment 100•23 years ago
|
||
Yes, this patch does accept all parameters quoted/unquoted (this is from
Justin's original patch).
Assignee | ||
Comment 101•23 years ago
|
||
this patch is ready to be reviewed.
Attachment #57977 -
Attachment is obsolete: true
Attachment #58578 -
Attachment is obsolete: true
Comment 102•23 years ago
|
||
Sorry; I checked through my archives: the thing IIS5 requires is that you *send*
the qop= and algorithm= values quoted in the request header; otherwise it gives
a 500 response. So in effect every parameter is quoted except nc; the v1.6 patch
doesn't seem to do this.
Despite the fact that this isn't what 2617 says, it seems to work interoperably
(having tested against Apache/mod_auth_digest, and several of the commercial
WebDAV servers which support digest).
Assignee | ||
Comment 103•23 years ago
|
||
what a lovely dilema... if we implement RFC2617 then we most likely won't work
with IIS (and possibly other servers), whereas if we go against RFC2617, then we
most likely will cause problems for yet another set of servers and/or yet to be
written servers. my inclination therefore is to implement RFC2617, and then
file a bug to support IIS.
to support IIS we would first need a testcase. we could add code to
nsHttpDigestAuth that sniffs the Server response header. if it finds a broken
web server such as IIS, it could then send out a modified set of credentials.
so, i think for the first revision of digest auth support in mozilla, we should
simply land our implementation of RFC2617. later on we can add workarounds as
need be on a per server basis.
Assignee | ||
Comment 104•23 years ago
|
||
cleanup patch per comments from dougt:
1- canReuseCredentials -> areCredentialsReusable
2- extra -> meta data
3- remove printfs
4- do this the other way around, so that you only call Length once:
+ nsCRT::memcpy(contents, username.get(), username.Length());
+ len = username.Length();
Attachment #59269 -
Attachment is obsolete: true
Assignee | ||
Comment 105•23 years ago
|
||
Comment on attachment 59959 [details] [diff] [review]
v1.7
got an r=dougt on this.
Attachment #59959 -
Flags: review+
Comment 106•23 years ago
|
||
This patch looks good...
i did notice that in nsHTTPDigestAuth::CalculateHA1(...) 'contents' can leak if
MD5Hash(...) fails...
-- rick
Comment 107•23 years ago
|
||
Attachment #59959 -
Flags: superreview+
Assignee | ||
Comment 108•23 years ago
|
||
rick, thanks for the review... i'll fix that leak before checking in.
Assignee | ||
Comment 109•23 years ago
|
||
landed v1.7 patch on the trunk!
thanks to everyone who made this possible :)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 110•23 years ago
|
||
the problem was that for a proxy the "path" in nsHttpAuthEntry is null,
so => SEGV on strlen() in nsHttpAuthNode::GetAuthEntryForPath().
Note that from what I could tell, as of 200112090001 the quote handling
has not changed in squid, so I still used the hack I mentioned above.
I've also found another issue in squid's digest implementation: it uses
atoi() on the nonce count (see authDigestNonceIsValid()), which is actually
a hex string as specified in RFC 2617. So the result was that the first
10 requests will work fine, but the 11th will be rejected because of atoi().
Changing it to strtol(,,16) resolves the issue (I think).
However, while testing with squid, I found a problem regarding how the nonce
count is incremented in mozilla. I'll describe it in the next comment.
Comment 111•23 years ago
|
||
When using a proxy (squid in my tests), sometimes the nonce count values
in mozilla's requests are not sequential. Here is what I saw using
squid with digest auth (with the hacks mentioned above):
1. clear disk/mem cache, restart mozilla
2. go to kernel.org
=> 7 requests generated (user/password asked on the first one)
=> works fine, i.e. nc values sequential (e.g. 1 to 7)
3. restart mozilla
4. go to kernel.org
=> 2 requests ("/" and "/ubar/ubar.png")
=> nc values not sequential. if "/" has nc value X, then
"/ubar/ubar.png" always has nc (X + 2)
5. reload
=> 7 requests with sequential nc values, but the first one is
like (X + 7)
So it seems that GenerateCredentials is always invoked even if an object
is cached and no request is sent...
Note that the above is with the cache pref set to "Automatically".
If I set it to "Every time...", there are always 7 requests, so this is
not an issue (I guess).
Also note that if I go to cnn.com, the requests (~100) will have
non-sequential nc values, even if I clear disk/mem cache or set cache
pref to "Every time...". So there may be other additional/deeper issues...
Maybe this should be a new bug?
Comment 112•23 years ago
|
||
By the way, since the purpose of the nonce count is to prevent replay,
the current implementation is sufficient (if the problem I described above
is what I guess it is), as long as the server/proxy enforces a
"strictly-increasing" policy, instead of an "increment-by-one" policy.
Of course, we might be asking for too much...
Assignee | ||
Comment 113•23 years ago
|
||
An-Cheng: the crash was fixed the day after this patch landed. for the other
problem, how about filing a new bug? this bug is long enough ;-)
Comment 114•23 years ago
|
||
oops, haven't updated my tree for some time...
filed bug 114451 for the nonce count issue
You need to log in
before you can comment on or make changes to this bug.
Description
•